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

Allow to set the PUID and PGID using docker #12596

Merged
merged 3 commits into from
Sep 28, 2024

Conversation

Macavity
Copy link
Contributor

Feature

This solves the request #9707

Description of the Issue:

The current Dockerfile hardcodes the creation of a user and group with specific user IDs (UID) and group IDs (GID). This approach lead to two major problems:

  1. Unknown groups in the container: When mounting directories from the host, if the GID of the mounted directory belongs to a group unknown to the container, permission issues arise, preventing the container from accessing the directory.
  2. The creation of the "/.config/" directory required root permissions, which the siyuan user does not have.

Changes Made:

  • Provide ARGs for docker-compose to allow overwriting the default 1000:1000 values
  • Dynamically assigns the siyuan user in case the provided GID exists already on the container
  • Ensured that mounted directories, such as the workspace, are accessible by the container user by aligning the user's group membership with host permissions.
  • Adjusted ownership of the relevant directories to ensure correct group-based permissions.

@88250 88250 changed the title Feature: Allow to set the PUID and PGID using docker compose Allow to set the PUID and PGID using docker compose Sep 25, 2024
@88250 88250 added this to the 3.1.8 milestone Sep 25, 2024
Copy link
Member

@88250 88250 left a comment

Choose a reason for hiding this comment

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

Another question, does it work just with docker (not compose)?

Dockerfile Outdated
ENV RUN_IN_CONTAINER=true
EXPOSE 6806
VOLUME /home/siyuan/workspace
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this will affect users who have specified the --workspace parameter, such as the usage described in the README:

docker run -d -v workspace_dir_host:workspace_dir_container -p 6806:6806 b3log/siyuan --workspace=workspace_dir_container --accessAuthCode=xxx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this will affect users who have specified the --workspace parameter, such as the usage described in the README:

docker run -d -v workspace_dir_host:workspace_dir_container -p 6806:6806 b3log/siyuan --workspace=workspace_dir_container --accessAuthCode=xxx

That's a very valid point. I think I can safely remove this line though, I will test if it impacts the result.

Another question, does it work just with docker (not compose)?

Yes, customizing USER_ID and GROUP_ID is possible with just Docker, not only via Docker Compose. You can pass --build-arg during the docker build process to specify these values.

docker build --build-arg USER_ID=1005 --build-arg GROUP_ID=100 -t siyuan_note_image .
docker run -d --name siyuan_note siyuan_note_image

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that the ownership of the workspace should also change if the workspace is specified.

@88250
Copy link
Member

88250 commented Sep 25, 2024

In addition, please modify the corresponding document example in README:

Alternatively, see below for an example Docker Compose file:

....

The above content should be adjusted so that users know how to use it. Thank you.

@Macavity
Copy link
Contributor Author

Macavity commented Sep 26, 2024

While working on the Readme I realized the approach has an issue. It works only for developers who have a copy of the project and will build the image themselves, it does not work for someone using the pre-built image 🤔

I will have to adjust my approach.

@88250 88250 removed this from the 3.1.8 milestone Sep 26, 2024
@Macavity Macavity force-pushed the feat/fix-docker-permissions branch from 3f4970f to 53c27f6 Compare September 27, 2024 13:46
@Macavity
Copy link
Contributor Author

@88250 I've extracted the logic for user and group creation from the Dockerfile to an entrypoint.sh which is called then the container is created initially.

I've updated the english readme and tried to do the same for the chinese readme using DeepL. Can you check if the chinese one makes any sense?

@88250
Copy link
Member

88250 commented Sep 28, 2024

Great! I'll adjust the details of the Chinese README later.

Thank you very much for your contribution!

@88250 88250 added this to the 3.1.8 milestone Sep 28, 2024
@88250 88250 changed the title Allow to set the PUID and PGID using docker compose Allow to set the PUID and PGID using docker Sep 28, 2024
@88250 88250 merged commit f0e0b98 into siyuan-note:dev Sep 28, 2024
1 check passed
This was referenced Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants