Skip to content
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

fix: improve docker set up #3921

Merged
merged 26 commits into from
Jan 11, 2025
Merged

fix: improve docker set up #3921

merged 26 commits into from
Jan 11, 2025

Conversation

aeneasr
Copy link
Member

@aeneasr aeneasr commented Jan 10, 2025

Improves the docker set up and removes some unused files.

Closes #3914
Closes #3916
Closes #3685
Closes #3683

Related issue(s)

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got the approval (please contact
    [email protected]) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

@aeneasr aeneasr requested a review from a team as a code owner January 10, 2025 15:05
@CLAassistant
Copy link

CLAassistant commented Jan 10, 2025

CLA assistant check
All committers have signed the CLA.

@aeneasr aeneasr force-pushed the aeneasr/fix-docker branch from 66013e9 to e0c0a4a Compare January 10, 2025 16:09
@aeneasr
Copy link
Member Author

aeneasr commented Jan 10, 2025

@polarathene can you please sign the CLA?

@polarathene
Copy link
Contributor

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.

Copy link
Contributor

@polarathene polarathene left a comment

Choose a reason for hiding this comment

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

Bit more thorough than anticipated 😅

Hope the feedback helps.

.docker/README.md Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@@ -18,15 +17,14 @@ RUN go build -ldflags="-extldflags=-static" -tags sqlite,sqlite_omit_load_extens

#########################

FROM gcr.io/distroless/static-debian12:nonroot AS runner
FROM gcr.io/distroless/static-debian12:debug-nonroot AS runner
Copy link
Contributor

Choose a reason for hiding this comment

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

While busybox for a shell is convenient, it kinda defeats the purpose of distroless.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Comment on lines 7 to 15
# 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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/sqlite

Copy link
Member Author

Choose a reason for hiding this comment

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

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

.docker/Dockerfile-alpine Outdated Show resolved Hide resolved
.docker/Dockerfile-local-build Outdated Show resolved Hide resolved
quickstart.yml Show resolved Hide resolved
@polarathene
Copy link
Contributor

Closes #3914
Closes #3916
Closes #3916

Also looks like you meant to reference another issue to close, not the same one twice

Co-authored-by: Brennan Kinney <[email protected]>
@aeneasr aeneasr merged commit 6c359c3 into master Jan 11, 2025
29 checks passed
@aeneasr aeneasr deleted the aeneasr/fix-docker branch January 11, 2025 12:30
@polarathene
Copy link
Contributor

For context to anyone landing here:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants