Skip to content

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 <[email protected]>
@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 <[email protected]>
@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 <[email protected]>
@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