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

Use one secret for TLS certificates #292

Merged
merged 1 commit into from
Oct 30, 2024
Merged

Use one secret for TLS certificates #292

merged 1 commit into from
Oct 30, 2024

Conversation

keyvaann
Copy link
Collaborator

This was changed a year ago in this commit
fb46962
But it turns out that now we have get rate limited by Let's Encrypt when requesting for a separate certificate for each application. This PR fixed this.

Copy link

github-actions bot commented Oct 28, 2024

Great PR! Please pay attention to the following items before merging:

Files matching charts/*/values.yaml:

  • Is the PR adding a new container? Please reviewer, add it to the models (internal process)
  • Is the PR adding a new parameter? Please, ensure it’s documented in the README.md

This is an automatically generated QA checklist based on modified files.

Copy link
Collaborator

@pvannierop pvannierop left a comment

Choose a reason for hiding this comment

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

I think the following tls secrets also need to be updated:

  • data dashboard backend
  • prometheus-alertmanager
  • prometheus-grafana
  • prometheus-prometheus

We set new secret names for services on a different sub-domain in helmfile layer. So, we can use the same default secret name in the helm charts.

@keyvaann
Copy link
Collaborator Author

Apparently this shouldn't be done. From:

Generate multiple certificates with multiple ingresses
If you need to generate certificates from multiple ingresses make sure it has the issuer annotation. Besides the annotation, it is necessary that each ingress possess a unique tls.secretName

https://cert-manager.io/docs/usage/ingress/#generate-multiple-certificates-with-multiple-ingresses

@pvannierop
Copy link
Collaborator

@keyvaann My point is that this is not the concern of the helm chart. The helmfile layer knows about the final domain a service runs on and therefore is also responsible for accommodating the requirement in the cert-manager docs i.e. make sure that when it runs on a domain that requires a different tls secret a new secrete name is set in accordance. I will approve; at least we have had this discussion and indeed it will not matter much for running the platform.

@pvannierop pvannierop merged commit c4b9890 into main Oct 30, 2024
5 checks passed
@keyvaann keyvaann deleted the one-secret-for-tls branch October 31, 2024 08:53
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