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

Introduce the support for specifying a CA bundle in the helm chart #24862

Merged
merged 2 commits into from Apr 28, 2023

Conversation

giorio94
Copy link
Member

This PR extends the helm chart to introduce the support for the specification of a CA trust bundle used for the validation of the certificates leveraged by hubble and clustermesh. If enabled, it overrides the content of the ca.crt field of the respective certificates, allowing for CA rotation with no down-time. The bundle can be either provided out-of-band, or created through Helm.

Introduce the support for specifying a CA bundle in the helm chart

@giorio94 giorio94 added kind/enhancement This would improve or streamline existing functionality. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/helm Impacts helm charts and user deployment experience labels Apr 13, 2023
@giorio94 giorio94 requested review from a team as code owners April 13, 2023 09:16
@giorio94 giorio94 requested a review from a team as a code owner April 13, 2023 09:22
@giorio94
Copy link
Member Author

ConformanceK8sKind hit unrelated flake: #24863

# -- Configure the CA trust bundle used for the validation of the certificates
# leveraged by hubble and clustermesh. When enabled, it overrides the content of the
# 'ca.crt' field of the respective certificates, allowing for CA rotation with no down-time.
caBundle:
Copy link
Contributor

@chancez chancez Apr 14, 2023

Choose a reason for hiding this comment

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

Could this be part of the ca field above instead? I understand the above is for a CA key pair but these do have some overlap. Not sure how it should look though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had the same doubt when adding this field. Personally, I feel it is a bit better to keep them disjoint to highlight that they are not strictly related, since you might want to specify the bundle but not the CA info (e.g., when using cert-manager). I don't have a strong opinion though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I don't feel too strongly, but I worry having two different sections might be confusing. Not sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

what happens when both ca and caBundle are specified?

Copy link
Member Author

Choose a reason for hiding this comment

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

These two fields are independent. Essentially, ca is used to specify the CA for certificate generation through helm or the cronJob, while the caBundle, if set, overrides the ca.crt entry part of the certificate secret (which would be ca.tls if generated through helm/cronJob, but may be different), and it represents the set of trusted root certificates for the different cilium components. It mainly enables seamless CA rotation, since you can specify multiple trusted certificates at the same time.

For instance, you would configure caBundle only when relying on cert-manager to generate the certificates, ca only if you are only interested in certificate generation through helm/cronJob, or both in case you want both certificate generation and the support for CA rotation with no downtime.

Copy link
Contributor

@zacharysarah zacharysarah left a comment

Choose a reason for hiding this comment

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

@giorio94 LGTM from a docs perspective.

@giorio94
Copy link
Member Author

/test

This commit adds a note to the helm chart to clarify that the CA
configuration does not take effect when using cert-manager to generate
the certificates.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
This commit extends the helm chart to introduce the support for the
specification of a CA trust bundle used for the validation of the
certificates leveraged by hubble and clustermesh. If enabled, it
overrides the content of the `ca.crt` field of the respective
certificates, allowing for CA rotation with no down-time. The bundle
can be either provided out-of-band, or created through Helm.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94
Copy link
Member Author

Rebased onto main to fix test failures due to mismatches between code and workflows.

@giorio94
Copy link
Member Author

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 26, 2023
@giorio94
Copy link
Member Author

giorio94 commented Apr 28, 2023

/test

Rerunning the tests, since the required list changed in the meanwhile.

@giorio94
Copy link
Member Author

giorio94 commented Apr 28, 2023

/ci-datapath

A few matrix entries failed while creating the LVH VMs. Rerunning

@giorio94 giorio94 removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 28, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 28, 2023
@pchaigno
Copy link
Member

/ci-datapath

A few matrix entries failed while creating the LVH VMs. Rerunning

I'd be good to point to the flake issues or to create them in such cases. Otherwise it's hard for the CI Health person to learn about these.

@pchaigno pchaigno merged commit 95ff606 into cilium:main Apr 28, 2023
59 of 60 checks passed
@giorio94
Copy link
Member Author

I'd be good to point to the flake issues or to create them in such cases. Otherwise it's hard for the CI Health person to learn about these.

Does it make sense to create an issue also for things like temporary apt failures? I don't think there would be much the CI Health person could do for that.

@giorio94 giorio94 deleted the mio/ca-bundle-support branch April 28, 2023 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Impacts helm charts and user deployment experience kind/enhancement This would improve or streamline existing functionality. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants