Add container environment variables correctly to the health check by boaz0 · Pull Request #33249 · moby/moby

boaz0 · 2017-05-17T13:12:06Z

fixes/closes #33021

- What I did

Add environment variables and set the environment variables for the health check.

- How I did it

Use ReplaceOrAppendEnvValues, CreateDaemonEnvironment and setupLinkedContainers to collect all of them and to set them correctly.

- How to verify it

Run the following test:

$ TESTFLAGS='-check.f TestUnsetEnvVarHealthCheck' hack/make.sh test-integration-cli

mlaventure · 2017-05-17T14:53:34Z

opts/env_test.go

This and the one below are not valid environment variable name usually

Agree but when doing os.Setenv("asd!qwe", "myvalue") it works.
So I am not sure what to do here. In addition, I have no idea whether that name is valid on windows too?

Tried to Google "valid environment variable names" but the only thing I found is that = can't be in the environment variable.

I think technically any character except = and NUL are allowed in the variable names, but implementations usually restrict themselves to the set of Portable characters as defined by POSIX -> [a-zA-Z_][a-zA-Z0-9_]*

Nope; we don't validate env var names; IIRC there's no official standard for it, and we did validation in the past, but broke people that used systems that don't comply with those rules.

We leave the validation to the container itself to tell if it's valid or not

Fair enough, I have yet to use a shell that accept non POSIX names though :)

for reference, see #16608

I was about to quote the open group page for this 👍

boaz0 · 2017-05-17T15:06:08Z

OK, now I realized that if the variable isn't set, it should be discarded (DockerSuite.TestRunEnvironmentErase).

I am going to change this now.

mlaventure

LGTM

thaJeztah · 2017-05-17T23:35:40Z

Failure in docker-py looks related;

19:09:27 =================================== FAILURES ===================================
19:09:27 ______ CreateContainerTest.test_create_with_environment_variable_no_value ______
19:09:27 /docker-py/tests/integration/api_container_test.py:370: in test_create_with_environment_variable_no_value
19:09:27     environment={'Foo': None, 'Other': 'one', 'Blank': ''},
19:09:27 /docker-py/docker/api/container.py:446: in create_container
19:09:27     return self.create_container_from_config(config, name)
19:09:27 /docker-py/docker/api/container.py:457: in create_container_from_config
19:09:27     return self._result(res, True)
19:09:27 /docker-py/docker/api/client.py:220: in _result
19:09:27     self._raise_for_status(response)
19:09:27 /docker-py/docker/api/client.py:216: in _raise_for_status
19:09:27     raise create_api_error_from_http_exception(e)
19:09:27 /docker-py/docker/errors.py:30: in create_api_error_from_http_exception
19:09:27     raise cls(e, response=response, explanation=explanation)
19:09:27 E   APIError: 500 Server Error: Internal Server Error ("environment variable does not exists")
19:09:27 ======== 1 failed, 205 passed, 31 skipped, 2 xfailed in 244.48 seconds =========
19:09:27 ---> Making bundle: .integration-daemon-stop (in bundles/17.06.0-dev/test-docker-py)
19:09:27 +++++ cat bundles/17.06.0-dev/test-docker-py/docker.pid
19:09:27 ++++ kill 31590
19:09:28 ++++ /etc/init.d/apparmor stop

boaz0 · 2017-05-18T06:08:04Z

@thaJeztah yes it is. I am working on fixing it 😺

thaJeztah · 2017-05-19T11:45:44Z

Did some testing with this. First ran a regular container, no healthcheck, and variations of ENV vars, to check the behavior;

$ docker build -t repro-33249 -<<EOF
FROM busybox
ENV IWILLBEUNSET=thiswasmyvalue
EOF


$ FROMENV=hello docker run -dit --name foobar \
  -e IDONOTEXIST \
  -e IWILLBEUNSET \
  -e IAMEMPTY= \
  -e FROMENV \
  repro-33249

$ docker inspect --format '{{json .Config.Env }}' foobar | jq .
[
  "IDONOTEXIST",
  "IWILLBEUNSET",
  "IAMEMPTY=",
  "FROMENV=hello",
  "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
]

$ docker exec foobar env
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
HOSTNAME=211bf084aab2
IAMEMPTY=
FROMENV=hello
HOME=/root

So; -e SOMEVAR will be ;

empty if followed by =
get its value from the current environment (if exists)
unset if it existed in the image, but does not exist in the environment

Basically, in .Config.Env, "SOMEVAR" means: unset the env-var

With this patch (adding a healthcheck):

$ docker build -t repro-33249 -<<EOF
FROM busybox
ENV IWILLBEUNSET=thiswasmyvalue
HEALTHCHECK --interval=1s --timeout=5s --retries=5 CMD env; exit 0;
EOF

$ FROMENV=hello docker run -dit --name foobar \
  -e IDONOTEXIST \
  -e IWILLBEUNSET \
  -e IAMEMPTY= \
  -e FROMENV \
  repro-33249

$ docker inspect --format '{{json .Config.Env }}' foobar | jq .
[
  "IDONOTEXIST",
  "IWILLBEUNSET",
  "IAMEMPTY=",
  "FROMENV=hello",
  "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
]

$ docker exec foobar env
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
HOSTNAME=2c0f5e0add62
IAMEMPTY=
FROMENV=hello
HOME=/root

All of the above are the same (as expected).

Checking the healthcheck;

$ docker inspect --format '{{json .State.Health.Log }}' foobar | jq .
[
  {
    "Start": "2017-05-19T11:27:41.45071088Z",
    "End": "2017-05-19T11:27:41.505086124Z",
    "ExitCode": 0,
    "Output": "SHLVL=1\nHOME=/root\nPATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin\nFROMENV=hello\nIAMEMPTY=\nPWD=/\n"
  }
]

(expanding, and sorting a bit)

PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
IAMEMPTY=
FROMENV=hello
HOME=/root
SHLVL=1
PWD=/

Interesting; HOSTNAME is present in docker exec, but not present in the healthcheck's environment

thaJeztah · 2017-05-19T11:46:36Z

So, as far as I can see, this works as expected.

I wonder if we (separate from this PR) should look into why HOSTNAME is not present in the health check's environment.

thaJeztah

LGTM, but @mlaventure PTAL if this is all good. Also wondering how a regular run / exec differs from this health check

boaz0 · 2017-05-21T18:20:56Z

@thaJeztah I have an idea what happened! Thanks for noticing the "HOSTNAME" thing.

The health check process doesn't have all the environment varialbes in the container or has them set incorrectly. This patch should fix that problem. Signed-off-by: Boaz Shuster <ripcurld.github@gmail.com>

mlaventure

LGTM.

Thanks for the fix!

GordonTheTurtle added the status/0-triage label May 17, 2017

thaJeztah added status/2-code-review and removed status/0-triage labels May 17, 2017

thaJeztah added this to the 17.06.0 milestone May 17, 2017

mlaventure reviewed May 17, 2017

View reviewed changes

mlaventure approved these changes May 17, 2017

View reviewed changes

boaz0 changed the title ~~Validate env vars during creation and set env vars if not exist~~ Clean unset variable in the health check process May 18, 2017

thaJeztah added the process/cherry-pick label May 18, 2017

thaJeztah approved these changes May 19, 2017

View reviewed changes

Add container environment variables correctly to the health check

5836d86

The health check process doesn't have all the environment varialbes in the container or has them set incorrectly. This patch should fix that problem. Signed-off-by: Boaz Shuster <ripcurld.github@gmail.com>

boaz0 changed the title ~~Clean unset variable in the health check process~~ Add container environment variables correctly to the health check May 21, 2017

thaJeztah added the rebuild/windowsRS1 label May 30, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label May 30, 2017

thaJeztah mentioned this pull request May 31, 2017

17.06.0 RC2 tracker docker/for-linux#2

Closed

23 tasks

mlaventure approved these changes May 31, 2017

View reviewed changes

mlaventure added status/4-merge and removed status/2-code-review labels May 31, 2017

mlaventure merged commit 7c2f201 into moby:master May 31, 2017

andrewhsu mentioned this pull request Jun 6, 2017

17.06.0 RC3 tracker docker/for-linux#8

Closed

40 tasks

seemethere mentioned this pull request Jun 9, 2017

Add container environment variables correctly to the health check docker-archive/docker-ce#47

Merged

thaJeztah added process/cherry-picked and removed process/cherry-pick labels Jun 10, 2017

Conversation

boaz0 commented May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlaventure May 17, 2017

Choose a reason for hiding this comment

Uh oh!

boaz0 May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlaventure May 17, 2017

Choose a reason for hiding this comment

Uh oh!

thaJeztah May 17, 2017

Choose a reason for hiding this comment

Uh oh!

mlaventure May 17, 2017

Choose a reason for hiding this comment

Uh oh!

thaJeztah May 17, 2017

Choose a reason for hiding this comment

Uh oh!

mlaventure May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boaz0 commented May 17, 2017

Uh oh!

mlaventure left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented May 17, 2017

Uh oh!

boaz0 commented May 18, 2017

Uh oh!

thaJeztah commented May 19, 2017

With this patch (adding a healthcheck):

Uh oh!

thaJeztah commented May 19, 2017

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

boaz0 commented May 21, 2017

Uh oh!

mlaventure left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

boaz0 commented May 17, 2017 •

edited

Loading

boaz0 May 17, 2017 •

edited

Loading

mlaventure May 17, 2017 •

edited

Loading