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

docker: Updated user and data dir handling. #5276

Merged
merged 2 commits into from
Apr 19, 2023

Conversation

winder
Copy link
Contributor

@winder winder commented Apr 7, 2023

Summary

Set UID and GID to 999 consistently.
Start as root user for init then drop to algorand user.

Test Plan

Build the container locally

docker build \
    -t algod_test \
    --build-arg CHANNEL=stable \
    --build-arg TARGETARCH=amd64 --no-cache \
    .

Without these changes, mounting a directory fails with a permission denied error.

docker run --rm -it \
    -p 4190:4190 \
    --name algod-test-run \
    -e TOKEN=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa \
    -v (pwd)/docker_data_dir:/algod/data \
    algod_test

Also had some help from Patrick testing in kubernetes.

Set UID and GID to 999 consistently.
Start as root user for init then drop to algorand user.
@winder winder self-assigned this Apr 7, 2023
@winder winder marked this pull request as ready for review April 7, 2023 21:26
onetechnical
onetechnical previously approved these changes Apr 11, 2023
Copy link
Contributor

@onetechnical onetechnical left a comment

Choose a reason for hiding this comment

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

I get it and it seems alright, but I haven't tested it.

@excalq
Copy link
Contributor

excalq commented Apr 11, 2023

As mentioned in the Slack thread, I think a better approach than doing chown of host-mounted $ALGORAND_DATA (which requires root), would be:

  1. Using a native Docker named volume in trivial cases, where the user doesn't wish to customize their setup.
  2. Where a large dedicated storage location is needed (e.g. EBS volume or NVME disk), the user should be able to bind mount a host volume, but they must ensure it's writable by uid 999 (the algod user in the container).

As https://hub.docker.com/_/postgres/ notes, supporting arbitrary UIDs requires setting the /etc/password with that uid, which gets complicated. (Or worse, mounting the host's /etc/password.)

@winder
Copy link
Contributor Author

winder commented Apr 11, 2023

As mentioned in the Slack thread, I think a better approach than doing chown of host-mounted $ALGORAND_DATA
...

What makes this a better approach? I was following the pattern used by redis and postgres. Those are both "curated containers", so it seemed like a safe pattern to follow. Also, it worked on my machine -- setting a UID/GID and mounting the passwd file did not.

https://github.com/docker-library/redis/blob/fb3e83ea2c60ba55da46a5eaf114c4d77cd0e97d/7.2-rc/docker-entrypoint.sh#L11
https://github.com/docker-library/postgres/blob/25b3034e9b0155c3e71acaf650243e7d12a571c1/15/bullseye/docker-entrypoint.sh#L57

I think there are other permission errors related to a custom UID/GID, at this time I have not looked into those problems.

tzaffi
tzaffi previously approved these changes Apr 11, 2023
Copy link
Contributor

@tzaffi tzaffi left a comment

Choose a reason for hiding this comment

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

image

@excalq
Copy link
Contributor

excalq commented Apr 11, 2023

As mentioned in the Slack thread, I think a better approach than doing chown of host-mounted $ALGORAND_DATA
...

What makes this a better approach? I was following the pattern used by redis and postgres. Those are both "curated containers", so it seemed like a safe pattern to follow. Also, it worked on my machine -- setting a UID/GID and mounting the passwd file did not.

https://github.com/docker-library/redis/blob/fb3e83ea2c60ba55da46a5eaf114c4d77cd0e97d/7.2-rc/docker-entrypoint.sh#L11 https://github.com/docker-library/postgres/blob/25b3034e9b0155c3e71acaf650243e7d12a571c1/15/bullseye/docker-entrypoint.sh#L57

I think there are other permission errors related to a custom UID/GID, at this time I have not looked into those problems.

Since we're not requiring the container to be run as root (the chown will be skipped), it's fine. But we should test a scenario where the container is run as non-root. This is the scenario that would require external volumes having correct uid:999 permissions.

@winder winder dismissed stale reviews from tzaffi and onetechnical via 01a0244 April 12, 2023 15:16
@winder
Copy link
Contributor Author

winder commented Apr 12, 2023

As mentioned in the Slack thread, I think a better approach than doing chown of host-mounted $ALGORAND_DATA
...

What makes this a better approach? I was following the pattern used by redis and postgres. Those are both "curated containers", so it seemed like a safe pattern to follow. Also, it worked on my machine -- setting a UID/GID and mounting the passwd file did not.
https://github.com/docker-library/redis/blob/fb3e83ea2c60ba55da46a5eaf114c4d77cd0e97d/7.2-rc/docker-entrypoint.sh#L11 https://github.com/docker-library/postgres/blob/25b3034e9b0155c3e71acaf650243e7d12a571c1/15/bullseye/docker-entrypoint.sh#L57
I think there are other permission errors related to a custom UID/GID, at this time I have not looked into those problems.

Since we're not requiring the container to be run as root (the chown will be skipped), it's fine. But we should test a scenario where the container is run as non-root. This is the scenario that would require external volumes having correct uid:999 permissions.

This is really fascinating. Thanks for the information. I attempted to explain/clarify this process as an option in the README.

@winder winder requested review from tzaffi and onetechnical April 12, 2023 15:17
@@ -80,7 +80,7 @@ The data directory located at `/algod/data`. Mounting a volume at that location

### Volume Permissions

The container executes in the context of the `algorand` user with UID=999 and GID=999 which is handled differently depending on your operating system. During startup the container is temporarily run as `root`, after modifying the permissions of `/algod/data` it drops to the `algorand` user. This can sometimes cause problems.
The container executes in the context of the `algorand` user with UID=999 and GID=999 which is handled differently depending on your operating system or deployment platform. During startup the container temporarily runs as `root` in order to modify the permissions of `/algod/data`. It then changes to the `algorand` user. This can sometimes cause problems, for example if your deployment platform doesn't allow containers to run as the root user.
Copy link
Contributor

Choose a reason for hiding this comment

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

As the run.sh only does the chown if running as root, it seems safe to run the container in a non-root context (provided externally mounted /algorand/data has the proper write permissions already). I think this section of the README could state that as such. In any case, this reads as discouraging running as non-root, which I don't think is warranted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Help! Could you suggest an alternative phrasing? I must have reworded this 5 times before getting to this point.

From my point of view, this is how I would recommend people run their container. It seems less likely to get us bug reports compared to telling people how to configure the data directory and UID. I tried to make it clear that the root user is being used so that anyone sensitive to this would not be surprised (and may even continue reading to see how to override the UID/GID.

@winder winder requested a review from excalq April 14, 2023 17:46
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.

4 participants