Loosening TLS validation to enable indirect TLS config #2721
Conversation
k8s-ci-robot
added
release-note
|
/hold |
k8s-ci-robot
added
the
approved
k8s-ci-robot
added
do-not-merge/hold
|
@robscott: GitHub didn't allow me to request PR reviews from the following users: EItanya. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| hostnames. This would also likely benefit from a Listener or Gateway-level | ||
| opt-in for the behavior. |
There was a problem hiding this comment.
Would an option be a bad example of a way to offer opt-in behavior here? It would definitely be a bit of an abuse of the map[string]string structure, but would have the benefit of not requiring API changes.
For example:
example.com/tls-allowed-namespace-selector: foo=bar
There was a problem hiding this comment.
I'm generally against any form of structured content within these kind of annotation-like fields since it's so easy to mess up. Maybe I should add some guidelines for what doesn't belong here? Was that what you were suggesting here?
There was a problem hiding this comment.
worth updating this page with some content as well? https://gateway-api.sigs.k8s.io/guides/tls
at least alluding to the fact that some implementations may decide to implement downstream TLS, specifying certs, other options, etc. in different ways
it strikes me that making the tls section no longer required for some protocols, it could make the table here confusing: https://gateway-api.sigs.k8s.io/guides/tls/#clientserver-and-tls
robscott
force-pushed
the
self-service-tls
branch
from
01f7fe2 to
a169bb9
Compare
Good point, added a note to start that page off. |
apis/v1/gateway_types.go
Outdated
| // | ||
| // +kubebuilder:validation:XValidation:message="certificateRefs must be specified when TLSModeType is Terminate",rule="self.mode == 'Terminate' ? size(self.certificateRefs) > 0 : true" |
There was a problem hiding this comment.
Is entirely removing this validation necessary? Maybe, instead of removing, it could be changed to at least one of either certificateRefs or options must be specified when mode is Terminate?
There was a problem hiding this comment.
@robscott note that the comment on line 385 (below) also needs to be updated
There was a problem hiding this comment.
@robscott note that the comment on line 385 (below) also needs to be updated\
Thanks @frankbu! I updated the relevant comments in that area.
Is entirely removing this validation necessary? Maybe, instead of removing, it could be changed to at least one of either certificateRefs or options must be specified when mode is Terminate?
Unfortunately I don't think that would work for one of the initial user stories that led to this - specifically a user wanting certs to be added automatically after they created a Gateway (#763 (comment)). I think the most likely way that would work would involve some kind of controller that automatically generates and populates certificates for Gateways in a given set of namespaces or maybe with a predefined label or annotation. I'm assuming that in most cases, this kind of config would apply to an entire Gateway, not just a listener, so TLS options wouldn't really fit.
There was a problem hiding this comment.
I was thinking that case could be handled by not having any user GatewayTLSConfig setting at all. In that case, the validation only applies when the user is explicitly setting the mode to Terminate using either certificateRefs or some other impl-specific option.
My concern is that if we remove all validation now, we won't be able to add it later. Alternatively, we can always take away more validation in the future without breaking anybody.
There was a problem hiding this comment.
Chatted on Slack about this and although there are some use cases that could benefit from setting mode and nothing else, they're not sufficient to justify dropping this validation altogether at this point. If we need to, we can revisit this in the future, but we can never retroactively tighten validation. Have updated this to require "options" or "certificateRefs" when mode is "Terminate" as @frankbu suggested.
robscott
force-pushed
the
self-service-tls
branch
from
a169bb9 to
5f4f16a
Compare
robscott
force-pushed
the
self-service-tls
branch
from
5f4f16a to
ab303cb
Compare
robscott
force-pushed
the
self-service-tls
branch
from
ab303cb to
097d08a
Compare
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: frankbu, robscott, shaneutt The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
What type of PR is this?
/kind documentation
/kind feature
What this PR does / why we need it:
This loosens TLS validation as requested by #2668 and #2713 and adds documentation for the self-service approaches and overall indirect TLS config this enables. This also adds a new
InvalidTLSConfigreason for theProgrammedcondition on Gateways.Which issue(s) this PR fixes:
Fixes #763
Fixes #2668
Fixes #2713
Does this PR introduce a user-facing change?:
This is a big PR, going to leave it on hold for a bit while we wait for consensus.