Make feature layer bind mounts read-write

[trxcllnt]

The new bind mounts for the feature source locations should be read-write, in case a feature does rm -rf /tmp/* while cleaning up after itself.

Or perhaps we should mount somewhere other than /tmp?

[jkeech]

@joshspicer @chrmarti thoughts on this change?

Or perhaps we should mount somewhere other than /tmp?

@trxcllnt, I like the idea of keeping the feature bind mount read-only, and I agree that putting it under /tmp makes it more difficult for a feature to run some cleanup with rm -rf /tmp/*. Do you have another path in mind? I could see it being under /opt/devcontainer or somewhere under /usr/local.

[trxcllnt]

Why not /build-features-src/${folder}, since /build-features-src doesn't conflict with any linux dirs? I worry about mounting to somewhere like /usr/local/src, since users may already be using that.

[chrmarti]

We might need to copy these files to the container for the lifecycle hooks @joshspicer is adding for features. That will make it writable.

If it is read-only like now, there shouldn't be any need for cleanup. Is there an intent of the change beyond cleanup?

[trxcllnt]

@chrmarti I don't have any other use-case besides cleanup. I ran into this when I ported something from a Dockerfile to a feature, and one of the layers downloaded things to /tmp then did rm -rf /tmp/* to clean up after itself.

[joshspicer]

This change is changing the location of the Features source inside the container to /usr/share/devcontainer/features.

[joshspicer]

Upon further discuss we decided in that linked PR to keep /tmp for now (and move the feature sources folder outside /tmp if there's demonstrated need. So i've pushed an update to this PR merging main's changes.

[chrmarti]

We mount the local folder we previously prepared. It would make more sense to delete that locally if we don't need it anymore (we could consider reusing it) instead of making it writeable from within the container. Does that make sense or am I missing something?

[trxcllnt]

tl;dr: After some testing, I think it's fine to change the feature mounts to read-write.

@chrmarti this issue doesn't have anything to do with removing files from the host, it's about feature code.

A feature's install.sh may do something like this:

# Download a big file to `/tmp`
wget -O /tmp/a_huge_file.sh https://get-a-big-file.com

# Executing a script may write other things to `/tmp`
sh /tmp/a_huge_file.sh

# cleanup /tmp
rm -rf /tmp/*

Today, the last line above will fail because the features are mounted as read-only under /tmp.

Test 1: deleting files in RW bind mounts from build stages

Since they're bind-mounts from a previous immutable build stage, any modifications to this mount are only visible to the current layer:

$ cat<<EOF> Dockerfile
FROM alpine as A
RUN touch /test.txt
FROM alpine
RUN --mount=type=bind,from=A,source=/,target=/A,rw rm /A/test.txt
RUN --mount=type=bind,from=A,source=/,target=/A,rw stat /A/test.txt
EOF

$ docker build --progress plain .
#6 [stage-1 2/3] RUN --mount=type=bind,from=A,source=/,target=/A,rw rm /A/test.txt
#6 DONE 0.6s
#7 [stage-1 3/3] RUN --mount=type=bind,from=A,source=/,target=/A,rw stat /A/test.txt
#7 0.546   File: /A/test.txt
#7 0.546   Size: 0         	Blocks: 0          IO Block: 4096   regular empty file

Test 2: running a script that deletes itself

This more accurately simulates what the feature does -- mounts a dir from a previous build stage that runs a script which could delete itself:

cat<<EOF> Dockerfile
FROM alpine as A
RUN echo 'rm -rf /tmp/*;' >> /tmp/test.sh
RUN echo 'echo "after rm -rf";' >> /tmp/test.sh
RUN chmod +x /tmp/test.sh
FROM alpine
RUN --mount=type=bind,from=A,source=/tmp,target=/tmp,rw /tmp/test.sh
EOF

$ docker build --progress plain --no-cache .
#9 [stage-1 2/2] RUN --mount=type=bind,from=A,source=/tmp,target=/tmp,rw /tmp/test.sh
#9 0.558 after rm -rf
#9 DONE 0.6s

Test 3: deleting files in rw bind mounts from the build context

Even if it was a bind mount to the build context, "read-write" doesn't actually persist anything to the host. I assume because the "build context" is actually somewhere in /var/lib/docker:

$ mkdir -p /tmp/docker-build-mount-test && cd /tmp/docker-build-mount-test
$ cat<<EOF> Dockerfile
FROM alpine
RUN --mount=type=bind,source=.,target=/context,rw touch /context/test.txt;
EOF
$ docker build .
$ ls -all .
drwxrwxr-x  2 ptaylor ptaylor   4096 Mar  8 10:04 .
drwxrwxrwt 25 root    root    102400 Mar  8 10:05 ..
-rw-rw-r--  1 ptaylor ptaylor     87 Mar  8 10:05 Dockerfile