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

internal/envoy/v3: RouteConfiguration ignores port when choosing virtualhost #5437

Merged

Conversation

sunjayBhatia
Copy link
Member

Previously we stripped the port from the Host header when matching a virtualhost, which also resulted in the header being modified for the downstream. For Gateway API conformance, we no longer want to do this.

This basically reverts #3458 and uses the newer RouteConfiguration field that allows us to ignore port when choosing a virtualhost. We reintroduce the logic in the SNI/Host misdirected request Lua to strip the port when checking the hostname against SNI.

…ualhost

Previously we stripped the port from the Host header when matching a
virtualhost, which also resulted in the header being modified for the
downstream. For Gateway API conformance, we no longer want to do this.

This basically reverts projectcontour#3458 and uses the newer RouteConfiguration field
that allows us to ignore port when choosing a virtualhost. We
reintroduce the logic in the SNI/Host misdirected request Lua to strip
the port when checking the hostname against SNI.

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
@sunjayBhatia sunjayBhatia added the release-note/small A small change that needs one line of explanation in the release notes. label Jun 5, 2023
@sunjayBhatia sunjayBhatia requested a review from a team as a code owner June 5, 2023 20:35
@sunjayBhatia sunjayBhatia requested review from tsaarni, stevesloka and skriss and removed request for a team June 5, 2023 20:35
@sunjayBhatia sunjayBhatia added release-note/minor A minor change that needs about a paragraph of explanation in the release notes. and removed release-note/small A small change that needs one line of explanation in the release notes. labels Jun 5, 2023
Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
@sunjayBhatia
Copy link
Member Author

huh the ingress test fails but almost identical gateway one passes

@sunjayBhatia
Copy link
Member Author

sunjayBhatia commented Jun 5, 2023

ah, the header regex match on a single DNS label seems to be doing it, it relies on the port not being present

needs to ignore port if present

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
@codecov
Copy link

codecov bot commented Jun 5, 2023

Codecov Report

Merging #5437 (50b8aa1) into main (3e1f1cd) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #5437   +/-   ##
=======================================
  Coverage   78.18%   78.18%           
=======================================
  Files         138      138           
  Lines       18496    18496           
=======================================
  Hits        14462    14462           
  Misses       3757     3757           
  Partials      277      277           
Impacted Files Coverage Δ
internal/dag/ingress_processor.go 97.07% <ø> (ø)
internal/dag/dag.go 98.68% <100.00%> (ø)
internal/envoy/v3/listener.go 98.43% <100.00%> (-0.01%) ⬇️
internal/envoy/v3/route.go 80.23% <100.00%> (+0.07%) ⬆️

Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

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

LGTM assuming E2E's are happy now, :authority header match change make sense

Looks like gateway-conformance failure is just the TLSRoute flake that should be fixed with 0.7.1.

@sunjayBhatia sunjayBhatia merged commit 5fc91fa into projectcontour:main Jun 5, 2023
@sunjayBhatia sunjayBhatia deleted the routeconfig-ignore-host-port branch June 5, 2023 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/minor A minor change that needs about a paragraph of explanation in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants