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.
2020-07-07 10:43:04 -05:00 · 2020-07-07 10:43:04 -05:00 · 9d92c42c90
parent 3ef57a8ead
commit 9d92c42c90
10 changed files with 83 additions and 25 deletions
--- a/agent/xds/routes.go
+++ b/agent/xds/routes.go
@ -3,6 +3,7 @@ package xds
 import (
 	"errors"
 	"fmt"
+	"net"
 	"strings"

 	envoy "github.com/envoyproxy/go-control-plane/envoy/api/v2"
@ -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
-				// "<service-name>.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
+		// "<service-name>.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,
--- 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",
--- 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": [
            {
--- 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": [
            {
--- 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": [
            {
--- 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": [
            {
--- 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": [
            {
--- 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"
--- 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" ]
 }
--- 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",