From 6f0c9696ec4e8b08428f2b61ac520e7cef060f19 Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Fri, 10 Nov 2017 11:08:19 -0800 Subject: [PATCH 1/3] Handle multiple environment templates Fixes https://github.com/hashicorp/nomad/issues/3498 --- client/consul_template.go | 4 +- client/consul_template_test.go | 80 ++++++++++++++++++++++++++++++++++ client/driver/env/env.go | 12 ++++- 3 files changed, 92 insertions(+), 4 deletions(-) diff --git a/client/consul_template.go b/client/consul_template.go index 52dd718e53f..2ea4855bf94 100644 --- a/client/consul_template.go +++ b/client/consul_template.go @@ -218,7 +218,7 @@ func (tm *TaskTemplateManager) run() { tm.config.Hooks.Kill(consulTemplateSourceName, err.Error(), true) return } - tm.config.EnvBuilder.SetTemplateEnv(envMap) + tm.config.EnvBuilder.MergeTemplateEnv(envMap) // Unblock the task tm.config.Hooks.UnblockStart(consulTemplateSourceName) @@ -394,7 +394,7 @@ func (tm *TaskTemplateManager) handleTemplateRerenders(allRenderedTime time.Time tm.config.Hooks.Kill(consulTemplateSourceName, err.Error(), true) return } - tm.config.EnvBuilder.SetTemplateEnv(envMap) + tm.config.EnvBuilder.MergeTemplateEnv(envMap) for _, tmpl := range tmpls { switch tmpl.ChangeMode { diff --git a/client/consul_template_test.go b/client/consul_template_test.go index f8368520407..c43782c3e50 100644 --- a/client/consul_template_test.go +++ b/client/consul_template_test.go @@ -1112,6 +1112,86 @@ func TestTaskTemplateManager_Env_Multi(t *testing.T) { } } +func TestTaskTemplateManager_Rerender_Env(t *testing.T) { + t.Parallel() + // Make a template that renders based on a key in Consul and sends restart + key1 := "bam" + key2 := "bar" + content1_1 := "cat" + content1_2 := "dog" + t1 := &structs.Template{ + EmbeddedTmpl: ` +FOO={{key "bam"}} +`, + DestPath: "test.env", + ChangeMode: structs.TemplateChangeModeRestart, + Envvars: true, + } + t2 := &structs.Template{ + EmbeddedTmpl: ` +BAR={{key "bar"}} +`, + DestPath: "test2.env", + ChangeMode: structs.TemplateChangeModeRestart, + Envvars: true, + } + + harness := newTestHarness(t, []*structs.Template{t1, t2}, true, false) + harness.start(t) + defer harness.stop() + + // Ensure no unblock + select { + case <-harness.mockHooks.UnblockCh: + t.Fatalf("Task unblock should have not have been called") + case <-time.After(time.Duration(1*testutil.TestMultiplier()) * time.Second): + } + + // Write the key to Consul + harness.consul.SetKV(t, key1, []byte(content1_1)) + harness.consul.SetKV(t, key2, []byte(content1_1)) + + // Wait for the unblock + select { + case <-harness.mockHooks.UnblockCh: + case <-time.After(time.Duration(5*testutil.TestMultiplier()) * time.Second): + t.Fatalf("Task unblock should have been called") + } + + env := harness.envBuilder.Build().Map() + if v, ok := env["FOO"]; !ok || v != content1_1 { + t.Fatalf("Bad env for FOO: %v %v", v, ok) + } + if v, ok := env["BAR"]; !ok || v != content1_1 { + t.Fatalf("Bad env for BAR: %v %v", v, ok) + } + + // Update the keys in Consul + harness.consul.SetKV(t, key1, []byte(content1_2)) + + // Wait for restart + timeout := time.After(time.Duration(1*testutil.TestMultiplier()) * time.Second) +OUTER: + for { + select { + case <-harness.mockHooks.RestartCh: + break OUTER + case <-harness.mockHooks.SignalCh: + t.Fatalf("Signal with restart policy: %+v", harness.mockHooks) + case <-timeout: + t.Fatalf("Should have received a restart: %+v", harness.mockHooks) + } + } + + env = harness.envBuilder.Build().Map() + if v, ok := env["FOO"]; !ok || v != content1_2 { + t.Fatalf("Bad env for FOO: %v %v", v, ok) + } + if v, ok := env["BAR"]; !ok || v != content1_1 { + t.Fatalf("Bad env for BAR: %v %v", v, ok) + } +} + // TestTaskTemplateManager_Config_ServerName asserts the tls_server_name // setting is propogated to consul-template's configuration. See #2776 func TestTaskTemplateManager_Config_ServerName(t *testing.T) { diff --git a/client/driver/env/env.go b/client/driver/env/env.go index 9e63accbc0a..2e05653fb4d 100644 --- a/client/driver/env/env.go +++ b/client/driver/env/env.go @@ -532,9 +532,17 @@ func (b *Builder) SetHostEnvvars(filter []string) *Builder { return b } -func (b *Builder) SetTemplateEnv(m map[string]string) *Builder { +// MergeTemplateEnv is used to merge the passed environment variables with +// existing environment variables set from a template. +func (b *Builder) MergeTemplateEnv(m map[string]string) *Builder { b.mu.Lock() - b.templateEnv = m + if b.templateEnv == nil { + b.templateEnv = m + } else { + for k, v := range m { + b.templateEnv[k] = v + } + } b.mu.Unlock() return b } From d93ecb90ebf4f0a911aba4c889b5044847826f8e Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Fri, 10 Nov 2017 11:10:34 -0800 Subject: [PATCH 2/3] changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 286bd3a4b92..350b01e69fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,9 @@ BUG FIXES: updates [GH-3445] * core: Fixes an issue with jobs that have `auto_revert` set to true, where reverting to a previously stable job that fails to start up causes an infinite cycle of reverts [GH-3496] + * template: Fix issue where multiple environment variable templates would be + parsed incorrectly when contents of one have changed after the initial + rendering [GH-3529] * sentinel: (Nomad Enterprise) Fix an issue that could cause an import error when multiple Sentinel policies are applied From da852ea65380ee3b7596f52fdaba65b44f9514ce Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Fri, 10 Nov 2017 12:35:51 -0800 Subject: [PATCH 3/3] alway load all templates --- client/consul_template.go | 6 +++--- client/driver/env/env.go | 12 ++---------- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/client/consul_template.go b/client/consul_template.go index 2ea4855bf94..a4adfc84dbd 100644 --- a/client/consul_template.go +++ b/client/consul_template.go @@ -218,7 +218,7 @@ func (tm *TaskTemplateManager) run() { tm.config.Hooks.Kill(consulTemplateSourceName, err.Error(), true) return } - tm.config.EnvBuilder.MergeTemplateEnv(envMap) + tm.config.EnvBuilder.SetTemplateEnv(envMap) // Unblock the task tm.config.Hooks.UnblockStart(consulTemplateSourceName) @@ -389,12 +389,12 @@ func (tm *TaskTemplateManager) handleTemplateRerenders(allRenderedTime time.Time } // Read environment variables from templates - envMap, err := loadTemplateEnv(tmpls, tm.config.TaskDir) + envMap, err := loadTemplateEnv(tm.config.Templates, tm.config.TaskDir) if err != nil { tm.config.Hooks.Kill(consulTemplateSourceName, err.Error(), true) return } - tm.config.EnvBuilder.MergeTemplateEnv(envMap) + tm.config.EnvBuilder.SetTemplateEnv(envMap) for _, tmpl := range tmpls { switch tmpl.ChangeMode { diff --git a/client/driver/env/env.go b/client/driver/env/env.go index 2e05653fb4d..9e63accbc0a 100644 --- a/client/driver/env/env.go +++ b/client/driver/env/env.go @@ -532,17 +532,9 @@ func (b *Builder) SetHostEnvvars(filter []string) *Builder { return b } -// MergeTemplateEnv is used to merge the passed environment variables with -// existing environment variables set from a template. -func (b *Builder) MergeTemplateEnv(m map[string]string) *Builder { +func (b *Builder) SetTemplateEnv(m map[string]string) *Builder { b.mu.Lock() - if b.templateEnv == nil { - b.templateEnv = m - } else { - for k, v := range m { - b.templateEnv[k] = v - } - } + b.templateEnv = m b.mu.Unlock() return b }