[WIP] Dockerfile Rework #56
Conversation
Codecov Report
@@ Coverage Diff @@
## master #56 +/- ##
==========================================
+ Coverage 47.14% 47.81% +0.66%
==========================================
Files 76 72 -4
Lines 2507 2405 -102
==========================================
- Hits 1182 1150 -32
+ Misses 1325 1255 -70Continue to review full report at Codecov.
|
Dockerfile
Outdated
|
|
||
| ENV CONFIG_DIR=/etc/promgen | ||
|
|
||
| USER promgen |
There was a problem hiding this comment.
I have to admit I'm quite ignorant of docker best practices, but I thought it was generally discourated to run as root. I know Prometheus 2.0 was changed to not run as root ( prometheus/prometheus#1637 ) though I think that also caused issues for some other things. Hmm.
There was a problem hiding this comment.
You are right, it is not as simple as I wrote in the description.
It is theoretically not possible for the docker root user to break out (and gain host root privileges). In production, containers are usually run as a non-root user to add an additional layer of security. And to prevent attackers from compromising the (ephemeral) container fs, of course.
Using nobody as the user to execute commands would be good for security and does not pose an issue for the Promgen Web App. The worker, however, needs write access to an external / shared volume.
Adding a new user to a docker image would require very specific configuration on the host system: Creating a new isolated user with the same uid and granting access only to the docker volume mount point.
By using root the "self-contained" aspect of the docker image is preserved. In more advanced or production deployments the user will (and should) most likely be changed to a custom user by the operator.
There was a problem hiding this comment.
I started reading up on the issue, as I'm not the most up-to-date either.
While most agree that it's best not to run as root, there is no consensus on what to do instead.
To me, it seems best to run under a random but static uid by default.
For Promgen the situation is slightly more complicated, as setting up write access to the external mount point is not optional but required for running the worker.
|
I also need to test a bit more, but can probably move the 'apk del' to the end to make the container a bit smaller diff --git a/Dockerfile b/Dockerfile
index 733a9a2..75e5547 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -22,11 +22,12 @@ RUN set -ex; \
apk add --no-cache curl tar; \
curl -L -s $PROMETHEUS_DOWNLOAD_URL \
| tar -xz -C /usr/local/bin --strip-components=1 prometheus-${PROMETHEUS_VERSION}.linux-amd64/promtool; \
- apk del curl tar; \
- rm -rf /var/cache/apk; \
mkdir -p /etc/prometheus; \
pip install -r /tmp/requirements.txt; \
pip install -e .; \
+ apk del curl tar build-base; \
+ rm -rf /var/cache/apk; \
SECRET_KEY=1 promgen collectstatic --noinput;
EXPOSE 8000 |
|
Nice savings :) |
Docker images for python 3.5.3 are no longer built
* Env vars only relevant during build now use ARG * Combined two ENV instructions * Removed PIP_NO_CACHE_DIR declaration (unclear why )
|
I rebuilt the container from the current master branch, and the size has been greatly reduced 👍 |
|
The primary open question is: What user should promgen run as? (comments) The most secure option is to create a new user in the container with a random but static uid and gid. However, this poses a problem because the worker has to write files to an (external) docker volume. By running as such a user, operators have to configure appropriate permissions for the prometheus configuration files. Some options:
|
|
Yeah, I do not have a good answer to that since I am quite unaware of the best practices. I think I will yield that decision to what you think is best :) |
Check PR line#56 for background and more details
|
I had some further ideas for a more holistic approach to the docker image, I introduced the following changes:
The aim is to have a simpler setup experience as well as to accomodate production setups. To do:
|
Dockerfile
Outdated
| EXPOSE 8000 | ||
| RUN set -ex; \ | ||
| addgroup -g 16395 promgen; \ | ||
| adduser -D -u 16395 -G promgen promgen; \ |
There was a problem hiding this comment.
Is there something special about the value 16395 here ? Shouldn't be a problem, I'm just curious why you chose this value :)
There was a problem hiding this comment.
There is nothing special about this value, it is an arbitrary uid >> 1000. This is the random but static uid I wrote about before.
Some projects choose to simply run adduser as the first command and hope for the same uid to be allocated every time. For promgen, the uid is of the upmost importance, because if permissions are misconfigured, the worker will be unable to write any configuration files. That's why I think explicitly setting the uid is a good idea.
Because the uid is much greater than 1000, it is unlikely to map to any user on the local system.
There was a problem hiding this comment.
Ok. Should be fine, I just wish there was more authoritative sources about values like this 😓
I agree blindly trusting adduser is scary.
For my own deployment I use 9090:9090 for Prometheus (to match the default port) but I suppose in Promgen's case we can't easily do something similar.
| #!/bin/bash | ||
| #!/bin/sh | ||
|
|
||
| function check_migrate() { |
There was a problem hiding this comment.
Moving this to a function to reuse is a good idea 👍
docker/docker-entrypoint.sh
Outdated
| } | ||
|
|
||
| function copy_static() { | ||
| if [ ! -f /srv/promgen/www/static/.present ]; then |
There was a problem hiding this comment.
Just reading the code, I'm not entirely sure the purpose for having both /srv/promgen/www and /usr/src/app/static. Is this so that one can mount the directory and then serve the files from nginx/apache or for some other reason?
I think a comment either in the file or the docker instructions will be necessary for clarification :)
There was a problem hiding this comment.
Yes, the reason is the docker volume management: Every time the container is created, a new volume is allocated and mounted at the path /srv/promgen/www. This means all content in the directory (e.g. created in the Dockerfile) will be invisible.
So instead, on the start of the web app, the files are copied across, if they don't exist yet. The .present file represents a minor security consideration: When not forbidden by the web server, the file will be accessible by anyone with HTTP access. I decided to only run collectstatic in the Dockerfile, just in case it involves some computationally complex work.
Other containers can then mount the volume (e.g. with the --volumes-from flag, by mounting a custom volume in both containers or by using the bind volumes).
There was a problem hiding this comment.
One consideration instead of adding a new file (.present) perhaps can just test for a file that we expect to be there, /srv/promgen/www/static/js/promgen.js otherwise, seems like a reasonable explanation :)
docker/docker-entrypoint.sh
Outdated
| shift | ||
| set -- gunicorn "promgen.wsgi:application" "$@" | ||
|
|
||
| exec promgen bootstrap --noinput "$@" |
There was a problem hiding this comment.
I could be mistaken, but I think using exec here means that this shell will be replaced by the running program so the later steps will not run.
There was a problem hiding this comment.
I meant to check this, you're absolutely right, exec just wraps the corresponding syscall.
|
I made the changes we discussed and also removed Still have to properly test everything though. |
|
This pr is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days |
|
Some of the ideas implemented in the PR are still valid but must be re-applied against the current version. I'll re-apply next week. |
Codecov Report
@@ Coverage Diff @@
## master #56 +/- ##
=======================================
Coverage 47.81% 47.81%
=======================================
Files 72 72
Lines 2405 2405
=======================================
Hits 1150 1150
Misses 1255 1255
Continue to review full report at Codecov.
|
|
Probably quite out of date at this point 😅 |
I re-wrote the
Dockerfileto be more in line with commonDockerfiles.The only significant difference is the user running promgen: I removed the promgen user, that means that the web app and the worker are now run as root. This simplifies the permissions required for the configuration files. Running applications in docker as root is a common strategy.