From be19d7c5ee0000ec9b0a054edf92e840bfe8bfd5 Mon Sep 17 00:00:00 2001 From: Daniel Bennett Date: Tue, 10 Jan 2023 11:52:30 -0600 Subject: [PATCH 1/3] 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. .ingress.dc1.consul * can't set hosts with "*" service name * test http2 and grpc too --- nomad/structs/services.go | 14 +++------ nomad/structs/services_test.go | 57 ++++++++++++++++++++-------------- 2 files changed, 39 insertions(+), 32 deletions(-) diff --git a/nomad/structs/services.go b/nomad/structs/services.go index e6899af75240..33f4b056aea6 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 9f81bd744369..d6cf75a149ec 100644 --- a/nomad/structs/services_test.go +++ b/nomad/structs/services_test.go @@ -1328,13 +1328,6 @@ func TestConsulIngressService_Validate(t *testing.T) { 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`) - }) - t.Run("tcp extraneous hosts", func(t *testing.T) { err := (&ConsulIngressService{ Name: "service1", @@ -1343,34 +1336,52 @@ func TestConsulIngressService_Validate(t *testing.T) { require.EqualError(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) - }) - 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`) }) + + // 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) + require.NoError(t, err) + }) + + t.Run(proto+" without hosts", func(t *testing.T) { + err := (&ConsulIngressService{ + Name: "service1", + }).Validate(proto) + require.NoErrorf(t, err, `should not require hosts with "%s" protocol`, proto) + }) + + t.Run(proto+" wildcard service", func(t *testing.T) { + err := (&ConsulIngressService{ + Name: "*", + }).Validate(proto) + require.NoErrorf(t, err, `should allow wildcard hosts with "%s" protocol`, proto) + }) + + t.Run(proto+" wildcard service and host", func(t *testing.T) { + err := (&ConsulIngressService{ + Name: "*", + Hosts: []string{"any"}, + }).Validate(proto) + require.EqualError(t, err, `Consul Ingress Service with a wildcard "*" service name can not also specify hosts`) + }) + } } func TestConsulIngressListener_Validate(t *testing.T) { From 90d81baaedc860469f6a1f114d5a797d0c10a6f7 Mon Sep 17 00:00:00 2001 From: Daniel Bennett Date: Tue, 10 Jan 2023 14:40:51 -0600 Subject: [PATCH 2/3] changelog: add entry for #15749 --- .changelog/15749.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/15749.txt diff --git a/.changelog/15749.txt b/.changelog/15749.txt new file mode 100644 index 000000000000..f8dfda285672 --- /dev/null +++ b/.changelog/15749.txt @@ -0,0 +1,3 @@ +```release-note:bug +connect: ingress http/2/grpc listeners may exclude hosts +``` From f81d0b5bc973d80bac6c19a9faf1d24fcc10f34f Mon Sep 17 00:00:00 2001 From: Daniel Bennett Date: Tue, 10 Jan 2023 17:15:23 -0600 Subject: [PATCH 3/3] test: use must, one must --- nomad/structs/services_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/nomad/structs/services_test.go b/nomad/structs/services_test.go index d6cf75a149ec..b43def12afde 100644 --- a/nomad/structs/services_test.go +++ b/nomad/structs/services_test.go @@ -1325,7 +1325,7 @@ func TestConsulIngressService_Validate(t *testing.T) { err := (&ConsulIngressService{ Name: "", }).Validate("http") - require.EqualError(t, err, "Consul Ingress Service requires a name") + must.EqError(t, err, "Consul Ingress Service requires a name") }) t.Run("tcp extraneous hosts", func(t *testing.T) { @@ -1333,21 +1333,21 @@ 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("tcp ok", func(t *testing.T) { err := (&ConsulIngressService{ Name: "service1", }).Validate("tcp") - 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. @@ -1357,21 +1357,21 @@ func TestConsulIngressService_Validate(t *testing.T) { Name: "service1", Hosts: []string{"host1"}, }).Validate(proto) - require.NoError(t, err) + must.NoError(t, err) }) t.Run(proto+" without hosts", func(t *testing.T) { err := (&ConsulIngressService{ Name: "service1", }).Validate(proto) - require.NoErrorf(t, err, `should not require hosts with "%s" protocol`, 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) - require.NoErrorf(t, err, `should allow wildcard hosts with "%s" protocol`, 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) { @@ -1379,7 +1379,7 @@ func TestConsulIngressService_Validate(t *testing.T) { Name: "*", Hosts: []string{"any"}, }).Validate(proto) - require.EqualError(t, err, `Consul Ingress Service with a wildcard "*" service name can not also specify hosts`) + must.EqError(t, err, `Consul Ingress Service with a wildcard "*" service name can not also specify hosts`) }) } }