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

driver: docker-container driver uses --config correctly in rootless mode #2093

Merged
merged 1 commit into from
Nov 13, 2023

Conversation

jsternberg
Copy link
Collaborator

@jsternberg jsternberg commented Oct 20, 2023

The docker-container driver relies on the default config file location
for buildkit when writing the configuration file. When run in a rootless
version of docker (dind), the default location is different.

Instead of trying to figure out where the appropriate default location
is, this just writes the files to the same location and sets the
--config parameter explicitly. This flag is placed first so a
user-specified config option in --buildkitd-flags will take precedence
over the implicit config parameter.

This also fixes the --config option with the rootless image.
Previously, the config directory was being copied in a way that rendered
/etc unreadable and the configuration file wasn't readable either. It
also wasn't copied to the correct place. Now, --config is used to
specify the directory, /etc isn't included in the copied archive (so
the permissions aren't overwritten), and the directory is set as world
readable to be readable from the rootless buildkit process`.

Fixes #2092.

@tonistiigi
Copy link
Member

tonistiigi commented Oct 20, 2023

@AkihiroSuda Looking at the issue, isn't the problem in incorrectly detecting rootless paths used by buildkit. There is no --driver-opt in that reproducer that would set a rootless buildkit image. Additionally, with this approach, could there be permission issues in accessing /etc.

@jjacobelli
Copy link

@AkihiroSuda Looking at the issue, isn't the problem in incorrectly detecting rootless paths used by buildkit. There is no --driver-opt in that reproducer that would set a rootless buildkit image. Additionally, with this approach, could there be permission issues in accessing /etc.

I would agree that maybe buildkitd should apply this logic when it's computing the default value for the --config flag instead of just checking if it's running in a user namespace: https://github.com/moby/buildkit/blob/762b2626ddafbc0874182b477f0393e8c5c3cb63/cmd/buildkitd/main.go#L414-L419

But maybe one solution could be to use a path that is writable by any user (like /tmp/buildkitd.toml?) and set the --config flag instead of relying on default values. WDYT?

@crazy-max
Copy link
Member

crazy-max commented Oct 23, 2023

Yes it seems buildkitd configuration is saved to the default location within the container when the builder is created:

f = path.Join(confutil.DefaultBuildKitConfigDir, f)

DefaultBuildKitConfigDir = "/etc/buildkit"

@jsternberg
Copy link
Collaborator Author

Two comments.

I would agree that maybe buildkitd should apply this logic when it's computing the default value for the --config flag instead of just checking if it's running in a user namespace: https://github.com/moby/buildkit/blob/762b2626ddafbc0874182b477f0393e8c5c3cb63/cmd/buildkitd/main.go#L414-L419

I like this. I took a simple look and it looks like setDefaultConfig is used after the config file is loaded. It seems reasonable to move that check to its own function and then use it for the config file loading too.

But maybe one solution could be to use a path that is writable by any user (like /tmp/buildkitd.toml?) and set the --config flag instead of relying on default values. WDYT?

Even with the above, I still think this is probably the best way to go. Explicitly setting --config is likely better and, if it's specified at the beginning, a user could overwrite it if they really wanted to. We likely don't want to rely on default behavior if we're writing a configuration file.

I am a bit confused by the /tmp folder requirement. My understanding is that the config file would not have to be writeable within the container and ContainerCopy likely worked differently for copying the contents. I can have the config file at /tmp easily, but I expected that the default location would continue working in rootless. I'll need to try it myself since I admit I didn't test it. So this is a good callout just in case it matters.

@jjacobelli
Copy link

I am a bit confused by the /tmp folder requirement. My understanding is that the config file would not have to be writeable within the container and ContainerCopy likely worked differently for copying the contents. I can have the config file at /tmp easily, but I expected that the default location would continue working in rootless. I'll need to try it myself since I admit I didn't test it. So this is a good callout just in case it matters.

I agree that having the file under /etc should be fine. I don't think this file needs to be writeable

@jsternberg
Copy link
Collaborator Author

I tested this and it works. Here's the output before the change:

5b8eb54588ab:/$ docker logs buildx_buildkit_a0
time="2023-11-07T20:59:27Z" level=info msg="auto snapshotter: using overlayfs"
time="2023-11-07T20:59:27Z" level=warning msg="failed to prepare cgroup controllers: mkdir /sys/fs/cgroup/init: permission denied"                                    time="2023-11-07T20:59:27Z" level=info msg="found worker \"46hll4gwasmbzwudaryggjls1\", labels=map[org.mobyproject.buildkit.worker.executor:oci org.mobyproject.buildkit.worker.hostname:8fd426fdd760 org.mobyproject.buildkit.worker.network:host org.mobyproject.buildkit.worker.oci.process-mode:sandbox org.mobyproject.buildkit.worker.selinux.enabled:false org.mobyproject.buildkit.worker.snapshotter:overlayfs], platforms=[linux/arm64 linux/amd64 linux/amd64/v2 linux/riscv64 linux/ppc64le linux/s390
x linux/386 linux/mips64le linux/mips64 linux/arm/v7 linux/arm/v6]"
time="2023-11-07T20:59:27Z" level=warning msg="skipping containerd worker, as \"/run/containerd/containerd.sock\" does not exist"
time="2023-11-07T20:59:27Z" level=info msg="found 1 workers, default=\"46hll4gwasmbzwudaryggjls1\""
time="2023-11-07T20:59:27Z" level=warning msg="currently, only the default worker can be used."
time="2023-11-07T20:59:27Z" level=info msg="running server on /run/buildkit/buildkitd.sock"

Here's the logs after the change:

5b8eb54588ab:~$ docker logs buildx_buildkit_a0
time="2023-11-07T21:03:36Z" level=info msg="auto snapshotter: using overlayfs"
time="2023-11-07T21:03:36Z" level=debug msg="running in rootless mode"
time="2023-11-07T21:03:36Z" level=warning msg="failed to prepare cgroup controllers: mkdir /sys/fs/cgroup/init: permission denied"
time="2023-11-07T21:03:36Z" level=info msg="found worker \"g361kgmo95xdbtgnf7rjk6lr0\", labels=map[org.mobyproject.buildkit.worker.executor:oci org.mobyproject.buildkit.worker.hostname:a44636f99be4 org.mobyproject.buildkit.worker.network:host org.mobyproject.buildkit.worker.oci.process-mode:sandbox org.mobyproject.buildkit.worker.selinux.enabled:false org.mobyproject.buildkit.worker.snapshotter:overlayfs], platforms=[linux/arm64 linux/amd64 linux/amd64/v2 linux/riscv64 linux/ppc64le linux/s390x linux/386 linux/mips64le linux/mips64 linux/arm/v7 linux/arm/v6]"
time="2023-11-07T21:03:36Z" level=warning msg="skipping containerd worker, as \"/run/containerd/containerd.sock\" does not exist"
time="2023-11-07T21:03:36Z" level=info msg="found 1 workers, default=\"g361kgmo95xdbtgnf7rjk6lr0\""
time="2023-11-07T21:03:36Z" level=warning msg="currently, only the default worker can be used."
time="2023-11-07T21:03:36Z" level=info msg="running server on /run/buildkit/buildkitd.sock"
time="2023-11-07T21:03:37Z" level=debug msg="remove snapshot" key=kiq5o65de3cbqjjjmr62yti4p snapshotter=overlayfs
time="2023-11-07T21:03:37Z" level=debug msg="schedule snapshotter cleanup" snapshotter=overlayfs
time="2023-11-07T21:03:37Z" level=debug msg="removed snapshot" key=buildkit/1/kiq5o65de3cbqjjjmr62yti4p snapshotter=overlayfs
time="2023-11-07T21:03:37Z" level=debug msg="snapshot garbage collected" d=6.464584ms snapshotter=overlayfs

The `docker-container` driver relies on the default config file location
for buildkit when writing the configuration file. When run in a rootless
version of docker (dind), the default location is different.

Instead of trying to figure out where the appropriate default location
is, this just writes the files to the same location and sets the
`--config` parameter explicitly. This flag is placed first so a
user-specified config option in `--buildkitd-flags` will take precedence
over the implicit config parameter.

This also fixes the `--config` option with the rootless image.
Previously, the config directory was being copied in a way that rendered
`/etc` unreadable and the configuration file wasn't readable either. It
also wasn't copied to the correct place. Now, `--config` is used to
specify the directory, `/etc` isn't included in the copied archive (so
the permissions aren't overwritten), and the directory is set as world
readable to be readable from the rootless buildkit process`.

Signed-off-by: Jonathan A. Sternberg <[email protected]>
@jsternberg
Copy link
Collaborator Author

I made a modification to this so now the --config option works properly with the rootless image.

Previously, the config directory was being copied with permissions of 0700 which meant the directory wasn't readable. It also wasn't being copied to the correct place so it wasn't being used anyway. Since the directory itself contained /etc and that was copied, it also overwrote the permissions from the original image and made the contents of /etc unreadable.

Now, it does not overwrite /etc, it uses --config to specify the directory in the command so the configuration is used, and the directory is now world readable so the rootless process can actually read the contents.

@tonistiigi tonistiigi merged commit 0408f3a into docker:master Nov 13, 2023
61 checks passed
@jsternberg jsternberg deleted the rootless-init-config branch November 20, 2023 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Buildkitd config file is missplaced when running in dind rootless with docker-container driver
4 participants