Conversation
66013e9 to
e0c0a4a
Compare
|
@polarathene can you please sign the CLA? |
|
Sorry for the delay in engagement, I've been caught up elsewhere 😅 Thanks for tackling all of this 💪 I'll do a quick review in a moment. |
polarathene
left a comment
There was a problem hiding this comment.
Bit more thorough than anticipated 😅
Hope the feedback helps.
| ######################### | ||
|
|
||
| FROM gcr.io/distroless/static-debian12:nonroot AS runner | ||
| FROM gcr.io/distroless/static-debian12:debug-nonroot AS runner |
There was a problem hiding this comment.
While busybox for a shell is convenient, it kinda defeats the purpose of distroless.
There was a problem hiding this comment.
True - but this really is only for local dev and I do know a few people including myself who sometimes need to exec locally i to the pod to do and test something. It should also point as a warning that this isn’t a prod image
.docker/Dockerfile-alpine
Outdated
| # Add a user/group for Ory with a stable UID + GID: | ||
| # NOTE: This only appears relevant for supporting hydra as non-root, otherwise unnecessary. | ||
| addgroup --system --gid 500 ory | ||
| adduser --system --uid 500 \ | ||
| --gecos "Ory User" \ | ||
| --home /home/ory \ | ||
| --ingroup ory \ | ||
| --shell /sbin/nologin \ | ||
| ory |
There was a problem hiding this comment.
Just for reference, the changes here don't completely reflect those from #3914
Notably with the sqlite path being dropped. Which as was detailed in the referenced PR would lose the ownership for the ory user if a bind mount volume was attached to the same location, requiring the user to manually adjust.
Might want to make note to document these caveats (or drop the non-root user to simplify and avoid the issue entirely).
There was a problem hiding this comment.
From the referenced PR this was the sqlite related snippet (useful for testing with sqlite even without a volume as DB state will still persist until container is destroyed, restarts are still valid for persistence):
# Create the sqlite directory with ownership to that user and group:
# NOTE: This is required for read/write by SQLite.
# - Path may be a default value somewhere, or only explicitly provided via DSN?
# - Owner/Group is only relevant to permissions allowing the hydra process to read/write to the location.
# - Bind mount volumes will replace the ownership with that of the host directory, requiring correction.
install --owner ory --group ory --directory /var/lib/sqliteThere was a problem hiding this comment.
It works fine in my testing - other variants did not, which is why i‘ve set it up like this. But yeah, could be I missed something
Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
|
For context to anyone landing here:
|
Improves the docker set up and removes some unused files.
Closes #3914
Closes #3916
Closes #3685
Closes #3683
Related issue(s)
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
security@ory.sh) from the maintainers to push
the changes.
works.
Further Comments