Merge pull request #6575 from hashicorp/b-gh-6571-missing-service-network

Fix some connect connect validation

nomad/job_endpoint_hook_connect.go

@@ -50,6 +50,15 @@ func (jobConnectHook) Name() string {
 
 func (jobConnectHook) Mutate(job *structs.Job) (_ *structs.Job, warnings []error, err error) {
 	for _, g := range job.TaskGroups {
+		// TG isn't validated yet, but validation
+		// may depend on mutation results.
+		// Do basic validation here and skip mutation,
+		// so Validate can return a meaningful error
+		// messages
+		if len(g.Networks) == 0 {
+			continue
+		}
+
 		if err := groupConnectHook(g); err != nil {
 			return nil, nil, err
 		}

nomad/job_endpoint_hooks.go

@@ -53,21 +53,23 @@ func (j *Job) admissionMutators(job *structs.Job) (_ *structs.Job, warnings []er
 
 // admissionValidators returns a slice of validation warnings and a multierror
 // of validation failures.
-func (j *Job) admissionValidators(origJob *structs.Job) (warnings []error, err error) {
+func (j *Job) admissionValidators(origJob *structs.Job) ([]error, error) {
 	// ensure job is not mutated
 	job := origJob.Copy()
 
-	var w []error
-	errs := new(multierror.Error)
+	var warnings []error
+	var errs error
+
 	for _, validator := range j.validators {
-		w, err = validator.Validate(job)
+		w, err := validator.Validate(job)
 		j.logger.Trace("job validate results", "validator", validator.Name(), "warnings", w, "error", err)
 		if err != nil {
-			multierror.Append(errs, err)
+			errs = multierror.Append(errs, err)
 		}
 		warnings = append(warnings, w...)
 	}
-	return warnings, err
+
+	return warnings, errs
 
 }
 

nomad/job_endpoint_test.go

@@ -1,6 +1,7 @@
 package nomad
 
 import (
+	"errors"
 	"fmt"
 	"reflect"
 	"strings"
@@ -4321,6 +4322,97 @@ func TestJobEndpoint_ImplicitConstraints_Vault(t *testing.T) {
 	}
 }
 
+func TestJobEndpoint_ValidateJob_ConsulConnect(t *testing.T) {
+	t.Parallel()
+
+	s1 := TestServer(t, nil)
+	defer s1.Shutdown()
+	codec := rpcClient(t, s1)
+	testutil.WaitForLeader(t, s1.RPC)
+
+	validateJob := func(j *structs.Job) error {
+		req := &structs.JobRegisterRequest{
+			Job: j,
+			WriteRequest: structs.WriteRequest{
+				Region:    "global",
+				Namespace: j.Namespace,
+			},
+		}
+		var resp structs.JobValidateResponse
+		if err := msgpackrpc.CallWithCodec(codec, "Job.Validate", req, &resp); err != nil {
+			return err
+		}
+
+		if resp.Error != "" {
+			return errors.New(resp.Error)
+		}
+
+		if len(resp.ValidationErrors) != 0 {
+			return errors.New(strings.Join(resp.ValidationErrors, ","))
+		}
+
+		if resp.Warnings != "" {
+			return errors.New(resp.Warnings)
+		}
+
+		return nil
+	}
+
+	tgServices := []*structs.Service{
+		{
+			Name:      "count-api",
+			PortLabel: "9001",
+			Connect: &structs.ConsulConnect{
+				SidecarService: &structs.ConsulSidecarService{},
+			},
+		},
+	}
+
+	t.Run("plain job", func(t *testing.T) {
+		j := mock.Job()
+		require.NoError(t, validateJob(j))
+	})
+	t.Run("valid consul connect", func(t *testing.T) {
+		j := mock.Job()
+
+		tg := j.TaskGroups[0]
+		tg.Services = tgServices
+		tg.Networks = structs.Networks{
+			{Mode: "bridge"},
+		}
+
+		err := validateJob(j)
+		require.NoError(t, err)
+	})
+
+	t.Run("consul connect but missing network", func(t *testing.T) {
+		j := mock.Job()
+
+		tg := j.TaskGroups[0]
+		tg.Services = tgServices
+
+		err := validateJob(j)
+		require.Error(t, err)
+		require.Contains(t, err.Error(), `Consul Connect sidecars require exactly 1 network`)
+	})
+
+	t.Run("consul connect but non bridge network", func(t *testing.T) {
+		j := mock.Job()
+
+		tg := j.TaskGroups[0]
+		tg.Services = tgServices
+
+		tg.Networks = structs.Networks{
+			{Mode: "host"},
+		}
+
+		err := validateJob(j)
+		require.Error(t, err)
+		require.Contains(t, err.Error(), `Consul Connect sidecar requires bridge network, found "host" in group "web"`)
+	})
+
+}
+
 func TestJobEndpoint_ImplicitConstraints_Signals(t *testing.T) {
 	t.Parallel()
 	s1 := TestServer(t, func(c *Config) {