Skip to content

builder: fix COPY --from should preserve ownership #38599

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.

Sign up for GitHub

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

Merged
tiborvass merged 1 commit into from
Mar 22, 2019
Merged

builder: fix COPY --from should preserve ownership #38599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 23 additions & 10 deletions builder/dockerfile/copy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ type copyInstruction struct {
dest string
chownStr string
allowLocalDecompression bool
preserveOwnership bool
}

// copier reads a raw COPY or ADD command, fetches remote sources using a downloader,
Expand Down Expand Up @@ -466,7 +467,7 @@ func downloadSource(output io.Writer, stdout io.Writer, srcURL string) (remote b

type copyFileOptions struct {
decompress bool
identity idtools.Identity
identity *idtools.Identity
archiver Archiver
}

Expand Down Expand Up @@ -532,7 +533,7 @@ func isArchivePath(driver containerfs.ContainerFS, path string) bool {
return err == nil
}

func copyDirectory(archiver Archiver, source, dest *copyEndpoint, identity idtools.Identity) error {
func copyDirectory(archiver Archiver, source, dest *copyEndpoint, identity *idtools.Identity) error {
destExists, err := isExistingDirectory(dest)
if err != nil {
return errors.Wrapf(err, "failed to query destination path")
Expand All @@ -541,28 +542,40 @@ func copyDirectory(archiver Archiver, source, dest *copyEndpoint, identity idtoo
if err := archiver.CopyWithTar(source.path, dest.path); err != nil {
return errors.Wrapf(err, "failed to copy directory")
}
// TODO: @gupta-ak. Investigate how LCOW permission mappings will work.
return fixPermissions(source.path, dest.path, identity, !destExists)
if identity != nil {
// TODO: @gupta-ak. Investigate how LCOW permission mappings will work.
return fixPermissions(source.path, dest.path, *identity, !destExists)
}
return nil
}

func copyFile(archiver Archiver, source, dest *copyEndpoint, identity idtools.Identity) error {
func copyFile(archiver Archiver, source, dest *copyEndpoint, identity *idtools.Identity) error {
if runtime.GOOS == "windows" && dest.driver.OS() == "linux" {
// LCOW
if err := dest.driver.MkdirAll(dest.driver.Dir(dest.path), 0755); err != nil {
return errors.Wrapf(err, "failed to create new directory")
}
} else {
if err := idtools.MkdirAllAndChownNew(filepath.Dir(dest.path), 0755, identity); err != nil {
// Normal containers
return errors.Wrapf(err, "failed to create new directory")
if identity == nil {
if err := os.MkdirAll(filepath.Dir(dest.path), 0755); err != nil {
return err
}
} else {
if err := idtools.MkdirAllAndChownNew(filepath.Dir(dest.path), 0755, *identity); err != nil {
// Normal containers
return errors.Wrapf(err, "failed to create new directory")
}
}
}

if err := archiver.CopyFileWithTar(source.path, dest.path); err != nil {
return errors.Wrapf(err, "failed to copy file")
}
// TODO: @gupta-ak. Investigate how LCOW permission mappings will work.
return fixPermissions(source.path, dest.path, identity, false)
if identity != nil {
// TODO: @gupta-ak. Investigate how LCOW permission mappings will work.
return fixPermissions(source.path, dest.path, *identity, false)
}
return nil
}

func endsInSlash(driver containerfs.Driver, path string) bool {
Expand Down
4 changes: 3 additions & 1 deletion builder/dockerfile/dispatchers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,9 @@ func dispatchCopy(d dispatchRequest, c *instructions.CopyCommand) error {
return err
}
copyInstruction.chownStr = c.Chown

if c.From != "" && copyInstruction.chownStr == "" {
copyInstruction.preserveOwnership = true
}
return d.builder.performCopy(d, copyInstruction)
}

Expand Down
4 changes: 3 additions & 1 deletion builder/dockerfile/internals.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,9 @@ func (b *Builder) performCopy(req dispatchRequest, inst copyInstruction) error {
opts := copyFileOptions{
decompress: inst.allowLocalDecompression,
archiver: b.getArchiver(info.root, destInfo.root),
identity: identity,
}
if !inst.preserveOwnership {
opts.identity = &identity
}
if err := performCopyForInfo(destInfo, info, opts); err != nil {
return errors.Wrapf(err, "failed to copy files")
Expand Down
39 changes: 39 additions & 0 deletions integration/build/build_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -522,6 +522,45 @@ func TestBuildWithEmptyDockerfile(t *testing.T) {
}
}

func TestBuildPreserveOwnership(t *testing.T) {
skip.If(t, testEnv.DaemonInfo.OSType == "windows", "FIXME")
skip.If(t, versions.LessThan(testEnv.DaemonAPIVersion(), "1.40"), "broken in earlier versions")

ctx := context.Background()

dockerfile, err := ioutil.ReadFile("testdata/Dockerfile.testBuildPreserveOwnership")
assert.NilError(t, err)

source := fakecontext.New(t, "", fakecontext.WithDockerfile(string(dockerfile)))
defer source.Close()

apiclient := testEnv.APIClient()

for _, target := range []string{"copy_from", "copy_from_chowned"} {
t.Run(target, func(t *testing.T) {
resp, err := apiclient.ImageBuild(
ctx,
source.AsTarReader(t),
types.ImageBuildOptions{
Remove: true,
ForceRemove: true,
Target: target,
},
)
assert.NilError(t, err)

out := bytes.NewBuffer(nil)
assert.NilError(t, err)
_, err = io.Copy(out, resp.Body)
_ = resp.Body.Close()
if err != nil {
t.Log(out)
}
assert.NilError(t, err)
})
}
}

func writeTarRecord(t *testing.T, w *tar.Writer, fn, contents string) {
err := w.WriteHeader(&tar.Header{
Name: fn,
Expand Down
57 changes: 57 additions & 0 deletions integration/build/testdata/Dockerfile.testBuildPreserveOwnership
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
# Set up files and directories with known ownership
FROM busybox AS source
RUN touch /file && chown 100:200 /file \
&& mkdir -p /dir/subdir \
&& touch /dir/subdir/nestedfile \
&& chown 100:200 /dir \
&& chown 101:201 /dir/subdir \
&& chown 102:202 /dir/subdir/nestedfile

FROM busybox AS test_base
RUN mkdir -p /existingdir/existingsubdir \
&& touch /existingdir/existingfile \
&& chown 500:600 /existingdir \
&& chown 501:601 /existingdir/existingsubdir \
&& chown 501:601 /existingdir/existingfile


# Copy files from the source stage
FROM test_base AS copy_from
COPY --from=source /file .
# Copy to a non-existing target directory creates the target directory (as root), then copies the _contents_ of the source directory into it
COPY --from=source /dir /dir
# Copying to an existing target directory will copy the _contents_ of the source directory into it
COPY --from=source /dir/. /existingdir

RUN e="100:200"; p="/file" ; a=`stat -c "%u:%g" "$p"`; if [ "$a" != "$e" ]; then echo "incorrect ownership on $p. expected $e, got $a"; exit 1; fi \
&& e="0:0"; p="/dir" ; a=`stat -c "%u:%g" "$p"`; if [ "$a" != "$e" ]; then echo "incorrect ownership on $p. expected $e, got $a"; exit 1; fi \
&& e="101:201"; p="/dir/subdir" ; a=`stat -c "%u:%g" "$p"`; if [ "$a" != "$e" ]; then echo "incorrect ownership on $p. expected $e, got $a"; exit 1; fi \
&& e="102:202"; p="/dir/subdir/nestedfile" ; a=`stat -c "%u:%g" "$p"`; if [ "$a" != "$e" ]; then echo "incorrect ownership on $p. expected $e, got $a"; exit 1; fi \
# Existing files and directories ownership should not be modified
&& e="500:600"; p="/existingdir" ; a=`stat -c "%u:%g" "$p"`; if [ "$a" != "$e" ]; then echo "incorrect ownership on $p. expected $e, got $a"; exit 1; fi \
&& e="501:601"; p="/existingdir/existingsubdir" ; a=`stat -c "%u:%g" "$p"`; if [ "$a" != "$e" ]; then echo "incorrect ownership on $p. expected $e, got $a"; exit 1; fi \
&& e="501:601"; p="/existingdir/existingfile" ; a=`stat -c "%u:%g" "$p"`; if [ "$a" != "$e" ]; then echo "incorrect ownership on $p. expected $e, got $a"; exit 1; fi \
# But new files and directories should maintain their ownership
&& e="101:201"; p="/existingdir/subdir" ; a=`stat -c "%u:%g" "$p"`; if [ "$a" != "$e" ]; then echo "incorrect ownership on $p. expected $e, got $a"; exit 1; fi \
&& e="102:202"; p="/existingdir/subdir/nestedfile"; a=`stat -c "%u:%g" "$p"`; if [ "$a" != "$e" ]; then echo "incorrect ownership on $p. expected $e, got $a"; exit 1; fi


# Copy files from the source stage and chown them.
FROM test_base AS copy_from_chowned
COPY --from=source --chown=300:400 /file .
# Copy to a non-existing target directory creates the target directory (as root), then copies the _contents_ of the source directory into it
COPY --from=source --chown=300:400 /dir /dir
# Copying to an existing target directory copies the _contents_ of the source directory into it
COPY --from=source --chown=300:400 /dir/. /existingdir

RUN e="300:400"; p="/file" ; a=`stat -c "%u:%g" "$p"`; if [ "$a" != "$e" ]; then echo "incorrect ownership on $p. expected $e, got $a"; exit 1; fi \
&& e="300:400"; p="/dir" ; a=`stat -c "%u:%g" "$p"`; if [ "$a" != "$e" ]; then echo "incorrect ownership on $p. expected $e, got $a"; exit 1; fi \
&& e="300:400"; p="/dir/subdir" ; a=`stat -c "%u:%g" "$p"`; if [ "$a" != "$e" ]; then echo "incorrect ownership on $p. expected $e, got $a"; exit 1; fi \
&& e="300:400"; p="/dir/subdir/nestedfile" ; a=`stat -c "%u:%g" "$p"`; if [ "$a" != "$e" ]; then echo "incorrect ownership on $p. expected $e, got $a"; exit 1; fi \
# Existing files and directories ownership should not be modified
&& e="500:600"; p="/existingdir" ; a=`stat -c "%u:%g" "$p"`; if [ "$a" != "$e" ]; then echo "incorrect ownership on $p. expected $e, got $a"; exit 1; fi \
&& e="501:601"; p="/existingdir/existingsubdir" ; a=`stat -c "%u:%g" "$p"`; if [ "$a" != "$e" ]; then echo "incorrect ownership on $p. expected $e, got $a"; exit 1; fi \
&& e="501:601"; p="/existingdir/existingfile" ; a=`stat -c "%u:%g" "$p"`; if [ "$a" != "$e" ]; then echo "incorrect ownership on $p. expected $e, got $a"; exit 1; fi \
# But new files and directories should be chowned
&& e="300:400"; p="/existingdir/subdir" ; a=`stat -c "%u:%g" "$p"`; if [ "$a" != "$e" ]; then echo "incorrect ownership on $p. expected $e, got $a"; exit 1; fi \
&& e="300:400"; p="/existingdir/subdir/nestedfile"; a=`stat -c "%u:%g" "$p"`; if [ "$a" != "$e" ]; then echo "incorrect ownership on $p. expected $e, got $a"; exit 1; fi