Skip to content
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

Wildcard hostname matching behavior is not clearly specified #1159

Closed
robscott opened this issue May 10, 2022 · 7 comments · Fixed by #1173
Closed

Wildcard hostname matching behavior is not clearly specified #1159

robscott opened this issue May 10, 2022 · 7 comments · Fixed by #1173
Labels
area/conformance kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API
Milestone

Comments

@robscott
Copy link
Member

What would you like to be added:
As @howardjohn noted in Slack, our hostname matching behavior is not clearly specified. Other than saying "exact matches must be processed before wildcard matches", we don't really cover how wildcard matches should be implemented. Specifically, which of the following should *.example.com match?

  1. a.example.com
  2. a.b.example.com

We recently added a conformance test that stated that only 1 should match. On further thought, I'm not sure that's what we want.

Here's my understanding of the default behavior for a variety of different products:

  1. nginx defaults to match 1 and 2 ref
  2. envoy defaults to match 1 and 2 ref
  3. haproxy can definitely support suffix matching (hdr_end), I'm not sure about the other ref
  4. GCP can only support suffix matching (not well documented yet)

Given the above, it seems that most underlying implementations treat wildcard host matching as suffix matching as the default, and some may only be able to support that approach. My suggestion would be to adjust the spec and conformance tests to reflect that.

Why this is needed:
This is an important gap in our spec today.

@robscott robscott added kind/feature Categorizes issue or PR as related to a new feature. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API area/conformance and removed kind/feature Categorizes issue or PR as related to a new feature. labels May 10, 2022
@howardjohn
Copy link
Contributor

One note in favor of the "1 label" behavior is that TLS verification behaves this way. So if we don't do that, we have inconsistency in certs and routing (which is ok, just inconsistent). I don' have a strong opinion one way or another though; as long as an implementation has regex header matching I think either approach can be implemented in most proxies?

@skriss
Copy link
Contributor

skriss commented May 10, 2022

xref #654 and #674 which also had discussion on this topic.

Also noting that matching a single label is consistent with the Ingress wildcard host spec: https://kubernetes.io/docs/concepts/services-networking/ingress/#hostname-wildcards

@robscott
Copy link
Member Author

In general, I think it's safe to describe the two options as "single label matching" and "multi label matching". Each have advantages:

Single Label Matching:

  • Consistent with Ingress spec
  • Consistent with TLS verification

Multi Label Matching:

  • Consistent with envoy, nginx, and GCP defaults
  • More broadly supportable

I'm biased here because GKE can only support the multi label matching approach here, so I would personally prefer that spec. It also seems like it would be more straightforward and performant for other implementations to support suffix matching than implementing regex matches. Want to see what others think though.

@jpeach
Copy link
Contributor

jpeach commented May 13, 2022

Multi label matching won't work with TLS and practically everything is deployed with TLS. Specifying multi label matching just seems likely to encourage broken configurations that are hard to debug.

@youngnick
Copy link
Contributor

I thought I had specified a single label match previously, so I'd kind of prefer that we keep that.

@robscott
Copy link
Member Author

I agree that this was at one point specified in the API, and I'm not actually sure how it ended up being removed from the spec - I think it must have been part of a larger restructuring or rewording. With that said, my strong bias here is to use the most broadly supportable interpretation of this, which is essentially a suffix match.

From what I can tell, both GCP and Azure are only able to interpret a wildcard match as a suffix match. That also matches the default behavior of nginx and envoy. Although this would differ from the Ingress spec, I think it would be more in line with how wildcard matches are interpreted across different providers.

Because this is a critical decision, I'm going to add it to the v0.5.0 milestone. I think there are two possible actions that could be sufficient to unblock the v0.5.0 for this:

  1. Remove the conformance tests that require single label matching as an interpretation of wildcard matching.
  2. Add guidance to the spec on how wildcard matching should be implemented.

I would prefer 2, but if we can't easily reach consensus on this, I think 1 may be sufficient.

@youngnick
Copy link
Contributor

To summarize what I said at the community meeting:

  • Keeping things consistent with Ingress would be preferable
  • But this is a core capability, and if some implementations can't implement it, we have a problem.
  • I'd rather loosen this and try to find a way for implementations to optionally tighten it than exclude already-working implementations from conformance because of this detail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/conformance kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants