Nginx latest runtime crashloop #713
Conversation
| && touch /var/run/nginx.pid \ | ||
| && chown -R nginx:nginx /var/cache/nginx /var/run/nginx.pid | ||
|
|
||
| COPY --from=builder /etc/nginx/pulp/*.conf /etc/nginx/conf.d |
There was a problem hiding this comment.
I am unsure of changing the location for the plugin's custom nginx files. I just changed it to /etc/nginx/pulp to put it in line with our all in one images (see #703). It would feel rude to change this location again and have users have to update their nginx files again. I think the operator's nginx file should be updated to point to this new location.
There was a problem hiding this comment.
That makes sense.
There was a problem hiding this comment.
I'd like the operator to not modify /etc/nginx/nginx.conf at all (if possible) and do all its work in /etc/nginx/pulp (or conf.d).
There was a problem hiding this comment.
So there are a couple of other related problems with these extra pulp config files. They are set up as location blocks, which aren't valid in http blocks, which is where the default nginx.conf (and current pulp-web:latest container) includes conf.d/*.conf. So those /etc/nginx/pulp config files wouldn't work if included in nginx.conf as it currently stands if you did s/conf.d/pulp/ in there.
So nginx.conf would need to grow a server{} block, or the include /etc/nginx/pulp/*.conf would need to be added to /etc/nginx/conf.d/default.conf. Looking at how pulp/pulp:latest does it, it sets up a server block, but doesn't include this /etc/nginx/pulp folder either, though the default.conf is handled very differently than the stock nginx:latest. That folder is empty though when I look at it. Is that all populated by docker-compose or something?
There was a problem hiding this comment.
pulp/pulp does include /etc/nginx/pulp folder, it just uses a relative path. (https://github.com/pulp/pulp-oci-images/blob/latest/images/s6_assets/nginx.conf.j2#L106). For pulp/pulp the nginx.conf file is created by a template script (https://github.com/pulp/pulp-oci-images/blob/latest/images/s6_assets/template_nginx.py) and is always place at /etc/nginx/nginx.conf by the nginx service we set up (https://github.com/pulp/pulp-oci-images/blob/latest/images/s6_assets/init/nginx#L3).
The pulp-web container is different. We expect users to bring their own nginx.conf file if they want full customization, else they can use the default one we provide in our example. (https://github.com/pulp/pulp-oci-images/blob/latest/images/compose/assets/nginx/nginx.conf.template) Of course our default example also requires to be templated, so you must also have the pulp-web service start with a command to the template script (https://github.com/pulp/pulp-oci-images/blob/latest/images/compose/assets/bin/nginx.sh).
Anyway, I still think we should change the pulp-operator /etc/nginx/nginx.conf since that is how the other installs work. What do you think?
There was a problem hiding this comment.
I think that's reasonable. If docker-compose already runs as root, we can close this, though reducing the layers is still a desirable outcome.
There was a problem hiding this comment.
I would accept the change reducing the layer count.
| RUN mkdir -p /var/cache/nginx \ | ||
| && touch /var/run/nginx.pid \ | ||
| && chown -R nginx:nginx /var/cache/nginx /var/run/nginx.pid |
There was a problem hiding this comment.
Can you explain the reasoning behind these changes? I'm just not understanding what is missing in the operator to warrant these changes.
There was a problem hiding this comment.
Nginx on startup tries to create folders in /var/cache/nginx and a pid file. But the operator has runAsRoot: False set on the pulp-web deployment, so it can't create the folders or pid file. The uid:gid from the operator don't line up with the nginx user (700 vs 101).
There was a problem hiding this comment.
Fix the operator to run as root (since nginx drops privileges) and the problem goes away.
There was a problem hiding this comment.
Here's the permissions on the nginx:latest:
root@4cbe14db5a19:/# ls -dl /var/cache/nginx
drwxr-xr-x. 2 root root 6 Nov 26 16:44 /var/cache/nginx
root@4cbe14db5a19:/# ls -ld /var/run/
drwxr-xr-x. 1 root root 42 Jan 16 22:47 /var/run/
* Reduce layers by combining shell commands * Copy extra config into /etc/nginx/conf.d, where it will be loaded correctly * Don't create unused /www/data directory * Run as nginx:nginx
josephtate
force-pushed
the
nginx-latest-runtime-crashloop
branch
from
c9ab22d to
ab17042
Compare
|
I didn't change pulp-minimal. Not sure why it's failing the actions workflow. |
Well you did change pulp-web, which is what pulp-minimal uses to be accessible. The logs aren't very helpful, either. Maybe it's the line |
I had trouble running the latest images with the current pulp-operator. These changes got me running.
Really the operator should switch to running as root since nginx drops privileges, so feel free to close this PR if that mechanism is preferred, though I'd still recommend using fewer Dockerfile commands as included here to reduce the layer counts.
BTW, the contributing guidelines, and code of conduct links are broken in the github new user help.