build: Cleanup transient mount destinations with every RUN step

[flouthoc]

Following PR ensures that we cleanup dangling /run and dir after every RUN
command and remount with every RUN inorder to make sure that it does not
persists on physical image. Ensureparity with how docker behaves with .dockerenv.

Closes: #3523

[flouthoc]

/hold

[rhatdan]

I think there are other RUN artifacts then just /run. We add some inodes in /etc as well.

[flouthoc]

@rhatdan I think /etc is not a problem for the issue which @cgwalters created since other directory are already part of expected file structure instead of /run but sure we can clean up those as well if its needed. @cgwalters could you please confirm ?

[flouthoc]

cc @nalind

[nalind]

This gets us closer to cleaning up everything we leave behind during RUN, but it doesn't get us all the way there.

[TomSweeneyRedHat]

So, just to make sure, let's assume we have a secret that contains database credentials. In the Containerfile there are three or more RUN statements that each relies on those credentials. The credentials will be available for each instance of RUN?

I'm not sure we have a test for this, it would be nice to add one if not.

[rhatdan]

@TomSweeneyRedHat I believe each RUN is independent and will recreate the /run/secrets. This patch is just removing them when the container completes, so that a buildah commit will not keep them.

[flouthoc]

@TomSweeneyRedHat so its exactly as @rhatdan stated every RUN is independent and things get recreated on individual run. We are just removing them so nothing persists in image.

[rhatdan]

My concern with this, without playing with it, is what happens if I want to add /run/foobar or /etc/resolv.conf to my image, can we differentiate between this.

from fedora
run touch /run/foobar
add hosts /etc/hosts

What happens in this case?

[flouthoc]

@rhatdan I have removed /etc/ from cleanup lists since i am not able to see /etc as dangling file in any of the image. Apart from that i tried building and i can see /run/foobar in image as it is not part of bindFiles list so cleanup will not remove any files manually added by user

FROM registry.fedoraproject.org/fedora:34
RUN touch /run/foobar

@nalind @rhatdan what are some other dangling stuff which we leave behind since i can only see /run/secrets and /run/.containerenv i dont think /etc is dangling at all i can only see that in rootfs

[rhatdan]

How about:
cat Containerfile
FROM registry.fedoraproject.org/fedora:34
RUN rm /etc/hosts
RUN echo hi
buildah build -t test1 .
Does test1 have an /etc/hosts file within it?

[flouthoc]

@rhatdan this build fails for me with

STEP 2/3: RUN rm /etc/hosts
rm: cannot remove '/etc/hosts': Device or resource busy
error building at STEP "RUN rm /etc/hosts": error while running runtime: exit status 1
ERRO[0005] exit status 1    

So /etc/hosts is there but i can't see that on exploded layers commit-ed so i think it does not persists on layers. I cant see it when I try to inspect exploded layers manually.

[rhatdan]

I guess the best test would be to create a statically linked c program put into an image

Cat Containerfile
from scratch
copy a.out /
run a.out

Then see if the image has anything in it except for a.out.

[flouthoc]

ps: discussed this with @nalind so we also have to take care of all the other mounts which are not getting cleanedup eg bind mounts
So I'll re-skim the code and add more components to this.

[edsantiago]

I may be misunderstanding the intention behind this PR. I ran this command, and expected null output (or at least only directories, no files). Instead:

$ printf "FROM quay.io/podman/stable:latest\nRUN podman pull quay.io/libpod/testimage:20210610\nRUN ls -lR /run\n" | ./bin/buildah bud -
STEP 1/3: FROM quay.io/podman/stable:latest
STEP 2/3: RUN podman pull quay.io/libpod/testimage:20210610
time="2021-09-21T20:22:17Z" level=warning msg="\"/\" is not a shared mount, this could cause issues or missing mounts with rootless containers"
Trying to pull quay.io/libpod/testimage:20210610...
Getting image source signatures
Copying blob sha256:9afcdfe780b4ea44cc52d22e3f93ccf212388a90370773571ce034a62e14174e
Copying blob sha256:9afcdfe780b4ea44cc52d22e3f93ccf212388a90370773571ce034a62e14174e
Copying config sha256:9f9ec7f2fdef9168f74e9d057f307955db14d782cff22ded51d277d74798cb2f
Writing manifest to image destination
Storing signatures
9f9ec7f2fdef9168f74e9d057f307955db14d782cff22ded51d277d74798cb2f
STEP 3/3: RUN ls -lR /run
/run:
total 0
drwxr-xr-x. 1 root root   0 Sep 21 08:06 console
drwx------. 1 root root  14 Sep 21 20:22 containers
drwxr-xr-x. 1 root root   0 Jan 26  2021 criu
drwx------. 1 root root   0 Sep 21 08:06 cryptsetup
drwxr-xr-x. 1 root root   0 Sep 21 08:06 faillock
drwxr-x--x. 1 root root  68 Sep 21 20:22 libpod
drwxr-xr-x. 1 root root  12 Sep 21 08:06 lock
drwxr-xr-x. 1 root root   0 Sep 21 08:06 log
-rw-r--r--. 1 root root   0 Sep 21 08:06 motd
....
/run/containers/storage/overlay-layers:
total 8
-rw-------. 1 root root  2 Sep 21 20:22 mountpoints.json
-rw-r--r--. 1 root root 64 Sep 21 20:22 mountpoints.lock
....

I tried with buildah @ main (without this patch) and the output looks identical.

[rhatdan]

@edsantiago during the run the files and directories will be there. The PR is attempting to make sure they do not get committed to the image.
So you would need to mount the final image with something like
mnt=$(podman image mount IMAGE)
ls -l $/mnt/run

[edsantiago]

$ printf "FROM quay.io/podman/stable:latest\nRUN podman pull quay.io/libpod/testimage:20210610\n" | ./bin/buildah bud -t foo -
.....
$ podman run --rm foo ls -laR /run|grep '^-r'
-rw-r--r--. 1 root root   0 Sep 21 20:45 .containerenv
-rw-r--r--. 1 root root   0 Sep 21 08:06 motd
-rw-------. 1 root root   2 Sep 21 20:40 mountpoints.json
-rw-r--r--. 1 root root  64 Sep 21 20:40 mountpoints.lock
-rw-r--r--. 1 root root  0 Sep 21 20:40 alive
-rw-r--r--. 1 root root  0 Sep 21 20:40 alive.lck
-rw-------. 1 root root  2 Sep 21 20:40 pause.pid
-rwx------. 1 root root 229 Sep 21 20:40 events.log
-rw-r--r--. 1 root root   0 Sep 21 20:40 events.log.lock

[rhatdan]

Podman puts them back as well. We are just looking at what goes into the image. Not what a running container sees.
Another way to look at this would be to see what the image looks like when Docker runs it. /run/.containerenv and /run/secrets should not be there.

[nalind]

Yes that is my concern.

Is
run echo localhost 126.0.0.1 > /etc/resolv.conf
Safe?

If that's a volume mounted file, then I wouldn't expect that content to show up in the image.

[flouthoc]

@rhatdan that is safe
Edit: as nalind mention it doesnt shows up in image. But we are not doing anything special to cleanup /etc so its expected behaviour.

[nalind]

I think this is fallout from #3518, which used pkg/rusage as its "quick" check to see if go test supported a -race flag. It didn't use -mod=vendor, and even for a small package, it has some external dependencies that would get pulled down.
Sorry, wrong PR.

[flouthoc]

@nalind @rhatdan please let me know if anything else is needed on this PR.

[rhatdan]

This PR makes it slightly better.

$ cat t.go 
package main
import ("fmt")
func main() {
	fmt.Println("Hello World")
}
$ go build -o t t.go
$ cat Containerfile
from scratch
copy t /t
run [ "/t" ]
$ ./bin/buildah bud -t dan .
STEP 1/3: FROM scratch
STEP 2/3: copy t /t
STEP 3/3: run [ "/t" ]
Hello World
COMMIT dan
Getting image source signatures
Copying blob b31164ddec40 done  
Copying config 82eb390304 done  
Writing manifest to image destination
Storing signatures
--> 82eb3903049
Successfully tagged localhost/dan:latest
82eb390304938f16dd707f32abaa8464af8d4a25959ab342e25696a540ec56b5
$ buildah unshare 
# mnt=$(podman image mount dan)
# find $mnt
/home/dwalsh/.local/share/containers/storage/overlay/b31164ddec40ddf1e0e42fa9848c17f0bf30fa4f8b9e4e0bcd4c4546f2579df0/merged
/home/dwalsh/.local/share/containers/storage/overlay/b31164ddec40ddf1e0e42fa9848c17f0bf30fa4f8b9e4e0bcd4c4546f2579df0/merged/dev
/home/dwalsh/.local/share/containers/storage/overlay/b31164ddec40ddf1e0e42fa9848c17f0bf30fa4f8b9e4e0bcd4c4546f2579df0/merged/etc
/home/dwalsh/.local/share/containers/storage/overlay/b31164ddec40ddf1e0e42fa9848c17f0bf30fa4f8b9e4e0bcd4c4546f2579df0/merged/etc/hosts
/home/dwalsh/.local/share/containers/storage/overlay/b31164ddec40ddf1e0e42fa9848c17f0bf30fa4f8b9e4e0bcd4c4546f2579df0/merged/etc/resolv.conf
/home/dwalsh/.local/share/containers/storage/overlay/b31164ddec40ddf1e0e42fa9848c17f0bf30fa4f8b9e4e0bcd4c4546f2579df0/merged/proc
/home/dwalsh/.local/share/containers/storage/overlay/b31164ddec40ddf1e0e42fa9848c17f0bf30fa4f8b9e4e0bcd4c4546f2579df0/merged/run
/home/dwalsh/.local/share/containers/storage/overlay/b31164ddec40ddf1e0e42fa9848c17f0bf30fa4f8b9e4e0bcd4c4546f2579df0/merged/sys
/home/dwalsh/.local/share/containers/storage/overlay/b31164ddec40ddf1e0e42fa9848c17f0bf30fa4f8b9e4e0bcd4c4546f2579df0/merged/t

I see /etc/hosts and /etc/resolv.conf in the image. Also see other main mount points /proc, /sys, /run, /dev and /etc

[flouthoc]

@rhatdan PR refrains from touching paths starting with /etc, /dev, /sys, /proc as unionfs has a bug or a feature ( dont know what to call it ) that whenever a file is removed and lower contains similar file a .wh or whiteout file is added as a replacement. We can cleanup .wh but don't know if we should perform cleanup on these paths. @nalind Could you confirm please.

[nalind]

@rhatdan PR refrains from touching paths starting with /etc, /dev, /sys, /proc as unionfs has a bug or a feature ( dont know what to call it ) that whenever a file is removed and lower contains similar file a .wh or whiteout file is added as a replacement. We can cleanup .wh but don't know if we should perform cleanup on these paths. @nalind Could you confirm please.

No, we wouldn't do that. If L is the lower layer, and U is the upper layer in a mounted overlay filesystem M, the whiteout is used to mark for the kernel that the file was removed at U and thus shouldn't appear when looking at M. L can be used as a lower layer for other mounts at the very same time, or it can be on read-only storage, but in either case the design of overlay is such that modifications made in the overlay mount are always, and only, recorded in the upper layer (U).

[flouthoc]

@nalind thanks for the detailed explanation.

@rhatdan @nalind then i guess these are the only mounts we can cleanup and i don't think people should face any issues with other paths.

[rhatdan]

I am not sure we want to hit those other paths anyways, since readonly containers will rely on them
LGTM.

[nalind]

This definitely needs a test.

[cgwalters]

When testing, I've been doing e.g. skopeo copy containers-storage:localhost/testimage oci:testimage and then look in testimage/blobs/sha256 at the tarballs. I doubt there's a podman cli to get the raw stream, but OTOH it probably wouldn't be hard to add.

[flouthoc]

Added tests

@rhatdan PR refrains from touching paths starting with /etc, /dev, /sys, /proc as unionfs has a bug or a feature ( dont know what to call it ) that whenever a file is removed and lower contains similar file a .wh or whiteout file is added as a replacement. We can cleanup .wh but don't know if we should perform cleanup on these paths. @nalind Could you confirm please.

Just for ref: I also experimented with how docker is handling cleanups for transient files in these paths looks like docker is also not doing cleanup at all for these paths as well. Infact first thing they do is create /etc/hosts and /etc/resolve.conf no matter if they exists on lower or not and both of them is persisted in image as well.

[rhatdan]

SGTM

[flouthoc]

@nalind @rhatdan PTAL

[nalind]

I'm always going to be uneasy about deleting data from a rootfs, but the tests pass, so LGTM.

[rhatdan]

/lgtm
/hold cancel