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
Clarifying TLS Spec on HTTPRoute, Moving to Extended Support #739
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: robscott 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 |
// oldest resource wins. | ||
// Attaching a TLS certificate to HTTPRoutes provides a way for HTTPRoute | ||
// owners to effectively populate certificates on Gateway Listeners. Note | ||
// that only one certificate may be attached to a Gateway Listener for a |
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.
do we need to talk about how wildcards are handled?
If I understand correctly, support levels have been mostly been decided based on implementation feasibility and not on reservations on whether something is good or not. We can clarify if the spec seems ambiguous or revisit this part of the API entirely if need be. |
// Support: Core | ||
// * The oldest Route based on creation timestamp. For example, a Route with | ||
// a creation timestamp of "2021-07-28 01:02:03" is given precedence over | ||
// a Route with a creation timestamp of "2021-07-28 01:02:04". |
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 apply latest timestamp win previous timestamp, which is similar to overwrite.
Do I miss any restrict/rules somewhere ?
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.
https://gateway-api.sigs.k8s.io/concepts/guidelines/#conflicts
Oldest-wins is least likely to break existing configurations.
// Attaching a TLS certificate to HTTPRoutes provides a way for HTTPRoute | ||
// owners to effectively populate certificates on Gateway Listeners. Note | ||
// that only one certificate may be attached to a Gateway Listener for a | ||
// specified hostname. It is not possible to use different certificates for |
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.
Only one secret can be attached to a listener, but we don't specify what is in that secret. You could have a whole pile of certificates if you want :)
// match multiple routes (in case multiple HTTPRoutes share the same | ||
// hostname). | ||
// If multiple HTTPRoutes define a TLS certificate for the same hostname and | ||
// are bound to the same Gateway Listener, precedence MUST be determined in |
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.
Are you are referring to the precedence of the certificate here?
So in this case
- all the routes are valid
- only one of the matching certificates is configured on the proxy
- there's no update to the route's status conditions
Adding a hold on this while we discuss #749. |
/hold |
With #749 merged in, this PR is irrelevant. /close |
@robscott: Closed this PR. 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. |
What type of PR is this?
/kind cleanup
/kind documentation
What this PR does / why we need it:
This PR clarifies the TLS spec on HTTPRoute and moves it to extended support. This has been one of the more confusing parts of the API, and it seemed like an update to the spec around it could help. I think there are at least some implementations that have reservations around supporting this feature, so I'm also proposing dropping the support level to "Extended" here.
Does this PR introduce a user-facing change?: