-
Notifications
You must be signed in to change notification settings - Fork 301
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
fix: Ignore ports in host headers when matching against HTTPRoute hostnames #1702
Conversation
fa52f01
to
b6e483d
Compare
cc @arkodg |
b6e483d
to
413ce4c
Compare
internal/cmd/egctl/testdata/translate/in/envoy-patch-policy.yaml
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## main #1702 +/- ##
==========================================
- Coverage 65.17% 65.10% -0.08%
==========================================
Files 84 84
Lines 12230 12230
==========================================
- Hits 7971 7962 -9
- Misses 3751 3759 +8
- Partials 508 509 +1
|
a9535b4
to
456eab0
Compare
@@ -80,7 +80,7 @@ func (c *XDSHookClient) PostRouteModifyHook(route *routeV3.Route, routeHostnames | |||
func (c *XDSHookClient) PostVirtualHostModifyHook(vh *routeV3.VirtualHost) (*routeV3.VirtualHost, error) { | |||
// Only make the change when the VirtualHost's name matches the expected testdata | |||
// This prevents us from having to update every single testfile.out | |||
if vh.Name == "extension-post-xdsvirtualhost-hook-error" { | |||
if vh.Name == "extension-post-xdsvirtualhost-hook-error-*" { |
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.
cc @AliceProxy
few minor comments, overall LGTM, thanks for hanging in there, and bringing clarity on precedence matching ! |
Signed-off-by: Jack Kleeman <jackkleeman@gmail.com>
Signed-off-by: Jack Kleeman <jackkleeman@gmail.com>
Signed-off-by: Jack Kleeman <jackkleeman@gmail.com>
Signed-off-by: Jack Kleeman <jackkleeman@gmail.com>
Signed-off-by: Jack Kleeman <jackkleeman@gmail.com>
Signed-off-by: Jack Kleeman <jackkleeman@gmail.com>
Signed-off-by: Jack Kleeman <jackkleeman@gmail.com>
728b9ac
to
d0dc313
Compare
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.
LGTM thanks !
great thanks, please merge when you're happy |
waiting for a LGTM from another reviewer |
What type of PR is this?
fix
What this PR does / why we need it:
We need to be able to match on hostnames in httproute (that are more specific than hostnames in the listener) while also ignoring ports in host headers. Currently we do not ignore ports, and there is no workaround because hostnames may not contain ports, and nor can headermatch values. There are two approaches to solving this:
This PR is the second approach
NB: This PR does not allow 'fallthrough' from one route hostname to a less specific one (eg from google.com to *.com) when there is no match for the first hostname, as envoy does not fall through after a virtual host is selected. This is a breaking change of behaviour as currently the less specific matcher will simply come later in the list of routes under the same virtual host. However, this is moving in line with the spec as discussed in kubernetes-sigs/gateway-api#2294
Which issue(s) this PR fixes:
Fixes #1687