Repository: uber/makisu (go)

Issue:
Recover root fs after build when using makisu-alpine
There are users trying to use makisu-alpine with Jenkins k8s plugin:

> I’m facing another issue. The Jenkins k8s plugin requires the containers to be available throughout the pipeline, so I’m using the makisu-alpine image, with tty: true, and cat as the command, so the container remains up. After that the plugin routinely runs touch to check all containers. However once I use the makisu binary to build the docker image, the k8s plugin can no longer execute any command within the makisu container (`sh: touch: not found`). Does building an image deletes all binaries or alters $PATH in the makisu container? If so, is there a workaround?

One solution is to recover root fs at the end of the build.

Pull request description:
Preserve root directory before build run and restore it afterward
It might fix #213 

Patch:
diff --git a/bin/makisu/cmd/build.go b/bin/makisu/cmd/build.go
index b65a94f..4595fe2 100644
--- a/bin/makisu/cmd/build.go
+++ b/bin/makisu/cmd/build.go
@@ -64,6 +64,8 @@ type buildCmd struct {
 
 	storageDir       string
 	compressionLevel string
+
+	preserveRoot bool
 }
 
 func getBuildCmd() *buildCmd {
@@ -119,6 +121,8 @@ func getBuildCmd() *buildCmd {
 	buildCmd.PersistentFlags().StringVar(&buildCmd.storageDir, "storage", "", "Directory that makisu uses for temp files and cached layers. Mount this path for better caching performance. If modifyfs is set, default to /makisu-storage; Otherwise default to /tmp/makisu-storage")
 	buildCmd.PersistentFlags().StringVar(&buildCmd.compressionLevel, "compression", "default", "Image compression level, could be 'no', 'speed', 'size', 'default'")
 
+	buildCmd.PersistentFlags().BoolVar(&buildCmd.preserveRoot, "preserve-root", false, "Copy / in the storage dir and copy it back after build.")
+
 	buildCmd.MarkFlagRequired("tag")
 	buildCmd.Flags().SortFlags = false
 	buildCmd.PersistentFlags().SortFlags = false
@@ -232,6 +236,14 @@ func (cmd *buildCmd) Build(contextDir string) error {
 	// Optionally remove everything before and after build.
 	defer storage.CleanupSandbox(cmd.storageDir)
 	if cmd.allowModifyFS {
+		if cmd.preserveRoot {
+			rootPreserver, err := storage.NewRootPreserver("/", cmd.storageDir, pathutils.DefaultBlacklist)
+			if err != nil {
+				return fmt.Errorf("failed to preserve root: %s", err)
+			}
+			defer rootPreserver.RestoreRoot()
+		}
+		log.Debugf("build.Cmd.Build() first call")
 		buildContext.MemFS.Remove()
 		defer buildContext.MemFS.Remove()
 	}
diff --git a/lib/fileio/copy.go b/lib/fileio/copy.go
index 79ab0b1..5e0a6e2 100644
--- a/lib/fileio/copy.go
+++ b/lib/fileio/copy.go
@@ -33,6 +33,8 @@ import (
 type Copier interface {
 	CopyFile(source, target string, uid, gid int) error
 	CopyDir(source, target string, uid, gid int) error
+	CopyDirPreserveOwner(source, target string) error
+	CopyFilePreserveOwner(source, target string) error
 }
 
 type copier struct {
@@ -60,13 +62,24 @@ func NewInternalCopier() Copier {
 // the given uid/gid.
 // For symlinks, the link target will be copied as-is.
 func (c copier) CopyFile(source, target string, uid, gid int) error {
+	// Make target parent directories (uid and gid will be computed from the sources one).
+	targetDir := filepath.Dir(target)
+	if err := mkdirAll(targetDir, os.ModePerm, uid, gid, false); err != nil {
+		return fmt.Errorf("mkdir all %s: %s", targetDir, err)
+	}
+	// Copy file permissions and contents.
+	return c.copyFile(source, target, uid, gid, false)
+}
+
+// CopyFilePreserveOwner follow the behavior of CopyFile but preserve file rights.
+func (c copier) CopyFilePreserveOwner(source, target string) error {
 	// Make target parent directories with passed uid & gid if they don't exist.
 	targetDir := filepath.Dir(target)
-	if err := mkdirAll(targetDir, os.ModePerm, uid, gid); err != nil {
+	if err := mkdirAll(targetDir, os.ModePerm, 0, 0, true); err != nil {
 		return fmt.Errorf("mkdir all %s: %s", targetDir, err)
 	}
 	// Copy file permissions and contents.
-	return c.copyFile(source, target, uid, gid)
+	return c.copyFile(source, target, 0, 0, true)
 }
 
 // CopyDir recursively copies the directory at source to target. The source
@@ -88,11 +101,26 @@ func (c copier) CopyDir(source, target string, uid, gid int) error {
 		return nil
 	}
 	// Make target parent directories with passed uid & gid if they don't exist.
-	if err := mkdirAll(target, os.ModePerm, uid, gid); err != nil {
+	if err := mkdirAll(target, os.ModePerm, uid, gid, false); err != nil {
+		return fmt.Errorf("mkdir all %s: %s", target, err)
+	}
+	// Recursively copy directories and files.
+	return c.copyDirContents(source, target, target, uid, gid, false)
+}
+
+// CopyDirPreserveOwner follow the behavior of CopyFile but preserve file rights.
+func (c copier) CopyDirPreserveOwner(source, target string) error {
+	if c.isBlacklisted(source) {
+		// Ignore this directory since it's blacklisted.
+		log.Infof("* Ignoring copy of directory %s because it is blacklisted", source)
+		return nil
+	}
+	// Make target parent directories (uid and gid will be computed from the sources one).
+	if err := mkdirAll(target, os.ModePerm, 0, 0, true); err != nil {
 		return fmt.Errorf("mkdir all %s: %s", target, err)
 	}
 	// Recursively copy directories and files.
-	return c.copyDirContents(source, target, target, uid, gid)
+	return c.copyDirContents(source, target, target, 0, 0, true)
 }
 
 func (c copier) isBlacklisted(source string) bool {
@@ -100,7 +128,7 @@ func (c copier) isBlacklisted(source string) bool {
 }
 
 // copyFile copies the permissions and contents of the file at src to dst.
-func (c copier) copyFile(src, dst string, uid, gid int) error {
+func (c copier) copyFile(src, dst string, uid, gid int, preserveOwner bool) error {
 	fi, err := os.Lstat(src)
 	if err != nil {
 		return fmt.Errorf("lstat %s: %s", src, err)
@@ -127,6 +155,10 @@ func (c copier) copyFile(src, dst string, uid, gid int) error {
 		}
 	}
 
+	if preserveOwner {
+		uid, gid = fileOwners(fi)
+	}
+
 	// Handle regular files.
 	return c.copyRegularFile(fi, src, dst, uid, gid)
 }
@@ -183,7 +215,7 @@ func (c copier) copySymlink(src, dst string) error {
 }
 
 // copyDirContents recursively copies the contents of directory src to dst. Both must exist.
-func (c copier) copyDirContents(src, dst, origDst string, uid, gid int) error {
+func (c copier) copyDirContents(src, dst, origDst string, uid, gid int, preserveOwner bool) error {
 	entries, err := ioutil.ReadDir(src)
 	if err != nil {
 		return fmt.Errorf("read dir %s: %s", src, err)
@@ -200,14 +232,14 @@ func (c copier) copyDirContents(src, dst, origDst string, uid, gid int) error {
 		}
 		currDst := filepath.Join(dst, entry.Name())
 		if entry.IsDir() {
-			if err := c.copyDir(currSrc, currDst, uid, gid); err != nil {
+			if err := c.copyDir(currSrc, currDst, uid, gid, preserveOwner); err != nil {
 				return fmt.Errorf("copy dir %s to %s: %s", currSrc, currDst, err)
 			}
-			if err := c.copyDirContents(currSrc, currDst, origDst, uid, gid); err != nil {
+			if err := c.copyDirContents(currSrc, currDst, origDst, uid, gid, preserveOwner); err != nil {
 				return fmt.Errorf("copy dir contents %s to %s: %s", currSrc, currDst, err)
 			}
 		} else {
-			if err := c.copyFile(currSrc, currDst, uid, gid); err != nil {
+			if err := c.copyFile(currSrc, currDst, uid, gid, preserveOwner); err != nil {
 				return fmt.Errorf("copy file %s to %s: %s", currSrc, currDst, err)
 			}
 		}
@@ -216,7 +248,7 @@ func (c copier) copyDirContents(src, dst, origDst string, uid, gid int) error {
 }
 
 // copyDir copies the directory at src to dst.
-func (c copier) copyDir(src, dst string, uid, gid int) error {
+func (c copier) copyDir(src, dst string, uid, gid int, preserveOwner bool) error {
 	srcInfo, err := os.Lstat(src)
 	if err != nil {
 		return fmt.Errorf("lstat %s: %s", src, err)
@@ -243,6 +275,9 @@ func (c copier) copyDir(src, dst string, uid, gid int) error {
 	// Change owner and mode of dst to that of src.
 	// Note: Chmod needs to be called after chown, otherwise setuid and setgid
 	// bits could be unset.
+	if preserveOwner {
+		uid, gid = fileOwners(srcInfo)
+	}
 	if err := os.Chown(dst, uid, gid); err != nil {
 		return fmt.Errorf("chown %s: %s", dst, err)
 	}
@@ -254,7 +289,7 @@ func (c copier) copyDir(src, dst string, uid, gid int) error {
 
 // mkdirAll performs the same operation as os.MkdirAll, but also sets the given
 // permissions & owners on all created directories.
-func mkdirAll(dst string, mode os.FileMode, uid, gid int) error {
+func mkdirAll(dst string, mode os.FileMode, uid, gid int, preserveOwner bool) error {
 	if dst == "" {
 		return errors.New("empty target directory")
 	}
@@ -269,12 +304,15 @@ func mkdirAll(dst string, mode os.FileMode, uid, gid int) error {
 	var prevDir string
 	for _, dir := range split {
 		absDir := filepath.Join(prevDir, dir)
-		if _, err := os.Lstat(absDir); err != nil {
+		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 {
 				return fmt.Errorf("mkdir %s: %s", absDir, err)
 			}
+			if preserveOwner {
+				uid, gid = fileOwners(fi)
+			}
 			if err := os.Chown(absDir, uid, gid); err != nil {
 				return fmt.Errorf("chown %s: %s", absDir, err)
 			}
diff --git a/lib/registry/security/basicauth.go b/lib/registry/security/basicauth.go
index ddc402b..9707f17 100644
--- a/lib/registry/security/basicauth.go
+++ b/lib/registry/security/basicauth.go
@@ -45,7 +45,8 @@ func BasicAuthTransport(addr, repo string, tr http.RoundTripper, authConfig type
 	// This looks weird but when using AWS ECR (especially the docker ecr helper) we get a Username and a Password
 	// Then, the ping will create a challenge by parsing the www-authenticate header from the ECR server (it will return a "Basic ...")
 	// So if we use the `NewTokenHandlerWithOptions` we will always fail the Scheme checking in vendor/github.com/docker/distribution/registry/client/auth/session.go#L98 ("basic" != "bearer")
-	if authConfig.Username != "" && authConfig.Password != "" {
+	// We also have a special case for index.docker.io because it does not support Basic Auth
+	if authConfig.Username != "" && authConfig.Password != "" && addr != "index.docker.io" {
 		return transport.NewTransport(tr, auth.NewAuthorizer(cm, auth.NewBasicHandler(defaultCredStore{authConfig}))), nil
 	} else {
 		return transport.NewTransport(tr, auth.NewAuthorizer(cm, auth.NewTokenHandlerWithOptions(auth.TokenHandlerOptions{
diff --git a/lib/storage/root_preserver.go b/lib/storage/root_preserver.go
new file mode 100644
index 0000000..8e5a467
--- /dev/null
+++ b/lib/storage/root_preserver.go
@@ -0,0 +1,82 @@
+//  Copyright (c) 2018 Uber Technologies, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package storage
+
+import (
+	"fmt"
+	"os"
+	"path"
+
+	"github.com/uber/makisu/lib/fileio"
+	"github.com/uber/makisu/lib/log"
+)
+
+const rootPreserverBackupDir = "initial_root"
+
+// RootPreserver contains the locations of:
+//  - the "old" root, the one before running makisu that will get removed by `MemFS.Remove()` at the end of the build command
+//  - the "saved" root, a dir containing a copy of everyfile of the "old" root
+type RootPreserver struct {
+	InitialRootDir string
+	SavedRootDir   string
+	blacklist      []string
+}
+
+// NewRootPreserver creates a new RootPreserver.
+func NewRootPreserver(rootDir, storageDir string, blacklist []string) (*RootPreserver, error) {
+	backupDir := path.Join(storageDir, rootPreserverBackupDir)
+
+	if err := copyOldRootToBackup(rootDir, backupDir, blacklist); err != nil {
+		return nil, fmt.Errorf("root preserver failed copying: %s", err)
+	}
+
+	return &RootPreserver{
+		InitialRootDir: rootDir,
+		SavedRootDir:   backupDir,
+	}, nil
+}
+
+func copyOldRootToBackup(rootDir, backupDir string, blacklist []string) error {
+	copier := fileio.NewCopier(blacklist)
+
+	// Remove and recreate backup dir.
+	os.RemoveAll(backupDir)
+	if err := os.MkdirAll(backupDir, 0755); err != nil {
+		log.Fatalf("Failed to create layer download dir %s: %s", backupDir, err)
+	}
+
+	// TODO: Handle uid, gid preservation
+	if err := copier.CopyDirPreserveOwner(rootDir, backupDir); err != nil {
+		return fmt.Errorf("copy dir %s to dir %s: %s", rootDir, backupDir, err)
+	}
+
+	return nil
+}
+
+// RestoreRoot will copy the backupDir in the initial root dir
+func (r *RootPreserver) RestoreRoot() error {
+	copier := fileio.NewCopier(r.blacklist)
+
+	// TODO: Handle uid, gid preservation
+	if err := copier.CopyDirPreserveOwner(r.SavedRootDir, r.InitialRootDir); err != nil {
+		return fmt.Errorf("copy dir %s to dir %s: %s", r.SavedRootDir, r.InitialRootDir, err)
+	}
+
+	if err := os.RemoveAll(r.SavedRootDir); err != nil {
+		return fmt.Errorf("remove saved root dir %s: %s", r.SavedRootDir, err)
+	}
+
+	return nil
+}


Test patch:
diff --git a/lib/storage/root_preserver_test.go b/lib/storage/root_preserver_test.go
new file mode 100644
index 0000000..d6667a9
--- /dev/null
+++ b/lib/storage/root_preserver_test.go
@@ -0,0 +1,88 @@
+//  Copyright (c) 2018 Uber Technologies, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package storage
+
+import (
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestRootPreserver(t *testing.T) {
+	require := require.New(t)
+
+	// This is /
+	tmpRoot, err := ioutil.TempDir("/tmp", "makisu-test")
+	require.NoError(err)
+	// defer os.RemoveAll(tmpRoot)
+
+	// This is the storage directory
+	storageDir, err := ioutil.TempDir(tmpRoot, "storage")
+	require.NoError(err)
+
+	initialRootBackup := filepath.Join(storageDir, rootPreserverBackupDir)
+
+	_hello := []byte("hello")
+	shouldBeCopied := "/test.txt"
+	shouldNotBeCopied := "/test2.txt"
+	require.NoError(ioutil.WriteFile(filepath.Join(tmpRoot, shouldBeCopied), _hello, os.ModePerm))
+	require.NoError(ioutil.WriteFile(filepath.Join(tmpRoot, shouldNotBeCopied), _hello, os.ModePerm))
+
+	// Before root preserver:
+	// /tmp/makisu-test362631474/
+	// /tmp/makisu-test362631474/test2.txt
+	// /tmp/makisu-test362631474/storage054318825
+	// /tmp/makisu-test362631474/test.txt
+
+	rootPreserver, err := NewRootPreserver(tmpRoot, storageDir, []string{storageDir, filepath.Join(tmpRoot, shouldNotBeCopied)})
+	require.NoError(err)
+
+	// Now we want:
+	// /tmp/makisu-test362631474/
+	// /tmp/makisu-test362631474/test2.txt
+	// /tmp/makisu-test362631474/storage054318825
+	// /tmp/makisu-test362631474/storage054318825/test.txt
+	// /tmp/makisu-test362631474/test.txt
+
+	b, err := ioutil.ReadFile(filepath.Join(initialRootBackup, shouldBeCopied))
+	require.NoError(err)
+	require.Equal(_hello, b)
+
+	b, err = ioutil.ReadFile(filepath.Join(initialRootBackup, shouldNotBeCopied))
+	require.Error(err)
+
+	// Remove initial file that would have been copied
+	err = os.Remove(filepath.Join(tmpRoot, shouldBeCopied))
+	require.NoError(err)
+
+	err = rootPreserver.RestoreRoot()
+	require.NoError(err)
+
+	b, err = ioutil.ReadFile(filepath.Join(tmpRoot, shouldBeCopied))
+	require.NoError(err)
+	require.Equal(_hello, b)
+
+	b, err = ioutil.ReadFile(filepath.Join(tmpRoot, shouldNotBeCopied))
+	require.NoError(err)
+	require.Equal(_hello, b)
+
+	// Check that the backup dir is deleted
+	if _, err := os.Stat(filepath.Join(storageDir, rootPreserverBackupDir)); !os.IsNotExist(err) {
+		require.Failf("Storage dir: %s should not exists", storageDir)
+	}
+}
diff --git a/test/python/test_build.py b/test/python/test_build.py
index f790293..9765402 100644
--- a/test/python/test_build.py
+++ b/test/python/test_build.py
@@ -237,3 +237,26 @@ def test_build_go_with_debian_package(registry1, storage_dir):
     assert l2.get_tar_header_count() == 1, [h.name for h in l2.get_tar_headers()]
     assert list(l2.get_tar_headers())[0].uname != "root"
     assert list(l2.get_tar_headers())[0].gname != "root"
+
+# A little bit of crazyness on this test but I didn't want to add more complex methods
+# to test the `--preserve-root` flag.
+# See the [Dockerfile](../testdata/build-context/preserve-root/Dockerfile)
+def test_build_with_preserve_root(registry1, storage_dir):
+    new_image = new_image_name()
+    context_dir = os.path.join(
+        os.getcwd(), 'testdata/build-context/preserve-root')
+
+    os.environ["MAKISU_ALPINE"] = "1"
+
+    utils.docker_push_image(utils.get_base_image(), registry1.addr)
+
+    docker_build_args = [
+        "BASE_IMAGE={}/{}".format(registry1.addr, utils.get_base_image()),
+    ]
+    utils.makisu_build_image(
+        new_image, context_dir, storage_dir, registry=registry1.addr, docker_args=docker_build_args, load=True)
+    code, err = utils.docker_run_image(registry1.addr, new_image)
+
+    del os.environ["MAKISU_ALPINE"]
+
+    assert code == 0, err
\ No newline at end of file
diff --git a/testdata/build-context/preserve-root/Dockerfile b/testdata/build-context/preserve-root/Dockerfile
new file mode 100644
index 0000000..e8c5235
--- /dev/null
+++ b/testdata/build-context/preserve-root/Dockerfile
@@ -0,0 +1,18 @@
+ARG BASE_IMAGE
+
+FROM $BASE_IMAGE
+
+# Create a file and dir owned by daemon user
+RUN touch /daemon_file && \
+    chown daemon:daemon /daemon_file && \
+    mkdir /daemon_dir && \
+    chown daemon:daemon /daemon_dir && \
+    touch /daemon_dir/root_file && \
+    mkdir /data && \
+    echo 'FROM alpine' > /data/Dockerfile
+
+ENTRYPOINT [ \
+  "/bin/sh", \
+  "-c", \
+  "cd /data && /makisu-internal/makisu --modifyfs=true build -t test-preserve-root --preserve-root . && test $(stat -c '%U:%G' /daemon_file) = 'daemon:daemon' && test $(stat -c '%U:%G' /daemon_dir) = 'daemon:daemon' && test $(stat -c '%U:%G' /daemon_dir/root_file) = 'root:root'" \
+  ]
\ No newline at end of file