diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index bcf7c5be2..6287876a6 100644 --- a/command/agent/job_endpoint.go +++ b/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, ®Req.WriteRequest) - regReq.Namespace = sJob.Namespace var out structs.JobRegisterResponse if err := s.agent.RPC("Job.Register", ®Req, &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() diff --git a/command/agent/job_endpoint_oss.go b/command/agent/job_endpoint_oss.go deleted file mode 100644 index 58d34d5b1..000000000 --- a/command/agent/job_endpoint_oss.go +++ /dev/null @@ -1,11 +0,0 @@ -// +build !ent - -package agent - -import ( - "github.com/hashicorp/nomad/api" -) - -func regionForJob(job *api.Job, requestRegion *string) *string { - return requestRegion -} diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index 797e51094..3f4227b68 100644 --- a/command/agent/job_endpoint_test.go +++ b/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),