conformance: add TLSRoute hostname intersection conformance tests#4437
Conversation
|
Skipping CI for Draft Pull Request. |
98f0b01 to
430dfa9
Compare
430dfa9 to
5410755
Compare
|
/cc @rikatz @Thealisyed |
|
@rostislavbobo: GitHub didn't allow me to request PR reviews from the following users: Thealisyed. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn 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-sigs/prow repository. |
|
/assign |
| apiVersion: gateway.networking.k8s.io/v1 | ||
| kind: Gateway | ||
| metadata: | ||
| name: gateway-exact-hostname |
There was a problem hiding this comment.
given this is a tlsroute test, can we have the name of the gateway containing it to differentiate from other tests?
There was a problem hiding this comment.
also question: what would be the impact of having all listeners on the same gateway, and attaching to the listener using sectionName?
There was a problem hiding this comment.
given this is a tlsroute test, can we have the name of the gateway containing it to differentiate from other tests?
Added "-tlsroute and "-intersection" to the Gateway names.
There was a problem hiding this comment.
what would be the impact of having all listeners on the same gateway, and attaching to the listener using sectionName?
This is possible, but it requires adding more TLS-terminating backends to be able to trace the request till it hits a distinct backend. Currently, the "gateway-conformance-infra" namespace has only one "tls_backend" Service and Deployment (see source).
We need to decide between
a) Multiple Gateways – create more Gateways in conformance/tests/*.yaml that use the single "tls_backend", or
b) Multiple Backends – add more "tls_backend_{N}" Services and Deployments to conformance/base/manifests.yaml and use a single Gateway with multiple listeners in conformance/tests/*.yaml
There was a problem hiding this comment.
ok I see your point, yes. We won't be able to assert which listener was used.
I think we can consider multiple backends as some part of the evolution, but looks good to me for now. I will approve and let the lgtm for someone else
|
@rostislavbobo two asks/questions, otherwise lgtm! |
|
/approve I will check with someone else for a 2nd pass |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rikatz, rostislavbobo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
I have tested with envoy gateway and cilium and it works fine. /lgtm Thanks! |
IMO if we have a test for TLS it should not be testing "too long hostname". The spec even says to copy the name: https://gateway-api.sigs.k8s.io/geps/gep-1762/?h=cl#automated-deployments |
It is not, it is testing within the boundaries of 62 characters for the Gateway name (not about hostname). But once Istio copies the labels to the deployment spec, it adds an "istio" suffix and this violates the size. |
The spec says an implementation should do this. A TLS test does not require 62 character Gateway name. If we want to enforce this for conformance, we should change the spec. We cannot have a conformance test that requires you to violate the spec... |
OK, I see your point, the issue here is that we enforce that the generated label is - and this test "enforces" the worst case. I have missed that. @rostislavbobo can you reduce the size name for now? @howardjohn while we do not explicitly define a limit for a Gateway name, I think we may need to make it clear that a user creating a gateway with a big name will cause problems once those names are concatenated. We should probably enforce that in this case a better condition is exposed on Gateway status to let users now that the name is too big (a new conformance, a new specification, etc). |
|
Yeah I definitely agree some tweaks are needed around how to handle large names - just that we should do that in a dedicated manner instead of tying it to TLSRoute 🙂 . Thanks! |
|
makes sense @howardjohn , will reduce the name sizes, we can address it separately, no need to be part of TLSRoutes |
|
Created the issue #4464 for a followup on gateway name size. Thanks folks |
|
/lgtm |
@youngnick , I've added more test cases to cover, here's a quick summary:
Here you can see we do support when both are wildcard hostnames (even for No matching hostnames are covered by #4433 |
|
/retest |
| - name: gw-tlsroute-more-specific-wc-hostname-x-2 | ||
| namespace: gateway-conformance-infra | ||
| hostnames: | ||
| - "*.com" |
There was a problem hiding this comment.
@rostislavbobo I think this should not pass? the spec of gw-tlsroute-more-specific-wc-hostname-x-2 says: hostname: "*.example.com" so should trying to attach a *.com work?
There was a problem hiding this comment.
it passes for istio, fails for eg
There was a problem hiding this comment.
I have some split feelings. Let me think loud here
- the hostname on listener is used for infrastructure configuration and routing. It means "what listener hostname can attach here"
- the hostname on route is used for proper routing, like "anything that is *.com should route here" and it may rely on the parentRef for the right routing
I think the test makes sense given the parentRef case, let's see how this goes during implementations running their conformance
There was a problem hiding this comment.
Same with HTTPRoutes: A Gateway { hostname: "*.example.com" } <-- HTTPRoute { hostnames: ["*.com"] } is supported by istio and gke-l7-*, but is not supported by eg.
@robscott , @youngnick , the GW spec doesn't clearly state how to handle wildcard-to-wildcard attachments. I can
a) add this test for HTTPRoute, or
b) remove this test from TLSRoute
so that it's consistent across both.
WDYT?
There was a problem hiding this comment.
The thing you're missing here @rikatz is that * only means a single label. So *.com will only match example.com, not www.example.com.
There was a problem hiding this comment.
I do not recall when we added the stuff about the suffix match, but I do recall us agreeing that * only matched a single label. Let me look at the blame for those files and see if I can find some context, as that's not how I remember things working.
There was a problem hiding this comment.
Okay, turns out I am wrong, I guess, sigh, as of #1173, years ago. So yes, @rostislavbobo, you're right here. That is how the wildcard matching works, so it makes sense to test it.
I suspect I've written Cilium's implementation incorrectly and I'll fail this test, but that's on me.
There was a problem hiding this comment.
Oh, one thing that is not the same as RFC-2818 is that * only matches on a full label (or multiple full labels). No partial matches, so f*.example.com is not valid and does not match anything. This is recorded on the Hostname type:
gateway-api/apis/v1/shared_types.go
Lines 601 to 610 in 716f2c5
But the "must match a full label or multiple labels" is implied rather than fully specified. I'll make a note to fix this.
There was a problem hiding this comment.
yes, * matches full label(s) only. I don't recall any implementation supporting partial domain matches with * (e.g. f*.example.com) for SNI hostname or L7 Host.
There was a problem hiding this comment.
I’ll also create a similar test for HTTPRoute (*.example.com and *.com intersection) since we keep that test for TLSRoute, and invite you all to the review.
|
/lgtm |
…bernetes-sigs#4437) * conformance: add TLSRoute hostname intersection conformance tests * TLSRoute: Indicate tlsroute in Gateway names * TLSRoute: Add test for wildcard hostname intersects with wildcard listener hostname * TLSRoute: Fetch the Secret once and reuse it * TLSRoute: Add more conformance tests to cover more intersection cases * TLSRoute: Make TLS request tests run in parallel
Gateway API uses distinct wildcard hostname match on HTTP and TLS: * Host header and HTTPRoute hostnames matching understand a wildcard as one or more DNS labels, so the wildcard hostname is used as a suffix; * TLS SNI extension and TLSRoute hostnames matching understand a wildcard as a single DNS label, following RFC-2818. Context: https://github.com/kubernetes-sigs/gateway-api/blob/8f9b904306371cbfed9f9d542e9af667de804286/site-src/concepts/hostnames.md?plain=1#L142-L145 kubernetes-sigs/gateway-api#4437 (comment) TLSRoute was not following this configuration, but instead the suffix approach of the HTTPRoute, making one of the conformance tests to fail. This update configures ExtendedWildcard to false, so HAProxy configures the TLS/SNI match using single DNS label instead.
Gateway API uses distinct wildcard hostname match on HTTP and TLS: * Host header and HTTPRoute hostnames matching understand a wildcard as one or more DNS labels, so the wildcard hostname is used as a suffix; * TLS SNI extension and TLSRoute hostnames matching understand a wildcard as a single DNS label, following RFC-2818. Context: https://github.com/kubernetes-sigs/gateway-api/blob/8f9b904306371cbfed9f9d542e9af667de804286/site-src/concepts/hostnames.md?plain=1#L142-L145 kubernetes-sigs/gateway-api#4437 (comment) TLSRoute was not following this configuration, but instead the suffix approach of the HTTPRoute, making one of the conformance tests to fail. This update configures ExtendedWildcard to false, so HAProxy configures the TLS/SNI match using single DNS label instead.
What type of PR is this?
/kind test
/area conformance-test
What this PR does / why we need it:
This PR adds a conformance test for Gateway and TLSRoute hostnames intersection using wildcards (GEP-2643).
abc.example.com*.example.com*.example.comabc.example.com*.com*.comabc.example.com*.example.comabc.example.com*.comNote: TLSRoute rejection based on hostname mismatch is covered by #4433
Which issue(s) this PR fixes:
Fixes #
Part of #1579
Does this PR introduce a user-facing change?: