From da38ac6c79fe902ed0687afc73d731c95c6d491a Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Thu, 31 Mar 2016 08:25:51 +1100 Subject: [PATCH] vendor: bump runc to 2441732d6fcc0fb0a542671a4372e0c7bc99c19e Also modify an integration test that hardcoded the error string so it uses the exported error variable from libcontainer/user. Signed-off-by: Aleksa Sarai --- hack/vendor.sh | 2 +- integration-cli/docker_cli_run_test.go | 5 +- .../runc/libcontainer/cgroups/cgroups.go | 2 +- .../runc/libcontainer/cgroups/stats.go | 9 +- .../runc/libcontainer/cgroups/utils.go | 2 +- .../runc/libcontainer/configs/cgroup_unix.go | 3 + .../runc/libcontainer/configs/config.go | 81 +++++++++- .../configs/namespaces_syscall.go | 2 +- .../configs/namespaces_syscall_unsupported.go | 2 +- .../runc/libcontainer/label/label.go | 2 +- .../runc/libcontainer/system/linux.go | 26 +++ .../runc/libcontainer/user/lookup.go | 8 +- .../runc/libcontainer/user/user.go | 149 ++++++++++-------- 13 files changed, 209 insertions(+), 84 deletions(-) diff --git a/hack/vendor.sh b/hack/vendor.sh index ba249342c69..f7195125ad7 100755 --- a/hack/vendor.sh +++ b/hack/vendor.sh @@ -59,7 +59,7 @@ clone git github.com/miekg/pkcs11 df8ae6ca730422dba20c768ff38ef7d79077a59f clone git github.com/docker/go v1.5.1-1-1-gbaf439e clone git github.com/agl/ed25519 d2b94fd789ea21d12fac1a4443dd3a3f79cda72c -clone git github.com/opencontainers/runc 7b6c4c418d5090f4f11eee949fdf49afd15838c9 # libcontainer +clone git github.com/opencontainers/runc 2441732d6fcc0fb0a542671a4372e0c7bc99c19e # libcontainer clone git github.com/opencontainers/specs 3ce138b1934bf227a418e241ead496c383eaba1c # specs clone git github.com/seccomp/libseccomp-golang 1b506fc7c24eec5a3693cdcbed40d9c226cfc6a1 # libcontainer deps (see src/github.com/opencontainers/runc/Godeps/Godeps.json) diff --git a/integration-cli/docker_cli_run_test.go b/integration-cli/docker_cli_run_test.go index c19603d9646..415e5a5ea3c 100644 --- a/integration-cli/docker_cli_run_test.go +++ b/integration-cli/docker_cli_run_test.go @@ -26,6 +26,7 @@ import ( "github.com/docker/libnetwork/netutils" "github.com/docker/libnetwork/resolvconf" "github.com/go-check/check" + libcontainerUser "github.com/opencontainers/runc/libcontainer/user" ) // "test123" should be printed by docker run @@ -707,7 +708,7 @@ func (s *DockerSuite) TestRunUserByIDBig(c *check.C) { if err == nil { c.Fatal("No error, but must be.", out) } - if !strings.Contains(out, "Uids and gids must be in range") { + if !strings.Contains(out, libcontainerUser.ErrRange.Error()) { c.Fatalf("expected error about uids range, got %s", out) } } @@ -720,7 +721,7 @@ func (s *DockerSuite) TestRunUserByIDNegative(c *check.C) { if err == nil { c.Fatal("No error, but must be.", out) } - if !strings.Contains(out, "Uids and gids must be in range") { + if !strings.Contains(out, libcontainerUser.ErrRange.Error()) { c.Fatalf("expected error about uids range, got %s", out) } } diff --git a/vendor/src/github.com/opencontainers/runc/libcontainer/cgroups/cgroups.go b/vendor/src/github.com/opencontainers/runc/libcontainer/cgroups/cgroups.go index c8f7796567b..274ab47dd83 100644 --- a/vendor/src/github.com/opencontainers/runc/libcontainer/cgroups/cgroups.go +++ b/vendor/src/github.com/opencontainers/runc/libcontainer/cgroups/cgroups.go @@ -9,7 +9,7 @@ import ( ) type Manager interface { - // Apply cgroup configuration to the process with the specified pid + // Applies cgroup configuration to the process with the specified pid Apply(pid int) error // Returns the PIDs inside the cgroup set diff --git a/vendor/src/github.com/opencontainers/runc/libcontainer/cgroups/stats.go b/vendor/src/github.com/opencontainers/runc/libcontainer/cgroups/stats.go index a9a07434733..797a923c388 100644 --- a/vendor/src/github.com/opencontainers/runc/libcontainer/cgroups/stats.go +++ b/vendor/src/github.com/opencontainers/runc/libcontainer/cgroups/stats.go @@ -47,13 +47,18 @@ type MemoryStats struct { // usage of memory + swap SwapUsage MemoryData `json:"swap_usage,omitempty"` // usage of kernel memory - KernelUsage MemoryData `json:"kernel_usage,omitempty"` - Stats map[string]uint64 `json:"stats,omitempty"` + KernelUsage MemoryData `json:"kernel_usage,omitempty"` + // usage of kernel TCP memory + KernelTCPUsage MemoryData `json:"kernel_tcp_usage,omitempty"` + + Stats map[string]uint64 `json:"stats,omitempty"` } type PidsStats struct { // number of pids in the cgroup Current uint64 `json:"current,omitempty"` + // active pids hard limit + Limit uint64 `json:"limit,omitempty"` } type BlkioStatEntry struct { diff --git a/vendor/src/github.com/opencontainers/runc/libcontainer/cgroups/utils.go b/vendor/src/github.com/opencontainers/runc/libcontainer/cgroups/utils.go index 006800dcfae..235273299fa 100644 --- a/vendor/src/github.com/opencontainers/runc/libcontainer/cgroups/utils.go +++ b/vendor/src/github.com/opencontainers/runc/libcontainer/cgroups/utils.go @@ -326,7 +326,7 @@ func RemovePaths(paths map[string]string) (err error) { return nil } } - return fmt.Errorf("Failed to remove paths: %s", paths) + return fmt.Errorf("Failed to remove paths: %v", paths) } func GetHugePageSize() ([]string, error) { diff --git a/vendor/src/github.com/opencontainers/runc/libcontainer/configs/cgroup_unix.go b/vendor/src/github.com/opencontainers/runc/libcontainer/configs/cgroup_unix.go index 2ea00658f51..f2eff91cf45 100644 --- a/vendor/src/github.com/opencontainers/runc/libcontainer/configs/cgroup_unix.go +++ b/vendor/src/github.com/opencontainers/runc/libcontainer/configs/cgroup_unix.go @@ -56,6 +56,9 @@ type Resources struct { // Kernel memory limit (in bytes) KernelMemory int64 `json:"kernel_memory"` + // Kernel memory limit for TCP use (in bytes) + KernelMemoryTCP int64 `json:"kernel_memory_tcp"` + // CPU shares (relative weight vs. other containers) CpuShares int64 `json:"cpu_shares"` diff --git a/vendor/src/github.com/opencontainers/runc/libcontainer/configs/config.go b/vendor/src/github.com/opencontainers/runc/libcontainer/configs/config.go index 668fa5e30f1..f83f638e3fa 100644 --- a/vendor/src/github.com/opencontainers/runc/libcontainer/configs/config.go +++ b/vendor/src/github.com/opencontainers/runc/libcontainer/configs/config.go @@ -3,7 +3,11 @@ package configs import ( "bytes" "encoding/json" + "fmt" "os/exec" + "time" + + "github.com/Sirupsen/logrus" ) type Rlimit struct { @@ -136,7 +140,7 @@ type Config struct { // Rlimits specifies the resource limits, such as max open files, to set in the container // If Rlimits are not set, the container will inherit rlimits from the parent process - Rlimits []Rlimit `json:"rlimits"` + Rlimits []Rlimit `json:"rlimits,omitempty"` // OomScoreAdj specifies the adjustment to be made by the kernel when calculating oom scores // for a process. Valid values are between the range [-1000, '1000'], where processes with @@ -175,8 +179,8 @@ type Config struct { NoNewPrivileges bool `json:"no_new_privileges,omitempty"` // Hooks are a collection of actions to perform at various container lifecycle events. - // Hooks are not able to be marshaled to json but they are also not needed to. - Hooks *Hooks `json:"-"` + // CommandHooks are serialized to JSON, but other hooks are not. + Hooks *Hooks // Version is the version of opencontainer specification that is supported. Version string `json:"version"` @@ -197,6 +201,52 @@ type Hooks struct { Poststop []Hook } +func (hooks *Hooks) UnmarshalJSON(b []byte) error { + var state struct { + Prestart []CommandHook + Poststart []CommandHook + Poststop []CommandHook + } + + if err := json.Unmarshal(b, &state); err != nil { + return err + } + + deserialize := func(shooks []CommandHook) (hooks []Hook) { + for _, shook := range shooks { + hooks = append(hooks, shook) + } + + return hooks + } + + hooks.Prestart = deserialize(state.Prestart) + hooks.Poststart = deserialize(state.Poststart) + hooks.Poststop = deserialize(state.Poststop) + return nil +} + +func (hooks Hooks) MarshalJSON() ([]byte, error) { + serialize := func(hooks []Hook) (serializableHooks []CommandHook) { + for _, hook := range hooks { + switch chook := hook.(type) { + case CommandHook: + serializableHooks = append(serializableHooks, chook) + default: + logrus.Warnf("cannot serialize hook of type %T, skipping", hook) + } + } + + return serializableHooks + } + + return json.Marshal(map[string]interface{}{ + "prestart": serialize(hooks.Prestart), + "poststart": serialize(hooks.Poststart), + "poststop": serialize(hooks.Poststop), + }) +} + // HookState is the payload provided to a hook on execution. type HookState struct { Version string `json:"version"` @@ -226,10 +276,11 @@ func (f FuncHook) Run(s HookState) error { } type Command struct { - Path string `json:"path"` - Args []string `json:"args"` - Env []string `json:"env"` - Dir string `json:"dir"` + Path string `json:"path"` + Args []string `json:"args"` + Env []string `json:"env"` + Dir string `json:"dir"` + Timeout *time.Duration `json:"timeout"` } // NewCommandHooks will execute the provided command when the hook is run. @@ -254,5 +305,19 @@ func (c Command) Run(s HookState) error { Env: c.Env, Stdin: bytes.NewReader(b), } - return cmd.Run() + errC := make(chan error, 1) + go func() { + errC <- cmd.Run() + }() + if c.Timeout != nil { + select { + case err := <-errC: + return err + case <-time.After(*c.Timeout): + cmd.Process.Kill() + cmd.Wait() + return fmt.Errorf("hook ran past specified timeout of %.1fs", c.Timeout.Seconds()) + } + } + return <-errC } diff --git a/vendor/src/github.com/opencontainers/runc/libcontainer/configs/namespaces_syscall.go b/vendor/src/github.com/opencontainers/runc/libcontainer/configs/namespaces_syscall.go index c962999efd4..fb4b8522223 100644 --- a/vendor/src/github.com/opencontainers/runc/libcontainer/configs/namespaces_syscall.go +++ b/vendor/src/github.com/opencontainers/runc/libcontainer/configs/namespaces_syscall.go @@ -18,7 +18,7 @@ var namespaceInfo = map[NamespaceType]int{ } // CloneFlags parses the container's Namespaces options to set the correct -// flags on clone, unshare. This functions returns flags only for new namespaces. +// flags on clone, unshare. This function returns flags only for new namespaces. func (n *Namespaces) CloneFlags() uintptr { var flag int for _, v := range *n { diff --git a/vendor/src/github.com/opencontainers/runc/libcontainer/configs/namespaces_syscall_unsupported.go b/vendor/src/github.com/opencontainers/runc/libcontainer/configs/namespaces_syscall_unsupported.go index 1644588dc79..0547223a9db 100644 --- a/vendor/src/github.com/opencontainers/runc/libcontainer/configs/namespaces_syscall_unsupported.go +++ b/vendor/src/github.com/opencontainers/runc/libcontainer/configs/namespaces_syscall_unsupported.go @@ -8,7 +8,7 @@ func (n *Namespace) Syscall() int { } // CloneFlags parses the container's Namespaces options to set the correct -// flags on clone, unshare. This functions returns flags only for new namespaces. +// flags on clone, unshare. This function returns flags only for new namespaces. func (n *Namespaces) CloneFlags() uintptr { panic("No namespace syscall support") return uintptr(0) diff --git a/vendor/src/github.com/opencontainers/runc/libcontainer/label/label.go b/vendor/src/github.com/opencontainers/runc/libcontainer/label/label.go index 97dc6baef4c..fedbae4f609 100644 --- a/vendor/src/github.com/opencontainers/runc/libcontainer/label/label.go +++ b/vendor/src/github.com/opencontainers/runc/libcontainer/label/label.go @@ -48,7 +48,7 @@ func UnreserveLabel(label string) error { return nil } -// DupSecOpt takes an process label and returns security options that +// DupSecOpt takes a process label and returns security options that // can be used to set duplicate labels on future container processes func DupSecOpt(src string) []string { return nil diff --git a/vendor/src/github.com/opencontainers/runc/libcontainer/system/linux.go b/vendor/src/github.com/opencontainers/runc/libcontainer/system/linux.go index babf55048b8..8b199d92ed5 100644 --- a/vendor/src/github.com/opencontainers/runc/libcontainer/system/linux.go +++ b/vendor/src/github.com/opencontainers/runc/libcontainer/system/linux.go @@ -11,6 +11,19 @@ import ( "unsafe" ) +// If arg2 is nonzero, set the "child subreaper" attribute of the +// calling process; if arg2 is zero, unset the attribute. When a +// process is marked as a child subreaper, all of the children +// that it creates, and their descendants, will be marked as +// having a subreaper. In effect, a subreaper fulfills the role +// of init(1) for its descendant processes. Upon termination of +// a process that is orphaned (i.e., its immediate parent has +// already terminated) and marked as having a subreaper, the +// nearest still living ancestor subreaper will receive a SIGCHLD +// signal and be able to wait(2) on the process to discover its +// termination status. +const PR_SET_CHILD_SUBREAPER = 36 + type ParentDeathSignal int func (p ParentDeathSignal) Restore() error { @@ -40,6 +53,14 @@ func Execv(cmd string, args []string, env []string) error { return syscall.Exec(name, args, env) } +func Prlimit(pid, resource int, limit syscall.Rlimit) error { + _, _, err := syscall.RawSyscall6(syscall.SYS_PRLIMIT64, uintptr(pid), uintptr(resource), uintptr(unsafe.Pointer(&limit)), uintptr(unsafe.Pointer(&limit)), 0, 0) + if err != 0 { + return err + } + return nil +} + func SetParentDeathSignal(sig uintptr) error { if _, _, err := syscall.RawSyscall(syscall.SYS_PRCTL, syscall.PR_SET_PDEATHSIG, sig, 0); err != 0 { return err @@ -113,6 +134,11 @@ func RunningInUserNS() bool { return true } +// SetSubreaper sets the value i as the subreaper setting for the calling process +func SetSubreaper(i int) error { + return Prctl(PR_SET_CHILD_SUBREAPER, uintptr(i), 0, 0, 0) +} + func Prctl(option int, arg2, arg3, arg4, arg5 uintptr) (err error) { _, _, e1 := syscall.Syscall6(syscall.SYS_PRCTL, uintptr(option), arg2, arg3, arg4, arg5, 0) if e1 != 0 { diff --git a/vendor/src/github.com/opencontainers/runc/libcontainer/user/lookup.go b/vendor/src/github.com/opencontainers/runc/libcontainer/user/lookup.go index 6f8a982ff72..ab1439f3614 100644 --- a/vendor/src/github.com/opencontainers/runc/libcontainer/user/lookup.go +++ b/vendor/src/github.com/opencontainers/runc/libcontainer/user/lookup.go @@ -2,13 +2,15 @@ package user import ( "errors" - "fmt" "syscall" ) var ( // The current operating system does not provide the required data for user lookups. ErrUnsupported = errors.New("user lookup: operating system does not provide passwd-formatted data") + // No matching entries found in file. + ErrNoPasswdEntries = errors.New("no matching entries in passwd file") + ErrNoGroupEntries = errors.New("no matching entries in group file") ) func lookupUser(filter func(u User) bool) (User, error) { @@ -27,7 +29,7 @@ func lookupUser(filter func(u User) bool) (User, error) { // No user entries found. if len(users) == 0 { - return User{}, fmt.Errorf("no matching entries in passwd file") + return User{}, ErrNoPasswdEntries } // Assume the first entry is the "correct" one. @@ -75,7 +77,7 @@ func lookupGroup(filter func(g Group) bool) (Group, error) { // No user entries found. if len(groups) == 0 { - return Group{}, fmt.Errorf("no matching entries in group file") + return Group{}, ErrNoGroupEntries } // Assume the first entry is the "correct" one. diff --git a/vendor/src/github.com/opencontainers/runc/libcontainer/user/user.go b/vendor/src/github.com/opencontainers/runc/libcontainer/user/user.go index e6375ea4dd5..43fd39ef54d 100644 --- a/vendor/src/github.com/opencontainers/runc/libcontainer/user/user.go +++ b/vendor/src/github.com/opencontainers/runc/libcontainer/user/user.go @@ -15,7 +15,7 @@ const ( ) var ( - ErrRange = fmt.Errorf("Uids and gids must be in range %d-%d", minId, maxId) + ErrRange = fmt.Errorf("uids and gids must be in range %d-%d", minId, maxId) ) type User struct { @@ -42,29 +42,30 @@ func parseLine(line string, v ...interface{}) { parts := strings.Split(line, ":") for i, p := range parts { + // Ignore cases where we don't have enough fields to populate the arguments. + // Some configuration files like to misbehave. if len(v) <= i { - // if we have more "parts" than we have places to put them, bail for great "tolerance" of naughty configuration files break } + // Use the type of the argument to figure out how to parse it, scanf() style. + // This is legit. switch e := v[i].(type) { case *string: - // "root", "adm", "/bin/bash" *e = p case *int: - // "0", "4", "1000" - // ignore string to int conversion errors, for great "tolerance" of naughty configuration files + // "numbers", with conversion errors ignored because of some misbehaving configuration files. *e, _ = strconv.Atoi(p) case *[]string: - // "", "root", "root,adm,daemon" + // Comma-separated lists. if p != "" { *e = strings.Split(p, ",") } else { *e = []string{} } default: - // panic, because this is a programming/logic error, not a runtime one - panic("parseLine expects only pointers! argument " + strconv.Itoa(i) + " is not a pointer!") + // Someone goof'd when writing code using this function. Scream so they can hear us. + panic(fmt.Sprintf("parseLine only accepts {*string, *int, *[]string} as arguments! %#v is not a pointer!", e)) } } } @@ -106,8 +107,8 @@ func ParsePasswdFilter(r io.Reader, filter func(User) bool) ([]User, error) { return nil, err } - text := strings.TrimSpace(s.Text()) - if text == "" { + line := strings.TrimSpace(s.Text()) + if line == "" { continue } @@ -117,10 +118,7 @@ func ParsePasswdFilter(r io.Reader, filter func(User) bool) ([]User, error) { // root:x:0:0:root:/root:/bin/bash // adm:x:3:4:adm:/var/adm:/bin/false p := User{} - parseLine( - text, - &p.Name, &p.Pass, &p.Uid, &p.Gid, &p.Gecos, &p.Home, &p.Shell, - ) + parseLine(line, &p.Name, &p.Pass, &p.Uid, &p.Gid, &p.Gecos, &p.Home, &p.Shell) if filter == nil || filter(p) { out = append(out, p) @@ -135,6 +133,7 @@ func ParseGroupFile(path string) ([]Group, error) { if err != nil { return nil, err } + defer group.Close() return ParseGroup(group) } @@ -178,10 +177,7 @@ func ParseGroupFilter(r io.Reader, filter func(Group) bool) ([]Group, error) { // root:x:0:root // adm:x:4:root,adm,daemon p := Group{} - parseLine( - text, - &p.Name, &p.Pass, &p.Gid, &p.List, - ) + parseLine(text, &p.Name, &p.Pass, &p.Gid, &p.List) if filter == nil || filter(p) { out = append(out, p) @@ -192,9 +188,10 @@ func ParseGroupFilter(r io.Reader, filter func(Group) bool) ([]Group, error) { } type ExecUser struct { - Uid, Gid int - Sgids []int - Home string + Uid int + Gid int + Sgids []int + Home string } // GetExecUserPath is a wrapper for GetExecUser. It reads data from each of the @@ -235,12 +232,12 @@ func GetExecUserPath(userSpec string, defaults *ExecUser, passwdPath, groupPath // * "uid:gid // * "user:gid" // * "uid:group" +// +// It should be noted that if you specify a numeric user or group id, they will +// not be evaluated as usernames (only the metadata will be filled). So attempting +// to parse a user with user.Name = "1337" will produce the user with a UID of +// 1337. func GetExecUser(userSpec string, defaults *ExecUser, passwd, group io.Reader) (*ExecUser, error) { - var ( - userArg, groupArg string - name string - ) - if defaults == nil { defaults = new(ExecUser) } @@ -258,87 +255,113 @@ func GetExecUser(userSpec string, defaults *ExecUser, passwd, group io.Reader) ( user.Sgids = []int{} } - // allow for userArg to have either "user" syntax, or optionally "user:group" syntax + // Allow for userArg to have either "user" syntax, or optionally "user:group" syntax + var userArg, groupArg string parseLine(userSpec, &userArg, &groupArg) + // Convert userArg and groupArg to be numeric, so we don't have to execute + // Atoi *twice* for each iteration over lines. + uidArg, uidErr := strconv.Atoi(userArg) + gidArg, gidErr := strconv.Atoi(groupArg) + + // Find the matching user. users, err := ParsePasswdFilter(passwd, func(u User) bool { if userArg == "" { + // Default to current state of the user. return u.Uid == user.Uid } - return u.Name == userArg || strconv.Itoa(u.Uid) == userArg + + if uidErr == nil { + // If the userArg is numeric, always treat it as a UID. + return uidArg == u.Uid + } + + return u.Name == userArg }) + + // If we can't find the user, we have to bail. if err != nil && passwd != nil { if userArg == "" { userArg = strconv.Itoa(user.Uid) } - return nil, fmt.Errorf("Unable to find user %v: %v", userArg, err) + return nil, fmt.Errorf("unable to find user %s: %v", userArg, err) } - haveUser := users != nil && len(users) > 0 - if haveUser { - // if we found any user entries that matched our filter, let's take the first one as "correct" - name = users[0].Name + var matchedUserName string + if len(users) > 0 { + // First match wins, even if there's more than one matching entry. + matchedUserName = users[0].Name user.Uid = users[0].Uid user.Gid = users[0].Gid user.Home = users[0].Home } else if userArg != "" { - // we asked for a user but didn't find them... let's check to see if we wanted a numeric user - user.Uid, err = strconv.Atoi(userArg) - if err != nil { - // not numeric - we have to bail - return nil, fmt.Errorf("Unable to find user %v", userArg) + // If we can't find a user with the given username, the only other valid + // option is if it's a numeric username with no associated entry in passwd. + + if uidErr != nil { + // Not numeric. + return nil, fmt.Errorf("unable to find user %s: %v", userArg, ErrNoPasswdEntries) } + user.Uid = uidArg // Must be inside valid uid range. if user.Uid < minId || user.Uid > maxId { return nil, ErrRange } - // if userArg couldn't be found in /etc/passwd but is numeric, just roll with it - this is legit + // Okay, so it's numeric. We can just roll with this. } - if groupArg != "" || name != "" { + // On to the groups. If we matched a username, we need to do this because of + // the supplementary group IDs. + if groupArg != "" || matchedUserName != "" { groups, err := ParseGroupFilter(group, func(g Group) bool { - // Explicit group format takes precedence. - if groupArg != "" { - return g.Name == groupArg || strconv.Itoa(g.Gid) == groupArg + // If the group argument isn't explicit, we'll just search for it. + if groupArg == "" { + // Check if user is a member of this group. + for _, u := range g.List { + if u == matchedUserName { + return true + } + } + return false } - // Check if user is a member. - for _, u := range g.List { - if u == name { - return true - } + if gidErr == nil { + // If the groupArg is numeric, always treat it as a GID. + return gidArg == g.Gid } - return false + return g.Name == groupArg }) if err != nil && group != nil { - return nil, fmt.Errorf("Unable to find groups for user %v: %v", users[0].Name, err) + return nil, fmt.Errorf("unable to find groups for spec %v: %v", matchedUserName, err) } - haveGroup := groups != nil && len(groups) > 0 + // Only start modifying user.Gid if it is in explicit form. if groupArg != "" { - if haveGroup { - // if we found any group entries that matched our filter, let's take the first one as "correct" + if len(groups) > 0 { + // First match wins, even if there's more than one matching entry. user.Gid = groups[0].Gid - } else { - // we asked for a group but didn't find id... let's check to see if we wanted a numeric group - user.Gid, err = strconv.Atoi(groupArg) - if err != nil { - // not numeric - we have to bail - return nil, fmt.Errorf("Unable to find group %v", groupArg) + } else if groupArg != "" { + // If we can't find a group with the given name, the only other valid + // option is if it's a numeric group name with no associated entry in group. + + if gidErr != nil { + // Not numeric. + return nil, fmt.Errorf("unable to find group %s: %v", groupArg, ErrNoGroupEntries) } + user.Gid = gidArg - // Ensure gid is inside gid range. + // Must be inside valid gid range. if user.Gid < minId || user.Gid > maxId { return nil, ErrRange } - // if groupArg couldn't be found in /etc/group but is numeric, just roll with it - this is legit + // Okay, so it's numeric. We can just roll with this. } - } else if haveGroup { - // If implicit group format, fill supplementary gids. + } else if len(groups) > 0 { + // Supplementary group ids only make sense if in the implicit form. user.Sgids = make([]int, len(groups)) for i, group := range groups { user.Sgids[i] = group.Gid