precreate nginx log files, fix permissions #97
Conversation
|
I am a bot, here are the test results for this PR:
|
|
I think we want this in the nginx base, not here. |
|
Sure, the precreation can move to the base, but we would still need the chown as the base no longer does that |
if we're precreating using |
|
We need it for other files in general. The base used to do a recursive chown so the nginx image never had to. But the recursive chown was removed from the base a few months ago so now we need it here. The precreation requirement is separate because without it the log files are created after init completes and chown runs so they remain root owned until the next container start |
|
In other words, this PR attempts to fix two separate issues, but one of those fixes (precreation) can be moved to the base. The other will stay here. |
| lsiown -R abc:abc \ | ||
| /config |
There was a problem hiding this comment.
I am still hesitant on the recursive chown on /config being here. I know we used to do it in the nginx base, but what would be the reason we need it here (outside of restoring the previous behavior)? I could maybe see doing a recursive chown on /config/www but that still makes me think permissions should maybe be up to the user. The other folders created in /config include keys, log (which sparked this), nginx, and php all of which maybe would be better to be handled by doing chowns in the nginx base. The www folder is either user-land or populated by the downstream images, and I can agree if there does need to be a recursive chown it can be added to downstream images, but I'm not positive if it's 100% the right option in this image (or swag) as they are the two most likely to have a user apply different permissions with some intention.
There was a problem hiding this comment.
I reviewed the nginx base, and here is what we're doing
https://github.com/linuxserver/docker-baseimage-alpine-nginx/blob/a05dfb3ef3424235465cc5d30e91d471a0c870af/root/etc/s6-overlay/s6-rc.d/init-permissions/run#L9-L16
Could we not just move the logs chown up to the recursive set and add the pre-creation of the files to the nginx base?
There was a problem hiding this comment.
It's up to you. You're the maintainer. I'm just letting you know that there are currently two issues, one affects all nginx based images where the logs in a fresh container are root owned and logrotate is busted as a result. The other affects the nginx image where if the user changes the PUID/PGID after creation, a bunch of files have incorrect ownership.
There was a problem hiding this comment.
It's up to you. You're the maintainer. I'm just letting you know that there are currently two issues, one affects all nginx based images where the logs in a fresh container are root owned and logrotate is busted as a result. The other affects the nginx image where if the user changes the PUID/PGID after creation, a bunch of files have incorrect ownership.
Does the second issue not affect all nginx based images? I think that's the part I'm misunderstanding.
There was a problem hiding this comment.
Wait... I get it now.
The second issue technically does affect all nginx based images that don't already set permissions in /config/www but in the case of this image and swag I'm not sure if the best option is to adjust it for the user or to have the user adjust the permissions on their own. If the user changed PUID/PGID that's an intentional change on their part (even if they don't know what it can cause). For those that do know what it does and want more specific control over permissions if we add a recursive chown on /config/www it limits that ability. For all other images (where the idea is more to run a specific app) I do think the recursive chown on /config/www is probably fine.
I'll prep a PR for the nginx base to fix the logrotate issue and chat a bit in discord about the secondary issue. It'll kind of come down to support efforts I think.
There was a problem hiding this comment.
Most if not all Nginx based images do a recursive chown in their init (including SWAG). Nginx image does not because it was relying on the baseimage's recursive chown which got removed a few months back.
There was a problem hiding this comment.
I didn't go look to see it in swag, but yeah I see it now. I guess at this point if we merge linuxserver/docker-baseimage-alpine-nginx#151 we can remove the log precreate in this PR and keep the recursive chown and call it a day.
Ref: linuxserver/docker-nginx#97 Signed-off-by: Eric Nemchik <[email protected]>
|
This pull request has been automatically marked as stale because it has not had recent activity. This might be due to missing feedback from OP. It will be closed if no further activity occurs. Thank you for your contributions. |
|
looks like this was implemented in the base |
We need to precreate the nginx log files and fix their permissions on first run. Otherwise nginx creates them as root owned.
fixes linuxserver/docker-baseimage-alpine-nginx#149