Merge pull request #10455 from hashicorp/b-cc-uppercase

consul/connect: check connect group and service names for uppercase characters

CHANGELOG.md

@@ -19,6 +19,7 @@ IMPROVEMENTS:
  * consul: Allow setting `body` field on service/check Consul health checks. [[GH-10186](https://github.com/hashicorp/nomad/issues/10186)]
  * consul/connect: Use exponential backoff for consul envoy bootstrap process [[GH-10453](https://github.com/hashicorp/nomad/pull/10453)]
  * 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)]

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,14 +461,27 @@ 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)
 	}
 
 	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
 }
 

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)
 	})
 }