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

[WIP] Warn (and remove) on empty env-var names instead of failing #38449

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@thaJeztah
Copy link
Member

thaJeztah commented Dec 28, 2018

fixes #38439

This patch;

  • Improves validation of empty env-var names to account for additional white-space
  • Instead of failing the container, produce a warning, and strip the empty env-var
  • Improves the error-message, but putting quotes around the value (making it easier
    to find the issue)

Before this patch, creating a container with an empty env-var would fail;

docker run --rm --name test -e "" busybox
invalid argument "" for "-e, --env" flag: invalid environment variable:
See 'docker run --help'.

However, the validation was too specific, and would fail to detect env-var
names containing only white-space:

docker run --rm --name test -e " = " -e "  " busybox

docker container create --name test -e " = " -e "  " busybox
5816ece7bc80289055a725783e18a2c3d7df21b3debdcc09cd99dc532868354a

docker container inspect --format '{{json .Config.Env}}' test
[" = ","  ","PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"]

curl -s \
  --unix-socket /var/run/docker.sock \
  -X POST \
  "http://localhost/v1.37/containers/create?name=test" \
  -H "Content-Type: application/json" \
  -d '{"Image": "busybox:latest","Env":[""],"HostConfig":{"AutoRemove":true}}'

{"message":"invalid environment variable: "}

With this patch applied, both empty and whitespace-only env-var names are detected,
and instead of failing the container, the empty env-vars are removed, and a
warning is returned instead:

docker run --rm --name test -e " = " -e "  " busybox
WARNING: invalid environment variable: " = "
WARNING: invalid environment variable: "  "

docker container create --name test -e " = " -e "  " busybox
WARNING: invalid environment variable: " = "
WARNING: invalid environment variable: "  "
25106252d65e322fc45fb3f0164584cf4854fa0d6f47ced423e4490c2197868b

docker container inspect --format '{{json .Config.Env}}' test
["PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"]

curl -s \
  --unix-socket /var/run/docker.sock \
  -X POST \
  "http://localhost/v1.37/containers/create?name=test" \
  -H "Content-Type: application/json" \
  -d '{"Image": "busybox:latest","Env":[""],"HostConfig":{"AutoRemove":true}}'

{"Id":"f3fd9ef61df36d8bc24c7d9d06d697e7609321002167f7aca4dc13b7c8d25a01","Warnings":["invalid environment variable: \"\""]}

With this patch applied, building from a base-image that contains an invalid
environment variable no longer fails, but still informs the user that an
invalid environment variable was found;

docker inspect --format '{{json .Config.Env}}' invalid
["","PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"]
docker build -t frominvalid -<<'EOF'
FROM invalid
ENV hello=world
RUN echo $hello
EOF

Produces a warning during build, but won't make the build fail:

Sending build context to Docker daemon  2.048kB
Step 1/3 : FROM invalid
 ---> 2037e217e3a9
Step 2/3 : ENV hello=world
 ---> [Warning] invalid environment variable: ""
 ---> Running in eb382e91cadb
Removing intermediate container eb382e91cadb
 ---> c4689179039b
Step 3/3 : RUN echo $hello
 ---> [Warning] invalid environment variable: ""
 ---> Running in 92dbf1c16168
world
Removing intermediate container 92dbf1c16168
 ---> e032e46a8bcd
Successfully built e032e46a8bcd
Successfully tagged frominvalid:latest

Signed-off-by: Sebastiaan van Stijn github@gone.nl

- What I did

- How I did it

- How to verify it

- Description for the changelog

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

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Dec 28, 2018

This is still work-in-progress;

  • Building from an invalid base-image does not strip the invalid env-var, thus the final image will still contain the empty env-var
  • I have not yet tested this patch with BuildKit (DOCKER_BUILDKIT=1); I suspect this patch may need changes in buildkit itself
  • to be discussed: should the CLI-side check be kept, or should we delegate this check to the daemon only?

@tonistiigi @vdemeester

Warn (and remove) on empty env-var names instead of failing
This patch;

- Improves validation of empty env-var names to account for additional white-space
- Instead of failing the container, produce a warning, and strip the empty env-var
- Improves the error-message, but putting quotes around the value (making it easier
  to find the issue)

Before this patch, creating a container with an empty env-var would fail;

```bash
docker run --rm --name test -e "" busybox
invalid argument "" for "-e, --env" flag: invalid environment variable:
See 'docker run --help'.
```

However, the validation was too specific, and would fail to detect env-var
names containing only white-space:

```bash
docker run --rm --name test -e " = " -e "  " busybox

docker container create --name test -e " = " -e "  " busybox
5816ece7bc80289055a725783e18a2c3d7df21b3debdcc09cd99dc532868354a

docker container inspect --format '{{json .Config.Env}}' test
[" = ","  ","PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"]

curl -s \
  --unix-socket /var/run/docker.sock \
  -X POST \
  "http://localhost/v1.37/containers/create?name=test" \
  -H "Content-Type: application/json" \
  -d '{"Image": "busybox:latest","Env":[""],"HostConfig":{"AutoRemove":true}}'

{"message":"invalid environment variable: "}
```

With this patch applied, both empty and whitespace-only env-var names are detected,
and instead of failing the container, the empty env-vars are removed, and a
warning is returned instead:

```bash
docker run --rm --name test -e " = " -e "  " busybox
WARNING: invalid environment variable: " = "
WARNING: invalid environment variable: "  "

docker container create --name test -e " = " -e "  " busybox
WARNING: invalid environment variable: " = "
WARNING: invalid environment variable: "  "
25106252d65e322fc45fb3f0164584cf4854fa0d6f47ced423e4490c2197868b

docker container inspect --format '{{json .Config.Env}}' test
["PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"]

curl -s \
  --unix-socket /var/run/docker.sock \
  -X POST \
  "http://localhost/v1.37/containers/create?name=test" \
  -H "Content-Type: application/json" \
  -d '{"Image": "busybox:latest","Env":[""],"HostConfig":{"AutoRemove":true}}'

{"Id":"f3fd9ef61df36d8bc24c7d9d06d697e7609321002167f7aca4dc13b7c8d25a01","Warnings":["invalid environment variable: \"\""]}
```

With this patch applied, building from a base-image that contains an invalid
environment variable no longer fails, but still informs the user that an
invalid environment variable was found;

```bash
docker inspect --format '{{json .Config.Env}}' invalid
["","PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"]
```

```
docker build -t frominvalid -<<'EOF'
FROM invalid
ENV hello=world
RUN echo $hello
EOF
```

Produces a _warning_ during build, but won't make the build fail:

```
Sending build context to Docker daemon  2.048kB
Step 1/3 : FROM invalid
 ---> 2037e217e3a9
Step 2/3 : ENV hello=world
 ---> [Warning] invalid environment variable: ""
 ---> Running in eb382e91cadb
Removing intermediate container eb382e91cadb
 ---> c4689179039b
Step 3/3 : RUN echo $hello
 ---> [Warning] invalid environment variable: ""
 ---> Running in 92dbf1c16168
world
Removing intermediate container 92dbf1c16168
 ---> e032e46a8bcd
Successfully built e032e46a8bcd
Successfully tagged frominvalid:latest
```

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

@thaJeztah thaJeztah force-pushed the thaJeztah:skip_invalid_env_vars branch from 0f971f6 to 7997a7e Jan 2, 2019

@thaJeztah thaJeztah requested a review from vdemeester as a code owner Jan 2, 2019

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 2, 2019

Codecov Report

Merging #38449 into master will increase coverage by 0.1%.
The diff coverage is 66.66%.

@@            Coverage Diff            @@
##           master   #38449     +/-   ##
=========================================
+ Coverage   36.53%   36.63%   +0.1%     
=========================================
  Files         608      608             
  Lines       45040    45048      +8     
=========================================
+ Hits        16455    16504     +49     
+ Misses      26304    26263     -41     
  Partials     2281     2281
@vdemeester
Copy link
Member

vdemeester left a comment

LGTM 🐯

to be discussed: should the CLI-side check be kept, or should we delegate this check to the daemon only?

I'm guessing it should be kept, mainly because the cli can target daemon's that do not have this check 👼

@olljanat

This comment has been minimized.

Copy link
Contributor

olljanat commented Jan 9, 2019

(reserved for my derek commands)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment