-
-
Notifications
You must be signed in to change notification settings - Fork 863
Add non-root user by default #850
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.
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @ToshY!
Perhaps, instead of changing the Dockerfile, it would make sense to describe the changes to this PR in the documentation page?
I assume FrankenPHP relies on Caddy's decision to run as root by default. See the discussion on this topic in the Caddy repository: caddyserver/caddy-docker#104. |
Okay, would you recommend adding a (new) documentation page, or update an existing one? Edit I've moved it to a new documentation page. I understand the decision to not directly apply the changes to the Dockerfile and document it instead. Maybe something to take into consideration for next time (and start with non-root by default then). |
90fb72e
to
368a331
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This template's documentation files use simple diffs, not patches.
How about adding a couple of examples of using the new arguments?
134351a
to
8abc11a
Compare
+ | ||
WORKDIR /app | ||
|
||
-VOLUME /app/var/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why shouldn't the /app/var
directory be a volume?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While there are multiple considerations on why not to use VOLUME
in the Dockerfile 1, regarding the scope of the this feature, if docker would not be run as "rootless" (not to be confused with "running docker as non-root user"), if a volume mounted ./:/app
(on development) but the ./var
directory does not exist on host, this will create the ./var
directory as root
on host (which prevents the application from starting as the container user can not write to ./var
).
Think this would either require the ./var
to already be present on host (untested), or remove the VOLUME
directive as done now (or the user should use docker "rootless" but this comes with possible side-effects). Maybe using the VOLUME
would only be really useful for prod
stage (for docker/compose in production; not kubernetes).
Footnotes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting note. I can't find any simple ways to make this work while keeping this volume.
1ff54bf
to
efc6a32
Compare
@7-zete-7 There's one comment still open, and I fixed markdown linter errors in CI, but markdown prettier fails without denoting on why. What/where are the dev tools/linters/formatters to fix the formatting in such cases? |
According to https://github.com/dunglas/symfony-docker/actions/runs/18161075332/job/51691731657?pr=850:
In diffs, it's important to use the formatting rules for the files for which edits are being proposed. For example, a Dockerfile should have tab indents. For its diff, the first column should be indented according to the diff rules, followed by tab indents. This will violate the MARKDOWNLINT symfony-docker/docs/makefile.md Lines 22 to 30 in 84aac83
|
Fixed markdown issues. The markdown prettier does not show what is wrong (even in debug mode), so I ended up running the super-linter with docker locally, as it can also fix these issues directly. docker run --rm -v $(pwd):/tmp/lint -e RUN_LOCAL=true -e DEFAULT_BRANCH=main -e LOG_LEVEL=DEBUG -e FIX_MARKDOWN=true -e FIX_MARKDOWN_PRETTIER=true ghcr.io/super-linter/super-linter:slim-v8 The still failing biome checks are unrelated to the PR. p.s. Please consider adding this to (not yet existing) contributing guidelines so next time contributors can check how to easily run super-linter locally and fix these problems faster. I'm open to create another PR for this if desired. |
Fixes #679
Based on:
Using a non-root user is regarded as a best practice.
I am not sure what the "many issues" a non-root user could cause (which might need further explanation), but I was able to build the images (dev/prod targets) and run it on dev without any apparent issues. Requires further testing.