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(devx): mount permission issues on Linux #1517

Merged
merged 1 commit into from
Feb 23, 2024

Conversation

hiddeco
Copy link
Contributor

@hiddeco hiddeco commented Feb 22, 2024

Fixes #1506

This addresses an issue observed on Linux systems where files written to mount paths would end up being owned by root, or files could not be read because they were not owned by the container's user.

It is achieved by creating a user within the development image with the same UID and GID as the caller on the host, while ensuring mount target paths do exist for this user and group before the container starts.

This effectively makes the hack-build-cli target work while also delivering smoother operations with hack-codegen.

In the future, we should likely rethink the way we work with tooling dependencies and their versions. As this problem was harder to tackle due to the codegen target performing system-global installations.

@hiddeco hiddeco added this to the v0.5.0 milestone Feb 22, 2024
@hiddeco hiddeco requested a review from a team as a code owner February 22, 2024 13:24
Copy link

netlify bot commented Feb 22, 2024

Deploy Preview for docs-kargo-akuity-io ready!

Name Link
🔨 Latest commit 2bb3b38
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-akuity-io/deploys/65d7c96446c4710008729d67
😎 Deploy Preview https://deploy-preview-1517.kargo.akuity.io
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Comment on lines +31 to +34
RUN addgroup --gid ${GROUP_ID} user \
&& adduser --disabled-password --gecos '' --uid ${USER_ID} --gid ${GROUP_ID} user \
&& mkdir -p /workspaces/kargo/ui/node_modules \
&& chown -R ${USER_ID}:${GROUP_ID} /workspaces/kargo
Copy link
Member

@krancour krancour Feb 22, 2024

Choose a reason for hiding this comment

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

This fails on Mac.

------                                                                                                                                      
 > [3/4] RUN addgroup --gid 20 user     && adduser --disabled-password --gecos '' --uid 501 --gid 20 user     && mkdir -p /workspaces/kargo/ui/node_modules     && chown -R 501:20 /workspaces/kargo:
0.105 addgroup: The GID `20' is already in use.
------
Dockerfile.dev:31
--------------------
  30 |     # create a non-root user with the UID and GID from the caller on the host
  31 | >>> RUN addgroup --gid ${GROUP_ID} user \
  32 | >>>     && adduser --disabled-password --gecos '' --uid ${USER_ID} --gid ${GROUP_ID} user \
  33 | >>>     && mkdir -p /workspaces/kargo/ui/node_modules \
  34 | >>>     && chown -R ${USER_ID}:${GROUP_ID} /workspaces/kargo
  35 |     
--------------------

Before zeroing in on the ID on the host being already in use in the container, however, there's a question of whether this could ever be expected to work on Mac or WSL anyway...

In either case, unlike the case with Linux, the Docker daemon runs inside a VM. That complicates things because one's uid and gid on the host machine aren't necessarily the same as the uid and gid of the user on the VM that executes the container.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The latter part (around macOS and WSL2) typically should not be an issue, as the mount there tends to be a network drive which does not suffer from the same problems.

Given this, one option would likely be to only provide the build arguments when Linux arch is detected. While defaulting to some 1xxx UID/GUID when none are provided.

Copy link
Member

Choose a reason for hiding this comment

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

sgtm

This addresses an issue observed on Linux systems where files written
to mount paths would end up being owned by root, or files could not
be read because they were _not_ owned by the container's user.

It is achieved by creating a user within the development image with
the same UID and GID as the caller on the host, while ensuring mount
target paths do exist for this user and group before the container
starts.

This effectively makes the `hack-build-cli` target work while also
delivering smoother operations with `hack-codegen`.

In the future, we should likely rethink the way we work with tooling
dependencies and their versions. As this problem was harder to tackle
due to the `codegen` target performing system-global installations.

Signed-off-by: Hidde Beydals <hidde@hhh.computer>
-v $(dir $(realpath $(firstword $(MAKEFILE_LIST)))):/workspaces/kargo \
-v /workspaces/kargo/ui/node_modules \
-w /workspaces/kargo \
kargo:dev-tools

DEV_TOOLS_BUILD_OPTS =
ifeq ($(GOOS),linux)
Copy link
Member

Choose a reason for hiding this comment

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

GOOS is often undefined. Would it make sense to use uname here instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$(GOOS) != ${GOOS}, and actually is based on uname already. See:

kargo/Makefile

Line 43 in 2bb3b38

GOOS ?= $(shell uname -s | tr '[:upper:]' '[:lower:]')
😄

Copy link
Member

Choose a reason for hiding this comment

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

Apologies... I overlooked that.

Copy link
Member

@krancour krancour left a comment

Choose a reason for hiding this comment

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

🔥

@hiddeco hiddeco added this pull request to the merge queue Feb 23, 2024
Merged via the queue into akuity:main with commit 5c66b9a Feb 23, 2024
14 checks passed
@hiddeco hiddeco deleted the fix-mount-perms-hack branch February 23, 2024 16:51
@webstradev webstradev mentioned this pull request Apr 7, 2024
4 tasks
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.

In-container CLI build fails to obtain VCS metadata (on a Linux host)
2 participants