From c467a94e2bbfac4d2467e9777328dc8edba09e4a Mon Sep 17 00:00:00 2001 From: Jasmine Dahilig Date: Thu, 2 May 2019 13:00:21 -0700 Subject: [PATCH] backfill region from job hcl in jobUpdate and jobPlan endpoints - updated region in job metadata that gets persisted to nomad datastore - fixed many unrelated unit tests that used an invalid region value (they previously passed because hcl wasn't getting picked up and the job would default to global region) --- api/compose_test.go | 4 +- api/jobs_test.go | 8 +- api/util_test.go | 2 +- client/client_test.go | 12 +- command/agent/config_test.go | 2 +- command/agent/job_endpoint.go | 18 ++- command/agent/job_endpoint_test.go | 168 +++++++++++++++++++++++++ command/agent/testingutils_test.go | 6 + command/util_test.go | 2 +- internal/testing/apitests/util_test.go | 2 +- nomad/rpc_test.go | 4 +- nomad/serf_test.go | 2 +- nomad/server_test.go | 2 +- 13 files changed, 210 insertions(+), 22 deletions(-) diff --git a/api/compose_test.go b/api/compose_test.go index a624f441591a..1157fcb07f3e 100644 --- a/api/compose_test.go +++ b/api/compose_test.go @@ -37,7 +37,7 @@ func TestCompose(t *testing.T) { AddTask(task) // Compose a job - job := NewServiceJob("job1", "myjob", "region1", 2). + job := NewServiceJob("job1", "myjob", "global", 2). SetMeta("foo", "bar"). AddDatacenter("dc1"). Constrain(NewConstraint("kernel.name", "=", "linux")). @@ -45,7 +45,7 @@ func TestCompose(t *testing.T) { // Check that the composed result looks correct expect := &Job{ - Region: stringToPtr("region1"), + Region: stringToPtr("global"), ID: stringToPtr("job1"), Name: stringToPtr("myjob"), Type: stringToPtr(JobTypeService), diff --git a/api/jobs_test.go b/api/jobs_test.go index e131d49dc377..a1639bdb49cd 100644 --- a/api/jobs_test.go +++ b/api/jobs_test.go @@ -1206,9 +1206,9 @@ func TestJobs_JobSummary(t *testing.T) { func TestJobs_NewBatchJob(t *testing.T) { t.Parallel() - job := NewBatchJob("job1", "myjob", "region1", 5) + job := NewBatchJob("job1", "myjob", "global", 5) expect := &Job{ - Region: stringToPtr("region1"), + Region: stringToPtr("global"), ID: stringToPtr("job1"), Name: stringToPtr("myjob"), Type: stringToPtr(JobTypeBatch), @@ -1221,9 +1221,9 @@ func TestJobs_NewBatchJob(t *testing.T) { func TestJobs_NewServiceJob(t *testing.T) { t.Parallel() - job := NewServiceJob("job1", "myjob", "region1", 5) + job := NewServiceJob("job1", "myjob", "global", 5) expect := &Job{ - Region: stringToPtr("region1"), + Region: stringToPtr("global"), ID: stringToPtr("job1"), Name: stringToPtr("myjob"), Type: stringToPtr(JobTypeService), diff --git a/api/util_test.go b/api/util_test.go index f89cb3945891..9b2358db2f03 100644 --- a/api/util_test.go +++ b/api/util_test.go @@ -39,7 +39,7 @@ func testJob() *Job { SizeMB: intToPtr(25), }) - job := NewBatchJob("job1", "redis", "region1", 1). + job := NewBatchJob("job1", "redis", "global", 1). AddDatacenter("dc1"). AddTaskGroup(group) diff --git a/client/client_test.go b/client/client_test.go index ea034ddf58fb..5491c7cc62a1 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -986,7 +986,7 @@ func TestClient_ReloadTLS_UpgradePlaintextToTLS(t *testing.T) { assert := assert.New(t) s1, addr := testServer(t, func(c *nomad.Config) { - c.Region = "regionFoo" + c.Region = "global" }) defer s1.Shutdown() testutil.WaitForLeader(t, s1.RPC) @@ -1006,7 +1006,7 @@ func TestClient_ReloadTLS_UpgradePlaintextToTLS(t *testing.T) { { req := structs.NodeSpecificRequest{ NodeID: c1.Node().ID, - QueryOptions: structs.QueryOptions{Region: "regionFoo"}, + QueryOptions: structs.QueryOptions{Region: "global"}, } testutil.WaitForResult(func() (bool, error) { @@ -1040,7 +1040,7 @@ func TestClient_ReloadTLS_UpgradePlaintextToTLS(t *testing.T) { { req := structs.NodeSpecificRequest{ NodeID: c1.Node().ID, - QueryOptions: structs.QueryOptions{Region: "regionFoo"}, + QueryOptions: structs.QueryOptions{Region: "global"}, } testutil.WaitForResult(func() (bool, error) { var out structs.SingleNodeResponse @@ -1062,7 +1062,7 @@ func TestClient_ReloadTLS_DowngradeTLSToPlaintext(t *testing.T) { assert := assert.New(t) s1, addr := testServer(t, func(c *nomad.Config) { - c.Region = "regionFoo" + c.Region = "global" }) defer s1.Shutdown() testutil.WaitForLeader(t, s1.RPC) @@ -1091,7 +1091,7 @@ func TestClient_ReloadTLS_DowngradeTLSToPlaintext(t *testing.T) { { req := structs.NodeSpecificRequest{ NodeID: c1.Node().ID, - QueryOptions: structs.QueryOptions{Region: "regionFoo"}, + QueryOptions: structs.QueryOptions{Region: "global"}, } testutil.WaitForResult(func() (bool, error) { var out structs.SingleNodeResponse @@ -1116,7 +1116,7 @@ func TestClient_ReloadTLS_DowngradeTLSToPlaintext(t *testing.T) { { req := structs.NodeSpecificRequest{ NodeID: c1.Node().ID, - QueryOptions: structs.QueryOptions{Region: "regionFoo"}, + QueryOptions: structs.QueryOptions{Region: "global"}, } testutil.WaitForResult(func() (bool, error) { var out structs.SingleNodeResponse diff --git a/command/agent/config_test.go b/command/agent/config_test.go index 7d7128d5f3d9..1da40ce0c067 100644 --- a/command/agent/config_test.go +++ b/command/agent/config_test.go @@ -188,7 +188,7 @@ func TestConfig_Merge(t *testing.T) { } c3 := &Config{ - Region: "region2", + Region: "global", Datacenter: "dc2", NodeName: "node2", DataDir: "/tmp/dir2", diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index e343af649870..5b4f68c212fa 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -8,6 +8,7 @@ import ( "github.com/golang/snappy" "github.com/hashicorp/nomad/api" + "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/jobspec" "github.com/hashicorp/nomad/nomad/structs" ) @@ -140,13 +141,20 @@ func (s *HTTPServer) jobPlan(resp http.ResponseWriter, req *http.Request, return nil, CodedError(400, "Job ID does not match") } + // Http region takes precedence over hcl region + if args.WriteRequest.Region != "" { + args.Job.Region = helper.StringToPtr(args.WriteRequest.Region) + } + + // If no region given, region is canonicalized to 'global' sJob := ApiJobToStructJob(args.Job) + planReq := structs.JobPlanRequest{ Job: sJob, Diff: args.Diff, PolicyOverride: args.PolicyOverride, WriteRequest: structs.WriteRequest{ - Region: args.WriteRequest.Region, + Region: sJob.Region, }, } s.parseWriteRequest(req, &planReq.WriteRequest) @@ -374,6 +382,12 @@ func (s *HTTPServer) jobUpdate(resp http.ResponseWriter, req *http.Request, return nil, CodedError(400, "Job ID does not match name") } + // Http region takes precedence over hcl region + if args.WriteRequest.Region != "" { + args.Job.Region = helper.StringToPtr(args.WriteRequest.Region) + } + + // If no region given, region is canonicalized to 'global' sJob := ApiJobToStructJob(args.Job) regReq := structs.JobRegisterRequest{ @@ -382,7 +396,7 @@ func (s *HTTPServer) jobUpdate(resp http.ResponseWriter, req *http.Request, JobModifyIndex: args.JobModifyIndex, PolicyOverride: args.PolicyOverride, WriteRequest: structs.WriteRequest{ - Region: args.WriteRequest.Region, + Region: sJob.Region, AuthToken: args.WriteRequest.SecretID, }, } diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index f93396939b5c..9b2f038fc5e9 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -465,6 +465,99 @@ func TestHTTP_JobUpdate(t *testing.T) { }) } +func TestHTTP_JobUpdateRegion(t *testing.T) { + t.Parallel() + + cases := []struct { + Name string + ConfigRegion string + APIRegion string + ExpectedRegion string + }{ + { + Name: "api region takes precedence", + ConfigRegion: "not-global", + APIRegion: "north-america", + ExpectedRegion: "north-america", + }, + { + Name: "config region is set", + ConfigRegion: "north-america", + APIRegion: "", + ExpectedRegion: "north-america", + }, + { + Name: "api region is set", + ConfigRegion: "", + APIRegion: "north-america", + ExpectedRegion: "north-america", + }, + { + Name: "falls back to default if no region is provided", + ConfigRegion: "", + APIRegion: "", + ExpectedRegion: "global", + }, + } + + for _, tc := range cases { + t.Run(tc.Name, func(t *testing.T) { + httpTest(t, func(c *Config) { c.Region = tc.ExpectedRegion }, func(s *TestAgent) { + // Create the job + job := MockRegionalJob() + + if tc.ConfigRegion == "" { + job.Region = nil + } else { + job.Region = &tc.ConfigRegion + } + + args := api.JobRegisterRequest{ + Job: job, + WriteRequest: api.WriteRequest{ + Namespace: api.DefaultNamespace, + Region: tc.APIRegion, + }, + } + + buf := encodeReq(args) + + // Make the HTTP request + url := "/v1/job/" + *job.ID + + req, err := http.NewRequest("PUT", url, buf) + require.NoError(t, err) + respW := httptest.NewRecorder() + + // Make the request + obj, err := s.Server.JobSpecificRequest(respW, req) + require.NoError(t, err) + + // Check the response + dereg := obj.(structs.JobRegisterResponse) + require.NotEmpty(t, dereg.EvalID) + + // Check for the index + require.NotEmpty(t, respW.HeaderMap.Get("X-Nomad-Index"), "missing index") + + // Check the job is registered + getReq := structs.JobSpecificRequest{ + JobID: *job.ID, + QueryOptions: structs.QueryOptions{ + Region: tc.ExpectedRegion, + Namespace: structs.DefaultNamespace, + }, + } + var getResp structs.SingleJobResponse + err = s.Agent.RPC("Job.GetJob", &getReq, &getResp) + require.NoError(t, err) + require.NotNil(t, getResp.Job, "job does not exist") + require.Equal(t, tc.ExpectedRegion, getResp.Job.Region) + }) + }) + } +} + func TestHTTP_JobDelete(t *testing.T) { t.Parallel() httpTest(t, nil, func(s *TestAgent) { @@ -1022,6 +1115,81 @@ func TestHTTP_JobPlan(t *testing.T) { }) } +func TestHTTP_JobPlanRegion(t *testing.T) { + t.Parallel() + + cases := []struct { + Name string + ConfigRegion string + APIRegion string + ExpectedRegion string + }{ + { + Name: "api region takes precedence", + ConfigRegion: "not-global", + APIRegion: "north-america", + ExpectedRegion: "north-america", + }, + { + Name: "config region is set", + ConfigRegion: "north-america", + APIRegion: "", + ExpectedRegion: "north-america", + }, + { + Name: "api region is set", + ConfigRegion: "", + APIRegion: "north-america", + ExpectedRegion: "north-america", + }, + { + Name: "falls back to default if no region is provided", + ConfigRegion: "", + APIRegion: "", + ExpectedRegion: "global", + }, + } + + for _, tc := range cases { + t.Run(tc.Name, func(t *testing.T) { + httpTest(t, func(c *Config) { c.Region = tc.ExpectedRegion }, func(s *TestAgent) { + // Create the job + job := MockRegionalJob() + + if tc.ConfigRegion == "" { + job.Region = nil + } else { + job.Region = &tc.ConfigRegion + } + + args := api.JobPlanRequest{ + Job: job, + Diff: true, + WriteRequest: api.WriteRequest{ + Region: tc.APIRegion, + Namespace: api.DefaultNamespace, + }, + } + buf := encodeReq(args) + + // Make the HTTP request + req, err := http.NewRequest("PUT", "/v1/job/"+*job.ID+"/plan", buf) + require.NoError(t, err) + respW := httptest.NewRecorder() + + // Make the request + obj, err := s.Server.JobSpecificRequest(respW, req) + require.NoError(t, err) + + // Check the response + plan := obj.(structs.JobPlanResponse) + require.NotNil(t, plan.Annotations) + require.NotNil(t, plan.Diff) + }) + }) + } +} + func TestHTTP_JobDispatch(t *testing.T) { t.Parallel() httpTest(t, nil, func(s *TestAgent) { diff --git a/command/agent/testingutils_test.go b/command/agent/testingutils_test.go index 63fc2603f787..3d821cd597a7 100644 --- a/command/agent/testingutils_test.go +++ b/command/agent/testingutils_test.go @@ -109,3 +109,9 @@ func MockPeriodicJob() *api.Job { } return j } + +func MockRegionalJob() *api.Job { + j := MockJob() + j.Region = helper.StringToPtr("north-america") + return j +} diff --git a/command/util_test.go b/command/util_test.go index c3f7a7fcac5f..a989e532eea6 100644 --- a/command/util_test.go +++ b/command/util_test.go @@ -42,7 +42,7 @@ func testJob(jobID string) *api.Job { SizeMB: helper.IntToPtr(20), }) - job := api.NewBatchJob(jobID, jobID, "region1", 1). + job := api.NewBatchJob(jobID, jobID, "global", 1). AddDatacenter("dc1"). AddTaskGroup(group) diff --git a/internal/testing/apitests/util_test.go b/internal/testing/apitests/util_test.go index 62941d36e7f5..d751c04eb2cd 100644 --- a/internal/testing/apitests/util_test.go +++ b/internal/testing/apitests/util_test.go @@ -62,7 +62,7 @@ func testJob() *api.Job { SizeMB: intToPtr(25), }) - job := api.NewBatchJob("job1", "redis", "region1", 1). + job := api.NewBatchJob("job1", "redis", "global", 1). AddDatacenter("dc1"). AddTaskGroup(group) diff --git a/nomad/rpc_test.go b/nomad/rpc_test.go index 0c05667ddc0e..a9413e525b74 100644 --- a/nomad/rpc_test.go +++ b/nomad/rpc_test.go @@ -79,7 +79,7 @@ func TestRPC_forwardRegion(t *testing.T) { s1 := TestServer(t, nil) defer s1.Shutdown() s2 := TestServer(t, func(c *Config) { - c.Region = "region2" + c.Region = "global" }) defer s2.Shutdown() TestJoin(t, s1, s2) @@ -87,7 +87,7 @@ func TestRPC_forwardRegion(t *testing.T) { testutil.WaitForLeader(t, s2.RPC) var out struct{} - err := s1.forwardRegion("region2", "Status.Ping", struct{}{}, &out) + err := s1.forwardRegion("global", "Status.Ping", struct{}{}, &out) if err != nil { t.Fatalf("err: %v", err) } diff --git a/nomad/serf_test.go b/nomad/serf_test.go index 2dbae85e41da..20fbe96c75e1 100644 --- a/nomad/serf_test.go +++ b/nomad/serf_test.go @@ -59,7 +59,7 @@ func TestNomad_RemovePeer(t *testing.T) { s1 := TestServer(t, nil) defer s1.Shutdown() s2 := TestServer(t, func(c *Config) { - c.Region = "region2" + c.Region = "global" }) defer s2.Shutdown() TestJoin(t, s1, s2) diff --git a/nomad/server_test.go b/nomad/server_test.go index c3d057f43ef8..2761b80347e0 100644 --- a/nomad/server_test.go +++ b/nomad/server_test.go @@ -210,7 +210,7 @@ func TestServer_Regions(t *testing.T) { func TestServer_Reload_Vault(t *testing.T) { t.Parallel() s1 := TestServer(t, func(c *Config) { - c.Region = "region1" + c.Region = "global" }) defer s1.Shutdown()