Repository: containers/buildah (language: go)

Issue:
buildah build --mount=bind can't access contents of previous stage when stage is cached
<!--
If you are reporting a new issue, make sure that we do not have any duplicates
already open. You can ensure this by searching the issue list for this
repository. If there is a duplicate, please close your issue and add a comment
to the existing issue instead.

If you suspect your issue is a bug, please edit your issue description to
include the BUG REPORT INFORMATION shown below. If you fail to provide this
information within 7 days, we cannot debug your issue and will close it. We
will, however, reopen it if you later provide the information.

---------------------------------------------------
BUG REPORT INFORMATION
---------------------------------------------------
Use the commands below to provide key information from your environment:
You do NOT have to include this information if this is a FEATURE REQUEST
-->

**Description**

`buildah build` with a `RUN --mount=type=bind` to a previous stage can't access contents of previous stage if stage is cached.

**Steps to reproduce the issue:**
Using this `Dockerfile`:
```Dockerfile
FROM quay.io/centos/centos:7 AS common
RUN echo "common" > /common.txt

FROM common AS build
RUN echo "build" > /build.txt

FROM quay.io/centos/centos:7
ARG NONCE
RUN --mount=type=bind,from=build,target=/build \
    set -ex; \
    cat /build/common.txt; \
    cat /build/build.txt; \
    : ;
```

Build twice
1. `buildah build --layers --build-arg NONCE="$(date +%N)" -f - < ./Dockerfile`
2. `buildah build --layers --build-arg NONCE="$(date +%N)" -f - < ./Dockerfile`

**Describe the results you received:**

Second build results in:
```text
[3/3] STEP 3/3: RUN --mount=type=bind,from=build,target=/build     set -ex;     cat /build/common.txt;     cat /build/build.txt;     : ;
+ cat /build/common.txt
common
+ cat /build/build.txt
cat: /build/build.txt: No such file or directory
Error: building at STEP "RUN --mount=type=bind,from=build,target=/build set -ex;     cat /build/common.txt;     cat /build/build.txt;     : ;": while running runtime: exit status 1
```

**Describe the results you expected:**

Build succeeds and outputs "build" during Step 3

**Output of `rpm -q buildah` or `apt list buildah`:**

Note: This doesn't match the version we used, we built latest to test
```
buildah-1.26.2-1.module+el8.6.0+997+05c9d812.x86_64
```

**Output of `buildah version`:**

(Built from master)
```
Version:         1.28.1-dev
Go Version:      go1.19
Image Spec:      1.0.2-dev
Runtime Spec:    1.0.2-dev
CNI Spec:        1.0.0
libcni Version:  v1.1.2
image Version:   5.23.1-dev
Git Commit:      ea425a2c85d0e62e5aa007877ff96316682213cc
Built:           Thu Oct 20 17:24:59 2022
OS/Arch:         linux/amd64
BuildPlatform:   linux/amd64
```


**Output of `cat /etc/*release`:**

```
Rocky Linux release 8.6 (Green Obsidian)
```

**Output of `uname -a`:**

```
Linux pd-ci 4.18.0-372.19.1.el8_6.x86_64 #1 SMP Tue Aug 2 16:19:42 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
```

**Output of `cat /etc/containers/storage.conf`:**

```
# This file is is the configuration file for all tools
# that use the containers/storage library. The storage.conf file
# overrides all other storage.conf files. Container engines using the
# container/storage library do not inherit fields from other storage.conf
# files.
#
#  Note: The storage.conf file overrides other storage.conf files based on this precedence:
#      /usr/containers/storage.conf
#      /etc/containers/storage.conf
#      $HOME/.config/containers/storage.conf
#      $XDG_CONFIG_HOME/containers/storage.conf (If XDG_CONFIG_HOME is set)
# See man 5 containers-storage.conf for more information
# The "container storage" table contains all of the server options.
[storage]

# Default Storage Driver, Must be set for proper operation.
driver = "overlay"

# Temporary storage location
runroot = "/run/containers/storage"

# Primary Read/Write location of container storage
# When changing the graphroot location on an SELINUX system, you must
# ensure  the labeling matches the default locations labels with the
# following commands:
# semanage fcontext -a -e /var/lib/containers/storage /NEWSTORAGEPATH
# restorecon -R -v /NEWSTORAGEPATH
graphroot = "/var/lib/containers/storage"


# Storage path for rootless users
#
# rootless_storage_path = "$HOME/.local/share/containers/storage"

[storage.options]
# Storage options to be passed to underlying storage drivers

# AdditionalImageStores is used to pass paths to additional Read/Only image stores
# Must be comma separated list.
additionalimagestores = [
]

# Remap-UIDs/GIDs is the mapping from UIDs/GIDs as they should appear inside of
# a container, to the UIDs/GIDs as they should appear outside of the container,
# and the length of the range of UIDs/GIDs.  Additional mapped sets can be
# listed and will be heeded by libraries, but there are limits to the number of
# mappings which the kernel will allow when you later attempt to run a
# container.
#
# remap-uids = 0:1668442479:65536
# remap-gids = 0:1668442479:65536

# Remap-User/Group is a user name which can be used to look up one or more UID/GID
# ranges in the /etc/subuid or /etc/subgid file.  Mappings are set up starting
# with an in-container ID of 0 and then a host-level ID taken from the lowest
# range that matches the specified name, and using the length of that range.
# Additional ranges are then assigned, using the ranges which specify the
# lowest host-level IDs first, to the lowest not-yet-mapped in-container ID,
# until all of the entries have been used for maps.
#
# remap-user = "containers"
# remap-group = "containers"

# Root-auto-userns-user is a user name which can be used to look up one or more UID/GID
# ranges in the /etc/subuid and /etc/subgid file.  These ranges will be partitioned
# to containers configured to create automatically a user namespace.  Containers
# configured to automatically create a user namespace can still overlap with containers
# having an explicit mapping set.
# This setting is ignored when running as rootless.
# root-auto-userns-user = "storage"
#
# Auto-userns-min-size is the minimum size for a user namespace created automatically.
# auto-userns-min-size=1024
#
# Auto-userns-max-size is the minimum size for a user namespace created automatically.
# auto-userns-max-size=65536

[storage.options.overlay]
# ignore_chown_errors can be set to allow a non privileged user running with
# a single UID within a user namespace to run containers. The user can pull
# and use any image even those with multiple uids.  Note multiple UIDs will be
# squashed down to the default uid in the container.  These images will have no
# separation between the users in the container. Only supported for the overlay
# and vfs drivers.
#ignore_chown_errors = "false"

# Inodes is used to set a maximum inodes of the container image.
# inodes = ""

# Path to an helper program to use for mounting the file system instead of mounting it
# directly.
#mount_program = "/usr/bin/fuse-overlayfs"

# mountopt specifies comma separated list of extra mount options
mountopt = "nodev,metacopy=on"

# Set to skip a PRIVATE bind mount on the storage home directory.
# skip_mount_home = "false"

# Size is used to set a maximum size of the container image.
# size = ""

# ForceMask specifies the permissions mask that is used for new files and
# directories.
#
# The values "shared" and "private" are accepted.
# Octal permission masks are also accepted.
#
#  "": No value specified.
#     All files/directories, get set with the permissions identified within the
#     image.
#  "private": it is equivalent to 0700.
#     All files/directories get set with 0700 permissions.  The owner has rwx
#     access to the files. No other users on the system can access the files.
#     This setting could be used with networked based homedirs.
#  "shared": it is equivalent to 0755.
#     The owner has rwx access to the files and everyone else can read, access
#     and execute them. This setting is useful for sharing containers storage
#     with other users.  For instance have a storage owned by root but shared
#     to rootless users as an additional store.
#     NOTE:  All files within the image are made readable and executable by any
#     user on the system. Even /etc/shadow within your image is now readable by
#     any user.
#
#   OCTAL: Users can experiment with other OCTAL Permissions.
#
#  Note: The force_mask Flag is an experimental feature, it could change in the
#  future.  When "force_mask" is set the original permission mask is stored in
#  the "user.containers.override_stat" xattr and the "mount_program" option must
#  be specified. Mount programs like "/usr/bin/fuse-overlayfs" present the
#  extended attribute permissions to processes within containers rather then the
#  "force_mask"  permissions.
#
# force_mask = ""

[storage.options.thinpool]
# Storage Options for thinpool

# autoextend_percent determines the amount by which pool needs to be
# grown. This is specified in terms of % of pool size. So a value of 20 means
# that when threshold is hit, pool will be grown by 20% of existing
# pool size.
# autoextend_percent = "20"

# autoextend_threshold determines the pool extension threshold in terms
# of percentage of pool size. For example, if threshold is 60, that means when
# pool is 60% full, threshold has been hit.
# autoextend_threshold = "80"

# basesize specifies the size to use when creating the base device, which
# limits the size of images and containers.
# basesize = "10G"

# blocksize specifies a custom blocksize to use for the thin pool.
# blocksize="64k"

# directlvm_device specifies a custom block storage device to use for the
# thin pool. Required if you setup devicemapper.
# directlvm_device = ""

# directlvm_device_force wipes device even if device already has a filesystem.
# directlvm_device_force = "True"

# fs specifies the filesystem type to use for the base device.
# fs="xfs"

# log_level sets the log level of devicemapper.
# 0: LogLevelSuppress 0 (Default)
# 2: LogLevelFatal
# 3: LogLevelErr
# 4: LogLevelWarn
# 5: LogLevelNotice
# 6: LogLevelInfo
# 7: LogLevelDebug
# log_level = "7"

# min_free_space specifies the min free space percent in a thin pool require for
# new device creation to succeed. Valid values are from 0% - 99%.
# Value 0% disables
# min_free_space = "10%"

# mkfsarg specifies extra mkfs arguments to be used when creating the base
# device.
# mkfsarg = ""

# metadata_size is used to set the `pvcreate --metadatasize` options when
# creating thin devices. Default is 128k
# metadata_size = ""

# Size is used to set a maximum size of the container image.
# size = ""

# use_deferred_removal marks devicemapper block device for deferred removal.
# If the thinpool is in use when the driver attempts to remove it, the driver
# tells the kernel to remove it as soon as possible. Note this does not free
# up the disk space, use deferred deletion to fully remove the thinpool.
# use_deferred_removal = "True"

# use_deferred_deletion marks thinpool device for deferred deletion.
# If the device is busy when the driver attempts to delete it, the driver
# will attempt to delete device every 30 seconds until successful.
# If the program using the driver exits, the driver will continue attempting
# to cleanup the next time the driver is used. Deferred deletion permanently
# deletes the device and all data stored in device will be lost.
# use_deferred_deletion = "True"

# xfs_nospace_max_retries specifies the maximum number of retries XFS should
# attempt to complete IO when ENOSPC (no space) error is returned by
# underlying storage device.
# xfs_nospace_max_retries = "0"
```



Discussion / hints:
I can replicate it with a single build and contrived Dockerfile if that's easier to add to a test, but I can't guarantee it's the same bug:

```Dockerfile
FROM quay.io/centos/centos:7 AS common

RUN echo "common" > /common.txt

FROM common AS buildA
RUN echo "foo" > /foo.txt

FROM common AS buildB
# This is contrived to force a cached layer without having to build twice
# Ordinarily you wouldn't have duplicate stages
RUN echo "foo" > /foo.txt

FROM quay.io/centos/centos:7

ARG NONCE

RUN --mount=type=bind,from=buildA,target=/buildA \
    --mount=type=bind,from=buildB,target=/buildB \
    set -ex; \
    cat /buildA/common.txt; \
    cat /buildA/foo.txt; \
    cat /buildB/common.txt; \
    cat /buildB/foo.txt; \
    : ;

```

Output:
```
<SNIP>
[4/4] STEP 3/3: RUN --mount=type=bind,from=buildA,target=/buildA     --mount=type=bind,from=buildB,target=/buildB     set -ex;     cat /buildA/common.txt;     cat /buildA/foo.txt;     cat /buildB/common.txt;     cat /buildB/foo.txt;     : ;
+ cat /buildA/common.txt
common
+ cat /buildA/foo.txt
foo
+ cat /buildB/common.txt
common
+ cat /buildB/foo.txt
cat: /buildB/foo.txt: No such file or directory
Error: building at STEP "RUN --mount=type=bind,from=buildA,target=/buildA --mount=type=bind,from=buildB,target=/buildB set -ex;     cat /buildA/common.txt;     cat /buildA/foo.txt;     cat /buildB/common.txt;     cat /buildB/foo.txt;     : ;
```
Ok, I think I figured out what's going on.

Every stage is a container (as opposed to an image) to support (I think) layer-less builds. Instead of sourcing the committed image of the previous stage it sources the container, which is created during the `FROM`.

Let's call our builds A and B, where A has no cache.
```Dockerfile
FROM quay.io/centos/centos:7 AS common (1)
RUN echo "common" > /common.txt (2)
(3)
FROM common AS build (4)
RUN echo "build" > /build.txt (5)
(6)
FROM quay.io/centos/centos:7 (7)
ARG NONCE (8)
RUN --mount=type=bind,from=build,target=/build \ (9)
    set -ex; \
    cat /build/common.txt; \
    cat /build/build.txt; \
    : ;
```

During build A we get to (4) and buildah creates a new container, we'll call it aaaa. It doesn't have a cache, so runs (5), then commits it to an image we'll call 1111. Crucially, when it gets to (9), it does _not_ mount 1111 (image) as the bind, it mounts aaaa (container) as the source. In this case it doesn't matter because aaaa (container) is the same as it was the source of 1111 (image).

Now we get to build B (4), and buildah again creates a new container, we'll call it bbbb. It discovers it has a cache and skips (5), reporting that it's cached as 1111 (image). But when we get to (9), it still mounts bbbb (container) which _does not_ have anything in it beyond the FROM so the bind mount does not contain `/build.txt`.

That's the root cause, now I'm going to see how it can be fixed.

Worth noting that inserting a `COPY` prior to the `RUN --mount` avoids the issue:

```Dockerfile
FROM artifacts.mitre.org:8200/busybox AS common
RUN echo "common" > /yescopy_file__common.txt

FROM common AS build
RUN echo "build" > /yescopy_file__build.txt

FROM artifacts.mitre.org:8200/busybox
ARG NONCE
ENV NONCE=$NONCE

RUN echo $NONCE

COPY --from=build /yescopy_file__build.txt /yescopy_file__copy_build.txt

RUN --mount=type=bind,from=build,target=/build \
    set -ex; \
    cat /yescopy_file__copy_build.txt; \
    cat /build/yescopy_file__common.txt; \
    cat /build/yescopy_file__build.txt; \
    : ;
```

If I can find the logic that helps that work I should be able to apply it here.
@marwatk Thanks for creating the issue and pasting detailed findings. I'll try checking this within this week and update back here ( hopefully with a patch ) or concrete idea with how to fix this.

Fix patch:
diff --git a/imagebuildah/executor.go b/imagebuildah/executor.go
index 8fcdeb8e3f9..8891f5045ec 100644
--- a/imagebuildah/executor.go
+++ b/imagebuildah/executor.go
@@ -757,6 +757,8 @@ func (b *Executor) Build(ctx context.Context, stages imagebuilder.Stages) (image
 										// add base to current stage's dependency tree
 										// but also confirm if this is not in additional context.
 										if _, ok := b.additionalBuildContexts[mountFrom]; !ok {
+											// Treat from as a rootfs we need to preserve
+											b.rootfsMap[mountFrom] = true
 											if _, ok := dependencyMap[mountFrom]; ok {
 												// update current stage's dependency info
 												currentStageInfo := dependencyMap[stage.Name]


Test patch:
diff --git a/tests/bud.bats b/tests/bud.bats
index c347fff2755..992144bbd7b 100644
--- a/tests/bud.bats
+++ b/tests/bud.bats
@@ -3842,6 +3842,12 @@ EOM
   run_buildah rm -a
 }
 
+@test "bud preserve rootfs for --mount=type=bind,from=" {
+  _prefetch alpine
+  run_buildah build --build-arg NONCE="$(date)" --layers --quiet --pull=false $WITH_POLICY_JSON -f Dockerfile.3 $BUDFILES/cache-stages
+  expect_output --substring "Worked"
+}
+
 @test "bud timestamp" {
   _prefetch alpine
   timestamp=40
diff --git a/tests/bud/cache-stages/Dockerfile.3 b/tests/bud/cache-stages/Dockerfile.3
new file mode 100644
index 00000000000..0dc1bfcb8d0
--- /dev/null
+++ b/tests/bud/cache-stages/Dockerfile.3
@@ -0,0 +1,25 @@
+FROM alpine AS common
+
+RUN echo "common" > /common.txt
+
+FROM common AS buildA
+RUN echo "foo" > /foo.txt
+
+FROM common AS buildB
+# This is contrived to force a cached layer without having to build twice
+# Ordinarily you wouldn't have duplicate stages
+RUN echo "foo" > /foo.txt
+
+FROM alpine
+
+ARG NONCE
+
+RUN --mount=type=bind,from=buildA,target=/buildA \
+    --mount=type=bind,from=buildB,target=/buildB \
+    set -ex; \
+    cat /buildA/common.txt; \
+    cat /buildA/foo.txt; \
+    cat /buildB/common.txt; \
+    cat /buildB/foo.txt; \
+    echo "Worked"; \
+    : ;