Ensure `/var/log/nginx` is writeable by GID 0 #4269

sigv · 2023-08-18T20:22:53Z

Proposed changes

In a standard deployment, error log is written to /dev/stderr and access log is written to /dev/stdout. Furthermore, error.log and access.log in /var/log/nginx are mapped to the respective stdio. However, a deployment may override configuration, and remove the symbolic links, to write to the container storage directly.

OpenShift tries to impose various restrictions by default. One of these is for UID/GID used by the container process. If these restrictions are supported in future, adjustments to file system permissions need to be done so that /var/log/nginx remains writeable. Specifically, OpenShift adds GID 0 as supplemental to container process for file system operations.

This PR ensures the nginx user (UID 101) and root group (GID 0) owns the log directory, and that owner group permissions match the owner user permissions (g=u). This ensures that OpenShift deployments retain write permissions in future.

This permissions change was already being applied to App Protect builds, where I presume writing to disk is enabled by default. Now with the PR, the permissions are ensured for all customers (not only App Protect variant).

Closes #4279.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works
I have checked that all unit tests pass after adding my changes
I have updated necessary documentation
I have rebased my branch onto main
I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

vepatel · 2023-08-22T09:35:42Z

Hi @sigv can you please open an issue for this PR: https://github.com/nginxinc/kubernetes-ingress/blob/main/CONTRIBUTING.md#open-a-pull-request thanks!

sigv · 2023-08-22T09:58:04Z

@vepatel, opened and referenced #4279.

codecov · 2023-09-01T11:59:45Z

Codecov Report

Merging #4269 (4c5f307) into main (9a52e02) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #4269      +/-   ##
==========================================
- Coverage   52.00%   51.99%   -0.02%     
==========================================
  Files          59       59              
  Lines       16960    16960              
==========================================
- Hits         8820     8818       -2     
- Misses       7845     7847       +2     
  Partials      295      295

see 1 file with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

lucacome

Sorry for the long wait again @sigv

I guess it doesn't cost us anything to set the permissions for /var/log/nginx and be on the safe side.

sigv · 2023-11-03T09:24:42Z

Sorry for the long wait again

No worries! I fully understand things sometimes take time 🙂

0c8462d shuffled lines around and there's now a merge conflict to resolve.
I will squeeze this in around end of day (Europe), so the blocker doesn't stick around over the weekend. 🤞🏻

sigv · 2023-11-04T00:49:48Z

Tests are failing as they want to re-build the Nginx Plus components for which my repo doesn't have the certificate. 😞

In a standard deployment, error log is written to `/dev/stderr` and access log is written to `/dev/stdout`. Furthermore, `error.log` and `access.log` in `/var/log/nginx` are mapped to the respective stdio. However, a deployment may override configuration, and remove the symbolic links, to write to the container storage directly. OpenShift tries to impose various restrictions by default. One of these is for UID/GID used by the container process. If these restrictions are supported in future, adjustments to file system permissions need to be done so that /var/log/nginx remains writeable. Specifically, OpenShift adds GID 0 as supplemental to container process for file system operations. This PR ensures the nginx user (UID `101`) and root group (GID `0`) owns the log directory, and that owner group permissions match the owner user permissions (`g=u`). This ensures that OpenShift deployments retain write permissions in future.

shaun-nx

Thanks again for being patient with us @sigv !

sigv · 2023-11-08T13:37:17Z

I'm not sure how the Alpine error could be triggered by this change (Error getting Endpoints for Upstream backend1: error retrieving endpoints for the service backend1-svc: no endpointslices for target port 8080 in service backend1-svc)

shaun-nx · 2023-11-08T14:09:15Z

@sigv That particular log message happens when we're enumerating pods before they are ready. From the logs its looks like the pods for that test took too long to become ready.

shaun-nx · 2023-11-08T14:11:20Z

@sigv I've re-run the failed test and everything is green now. Just waiting on images to be built and we should be good 😄

shaun-nx · 2023-11-08T14:27:04Z

🚀

sigv requested a review from a team as a code owner August 18, 2023 20:22

sigv changed the title ~~Ensure /var/log/nginx is writeable by 101:0~~ Ensure /var/log/nginx is writeable by GID 0 Aug 18, 2023

sigv force-pushed the log-rw-101-0 branch from 0f5d345 to 7060911 Compare August 18, 2023 20:23

sigv mentioned this pull request Aug 18, 2023

Apply UID/GID defaults from image #3665

Closed

6 tasks

sigv force-pushed the log-rw-101-0 branch 2 times, most recently from 44c3692 to b5784b6 Compare August 22, 2023 08:29

sigv force-pushed the log-rw-101-0 branch 4 times, most recently from a304611 to 7ab723f Compare August 30, 2023 07:20

sigv force-pushed the log-rw-101-0 branch 4 times, most recently from d640e6c to 6b93eec Compare September 7, 2023 17:39

sigv force-pushed the log-rw-101-0 branch 9 times, most recently from 083732d to 13219c8 Compare September 19, 2023 19:12

sigv force-pushed the log-rw-101-0 branch 4 times, most recently from 9ef13ec to e98cc72 Compare September 20, 2023 19:38

sigv force-pushed the log-rw-101-0 branch 7 times, most recently from 30030e0 to 78bce8d Compare October 19, 2023 15:44

sigv force-pushed the log-rw-101-0 branch 4 times, most recently from 21edb11 to 0d31e25 Compare October 28, 2023 22:13

sigv force-pushed the log-rw-101-0 branch 3 times, most recently from 10834df to fe561c6 Compare November 2, 2023 19:33

lucacome approved these changes Nov 3, 2023

View reviewed changes

lucacome requested a review from a team November 3, 2023 01:53

sigv force-pushed the log-rw-101-0 branch from fe561c6 to bd61cdc Compare November 3, 2023 18:27

sigv force-pushed the log-rw-101-0 branch 2 times, most recently from a1d5c86 to dfd1f02 Compare November 7, 2023 10:22

sigv force-pushed the log-rw-101-0 branch from dfd1f02 to 4c5f307 Compare November 8, 2023 08:15

shaun-nx approved these changes Nov 8, 2023

View reviewed changes

shaun-nx merged commit 0e1e647 into nginx:main Nov 8, 2023
63 checks passed

sigv deleted the log-rw-101-0 branch November 8, 2023 15:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure `/var/log/nginx` is writeable by GID 0 #4269

Ensure `/var/log/nginx` is writeable by GID 0 #4269

sigv commented Aug 18, 2023 •

edited

Loading

vepatel commented Aug 22, 2023

sigv commented Aug 22, 2023

codecov bot commented Sep 1, 2023 •

edited

Loading

lucacome left a comment

sigv commented Nov 3, 2023 •

edited

Loading

sigv commented Nov 4, 2023

shaun-nx left a comment

sigv commented Nov 8, 2023

shaun-nx commented Nov 8, 2023

shaun-nx commented Nov 8, 2023 •

edited

Loading

shaun-nx commented Nov 8, 2023

Ensure /var/log/nginx is writeable by GID 0 #4269

Ensure /var/log/nginx is writeable by GID 0 #4269

Conversation

sigv commented Aug 18, 2023 • edited Loading

Proposed changes

Checklist

vepatel commented Aug 22, 2023

sigv commented Aug 22, 2023

codecov bot commented Sep 1, 2023 • edited Loading

Codecov Report

lucacome left a comment

Choose a reason for hiding this comment

sigv commented Nov 3, 2023 • edited Loading

sigv commented Nov 4, 2023

shaun-nx left a comment

Choose a reason for hiding this comment

sigv commented Nov 8, 2023

shaun-nx commented Nov 8, 2023

shaun-nx commented Nov 8, 2023 • edited Loading

shaun-nx commented Nov 8, 2023

Ensure `/var/log/nginx` is writeable by GID 0 #4269

Ensure `/var/log/nginx` is writeable by GID 0 #4269

sigv commented Aug 18, 2023 •

edited

Loading

codecov bot commented Sep 1, 2023 •

edited

Loading

sigv commented Nov 3, 2023 •

edited

Loading

shaun-nx commented Nov 8, 2023 •

edited

Loading