Support for env var replacement #19

wtrocki · 2017-01-31T23:44:34Z

Motivation

Add init script support. This script would be executed before launching nginx proxy and would allow to modify nginx configuration. Script can be used to replace environment variables or setup additional elements in image. Some examples and test provided.

Notes

Nginx configuration can contain variables that start with dollar which makes difficult to replace all placeholders. Using envreplace is still possible but we would need to specify list of the environment variables as input. Alternative solution would be to use sed to replace some placeholders but this would reduce visibility and IMHO make this more complex than it should be.

I wanted to make this solution generic enough to be used for various use cases. One of the most common use cases for this container is to use it as proxy. To proxy to backends using dns names , dns setup is required. Unfortunately nginx using internal dns resolver and cannot relay on resolv.conf file.

In example for init script I have added solution for this problem - reading values from resolv.conf and injecting them into script as environment variables. It may be reasonable to include resolver directive into global script.

Implementation for #18

centos-ci · 2017-01-31T23:46:49Z

Can one of the admins verify this patch?

rhscl-bot · 2017-01-31T23:47:00Z

Can one of the admins verify this patch?

omron93 · 2017-02-01T09:23:00Z

@wtrocki Git is complaining about missing newline at the end of some modified files. Please add it.

[test]

omron93 · 2017-02-01T10:02:24Z

Tests are failing due to missing $PRE_RUN_SCRIPT when running s2i build. Dockerfile.rhel7 is not modified.

---> Copying nginx configuration files...
09:33:08 './nginx-cfg/default.conf' -> '/opt/app-root/etc/nginx.d/default.conf'
09:33:08 E0201 04:32:48.557733 01064 util.go:91] cp: missing destination file operand after './bin/init_script'
09:33:08 E0201 04:32:48.557751 01064 util.go:91] Try 'cp --help' for more information.
09:33:08 ---> Copying nginx init script...

Also tests fail with:

09:34:35 Using system dns server 172.19.0.12 
09:34:35 /opt/app-root/etc/init_script: line 13: /opt/app-root/etc/nginx.d/default.conf: Permission denied
09:34:35 server {
09:34:35 	listen       8080 default_server;
09:34:35 	server_name  localhost2;
09:34:35 	root         /opt/app-root/src;
09:34:35 	index        ${INDEX_FILE};
09:34:35 	resolver ${DNS_SERVER};
09:34:35 
09:34:35 }
09:34:35 nginx: [emerg] directive "index" is not terminated by ";" in /opt/app-root/etc/nginx.d/default.conf:5

To note: CI is configured, that only members of sclorg/ github organization can trigger tests.

omron93 · 2017-02-01T10:06:15Z

Everyone can run same tests as in CI by make test UPDATE_BASE=1 SKIP_SQUASH=1

[test]

wtrocki · 2017-02-01T10:12:34Z

@omron93 - Thanks for help on this one. I would try to fix and also extend tests now.

omron93 · 2017-02-01T10:13:41Z

@wtrocki You are welcomed. Thanks for this PR.

wtrocki · 2017-02-01T21:45:12Z

@omron93 - Tests are passing now on my local machine.

omron93 · 2017-02-02T08:47:17Z

[test]

omron93 · 2017-02-02T08:48:07Z

1.8/s2i/bin/assemble

@@ -9,9 +9,17 @@ if [ -d ./nginx-cfg ]; then
  echo "---> Copying nginx configuration files..."
  if [ "$(ls -A ./nginx-cfg/*.conf)" ]; then
    cp -v ./nginx-cfg/*.conf "${NGINX_CONFIGURATION_PATH}"
+    chmod -Rf 777 ${NGINX_CONFIGURATION_PATH}


@wtrocki Is 777 necessary? It is very open...

In order to change configuration in running container we would need to have write access to config for default user. Files are created by root user, so we can:

Change file owner to default (risky as container can use different user)

Make files writable by others by setting 666 👿 mask on file

Making files writable do not impose security risk. If attacker would get access to the container he can switch configuration to any file he wants and then send HUP signal to the nginx process to reload it.

Change file owner to default (risky as container can use different user)

Make files writable by others by setting 666 👿 mask on file

I am not sure, but I think GID remains the same. So some other images g+rw is used to allow running with arbitrary user.

omron93 · 2017-02-02T15:15:41Z

Anyway, it LGTM. But I am not experienced with nginx, so I would be happy someone else take a look to it too.
@notroj ?

centos-ci · 2017-02-07T12:42:31Z

Can one of the admins verify this patch?

wtrocki · 2017-03-14T09:18:55Z

ping @notroj

wtrocki · 2017-06-10T21:47:11Z

Any update on this? This feature will be really useful for this image.

hhorak · 2017-09-18T16:01:34Z

Sorry, that it took longer than I'd wish. We've worked on a similar feature in case of databases containers and mongodb was the first that included it: sclorg/mongodb-container#239

By that we also wanted to set the same approaches that will work fine when running the container by docker run ... or in OpenShift/k8s. I believe the main idea is similar, but there is a bit different implementation.

The main point is that the image has one or more known trigger points -- in this case it would be the point in "before launching nginx proxy", as you wrote (let's call it simply init). At that point, the starting script would include all bash scripts from either /opt/app-root/src/init or from /usr/share/container-scripts/nginx/init, giving priority for /opt/app-root if the same filename is found in both directories. This is the genera concept how we aim to make the daemon container images extensible.

So, depending on whether the functionality is general enough, your init-script would either be placed into /usr/share/container-scripts/nginx/init in the image itself, or you'd be adding it yourself during start (no matter what environment you use).

Anyway, the bottom line is that I'd like to see this implemented similarly as in case of the mongodb container above..

hhorak · 2017-09-18T16:02:51Z

IMO the approach above should also fix #20.

wtrocki · 2017-09-18T16:43:23Z

@hhorak Great idea! Especially that you can use the same patterns for most of the scl images.
I'm going to close this issue then as it's mostly outdated and now I will also do that differently.

Feel free to ping me on other containers PR for reviews.

Support for env var replacement

5627af2

Add additional line breaks and minor fixes

1849d78

wtrocki force-pushed the ec18-env-var-replacement branch from 1842f38 to 1849d78 Compare February 1, 2017 10:05

Allow to change scripts after s2i process

5482717

omron93 reviewed Feb 2, 2017

View reviewed changes

wtrocki closed this Sep 18, 2017

wtrocki deleted the ec18-env-var-replacement branch September 18, 2017 16:43

kosciCZ mentioned this pull request Oct 19, 2017

Pre-init s2i extensibility #32

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for env var replacement #19

Support for env var replacement #19

wtrocki commented Jan 31, 2017 •

edited

Loading

centos-ci commented Jan 31, 2017

rhscl-bot commented Jan 31, 2017

omron93 commented Feb 1, 2017

omron93 commented Feb 1, 2017 •

edited

Loading

omron93 commented Feb 1, 2017

wtrocki commented Feb 1, 2017

omron93 commented Feb 1, 2017

wtrocki commented Feb 1, 2017

omron93 commented Feb 2, 2017

omron93 Feb 2, 2017

wtrocki Feb 2, 2017 •

edited

Loading

omron93 Feb 2, 2017

omron93 commented Feb 2, 2017

centos-ci commented Feb 7, 2017

wtrocki commented Mar 14, 2017

wtrocki commented Jun 10, 2017

hhorak commented Sep 18, 2017

hhorak commented Sep 18, 2017

wtrocki commented Sep 18, 2017

Support for env var replacement #19

Support for env var replacement #19

Conversation

wtrocki commented Jan 31, 2017 • edited Loading

Motivation

Notes

centos-ci commented Jan 31, 2017

rhscl-bot commented Jan 31, 2017

omron93 commented Feb 1, 2017

omron93 commented Feb 1, 2017 • edited Loading

omron93 commented Feb 1, 2017

wtrocki commented Feb 1, 2017

omron93 commented Feb 1, 2017

wtrocki commented Feb 1, 2017

omron93 commented Feb 2, 2017

omron93 Feb 2, 2017

Choose a reason for hiding this comment

wtrocki Feb 2, 2017 • edited Loading

Choose a reason for hiding this comment

omron93 Feb 2, 2017

Choose a reason for hiding this comment

omron93 commented Feb 2, 2017

centos-ci commented Feb 7, 2017

wtrocki commented Mar 14, 2017

wtrocki commented Jun 10, 2017

hhorak commented Sep 18, 2017

hhorak commented Sep 18, 2017

wtrocki commented Sep 18, 2017

wtrocki commented Jan 31, 2017 •

edited

Loading

omron93 commented Feb 1, 2017 •

edited

Loading

wtrocki Feb 2, 2017 •

edited

Loading