internal/envoy/v3: Strip port from hostname #3458
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sunjayBhatia
requested review from
danehans and
skriss
and removed request for
a team
- Strips host port using the strip_any_host_port field on HTTP connection manager - All filter/router processing will be done internally without the port number attached - Ensures <HOST>:<PORT_NUM> is accepted and treated the same as the plain <HOST> - Removes Lua logic to check port number as it is no longer needed, the filter will never see the port on the :authority header - Removes <HOST>:* wildcard domain match in router, the router will never see the port on the domain to match - Note that this will strip the port from the Host/:authority header sent to upstreams - Bumps go-control-plane as latest tagged release is not new enough and does not include the field used See: https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto.html#envoy-v3-api-field-extensions-filters-network-http-connection-manager-v3-httpconnectionmanager-strip-any-host-port Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
sunjayBhatia
force-pushed
the
strip-host-port
branch
from
87f45a8
to
0fa63fa
Compare
Codecov Report
@@ Coverage Diff @@
## main #3458 +/- ##
==========================================
+ Coverage 75.18% 75.20% +0.02%
==========================================
Files 98 98
Lines 6593 6587 -6
==========================================
- Hits 4957 4954 -3
+ Misses 1523 1520 -3
Partials 113 113
|
sunjayBhatia
commented
Mar 8, 2021
| // before processing by filters or routing. | ||
| // Note that the port a listener is bound to will already be selected | ||
| // and that the port is stripped from the header sent upstream as well. | ||
| StripPortMode: &http.HttpConnectionManager_StripAnyHostPort{ |
There was a problem hiding this comment.
Note: I did not mention the redirect/multiple listener thing b/c I don't think it should be relevant.
- The hierarchy of the configs is Listener -> Filter chain -> Route
- When we build the redirect at the Route level, we know in contour what the "partner" secure port of the plaintext HTTP port should be and can set the redirect to the port with this field: https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#envoy-v3-api-field-config-route-v3-redirectaction-port-redirect
- The redirect will result in a whole new request to a different Listener so stripping the port at the HTTP Connection Manager filter level should be irrelevant
sunjayBhatia
commented
Mar 8, 2021
| @@ -4,7 +4,7 @@ go 1.15 | |||
|
|
|||
| require ( | |||
| github.com/ahmetb/gen-crd-api-reference-docs v0.3.0 | |||
| github.com/envoyproxy/go-control-plane v0.9.8 | |||
| github.com/envoyproxy/go-control-plane v0.9.9-0.20210111201334-f1f47757da33 | |||
There was a problem hiding this comment.
This commit is the one that mirrored the protos from the 1.17.0 release
skriss
approved these changes
Mar 11, 2021
Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
youngnick
approved these changes
Mar 12, 2021
sunjayBhatia
added
the
release-note
|
Release Note: Ensure Envoy strips port from |
sunjayBhatia
added a commit
to sunjayBhatia/contour
that referenced
this pull request
Jun 5, 2023
…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
added a commit
that referenced
this pull request
Jun 5, 2023
…ualhost (#5437) 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. We also make sure the Ingress/HTTPProxy wildcard matching ignores the port in the Host header. Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See: https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto.html#envoy-v3-api-field-extensions-filters-network-http-connection-manager-v3-httpconnectionmanager-strip-any-host-port