Repository: uber/makisu
Pull request #210 (2019-04-19 10:11:16)

Handle USER in the RUN command
With this PR, after a USER step is done, any subsequent RUN command will use the "saved" USER.

CLAassistant: [![CLA assistant check](https://cla-assistant.io/pull/badge/not_signed)](https://cla-assistant.io/uber/makisu?pullRequest=210) <br/>Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our [Contributor License Agreement](https://cla-assistant.io/uber/makisu?pullRequest=210) before we can accept your contribution.<br/><hr/>**Côme Tresarrieu** seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please [add the email address used for this commit to your account](https://help.github.com/articles/why-are-my-commits-linked-to-the-wrong-user/#commits-are-not-linked-to-any-user).<br/><sub>You have signed the CLA already but the status is still pending? Let us [recheck](https://cla-assistant.io/check/uber/makisu?pullRequest=210) it.</sub>
yiranwang52: Instead of adding one more variable, `USER` was already persisted in previous step's image config. Just need to make use of it in `ApplyCtxAndConfig`
Rowern: Thanks for the feedback! I did that, I'm waiting for this to be reviewed and merged to flesh more work on the first PR (handling chown). I saw some other case where USER is not presisted like docker does (a COPY step after a USER change should set the right of the copied file to the user).
Rowern: I see that tests are failing, I'm looking at why 😄
Rowern: I forgot that by overriding `ApplyCtxAndConfig` the code setting up the workingdir would fail.

@yiranwang52 I'm not sure of the impact of not having the "Set env from previous stage" not being called before the run step. I think it would not be too impactful but I'm not sure...
Rowern: I do have my answer with a simple Dockerfile test:

```Dockerfile
FROM alpine

ARG HELLO=1

RUN env
```

If we do not run the set env code, `env` does not contain `HELLO` (but that's logic) so I'm also exporting the code to a method in base_step

Patch:
diff --git a/.travis.yml b/.travis.yml
index 5cc97af..bd17546 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -5,8 +5,5 @@ language: python
 services:
     - docker
 
-install:
-    - for i in $(seq 1 5); do (make vendor && break) || sleep 30; done
-
 script:
     - make cunit-test integration
diff --git a/Makefile b/Makefile
index efe37ef..ebefdac 100644
--- a/Makefile
+++ b/Makefile
@@ -41,12 +41,12 @@ REGISTRY ?= gcr.io/makisu-project
 bins: bin/makisu/makisu
 
 bin/makisu/makisu: $(ALL_SRC) vendor
-	go build -tags bins $(GO_FLAGS) -o $@ bin/makisu/*.go
+	go build -mod=vendor -tags bins $(GO_FLAGS) -o $@ bin/makisu/*.go
 
 lbins: bin/makisu/makisu.linux
 
 bin/makisu/makisu.linux: $(ALL_SRC) vendor
-	CGO_ENABLED=0 GOOS=linux go build -tags bins $(GO_FLAGS) -o $@ bin/makisu/*.go
+	CGO_ENABLED=0 GOOS=linux go build -mod=vendor -tags bins $(GO_FLAGS) -o $@ bin/makisu/*.go
 
 cbins:
 	docker run -i --rm -v $(PWD):/workspace/$(PACKAGE_NAME) \
diff --git a/bin/makisu/cmd/build.go b/bin/makisu/cmd/build.go
index 61e9ba0..b65a94f 100644
--- a/bin/makisu/cmd/build.go
+++ b/bin/makisu/cmd/build.go
@@ -69,9 +69,9 @@ type buildCmd struct {
 func getBuildCmd() *buildCmd {
 	buildCmd := &buildCmd{
 		Command: &cobra.Command{
-			Use: "build -t=<image_tag> [flags] <context_path>",
+			Use:                   "build -t=<image_tag> [flags] <context_path>",
 			DisableFlagsInUseLine: true,
-			Short: "Build docker image, optionally push to registries and/or load into docker daemon",
+			Short:                 "Build docker image, optionally push to registries and/or load into docker daemon",
 		},
 	}
 	buildCmd.Args = func(cmd *cobra.Command, args []string) error {
diff --git a/lib/builder/step/add_copy_step.go b/lib/builder/step/add_copy_step.go
index 3f60a91..ea062c6 100644
--- a/lib/builder/step/add_copy_step.go
+++ b/lib/builder/step/add_copy_step.go
@@ -17,7 +17,7 @@ package step
 import (
 	"fmt"
 	"hash/crc32"
-	"io/ioutil"
+	"io"
 	"math/rand"
 	"os"
 	"path/filepath"
@@ -104,15 +104,18 @@ func (s *addCopyStep) SetCacheID(ctx *context.BuildContext, seed string) error {
 		s.cacheID = fmt.Sprintf("%x", b)
 	} else {
 		// Initialize the checksum with the seed, directive and args.
-		checksum := crc32.ChecksumIEEE([]byte(seed + string(s.directive) + s.args))
+		checksum := crc32.NewIEEE()
+		_, err := checksum.Write([]byte(seed + string(s.directive) + s.args))
+		if err != nil {
+			return fmt.Errorf("hash copy directive: %s", err)
+		}
 
 		// If the step args and the contents of sources are identical,
 		// we should be able to use the cache from the previous build.
-		checksum, err := s.updateContextChecksum(ctx, checksum)
-		if err != nil {
+		if err := s.updateContextChecksum(ctx, checksum); err != nil {
 			return fmt.Errorf("hash context sources: %s", err)
 		}
-		s.cacheID = fmt.Sprintf("%x", checksum)
+		s.cacheID = fmt.Sprintf("%x", checksum.Sum32())
 	}
 	return nil
 }
@@ -145,9 +148,9 @@ func (s *addCopyStep) Execute(ctx *context.BuildContext, modifyFS bool) (err err
 }
 
 // Updates the checksum passed in with the data stored in the context on the filesystem.
-func (s *addCopyStep) updateContextChecksum(ctx *context.BuildContext, checksum uint32) (uint32, error) {
+func (s *addCopyStep) updateContextChecksum(ctx *context.BuildContext, checksum io.Writer) error {
 	if s.fromStage != "" {
-		return 0, fmt.Errorf("Not supported: the copy step has from stage flag")
+		return fmt.Errorf("not supported: the copy step has from stage flag")
 	}
 
 	for _, source := range s.resolveFromPaths(ctx) {
@@ -155,13 +158,12 @@ func (s *addCopyStep) updateContextChecksum(ctx *context.BuildContext, checksum
 			if err != nil {
 				return fmt.Errorf("prev error during walk: %s", err)
 			}
-			checksum, err = checksumPathContents(path, fi, checksum)
-			return err
+			return checksumPathContents(path, fi, checksum)
 		}); err != nil {
-			return 0, fmt.Errorf("walk %s: %s", source, err)
+			return fmt.Errorf("walk %s: %s", source, err)
 		}
 	}
-	return checksum, nil
+	return nil
 }
 
 func (s *addCopyStep) resolveFromPaths(ctx *context.BuildContext) []string {
@@ -186,33 +188,40 @@ func (s *addCopyStep) contextRootDir(ctx *context.BuildContext) string {
 	return ctx.ContextDir
 }
 
-func checksumPathContents(path string, fi os.FileInfo, checksum uint32) (uint32, error) {
+func checksumPathContents(path string, fi os.FileInfo, checksum io.Writer) error {
 	// Skip special files.
 	if utils.IsSpecialFile(fi) {
 		if fi.IsDir() {
-			return checksum, filepath.SkipDir
+			return filepath.SkipDir
 		}
-		return checksum, nil
+		return nil
+	}
+
+	if _, err := checksum.Write([]byte(path)); err != nil {
+		return fmt.Errorf("write path to checksum")
 	}
 
-	checksum = crc32.Update(checksum, crc32.IEEETable, []byte(path))
 	// If it is a directory, just return after checksumming the dir name.
 	if fi.IsDir() {
-		return checksum, nil
+		return nil
 	}
 
 	// If it's a symlink, don't follow.
 	if fi.Mode()&os.ModeSymlink != 0 {
 		target, err := os.Readlink(path)
 		if err != nil {
-			return 0, fmt.Errorf("read link %s: %s", path, err)
+			return fmt.Errorf("read link %s: %s", path, err)
 		}
-		return crc32.Update(checksum, crc32.IEEETable, []byte(target)), nil
+		_, err = checksum.Write([]byte(target))
+		return err
 	}
 
-	b, err := ioutil.ReadFile(path)
+	fh, err := os.Open(path)
 	if err != nil {
-		return 0, fmt.Errorf("read %s: %s", path, err)
+		return fmt.Errorf("open %s: %s", path, err)
 	}
-	return crc32.Update(checksum, crc32.IEEETable, b), nil
+	if _, err := io.Copy(checksum, fh); err != nil {
+		return fmt.Errorf("read %s: %s", path, err)
+	}
+	return nil
 }
diff --git a/lib/builder/step/base_step.go b/lib/builder/step/base_step.go
index 8beb84b..37c805a 100644
--- a/lib/builder/step/base_step.go
+++ b/lib/builder/step/base_step.go
@@ -66,29 +66,14 @@ func (s *baseStep) SetCacheID(ctx *context.BuildContext, seed string) error {
 	return nil
 }
 
-// ApplyCtxAndConfig sets up the execution environment from build context and
-// image config.
-// This function will not be skipped.
-func (s *baseStep) ApplyCtxAndConfig(
+// SetWorkingDir set the working dir of the current step
+// Exporting the logic to this method allows for an easier `ApplyCtxAndConfig` overwriting
+func (s *baseStep) SetWorkingDir(
 	ctx *context.BuildContext, imageConfig *image.Config) error {
 	s.workingDir = ctx.RootDir // Default workingDir to root.
-	if imageConfig == nil {
-		return nil
-	}
 
-	for key, value := range ctx.StageVars {
-		unquoted, err := strconv.Unquote(value)
-		if err == nil {
-			value = unquoted
-		}
-		value = os.ExpandEnv(value)
-		if err := os.Setenv(key, value); err != nil {
-			return fmt.Errorf("failed to set env %s=%s: %s", key, value, err)
-		}
-	}
-
-	// Set working dir.
-	if imageConfig.Config.WorkingDir != "" {
+	// Set working dir from imageConfig
+	if imageConfig != nil && imageConfig.Config.WorkingDir != "" {
 		s.workingDir = os.ExpandEnv(imageConfig.Config.WorkingDir)
 	}
 
@@ -105,6 +90,33 @@ func (s *baseStep) ApplyCtxAndConfig(
 	return nil
 }
 
+// SetEnvFromContext set environment variables from previous stages
+// Exporting the logic to this method allows for an easier `ApplyCtxAndConfig` overwriting
+func (s *baseStep) SetEnvFromContext(
+	ctx *context.BuildContext) error {
+	for key, value := range ctx.StageVars {
+		unquoted, err := strconv.Unquote(value)
+		if err == nil {
+			value = unquoted
+		}
+		value = os.ExpandEnv(value)
+		if err := os.Setenv(key, value); err != nil {
+			return fmt.Errorf("failed to set env %s=%s: %s", key, value, err)
+		}
+	}
+	return nil
+}
+
+// ApplyCtxAndConfig sets up the execution environment from build context and
+// image config.
+// This function will not be skipped.
+func (s *baseStep) ApplyCtxAndConfig(
+	ctx *context.BuildContext, imageConfig *image.Config) error {
+	s.SetWorkingDir(ctx, imageConfig)
+	s.SetEnvFromContext(ctx)
+	return nil
+}
+
 // Execute executes the step. If modifyFS is true, the command might change the
 // local file system.
 // Default implementation is noop.
diff --git a/lib/builder/step/run_step.go b/lib/builder/step/run_step.go
index 6e44c61..941cd46 100644
--- a/lib/builder/step/run_step.go
+++ b/lib/builder/step/run_step.go
@@ -18,6 +18,7 @@ import (
 	"errors"
 
 	"github.com/uber/makisu/lib/context"
+	"github.com/uber/makisu/lib/docker/image"
 	"github.com/uber/makisu/lib/log"
 	"github.com/uber/makisu/lib/shell"
 )
@@ -27,6 +28,9 @@ type RunStep struct {
 	*baseStep
 
 	cmd string
+
+	// Used by the user step and the run step to determine which user should run a command (format should be <user>[:<group>] or <UID>[:<GID>], default is "" which is 0:0)
+	user string
 }
 
 // NewRunStep returns a BuildStep from given arguments.
@@ -41,6 +45,21 @@ func NewRunStep(args, cmd string, commit bool) *RunStep {
 // layers to be present on disk.
 func (s *RunStep) RequireOnDisk() bool { return true }
 
+// ApplyCtxAndConfig setup the user that should be used to run the command
+// See ./user_step.go to see how it's set in image.Config
+func (s *RunStep) ApplyCtxAndConfig(ctx *context.BuildContext, imageConfig *image.Config) error {
+	// This is from ./base_step.go
+	s.SetWorkingDir(ctx, imageConfig)
+	s.SetEnvFromContext(ctx)
+
+	if imageConfig == nil {
+		return nil
+	}
+
+	s.user = imageConfig.Config.User
+	return nil
+}
+
 // Execute executes the step.
 // It shells out to run the specified command, which might change local file system.
 func (s *RunStep) Execute(ctx *context.BuildContext, modifyFS bool) error {
@@ -48,5 +67,5 @@ func (s *RunStep) Execute(ctx *context.BuildContext, modifyFS bool) error {
 		return errors.New("attempted to execute RUN step without modifying file system")
 	}
 	ctx.MustScan = true
-	return shell.ExecCommand(log.Infof, log.Errorf, s.workingDir, "sh", "-c", s.cmd)
+	return shell.ExecCommand(log.Infof, log.Errorf, s.workingDir, s.user, "sh", "-c", s.cmd)
 }
diff --git a/lib/shell/cmd.go b/lib/shell/cmd.go
index 7c3f2ae..537eb8f 100644
--- a/lib/shell/cmd.go
+++ b/lib/shell/cmd.go
@@ -17,20 +17,48 @@ package shell
 import (
 	"fmt"
 	"io"
+	"os"
 	"os/exec"
+	"strings"
 	"syscall"
+
+	"github.com/uber/makisu/lib/utils"
 )
 
 // ShellStreamBufferSize is the size of the output buffers when streaming command stdout and stderr
 const ShellStreamBufferSize = 1 << 20
 
-// ExecCommand exec a command given workingDir, cmd and args, returns error if cmd fails
-func ExecCommand(outStream, errStream func(string, ...interface{}), workingDir, cmdName string, cmdArgs ...string) error {
+// ExecCommand exec a cmd and args inside workingDir as user, returns error if cmd fails
+func ExecCommand(outStream, errStream func(string, ...interface{}), workingDir, user, cmdName string, cmdArgs ...string) error {
 	cmd := exec.Command(cmdName, cmdArgs...)
 	if workingDir != "" {
 		cmd.Dir = workingDir
 	}
 
+	cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
+
+	currentEnv := os.Environ()
+
+	if user != "" {
+		// Set the user to the one specified before
+		uid, gid, err := utils.ResolveChown(user)
+		if err != nil {
+			return fmt.Errorf("cmd user resolve: %s", err)
+		}
+
+		uid32 := uint32(uid)
+		gid32 := uint32(gid)
+		cmd.SysProcAttr.Credential = &syscall.Credential{Uid: uid32, Gid: gid32}
+
+		// We also need to change the HOME env var if we change user
+		home := fmt.Sprintf("HOME=/home/%s", strings.Split(user, ":")[0])
+
+		// Append it so it has a priority on any other env var from before (and will override previous HOME definition)
+		currentEnv = append(currentEnv, home)
+	}
+
+	cmd.Env = currentEnv
+
 	outReader, outWriter := io.Pipe()
 	errReader, errWriter := io.Pipe()
 	cmd.Stdout, cmd.Stderr = outWriter, errWriter
diff --git a/lib/snapshot/copy_op.go b/lib/snapshot/copy_op.go
index 38e14a1..26ef53e 100644
--- a/lib/snapshot/copy_op.go
+++ b/lib/snapshot/copy_op.go
@@ -15,16 +15,14 @@
 package snapshot
 
 import (
-	"errors"
 	"fmt"
 	"os"
-	"os/user"
 	"path/filepath"
-	"strconv"
 	"strings"
 
 	"github.com/uber/makisu/lib/fileio"
 	"github.com/uber/makisu/lib/pathutils"
+	"github.com/uber/makisu/lib/utils"
 )
 
 // CopyOperation defines a copy operation that occurred to generate a layer from.
@@ -50,7 +48,7 @@ func NewCopyOperation(
 		return nil, fmt.Errorf("check copy param: %s", err)
 	}
 
-	uid, gid, err := resolveChown(chown)
+	uid, gid, err := utils.ResolveChown(chown)
 	if err != nil {
 		return nil, fmt.Errorf("resolve chown: %s", err)
 	}
@@ -113,50 +111,6 @@ func (c *CopyOperation) Execute() error {
 	return nil
 }
 
-// resolveChown converts a chown string to uid and gid integers.
-// Format: <user>[:<group>]
-// Both <user> and <group> can be either user/group strings or uid/gids.
-// If <group> is not specified, gid will be set to the resolved uid.
-func resolveChown(chown string) (uid, gid int, err error) {
-	// Default to 0 for both.
-	if chown == "" {
-		return 0, 0, nil
-	}
-
-	split := strings.Split(chown, ":")
-	if len(split) < 1 || len(split) > 2 {
-		return 0, 0, errors.New("failed to split on ':'")
-	}
-
-	uid, err = strconv.Atoi(split[0])
-	if err != nil {
-		user, err := user.Lookup(split[0])
-		if err != nil {
-			return 0, 0, fmt.Errorf("failed to look up user '%s': %s", split[0], err)
-		}
-		uid, err = strconv.Atoi(user.Uid)
-		if err != nil {
-			return 0, 0, fmt.Errorf("failed to parse uid to int '%s': %s", user.Uid, err)
-		}
-	}
-
-	if len(split) == 1 {
-		return uid, uid, nil
-	} else if gid, err := strconv.Atoi(split[1]); err == nil {
-		return uid, gid, nil
-	}
-
-	group, err := user.LookupGroup(split[1])
-	if err != nil {
-		return 0, 0, fmt.Errorf("failed to look up group '%s': %s", split[0], err)
-	}
-	gid, err = strconv.Atoi(group.Gid)
-	if err != nil {
-		return 0, 0, fmt.Errorf("failed to parse gid to int '%s': %s", group.Gid, err)
-	}
-	return uid, gid, nil
-}
-
 func resolveDestination(workDir, dst string) string {
 	if filepath.IsAbs(dst) {
 		return dst
diff --git a/lib/utils/utils.go b/lib/utils/utils.go
index 6a1bce5..9187de5 100644
--- a/lib/utils/utils.go
+++ b/lib/utils/utils.go
@@ -16,9 +16,12 @@ package utils
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"os"
+	"os/user"
 	"sort"
+	"strconv"
 	"strings"
 	"sync"
 	"syscall"
@@ -179,3 +182,47 @@ func IsValidJSON(blob []byte) bool {
 	into := map[string]interface{}{}
 	return json.Unmarshal(blob, &into) == nil
 }
+
+// ResolveChown converts a chown string to uid and gid integers.
+// Format: <user>[:<group>]
+// Both <user> and <group> can be either user/group strings or uid/gids.
+// If <group> is not specified, gid will be set to the resolved uid.
+func ResolveChown(chown string) (uid, gid int, err error) {
+	// Default to 0 for both.
+	if chown == "" {
+		return 0, 0, nil
+	}
+
+	split := strings.Split(chown, ":")
+	if len(split) < 1 || len(split) > 2 {
+		return 0, 0, errors.New("failed to split on ':'")
+	}
+
+	uid, err = strconv.Atoi(split[0])
+	if err != nil {
+		user, err := user.Lookup(split[0])
+		if err != nil {
+			return 0, 0, fmt.Errorf("failed to look up user '%s': %s", split[0], err)
+		}
+		uid, err = strconv.Atoi(user.Uid)
+		if err != nil {
+			return 0, 0, fmt.Errorf("failed to parse uid to int '%s': %s", user.Uid, err)
+		}
+	}
+
+	if len(split) == 1 {
+		return uid, uid, nil
+	} else if gid, err := strconv.Atoi(split[1]); err == nil {
+		return uid, gid, nil
+	}
+
+	group, err := user.LookupGroup(split[1])
+	if err != nil {
+		return 0, 0, fmt.Errorf("failed to look up group '%s': %s", split[0], err)
+	}
+	gid, err = strconv.Atoi(group.Gid)
+	if err != nil {
+		return 0, 0, fmt.Errorf("failed to parse gid to int '%s': %s", group.Gid, err)
+	}
+	return uid, gid, nil
+}


Test patch:
diff --git a/lib/docker/image/digest_test.go b/lib/docker/image/digest_test.go
index 80d5544..e47c23b 100644
--- a/lib/docker/image/digest_test.go
+++ b/lib/docker/image/digest_test.go
@@ -42,7 +42,7 @@ func TestEmptyDigest(t *testing.T) {
 	defer os.RemoveAll(tmpDir)
 
 	targetPath := tmpDir + ".tar"
-	err = shell.ExecCommand(log.Infof, log.Errorf, "", "tar", "cvf", targetPath, "--files-from", "/dev/null")
+	err = shell.ExecCommand(log.Infof, log.Errorf, "", "", "tar", "cvf", targetPath, "--files-from", "/dev/null")
 	require.NoError(err)
 	defer os.Remove(targetPath)
 
diff --git a/lib/docker/image/digester_test.go b/lib/docker/image/digester_test.go
index b260046..65be43a 100644
--- a/lib/docker/image/digester_test.go
+++ b/lib/docker/image/digester_test.go
@@ -33,7 +33,7 @@ func TestDigestFromBytes(t *testing.T) {
 	defer os.RemoveAll(tmpDir)
 
 	targetPath := tmpDir + ".tar"
-	err = shell.ExecCommand(log.Infof, log.Errorf, "", "tar", "cvf", targetPath, "--files-from", "/dev/null")
+	err = shell.ExecCommand(log.Infof, log.Errorf, "", "", "tar", "cvf", targetPath, "--files-from", "/dev/null")
 	require.NoError(err)
 	defer os.Remove(targetPath)
 
diff --git a/lib/shell/cmd_test.go b/lib/shell/cmd_test.go
index c7ff121..7df604a 100644
--- a/lib/shell/cmd_test.go
+++ b/lib/shell/cmd_test.go
@@ -53,7 +53,7 @@ func (s *bufferedSyncWriter) String() string {
 func TestExecCommandNoError(t *testing.T) {
 	require := require.New(t)
 	stdout, stderr := syncWriterFixture(), syncWriterFixture()
-	err := ExecCommand(stdout.Write, stderr.Write, ".", "go", "version")
+	err := ExecCommand(stdout.Write, stderr.Write, ".", "", "go", "version")
 	require.NoError(err)
 	require.Empty(stderr.String())
 	require.NotEmpty(stdout.String())
@@ -62,7 +62,7 @@ func TestExecCommandNoError(t *testing.T) {
 func TestExecCommandError(t *testing.T) {
 	require := require.New(t)
 	stdout, stderr := syncWriterFixture(), syncWriterFixture()
-	err := ExecCommand(stdout.Write, stderr.Write, ".", "go", "wrong")
+	err := ExecCommand(stdout.Write, stderr.Write, ".", "", "go", "wrong")
 	require.Error(err)
 	require.NotEmpty(stderr.String())
 }
diff --git a/lib/snapshot/copy_op_test.go b/lib/snapshot/copy_op_test.go
index f3d1d53..322922a 100644
--- a/lib/snapshot/copy_op_test.go
+++ b/lib/snapshot/copy_op_test.go
@@ -33,39 +33,6 @@ var (
 	validChown = fmt.Sprintf("%d:%d", testutil.CurrUID(), testutil.CurrGID())
 )
 
-func TestResolveChown(t *testing.T) {
-	tests := []struct {
-		desc    string
-		succeed bool
-		chown   string
-		uid     int
-		gid     int
-	}{
-		{"missing group", false, "user:", 0, 0},
-		{"missing user", false, ":group", 0, 0},
-		{"missing group or user", false, ":", 0, 0},
-		{"empty", true, "", 0, 0},
-		{"uid no group", true, "1", 1, 1},
-		{"uid and gid", true, "1:2", 1, 2},
-		{"user no group", true, fmt.Sprintf("%s", testutil.CurrUser()), testutil.CurrUID(), testutil.CurrUID()},
-		{"user and gid", true, fmt.Sprintf("%s:1", testutil.CurrUser()), testutil.CurrUID(), 1},
-		{"uid and gid", true, fmt.Sprintf("%d:%d", testutil.CurrUID(), testutil.CurrGID()), testutil.CurrUID(), testutil.CurrGID()},
-	}
-
-	for _, test := range tests {
-		t.Run(test.desc, func(t *testing.T) {
-			require := require.New(t)
-			uid, gid, err := resolveChown(test.chown)
-			if test.succeed {
-				require.Equal(test.uid, uid)
-				require.Equal(test.gid, gid)
-			} else {
-				require.Error(err)
-			}
-		})
-	}
-}
-
 func TestNewCopyOperation(t *testing.T) {
 	require := require.New(t)
 
diff --git a/lib/tario/testutils_test.go b/lib/tario/testutils_test.go
index c4f3fbf..b5e640c 100644
--- a/lib/tario/testutils_test.go
+++ b/lib/tario/testutils_test.go
@@ -21,5 +21,5 @@ import (
 
 func untarHelper(t, out string) error {
 	// Use p option to preserve permissions.
-	return shell.ExecCommand(log.Infof, log.Errorf, "", "tar", "xfp", t, "-C", out)
+	return shell.ExecCommand(log.Infof, log.Errorf, "", "", "tar", "xfp", t, "-C", out)
 }
diff --git a/lib/utils/utils_test.go b/lib/utils/utils_test.go
index 2d0dd2d..b25a5f2 100644
--- a/lib/utils/utils_test.go
+++ b/lib/utils/utils_test.go
@@ -27,6 +27,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/require"
+	"github.com/uber/makisu/lib/utils/testutil"
 )
 
 func TestMultiError(t *testing.T) {
@@ -174,3 +175,36 @@ func TestIsSpecialFileWithSocket(t *testing.T) {
 	require.NoError(err)
 	require.True(IsSpecialFile(fi))
 }
+
+func TestResolveChown(t *testing.T) {
+	tests := []struct {
+		desc    string
+		succeed bool
+		chown   string
+		uid     int
+		gid     int
+	}{
+		{"missing group", false, "user:", 0, 0},
+		{"missing user", false, ":group", 0, 0},
+		{"missing group or user", false, ":", 0, 0},
+		{"empty", true, "", 0, 0},
+		{"uid no group", true, "1", 1, 1},
+		{"uid and gid", true, "1:2", 1, 2},
+		{"user no group", true, fmt.Sprintf("%s", testutil.CurrUser()), testutil.CurrUID(), testutil.CurrUID()},
+		{"user and gid", true, fmt.Sprintf("%s:1", testutil.CurrUser()), testutil.CurrUID(), 1},
+		{"uid and gid", true, fmt.Sprintf("%d:%d", testutil.CurrUID(), testutil.CurrGID()), testutil.CurrUID(), testutil.CurrGID()},
+	}
+
+	for _, test := range tests {
+		t.Run(test.desc, func(t *testing.T) {
+			require := require.New(t)
+			uid, gid, err := ResolveChown(test.chown)
+			if test.succeed {
+				require.Equal(test.uid, uid)
+				require.Equal(test.gid, gid)
+			} else {
+				require.Error(err)
+			}
+		})
+	}
+}
diff --git a/test/python/test_build.py b/test/python/test_build.py
index 1e67be1..af544d2 100644
--- a/test/python/test_build.py
+++ b/test/python/test_build.py
@@ -79,6 +79,17 @@ def test_build_arg_and_env(registry1, storage_dir):
     assert code == 0, err
 
 
+def test_user_change(registry1, storage_dir):
+    new_image = new_image_name()
+    context_dir = os.path.join(
+        os.getcwd(), 'testdata/build-context/user-change')
+
+    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
+
+
 # When we run makisu build, some mountpoints and their parent directories are
 # already created. As such, when we unpack `/var/run -> /run` symlink in
 # `debian:9` for instance, we get an error because we think we should delete
diff --git a/testdata/build-context/user-change/Dockerfile b/testdata/build-context/user-change/Dockerfile
new file mode 100644
index 0000000..3c64539
--- /dev/null
+++ b/testdata/build-context/user-change/Dockerfile
@@ -0,0 +1,11 @@
+FROM alpine
+
+RUN whoami > /tmp/root_file
+
+USER daemon
+
+RUN whoami > /tmp/daemon_file
+
+USER root
+
+ENTRYPOINT [ "sh", "-c", "test $(cat /tmp/root_file) = 'root' && test $(cat /tmp/daemon_file) = 'daemon'" ]
\ No newline at end of file