Fix COPY target dir owner and permission if target dir already exists (…

…#318)

* Fix COPY permission when target dir already exists

* Update integration test

* Fix test

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)

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 {

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(

testdata/build-context/copy-from/Dockerfile

@@ -18,25 +18,29 @@ 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
 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'"]