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

[jsternberg]

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]

@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]

@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]

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

buildx/driver/docker-container/driver.go

Line 470 in 7838ade

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

buildx/util/confutil/container.go

Line 20 in 7838ade

DefaultBuildKitConfigDir = "/etc/buildkit"

[jsternberg]

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]

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]

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

[jsternberg]

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.