Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release-1.19] ADD/COPY: create the destination directory first, chroot to it #3052

Merged

Conversation

nalind
Copy link
Member

@nalind nalind commented Mar 3, 2021

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 build would.

How to verify it

New conformance tests!
Additions to unit tests!

Which issue(s) this PR fixes:

Special notes for your reviewer:

Cherry-picked from #3015.

Does this PR introduce a user-facing change?

None

nalind added 5 commits March 3, 2021 08:43
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]>
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]>
@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Mar 3, 2021
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nalind

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rhatdan
Copy link
Member

rhatdan commented Mar 3, 2021

/lgtm

@openshift-merge-robot openshift-merge-robot merged commit d51c437 into containers:release-1.19 Mar 3, 2021
@nalind nalind deleted the copier-mkdir-first-1.19 branch March 3, 2021 15:19
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved kind/bug Categorizes issue or PR as related to a bug. lgtm locked - please file new issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants