Merge pull request #41377 from thaJeztah/refactor_idtools

pkg/idtools: refactor to avoid string-splitting

pkg/idtools/idtools_unix.go

@@ -9,7 +9,6 @@ import (
 	"os"
 	"path/filepath"
 	"strconv"
-	"strings"
 	"sync"
 	"syscall"
 
@@ -107,14 +106,14 @@ func accessible(isOwner, isGroup bool, perms os.FileMode) bool {
 
 // LookupUser uses traditional local system files lookup (from libcontainer/user) on a username,
 // followed by a call to `getent` for supporting host configured non-files passwd and group dbs
-func LookupUser(username string) (user.User, error) {
+func LookupUser(name string) (user.User, error) {
 	// first try a local system files lookup using existing capabilities
-	usr, err := user.LookupUser(username)
+	usr, err := user.LookupUser(name)
 	if err == nil {
 		return usr, nil
 	}
 	// local files lookup failed; attempt to call `getent` to query configured passwd dbs
-	usr, err = getentUser(fmt.Sprintf("%s %s", "passwd", username))
+	usr, err = getentUser(name)
 	if err != nil {
 		return user.User{}, err
 	}
@@ -130,11 +129,11 @@ func LookupUID(uid int) (user.User, error) {
 		return usr, nil
 	}
 	// local files lookup failed; attempt to call `getent` to query configured passwd dbs
-	return getentUser(fmt.Sprintf("%s %d", "passwd", uid))
+	return getentUser(strconv.Itoa(uid))
 }
 
-func getentUser(args string) (user.User, error) {
+func getentUser(name string) (user.User, error) {
-	reader, err := callGetent(args)
+	reader, err := callGetent("passwd", name)
 	if err != nil {
 		return user.User{}, err
 	}
@@ -143,21 +142,21 @@ func getentUser(args string) (user.User, error) {
 		return user.User{}, err
 	}
 	if len(users) == 0 {
-		return user.User{}, fmt.Errorf("getent failed to find passwd entry for %q", strings.Split(args, " ")[1])
+		return user.User{}, fmt.Errorf("getent failed to find passwd entry for %q", name)
 	}
 	return users[0], nil
 }
 
 // LookupGroup uses traditional local system files lookup (from libcontainer/user) on a group name,
 // followed by a call to `getent` for supporting host configured non-files passwd and group dbs
-func LookupGroup(groupname string) (user.Group, error) {
+func LookupGroup(name string) (user.Group, error) {
 	// first try a local system files lookup using existing capabilities
-	group, err := user.LookupGroup(groupname)
+	group, err := user.LookupGroup(name)
 	if err == nil {
 		return group, nil
 	}
 	// local files lookup failed; attempt to call `getent` to query configured group dbs
-	return getentGroup(fmt.Sprintf("%s %s", "group", groupname))
+	return getentGroup(name)
 }
 
 // LookupGID uses traditional local system files lookup (from libcontainer/user) on a group ID,
@@ -169,11 +168,11 @@ func LookupGID(gid int) (user.Group, error) {
 		return group, nil
 	}
 	// local files lookup failed; attempt to call `getent` to query configured group dbs
-	return getentGroup(fmt.Sprintf("%s %d", "group", gid))
+	return getentGroup(strconv.Itoa(gid))
 }
 
-func getentGroup(args string) (user.Group, error) {
+func getentGroup(name string) (user.Group, error) {
-	reader, err := callGetent(args)
+	reader, err := callGetent("group", name)
 	if err != nil {
 		return user.Group{}, err
 	}
@@ -182,18 +181,18 @@ func getentGroup(args string) (user.Group, error) {
 		return user.Group{}, err
 	}
 	if len(groups) == 0 {
-		return user.Group{}, fmt.Errorf("getent failed to find groups entry for %q", strings.Split(args, " ")[1])
+		return user.Group{}, fmt.Errorf("getent failed to find groups entry for %q", name)
 	}
 	return groups[0], nil
 }
 
-func callGetent(args string) (io.Reader, error) {
+func callGetent(database, key string) (io.Reader, error) {
 	entOnce.Do(func() { getentCmd, _ = resolveBinary("getent") })
 	// if no `getent` command on host, can't do anything else
 	if getentCmd == "" {
-		return nil, fmt.Errorf("")
+		return nil, fmt.Errorf("unable to find getent command")
 	}
-	out, err := execCmd(getentCmd, args)
+	out, err := execCmd(getentCmd, database, key)
 	if err != nil {
 		exitCode, errC := system.GetExitCode(err)
 		if errC != nil {
@@ -203,8 +202,7 @@ func callGetent(args string) (io.Reader, error) {
 		case 1:
 			return nil, fmt.Errorf("getent reported invalid parameters/database unknown")
 		case 2:
-			terms := strings.Split(args, " ")
+			return nil, fmt.Errorf("getent unable to find entry %q in %s database", key, database)
-			return nil, fmt.Errorf("getent unable to find entry %q in %s database", terms[1], terms[0])
 		case 3:
 			return nil, fmt.Errorf("getent database doesn't support enumeration")
 		default:
@@ -235,19 +233,19 @@ func lazyChown(p string, uid, gid int, stat *system.StatT) error {
 // NewIdentityMapping takes a requested username and
 // using the data from /etc/sub{uid,gid} ranges, creates the
 // proper uid and gid remapping ranges for that user/group pair
-func NewIdentityMapping(username string) (*IdentityMapping, error) {
+func NewIdentityMapping(name string) (*IdentityMapping, error) {
-	usr, err := LookupUser(username)
+	usr, err := LookupUser(name)
 	if err != nil {
-		return nil, fmt.Errorf("Could not get user for username %s: %v", username, err)
+		return nil, fmt.Errorf("Could not get user for username %s: %v", name, err)
 	}
 
 	uid := strconv.Itoa(usr.Uid)
 
-	subuidRangesWithUserName, err := parseSubuid(username)
+	subuidRangesWithUserName, err := parseSubuid(name)
 	if err != nil {
 		return nil, err
 	}
-	subgidRangesWithUserName, err := parseSubgid(username)
+	subgidRangesWithUserName, err := parseSubgid(name)
 	if err != nil {
 		return nil, err
 	}
@@ -265,10 +263,10 @@ func NewIdentityMapping(username string) (*IdentityMapping, error) {
 	subgidRanges := append(subgidRangesWithUserName, subgidRangesWithUID...)
 
 	if len(subuidRanges) == 0 {
-		return nil, errors.Errorf("no subuid ranges found for user %q", username)
+		return nil, errors.Errorf("no subuid ranges found for user %q", name)
 	}
 	if len(subgidRanges) == 0 {
-		return nil, errors.Errorf("no subgid ranges found for user %q", username)
+		return nil, errors.Errorf("no subgid ranges found for user %q", name)
 	}
 
 	return &IdentityMapping{

pkg/idtools/usergroupadd_linux.go

@@ -17,18 +17,13 @@ import (
 var (
 	once        sync.Once
 	userCommand string
-
-	cmdTemplates = map[string]string{
-		"adduser": "--system --shell /bin/false --no-create-home --disabled-login --disabled-password --group %s",
-		"useradd": "-r -s /bin/false %s",
-		"usermod": "-%s %d-%d %s",
-	}
-
 	idOutRegexp = regexp.MustCompile(`uid=([0-9]+).*gid=([0-9]+)`)
+)
+
+const (
 	// default length for a UID/GID subordinate range
 	defaultRangeLen   = 65536
 	defaultRangeStart = 100000
-	userMod           = "usermod"
 )
 
 // AddNamespaceRangesUser takes a username and uses the standard system
@@ -67,7 +62,7 @@ func AddNamespaceRangesUser(name string) (int, int, error) {
 	return uid, gid, nil
 }
 
-func addUser(userName string) error {
+func addUser(name string) error {
 	once.Do(func() {
 		// set up which commands are used for adding users/groups dependent on distro
 		if _, err := resolveBinary("adduser"); err == nil {
@@ -76,13 +71,18 @@ func addUser(userName string) error {
 			userCommand = "useradd"
 		}
 	})
-	if userCommand == "" {
+	var args []string
-		return fmt.Errorf("Cannot add user; no useradd/adduser binary found")
+	switch userCommand {
+	case "adduser":
+		args = []string{"--system", "--shell", "/bin/false", "--no-create-home", "--disabled-login", "--disabled-password", "--group", name}
+	case "useradd":
+		args = []string{"-r", "-s", "/bin/false", name}
+	default:
+		return fmt.Errorf("cannot add user; no useradd/adduser binary found")
 	}
-	args := fmt.Sprintf(cmdTemplates[userCommand], userName)
+
-	out, err := execCmd(userCommand, args)
+	if out, err := execCmd(userCommand, args...); err != nil {
-	if err != nil {
+		return fmt.Errorf("failed to add user with error: %v; output: %q", err, string(out))
-		return fmt.Errorf("Failed to add user with error: %v; output: %q", err, string(out))
 	}
 	return nil
 }
@@ -101,7 +101,7 @@ func createSubordinateRanges(name string) error {
 		if err != nil {
 			return fmt.Errorf("Can't find available subuid range: %v", err)
 		}
-		out, err := execCmd(userMod, fmt.Sprintf(cmdTemplates[userMod], "v", startID, startID+defaultRangeLen-1, name))
+		out, err := execCmd("usermod", "-v", fmt.Sprintf("%d-%d", startID, startID+defaultRangeLen-1), name)
 		if err != nil {
 			return fmt.Errorf("Unable to add subuid range to user: %q; output: %s, err: %v", name, out, err)
 		}
@@ -117,7 +117,7 @@ func createSubordinateRanges(name string) error {
 		if err != nil {
 			return fmt.Errorf("Can't find available subgid range: %v", err)
 		}
-		out, err := execCmd(userMod, fmt.Sprintf(cmdTemplates[userMod], "w", startID, startID+defaultRangeLen-1, name))
+		out, err := execCmd("usermod", "-w", fmt.Sprintf("%d-%d", startID, startID+defaultRangeLen-1), name)
 		if err != nil {
 			return fmt.Errorf("Unable to add subgid range to user: %q; output: %s, err: %v", name, out, err)
 		}

pkg/idtools/utils_unix.go

@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"os/exec"
 	"path/filepath"
-	"strings"
 )
 
 func resolveBinary(binname string) (string, error) {
@@ -26,7 +25,7 @@ func resolveBinary(binname string) (string, error) {
 	return "", fmt.Errorf("Binary %q does not resolve to a binary of that name in $PATH (%q)", binname, resolvedPath)
 }
 
-func execCmd(cmd, args string) ([]byte, error) {
+func execCmd(cmd string, arg ...string) ([]byte, error) {
-	execCmd := exec.Command(cmd, strings.Split(args, " ")...)
+	execCmd := exec.Command(cmd, arg...)
 	return execCmd.CombinedOutput()
 }