From cbf143844fff8bf5d7168ba5fc2d656d6bcb4851 Mon Sep 17 00:00:00 2001 From: Chris Piraino Date: Tue, 7 Jul 2020 10:43:04 -0500 Subject: [PATCH] Append port number to ingress host domain (#8190) A port can be sent in the Host header as defined in the HTTP RFC, so we take any hosts that we want to match traffic to and also add another host with the listener port added. Also fix an issue with envoy integration tests not running the case-ingress-gateway-tls test. --- agent/xds/routes.go | 61 +++++++++++++++---- agent/xds/routes_test.go | 6 +- .../ingress-http-multiple-services.golden | 13 ++-- ...ess-splitter-with-resolver-redirect.golden | 3 +- .../ingress-with-chain-and-router.golden | 3 +- .../ingress-with-chain-and-splitter.golden | 3 +- .../routes/ingress-with-grpc-router.golden | 3 +- .../config_entries.hcl | 9 +++ .../case-ingress-gateway-tls/verify.bats | 6 +- test/integration/connect/envoy/main_test.go | 1 + 10 files changed, 83 insertions(+), 25 deletions(-) diff --git a/agent/xds/routes.go b/agent/xds/routes.go index 31c4bf6871..db92dd6691 100644 --- a/agent/xds/routes.go +++ b/agent/xds/routes.go @@ -3,6 +3,7 @@ package xds import ( "errors" "fmt" + "net" "strings" "github.com/gogo/protobuf/proto" @@ -98,19 +99,7 @@ func routesFromSnapshotIngressGateway(cfgSnap *proxycfg.ConfigSnapshot) ([]proto continue } - namespace := u.GetEnterpriseMeta().NamespaceOrDefault() - var domains []string - switch { - case len(u.IngressHosts) > 0: - // If a user has specified hosts, do not add the default - // ".ingress.*" prefixes - domains = u.IngressHosts - case namespace != structs.IntentionDefaultNamespace: - domains = []string{fmt.Sprintf("%s.ingress.%s.*", chain.ServiceName, namespace)} - default: - domains = []string{fmt.Sprintf("%s.ingress.*", chain.ServiceName)} - } - + domains := generateUpstreamIngressDomains(listenerKey, u) virtualHost, err := makeUpstreamRouteForDiscoveryChain(upstreamID, chain, domains) if err != nil { return nil, err @@ -124,6 +113,52 @@ func routesFromSnapshotIngressGateway(cfgSnap *proxycfg.ConfigSnapshot) ([]proto return result, nil } +func generateUpstreamIngressDomains(listenerKey proxycfg.IngressListenerKey, u structs.Upstream) []string { + var domains []string + domainsSet := make(map[string]bool) + + namespace := u.GetEnterpriseMeta().NamespaceOrDefault() + switch { + case len(u.IngressHosts) > 0: + // If a user has specified hosts, do not add the default + // ".ingress.*" prefixes + domains = u.IngressHosts + case namespace != structs.IntentionDefaultNamespace: + domains = []string{fmt.Sprintf("%s.ingress.%s.*", u.DestinationName, namespace)} + default: + domains = []string{fmt.Sprintf("%s.ingress.*", u.DestinationName)} + } + + for _, h := range domains { + domainsSet[h] = true + } + + // Host headers may contain port numbers in them, so we need to make sure + // we match on the host with and without the port number. Well-known + // ports like HTTP/HTTPS are stripped from Host headers, but other ports + // will be in the header. + for _, h := range domains { + _, _, err := net.SplitHostPort(h) + // Error message from Go's net/ipsock.go + // We check to see if a port is not missing, and ignore the + // error from SplitHostPort otherwise, since we have previously + // validated the Host values and should trust the user's input. + if err == nil || !strings.Contains(err.Error(), "missing port in address") { + continue + } + + domainWithPort := fmt.Sprintf("%s:%d", h, listenerKey.Port) + + // Do not add a duplicate domain if a hostname with port is already in the + // set + if !domainsSet[domainWithPort] { + domains = append(domains, domainWithPort) + } + } + + return domains +} + func makeUpstreamRouteForDiscoveryChain( routeName string, chain *structs.CompiledDiscoveryChain, diff --git a/agent/xds/routes_test.go b/agent/xds/routes_test.go index d6614025c6..c011cf28f9 100644 --- a/agent/xds/routes_test.go +++ b/agent/xds/routes_test.go @@ -117,7 +117,11 @@ func TestRoutesFromSnapshot(t *testing.T) { { DestinationName: "foo", LocalBindPort: 8080, - IngressHosts: []string{"test1.example.com", "test2.example.com"}, + IngressHosts: []string{ + "test1.example.com", + "test2.example.com", + "test2.example.com:8080", + }, }, { DestinationName: "bar", diff --git a/agent/xds/testdata/routes/ingress-http-multiple-services.golden b/agent/xds/testdata/routes/ingress-http-multiple-services.golden index cdb5d2daa6..aaab96ef9b 100644 --- a/agent/xds/testdata/routes/ingress-http-multiple-services.golden +++ b/agent/xds/testdata/routes/ingress-http-multiple-services.golden @@ -8,7 +8,8 @@ { "name": "baz", "domains": [ - "baz.ingress.*" + "baz.ingress.*", + "baz.ingress.*:443" ], "routes": [ { @@ -24,7 +25,8 @@ { "name": "qux", "domains": [ - "qux.ingress.*" + "qux.ingress.*", + "qux.ingress.*:443" ], "routes": [ { @@ -48,7 +50,9 @@ "name": "foo", "domains": [ "test1.example.com", - "test2.example.com" + "test2.example.com", + "test2.example.com:8080", + "test1.example.com:8080" ], "routes": [ { @@ -64,7 +68,8 @@ { "name": "bar", "domains": [ - "bar.ingress.*" + "bar.ingress.*", + "bar.ingress.*:8080" ], "routes": [ { diff --git a/agent/xds/testdata/routes/ingress-splitter-with-resolver-redirect.golden b/agent/xds/testdata/routes/ingress-splitter-with-resolver-redirect.golden index ebcc9f0845..ef5665e0f2 100644 --- a/agent/xds/testdata/routes/ingress-splitter-with-resolver-redirect.golden +++ b/agent/xds/testdata/routes/ingress-splitter-with-resolver-redirect.golden @@ -8,7 +8,8 @@ { "name": "db", "domains": [ - "db.ingress.*" + "db.ingress.*", + "db.ingress.*:9191" ], "routes": [ { diff --git a/agent/xds/testdata/routes/ingress-with-chain-and-router.golden b/agent/xds/testdata/routes/ingress-with-chain-and-router.golden index 0f15394b64..1c8b1b2c3b 100644 --- a/agent/xds/testdata/routes/ingress-with-chain-and-router.golden +++ b/agent/xds/testdata/routes/ingress-with-chain-and-router.golden @@ -8,7 +8,8 @@ { "name": "db", "domains": [ - "db.ingress.*" + "db.ingress.*", + "db.ingress.*:9191" ], "routes": [ { diff --git a/agent/xds/testdata/routes/ingress-with-chain-and-splitter.golden b/agent/xds/testdata/routes/ingress-with-chain-and-splitter.golden index b9a403fa05..4601126e10 100644 --- a/agent/xds/testdata/routes/ingress-with-chain-and-splitter.golden +++ b/agent/xds/testdata/routes/ingress-with-chain-and-splitter.golden @@ -8,7 +8,8 @@ { "name": "db", "domains": [ - "db.ingress.*" + "db.ingress.*", + "db.ingress.*:9191" ], "routes": [ { diff --git a/agent/xds/testdata/routes/ingress-with-grpc-router.golden b/agent/xds/testdata/routes/ingress-with-grpc-router.golden index 789c7015ba..5e2dd93481 100644 --- a/agent/xds/testdata/routes/ingress-with-grpc-router.golden +++ b/agent/xds/testdata/routes/ingress-with-grpc-router.golden @@ -8,7 +8,8 @@ { "name": "db", "domains": [ - "db.ingress.*" + "db.ingress.*", + "db.ingress.*:9191" ], "routes": [ { diff --git a/test/integration/connect/envoy/case-ingress-gateway-tls/config_entries.hcl b/test/integration/connect/envoy/case-ingress-gateway-tls/config_entries.hcl index 9a24876301..2cde25a7c5 100644 --- a/test/integration/connect/envoy/case-ingress-gateway-tls/config_entries.hcl +++ b/test/integration/connect/envoy/case-ingress-gateway-tls/config_entries.hcl @@ -18,6 +18,15 @@ config_entries { } listeners = [ + { + port = 9998 + protocol = "http" + services = [ + { + name = "s1" + } + ] + }, { port = 9999 protocol = "http" diff --git a/test/integration/connect/envoy/case-ingress-gateway-tls/verify.bats b/test/integration/connect/envoy/case-ingress-gateway-tls/verify.bats index 0c08a224d9..6ee2288908 100644 --- a/test/integration/connect/envoy/case-ingress-gateway-tls/verify.bats +++ b/test/integration/connect/envoy/case-ingress-gateway-tls/verify.bats @@ -23,12 +23,12 @@ load helpers } @test "should be able to connect to s1 through the TLS-enabled ingress port" { - assert_dnssan_in_cert localhost:9999 '\*.ingress.consul' + assert_dnssan_in_cert localhost:9998 '\*.ingress.consul' # Use the --resolve argument to fake dns resolution for now so we can use the # s1.ingress.consul domain to validate the cert run retry_default curl --cacert <(get_ca_root) -s -f -d hello \ - --resolve s1.ingress.consul:9999:127.0.0.1 \ - https://s1.ingress.consul:9999 + --resolve s1.ingress.consul:9998:127.0.0.1 \ + https://s1.ingress.consul:9998 [ "$status" -eq 0 ] [ "$output" = "hello" ] } diff --git a/test/integration/connect/envoy/main_test.go b/test/integration/connect/envoy/main_test.go index 90bfd01e4d..f7844524e0 100644 --- a/test/integration/connect/envoy/main_test.go +++ b/test/integration/connect/envoy/main_test.go @@ -32,6 +32,7 @@ func TestEnvoy(t *testing.T) { "case-ingress-gateway-http", "case-ingress-gateway-multiple-services", "case-ingress-gateway-simple", + "case-ingress-gateway-tls", "case-ingress-mesh-gateways-resolver", "case-multidc-rsa-ca", "case-prometheus",