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

Clarifying that wildcard hostname matching is suffix matching #1173

Merged

Conversation

robscott
Copy link
Member

What type of PR is this?
/kind documentation
/area conformance

What this PR does / why we need it:
This is a follow up from #1159 that clarifies that wildcard hostname matching should be considered equivalent to suffix matching. This is a change from the Ingress spec but is more widely implementable and matches the most widespread default behavior, including nginx, envoy, GCP, and Azure.

Which issue(s) this PR fixes:
Fixes #1159

Does this PR introduce a user-facing change?:

Wildcard hostname matching behavior has been documented as equivalent to suffix matching.

If merged, this will cause conformance tests to start failing for some implementations, adding a temporary hold so everyone has a chance to weigh in.

/hold

/cc @bowei @shaneutt @youngnick @mikemorris @howardjohn

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/documentation Categorizes issue or PR as related to documentation. labels May 26, 2022
@k8s-ci-robot k8s-ci-robot requested a review from bowei May 26, 2022 19:01
@k8s-ci-robot
Copy link
Contributor

@robscott: GitHub didn't allow me to request PR reviews from the following users: howardjohn, mikemorris.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

What type of PR is this?
/kind documentation
/area conformance

What this PR does / why we need it:
This is a follow up from #1159 that clarifies that wildcard hostname matching should be considered equivalent to suffix matching. This is a change from the Ingress spec but is more widely implementable and matches the most widespread default behavior, including nginx, envoy, GCP, and Azure.

Which issue(s) this PR fixes:
Fixes #1159

Does this PR introduce a user-facing change?:

Wildcard hostname matching behavior has been documented as equivalent to suffix matching.

If merged, this will cause conformance tests to start failing for some implementations, adding a temporary hold so everyone has a chance to weigh in.

/hold

/cc @bowei @shaneutt @youngnick @mikemorris @howardjohn

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.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 26, 2022
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 26, 2022
@youngnick
Copy link
Contributor

As I said in #1159, although I would slightly prefer to keep this similar to a single label, it's not worth excluding already-working implementations from conformance (which would be the cost of not making this change).

So, this LGTM to me, but interested to hear from others.

@howardjohn
Copy link
Contributor

I would slightly prefer to keep it a single label [and have someone make Envoy support single labels efficiently].

@robscott
Copy link
Member Author

robscott commented Jun 1, 2022

I would slightly prefer to keep it a single label

Although I agree that it would be nice to match the Ingress spec and more closely align with TLS certs, I think there's likely a compelling reason that so many APIs/implementations (envoy, nginx, GCP, Azure, etc) chose to interpret this as suffix matching. The fact that some of those can only implement suffix matching makes me think it's our only real option here.

Copy link
Member

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I don't have a problem with the approach. It's implementable and if it's the thing that everyone can easily implement then that's good.

I do have a small comment/question about the testing for this, but it's possible I'm just missing something.

Comment on lines +69 to +71
Request: http.ExpectedRequest{Host: "multiple.prefixes.bar.com", Path: "/"},
Backend: "infra-backend-v3",
Namespace: ns,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be reading this wrong: this test proves the change by making sure the * match works right? Are we missing the test for the assertion that the suffix without a prefix doesn't match?

e.g. we need these tests:

  • *.ocean.example.com matches whales.ocean.example.com and sharks.ocean.example.com
  • *.ocean.example.com does not match ocean.example.com

Copy link
Member Author

@robscott robscott Jun 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, added test case for that. On a related note - I'm a big fan of those example domains. Trying to keep the surface area of this PR small, but if anyone wants to move our tests to ocean themed example.com/net/org domains, that would also be great.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fits right in with our already nautical software themes 😂

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 2, 2022
@robscott
Copy link
Member Author

robscott commented Jun 3, 2022

Also ran this by @mikemorris to confirm this made sense for their implementation. I think there's enough consensus here to remove the hold - still need someone to add an LGTM though.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 3, 2022
@youngnick
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 6, 2022
@k8s-ci-robot k8s-ci-robot merged commit c5bca72 into kubernetes-sigs:master Jun 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/documentation Categorizes issue or PR as related to documentation. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wildcard hostname matching behavior is not clearly specified
5 participants