-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add envconsul-like support and refactor environment handling #2654
Changes from 17 commits
7dac668
ace0098
3b24980
96753dc
37c1cbc
6db3501
3743d34
a96fb5d
1295f88
59b3cca
72a24ae
1188924
d7eccc1
9c66952
826aec4
0288582
43cc0a0
6571fa5
8f2ea2f
572ca97
be959a4
9ccb53d
fc5254d
de4b008
c4aa3c3
6d0bc98
aa0e977
68db2aa
5933cac
361db24
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -138,6 +138,14 @@ func TestAllocRunner_TerminalUpdate_Destroy(t *testing.T) { | |
ctestutil.ExecCompatible(t) | ||
upd, ar := testAllocRunner(false) | ||
|
||
// Shrink chroot | ||
ar.config.ChrootEnv = map[string]string{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Switch to mock driver instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call! Way faster and one more test that doesn't require root. |
||
"/bin": "/bin", | ||
"/lib": "/lib", | ||
"/lib32": "/lib32", | ||
"/lib64": "/lib64", | ||
} | ||
|
||
// Ensure task takes some time | ||
task := ar.alloc.Job.TaskGroups[0].Tasks[0] | ||
task.Config["command"] = "/bin/sleep" | ||
|
@@ -237,6 +245,14 @@ func TestAllocRunner_Destroy(t *testing.T) { | |
ctestutil.ExecCompatible(t) | ||
upd, ar := testAllocRunner(false) | ||
|
||
// Shrink chroot | ||
ar.config.ChrootEnv = map[string]string{ | ||
"/bin": "/bin", | ||
"/lib": "/lib", | ||
"/lib32": "/lib32", | ||
"/lib64": "/lib64", | ||
} | ||
|
||
// Ensure task takes some time | ||
task := ar.alloc.Job.TaskGroups[0].Tasks[0] | ||
task.Config["command"] = "/bin/sleep" | ||
|
@@ -269,7 +285,7 @@ func TestAllocRunner_Destroy(t *testing.T) { | |
|
||
return nil | ||
}); err != nil { | ||
return false, fmt.Errorf("state not destroyed") | ||
return false, fmt.Errorf("state not destroyed: %v", err) | ||
} | ||
|
||
// Check the alloc directory was cleaned | ||
|
@@ -612,6 +628,7 @@ func TestAllocRunner_RestoreOldState(t *testing.T) { | |
|
||
logger := testLogger() | ||
conf := config.DefaultConfig() | ||
conf.Node = mock.Node() | ||
conf.StateDir = os.TempDir() | ||
conf.AllocDir = os.TempDir() | ||
tmp, err := ioutil.TempFile("", "state-db") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,10 @@ | ||
package client | ||
|
||
import ( | ||
"bufio" | ||
"bytes" | ||
"fmt" | ||
"io" | ||
"math/rand" | ||
"os" | ||
"path/filepath" | ||
|
@@ -76,7 +79,7 @@ type TaskTemplateManager struct { | |
|
||
func NewTaskTemplateManager(hook TaskHooks, tmpls []*structs.Template, | ||
config *config.Config, vaultToken, taskDir string, | ||
taskEnv *env.TaskEnvironment) (*TaskTemplateManager, error) { | ||
envBuilder *env.Builder) (*TaskTemplateManager, error) { | ||
|
||
// Check pre-conditions | ||
if hook == nil { | ||
|
@@ -85,7 +88,7 @@ func NewTaskTemplateManager(hook TaskHooks, tmpls []*structs.Template, | |
return nil, fmt.Errorf("Invalid config given") | ||
} else if taskDir == "" { | ||
return nil, fmt.Errorf("Invalid task directory given") | ||
} else if taskEnv == nil { | ||
} else if envBuilder == nil { | ||
return nil, fmt.Errorf("Invalid task environment given") | ||
} | ||
|
||
|
@@ -114,14 +117,14 @@ func NewTaskTemplateManager(hook TaskHooks, tmpls []*structs.Template, | |
} | ||
|
||
// Build the consul-template runner | ||
runner, lookup, err := templateRunner(tmpls, config, vaultToken, taskDir, taskEnv) | ||
runner, lookup, err := templateRunner(tmpls, config, vaultToken, taskDir, envBuilder.Build()) | ||
if err != nil { | ||
return nil, err | ||
} | ||
tm.runner = runner | ||
tm.lookup = lookup | ||
|
||
go tm.run() | ||
go tm.run(envBuilder, taskDir) | ||
return tm, nil | ||
} | ||
|
||
|
@@ -144,7 +147,7 @@ func (tm *TaskTemplateManager) Stop() { | |
} | ||
|
||
// run is the long lived loop that handles errors and templates being rendered | ||
func (tm *TaskTemplateManager) run() { | ||
func (tm *TaskTemplateManager) run(envBuilder *env.Builder, taskDir string) { | ||
// Runner is nil if there is no templates | ||
if tm.runner == nil { | ||
// Unblock the start if there is nothing to do | ||
|
@@ -192,6 +195,12 @@ WAIT: | |
} | ||
} | ||
|
||
for _, t := range tm.templates { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment explaining what you are doing. Blank line between the block please |
||
if err := loadTemplateEnv(envBuilder, taskDir, t); err != nil { | ||
tm.hook.Kill("consul-template", err.Error(), true) | ||
return | ||
} | ||
} | ||
allRenderedTime = time.Now() | ||
tm.hook.UnblockStart("consul-template") | ||
|
||
|
@@ -243,6 +252,11 @@ WAIT: | |
} | ||
|
||
for _, tmpl := range tmpls { | ||
if err := loadTemplateEnv(envBuilder, taskDir, tmpl); err != nil { | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Extra line |
||
tm.hook.Kill("consul-template", err.Error(), true) | ||
return | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blank below |
||
switch tmpl.ChangeMode { | ||
case structs.TemplateChangeModeSignal: | ||
signals[tmpl.ChangeSignal] = struct{}{} | ||
|
@@ -317,7 +331,7 @@ func (tm *TaskTemplateManager) allTemplatesNoop() bool { | |
// lookup by destination to the template. If no templates are given, a nil | ||
// template runner and lookup is returned. | ||
func templateRunner(tmpls []*structs.Template, config *config.Config, | ||
vaultToken, taskDir string, taskEnv *env.TaskEnvironment) ( | ||
vaultToken, taskDir string, taskEnv *env.TaskEnv) ( | ||
*manager.Runner, map[string][]*structs.Template, error) { | ||
|
||
if len(tmpls) == 0 { | ||
|
@@ -350,7 +364,7 @@ func templateRunner(tmpls []*structs.Template, config *config.Config, | |
} | ||
|
||
// Set Nomad's environment variables | ||
runner.Env = taskEnv.Build().EnvMapAll() | ||
runner.Env = taskEnv.All() | ||
|
||
// Build the lookup | ||
idMap := runner.TemplateConfigMapping() | ||
|
@@ -368,9 +382,7 @@ func templateRunner(tmpls []*structs.Template, config *config.Config, | |
|
||
// parseTemplateConfigs converts the tasks templates into consul-templates | ||
func parseTemplateConfigs(tmpls []*structs.Template, taskDir string, | ||
taskEnv *env.TaskEnvironment, allowAbs bool) (map[ctconf.TemplateConfig]*structs.Template, error) { | ||
// Build the task environment | ||
taskEnv.Build() | ||
taskEnv *env.TaskEnv, allowAbs bool) (map[ctconf.TemplateConfig]*structs.Template, error) { | ||
|
||
ctmpls := make(map[ctconf.TemplateConfig]*structs.Template, len(tmpls)) | ||
for _, tmpl := range tmpls { | ||
|
@@ -492,3 +504,60 @@ func runnerConfig(config *config.Config, vaultToken string) (*ctconf.Config, err | |
conf.Finalize() | ||
return conf, nil | ||
} | ||
|
||
// loadTemplateEnv loads task environment variables from templates. | ||
func loadTemplateEnv(builder *env.Builder, taskDir string, t *structs.Template) error { | ||
if !t.Envvars { | ||
return nil | ||
} | ||
f, err := os.Open(filepath.Join(taskDir, t.DestPath)) | ||
if err != nil { | ||
return fmt.Errorf("error opening env template: %v", err) | ||
} | ||
defer f.Close() | ||
|
||
// Parse environment fil | ||
vars, err := parseEnvFile(f) | ||
if err != nil { | ||
return fmt.Errorf("error parsing env template %q: %v", t.DestPath, err) | ||
} | ||
|
||
// Set the environment variables | ||
builder.SetTemplateEnv(vars) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This breaks if they have multiple templates generating environment variables. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wow, good catch. Will add a test as well. |
||
return nil | ||
} | ||
|
||
// parseEnvFile and return a map of the environment variables suitable for | ||
// TaskEnvironment.AppendEnvvars or an error. | ||
// | ||
// See nomad/structs#Template.Envvars comment for format. | ||
func parseEnvFile(r io.Reader) (map[string]string, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It may be worth just using this: https://github.com/joho/godotenv There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lots of edge cases we don't need to handle There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this is tied to a template it must be a single file. It's probably worth documenting how to use env files from artifacts by setting source path to where the artifact is downloaded, but env dirs (or autoloading |
||
vars := make(map[string]string, 50) | ||
lines := 0 | ||
scanner := bufio.NewScanner(r) | ||
for scanner.Scan() { | ||
lines++ | ||
buf := scanner.Bytes() | ||
if len(buf) == 0 { | ||
// Skip empty lines | ||
continue | ||
} | ||
if buf[0] == '#' { | ||
// Skip lines starting with a # | ||
continue | ||
} | ||
n := bytes.IndexByte(buf, '=') | ||
if n == -1 { | ||
return nil, fmt.Errorf("line %d: no '=' sign: %q", lines, string(buf)) | ||
} | ||
if len(buf) > n { | ||
vars[string(buf[0:n])] = string(buf[n+1 : len(buf)]) | ||
} else { | ||
vars[string(buf[0:n])] = "" | ||
} | ||
} | ||
if err := scanner.Err(); err != nil { | ||
return nil, err | ||
} | ||
return vars, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -91,7 +91,7 @@ type testHarness struct { | |
manager *TaskTemplateManager | ||
mockHooks *MockTaskHooks | ||
templates []*structs.Template | ||
taskEnv *env.TaskEnvironment | ||
envBuilder *env.Builder | ||
node *structs.Node | ||
config *config.Config | ||
vaultToken string | ||
|
@@ -103,18 +103,22 @@ type testHarness struct { | |
// newTestHarness returns a harness starting a dev consul and vault server, | ||
// building the appropriate config and creating a TaskTemplateManager | ||
func newTestHarness(t *testing.T, templates []*structs.Template, consul, vault bool) *testHarness { | ||
region := "global" | ||
harness := &testHarness{ | ||
mockHooks: NewMockTaskHooks(), | ||
templates: templates, | ||
node: mock.Node(), | ||
config: &config.Config{}, | ||
config: &config.Config{Region: region}, | ||
} | ||
|
||
// Build the task environment | ||
harness.taskEnv = env.NewTaskEnvironment(harness.node).SetTaskName(TestTaskName) | ||
a := mock.Alloc() | ||
task := a.Job.TaskGroups[0].Tasks[0] | ||
task.Name = TestTaskName | ||
harness.envBuilder = env.NewBuilder(harness.node, a, task, region) | ||
|
||
// Make a tempdir | ||
d, err := ioutil.TempDir("", "") | ||
d, err := ioutil.TempDir("", "ct_test") | ||
if err != nil { | ||
t.Fatalf("Failed to make tmpdir: %v", err) | ||
} | ||
|
@@ -141,7 +145,7 @@ func newTestHarness(t *testing.T, templates []*structs.Template, consul, vault b | |
|
||
func (h *testHarness) start(t *testing.T) { | ||
manager, err := NewTaskTemplateManager(h.mockHooks, h.templates, | ||
h.config, h.vaultToken, h.taskDir, h.taskEnv) | ||
h.config, h.vaultToken, h.taskDir, h.envBuilder) | ||
if err != nil { | ||
t.Fatalf("failed to build task template manager: %v", err) | ||
} | ||
|
@@ -151,7 +155,7 @@ func (h *testHarness) start(t *testing.T) { | |
|
||
func (h *testHarness) startWithErr() error { | ||
manager, err := NewTaskTemplateManager(h.mockHooks, h.templates, | ||
h.config, h.vaultToken, h.taskDir, h.taskEnv) | ||
h.config, h.vaultToken, h.taskDir, h.envBuilder) | ||
h.manager = manager | ||
return err | ||
} | ||
|
@@ -175,27 +179,29 @@ func (h *testHarness) stop() { | |
func TestTaskTemplateManager_Invalid(t *testing.T) { | ||
hooks := NewMockTaskHooks() | ||
var tmpls []*structs.Template | ||
config := &config.Config{} | ||
region := "global" | ||
config := &config.Config{Region: region} | ||
taskDir := "foo" | ||
vaultToken := "" | ||
taskEnv := env.NewTaskEnvironment(mock.Node()) | ||
a := mock.Alloc() | ||
envBuilder := env.NewBuilder(mock.Node(), a, a.Job.TaskGroups[0].Tasks[0], config.Region) | ||
|
||
_, err := NewTaskTemplateManager(nil, nil, nil, "", "", nil) | ||
if err == nil { | ||
t.Fatalf("Expected error") | ||
} | ||
|
||
_, err = NewTaskTemplateManager(nil, tmpls, config, vaultToken, taskDir, taskEnv) | ||
_, err = NewTaskTemplateManager(nil, tmpls, config, vaultToken, taskDir, envBuilder) | ||
if err == nil || !strings.Contains(err.Error(), "task hook") { | ||
t.Fatalf("Expected invalid task hook error: %v", err) | ||
} | ||
|
||
_, err = NewTaskTemplateManager(hooks, tmpls, nil, vaultToken, taskDir, taskEnv) | ||
_, err = NewTaskTemplateManager(hooks, tmpls, nil, vaultToken, taskDir, envBuilder) | ||
if err == nil || !strings.Contains(err.Error(), "config") { | ||
t.Fatalf("Expected invalid config error: %v", err) | ||
} | ||
|
||
_, err = NewTaskTemplateManager(hooks, tmpls, config, vaultToken, "", taskEnv) | ||
_, err = NewTaskTemplateManager(hooks, tmpls, config, vaultToken, "", envBuilder) | ||
if err == nil || !strings.Contains(err.Error(), "task directory") { | ||
t.Fatalf("Expected invalid task dir error: %v", err) | ||
} | ||
|
@@ -205,7 +211,7 @@ func TestTaskTemplateManager_Invalid(t *testing.T) { | |
t.Fatalf("Expected invalid task environment error: %v", err) | ||
} | ||
|
||
tm, err := NewTaskTemplateManager(hooks, tmpls, config, vaultToken, taskDir, taskEnv) | ||
tm, err := NewTaskTemplateManager(hooks, tmpls, config, vaultToken, taskDir, envBuilder) | ||
if err != nil { | ||
t.Fatalf("Unexpected error: %v", err) | ||
} else if tm == nil { | ||
|
@@ -221,7 +227,7 @@ func TestTaskTemplateManager_Invalid(t *testing.T) { | |
} | ||
|
||
tmpls = append(tmpls, tmpl) | ||
tm, err = NewTaskTemplateManager(hooks, tmpls, config, vaultToken, taskDir, taskEnv) | ||
tm, err = NewTaskTemplateManager(hooks, tmpls, config, vaultToken, taskDir, envBuilder) | ||
if err == nil || !strings.Contains(err.Error(), "Failed to parse signal") { | ||
t.Fatalf("Expected signal parsing error: %v", err) | ||
} | ||
|
@@ -905,3 +911,45 @@ func TestTaskTemplateManager_Signal_Error(t *testing.T) { | |
t.Fatalf("Unexpected error: %v", harness.mockHooks.KillReason) | ||
} | ||
} | ||
|
||
// TestTaskTemplateManager_Env asserts templates with the env flag set are read | ||
// into the task's environment. | ||
func TestTaskTemplateManager_Env(t *testing.T) { | ||
template := &structs.Template{ | ||
EmbeddedTmpl: ` | ||
# Comment lines are ok | ||
|
||
FOO=bar | ||
foo=123 | ||
ANYTHING-goes=Spaces are=ok! | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are newlines okay too? assume key
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Newlines need to be escaped. The 2 easiest ways are probably:
Both will properly escape newlines. However, in this current PR they won't be unescaped and if your task read the environment it would look like: derp := os.Getenv("derp")
fmt.Printf(">>%s<<\n", derp) It would print the escaped value:
So I'll post a followup PR that properly un-escapes values, so that the code above would print:
|
||
`, | ||
DestPath: "test.env", | ||
ChangeMode: structs.TemplateChangeModeNoop, | ||
Envvars: true, | ||
} | ||
harness := newTestHarness(t, []*structs.Template{template}, true, false) | ||
harness.start(t) | ||
defer harness.stop() | ||
|
||
// Wait a little | ||
select { | ||
case <-harness.mockHooks.UnblockCh: | ||
case <-time.After(time.Duration(2*testutil.TestMultiplier()) * time.Second): | ||
t.Fatalf("Should have received unblock: %+v", harness.mockHooks) | ||
} | ||
|
||
// Validate environment | ||
env := harness.envBuilder.Build().Map() | ||
if len(env) < 3 { | ||
t.Fatalf("expected at least 3 env vars but found %d:\n%#v\n", len(env), env) | ||
} | ||
if env["FOO"] != "bar" { | ||
t.Errorf("expected FOO=bar but found %q", env["FOO"]) | ||
} | ||
if env["foo"] != "123" { | ||
t.Errorf("expected foo=123 but found %q", env["foo"]) | ||
} | ||
if env["ANYTHING_goes"] != "Spaces are=ok!" { | ||
t.Errorf("expected ANYTHING_GOES='Spaces are ok!' but found %q", env["ANYTHING_goes"]) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Believe there is a canonicalize test as well