From ce0da241beae7690d9e9e52a6247345ec3921def Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Tue, 27 Apr 2021 10:23:57 -0600 Subject: [PATCH] consul/connect: check connect group and service names for uppercase characters This PR adds job-submission validation that checks for the use of uppercase characters in group and service names for services that make use of Consul Connect. This prevents attempting to launch services that Consul will not validate correctly, which in turn causes tasks to fail to launch in Nomad. Underlying Consul issue: https://github.com/hashicorp/consul/issues/6765 Closes #7581 #10450 --- CHANGELOG.md | 1 + nomad/job_endpoint_hook_connect.go | 18 ++++++- nomad/job_endpoint_hook_connect_test.go | 71 +++++++++++++++++++++++-- 3 files changed, 85 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d8b505b0d15..bc92c92dfbc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ IMPROVEMENTS: * client/fingerprint: Added support multiple host network aliases for the same interface. [[GH-10104](https://github.com/hashicorp/nomad/issues/10104)] * consul: Allow setting `body` field on service/check Consul health checks. [[GH-10186](https://github.com/hashicorp/nomad/issues/10186)] * consul/connect: Enable setting `local_bind_address` field on connect upstreams [[GH-6248](https://github.com/hashicorp/nomad/issues/6248)] + * consul/connect: Added job-submission validation for Connect sidecar service and group names [[GH-10455](https://github.com/hashicorp/nomad/pull/10455)] * consul/connect: Automatically populate `CONSUL_HTTP_ADDR` for connect native tasks in host networking mode. [[GH-10239](https://github.com/hashicorp/nomad/issues/10239)] * csi: Added support for jobs to request a unique volume ID per allocation. [[GH-10136](https://github.com/hashicorp/nomad/issues/10136)] * driver/docker: Added support for optional extra container labels. [[GH-9885](https://github.com/hashicorp/nomad/issues/9885)] diff --git a/nomad/job_endpoint_hook_connect.go b/nomad/job_endpoint_hook_connect.go index 1e215f6196a..560a993f863 100644 --- a/nomad/job_endpoint_hook_connect.go +++ b/nomad/job_endpoint_hook_connect.go @@ -2,6 +2,7 @@ package nomad import ( "fmt" + "strings" "time" "github.com/hashicorp/nomad/client/taskenv" @@ -444,7 +445,7 @@ func groupConnectValidate(g *structs.TaskGroup) (warnings []error, err error) { for _, s := range g.Services { switch { case s.Connect.HasSidecar(): - if err := groupConnectSidecarValidate(g); err != nil { + if err := groupConnectSidecarValidate(g, s); err != nil { return nil, err } case s.Connect.IsNative(): @@ -460,7 +461,7 @@ func groupConnectValidate(g *structs.TaskGroup) (warnings []error, err error) { return nil, nil } -func groupConnectSidecarValidate(g *structs.TaskGroup) error { +func groupConnectSidecarValidate(g *structs.TaskGroup, s *structs.Service) error { if n := len(g.Networks); n != 1 { return fmt.Errorf("Consul Connect sidecars require exactly 1 network, found %d in group %q", n, g.Name) } @@ -468,6 +469,19 @@ func groupConnectSidecarValidate(g *structs.TaskGroup) error { if g.Networks[0].Mode != "bridge" { return fmt.Errorf("Consul Connect sidecar requires bridge network, found %q in group %q", g.Networks[0].Mode, g.Name) } + + // We must enforce lowercase characters on group and service names for connect + // sidecar proxies, because Consul assumes this invariant without validating it. + // https://github.com/hashicorp/consul/blob/v1.9.5/command/connect/proxy/proxy.go#L235 + + if s.Name != strings.ToLower(s.Name) { + return fmt.Errorf("Consul Connect service name %q in group %q must not contain uppercase characters", s.Name, g.Name) + } + + if g.Name != strings.ToLower(g.Name) { + return fmt.Errorf("Consul Connect group %q with service %q must not contain uppercase characters", g.Name, s.Name) + } + return nil } diff --git a/nomad/job_endpoint_hook_connect_test.go b/nomad/job_endpoint_hook_connect_test.go index 4b197aedce8..6d37c238f95 100644 --- a/nomad/job_endpoint_hook_connect_test.go +++ b/nomad/job_endpoint_hook_connect_test.go @@ -239,11 +239,19 @@ func TestJobEndpointConnect_ConnectInterpolation(t *testing.T) { func TestJobEndpointConnect_groupConnectSidecarValidate(t *testing.T) { t.Parallel() + // network validation + + makeService := func(name string) *structs.Service { + return &structs.Service{Name: name, Connect: &structs.ConsulConnect{ + SidecarService: new(structs.ConsulSidecarService), + }} + } + t.Run("sidecar 0 networks", func(t *testing.T) { require.EqualError(t, groupConnectSidecarValidate(&structs.TaskGroup{ Name: "g1", Networks: nil, - }), `Consul Connect sidecars require exactly 1 network, found 0 in group "g1"`) + }, makeService("connect-service")), `Consul Connect sidecars require exactly 1 network, found 0 in group "g1"`) }) t.Run("sidecar non bridge", func(t *testing.T) { @@ -252,7 +260,7 @@ func TestJobEndpointConnect_groupConnectSidecarValidate(t *testing.T) { Networks: structs.Networks{{ Mode: "host", }}, - }), `Consul Connect sidecar requires bridge network, found "host" in group "g2"`) + }, makeService("connect-service")), `Consul Connect sidecar requires bridge network, found "host" in group "g2"`) }) t.Run("sidecar okay", func(t *testing.T) { @@ -261,7 +269,64 @@ func TestJobEndpointConnect_groupConnectSidecarValidate(t *testing.T) { Networks: structs.Networks{{ Mode: "bridge", }}, - })) + }, makeService("connect-service"))) + }) + + // group and service name validation + + t.Run("non-connect service contains uppercase characters", func(t *testing.T) { + _, err := groupConnectValidate(&structs.TaskGroup{ + Name: "group", + Networks: structs.Networks{{Mode: "bridge"}}, + Services: []*structs.Service{{ + Name: "Other-Service", + }}, + }) + require.NoError(t, err) + }) + + t.Run("connect service contains uppercase characters", func(t *testing.T) { + _, err := groupConnectValidate(&structs.TaskGroup{ + Name: "group", + Networks: structs.Networks{{Mode: "bridge"}}, + Services: []*structs.Service{{ + Name: "Other-Service", + }, makeService("Connect-Service")}, + }) + require.EqualError(t, err, `Consul Connect service name "Connect-Service" in group "group" must not contain uppercase characters`) + }) + + t.Run("non-connect group contains uppercase characters", func(t *testing.T) { + _, err := groupConnectValidate(&structs.TaskGroup{ + Name: "Other-Group", + Networks: structs.Networks{{Mode: "bridge"}}, + Services: []*structs.Service{{ + Name: "other-service", + }}, + }) + require.NoError(t, err) + }) + + t.Run("connect-group contains uppercase characters", func(t *testing.T) { + _, err := groupConnectValidate(&structs.TaskGroup{ + Name: "Connect-Group", + Networks: structs.Networks{{Mode: "bridge"}}, + Services: []*structs.Service{{ + Name: "other-service", + }, makeService("connect-service")}, + }) + require.EqualError(t, err, `Consul Connect group "Connect-Group" with service "connect-service" must not contain uppercase characters`) + }) + + t.Run("connect group and service lowercase", func(t *testing.T) { + _, err := groupConnectValidate(&structs.TaskGroup{ + Name: "connect-group", + Networks: structs.Networks{{Mode: "bridge"}}, + Services: []*structs.Service{{ + Name: "other-service", + }, makeService("connect-service")}, + }) + require.NoError(t, err) }) }