This comes from umoci, and I thought this semantic change made sense (adding supplementary groups if someone explicitly used a numeric uid doesn't seem right). PTAL?

/cc @crosbymichael

LGTM

I think kube is a big users of sgroups so we need to make sure this is not going to affect something that they depend on.

LGTM. We can ping someone from the kube team if you want to verify before merging?

ping

@crosbymichael

It looks like they don't use libcontainer/user directly anymore, it's just used by github.com/docker/go-connections. So I reckon this is fine to merge, but we should probably ping @vishh.

% git grep 'libcontainer/user'
Godeps/Godeps.json:                     "ImportPath": "github.com/opencontainers/runc/libcontainer/user",
Godeps/LICENSES:= vendor/github.com/opencontainers/runc/libcontainer/user licensed under: =
vendor/github.com/docker/go-connections/sockets/BUILD:        "//vendor/github.com/opencontainers/runc/libcontainer/user:go_default_library",
vendor/github.com/docker/go-connections/sockets/unix_socket.go: "github.com/opencontainers/runc/libcontainer/user"
vendor/github.com/opencontainers/runc/libcontainer/BUILD:        "//vendor/github.com/opencontainers/runc/libcontainer/user:go_default_library",
vendor/github.com/opencontainers/runc/libcontainer/BUILD:        "//vendor/github.com/opencontainers/runc/libcontainer/user:all-srcs",
vendor/github.com/opencontainers/runc/libcontainer/init_linux.go:       "github.com/opencontainers/runc/libcontainer/user"

This is breaking my attempt to have docker use runc master atm.

I do not see the rational for depriving a user of its additional gids for the simple reason that it was referenced by its numeric ID instead of its human readable alias.

ping @cyphar @crosbymichael 😇

We only use numeric ids in LinuxKit, so this also breaks our ability to use supplemental groups. I really don't understand the rationale.

The concept of getting a supplementary group from /etc/passwd for a numeric ID doesn't make all that much sense (to me at least) -- not the least of which because there isn't a unique relationship from entry --> ID so it's possible that there's ambiguity about what supplementary groups a particular UID might have if they were referenced as a user.

To be fair however, sudo does appear to do the same thing:

% sudo -u '#1000' id
uid=1000(cyphar) gid=100(users) groups=100(users),10(wheel),16(dialout),469(libvirt),473(vboxusers),475(docker)

Maybe there is a justification for it...

In practice, there should only be a single entry for a given UID. And since standard tools seem to assume so, I think it'd make sense for this repo packages to do the same.

Bottom line, it would be nice if the maintainers could vote on reverting this PR 👼

I'm fine reverting this as it causes issues in practice and wasn't really fixing a problem in the first place.

Created a PR reverting this change #1548

Merged it. Thanks @mlaventure.