Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Use Cert Manager to Create Certificates for News Aggregator Server #15

Merged
merged 44 commits into from
Oct 2, 2024

Conversation

werniq
Copy link
Owner

@werniq werniq commented Aug 19, 2024

Introduce Certificate Generation using Helm Chart of Cert-Manager

Summary

This pull request introduces the implementation of certificate generation and management using Cert-Manager.
The main focus is on integrating certificate creation and retrieval within our news aggregator, using Kubernetes' Golang client to manage and apply the certificates dynamically when running the server.

Key Changes

  1. Certificate Resource Creation:

    • A new Certificate resource is created using Cert-Manager. This resource defines the specifications for the TLS certificate, such as the issuer, common name, and DNS names.
    • The Certificate resource is managed through Helm, ensuring consistent and reproducible deployments.
  2. Secret Management:

    • After the Certificate is issued, the certificate and private key are stored in a Kubernetes secret, identified by defaultSecretName.
    • The program retrieves the certificate and key from this secret using Kubernetes' Golang client.
  3. Certificate and Key Handling:

    • The retrieved certificate and key are stored as files with the names defaultCertName and defaultPrivateKey, they are retrieved from secret, generated by cert-manager.
    • We add this secret as additional Volume to the deployment
  4. Server Configuration:

    • The server is configured to use the paths to the certificate and key files when starting up.

@werniq werniq changed the title feat: Added dynamic Certificate And Private Key registration feat: Use Cert Manager to Create Certificates for News Aggregator Server Aug 19, 2024
Copy link
Collaborator

@VitaliyShevchenko VitaliyShevchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouln't this PR be pointed to a branch with helm ?

@werniq werniq changed the base branch from feature/migrate-to-taskfile to feature/helm September 23, 2024 06:06
@werniq
Copy link
Owner Author

werniq commented Sep 23, 2024

shouln't this PR be pointed to a branch with helm ?

Changed base branch

@@ -0,0 +1,10 @@
-----BEGIN CERTIFICATE-----
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so if you are using cert-manager there is no need to push certs anymore, they can be created dynamically using cert-manager
pls integrate that

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed certs from git

@@ -91,10 +112,69 @@ func ConfAndRun() error {

setupRoutes(server)

c := config.GetConfigOrDie()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

news-aggregator is just a pure https server, it should not care about certs
it should read certs from the file system
you can attach certs generated by k8s to the file system of news-aggregator

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

}

// loadCertsFromSecrets loads certificates from Kubernetes secrets
func loadCertsFromSecrets() (string, string, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls check my comment above, this should not be there

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

??
i think we get rid of makefile

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pulled data from prev PRs. Deleted

@@ -1,252 +1,158 @@
version: '3'

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why changed again ?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't changed, I've just have not pulled data from prev PR's. Resolved

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be under helm chart right?
+ integrate helm values

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to helm chart

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added values for issuer and secrets in helm

@@ -4,9 +4,3 @@ description: A Helm chart for Kubernetes
type: application
version: 0.1.0
appVersion: "1.16.0"
dependencies:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why removed?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong branch, added dependencies

- server auth
- client auth
dnsNames:
- "go-gator-server"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you have such dns name?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is creating without dnsNames, removed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want you to put a dns name which you use. not just removing it.
what dns name do you use?

@@ -0,0 +1,5 @@
apiVersion: v1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the secret will be created automatically by certificate CR (cert-manager)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But how do we assign it to the deployment? It will add random sequence of letters after cert name

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can specify secret name in the cert itself

Copy link
Owner Author

@werniq werniq Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to reproduce this without secret, but:
image

It is not creating the secret with exactly same name from values.

@@ -0,0 +1,17 @@
apiVersion: cert-manager.io/v1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you need that cert? how is this used? I see you have almost similar below.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

templates folder can be completely removed since you moved to helm charts

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in helm PR, to avoid duplication

@werniq
Copy link
Owner Author

werniq commented Sep 30, 2024

@VitaliyShevchenko

Working fine:
image

go.mod Outdated
@@ -8,41 +8,89 @@ require (
github.com/PuerkitoBio/goquery v1.9.2
github.com/gin-gonic/gin v1.10.0
github.com/jarcoal/httpmock v1.3.1
github.com/spf13/cobra v1.8.0
github.com/spf13/cobra v1.8.1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why changed?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure from where this changes are, but I will revert to previous version

@werniq werniq merged commit a0c0742 into feature/helm Oct 2, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants