ADD/COPY: create the destination directory first, chroot to it #3015
Conversation
openshift-ci-robot
added
the
kind/bug
|
/cc @vrothberg |
nalind
force-pushed
the
copier-mkdir-first
branch
from
348f74a to
c9dd9ce
Compare
|
LGTM |
| GIDMap: destGIDMap, | ||
| ChownNew: chownDirs, | ||
| } | ||
| if err := copier.Mkdir(mountPoint, extractDirectory, mkdirOptions); err != nil { |
There was a problem hiding this comment.
Would it be possible to move that down into the copier package? podman cp would benefit from it, also when copying from a container to the host.
There was a problem hiding this comment.
Not without pretty drastic restructuring - in cases where the destination doesn't exist yet, and the path can include components which are symbolic links, we have to chroot to the rootfs first, then evaluate the path for symbolic links, then create the target directory, and then chroot to the target directory, and trying to stuff all of that into Put() broke just about every other unit test and several integration tests. In the end, having Add() do both an Eval() and a Mkdir() explicitly looks much easier to test.
|
LGTM, but may need a rebase. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nalind, rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
If the subprocess exits with an error, but we can't decode its stdout as a proper status result, check if it produced error output. If it did, then return its error output as the error. Signed-off-by: Nalin Dahyabhai <[email protected]>
When attempting to handle renames, we'd fail to correctly handle renames of prefixes of a given item's path because of a string handling error, and add a unit test for the rename logic (finally). Signed-off-by: Nalin Dahyabhai <[email protected]>
nalind
force-pushed
the
copier-mkdir-first
branch
from
c9dd9ce to
42d2f42
Compare
Add copier.Eval(), for expanding paths using symbolic links in a chrooted scope, without failing if a path component doesn't exist. Signed-off-by: Nalin Dahyabhai <[email protected]>
Add a NoDerefSymlinks flag to force items that are matched to the Globs we're given to be treated as symlinks, rather than dereferencing them as we would need to do for sources for ADD or COPY. Signed-off-by: Nalin Dahyabhai <[email protected]>
Always create the destination directory first when ADDing or COPYing content into a container, then extract contents into it using the destination directory as the chroot instead of the container's root directory. Signed-off-by: Nalin Dahyabhai <[email protected]>
nalind
force-pushed
the
copier-mkdir-first
branch
from
42d2f42 to
effb375
Compare
|
Added |
|
/lgtm |
|
Can we backport the changes? I need them in Podman 3.0. |
|
@vrothberg But not for RHEL 8.4 right? |
|
@TomSweeneyRedHat, especially for 8.4. This will help fix the reported |
|
Do we have a bugzilla for this? I have opened a PR to podman to vendor in the latest buildah 1.19.7. |
Not yet. There is a number of |
|
There are some challenges in Podman since we have to create the destination directory in some cases. It works well in Buildah but with the remote client in Podman, it's tricky. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
When extracting archives that include symbolic links and paths accessed through those links, we would chroot to the root of the container's filesystem rather than the destination directory. While we didn't write outside of the container's root directory, this allowed for cases where we put things in different locations than
docker buildwould.How to verify it
New conformance tests!
Additions to unit tests!
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?