Skip to content

Commit

Permalink
internal/envoy/v3: RouteConfiguration ignores port when choosing virt…
Browse files Browse the repository at this point in the history 
…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>
  • Loading branch information
@sunjayBhatia
sunjayBhatia authored Jun 5, 2023
1 parent c923b99 commit 5fc91fa
Show file tree
Hide file tree
Showing 9 changed files with 40 additions and 72 deletions.
5 changes: 5 additions & 0 deletions changelogs/unreleased/5437-sunjayBhatia-minor.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
## Host header including port is passed through unmodified to backend

Previously Contour would strip any port from the Host header in a downstream request for convenience in routing.
This resulted in backends not receiving the Host header with a port.
We no longer do this, for conformance with Gateway API (this change also applies to HTTPProxy and Ingress configuration).
4 changes: 2 additions & 2 deletions internal/dag/builder_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10359,7 +10359,7 @@ func TestDAGInsert(t *testing.T) {
&Route{
PathMatchCondition: prefixString("/"),
HeaderMatchConditions: []HeaderMatchCondition{
{Name: ":authority", Value: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.projectcontour\\.io", MatchType: "regex", Invert: false},
{Name: ":authority", Value: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.projectcontour\\.io(:[0-9]+)?", MatchType: "regex", Invert: false},
},
Clusters: clusters(service(s1)),
}),
Expand Down Expand Up @@ -10780,7 +10780,7 @@ func TestDAGInsert(t *testing.T) {
{
Name: ":authority",
MatchType: HeaderMatchTypeRegex,
Value: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.example\\.com",
Value: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.example\\.com(:[0-9]+)?",
},
},
Clusters: clusters(service(s1)),
Expand Down
10 changes: 9 additions & 1 deletion internal/dag/dag.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1170,14 +1170,22 @@ type ExtensionCluster struct {
ClientCertificate *Secret
}

const singleDNSLabelWildcardRegex = "^[a-z0-9]([-a-z0-9]*[a-z0-9])?"

var _ = regexp.MustCompile(singleDNSLabelWildcardRegex)

const ignorePortRegex = "(:[0-9]+)?"

var _ = regexp.MustCompile(ignorePortRegex)

func wildcardDomainHeaderMatch(fqdn string) HeaderMatchCondition {
return HeaderMatchCondition{
// Internally Envoy uses the HTTP/2 ":authority" header in
// place of the HTTP/1 "host" header.
// See: https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#config-route-v3-headermatcher
Name: ":authority",
MatchType: HeaderMatchTypeRegex,
Value: singleDNSLabelWildcardRegex + regexp.QuoteMeta(fqdn[1:]),
Value: singleDNSLabelWildcardRegex + regexp.QuoteMeta(fqdn[1:]) + ignorePortRegex,
}
}

Expand Down
5 changes: 0 additions & 5 deletions internal/dag/ingress_processor.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
package dag

import (
"regexp"
"strconv"
"strings"
"time"
Expand Down Expand Up @@ -203,10 +202,6 @@ func (p *IngressProcessor) computeIngressRule(ing *networking_v1.Ingress, rule n
}
}

const singleDNSLabelWildcardRegex = "^[a-z0-9]([-a-z0-9]*[a-z0-9])?"

var _ = regexp.MustCompile(singleDNSLabelWildcardRegex)

// route builds a dag.Route for the supplied Ingress.
func (p *IngressProcessor) route(ingress *networking_v1.Ingress, host string, path string, pathType networking_v1.PathType, service *Service, clientCertSecret *Secret, serviceName string, servicePort int32, log logrus.FieldLogger) (*Route, error) {
log = log.WithFields(logrus.Fields{
Expand Down
13 changes: 5 additions & 8 deletions internal/envoy/v3/listener.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -474,14 +474,6 @@ func (b *httpConnectionManagerBuilder) Get() *envoy_listener_v3.Filter {

NormalizePath: wrapperspb.Bool(true),

// We can ignore any port number supplied in the Host/:authority header
// 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{
StripAnyHostPort: true,
},

// issue #1487 pass through X-Request-Id if provided.
PreserveExternalRequestId: true,
MergeSlashes: b.mergeSlashes,
Expand Down Expand Up @@ -713,6 +705,11 @@ function envoy_on_request(request_handle)
local host = string.lower(headers:get(":authority"))
local target = %s
s, e = string.find(host, ":", 1, true)
if s ~= nil then
host = string.sub(host, 1, s - 1)
end
if host ~= target then
request_handle:respond(
{[":status"] = "421"},
Expand Down
62 changes: 10 additions & 52 deletions internal/envoy/v3/listener_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -655,9 +655,6 @@ func TestHTTPConnectionManager(t *testing.T) {
AccessLog: FileAccessLogEnvoy("/dev/stdout", "", nil, v1alpha1.LogLevelInfo),
UseRemoteAddress: wrapperspb.Bool(true),
NormalizePath: wrapperspb.Bool(true),
StripPortMode: &http.HttpConnectionManager_StripAnyHostPort{
StripAnyHostPort: true,
},
PreserveExternalRequestId: true,
MergeSlashes: false,
}),
Expand Down Expand Up @@ -705,9 +702,6 @@ func TestHTTPConnectionManager(t *testing.T) {
AccessLog: FileAccessLogEnvoy("/dev/stdout", "", nil, v1alpha1.LogLevelInfo),
UseRemoteAddress: wrapperspb.Bool(true),
NormalizePath: wrapperspb.Bool(true),
StripPortMode: &http.HttpConnectionManager_StripAnyHostPort{
StripAnyHostPort: true,
},
RequestTimeout: durationpb.New(10 * time.Second),
PreserveExternalRequestId: true,
MergeSlashes: false,
Expand Down Expand Up @@ -755,12 +749,9 @@ func TestHTTPConnectionManager(t *testing.T) {
CommonHttpProtocolOptions: &envoy_core_v3.HttpProtocolOptions{
IdleTimeout: durationpb.New(90 * time.Second),
},
AccessLog: FileAccessLogEnvoy("/dev/stdout", "", nil, v1alpha1.LogLevelInfo),
UseRemoteAddress: wrapperspb.Bool(true),
NormalizePath: wrapperspb.Bool(true),
StripPortMode: &http.HttpConnectionManager_StripAnyHostPort{
StripAnyHostPort: true,
},
AccessLog: FileAccessLogEnvoy("/dev/stdout", "", nil, v1alpha1.LogLevelInfo),
UseRemoteAddress: wrapperspb.Bool(true),
NormalizePath: wrapperspb.Bool(true),
PreserveExternalRequestId: true,
MergeSlashes: false,
}),
Expand Down Expand Up @@ -808,9 +799,6 @@ func TestHTTPConnectionManager(t *testing.T) {
AccessLog: FileAccessLogEnvoy("/dev/stdout", "", nil, v1alpha1.LogLevelInfo),
UseRemoteAddress: wrapperspb.Bool(true),
NormalizePath: wrapperspb.Bool(true),
StripPortMode: &http.HttpConnectionManager_StripAnyHostPort{
StripAnyHostPort: true,
},
PreserveExternalRequestId: true,
MergeSlashes: false,
StreamIdleTimeout: durationpb.New(90 * time.Second),
Expand Down Expand Up @@ -858,12 +846,9 @@ func TestHTTPConnectionManager(t *testing.T) {
CommonHttpProtocolOptions: &envoy_core_v3.HttpProtocolOptions{
MaxConnectionDuration: durationpb.New(90 * time.Second),
},
AccessLog: FileAccessLogEnvoy("/dev/stdout", "", nil, v1alpha1.LogLevelInfo),
UseRemoteAddress: wrapperspb.Bool(true),
NormalizePath: wrapperspb.Bool(true),
StripPortMode: &http.HttpConnectionManager_StripAnyHostPort{
StripAnyHostPort: true,
},
AccessLog: FileAccessLogEnvoy("/dev/stdout", "", nil, v1alpha1.LogLevelInfo),
UseRemoteAddress: wrapperspb.Bool(true),
NormalizePath: wrapperspb.Bool(true),
PreserveExternalRequestId: true,
MergeSlashes: false,
}),
Expand Down Expand Up @@ -911,9 +896,6 @@ func TestHTTPConnectionManager(t *testing.T) {
AccessLog: FileAccessLogEnvoy("/dev/stdout", "", nil, v1alpha1.LogLevelInfo),
UseRemoteAddress: wrapperspb.Bool(true),
NormalizePath: wrapperspb.Bool(true),
StripPortMode: &http.HttpConnectionManager_StripAnyHostPort{
StripAnyHostPort: true,
},
PreserveExternalRequestId: true,
MergeSlashes: false,
}),
Expand Down Expand Up @@ -961,9 +943,6 @@ func TestHTTPConnectionManager(t *testing.T) {
AccessLog: FileAccessLogEnvoy("/dev/stdout", "", nil, v1alpha1.LogLevelInfo),
UseRemoteAddress: wrapperspb.Bool(true),
NormalizePath: wrapperspb.Bool(true),
StripPortMode: &http.HttpConnectionManager_StripAnyHostPort{
StripAnyHostPort: true,
},
PreserveExternalRequestId: true,
MergeSlashes: false,
DelayedCloseTimeout: durationpb.New(90 * time.Second),
Expand Down Expand Up @@ -1012,9 +991,6 @@ func TestHTTPConnectionManager(t *testing.T) {
AccessLog: FileAccessLogEnvoy("/dev/stdout", "", nil, v1alpha1.LogLevelInfo),
UseRemoteAddress: wrapperspb.Bool(true),
NormalizePath: wrapperspb.Bool(true),
StripPortMode: &http.HttpConnectionManager_StripAnyHostPort{
StripAnyHostPort: true,
},
PreserveExternalRequestId: true,
MergeSlashes: false,
DrainTimeout: durationpb.New(90 * time.Second),
Expand Down Expand Up @@ -1065,9 +1041,6 @@ func TestHTTPConnectionManager(t *testing.T) {
AccessLog: FileAccessLogEnvoy("/dev/stdout", "", nil, v1alpha1.LogLevelInfo),
UseRemoteAddress: wrapperspb.Bool(true),
NormalizePath: wrapperspb.Bool(true),
StripPortMode: &http.HttpConnectionManager_StripAnyHostPort{
StripAnyHostPort: true,
},
PreserveExternalRequestId: true,
MergeSlashes: false,
DrainTimeout: durationpb.New(90 * time.Second),
Expand Down Expand Up @@ -1116,9 +1089,6 @@ func TestHTTPConnectionManager(t *testing.T) {
AccessLog: FileAccessLogEnvoy("/dev/stdout", "", nil, v1alpha1.LogLevelInfo),
UseRemoteAddress: wrapperspb.Bool(true),
NormalizePath: wrapperspb.Bool(true),
StripPortMode: &http.HttpConnectionManager_StripAnyHostPort{
StripAnyHostPort: true,
},
PreserveExternalRequestId: true,
MergeSlashes: true,
}),
Expand Down Expand Up @@ -1162,13 +1132,10 @@ func TestHTTPConnectionManager(t *testing.T) {
// a Host: header. See #537.
AcceptHttp_10: true,
},
CommonHttpProtocolOptions: &envoy_core_v3.HttpProtocolOptions{},
AccessLog: FileAccessLogEnvoy("/dev/stdout", "", nil, v1alpha1.LogLevelInfo),
UseRemoteAddress: wrapperspb.Bool(true),
NormalizePath: wrapperspb.Bool(true),
StripPortMode: &http.HttpConnectionManager_StripAnyHostPort{
StripAnyHostPort: true,
},
CommonHttpProtocolOptions: &envoy_core_v3.HttpProtocolOptions{},
AccessLog: FileAccessLogEnvoy("/dev/stdout", "", nil, v1alpha1.LogLevelInfo),
UseRemoteAddress: wrapperspb.Bool(true),
NormalizePath: wrapperspb.Bool(true),
PreserveExternalRequestId: true,
ServerHeaderTransformation: http.HttpConnectionManager_PASS_THROUGH,
}),
Expand Down Expand Up @@ -1230,9 +1197,6 @@ func TestHTTPConnectionManager(t *testing.T) {
AccessLog: FileAccessLogEnvoy("/dev/stdout", "", nil, v1alpha1.LogLevelInfo),
UseRemoteAddress: wrapperspb.Bool(true),
NormalizePath: wrapperspb.Bool(true),
StripPortMode: &http.HttpConnectionManager_StripAnyHostPort{
StripAnyHostPort: true,
},
PreserveExternalRequestId: true,
MergeSlashes: false,
}),
Expand Down Expand Up @@ -1290,9 +1254,6 @@ func TestHTTPConnectionManager(t *testing.T) {
AccessLog: FileAccessLogEnvoy("/dev/stdout", "", nil, v1alpha1.LogLevelInfo),
UseRemoteAddress: wrapperspb.Bool(true),
NormalizePath: wrapperspb.Bool(true),
StripPortMode: &http.HttpConnectionManager_StripAnyHostPort{
StripAnyHostPort: true,
},
PreserveExternalRequestId: true,
MergeSlashes: false,
}),
Expand Down Expand Up @@ -1340,9 +1301,6 @@ func TestHTTPConnectionManager(t *testing.T) {
AccessLog: FileAccessLogEnvoy("/dev/stdout", "", nil, v1alpha1.LogLevelInfo),
UseRemoteAddress: wrapperspb.Bool(true),
NormalizePath: wrapperspb.Bool(true),
StripPortMode: &http.HttpConnectionManager_StripAnyHostPort{
StripAnyHostPort: true,
},
PreserveExternalRequestId: true,
MergeSlashes: false,
XffNumTrustedHops: uint32(1),
Expand Down
3 changes: 3 additions & 0 deletions internal/envoy/v3/route.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -649,6 +649,9 @@ func RouteConfiguration(name string, virtualhosts ...*envoy_route_v3.VirtualHost
RequestHeadersToAdd: headers(
appendHeader("x-request-start", "t=%START_TIME(%s.%3f)%"),
),
// We can ignore any port number supplied in the Host/:authority header
// when selecting a virtualhost.
IgnorePortInHostMatching: true,
}
}

Expand Down
2 changes: 2 additions & 0 deletions internal/envoy/v3/route_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1070,6 +1070,7 @@ func TestRouteConfiguration(t *testing.T) {
},
AppendAction: envoy_core_v3.HeaderValueOption_APPEND_IF_EXISTS_OR_ADD,
}},
IgnorePortInHostMatching: true,
},
},
"one virtualhost": {
Expand All @@ -1089,6 +1090,7 @@ func TestRouteConfiguration(t *testing.T) {
},
AppendAction: envoy_core_v3.HeaderValueOption_APPEND_IF_EXISTS_OR_ADD,
}},
IgnorePortInHostMatching: true,
},
},
}
Expand Down
8 changes: 4 additions & 4 deletions internal/featuretests/v3/wildcardhost_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ func TestIngressWildcardHostHTTP(t *testing.T) {
StringMatch: &matcher.StringMatcher{
MatchPattern: &matcher.StringMatcher_SafeRegex{
SafeRegex: &matcher.RegexMatcher{
Regex: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.foo\\.com",
Regex: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.foo\\.com(:[0-9]+)?",
},
},
},
Expand Down Expand Up @@ -154,7 +154,7 @@ func TestHTTPProxyWildcardFQDN(t *testing.T) {
StringMatch: &matcher.StringMatcher{
MatchPattern: &matcher.StringMatcher_SafeRegex{
SafeRegex: &matcher.RegexMatcher{
Regex: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.projectcontour\\.io",
Regex: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.projectcontour\\.io(:[0-9]+)?",
},
},
},
Expand Down Expand Up @@ -259,7 +259,7 @@ func TestIngressWildcardHostHTTPSWildcardSecret(t *testing.T) {
StringMatch: &matcher.StringMatcher{
MatchPattern: &matcher.StringMatcher_SafeRegex{
SafeRegex: &matcher.RegexMatcher{
Regex: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.foo-tls\\.com",
Regex: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.foo-tls\\.com(:[0-9]+)?",
},
},
},
Expand Down Expand Up @@ -287,7 +287,7 @@ func TestIngressWildcardHostHTTPSWildcardSecret(t *testing.T) {
StringMatch: &matcher.StringMatcher{
MatchPattern: &matcher.StringMatcher_SafeRegex{
SafeRegex: &matcher.RegexMatcher{
Regex: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.foo-tls\\.com",
Regex: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.foo-tls\\.com(:[0-9]+)?",
},
},
},
Expand Down

0 comments on commit 5fc91fa

Please sign in to comment.