From 7d1059b5aed1a7c17f08380fbc46dacc909fe751 Mon Sep 17 00:00:00 2001
From: Daniel Bennett <dbennett@hashicorp.com>
Date: Wed, 11 Jan 2023 11:52:32 -0600
Subject: [PATCH] connect: ingress gateway validation for http hosts and
 wildcards (#15749)

* connect: fix non-"tcp" ingress gateway validation

changes apply to http, http2, and grpc:
* if "hosts" is excluded, consul will use its default domain
  e.g. <service-name>.ingress.dc1.consul
* can't set hosts with "*" service name
* test http2 and grpc too
---
 .changelog/15749.txt           |  3 ++
 nomad/structs/services.go      | 14 +++-----
 nomad/structs/services_test.go | 65 ++++++++++++++++++++--------------
 3 files changed, 46 insertions(+), 36 deletions(-)
 create mode 100644 .changelog/15749.txt

diff --git a/.changelog/15749.txt b/.changelog/15749.txt
new file mode 100644
index 000000000..f8dfda285
--- /dev/null
+++ b/.changelog/15749.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+connect: ingress http/2/grpc listeners may exclude hosts
+```
diff --git a/nomad/structs/services.go b/nomad/structs/services.go
index e6899af75..33f4b056a 100644
--- a/nomad/structs/services.go
+++ b/nomad/structs/services.go
@@ -1874,13 +1874,13 @@ func (s *ConsulIngressService) Validate(protocol string) error {
 		return nil
 	}
 
+	// pre-validate service Name and Hosts before passing along to consul:
+	// https://developer.hashicorp.com/consul/docs/connect/config-entries/ingress-gateway#services
+
 	if s.Name == "" {
 		return errors.New("Consul Ingress Service requires a name")
 	}
 
-	// Validation of wildcard service name and hosts varies depending on the
-	// protocol for the gateway.
-	// https://www.consul.io/docs/connect/config-entries/ingress-gateway#hosts
 	switch protocol {
 	case "tcp":
 		if s.Name == "*" {
@@ -1891,12 +1891,8 @@ func (s *ConsulIngressService) Validate(protocol string) error {
 			return errors.New(`Consul Ingress Service doesn't support associating hosts to a service for the "tcp" protocol`)
 		}
 	default:
-		if s.Name == "*" {
-			return nil
-		}
-
-		if len(s.Hosts) == 0 {
-			return fmt.Errorf("Consul Ingress Service requires one or more hosts when using %q protocol", protocol)
+		if s.Name == "*" && len(s.Hosts) != 0 {
+			return errors.New(`Consul Ingress Service with a wildcard "*" service name can not also specify hosts`)
 		}
 	}
 
diff --git a/nomad/structs/services_test.go b/nomad/structs/services_test.go
index 9f81bd744..b43def12a 100644
--- a/nomad/structs/services_test.go
+++ b/nomad/structs/services_test.go
@@ -1325,14 +1325,7 @@ func TestConsulIngressService_Validate(t *testing.T) {
 		err := (&ConsulIngressService{
 			Name: "",
 		}).Validate("http")
-		require.EqualError(t, err, "Consul Ingress Service requires a name")
-	})
-
-	t.Run("http missing hosts", func(t *testing.T) {
-		err := (&ConsulIngressService{
-			Name: "service1",
-		}).Validate("http")
-		require.EqualError(t, err, `Consul Ingress Service requires one or more hosts when using "http" protocol`)
+		must.EqError(t, err, "Consul Ingress Service requires a name")
 	})
 
 	t.Run("tcp extraneous hosts", func(t *testing.T) {
@@ -1340,37 +1333,55 @@ func TestConsulIngressService_Validate(t *testing.T) {
 			Name:  "service1",
 			Hosts: []string{"host1"},
 		}).Validate("tcp")
-		require.EqualError(t, err, `Consul Ingress Service doesn't support associating hosts to a service for the "tcp" protocol`)
+		must.EqError(t, err, `Consul Ingress Service doesn't support associating hosts to a service for the "tcp" protocol`)
 	})
 
-	t.Run("ok tcp", func(t *testing.T) {
+	t.Run("tcp ok", func(t *testing.T) {
 		err := (&ConsulIngressService{
 			Name: "service1",
 		}).Validate("tcp")
-		require.NoError(t, err)
-	})
-
-	t.Run("ok http", func(t *testing.T) {
-		err := (&ConsulIngressService{
-			Name:  "service1",
-			Hosts: []string{"host1"},
-		}).Validate("http")
-		require.NoError(t, err)
-	})
-
-	t.Run("http with wildcard service", func(t *testing.T) {
-		err := (&ConsulIngressService{
-			Name: "*",
-		}).Validate("http")
-		require.NoError(t, err)
+		must.NoError(t, err)
 	})
 
 	t.Run("tcp with wildcard service", func(t *testing.T) {
 		err := (&ConsulIngressService{
 			Name: "*",
 		}).Validate("tcp")
-		require.EqualError(t, err, `Consul Ingress Service doesn't support wildcard name for "tcp" protocol`)
+		must.EqError(t, err, `Consul Ingress Service doesn't support wildcard name for "tcp" protocol`)
 	})
+
+	// non-"tcp" protocols should be all treated the same.
+	for _, proto := range []string{"http", "http2", "grpc"} {
+		t.Run(proto+" ok", func(t *testing.T) {
+			err := (&ConsulIngressService{
+				Name:  "service1",
+				Hosts: []string{"host1"},
+			}).Validate(proto)
+			must.NoError(t, err)
+		})
+
+		t.Run(proto+" without hosts", func(t *testing.T) {
+			err := (&ConsulIngressService{
+				Name: "service1",
+			}).Validate(proto)
+			must.NoError(t, err, must.Sprintf(`"%s" protocol should not require hosts`, proto))
+		})
+
+		t.Run(proto+" wildcard service", func(t *testing.T) {
+			err := (&ConsulIngressService{
+				Name: "*",
+			}).Validate(proto)
+			must.NoError(t, err, must.Sprintf(`"%s" protocol should allow wildcard service`, proto))
+		})
+
+		t.Run(proto+" wildcard service and host", func(t *testing.T) {
+			err := (&ConsulIngressService{
+				Name:  "*",
+				Hosts: []string{"any"},
+			}).Validate(proto)
+			must.EqError(t, err, `Consul Ingress Service with a wildcard "*" service name can not also specify hosts`)
+		})
+	}
 }
 
 func TestConsulIngressListener_Validate(t *testing.T) {