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

issue self signed certificate #316

Merged
merged 2 commits into from
Jun 21, 2024
Merged

Conversation

nabuskey
Copy link
Collaborator

fixes: #137
related to: #300 #293

With this PR, idpbuilder will:

  1. Create a self signed certificate.
  2. Create a TLS secret in the ingress-nginx NS.
  3. Use it as the default TLS certificate for ingress-nginx. (you can still use other certs configured at ingress level)
  4. Update ArgoCD's CM to trust the CA.
  5. Create a CM that contains cert to the default NS.

I thought about using cert-manager but decided not to use it. For our purposes, we just need a certificate for ingress-nginx for in-cluster and incoming traffic only. Introducing cert-manager means:

  1. We need to maintain cert-manager manifests.
  2. Slower installation speed. We now need to wait for cert-manager to do its thing and be ready before anything else can continue.
  3. Introduces three pods that will do nothing for the most part.

hack/argo-cd/argocd-tls-certs-cm.yaml.tmpl Show resolved Hide resolved
pkg/build/tls.go Outdated Show resolved Hide resolved
Copy link
Contributor

@nimakaviani nimakaviani left a comment

Choose a reason for hiding this comment

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

Thanks for this. made a few minor comments. otherwise looks good.

I hear your points on the use of self-signed certs and it makes sense in the context of idpBuidler alone. However, if we try to move towards an AppSet strategy that also happens to apply to external clusters, then I wonder whether the idea of using CertManager has a stronger ground.

If users will have cert-manager installed to their clusters by default, and if the idpBuilder is supposed to pave the path for them to eventually transitioin from a dev / test environment to a prod env, shall we just bite the bullet and pay the extra cost of enabling them with the cert-manager in the test environment too?

that said, given the amount of work put into this, this works as an interim solution. But lets revisit this as we move towards expanding on the deployment strategy.

pkg/build/tls.go Outdated Show resolved Hide resolved
pkg/build/tls.go Show resolved Hide resolved
pkg/cmd/create/root.go Show resolved Hide resolved
pkg/build/tls.go Outdated Show resolved Hide resolved
nabuskey added 2 commits June 20, 2024 21:46
Signed-off-by: Manabu McCloskey <[email protected]>
Signed-off-by: Manabu McCloskey <[email protected]>
@nabuskey
Copy link
Collaborator Author

I think I'd rather wait for concrete use cases for cert-manager until we pull it into core. Ready for another round of review.

@jessesanford
Copy link
Contributor

I agree on waiting for more use cases before bringing in cert manager. It should be reasonably easy to roll forward to it when the time comes.

@cmoulliard
Copy link
Contributor

I think I'd rather wait for concrete use cases for cert-manager until we pull it into core. Ready for another round of review.

Cert Manager can help to deal with many use cases like:

@cmoulliard cmoulliard self-requested a review June 21, 2024 06:44
Copy link
Contributor

@nimakaviani nimakaviani left a comment

Choose a reason for hiding this comment

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

LGTM. thanks!

@nimakaviani nimakaviani merged commit ee78c16 into cnoe-io:main Jun 21, 2024
3 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.

Ingress-nginx certificate handling
4 participants