-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
helm: mandate issuer configuration when using cert-manager to generate certificates #24666
Conversation
This reverts commit bc2ed14. Currently, in the helm chart, if the cert-manager approach is selected to generate the hubble and clustermesh certificates but no issuer is specified, a new issuer is created for each of them, along with a secret containing the CA information. Still, this approach is currently broken, since the CA secret which is created does not match the format expected by cert-manager. At the same time, this might also hide misconfigurations (e.g., if there is a typo in the issuer configuration) and possibly lead to different CAs for different components. Hence, let's just stick to the approach documented in the user guide and make it mandatory to specify the issuer when cert-manager is used. It is a task of the users (as unrelated from cilium) to create the appropriate issuer in advance, according to their own preference. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
This reverts commit 082fa15. Currently, in the helm chart, if the cert-manager approach is selected to generate the hubble and clustermesh certificates but no issuer is specified, a new issuer is created for each of them, along with a secret containing the CA information. Still, this approach is currently broken, since the CA secret which is created does not match the format expected by cert-manager. At the same time, this might also hide misconfigurations (e.g., if there is a typo in the issuer configuration) and possibly lead to different CAs for different components. Hence, let's just stick to the approach documented in the user guide and make it mandatory to specify the issuer when cert-manager is used. It is a task of the users (as unrelated from cilium) to create the appropriate issuer in advance, according to their own preference. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
35dac97
to
d84bc71
Compare
I'm not running the additional tests, since they would provide no additional coverage for this helm-related change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes are looking good to me ✔️, only one question on upgrade path.
install/kubernetes/cilium/templates/hubble/tls-certmanager/server-secret.yaml
Show resolved
Hide resolved
install/kubernetes/cilium/templates/clustermesh-apiserver/tls-certmanager/server-secret.yaml
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @giorio94!
Marking as ready to merge without running the additional tests, as this includes helm related changes and they would provide no additional coverage. |
Currently, in the helm chart, if the cert-manager approach is selected to generate the hubble and clustermesh certificates but no issuer is specified, a new issuer is created for each of them, along with a secret containing the CA information. Still, this approach is currently broken, since the CA secret which is created does not match the format expected by cert-manager. At the same time, this might also hide misconfigurations (e.g., if there is a typo in the issuer configuration) and possibly lead to different CAs for different components. Hence, let's just stick to the approach documented in the user guide and make it mandatory to specify the issuer when cert-manager is used. It is a task of the users (as unrelated from cilium) to create the appropriate issuer in advance, according to their own preference.
This PR supersedes #24505, since the automatic creation of a per-component self-signed issuer would lead to certificates signed by different authorities, which is something we are trying to move away (https://github.com/cilium/cilium/blob/master/install/kubernetes/cilium/values.yaml#L1000-L1002). Using a well-defined CA, shared among all clusters to be joined with clustermesh is also the best practice for a smooth setup, and we should encourage it (https://docs.cilium.io/en/v1.13/network/clustermesh/clustermesh/#shared-certificate-authority).
I'm marking this as a bug fix, since selecting cert-manager and leaving the issuer unset currently leads to a broken setup (since the certificates are never generated due to the incorrect secret content).
I'm marking this for backport to v1.12 and v1.13 since the reverted PR (#22921) which removed the validation has been backported to those versions.
Fixes: #24500