-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
add upstream TLS trust from CM bundles #14717
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
6bdacfb
Add upstream TLS trust from CM bundles
ReToCode 43b9e6f
Debug activator not starting
ReToCode 2fa4dc8
Make tests run in sequence as they use a shared state
ReToCode c13c7f9
Update tests to loop with new state per test
ReToCode b30b63d
Use namespaced configmap informer
ReToCode d4c28a4
Drop debug logging
ReToCode File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Curious - I'm wondering if it's simpler to just watch (or poll) a certain directory for certs instead of using informers here.
Then in theory we wouldn't necessarily be tied to 'trust-manager` and could swap to use k8s cluster trust bundles (ref).
It's going beta in 1.30
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.
We could, we do it in Queue-Proxy already. Having multiple config-maps is not tying only to trust-manager.We also have downstream solutions with the same pattern (https://github.com/ReToCode/knative-encryption/tree/main/8-trust-sources), so we'll have multiple ConfigMaps to look at.
If we do it via filesystem, we'd need a way to configure multiple mount-configs somehow. But I'm up for looking at this again once cluster trust bundles landed (we'd also need support for this downstream).
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.
I think it might be simple enough for operators to add a projected volume mount (ref) coalescing all the CA's into the same volume mount. If we use the standard
SSL_CERT_DIR
then I think the activator might simply need to callx509.SystemCertPool()
every few minutes (https://pkg.go.dev/crypto/x509#SystemCertPool) to get updates.One thing to verify is if projected volumes receive updates when config maps change - if they don't we should file a k8s bug.
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.
I think that would work. But how would a user define the volume-mounts? He'd need to customize our yamls on installation (and on every update) to add the volume mounts. Also operator would probably need some form of extra-mounts attributes for all containers. Isn't that more complicated that what we have in this PR?
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.
We require operators do this for tag-to-digest resolution - https://knative.dev/docs/serving/tag-resolution/#custom-certificates
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.
In this PR we also trust the CA in a Secret (if present), I think it's a good thing to make the "easy" setup work without additional configuration. That certificate will not be in the system pool by default, so users always would need to give Activator the CA in that case. So I'm not (yet) really convinced that is good UX for our users.
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.
Shouldn't we just add a projected volume to the activator and add that ca secret as an file entry?
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 CA is not always populated, also what component would manage that volume in the YAML? The secret only exists when encryption is enabled and cert-manager is deployed. I'd assume the deployment would fail if no Secret exists. Even when it exists, the CA can be empty depending on the cert-manager issuer type (only CA and self-signed populate the
ca.crt
field). So we'd mount an empty file.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.
I did think about this a bit more and have another reason to keep this PR. This affects not just serving but also the net-* implementations. With the current solution, a user just provides the CM (ideally also using trust-manager to distribute it in multiple namespaces) and it works directly. No changes necessary to the YAML (that one has to do on every upgrade), no further changes on the operator and the KnativeServing CR.
With the mount approach, a user would need to customize the volumes of controller and net-* controller for now. We might at some point add mTLS, then that would also be needed on every KnativeService in the QP manifest. So I'm not sure this is really a good alternative as it has way worse UX.
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.
I'm still mixed on this - cause I'm seeing two technologies on 'how do we distribute certificates' and ideally it would be great if we didn't overfit towards one solution. Informing on secrets/certificates seems like we're overfitting for trust-manager. Projected volumes seems like it would be the most flexible since you can combine both certificates from
trust-manager
and ClusterTrustBundles.ClustersTrustBundles are right around the corner (beta in K8s 1.30 - April). And they only work with projected volumes - so I think I'd attempt to future proof this design.
I think an in-tree solution (ClusterTrustBundles) will be more popular than an external one (trust-manager).
Mounting an empty file should be fine - we can ignore empty files when we refresh the certificates.
I don't think so - we could just have projected volumes as part of our controller/net-* default deployment.
In the future if a user wants to use ClusterTrustBundles they can patch this project volume to include it.
I don't understand concern your'e trying to surface here - we already use volumes for certificates in the queue proxy here
serving/pkg/reconciler/revision/resources/deploy.go
Lines 196 to 199 in cb84c77