Skip to content

Conversation

@mselim00
Copy link

@mselim00 mselim00 commented May 4, 2025

Issue #, if available:

Description of changes:

Currently attempts to run this image with nginx in the foreground, as most other nginx images are ran, fails:

$ docker run public.ecr.aws/eks-distro-build-tooling/eks-distro-minimal-base-nginx:latest nginx -g 'daemon off;'
nginx: [alert] could not open error log file: open() "/var/log/nginx/error.log" failed (13: Permission denied)
2025/05/05 19:58:41 [warn] 1#1: the "user" directive makes sense only if the master process runs with super-user privileges, ignored in /etc/nginx/nginx.conf:5
2025/05/05 19:58:41 [emerg] 1#1: open() "/var/log/nginx/error.log" failed (13: Permission denied)

This is because the default configurations for nginx assume that the master process is ran by the super-user/root and tries to access conventionally privileged files and directories. This means that, without a new image built with changes like the one in this PR, the current image essentially has to be run as root

docker run -p 8080:80 --user=root public.ecr.aws/eks-distro-build-tooling/eks-distro-minimal-base-nginx:latest nginx -g 'daemon off;'

This PR also bumps up the error verbosity to notice to match other nginx images and links the access/error logs to stdout/stderr so they can be seen from container run and logs commands. Ref the alpine slim example for comparison: https://github.com/nginx/docker-nginx/blob/master/Dockerfile-alpine-slim.template#L86

After this change, users can still run the image as root and get the same behavior (other than the log links to standard out/error, but I don't think there should be a dependency on that not being the case since it is after all an nginx image), but they can also run it as the default user and get a functional nginx image.

docker run <minimal-nginx-image> should exhibit similar behavior to docker run nginx. Currently it immediately exits because there's no default command set. This change sets the same command as the typical nginx container image, and makes additional tweaks so that the server can be ran by/as the current user (nginx).

Shell 1:

$ docker run --rm -p 8080:80 <new-nginx-image>
2025/05/04 03:04:40 [notice] 1#1: using the "epoll" event method
2025/05/04 03:04:40 [notice] 1#1: nginx/1.26.3
2025/05/04 03:04:40 [notice] 1#1: built by gcc 7.3.1 20180712 (Red Hat 7.3.1-17) (GCC) 
2025/05/04 03:04:40 [notice] 1#1: OS: Linux 5.10.235-208.919.amzn2int.x86_64
2025/05/04 03:04:40 [notice] 1#1: getrlimit(RLIMIT_NOFILE): 32768:65536
2025/05/04 03:04:40 [notice] 1#1: start worker processes
2025/05/04 03:04:40 [notice] 1#1: start worker process 7
2025/05/04 03:04:40 [notice] 1#1: start worker process 8
2025/05/04 03:04:40 [notice] 1#1: start worker process 9
2025/05/04 03:04:40 [notice] 1#1: start worker process 10
2025/05/04 03:04:40 [notice] 1#1: start worker process 11
2025/05/04 03:04:40 [notice] 1#1: start worker process 12
2025/05/04 03:04:40 [notice] 1#1: start worker process 13
2025/05/04 03:04:40 [notice] 1#1: start worker process 14

Shell 2:

$ curl http://localhost:8080
<!DOCTYPE html>
<html>
<head>
<title>Welcome to nginx!</title>
<style>
html { color-scheme: light dark; }
body { width: 35em; margin: 0 auto;
font-family: Tahoma, Verdana, Arial, sans-serif; }
</style>
</head>
<body>
<h1>Welcome to nginx!</h1>
<p>If you see this page, the nginx web server is successfully installed and
working. Further configuration is required.</p>

<p>For online documentation and support please refer to
<a href="http://nginx.org/">nginx.org</a>.<br/>
Commercial support is available at
<a href="http://nginx.com/">nginx.com</a>.</p>

<p><em>Thank you for using nginx.</em></p>
</body>
</html>

Shell 1:

...
240.10.0.1 - - [04/May/2025:03:05:23 +0000] "GET / HTTP/1.1" 200 615 "-" "curl/8.3.0" "-"

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@eks-distro-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign rcrozean for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@github-actions github-actions bot added the minimal-image Changes related to the minimal-images or tooling label May 4, 2025
@eks-distro-bot
Copy link
Collaborator

Hi @mselim00. Thanks for your PR.

I'm waiting for a aws member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@mselim00 mselim00 force-pushed the run-nginx branch 2 times, most recently from 6a5f391 to b6e539f Compare May 4, 2025 04:50
@mselim00 mselim00 force-pushed the run-nginx branch 2 times, most recently from f602c10 to aa88f77 Compare May 4, 2025 05:36
@mselim00 mselim00 marked this pull request as ready for review May 4, 2025 05:46
# TODO: remove these when changes can be coordinated in eks-a-build-tooling
install_rpm bash \
coreutils && \
coreutils && \
Copy link
Member

Choose a reason for hiding this comment

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

Seems indent missing

export OUTPUT_DEBUG_LOG=${OUTPUT_DEBUG_LOG} && \
enable_extra nginx1 && \
install_rpm nginx-filesystem \
nginx \
Copy link
Member

Choose a reason for hiding this comment

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

Indent missing here

COPY --from=base-nginx-builder /newroot /

# Let nginx read/write its pid
RUN chmod o+rw /run
Copy link
Member

Choose a reason for hiding this comment

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

read-write permissions to (o+rw) on /run can be risky from a security perspective. If possible, can you restrict permissions to the nginx user or group?

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense - I added an nginx config file which allows us to further scope this down by writing the pid to /run/nginx/nginx.pid rather than the default /run/nginx.pid, and gave nginx ownership of /run/nginx

@Ganiredi
Copy link
Member

Ganiredi commented May 5, 2025

/ok-to-test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

minimal-image Changes related to the minimal-images or tooling ok-to-test size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants