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

gateway-api: Ensure hostname check when set on both the HTTPRoute and the Gateway Listener #30686

Merged
merged 1 commit into from Mar 10, 2024

Conversation

cjvirtucio87
Copy link
Contributor

@cjvirtucio87 cjvirtucio87 commented Feb 8, 2024

Description

This PR ensures that the gateway_checks code for the operator respects the hostname field, when it is set on both the HTTPRoute and the Gateway listener. Per the text of kubectl explain gateway.spec.listeners.hostname:

     For HTTPRoute and TLSRoute resources, there is an interaction with the
    `spec.hostnames` array. When both listener and route specify hostnames,
    there MUST be an intersection between the values for a Route to be accepted.

Contribution Text

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Are you a user of Cilium? Please add yourself to the Users doc
  • Thanks for contributing!

Fixes: #30685

gateway-api: Ensure hostname check when set on both the HTTPRoute and the Gateway Listener

@cjvirtucio87 cjvirtucio87 requested a review from a team as a code owner February 8, 2024 21:26
@maintainer-s-little-helper
Copy link

Commit 72eb9e8 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Feb 8, 2024
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Feb 8, 2024
@cjvirtucio87 cjvirtucio87 changed the title hostnames for httproute and gateway must be respected Ensure that the gateway_checks on the operator respects the hostname field when set on both the HTTPRoute and the Gateway Listener Feb 8, 2024
@maintainer-s-little-helper
Copy link

Commits 72eb9e8, 9813250 do not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper
Copy link

Commits 72eb9e8, 9813250, df2aa76 do not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Feb 11, 2024
@youngnick youngnick added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Feb 14, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 14, 2024
@maintainer-s-little-helper
Copy link

Commit fef4274 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Feb 17, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Feb 17, 2024
@cjvirtucio87 cjvirtucio87 force-pushed the hostnames-listeners branch 2 times, most recently from ce60900 to 86bed84 Compare February 17, 2024 16:43
@sayboras sayboras self-requested a review February 19, 2024 09:13
@cjvirtucio87 cjvirtucio87 force-pushed the hostnames-listeners branch 3 times, most recently from 96898af to 6be3c4e Compare February 19, 2024 15:09
@sayboras
Copy link
Member

Thanks for your PR, I vaguely remembered that we have similar checks, let me double-check and get back to you.

@sayboras
Copy link
Member

👋

We are having the below check to compute the overlapping host name, so I think better to update this function if you see there is any gap/discrepancy. Thanks

https://github.com/cilium/cilium/blob/main/operator/pkg/gateway-api/routechecks/gateway_checks.go#L146

@cjvirtucio87
Copy link
Contributor Author

cjvirtucio87 commented Feb 21, 2024

👋

We are having the below check to compute the overlapping host name, so I think better to update this function if you see there is any gap/discrepancy. Thanks

https://github.com/cilium/cilium/blob/main/operator/pkg/gateway-api/routechecks/gateway_checks.go#L146

hi, @sayboras , thanks for giving this a look. I think the matching hostnames validator is only tangentially related. this is about matching the right listener to the HTTPRoute while its gateway reference is being validated for the namespace restriction. currently, the gateway namespace validator matches the HTTPRoute with the first listener to have a NamespacesFromSelector, and will then immediately fail-close because the wrong listener's namespace selector is used.

is the intention to let the hostnames validator function be the sole gatekeeper for the hostname-matching logic? if so, maybe the NamespacesFromSelector check should fail-open, instead. that way, it can run through all the listeners' namespace selectors, and if none of them match, it could re-attempt validation through the downstream validators such as the matching hostnames validator.

@sayboras
Copy link
Member

ah thanks for your explanation.

Should we use the computeHost function for pattern matching? or is it should be exact match based on your implementation only ?

@cjvirtucio87
Copy link
Contributor Author

ah thanks for your explanation.

Should we use the computeHost function for pattern matching? or is it should be exact match based on your implementation only ?

Sorry maybe I misunderstood what you said earlier. Are you saying I should reuse the ComputeHosts function here?

@sayboras sayboras added this pull request to the merge queue Mar 10, 2024
Merged via the queue into cilium:main with commit a052869 Mar 10, 2024
61 of 62 checks passed
@cjvirtucio87 cjvirtucio87 deleted the hostnames-listeners branch March 10, 2024 13:44
@sayboras sayboras added needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch labels Mar 11, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.15.2 Mar 11, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.8 Mar 11, 2024
@sayboras sayboras added the backport/author The backport will be carried out by the author of the PR. label Mar 12, 2024
@sayboras sayboras assigned sayboras and unassigned cjvirtucio87 Mar 12, 2024
@jrajahalme jrajahalme removed this from Needs backport from main in 1.15.2 Mar 13, 2024
@thorn3r thorn3r added this to Needs backport from main in 1.14.9 Mar 13, 2024
@thorn3r thorn3r removed this from Needs backport from main in 1.14.8 Mar 13, 2024
@jrajahalme jrajahalme added this to Needs backport from main in 1.14.10 Mar 26, 2024
@jrajahalme jrajahalme removed this from Needs backport from main in 1.14.9 Mar 26, 2024
@sayboras sayboras added backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. and removed needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Apr 4, 2024
@github-actions github-actions bot added backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. and removed backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. labels Apr 5, 2024
@asauber asauber added this to Needs backport from main in 1.14.11 Apr 11, 2024
@asauber asauber removed this from Needs backport from main in 1.14.10 Apr 11, 2024
@nebril nebril added this to Needs backport from main in 1.14.12 May 10, 2024
@nebril nebril removed this from Needs backport from main in 1.14.11 May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/author The backport will be carried out by the author of the PR. backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. kind/community-contribution This was a contribution made by a community member. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/agent Cilium agent related.
Projects
1.14.12
Needs backport from main
Development

Successfully merging this pull request may close these issues.

cilium-operator not respecting hostname config on HTTPRoutes and Gateway Listeners
4 participants