diff --git a/lib/fileio/copy.go b/lib/fileio/copy.go index b5725709..a524b7e2 100644 --- a/lib/fileio/copy.go +++ b/lib/fileio/copy.go @@ -100,10 +100,12 @@ func (c copier) CopyDir(source, target string, uid, gid int) error { log.Infof("* Ignoring copy of directory %s because it is blacklisted", source) return nil } + // Make target parent directories with passed uid & gid if they don't exist. if err := mkdirAll(target, os.ModePerm, uid, gid, false); err != nil { return fmt.Errorf("mkdir all %s: %s", target, err) } + // Recursively copy contents of source directory. return c.copyDirContents(source, target, target, uid, gid, false) } @@ -115,25 +117,32 @@ func (c copier) CopyDirPreserveOwner(source, target string) error { log.Infof("* Ignoring copy of directory %s because it is blacklisted", source) return nil } - // Copy the parent directory of target. - targetParentDir := filepath.Dir(target) - if err := mkdirAll(targetParentDir, os.ModePerm, 0, 0, true); err != nil { - return fmt.Errorf("mkdir parent dir %s: %s", targetParentDir, err) - } - // If the source dir is a symlink, makisu would try to copy the contents rather than a link. + + // Use stat instead of Lstat here. + // If the source is a symlink, makisu would try to copy the contents rather than the link. fi, _ := os.Stat(source) sourceUid, sourceGid := fileOwners(fi) if _, err := os.Lstat(target); err != nil { if !os.IsNotExist(err) { return fmt.Errorf("stat %s: %s", target, err) - } else if err := os.Mkdir(target, os.ModePerm); err != nil { + } + + // Copy the parent directory of target. + targetParentDir := filepath.Dir(target) + if err := mkdirAll(targetParentDir, os.ModePerm, 0, 0, true); err != nil { + return fmt.Errorf("mkdir parent dir %s: %s", targetParentDir, err) + } + + // Preserve the source dir permission and ownership. + if err := os.Mkdir(target, fi.Mode()); err != nil { return fmt.Errorf("mkdir %s: %s", target, err) } + + if err := os.Chown(target, sourceUid, sourceGid); err != nil { + return fmt.Errorf("chown %s: %s", target, err) + } } - // Preserve the source dir ownership. - if err := os.Chown(target, sourceUid, sourceGid); err != nil { - return fmt.Errorf("chown %s: %s", target, err) - } + // Recursively copy contents of source directory. return c.copyDirContents(source, target, target, 0, 0, true) } @@ -321,11 +330,12 @@ func mkdirAll(dst string, mode os.FileMode, uid, gid int, preserveParentOwner bo if fi, err := os.Lstat(absDir); err != nil { if !os.IsNotExist(err) { return fmt.Errorf("stat %s: %s", absDir, err) - } else if err := os.Mkdir(absDir, mode); err != nil { + } + if err := os.Mkdir(absDir, mode); err != nil { return fmt.Errorf("mkdir %s: %s", absDir, err) } - // Update file info + // Update file info. fi, _ = os.Lstat(prevDir) if preserveParentOwner { uid, gid = fileOwners(fi) diff --git a/lib/snapshot/copy_op.go b/lib/snapshot/copy_op.go index 06aef3f3..90682d03 100644 --- a/lib/snapshot/copy_op.go +++ b/lib/snapshot/copy_op.go @@ -115,7 +115,6 @@ func (c *CopyOperation) Execute() error { return fmt.Errorf("copy file %s to dir %s: %s", src, targetFilePath, err) } } - } else { // File to file if c.preserveOwner { diff --git a/test/python/test_build.py b/test/python/test_build.py index 014e7dd8..cc3e4c10 100644 --- a/test/python/test_build.py +++ b/test/python/test_build.py @@ -76,16 +76,6 @@ def test_build_copy_add_chown(registry1, storage_dir): assert code == 0, err -def test_build_copy_archive(registry1, storage_dir): - new_image = utils.new_image_name() - context_dir = os.path.join( - os.getcwd(), 'testdata/build-context/copy-archive') - utils.makisu_build_image( - new_image, context_dir, storage_dir, registry=registry1.addr) - code, err = utils.docker_run_image(registry1.addr, new_image) - assert code == 0, err - - def test_build_arg_and_env(registry1, storage_dir): new_image = utils.new_image_name() context_dir = os.path.join( diff --git a/testdata/build-context/copy-archive/Dockerfile b/testdata/build-context/copy-archive/Dockerfile deleted file mode 100644 index 27c35b5d..00000000 --- a/testdata/build-context/copy-archive/Dockerfile +++ /dev/null @@ -1,11 +0,0 @@ -FROM alpine as phaseA -RUN mkdir /hello -RUN mkdir /hello/world -RUN touch /hello/world/a.txt -RUN chmod 775 /hello/world/a.txt -RUN chown daemon:daemon /hello/world - -FROM alpine -RUN mkdir /hello -COPY --from=phaseA --archive /hello /hello -ENTRYPOINT ["/bin/sh", "-c", "test $(stat -c '%U:%G' /hello) = 'root:root' && test $(stat -c '%U:%G' /hello/world) = 'daemon:daemon' && test $(stat -c '%U:%G' /hello/world/a.txt) = 'root:root' && test $(stat -c '%a' /hello/world/a.txt) = 775 "] diff --git a/testdata/build-context/copy-from/Dockerfile b/testdata/build-context/copy-from/Dockerfile index b27a52fc..d708326e 100644 --- a/testdata/build-context/copy-from/Dockerfile +++ b/testdata/build-context/copy-from/Dockerfile @@ -18,7 +18,6 @@ RUN touch /mine/b.txt #!COMMIT RUN mkdir /mine/c RUN touch /mine/c/1.txt RUN touch /mine/c/2.txt -RUN touch /mine/c/3.txt FROM alpine RUN mkdir /mine @@ -26,17 +25,22 @@ RUN touch /mine/a.txt RUN touch /mine/b.txt #!COMMIT # Copy files that were not cached from previous stage. COPY --from=phaseA /mine/c/ /mine/c/ -RUN cat /mine/c/1.txt && cat /mine/c/2.txt && cat /mine/c/3.txt +RUN cat /mine/c/1.txt && cat /mine/c/2.txt RUN rm -rf /mine/c/ #!COMMIT -FROM alpine +FROM alpine AS phaseB RUN mkdir /mine # Copy files again that were deleted from previous stage. COPY --from=phaseA /mine/c/ /mine/c/ -RUN cat /mine/c/1.txt && cat /mine/c/2.txt && cat /mine/c/3.txt +RUN cat /mine/c/1.txt && cat /mine/c/2.txt +# Copy to existing dir with changed permission. +RUN mkdir /mine/d && chown daemon:daemon /mine/d && chmod 777 /mine/d +COPY --from=phaseA --archive /mine/c/ /mine/d/ +RUN mkdir /mine/e && chown daemon:daemon /mine/e +COPY --from=phaseA /mine/c/ /mine/e/ FROM alpine RUN mkdir /mine # Copy root. -COPY --from=phaseA / / -ENTRYPOINT ["/bin/sh", "-c", "cat /mine/a.txt && cat /mine/b.txt && cat /mine/c/1.txt && cat /mine/c/2.txt && cat /mine/c/3.txt"] +COPY --from=phaseB --archive / / +ENTRYPOINT ["/bin/sh", "-c", "cat /mine/c/1.txt && cat /mine/c/2.txt && cat /mine/d/1.txt && cat /mine/d/2.txt && test $(stat -c '%U:%G' /mine/d) = 'daemon:daemon' && test $(stat -c '%a' /mine/d) = 777 && test $(stat -c '%U:%G' /mine/e) = 'daemon:daemon'"]