fix region flag vs job region handling in plan/submit (#8347)

command/agent/job_endpoint.go

@@ -152,34 +152,13 @@ func (s *HTTPServer) jobPlan(resp http.ResponseWriter, req *http.Request,
 		}
 	}
 
-	var region *string
-
-	// Region in http request query param takes precedence over region in job hcl config
-	if args.WriteRequest.Region != "" {
-		region = helper.StringToPtr(args.WriteRequest.Region)
-	}
-	// If 'global' region is specified or if no region is given,
-	// default to region of the node you're submitting to
-	if region == nil || args.Job.Region == nil ||
-		*args.Job.Region == "" || *args.Job.Region == api.GlobalRegion {
-		region = &s.agent.config.Region
-	}
-
-	args.Job.Region = regionForJob(args.Job, region)
-	sJob := ApiJobToStructJob(args.Job)
-
+	sJob, writeReq := s.apiJobAndRequestToStructs(args.Job, req, args.WriteRequest)
 	planReq := structs.JobPlanRequest{
 		Job:            sJob,
 		Diff:           args.Diff,
 		PolicyOverride: args.PolicyOverride,
-		WriteRequest: structs.WriteRequest{
-			Region: *region,
-		},
+		WriteRequest:   *writeReq,
 	}
-	// parseWriteRequest overrides Namespace, Region and AuthToken
-	// based on values from the original http request
-	s.parseWriteRequest(req, &planReq.WriteRequest)
-	planReq.Namespace = sJob.Namespace
 
 	var out structs.JobPlanResponse
 	if err := s.agent.RPC("Job.Plan", &planReq, &out); err != nil {
@@ -412,37 +391,15 @@ func (s *HTTPServer) jobUpdate(resp http.ResponseWriter, req *http.Request,
 		}
 	}
 
-	var region *string
-
-	// Region in http request query param takes precedence over region in job hcl config
-	if args.WriteRequest.Region != "" {
-		region = helper.StringToPtr(args.WriteRequest.Region)
-	}
-	// If 'global' region is specified or if no region is given,
-	// default to region of the node you're submitting to
-	if region == nil || args.Job.Region == nil ||
-		*args.Job.Region == "" || *args.Job.Region == api.GlobalRegion {
-		region = &s.agent.config.Region
-	}
-
-	args.Job.Region = regionForJob(args.Job, region)
-	sJob := ApiJobToStructJob(args.Job)
-
+	sJob, writeReq := s.apiJobAndRequestToStructs(args.Job, req, args.WriteRequest)
 	regReq := structs.JobRegisterRequest{
 		Job:            sJob,
 		EnforceIndex:   args.EnforceIndex,
 		JobModifyIndex: args.JobModifyIndex,
 		PolicyOverride: args.PolicyOverride,
 		PreserveCounts: args.PreserveCounts,
-		WriteRequest: structs.WriteRequest{
-			Region:    *region,
-			AuthToken: args.WriteRequest.SecretID,
-		},
+		WriteRequest:   *writeReq,
 	}
-	// parseWriteRequest overrides Namespace, Region and AuthToken
-	// based on values from the original http request
-	s.parseWriteRequest(req, &regReq.WriteRequest)
-	regReq.Namespace = sJob.Namespace
 
 	var out structs.JobRegisterResponse
 	if err := s.agent.RPC("Job.Register", &regReq, &out); err != nil {
@@ -719,6 +676,87 @@ func (s *HTTPServer) JobsParseRequest(resp http.ResponseWriter, req *http.Reques
 	return jobStruct, nil
 }
 
+// apiJobAndRequestToStructs parses the query params from the incoming
+// request and converts to a structs.Job and WriteRequest with the
+func (s *HTTPServer) apiJobAndRequestToStructs(job *api.Job, req *http.Request, apiReq api.WriteRequest) (*structs.Job, *structs.WriteRequest) {
+
+	// parseWriteRequest gets the Namespace, Region, and AuthToken from
+	// the original HTTP request's query params and headers and overrides
+	// those values set in the request body
+	writeReq := &structs.WriteRequest{
+		Namespace: apiReq.Namespace,
+		Region:    apiReq.Region,
+		AuthToken: apiReq.SecretID,
+	}
+
+	queryRegion := req.URL.Query().Get("region")
+	s.parseToken(req, &writeReq.AuthToken)
+	parseNamespace(req, &writeReq.Namespace)
+
+	requestRegion, jobRegion := regionForJob(
+		job, queryRegion, writeReq.Region, s.agent.config.Region,
+	)
+
+	sJob := ApiJobToStructJob(job)
+	sJob.Region = jobRegion
+	writeReq.Region = requestRegion
+	writeReq.Namespace = sJob.Namespace
+
+	return sJob, writeReq
+}
+
+func regionForJob(job *api.Job, queryRegion, apiRegion, agentRegion string) (string, string) {
+	var requestRegion string
+	var jobRegion string
+
+	// Region in query param (-region flag) takes precedence.
+	if queryRegion != "" {
+		requestRegion = queryRegion
+		jobRegion = queryRegion
+	}
+
+	// Next the request body...
+	if apiRegion != "" {
+		requestRegion = apiRegion
+		jobRegion = apiRegion
+	}
+
+	// If no query param was passed, we forward to the job's region
+	// if one is available
+	if requestRegion == "" && job.Region != nil {
+		requestRegion = *job.Region
+		jobRegion = *job.Region
+	}
+
+	// otherwise we default to the region of this node
+	if requestRegion == "" || requestRegion == api.GlobalRegion {
+		requestRegion = agentRegion
+		jobRegion = agentRegion
+	}
+
+	// Multiregion jobs have their job region set to the global region,
+	// and enforce that we forward to a region where they will be deployed
+	if job.Multiregion != nil {
+		jobRegion = api.GlobalRegion
+
+		// multiregion jobs with 0 regions won't pass validation,
+		// but this protects us from NPE
+		if len(job.Multiregion.Regions) > 0 {
+			found := false
+			for _, region := range job.Multiregion.Regions {
+				if region.Name == requestRegion {
+					found = true
+				}
+			}
+			if !found {
+				requestRegion = job.Multiregion.Regions[0].Name
+			}
+		}
+	}
+
+	return requestRegion, jobRegion
+}
+
 func ApiJobToStructJob(job *api.Job) *structs.Job {
 	job.Canonicalize()
 

command/agent/job_endpoint_oss.go

@@ -1,11 +0,0 @@
-// +build !ent
-
-package agent
-
-import (
-	"github.com/hashicorp/nomad/api"
-)
-
-func regionForJob(job *api.Job, requestRegion *string) *string {
-	return requestRegion
-}

command/agent/job_endpoint_test.go

@@ -1510,6 +1510,249 @@ func TestHTTP_JobStable(t *testing.T) {
 	})
 }
 
+func TestJobs_ParsingWriteRequest(t *testing.T) {
+	t.Parallel()
+
+	// defaults
+	agentRegion := "agentRegion"
+
+	cases := []struct {
+		name                  string
+		jobRegion             string
+		multiregion           *api.Multiregion
+		queryRegion           string
+		queryNamespace        string
+		queryToken            string
+		apiRegion             string
+		apiNamespace          string
+		apiToken              string
+		expectedRequestRegion string
+		expectedJobRegion     string
+		expectedToken         string
+		expectedNamespace     string
+	}{
+		{
+			name:                  "no region provided at all",
+			jobRegion:             "",
+			multiregion:           nil,
+			queryRegion:           "",
+			expectedRequestRegion: agentRegion,
+			expectedJobRegion:     agentRegion,
+			expectedToken:         "",
+			expectedNamespace:     "default",
+		},
+		{
+			name:                  "no region provided but multiregion safe",
+			jobRegion:             "",
+			multiregion:           &api.Multiregion{},
+			queryRegion:           "",
+			expectedRequestRegion: agentRegion,
+			expectedJobRegion:     api.GlobalRegion,
+			expectedToken:         "",
+			expectedNamespace:     "default",
+		},
+		{
+			name:                  "region flag provided",
+			jobRegion:             "",
+			multiregion:           nil,
+			queryRegion:           "west",
+			expectedRequestRegion: "west",
+			expectedJobRegion:     "west",
+			expectedToken:         "",
+			expectedNamespace:     "default",
+		},
+		{
+			name:                  "job region provided",
+			jobRegion:             "west",
+			multiregion:           nil,
+			queryRegion:           "",
+			expectedRequestRegion: "west",
+			expectedJobRegion:     "west",
+			expectedToken:         "",
+			expectedNamespace:     "default",
+		},
+		{
+			name:                  "job region overridden by region flag",
+			jobRegion:             "west",
+			multiregion:           nil,
+			queryRegion:           "east",
+			expectedRequestRegion: "east",
+			expectedJobRegion:     "east",
+			expectedToken:         "",
+			expectedNamespace:     "default",
+		},
+		{
+			name:      "multiregion to valid region",
+			jobRegion: "",
+			multiregion: &api.Multiregion{Regions: []*api.MultiregionRegion{
+				{Name: "west"},
+				{Name: "east"},
+			}},
+			queryRegion:           "east",
+			expectedRequestRegion: "east",
+			expectedJobRegion:     api.GlobalRegion,
+			expectedToken:         "",
+			expectedNamespace:     "default",
+		},
+		{
+			name:      "multiregion sent to wrong region",
+			jobRegion: "",
+			multiregion: &api.Multiregion{Regions: []*api.MultiregionRegion{
+				{Name: "west"},
+				{Name: "east"},
+			}},
+			queryRegion:           "north",
+			expectedRequestRegion: "west",
+			expectedJobRegion:     api.GlobalRegion,
+			expectedToken:         "",
+			expectedNamespace:     "default",
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+
+			// we need a valid agent config but we don't want to start up
+			// a real server for this
+			srv := &HTTPServer{}
+			srv.agent = &Agent{config: &Config{Region: agentRegion}}
+
+			job := &api.Job{
+				Region:      helper.StringToPtr(tc.jobRegion),
+				Multiregion: tc.multiregion,
+			}
+
+			req, _ := http.NewRequest("POST", "/", nil)
+			if tc.queryToken != "" {
+				req.Header.Set("X-Nomad-Token", tc.queryToken)
+			}
+			q := req.URL.Query()
+			if tc.queryNamespace != "" {
+				q.Add("namespace", tc.queryNamespace)
+			}
+			if tc.queryRegion != "" {
+				q.Add("region", tc.queryRegion)
+			}
+			req.URL.RawQuery = q.Encode()
+
+			apiReq := api.WriteRequest{
+				Region:    tc.apiRegion,
+				Namespace: tc.apiNamespace,
+				SecretID:  tc.apiToken,
+			}
+
+			sJob, sWriteReq := srv.apiJobAndRequestToStructs(job, req, apiReq)
+			require.Equal(t, tc.expectedJobRegion, sJob.Region)
+			require.Equal(t, tc.expectedNamespace, sJob.Namespace)
+			require.Equal(t, tc.expectedNamespace, sWriteReq.Namespace)
+			require.Equal(t, tc.expectedRequestRegion, sWriteReq.Region)
+			require.Equal(t, tc.expectedToken, sWriteReq.AuthToken)
+		})
+	}
+}
+
+func TestJobs_RegionForJob(t *testing.T) {
+	t.Parallel()
+
+	// defaults
+	agentRegion := "agentRegion"
+
+	cases := []struct {
+		name                  string
+		jobRegion             string
+		multiregion           *api.Multiregion
+		queryRegion           string
+		apiRegion             string
+		agentRegion           string
+		expectedRequestRegion string
+		expectedJobRegion     string
+	}{
+		{
+			name:                  "no region provided",
+			jobRegion:             "",
+			multiregion:           nil,
+			queryRegion:           "",
+			expectedRequestRegion: agentRegion,
+			expectedJobRegion:     agentRegion,
+		},
+		{
+			name:                  "no region provided but multiregion safe",
+			jobRegion:             "",
+			multiregion:           &api.Multiregion{},
+			queryRegion:           "",
+			expectedRequestRegion: agentRegion,
+			expectedJobRegion:     api.GlobalRegion,
+		},
+		{
+			name:                  "region flag provided",
+			jobRegion:             "",
+			multiregion:           nil,
+			queryRegion:           "west",
+			expectedRequestRegion: "west",
+			expectedJobRegion:     "west",
+		},
+		{
+			name:                  "job region provided",
+			jobRegion:             "west",
+			multiregion:           nil,
+			queryRegion:           "",
+			expectedRequestRegion: "west",
+			expectedJobRegion:     "west",
+		},
+		{
+			name:                  "job region overridden by region flag",
+			jobRegion:             "west",
+			multiregion:           nil,
+			queryRegion:           "east",
+			expectedRequestRegion: "east",
+			expectedJobRegion:     "east",
+		},
+		{
+			name:                  "job region overridden by api body",
+			jobRegion:             "west",
+			multiregion:           nil,
+			apiRegion:             "east",
+			expectedRequestRegion: "east",
+			expectedJobRegion:     "east",
+		},
+		{
+			name:      "multiregion to valid region",
+			jobRegion: "",
+			multiregion: &api.Multiregion{Regions: []*api.MultiregionRegion{
+				{Name: "west"},
+				{Name: "east"},
+			}},
+			queryRegion:           "east",
+			expectedRequestRegion: "east",
+			expectedJobRegion:     api.GlobalRegion,
+		},
+		{
+			name:      "multiregion sent to wrong region",
+			jobRegion: "",
+			multiregion: &api.Multiregion{Regions: []*api.MultiregionRegion{
+				{Name: "west"},
+				{Name: "east"},
+			}},
+			queryRegion:           "north",
+			expectedRequestRegion: "west",
+			expectedJobRegion:     api.GlobalRegion,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			job := &api.Job{
+				Region:      helper.StringToPtr(tc.jobRegion),
+				Multiregion: tc.multiregion,
+			}
+			requestRegion, jobRegion := regionForJob(
+				job, tc.queryRegion, tc.apiRegion, agentRegion)
+			require.Equal(t, tc.expectedRequestRegion, requestRegion)
+			require.Equal(t, tc.expectedJobRegion, jobRegion)
+		})
+	}
+}
+
 func TestJobs_ApiJobToStructsJob(t *testing.T) {
 	apiJob := &api.Job{
 		Stop:        helper.BoolToPtr(true),