Rebase --chown function for ADD/COPY

Rebases and completes initial PR for (prior: --user) --chown flag for
ADD/COPY commands in Dockerfile.

Docker-DCO-1.1-Signed-off-by: Phil Estes <estesp@linux.vnet.ibm.com>

builder/dockerfile/copy.go

@@ -56,6 +56,7 @@ type copyInstruction struct {
 	cmdName                 string
 	infos                   []copyInfo
 	dest                    string
+	chownStr                string
 	allowLocalDecompression bool
 }
 
@@ -369,6 +370,7 @@ func downloadSource(output io.Writer, stdout io.Writer, srcURL string) (remote b
 type copyFileOptions struct {
 	decompress bool
 	archiver   *archive.Archiver
+	chownPair  idtools.IDPair
 }
 
 func performCopyForInfo(dest copyInfo, source copyInfo, options copyFileOptions) error {
@@ -388,7 +390,7 @@ func performCopyForInfo(dest copyInfo, source copyInfo, options copyFileOptions)
 		return errors.Wrapf(err, "source path not found")
 	}
 	if src.IsDir() {
-		return copyDirectory(archiver, srcPath, destPath)
+		return copyDirectory(archiver, srcPath, destPath, options.chownPair)
 	}
 	if options.decompress && archive.IsArchivePath(srcPath) && !source.noDecompress {
 		return archiver.UntarPath(srcPath, destPath)
@@ -405,26 +407,28 @@ func performCopyForInfo(dest copyInfo, source copyInfo, options copyFileOptions)
 		// is a symlink
 		destPath = filepath.Join(destPath, filepath.Base(source.path))
 	}
-	return copyFile(archiver, srcPath, destPath)
+	return copyFile(archiver, srcPath, destPath, options.chownPair)
 }
 
-func copyDirectory(archiver *archive.Archiver, source, dest string) error {
+func copyDirectory(archiver *archive.Archiver, source, dest string, chownPair idtools.IDPair) error {
+	destExists, err := isExistingDirectory(dest)
+	if err != nil {
+		return errors.Wrapf(err, "failed to query destination path")
+	}
 	if err := archiver.CopyWithTar(source, dest); err != nil {
 		return errors.Wrapf(err, "failed to copy directory")
 	}
-	return fixPermissions(source, dest, archiver.IDMappings.RootPair())
+	return fixPermissions(source, dest, chownPair, !destExists)
 }
 
-func copyFile(archiver *archive.Archiver, source, dest string) error {
+func copyFile(archiver *archive.Archiver, source, dest string, chownPair idtools.IDPair) error {
-	rootIDs := archiver.IDMappings.RootPair()
+	if err := idtools.MkdirAllAndChownNew(filepath.Dir(dest), 0755, chownPair); err != nil {
-
-	if err := idtools.MkdirAllAndChownNew(filepath.Dir(dest), 0755, rootIDs); err != nil {
 		return errors.Wrapf(err, "failed to create new directory")
 	}
 	if err := archiver.CopyFileWithTar(source, dest); err != nil {
 		return errors.Wrapf(err, "failed to copy file")
 	}
-	return fixPermissions(source, dest, rootIDs)
+	return fixPermissions(source, dest, chownPair, false)
 }
 
 func endsInSlash(path string) bool {

builder/dockerfile/copy_unix.go

@@ -9,11 +9,17 @@ import (
 	"github.com/docker/docker/pkg/idtools"
 )
 
-func fixPermissions(source, destination string, rootIDs idtools.IDPair) error {
+func fixPermissions(source, destination string, rootIDs idtools.IDPair, overrideSkip bool) error {
-	skipChownRoot, err := isExistingDirectory(destination)
+	var (
+		skipChownRoot bool
+		err           error
+	)
+	if !overrideSkip {
+		skipChownRoot, err = isExistingDirectory(destination)
 		if err != nil {
 			return err
 		}
+	}
 
 	// We Walk on the source rather than on the destination because we don't
 	// want to change permissions on things we haven't created or modified.

builder/dockerfile/copy_windows.go

@@ -2,7 +2,7 @@ package dockerfile
 
 import "github.com/docker/docker/pkg/idtools"
 
-func fixPermissions(source, destination string, rootIDs idtools.IDPair) error {
+func fixPermissions(source, destination string, rootIDs idtools.IDPair, overrideSkip bool) error {
 	// chown is not supported on Windows
 	return nil
 }

builder/dockerfile/dispatchers.go

@@ -158,6 +158,7 @@ func add(req dispatchRequest) error {
 	if err != nil {
 		return err
 	}
+	copyInstruction.chownStr = flChown.Value
 	copyInstruction.allowLocalDecompression = true
 
 	return req.builder.performCopy(req.state, copyInstruction)
@@ -189,6 +190,7 @@ func dispatchCopy(req dispatchRequest) error {
 	if err != nil {
 		return err
 	}
+	copyInstruction.chownStr = flChown.Value
 
 	return req.builder.performCopy(req.state, copyInstruction)
 }

builder/dockerfile/internals.go

@@ -7,13 +7,18 @@ import (
 	"crypto/sha256"
 	"encoding/hex"
 	"fmt"
+	"path/filepath"
+	"strconv"
 	"strings"
 
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/backend"
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/image"
+	"github.com/docker/docker/pkg/idtools"
 	"github.com/docker/docker/pkg/stringid"
+	"github.com/docker/docker/pkg/symlink"
+	lcUser "github.com/opencontainers/runc/libcontainer/user"
 	"github.com/pkg/errors"
 )
 
@@ -107,10 +112,16 @@ func (b *Builder) exportImage(state *dispatchState, imageMount *imageMount, runC
 func (b *Builder) performCopy(state *dispatchState, inst copyInstruction) error {
 	srcHash := getSourceHashFromInfos(inst.infos)
 
+	var chownComment string
+	if inst.chownStr != "" {
+		chownComment = fmt.Sprintf("--chown=%s", inst.chownStr)
+	}
+	commentStr := fmt.Sprintf("%s %s%s in %s ", inst.cmdName, chownComment, srcHash, inst.dest)
+
 	// TODO: should this have been using origPaths instead of srcHash in the comment?
 	runConfigWithCommentCmd := copyRunConfig(
 		state.runConfig,
-		withCmdCommentString(fmt.Sprintf("%s %s in %s ", inst.cmdName, srcHash, inst.dest), b.platform))
+		withCmdCommentString(commentStr, b.platform))
 	hit, err := b.probeCache(state, runConfigWithCommentCmd)
 	if err != nil || hit {
 		return err
@@ -125,9 +136,21 @@ func (b *Builder) performCopy(state *dispatchState, inst copyInstruction) error
 		return err
 	}
 
+	chownPair := b.archiver.IDMappings.RootPair()
+	// if a chown was requested, perform the steps to get the uid, gid
+	// translated (if necessary because of user namespaces), and replace
+	// the root pair with the chown pair for copy operations
+	if inst.chownStr != "" {
+		chownPair, err = parseChownFlag(inst.chownStr, destInfo.root, b.archiver.IDMappings)
+		if err != nil {
+			return errors.Wrapf(err, "unable to convert uid/gid chown string to host mapping")
+		}
+	}
+
 	opts := copyFileOptions{
 		decompress: inst.allowLocalDecompression,
 		archiver:   b.archiver,
+		chownPair:  chownPair,
 	}
 	for _, info := range inst.infos {
 		if err := performCopyForInfo(destInfo, info, opts); err != nil {
@@ -137,6 +160,88 @@ func (b *Builder) performCopy(state *dispatchState, inst copyInstruction) error
 	return b.exportImage(state, imageMount, runConfigWithCommentCmd)
 }
 
+func parseChownFlag(chown, ctrRootPath string, idMappings *idtools.IDMappings) (idtools.IDPair, error) {
+	var userStr, grpStr string
+	parts := strings.Split(chown, ":")
+	if len(parts) > 2 {
+		return idtools.IDPair{}, errors.New("invalid chown string format: " + chown)
+	}
+	if len(parts) == 1 {
+		// if no group specified, use the user spec as group as well
+		userStr, grpStr = parts[0], parts[0]
+	} else {
+		userStr, grpStr = parts[0], parts[1]
+	}
+
+	passwdPath, err := symlink.FollowSymlinkInScope(filepath.Join(ctrRootPath, "etc", "passwd"), ctrRootPath)
+	if err != nil {
+		return idtools.IDPair{}, errors.Wrapf(err, "can't resolve /etc/passwd path in container rootfs")
+	}
+	groupPath, err := symlink.FollowSymlinkInScope(filepath.Join(ctrRootPath, "etc", "group"), ctrRootPath)
+	if err != nil {
+		return idtools.IDPair{}, errors.Wrapf(err, "can't resolve /etc/group path in container rootfs")
+	}
+	uid, err := lookupUser(userStr, passwdPath)
+	if err != nil {
+		return idtools.IDPair{}, errors.Wrapf(err, "can't find uid for user "+userStr)
+	}
+	gid, err := lookupGroup(grpStr, groupPath)
+	if err != nil {
+		return idtools.IDPair{}, errors.Wrapf(err, "can't find gid for group "+grpStr)
+	}
+
+	// convert as necessary because of user namespaces
+	chownPair, err := idMappings.ToHost(idtools.IDPair{UID: uid, GID: gid})
+	if err != nil {
+		return idtools.IDPair{}, errors.Wrapf(err, "unable to convert uid/gid to host mapping")
+	}
+	return chownPair, nil
+}
+
+func lookupUser(userStr, filepath string) (int, error) {
+	// if the string is actually a uid integer, parse to int and return
+	// as we don't need to translate with the help of files
+	uid, err := strconv.Atoi(userStr)
+	if err == nil {
+		return uid, nil
+	}
+	users, err := lcUser.ParsePasswdFileFilter(filepath, func(u lcUser.User) bool {
+		if u.Name == userStr {
+			return true
+		}
+		return false
+	})
+	if err != nil {
+		return 0, err
+	}
+	if len(users) == 0 {
+		return 0, errors.New("no such user: " + userStr)
+	}
+	return users[0].Uid, nil
+}
+
+func lookupGroup(groupStr, filepath string) (int, error) {
+	// if the string is actually a gid integer, parse to int and return
+	// as we don't need to translate with the help of files
+	gid, err := strconv.Atoi(groupStr)
+	if err == nil {
+		return gid, nil
+	}
+	groups, err := lcUser.ParseGroupFileFilter(filepath, func(g lcUser.Group) bool {
+		if g.Name == groupStr {
+			return true
+		}
+		return false
+	})
+	if err != nil {
+		return 0, err
+	}
+	if len(groups) == 0 {
+		return 0, errors.New("no such group: " + groupStr)
+	}
+	return groups[0].Gid, nil
+}
+
 func createDestInfo(workingDir string, inst copyInstruction, imageMount *imageMount) (copyInfo, error) {
 	// Twiddle the destination when it's a relative path - meaning, make it
 	// relative to the WORKINGDIR

builder/dockerfile/internals_test.go

@@ -2,6 +2,8 @@ package dockerfile
 
 import (
 	"fmt"
+	"os"
+	"path/filepath"
 	"runtime"
 	"testing"
 
@@ -11,6 +13,7 @@ import (
 	"github.com/docker/docker/builder"
 	"github.com/docker/docker/builder/remotecontext"
 	"github.com/docker/docker/pkg/archive"
+	"github.com/docker/docker/pkg/idtools"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -129,3 +132,130 @@ func TestCopyRunConfig(t *testing.T) {
 	}
 
 }
+
+func TestChownFlagParsing(t *testing.T) {
+	testFiles := map[string]string{
+		"passwd": `root:x:0:0::/bin:/bin/false
+bin:x:1:1::/bin:/bin/false
+wwwwww:x:21:33::/bin:/bin/false
+unicorn:x:1001:1002::/bin:/bin/false
+		`,
+		"group": `root:x:0:
+bin:x:1:
+wwwwww:x:33:
+unicorn:x:1002:
+somegrp:x:5555:
+othergrp:x:6666:
+		`,
+	}
+	// test mappings for validating use of maps
+	idMaps := []idtools.IDMap{
+		{
+			ContainerID: 0,
+			HostID:      100000,
+			Size:        65536,
+		},
+	}
+	remapped := idtools.NewIDMappingsFromMaps(idMaps, idMaps)
+	unmapped := &idtools.IDMappings{}
+
+	contextDir, cleanup := createTestTempDir(t, "", "builder-chown-parse-test")
+	defer cleanup()
+
+	if err := os.Mkdir(filepath.Join(contextDir, "etc"), 0755); err != nil {
+		t.Fatalf("error creating test directory: %v", err)
+	}
+
+	for filename, content := range testFiles {
+		createTestTempFile(t, filepath.Join(contextDir, "etc"), filename, content, 0644)
+	}
+
+	// positive tests
+	for _, testcase := range []struct {
+		name      string
+		chownStr  string
+		idMapping *idtools.IDMappings
+		expected  idtools.IDPair
+	}{
+		{
+			name:      "UIDNoMap",
+			chownStr:  "1",
+			idMapping: unmapped,
+			expected:  idtools.IDPair{UID: 1, GID: 1},
+		},
+		{
+			name:      "UIDGIDNoMap",
+			chownStr:  "0:1",
+			idMapping: unmapped,
+			expected:  idtools.IDPair{UID: 0, GID: 1},
+		},
+		{
+			name:      "UIDWithMap",
+			chownStr:  "0",
+			idMapping: remapped,
+			expected:  idtools.IDPair{UID: 100000, GID: 100000},
+		},
+		{
+			name:      "UIDGIDWithMap",
+			chownStr:  "1:33",
+			idMapping: remapped,
+			expected:  idtools.IDPair{UID: 100001, GID: 100033},
+		},
+		{
+			name:      "UserNoMap",
+			chownStr:  "bin:5555",
+			idMapping: unmapped,
+			expected:  idtools.IDPair{UID: 1, GID: 5555},
+		},
+		{
+			name:      "GroupWithMap",
+			chownStr:  "0:unicorn",
+			idMapping: remapped,
+			expected:  idtools.IDPair{UID: 100000, GID: 101002},
+		},
+		{
+			name:      "UserOnlyWithMap",
+			chownStr:  "unicorn",
+			idMapping: remapped,
+			expected:  idtools.IDPair{UID: 101001, GID: 101002},
+		},
+	} {
+		t.Run(testcase.name, func(t *testing.T) {
+			idPair, err := parseChownFlag(testcase.chownStr, contextDir, testcase.idMapping)
+			require.NoError(t, err, "Failed to parse chown flag: %q", testcase.chownStr)
+			assert.Equal(t, testcase.expected, idPair, "chown flag mapping failure")
+		})
+	}
+
+	// error tests
+	for _, testcase := range []struct {
+		name      string
+		chownStr  string
+		idMapping *idtools.IDMappings
+		descr     string
+	}{
+		{
+			name:      "BadChownFlagFormat",
+			chownStr:  "bob:1:555",
+			idMapping: unmapped,
+			descr:     "invalid chown string format: bob:1:555",
+		},
+		{
+			name:      "UserNoExist",
+			chownStr:  "bob",
+			idMapping: unmapped,
+			descr:     "can't find uid for user bob: no such user: bob",
+		},
+		{
+			name:      "GroupNoExist",
+			chownStr:  "root:bob",
+			idMapping: unmapped,
+			descr:     "can't find gid for group bob: no such group: bob",
+		},
+	} {
+		t.Run(testcase.name, func(t *testing.T) {
+			_, err := parseChownFlag(testcase.chownStr, contextDir, testcase.idMapping)
+			assert.EqualError(t, err, testcase.descr, "Expected error string doesn't match")
+		})
+	}
+}

container/container.go

@@ -44,7 +44,6 @@ import (
 	"github.com/docker/libnetwork/options"
 	"github.com/docker/libnetwork/types"
 	agentexec "github.com/docker/swarmkit/agent/exec"
-	"github.com/opencontainers/runc/libcontainer/user"
 	"golang.org/x/net/context"
 )
 
@@ -332,36 +331,6 @@ func (container *Container) GetRootResourcePath(path string) (string, error) {
 	return symlink.FollowSymlinkInScope(filepath.Join(container.Root, cleanPath), container.Root)
 }
 
-// ParseUserGrp takes `username` in the format of username, uid, username:groupname,
-// uid:gid, username:gid, or uid:groupname and parses the passwd file in the container
-// to return the ExecUser referred to by `username`.
-func (container *Container) ParseUserGrp(username string) (*user.ExecUser, error) {
-	passwdPath, err := user.GetPasswdPath()
-	if err != nil {
-		return nil, err
-	}
-	passwdPath, err = container.GetResourcePath(passwdPath)
-	if err != nil {
-		return nil, err
-	}
-
-	groupPath, err := user.GetGroupPath()
-	if err != nil {
-		return nil, err
-	}
-	groupPath, err = container.GetResourcePath(groupPath)
-	if err != nil {
-		return nil, err
-	}
-
-	execUser, err := user.GetExecUserPath(username, nil, passwdPath, groupPath)
-	if err != nil {
-		return nil, err
-	}
-
-	return execUser, nil
-}
-
 // ExitOnNext signals to the monitor that it should not restart the container
 // after we send the kill signal.
 func (container *Container) ExitOnNext() {

integration-cli/docker_api_build_test.go

@@ -410,6 +410,35 @@ func (s *DockerSuite) TestBuildAddRemoteNoDecompress(c *check.C) {
 	assert.Contains(c, string(out), "Successfully built")
 }
 
+func (s *DockerSuite) TestBuildChownOnCopy(c *check.C) {
+	testRequires(c, DaemonIsLinux)
+	dockerfile := `FROM busybox
+		RUN echo 'test1:x:1001:1001::/bin:/bin/false' >> /etc/passwd
+		RUN echo 'test1:x:1001:' >> /etc/group
+		RUN echo 'test2:x:1002:' >> /etc/group
+		COPY --chown=test1:1002 . /new_dir
+		RUN ls -l /
+		RUN [ $(ls -l / | grep new_dir | awk '{print $3":"$4}') = 'test1:test2' ]
+		RUN [ $(ls -nl / | grep new_dir | awk '{print $3":"$4}') = '1001:1002' ]
+	`
+	ctx := fakecontext.New(c, "",
+		fakecontext.WithDockerfile(dockerfile),
+		fakecontext.WithFile("test_file1", "some test content"),
+	)
+	defer ctx.Close()
+
+	res, body, err := request.Post(
+		"/build",
+		request.RawContent(ctx.AsTarReader(c)),
+		request.ContentType("application/x-tar"))
+	c.Assert(err, checker.IsNil)
+	c.Assert(res.StatusCode, checker.Equals, http.StatusOK)
+
+	out, err := testutil.ReadBody(body)
+	require.NoError(c, err)
+	assert.Contains(c, string(out), "Successfully built")
+}
+
 func (s *DockerSuite) TestBuildWithSession(c *check.C) {
 	testRequires(c, ExperimentalDaemon)
 

integration-cli/docker_cli_build_test.go

@@ -602,78 +602,6 @@ RUN [ $(cat "/test dir/test_file6") = 'test6' ]`, command, command, command, com
 	}
 }
 
-func (s *DockerSuite) TestBuildAddChownFlag(c *check.C) {
-	testRequires(c, DaemonIsLinux) // Linux specific test
-	name := "testaddtonewdest"
-	ctx, err := fakeContext(`FROM busybox
-RUN echo 'test1:x:1001:1001::/bin:/bin/false' >> /etc/passwd
-RUN echo 'test1:x:1001:' >> /etc/group
-RUN echo 'test2:x:1002:' >> /etc/group
-ADD --chown=test1:1002 . /new_dir
-RUN ls -l /
-RUN [ $(ls -l / | grep new_dir | awk '{print $3":"$4}') = 'test1:test2' ]`,
-		map[string]string{
-			"test_dir/test_file": "test file",
-		})
-	if err != nil {
-		c.Fatal(err)
-	}
-	defer ctx.Close()
-
-	if _, err := buildImageFromContext(name, ctx, true); err != nil {
-		c.Fatal(err)
-	}
-}
-
-func (s *DockerDaemonSuite) TestBuildAddChownFlagUserNamespace(c *check.C) {
-	testRequires(c, DaemonIsLinux) // Linux specific test
-
-	c.Assert(s.d.StartWithBusybox("--userns-remap", "default"), checker.IsNil)
-
-	name := "testaddtonewdest"
-	ctx, err := fakeContext(`FROM busybox
-RUN echo 'test1:x:1001:1001::/bin:/bin/false' >> /etc/passwd
-RUN echo 'test1:x:1001:' >> /etc/group
-RUN echo 'test2:x:1002:' >> /etc/group
-ADD --chown=test1:1002 . /new_dir
-RUN ls -l /
-RUN [ $(ls -l / | grep new_dir | awk '{print $3":"$4}') = 'test1:test2' ]`,
-		map[string]string{
-			"test_dir/test_file": "test file",
-		})
-	if err != nil {
-		c.Fatal(err)
-	}
-	defer ctx.Close()
-
-	if _, err := buildImageFromContext(name, ctx, true); err != nil {
-		c.Fatal(err)
-	}
-}
-
-func (s *DockerSuite) TestBuildCopyChownFlag(c *check.C) {
-	testRequires(c, DaemonIsLinux) // Linux specific test
-	name := "testaddtonewdest"
-	ctx, err := fakeContext(`FROM busybox
-RUN echo 'test1:x:1001:1001::/bin:/bin/false' >> /etc/passwd
-RUN echo 'test1:x:1001:' >> /etc/group
-RUN echo 'test2:x:1002:' >> /etc/group
-COPY --chown=test1:1002 . /new_dir
-RUN ls -l /
-RUN [ $(ls -l / | grep new_dir | awk '{print $3":"$4}') = 'test1:test2' ]`,
-		map[string]string{
-			"test_dir/test_file": "test file",
-		})
-	if err != nil {
-		c.Fatal(err)
-	}
-	defer ctx.Close()
-
-	if _, err := buildImageFromContext(name, ctx, true); err != nil {
-		c.Fatal(err)
-	}
-}
-
 func (s *DockerSuite) TestBuildCopyFileWithWhitespaceOnWindows(c *check.C) {
 	testRequires(c, DaemonIsWindows)
 	dockerfile := `FROM ` + testEnv.MinimalBaseImage() + `