Skip to content
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.

Sign up for GitHub

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

Jump to bottom

The behavior of .envsh executed by docker-entrypoint.sh #745

Open
yu-shiba opened this issue Jan 21, 2023 · 6 comments · May be fixed by #746
Open

The behavior of .envsh executed by docker-entrypoint.sh #745

yu-shiba opened this issue Jan 21, 2023 · 6 comments · May be fixed by #746

Comments

@yu-shiba
Copy link

Describe the problem

The .envsh merged in #687 is a very nice solution, but the current implementation does not passed to nginx.
This is due to bash/dash (POSIX spec?). This is because the pipeline is run in subshell.
https://www.gnu.org/software/bash/manual/html_node/Pipelines.html

The pipeline does not affect the behavior of 20-envsubst-on-templates.sh because the environment variables are reflected in the pipeline, but it may be a confusing implementation.
(In fact, I was fitted with a project that incorporated 20-envsubst-on-templates.sh.)

To Reproduce

cat << EOT >> Dockerfile
FROM nginx:latest

COPY 10-export-env.envsh /docker-entrypoint.d/
COPY nginx /usr/sbin/nginx
EOT

cat << EOT >> 10-export-env.envsh
#!/bin/sh

export FOO=bar
EOT

cat << EOT >> nginx
#!/bin/sh

env
EOT
chmod +x 10-export-env.envsh nginx
docker build -t test .
docker run --rm test

Expected behavior

Exported environment variables are reflected in nginx
@yu-shiba yu-shiba linked a pull request Jan 21, 2023 that will close this issue
Open
@thresheek
Copy link
Collaborator

Hello @yu-shiba san!

Indeed, the variable exported does not go into resulting runtime of nginx binary, however it's still available during the run of entrypoint scripts. So I'm not sure about the 20-envsubst-on-templates.sh remark.

Let me illustrate:

cat > Dockerfile << EOT
FROM nginx:latest

COPY 10-export-env.envsh /docker-entrypoint.d/
COPY 40-show-env.sh /docker-entrypoint.d/
EOT

cat > 10-export-env.envsh << EOT
#!/bin/sh

export FOO=bar
EOT

cat > 40-show-env.sh << EOT
#!/bin/sh

env
EOT

chmod +x *sh

docker build -t test .

docker run --rm test
/docker-entrypoint.sh: /docker-entrypoint.d/ is not empty, will attempt to perform configuration
/docker-entrypoint.sh: Looking for shell scripts in /docker-entrypoint.d/
/docker-entrypoint.sh: Sourcing /docker-entrypoint.d/10-export-env.envsh
/docker-entrypoint.sh: Launching /docker-entrypoint.d/10-listen-on-ipv6-by-default.sh
10-listen-on-ipv6-by-default.sh: info: Getting the checksum of /etc/nginx/conf.d/default.conf
10-listen-on-ipv6-by-default.sh: info: Enabled listen on IPv6 in /etc/nginx/conf.d/default.conf
/docker-entrypoint.sh: Launching /docker-entrypoint.d/20-envsubst-on-templates.sh
/docker-entrypoint.sh: Launching /docker-entrypoint.d/30-tune-worker-processes.sh
/docker-entrypoint.sh: Launching /docker-entrypoint.d/40-show-env.sh
HOSTNAME=674344fb7d59
HOME=/root
PKG_RELEASE=1~bullseye
container=podman
TERM=xterm
NGINX_VERSION=1.23.3
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
NJS_VERSION=0.7.9
FOO=bar
PWD=/
/docker-entrypoint.sh: Configuration complete; ready for start up
2023/01/21 01:34:22 [notice] 1#1: using the "epoll" event method

@yu-shiba
Copy link
Author

Hello @thresheek,

My apologies for not explaining properly. (It is my English problem).

I have introduced a mechanism to replace environment variables in other projects using 20-envsubst-on-templates.sh. Of course, I have also installed docker-entrypoint.sh.
Then, I needed to do environment variable injection in the parent process, and when I used .envsh, it did not work as I intended, and I was confused.
And I understood that it was due to the shell pipeline specification as I previously explained.

As you indicated, the current behavior doesn't affect the .sh and .envsh series run from docker-entrypoint.sh, but I figured someone else might have the same problem.

I've submitted a PR (#746) to change this behavior because it's easy to do.
If this PR conforms to nginx's policy, I would appreciate it if you could apply it.

@thresheek
Copy link
Collaborator

Hello @yu-shiba san!

This would be a behaviour change. While I understand this might be useful for some cases, it also might accidentally leak some sensitive information to a resulting image.

@yu-shiba
Copy link
Author

Hello @thresheek,

Given the particularities of .envsh, I thought that the user controls the environment variables to be exported.

Normally, source(.) in shell is done to reflect the environment variable to itself, but in /docker-entrypoint.sh I would think that source(.) in /docker-entrypoint.sh would work the same way. .envsh in /docker-entrypoint.sh is probably considered to be the same.
Therefore, I think .envsh users will implement it without including sensitive information in the environment variables they export.

Of course, the current implementation can isolate the environment from problematic code, and if you want to pass environment variables to nginx in the first place, you can simply use ENV in the Dockerfile.

In fact, nginx only uses environment variables in a few cases (like when referring to them in Lua? I have never used it myself).
However, I think it is also complicated to have to overwrite /docker-entrypoint.sh even in a few cases.

@thresheek
Copy link
Collaborator

nginx can use some environment variables (and possibly some third-party modules like lua can too): http://nginx.org/en/docs/ngx_core_module.html#env and you need to explicitely list them in that directive.

This means that you still need to provide an actual nginx configuration with those vars (set or unset), and whether they're exported to runtime is irrelevant...
I believe the current behaviour of .envsh and 20-envsubst-on-templates.sh allows the user to use those variables that without changing docker-entrypoint.sh script, therefore we don't need this change at all.

@yu-shiba
Copy link
Author

This means that you still need to provide an actual nginx configuration with those vars (set or unset), and whether they're exported to runtime is irrelevant...

I had missed this. You are right, it has to be set with the nginx configuration (nginx.conf).
However, I guess this env directive eliminates one concern about changing to this behavior this time.

I believe the current behaviour of .envsh and 20-envsubst-on-templates.sh allows the user to use those variables that without changing docker-entrypoint.sh script, therefore we don't need this change at all.

I guess it would be like turning nginx.conf into a template and processing it in two steps.

cat > Dockerfile << EOT
FROM nginx:latest

COPY 10-export-env.envsh /docker-entrypoint.d/
COPY 40-show-env.sh /docker-entrypoint.d/
COPY 50-move-nginx-conf.sh /docker-entrypoint.d/
COPY nginx /usr/sbin/nginx

COPY nginx.conf.template /etc/nginx/templates/
EOT

cat > 10-export-env.envsh << EOT
#!/bin/sh

export FOO=bar
EOT

cat > 40-show-env.sh << EOT
#!/bin/sh

env
EOT

cat > 50-move-nginx-conf.sh << EOT
#!/bin/sh

mv -f /etc/nginx/conf.d/nginx.conf /etc/nginx
EOT

cat > nginx << EOT
#!/bin/sh -x

env
cat /etc/nginx/nginx.conf
EOT

chmod +x *sh nginx

cat > nginx.conf.template << 'EOT'
...
env FOO=${FOO};
...
EOT

docker build -t test .
docker run --rm test
/docker-entrypoint.sh: /docker-entrypoint.d/ is not empty, will attempt to perform configuration
/docker-entrypoint.sh: Looking for shell scripts in /docker-entrypoint.d/
/docker-entrypoint.sh: Sourcing /docker-entrypoint.d/10-export-env.envsh
/docker-entrypoint.sh: Launching /docker-entrypoint.d/10-listen-on-ipv6-by-default.sh
10-listen-on-ipv6-by-default.sh: info: Getting the checksum of /etc/nginx/conf.d/default.conf
10-listen-on-ipv6-by-default.sh: info: Enabled listen on IPv6 in /etc/nginx/conf.d/default.conf
/docker-entrypoint.sh: Launching /docker-entrypoint.d/20-envsubst-on-templates.sh
20-envsubst-on-templates.sh: Running envsubst on /etc/nginx/templates/nginx.conf.template to /etc/nginx/conf.d/nginx.conf
/docker-entrypoint.sh: Launching /docker-entrypoint.d/30-tune-worker-processes.sh
/docker-entrypoint.sh: Launching /docker-entrypoint.d/40-show-env.sh
HOSTNAME=d0817cd9e1ae
HOME=/root
PKG_RELEASE=1~bullseye
NGINX_VERSION=1.23.3
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
NJS_VERSION=0.7.9
FOO=bar
PWD=/
/docker-entrypoint.sh: Launching /docker-entrypoint.d/50-move-nginx-conf.sh
/docker-entrypoint.sh: Configuration complete; ready for start up
+ env
HOSTNAME=d0817cd9e1ae
HOME=/root
PKG_RELEASE=1~bullseye
NGINX_VERSION=1.23.3
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
NJS_VERSION=0.7.9
PWD=/
+ cat /etc/nginx/nginx.conf
...
env FOO=bar;
...

It is certainly possible, but this part seems forced.

env FOO=${FOO};

I would like to write it this way.

env FOO;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Change the loop that executes .envsh from pipeline(while + read) to for. yu-shiba/docker-nginx
2 participants
@yu-shiba @thresheek