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

Nginx latest runtime crashloop #713

Closed
wants to merge 1 commit into from
Closed

Nginx latest runtime crashloop #713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions images/pulp-minimal/nightly/Containerfile.webserver
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,20 @@
ARG FROM_TAG="nightly"
FROM pulp/pulp-minimal:${FROM_TAG} as builder

RUN mkdir -p /etc/nginx/pulp \
/www/data
RUN ln $(pip3 show pulp_ansible | sed -n -e 's/Location: //p')/pulp_ansible/app/webserver_snippets/nginx.conf /etc/nginx/pulp/pulp_ansible.conf
RUN ln $(pip3 show pulp_container | sed -n -e 's/Location: //p')/pulp_container/app/webserver_snippets/nginx.conf /etc/nginx/pulp/pulp_container.conf
RUN ln $(pip3 show pulp_python | sed -n -e 's/Location: //p')/pulp_python/app/webserver_snippets/nginx.conf /etc/nginx/pulp/pulp_python.conf

RUN mkdir -p /etc/nginx/pulp
RUN ln $(pip3 show pulp_ansible | sed -n -e 's/Location: //p')/pulp_ansible/app/webserver_snippets/nginx.conf /etc/nginx/pulp/pulp_ansible.conf \
&& ln $(pip3 show pulp_container | sed -n -e 's/Location: //p')/pulp_container/app/webserver_snippets/nginx.conf /etc/nginx/pulp/pulp_container.conf \
&& ln $(pip3 show pulp_python | sed -n -e 's/Location: //p')/pulp_python/app/webserver_snippets/nginx.conf /etc/nginx/pulp/pulp_python.conf


FROM docker.io/nginx:latest

RUN mkdir -p /etc/nginx/pulp
COPY --from=builder /etc/nginx/pulp/*.conf /etc/nginx/pulp
RUN mkdir -p /var/cache/nginx \
&& touch /var/run/nginx.pid \
&& chown -R nginx:nginx /var/cache/nginx /var/run/nginx.pid
Comment on lines +12 to +14
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain the reasoning behind these changes? I'm just not understanding what is missing in the operator to warrant these changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix the operator to run as root (since nginx drops privileges) and the problem goes away.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/


COPY --from=builder /etc/nginx/pulp/*.conf /etc/nginx/conf.d
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would accept the change reducing the layer count.


# Run script uses standard ways to run the application
USER nginx:nginx
CMD nginx -g "daemon off;"
18 changes: 10 additions & 8 deletions images/pulp-minimal/stable/Containerfile.webserver
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,20 @@
ARG FROM_TAG="stable"
FROM pulp/pulp-minimal:${FROM_TAG} as builder

RUN mkdir -p /etc/nginx/pulp \
/www/data
RUN ln $(pip3 show pulp_ansible | sed -n -e 's/Location: //p')/pulp_ansible/app/webserver_snippets/nginx.conf /etc/nginx/pulp/pulp_ansible.conf
RUN ln $(pip3 show pulp_container | sed -n -e 's/Location: //p')/pulp_container/app/webserver_snippets/nginx.conf /etc/nginx/pulp/pulp_container.conf
RUN ln $(pip3 show pulp_python | sed -n -e 's/Location: //p')/pulp_python/app/webserver_snippets/nginx.conf /etc/nginx/pulp/pulp_python.conf

RUN mkdir -p /etc/nginx/pulp
RUN ln $(pip3 show pulp_ansible | sed -n -e 's/Location: //p')/pulp_ansible/app/webserver_snippets/nginx.conf /etc/nginx/pulp/pulp_ansible.conf \
&& ln $(pip3 show pulp_container | sed -n -e 's/Location: //p')/pulp_container/app/webserver_snippets/nginx.conf /etc/nginx/pulp/pulp_container.conf \
&& ln $(pip3 show pulp_python | sed -n -e 's/Location: //p')/pulp_python/app/webserver_snippets/nginx.conf /etc/nginx/pulp/pulp_python.conf


FROM docker.io/nginx:latest

RUN mkdir -p /etc/nginx/pulp
COPY --from=builder /etc/nginx/pulp/*.conf /etc/nginx/pulp
RUN mkdir -p /var/cache/nginx \
&& 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/

# Run script uses standard ways to run the application
USER nginx:nginx
CMD nginx -g "daemon off;"
Loading