Add envconsul-like support and refactor environment handling #2654

schmichael · 2017-05-18T18:42:26Z

There's two major goals of this PR:

Add envconsul-like support
Refactor env.TaskEnvironment into a dynamic Builder and a static TaskEnv.

The refactor was to avoid a new circular dependency between environments and templates. Templates get an immutable TaskEnv and task_runner handles populating the environment from environment template files.

Unfortunately there's still a circular dependency between environments and drivers in two places:

To set environment paths we need to call Driver.FSIsolation()
Docker's port_map affects how network env vars are populated

So the new env.Builder struct has to be exposed to task_runner and templates, but everything else (drivers, executors, etc) only have access to the immutable env.TaskEnv.

Graph of the dependencies:

jippi · 2017-05-19T06:12:26Z

amazing job @schmichael ! Thank you

Moves env.Builder out of drivers entirely so one less thing to worry about when implementing driver plugins.

Also inject PATH into rkt commands since we're no longer appending host env vars for it.

dadgar · 2017-05-24T22:22:52Z

client/alloc_runner_test.go

@@ -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{


Switch to mock driver instead?

Good call! Way faster and one more test that doesn't require root.

dadgar · 2017-05-24T22:26:15Z

client/consul_template.go

+	}
+
+	// Set the environment variables
+	builder.SetTemplateEnv(vars)


This breaks if they have multiple templates generating environment variables.

Wow, good catch. Will add a test as well.

dadgar · 2017-05-24T22:33:02Z

client/consul_template.go

@@ -192,6 +195,12 @@ WAIT:
 		}
 	}

+	for _, t := range tm.templates {


Comment explaining what you are doing. Blank line between the block please

dadgar · 2017-05-24T22:33:19Z

client/consul_template.go

@@ -243,6 +252,11 @@ WAIT:
 				}

 				for _, tmpl := range tmpls {
+					if err := loadTemplateEnv(envBuilder, taskDir, tmpl); err != nil {
+


dadgar · 2017-05-24T22:33:37Z

client/consul_template.go

+
+						tm.hook.Kill("consul-template", err.Error(), true)
+						return
+					}


Blank below

dadgar · 2017-05-24T22:54:09Z

client/driver/env/env.go

-func (t *TaskEnvironment) ClearRegionName() *TaskEnvironment {
-	t.Region = ""
-	return t
+func (b *Builder) SetTemplateEnv(m map[string]string) *Builder {


See comment in consul_template.go

dadgar · 2017-05-24T23:01:36Z

client/driver/rkt.go

+	cmdArgs := make([]string, 0, 50)
+
+	// Run rkt with env command to inject PATH as rkt needs to be able to find iptables
+	envAbsPath, err := GetAbsolutePath(envCmd)


Unclear why we can just use the option -set-env and just run Rkt with all environment variables minus those in the blacklist passed through?

Switched it to use a fresh env with just the host env vars on it. Seems more reasonable than mutating TaskEnv to add the PATH in just for the rkt command.

dadgar · 2017-05-24T23:07:04Z

jobspec/parse.go

@@ -861,6 +861,7 @@ func parseTemplates(result *[]*api.Template, list *ast.ObjectList) error {
 			"right_delimiter",
 			"source",
 			"splay",
+			"env",


dadgar · 2017-05-24T23:07:20Z

command/agent/job_endpoint.go

@@ -680,6 +680,7 @@ func ApiTaskToStructsTask(apiTask *api.Task, structsTask *structs.Task) {
 				Perms:        *template.Perms,
 				LeftDelim:    *template.LeftDelim,
 				RightDelim:   *template.RightDelim,
+				Envvars:      *template.Envvars,


add to test

dadgar · 2017-05-24T23:07:44Z

api/tasks.go

@@ -373,6 +374,9 @@ func (tmpl *Template) Canonicalize() {
 	if tmpl.RightDelim == nil {
 		tmpl.RightDelim = helper.StringToPtr("}}")
 	}
+	if tmpl.Envvars == nil {


Believe there is a canonicalize test as well

And use an interface for ReplaceEnv since its all getter needs.

schmichael · 2017-05-27T00:05:42Z

Ready for re-review.

jippi · 2017-05-30T19:20:20Z

client/consul_template_test.go

+
+FOO=bar
+foo=123
+ANYTHING-goes=Spaces are=ok!


are newlines okay too?

assume key derp has a value in Consul of the following, and you do DERP={{ key "derp" }}

hello world

Newlines need to be escaped. The 2 easiest ways are probably:

{{ key "derp" | toJSON}}

{{ key "derp" | printf "%q" }}

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:

>>"hello\nworld"<<

So I'll post a followup PR that properly un-escapes values, so that the code above would print:

>>hello world<<

c4milo · 2017-05-30T23:01:09Z

Excellent work!

jippi · 2017-05-31T06:03:11Z

Amazing @schmichael ! :D another $work owned step to remove soon'ish : D

github-actions · 2023-04-01T02:10:44Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

schmichael force-pushed the f-env-consul branch from e7dd0f8 to 6268c17 Compare May 23, 2017 17:20

schmichael added 10 commits May 23, 2017 13:45

Functional consul template env file support

7dac668

Refactor TaskEnvironment into Builder and TaskEnv

ace0098

Move path building to task dir initialization

3b24980

Improve PortMap handling and simplify Builder creation

96753dc

Move env template handling into consul_template.go

37c1cbc

Add PortMap to struct returned by Driver.Prestart

6db3501

Moves env.Builder out of drivers entirely so one less thing to worry about when implementing driver plugins.

Fix env var interpolation and env tests

3743d34

Move task env into execcontext

a96fb5d

Also inject PATH into rkt commands since we're no longer appending host env vars for it.

Handle Driver.Prestart returning nil, nil

1295f88

Fix test data

59b3cca

schmichael force-pushed the f-env-consul branch from 73e760d to 3492ec5 Compare May 23, 2017 22:58

Add env.Builder.UpdateTask for alloc updates

72a24ae

schmichael force-pushed the f-env-consul branch from 3492ec5 to 72a24ae Compare May 23, 2017 23:01

schmichael added 3 commits May 23, 2017 16:11

Shrink chroot to avoid timing test failure

1188924

Add env testing

d7eccc1

Add env file test

9c66952

schmichael force-pushed the f-env-consul branch from b2af9b3 to 9c66952 Compare May 24, 2017 00:07

Document env templates

826aec4

schmichael mentioned this pull request May 24, 2017

Add ability to read environment variables from a local file #2669

Closed

Fail fast on env template failures

0288582

schmichael force-pushed the f-env-consul branch from fb555e2 to 0288582 Compare May 24, 2017 20:45

Minor wording updates

43cc0a0

schmichael changed the title ~~[WIP] Add envconsul-like support and refactor environment handling~~ Add envconsul-like support and refactor environment handling May 24, 2017

dadgar requested changes May 24, 2017

View reviewed changes

schmichael added 4 commits May 25, 2017 09:28

Switch tests to mock_driver

6571fa5

Comment and correct formatting

8f2ea2f

Fix formatting

572ca97

Fix and test multi-env-template loading

be959a4

schmichael added 8 commits May 26, 2017 15:06

Let's pretend I never committed this

9ccb53d

Use custom TaskEnv to provide PATH for rkt

fc5254d

Test env parsing

de4b008

Always use PATH-only env for rkt commands

c4aa3c3

Fix template canonicalize test

6d0bc98

Fix executor tests

aa0e977

Fix getter tests

68db2aa

And use an interface for ReplaceEnv since its all getter needs.

Fix diff test

5933cac

Fix Error -> Errorf

361db24

jippi reviewed May 30, 2017

View reviewed changes

schmichael merged commit aac319c into master May 30, 2017

schmichael deleted the f-env-consul branch May 30, 2017 21:40

schmichael mentioned this pull request Jun 2, 2017

Move env file parsing to a library #2683

Merged

pznamensky mentioned this pull request Jun 28, 2017

Improvement: multiple key's prefixes in templates like in envconsul #2751

Closed

schmichael mentioned this pull request Jul 3, 2017

Feature: Vault secrets in job specifications #2435

Closed

schmichael added a commit that referenced this pull request Jul 3, 2017

Add #2654 #2695 #2702 #2709 #2729 #2735 #2746 #2750 #2753

bde854c

mqasim1983 mentioned this pull request Jul 26, 2017

How To Set Docker Env From Vault #2902

Closed

github-actions bot locked as resolved and limited conversation to collaborators Apr 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add envconsul-like support and refactor environment handling #2654

Add envconsul-like support and refactor environment handling #2654

schmichael commented May 18, 2017 •

edited

Loading

jippi commented May 19, 2017

dadgar May 24, 2017

schmichael May 25, 2017

dadgar May 24, 2017

schmichael May 25, 2017

dadgar May 24, 2017

dadgar May 24, 2017

dadgar May 24, 2017

dadgar May 24, 2017

schmichael May 26, 2017

dadgar May 24, 2017

schmichael May 26, 2017

dadgar May 24, 2017

schmichael May 26, 2017

dadgar May 24, 2017

dadgar May 24, 2017

schmichael commented May 27, 2017

jippi May 30, 2017

schmichael May 30, 2017

c4milo commented May 30, 2017

jippi commented May 31, 2017

github-actions bot commented Apr 1, 2023

Add envconsul-like support and refactor environment handling #2654

Add envconsul-like support and refactor environment handling #2654

Conversation

schmichael commented May 18, 2017 • edited Loading

jippi commented May 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmichael commented May 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c4milo commented May 30, 2017

jippi commented May 31, 2017

github-actions bot commented Apr 1, 2023

schmichael commented May 18, 2017 •

edited

Loading