From 165a9af053b14da3c86995028ae067b583f7845f Mon Sep 17 00:00:00 2001 From: Chris Piraino Date: Mon, 8 Jun 2020 13:16:24 -0500 Subject: [PATCH] Always require Host header values for http services (#7990) Previously, we did not require the 'service-name.*' host header value when on a single http service was exposed. However, this allows a user to get into a situation where, if they add another service to the listener, suddenly the previous service's traffic might not be routed correctly. Thus, we always require the Host header, even if there is only 1 service. Also, we add the make the default domain matching more restrictive by matching "service-name.ingress.*" by default. This lines up better with the namespace case and more accurately matches the Consul DNS value we expect people to use in this case. --- agent/xds/routes.go | 10 ++-------- .../routes/ingress-http-multiple-services.golden | 6 +++--- .../ingress-splitter-with-resolver-redirect.golden | 2 +- .../routes/ingress-with-chain-and-router.golden | 2 +- .../routes/ingress-with-chain-and-splitter.golden | 2 +- .../testdata/routes/ingress-with-grpc-router.golden | 2 +- .../envoy/case-ingress-gateway-http/verify.bats | 4 ++-- .../case-ingress-gateway-multiple-services/verify.bats | 4 ++-- 8 files changed, 13 insertions(+), 19 deletions(-) diff --git a/agent/xds/routes.go b/agent/xds/routes.go index 0b870227f0..31c4bf6871 100644 --- a/agent/xds/routes.go +++ b/agent/xds/routes.go @@ -101,20 +101,14 @@ func routesFromSnapshotIngressGateway(cfgSnap *proxycfg.ConfigSnapshot) ([]proto namespace := u.GetEnterpriseMeta().NamespaceOrDefault() var domains []string switch { - case len(upstreams) == 1: - // Don't require a service prefix on the domain if there is only 1 - // upstream. This makes it a smoother experience when only having a - // single service associated to a listener, which is probably a common - // case when demoing/testing - domains = []string{"*"} case len(u.IngressHosts) > 0: // If a user has specified hosts, do not add the default - // ".*" prefix + // ".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.*", chain.ServiceName)} + domains = []string{fmt.Sprintf("%s.ingress.*", chain.ServiceName)} } virtualHost, err := makeUpstreamRouteForDiscoveryChain(upstreamID, chain, domains) diff --git a/agent/xds/testdata/routes/ingress-http-multiple-services.golden b/agent/xds/testdata/routes/ingress-http-multiple-services.golden index 83a38ebff2..cdb5d2daa6 100644 --- a/agent/xds/testdata/routes/ingress-http-multiple-services.golden +++ b/agent/xds/testdata/routes/ingress-http-multiple-services.golden @@ -8,7 +8,7 @@ { "name": "baz", "domains": [ - "baz.*" + "baz.ingress.*" ], "routes": [ { @@ -24,7 +24,7 @@ { "name": "qux", "domains": [ - "qux.*" + "qux.ingress.*" ], "routes": [ { @@ -64,7 +64,7 @@ { "name": "bar", "domains": [ - "bar.*" + "bar.ingress.*" ], "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 2b7e323496..ebcc9f0845 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,7 @@ { "name": "db", "domains": [ - "*" + "db.ingress.*" ], "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 113d7e24cb..0f15394b64 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,7 @@ { "name": "db", "domains": [ - "*" + "db.ingress.*" ], "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 c97aea439a..b9a403fa05 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,7 @@ { "name": "db", "domains": [ - "*" + "db.ingress.*" ], "routes": [ { diff --git a/agent/xds/testdata/routes/ingress-with-grpc-router.golden b/agent/xds/testdata/routes/ingress-with-grpc-router.golden index 616d47afb8..789c7015ba 100644 --- a/agent/xds/testdata/routes/ingress-with-grpc-router.golden +++ b/agent/xds/testdata/routes/ingress-with-grpc-router.golden @@ -8,7 +8,7 @@ { "name": "db", "domains": [ - "*" + "db.ingress.*" ], "routes": [ { diff --git a/test/integration/connect/envoy/case-ingress-gateway-http/verify.bats b/test/integration/connect/envoy/case-ingress-gateway-http/verify.bats index 4e3e08633c..51be6cf226 100644 --- a/test/integration/connect/envoy/case-ingress-gateway-http/verify.bats +++ b/test/integration/connect/envoy/case-ingress-gateway-http/verify.bats @@ -31,10 +31,10 @@ load helpers } @test "ingress should be able to connect to s1 via configured path" { - assert_expected_fortio_name s1 localhost 9999 /s1 + assert_expected_fortio_name s1 router.ingress.consul 9999 /s1 } @test "ingress should be able to connect to s2 via configured path" { - assert_expected_fortio_name s2 localhost 9999 /s2 + assert_expected_fortio_name s2 router.ingress.consul 9999 /s2 } diff --git a/test/integration/connect/envoy/case-ingress-gateway-multiple-services/verify.bats b/test/integration/connect/envoy/case-ingress-gateway-multiple-services/verify.bats index e7e8389f02..17cd62f2e9 100644 --- a/test/integration/connect/envoy/case-ingress-gateway-multiple-services/verify.bats +++ b/test/integration/connect/envoy/case-ingress-gateway-multiple-services/verify.bats @@ -31,11 +31,11 @@ load helpers } @test "ingress should be able to connect to s1 using Host header" { - assert_expected_fortio_name s1 s1.example.consul 9999 + assert_expected_fortio_name s1 s1.ingress.consul 9999 } @test "ingress should be able to connect to s2 using Host header" { - assert_expected_fortio_name s2 s2.example.consul 9999 + assert_expected_fortio_name s2 s2.ingress.consul 9999 } @test "ingress should be able to connect to s1 using a user-specified Host" {