compose: fix environment interpolation from the client by AkihiroSuda · Pull Request #30781 · moby/moby

AkihiroSuda · 2017-02-07T09:55:31Z

- What I did

Fix (newly support?) environment interpolation from the client

e.g.

services:
  x:
    environment:
# needs to be propagated from the client
      - FOO

- How I did it

Please refer to the code.

- How to verify it
Prepare following files:

docker-compose.yaml:

version: "3"
services:
  x:
    image: "busybox"
    command: ["sh", "-c", "env; tail -f /dev/null"]
    environment:
      - FOO
      - BAR
      - QUX
      - QUUX=quux_from_yaml
    env_file: a.env

a.env:

FOO=foo_from_file
BAR=bar_from_file
BAZ=baz_from_file

on Docker Compose:

$ FOO=foo_from_env QUUX=quux_from_env docker-compose up

on Docker Swarm-mode:

$ FOO=foo_from_env QUUX=quux_from_env docker stack deploy -c docker-compose.yaml stack01
$ docker service logs stack01_x

Result:

Composer	`FOO`	`BAR`	`BAZ`	`QUX`	`QUUX`
Compose 1.11.1, Swarm-mode with this PR	`"foo_from_env"`	unset (not `""`)	`"baz_from_file"`	unset (not `""`)	`"quux_from_yaml"`
Swarm-mode 1.13.1	`""`	`""`	`"baz_from_file"`	`""`	`"quux_from_yaml"`

~~For BAR, I wonder Compose's result (unset) is unexpected and should be "", but not sure. (Could not find any doc 😅)~~

- Description for the changelog

compose: support environment interpolation from the client

- A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: Akihiro Suda suda.akihiro@lab.ntt.co.jp

AkihiroSuda · 2017-02-07T09:57:42Z

cc @dnephin

dnephin

This looks great, thanks!

Just a couple small issues

dnephin · 2017-02-07T15:44:29Z

cli/compose/loader/loader.go

+		home, ok := lookupEnv("HOME")
+		if !ok {
+			panic(errors.New("cannot expand '~', because the environment lacks HOME"))
+		}


I don't think this should be a panic. At most a warning.

@dnephin What should we do with warning?
Keep unparsed '~' as-is? Or Ignore the entry?

Keep it as-is with the ~ I think

dnephin · 2017-02-07T15:52:43Z

cli/compose/loader/loader.go

@@ -358,10 +363,18 @@ func resolveEnvironment(serviceConfig *types.ServiceConfig, workingDir string) e

 	serviceConfig.Environment = environment


nit: To be consistent I think this line should be at the end of the function, right before return nil

dnephin · 2017-02-07T15:53:14Z

cli/compose/loader/loader.go


+	// env is prioritized over the file
+	for k := range serviceConfig.Environment {
+		v, ok := lookupEnv(k)


I think this is missing a check to see if v == "". If v is not the empty string we should not override from the client environment.

aaronlehmann · 2017-02-07T18:27:29Z

cli/command/stack/deploy.go

+	env := os.Environ()
+	details.Environment = make(map[string]string, len(env))
+	for _, s := range env {
+		// if value is empty, s is like "K=", not "K".


This is probably a reasonable assumption, but it couldn't hurt to check.

AkihiroSuda · 2017-02-20T08:03:31Z

@dnephin @aaronlehmann
Updated PR (sorry for being late)

Now this PR is much more compatible with Docker Compose, but maybe breaking compatibility with Docker Swarm-mode 1.13.1. (Please refer to changes in UT)

cc @shin-

dnephin · 2017-02-21T16:46:13Z

cli/compose/loader/loader.go

+			if ok {
+				environment[k] = interpolatedV
+			} else {
+				delete(environment, k)


I don't think you ever want to delete things from the environment. What's this about?

If the environment variable is specified to be interpolated in the YAML but its value is unset in the actual environment, it should be deleted (in Docker Compose's implementation).

Please refer to the example table listed in the description text of this PR.

I don't think it's actually deleted. I don't see that in the Compose code anywhere. I think that's handled by the server when starting a container. It creates the variable unset.

Yes, you are right, I didn't know that 😅

Opened #31634 for clarifying this behavior in the doc.

I'll update this PR as well in short.

dnephin · 2017-02-21T16:54:50Z

cli/compose/loader/loader.go

+				environment[k] = interpolatedV
+			} else {
+				environment[k] = v
+			}


Here's the logic from Compose: https://github.com/docker/compose/blob/master/compose/config/config.py#L589-L598

I think we can reduce the size of this function quite a bit by matching the logic.

Once delete() is removed from below, i think these blocks will be nearly identical. So we could have a function like this:

func updateEnvironment(environment, map[string]string, vars map[string]string, lookupEnv template.Mapping) map[string[string { ... } environment = updateEnvironment(environment, runconfigopts.ConvertKVStringsToMap(envVars), lookupEnv) serviceConfig.Environment = updateEnvironment(environment, serviceConfig.Environment, lookupEnv) return nil

Thank you for the pointer, I'll look into that.

vdemeester

Design looks good :)

cpuguy83 · 2017-03-07T18:48:18Z

Ping, any update here?

AkihiroSuda · 2017-03-08T05:09:13Z

updated PR, sorry for delay

AkihiroSuda · 2017-03-08T05:23:26Z

oops, still something is wrong... (expected: BAR, got BAR=)

$ docker inspect task_container_of_the_example_above
            "Env": [
                "BAR=",
                "BAZ=baz_from_file",
                "FOO=foo_from_env",
                "QUUX=quux_from_env",
                "QUX=",
                "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
            ],

AkihiroSuda · 2017-03-08T08:56:19Z

trying to fix the issue 👆 by defining MappingWithEquals as map[string]*string rather than map[string]string, but getting stuck due to reflection ... 😭 https://github.com/AkihiroSuda/docker/commits/fix-stack-env.wip.20170308

For an environment variable defined in the yaml without value, the value needs to be propagated from the client, as in Docker Compose. Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

Load from env should only happen if the value is unset. Extract a buildEnvironment function and revert some changes to tests. Signed-off-by: Daniel Nephin <dnephin@docker.com>

dnephin · 2017-03-14T20:01:06Z

I've rebased this PR, and fixed the remaining issue with empty values.

AkihiroSuda · 2017-03-15T01:28:21Z

@dnephin Thank you a lot 😃

AkihiroSuda · 2017-03-15T01:33:14Z

cli/compose/types/types.go

@@ -135,7 +135,10 @@ type StringOrNumberList []string

 // MappingWithEquals is a mapping type that can be converted from a list of
 // key=value strings


// MappingWithEquals is a mapping type that can be converted from a list of // key[=value] strings. // For the key with an empty value (`key=`), the mapped value is set to a pointer to `""`. // For the key without value (`key`), the mapped value is set to nil.

committed as 2cdc9c4

vdemeester

LGTM 🦁

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

AkihiroSuda · 2017-03-17T06:23:22Z

LGTM for @dnephin 's work 😃

dnephin

LGTM

AkihiroSuda · 2017-03-22T05:15:12Z

@vieux @dnephin

I'm unsure this is a new feature of bug-fix 😅, but can we cherry-pick this for 17.04?

dnephin · 2017-03-22T13:25:29Z

I think it's a bug fix

thaJeztah · 2017-03-22T15:53:32Z

ping @AkihiroSuda could you prepare a cherry-pick for this?

AkihiroSuda · 2017-03-23T02:12:04Z

sure, opened #32033

compose: fix environment interpolation from the client

bitsofinfo · 2017-11-29T17:59:48Z

When is this slated to be released?

thaJeztah · 2017-11-29T18:57:21Z

This has been released quite a while ago already

thaJeztah · 2017-11-29T18:57:45Z

Docker 17.04 and up should have this change

GordonTheTurtle added the status/0-triage label Feb 7, 2017

AkihiroSuda added area/stack status/1-design-review and removed status/0-triage labels Feb 7, 2017

vdemeester requested review from dnephin and vdemeester February 7, 2017 09:59

AkihiroSuda force-pushed the fix-stack-env branch from 975f9e4 to 9e92823 Compare February 7, 2017 10:07

dnephin reviewed Feb 7, 2017

View reviewed changes

aaronlehmann reviewed Feb 7, 2017

View reviewed changes

AkihiroSuda force-pushed the fix-stack-env branch from 9e92823 to 3852fbc Compare February 20, 2017 07:56

dnephin reviewed Feb 21, 2017

View reviewed changes

GordonTheTurtle assigned cpuguy83 Feb 22, 2017

This was referenced Feb 24, 2017

stack deploy does not use env-vars from shell #31319

Closed

Use environment variable from shell when empty #31334

Closed

vdemeester reviewed Mar 1, 2017

View reviewed changes

vdemeester added status/2-code-review and removed status/1-design-review labels Mar 1, 2017

tpitale mentioned this pull request Mar 6, 2017

Running docker stack deploy with compose file does not pass environment variable values #31561

Closed

tpitale mentioned this pull request Mar 7, 2017

Finish provisioning tpitale/advocate-provision#2

Open

4 tasks

AkihiroSuda force-pushed the fix-stack-env branch from 3852fbc to 444a243 Compare March 8, 2017 05:05

AkihiroSuda added status/1-design-review and removed status/2-code-review labels Mar 8, 2017

dnephin mentioned this pull request Mar 14, 2017

docker stack deploy ignores environment variables and replaces them with blank values #30251

Closed

dnephin force-pushed the fix-stack-env branch from 444a243 to bf84ddd Compare March 14, 2017 19:56

dnephin added status/2-code-review and removed status/1-design-review labels Mar 14, 2017

compose: fix environment interpolation from the client

ea43c33

For an environment variable defined in the yaml without value, the value needs to be propagated from the client, as in Docker Compose. Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

dnephin force-pushed the fix-stack-env branch from bf84ddd to a9c86b6 Compare March 14, 2017 20:00

Fix environment resolving.

a9c86b6

Load from env should only happen if the value is unset. Extract a buildEnvironment function and revert some changes to tests. Signed-off-by: Daniel Nephin <dnephin@docker.com>

AkihiroSuda commented Mar 15, 2017

View reviewed changes

vdemeester approved these changes Mar 15, 2017

View reviewed changes

compose: update the comment about MappingWithEquals

2cdc9c4

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

dnephin approved these changes Mar 17, 2017

View reviewed changes

vdemeester merged commit 8bc7038 into moby:master Mar 17, 2017

GordonTheTurtle added this to the 17.05.0 milestone Mar 17, 2017

This was referenced Mar 23, 2017

Add ability to pass wildcard pattern for env vars #31868

Closed

[17.04] compose: fix environment interpolation from the client #32033

Merged

vdemeester mentioned this pull request Mar 27, 2017

docker-compose does not accept variable substitution in some cases #32118

Closed

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#30781 from AkihiroSuda/fix-stack-env

6e24fc3

compose: fix environment interpolation from the client

Code-Egg mentioned this pull request Mar 9, 2020

Stack deploy issue litespeedtech/ols-docker-env#9

Closed

		@@ -358,10 +363,18 @@ func resolveEnvironment(serviceConfig *types.ServiceConfig, workingDir string) e

		serviceConfig.Environment = environment

		@@ -135,7 +135,10 @@ type StringOrNumberList []string

		// MappingWithEquals is a mapping type that can be converted from a list of
		// key=value strings

Conversation

AkihiroSuda commented Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda commented Feb 7, 2017

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Feb 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Mar 7, 2017

Uh oh!

AkihiroSuda commented Mar 8, 2017

Uh oh!

AkihiroSuda commented Mar 8, 2017

Uh oh!

AkihiroSuda commented Mar 8, 2017

Uh oh!

dnephin commented Mar 14, 2017

Uh oh!

AkihiroSuda commented Mar 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Mar 17, 2017

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Mar 22, 2017

Uh oh!

dnephin commented Mar 22, 2017

Uh oh!

thaJeztah commented Mar 22, 2017

Uh oh!

AkihiroSuda commented Mar 23, 2017

Uh oh!

bitsofinfo commented Nov 29, 2017

Uh oh!

thaJeztah commented Nov 29, 2017

Uh oh!

thaJeztah commented Nov 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

AkihiroSuda commented Feb 7, 2017 •

edited

Loading