-
Notifications
You must be signed in to change notification settings - Fork 512
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
[kind/api-change] Allow wildcard suffix in hostnames #3644
base: main
Are you sure you want to change the base?
[kind/api-change] Allow wildcard suffix in hostnames #3644
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: simonfelding The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Welcome @simonfelding! |
Hi @simonfelding. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
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 this needs a lot more discussion around interactions with RFCs, implementation, and between objects.
For example, what is the semantics for matching between a listener hostname suffix match with a HTTPRoute hostname prefix match?
Which implementations can support this today? I am fairly sure envoy does not support suffix SNI matching.
This also has tricky impacts on certs which only support a prefix wildcard.
Further, most suffix match use cases I would imagine are a string suffix match. All wildcards in the API today are a single label wildcard match. So grafana.*
would not match grafana.example.com
which probably makes the use case not met.
@howardjohn Totally agree it needs careful review and discussion of interactions! I haven't been able to think of any that aren't already supported in the major implementations, nginx for example, but there might be something I can't come up with. Envoy also supports it and it sounds like the developers have already thought these issues through: (Wildcard hosts are supported in the suffix or prefix form).
I don't think it has any interaction with certs? It's true that certs only match on a single wildcard subdomain; The gateway API already violates this by matching Let's say a server has two virtual hosts, grafana.example.com and grafana.example.org. When a HTTPS request comes in, the client_hello stage is still unencrypted and contains the requested hostname. |
Envoy supports it for HTTP Host headers, but the API you are changing here impacts SNI where its not supported: https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/listener/v3/listener_components.proto#config-listener-v3-filterchainmatch |
It's not in the given examples in that document, but I don't see why it wouldn't work? SNI is just a simple string extracted from the client_hello. I'm not deep enough in the envoy code to be sure, but from a superficial look at the code I can't see any validation that treats the SNI as a special type of hostname string that cannot be wildcard-matched like other hostnames can.
The name is determined in the following order:
I also know for sure that the Citrix Netscaler has no problem treating the client hello SNI string as any other string and making complex routing decisions based on that - prior to serving a cert. tl;dr: SNI just means sending the hostname before the cert is served, which makes routing a TLS connected work like unencrypted HTTP. After hostname is matched with a server (by SNI or by Host header), a cert is served. I'm pretty confident this GEP does not impact SNI - it's only about whether or not we can allow a Gateway implementation to match a hostname to a string ending with a wildcard :) |
Its changing the valid set of matchers against SNI so it is. |
Dang, nice find. Youre right, Envoy does not support it right now because of that line. Deserves a PR to that as well :) But anyway, why should the Gateway API decide whether or not the implementation can get the wildcard-suffixed hostname? I think it's pretty clear that nginx supports it for example, and it looks like to me that it's just a matter of a submitting a minor patch to make envoy support it too. |
What type of PR is this?
/kind gep
What this PR does / why we need it:
It adds support for wildcard suffixes in addition to wildcard prefixes in the Hostname type.
It is fully backwards-compatible.
Which issue(s) this PR fixes:
Fixes #3643
Does this PR introduce a user-facing change?: