Merge pull request containers#3095 from nalind/user-spec

COPY --chown: expand the conformance test

add.go

@@ -251,9 +251,9 @@ func (b *Builder) Add(destination string, extract bool, options AddAndCopyOption
 
 	// Find out which user (and group) the destination should belong to.
 	var chownDirs, chownFiles *idtools.IDPair
-	var user specs.User
+	var userUID, userGID uint32
 	if options.Chown != "" {
-		user, _, err = b.user(mountPoint, options.Chown)
+		userUID, userGID, err = b.userForCopy(mountPoint, options.Chown)
 		if err != nil {
 			return errors.Wrapf(err, "error looking up UID/GID for %q", options.Chown)
 		}
@@ -268,8 +268,8 @@ func (b *Builder) Add(destination string, extract bool, options AddAndCopyOption
 		chmodDirsFiles = &perm
 	}
 
-	chownDirs = &idtools.IDPair{UID: int(user.UID), GID: int(user.GID)}
-	chownFiles = &idtools.IDPair{UID: int(user.UID), GID: int(user.GID)}
+	chownDirs = &idtools.IDPair{UID: int(userUID), GID: int(userGID)}
+	chownFiles = &idtools.IDPair{UID: int(userUID), GID: int(userGID)}
 	if options.Chown == "" && options.PreserveOwnership {
 		chownDirs = nil
 		chownFiles = nil
@@ -570,8 +570,9 @@ func (b *Builder) Add(destination string, extract bool, options AddAndCopyOption
 	return nil
 }
 
-// user returns the user (and group) information which the destination should belong to.
-func (b *Builder) user(mountPoint string, userspec string) (specs.User, string, error) {
+// userForRun returns the user (and group) information which we should use for
+// running commands
+func (b *Builder) userForRun(mountPoint string, userspec string) (specs.User, string, error) {
 	if userspec == "" {
 		userspec = b.User()
 	}
@@ -595,3 +596,18 @@ func (b *Builder) user(mountPoint string, userspec string) (specs.User, string,
 	}
 	return u, homeDir, err
 }
+
+// userForCopy returns the user (and group) information which we should use for
+// setting ownership of contents being copied.  It's just like what
+// userForRun() does, except for the case where we're passed a single numeric
+// value, where we need to use that value for both the UID and the GID.
+func (b *Builder) userForCopy(mountPoint string, userspec string) (uint32, uint32, error) {
+	if id, err := strconv.ParseUint(userspec, 10, 32); err == nil {
+		return uint32(id), uint32(id), nil
+	}
+	user, _, err := b.userForRun(mountPoint, userspec)
+	if err != nil {
+		return 0xffffffff, 0xffffffff, err
+	}
+	return user.UID, user.GID, nil
+}

pkg/chrootuser/user_linux.go

@@ -15,7 +15,6 @@ import (
 	"sync"
 
 	"github.com/containers/storage/pkg/reexec"
-	"github.com/sirupsen/logrus"
 	"golang.org/x/sys/unix"
 )
 
@@ -92,36 +91,13 @@ type lookupGroupEntry struct {
 	user string
 }
 
-func readWholeLine(rc *bufio.Reader) ([]byte, error) {
-	line, isPrefix, err := rc.ReadLine()
-	if err != nil {
-		return nil, err
-	}
-	for isPrefix {
-		// We didn't get a whole line.  Keep reading chunks until we find an end of line, and discard them.
-		for isPrefix {
-			logrus.Debugf("discarding partial line %q", string(line))
-			_, isPrefix, err = rc.ReadLine()
-			if err != nil {
-				return nil, err
-			}
-		}
-		// That last read was the end of a line, so now we try to read the (beginning of?) the next line.
-		line, isPrefix, err = rc.ReadLine()
-		if err != nil {
-			return nil, err
-		}
-	}
-	return line, nil
-}
-
-func parseNextPasswd(rc *bufio.Reader) *lookupPasswdEntry {
-	line, err := readWholeLine(rc)
-	if err != nil {
+func parseNextPasswd(rc *bufio.Scanner) *lookupPasswdEntry {
+	if !rc.Scan() {
 		return nil
 	}
-	fields := strings.Split(string(line), ":")
-	if len(fields) < 7 {
+	line := rc.Text()
+	fields := strings.Split(line, ":")
+	if len(fields) != 7 {
 		return nil
 	}
 	uid, err := strconv.ParseUint(fields[2], 10, 32)
@@ -140,13 +116,13 @@ func parseNextPasswd(rc *bufio.Reader) *lookupPasswdEntry {
 	}
 }
 
-func parseNextGroup(rc *bufio.Reader) *lookupGroupEntry {
-	line, err := readWholeLine(rc)
-	if err != nil {
+func parseNextGroup(rc *bufio.Scanner) *lookupGroupEntry {
+	if !rc.Scan() {
 		return nil
 	}
-	fields := strings.Split(string(line), ":")
-	if len(fields) < 4 {
+	line := rc.Text()
+	fields := strings.Split(line, ":")
+	if len(fields) != 4 {
 		return nil
 	}
 	gid, err := strconv.ParseUint(fields[2], 10, 32)
@@ -168,7 +144,7 @@ func lookupUserInContainer(rootdir, username string) (uid uint64, gid uint64, er
 	defer func() {
 		_ = cmd.Wait()
 	}()
-	rc := bufio.NewReader(f)
+	rc := bufio.NewScanner(f)
 	defer f.Close()
 
 	lookupUser.Lock()
@@ -194,7 +170,7 @@ func lookupGroupForUIDInContainer(rootdir string, userid uint64) (username strin
 	defer func() {
 		_ = cmd.Wait()
 	}()
-	rc := bufio.NewReader(f)
+	rc := bufio.NewScanner(f)
 	defer f.Close()
 
 	lookupUser.Lock()
@@ -226,7 +202,7 @@ func lookupAdditionalGroupsForUIDInContainer(rootdir string, userid uint64) (gid
 	defer func() {
 		_ = cmd.Wait()
 	}()
-	rc := bufio.NewReader(f)
+	rc := bufio.NewScanner(f)
 	defer f.Close()
 
 	lookupGroup.Lock()
@@ -250,7 +226,7 @@ func lookupGroupInContainer(rootdir, groupname string) (gid uint64, err error) {
 	defer func() {
 		_ = cmd.Wait()
 	}()
-	rc := bufio.NewReader(f)
+	rc := bufio.NewScanner(f)
 	defer f.Close()
 
 	lookupGroup.Lock()
@@ -276,7 +252,7 @@ func lookupUIDInContainer(rootdir string, uid uint64) (string, uint64, error) {
 	defer func() {
 		_ = cmd.Wait()
 	}()
-	rc := bufio.NewReader(f)
+	rc := bufio.NewScanner(f)
 	defer f.Close()
 
 	lookupUser.Lock()
@@ -302,7 +278,7 @@ func lookupHomedirInContainer(rootdir string, uid uint64) (string, error) {
 	defer func() {
 		_ = cmd.Wait()
 	}()
-	rc := bufio.NewReader(f)
+	rc := bufio.NewScanner(f)
 	defer f.Close()
 
 	lookupUser.Lock()

run_linux.go

@@ -1967,7 +1967,7 @@ func getDNSIP(dnsServers []string) (dns []net.IP, err error) {
 
 func (b *Builder) configureUIDGID(g *generate.Generator, mountPoint string, options RunOptions) (string, error) {
 	// Set the user UID/GID/supplemental group list/capabilities lists.
-	user, homeDir, err := b.user(mountPoint, options.User)
+	user, homeDir, err := b.userForRun(mountPoint, options.User)
 	if err != nil {
 		return "", err
 	}

tests/conformance/conformance_test.go

@@ -1374,6 +1374,12 @@ var internalTestCases = []testCase{
 		fsSkip:     []string{"(dir):usr:(dir):bin:mtime"},
 	},
 
+	{
+		name:       "copy with --chown",
+		contextDir: "copychown",
+		fsSkip:     []string{"(dir):usr:(dir):bin:mtime", "(dir):usr:(dir):local:(dir):bin:mtime"},
+	},
+
 	{
 		name:       "directory with slash",
 		contextDir: "overlapdirwithslash",

tests/conformance/testdata/copychown/Dockerfile

@@ -0,0 +1,11 @@
+FROM centos:7
+COPY --chown=1:2    script /usr/bin/script.12
+COPY --chown=1:adm  script /usr/bin/script.1-adm
+COPY --chown=1      script /usr/bin/script.1
+COPY --chown=lp:adm script /usr/bin/script.lp-adm
+COPY --chown=2:mail script /usr/bin/script.2-mail
+COPY --chown=2      script /usr/bin/script.2
+COPY --chown=bin    script /usr/bin/script.bin
+COPY --chown=lp     script /usr/bin/script.lp
+COPY --chown=3      script script2 /usr/local/bin/
+RUN ls -al /usr/bin/script

tests/conformance/testdata/copychown/script

@@ -0,0 +1,2 @@
+#!/bin/bash
+exit 0

tests/conformance/testdata/copychown/script2

@@ -0,0 +1,2 @@
+#!/bin/bash
+exit 1