From ca7ead191e00db483f6d42afe878eeee206c1d7b Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Wed, 1 Feb 2023 14:02:45 -0600 Subject: [PATCH] consul: restore consul token when reverting a job (#15996) * consul: reset consul token on job during registration of a reversion * e2e: add test for reverting a job with a consul service * cl: fixup cl entry --- .changelog/15996.txt | 3 + e2e/consul/input/service_reversion.nomad | 36 ++++++++++ e2e/consul/service_revert_test.go | 85 ++++++++++++++++++++++++ e2e/e2eutil/job.go | 24 +++++-- nomad/job_endpoint.go | 4 +- 5 files changed, 143 insertions(+), 9 deletions(-) create mode 100644 .changelog/15996.txt create mode 100644 e2e/consul/input/service_reversion.nomad create mode 100644 e2e/consul/service_revert_test.go diff --git a/.changelog/15996.txt b/.changelog/15996.txt new file mode 100644 index 000000000..5e3bc839d --- /dev/null +++ b/.changelog/15996.txt @@ -0,0 +1,3 @@ +```release-note:bug +consul: Fixed a bug where consul token was not respected when reverting a job +``` diff --git a/e2e/consul/input/service_reversion.nomad b/e2e/consul/input/service_reversion.nomad new file mode 100644 index 000000000..5032e5c6c --- /dev/null +++ b/e2e/consul/input/service_reversion.nomad @@ -0,0 +1,36 @@ +variable "service" { + type = string +} + +job "service-reversion" { + datacenters = ["dc1"] + type = "service" + + constraint { + attribute = "${attr.kernel.name}" + value = "linux" + } + + group "sleep" { + + service { + name = "${var.service}" + } + + task "busybox" { + driver = "docker" + + config { + image = "busybox:1" + command = "sleep" + args = ["infinity"] + } + + resources { + cpu = 16 + memory = 32 + disk = 64 + } + } + } +} \ No newline at end of file diff --git a/e2e/consul/service_revert_test.go b/e2e/consul/service_revert_test.go new file mode 100644 index 000000000..2accd1a51 --- /dev/null +++ b/e2e/consul/service_revert_test.go @@ -0,0 +1,85 @@ +package consul + +import ( + "context" + "testing" + + "github.com/hashicorp/nomad/e2e/e2eutil" + "github.com/hashicorp/nomad/helper/uuid" + "github.com/hashicorp/nomad/nomad/structs" + "github.com/shoenig/test/must" +) + +func TestConsul(t *testing.T) { + // todo: migrate the remaining consul tests + + nomad := e2eutil.NomadClient(t) + + e2eutil.WaitForLeader(t, nomad) + e2eutil.WaitForNodesReady(t, nomad, 1) + + t.Run("testServiceReversion", testServiceReversion) +} + +// testServiceReversion asserts we can +// - submit a job with a service +// - update that job and modify service +// - revert the job, restoring the original service +func testServiceReversion(t *testing.T) { + const jobFile = "./input/service_reversion.nomad" + jobID := "service-reversion-" + uuid.Short() + jobIDs := []string{jobID} + + // Defer a cleanup function to remove the job. This will trigger if the + // test fails, unless the cancel function is called. + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + defer e2eutil.CleanupJobsAndGCWithContext(t, ctx, &jobIDs) + + // initial register of job, with service="one" + vars := []string{"-var", "service=one"} + err := e2eutil.RegisterWithArgs(jobID, jobFile, vars...) + must.NoError(t, err) + + // wait for job to be running + err = e2eutil.WaitForAllocStatusExpected(jobID, "", []string{structs.AllocClientStatusRunning}) + must.NoError(t, err) + + // get our consul client + consulClient := e2eutil.ConsulClient(t) + + assertService := func(name string, count int) { + services, _, consulErr := consulClient.Catalog().Service(name, "", nil) + must.NoError(t, consulErr) + must.Len(t, count, services, must.Sprintf("expected %d instances of %s, got %d", count, name, len(services))) + } + + // query services, assert 1 instance of "one" + assertService("one", 1) + assertService("two", 0) + + // second register of job, with service="two" + vars = []string{"-var", "service=two"} + err = e2eutil.RegisterWithArgs(jobID, jobFile, vars...) + must.NoError(t, err) + + // wait for job to be running + err = e2eutil.WaitForAllocStatusExpected(jobID, "", []string{structs.AllocClientStatusRunning}) + must.NoError(t, err) + + // query services, assert 0 instance of "one" (replaced), 1 of "two" + assertService("one", 0) + assertService("two", 1) + + // now revert our job back to version 0 + err = e2eutil.Revert(jobID, jobFile, 0) + must.NoError(t, err) + + // wait for job to be running + err = e2eutil.WaitForAllocStatusExpected(jobID, "", []string{structs.AllocClientStatusRunning}) + must.NoError(t, err) + + // query services, assert 1 instance of "one" (reverted), 1 of "two" (removed) + assertService("one", 1) + assertService("two", 0) +} diff --git a/e2e/e2eutil/job.go b/e2e/e2eutil/job.go index 16bd035c1..316ace35a 100644 --- a/e2e/e2eutil/job.go +++ b/e2e/e2eutil/job.go @@ -7,6 +7,7 @@ import ( "io/ioutil" "os/exec" "regexp" + "strconv" "strings" "testing" "time" @@ -19,7 +20,7 @@ import ( func Register(jobID, jobFilePath string) error { ctx, cancel := context.WithTimeout(context.Background(), time.Second*10) defer cancel() - return register(jobID, jobFilePath, exec.CommandContext(ctx, "nomad", "job", "run", "-detach", "-")) + return execCmd(jobID, jobFilePath, exec.CommandContext(ctx, "nomad", "job", "run", "-detach", "-")) } // RegisterWithArgs registers a jobspec from a file but with a unique ID. The @@ -34,11 +35,18 @@ func RegisterWithArgs(jobID, jobFilePath string, args ...string) error { baseArgs = append(baseArgs, "-") ctx, cancel := context.WithTimeout(context.Background(), time.Second*10) defer cancel() - - return register(jobID, jobFilePath, exec.CommandContext(ctx, "nomad", baseArgs...)) + return execCmd(jobID, jobFilePath, exec.CommandContext(ctx, "nomad", baseArgs...)) } -func register(jobID, jobFilePath string, cmd *exec.Cmd) error { +// Revert reverts the job to the given version. +func Revert(jobID, jobFilePath string, version int) error { + args := []string{"job", "revert", "-detach", jobID, strconv.Itoa(version)} + ctx, cancel := context.WithTimeout(context.Background(), time.Second*10) + defer cancel() + return execCmd(jobID, jobFilePath, exec.CommandContext(ctx, "nomad", args...)) +} + +func execCmd(jobID, jobFilePath string, cmd *exec.Cmd) error { stdin, err := cmd.StdinPipe() if err != nil { return fmt.Errorf("could not open stdin?: %w", err) @@ -49,14 +57,16 @@ func register(jobID, jobFilePath string, cmd *exec.Cmd) error { return fmt.Errorf("could not open job file: %w", err) } - // hack off the first line to replace with our unique ID + // hack off the job block to replace with our unique ID var re = regexp.MustCompile(`(?m)^job ".*" \{`) jobspec := re.ReplaceAllString(string(content), fmt.Sprintf("job \"%s\" {", jobID)) go func() { - defer stdin.Close() - io.WriteString(stdin, jobspec) + defer func() { + _ = stdin.Close() + }() + _, _ = io.WriteString(stdin, jobspec) }() out, err := cmd.CombinedOutput() diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go index 13d725396..1376d9c77 100644 --- a/nomad/job_endpoint.go +++ b/nomad/job_endpoint.go @@ -643,8 +643,8 @@ func (j *Job) Revert(args *structs.JobRevertRequest, reply *structs.JobRegisterR // Build the register request revJob := jobV.Copy() - // Use Vault Token from revert request to perform registration of reverted job. - revJob.VaultToken = args.VaultToken + revJob.VaultToken = args.VaultToken // use vault token from revert to perform (re)registration + revJob.ConsulToken = args.ConsulToken // use consul token from revert to perform (re)registration reg := &structs.JobRegisterRequest{ Job: revJob, WriteRequest: args.WriteRequest,