Adds support for specifying additional groups.

[mrunalp]

See #9360 for why this is needed.
close #9360

This PR adds the functionality to specify additional groups that will be joined by the container process.
libcontainer support was added by docker-archive/libcontainer#322

[root@localhost ~]# /root/gosrc/src/github.com/docker/docker/bundles/1.5.0-dev/dynbinary/docker-1.5.0-dev run --rm -it --group-add='9999' --group-add=8888 busybox sh
/ # id
uid=0(root) gid=0(root) groups=10(wheel),8888,9999

I will add documents and tests once I get some initial feedback.

Signed-off-by: Mrunal Patel mrunalp@gmail.com

[thockin]

If you are just going to atoi() it, why not make it []int instead of []string in the first place? Accepting string means validation is more clumsy and it implies that accepting a group name is in the plans

[mrunalp]

@thockin I expect review comments that we should probably accept group names. If not, then I can modify the code to use []int all the way up the stack.

[thockin]

If we accept group names - are they from the host's groups file or the
container's? The latter is much more complicated, no?

On Wed, Feb 11, 2015 at 3:48 PM, Mrunal Patel notifications@github.com
wrote:

In daemon/execdriver/native/create.go
#10717 (comment):

@@ -222,3 +227,17 @@ func (d *driver) setupLabels(container *libcontainer.Config, c *execdriver.Comma

return nil

}
+
+func (d *driver) setupAdditionalGroups(container *libcontainer.Config, c *execdriver.Command) error {

• container.AdditionalGroups = make([]int, len(c.GroupAdd))

• for i, g := range c.GroupAdd {

•   group, err := strconv.Atoi(g)
  

@thockin https://github.com/thockin I expect review comments that we
should probably accept group names. If not, then I can modify the code to
use []int all the way up the stack.

Reply to this email directly or view it on GitHub
https://github.com/docker/docker/pull/10717/files#r24546022.

[mrunalp]

I think they would have to be from the host's groups file. They could be picked from the container but different containers could have same group names mapped to different gids and that kind of defeats the purpose here.

[thockin]

But the host's groups file is pretty meaningless inside a container,
especially one we have user ns.

I guess I am arguing simpler is better.

On Wed, Feb 11, 2015 at 4:01 PM, Mrunal Patel notifications@github.com
wrote:

In daemon/execdriver/native/create.go
#10717 (comment):

@@ -222,3 +227,17 @@ func (d *driver) setupLabels(container *libcontainer.Config, c *execdriver.Comma

return nil

}
+
+func (d *driver) setupAdditionalGroups(container *libcontainer.Config, c *execdriver.Command) error {

• container.AdditionalGroups = make([]int, len(c.GroupAdd))

• for i, g := range c.GroupAdd {

•   group, err := strconv.Atoi(g)
  

I think they would have to be from the host's groups file. They could be
picked from the container but different containers could have same group
names mapped to different gids and that kind of defeats the purpose here.

Reply to this email directly or view it on GitHub
https://github.com/docker/docker/pull/10717/files#r24546903.

[mrunalp]

@tianon @crosbymichael please take a look. Also, what do you think about accepting group names versus just gids here?

[tianon]

We've already set the precedent that group names are expected to work, and are expected to come from the container (-u user:group). This is exactly why I argued that the libcontainer endpoint for this feature ought top be a string just like user is, since it's libcontainer that handles converting user:group into uid+gid+supplementary.

[mrunalp]

@tianon @thockin I am not against changing this in libcontainer to look it up like we do for user. I thought that the original use case was around sharing volumes and passing group names couldn't guarantee that as same group name could map to different gids in different containers. If we agree one way or the other the changes could be made easily :)

[thockin]

The only use case I have is for numeric GIDs. As long as that works, I
don't care much if names are allowed or not. I was trying to limit
creature feep.

On Wed, Feb 11, 2015 at 7:11 PM, Mrunal Patel notifications@github.com
wrote:

@tianon https://github.com/tianon @thockin https://github.com/thockin
I am not against changing this in libcontainer to look it up like we do for
user. I thought that the original use case was around sharing volumes and
passing group names couldn't guarantee that as same group name could map to
different gids in different containers. If we agree one way or the other
the changes could be made easily :)

Reply to this email directly or view it on GitHub
#10717 (comment).

[icecrime]

I agree with the use case, but I'd rather not add another command line flag.

What do you think about extending -u for this? So -u|--user could accept values of the form uid[:gid[:extragrp1,extragrp2,extragrp3]].

[thockin]

So we have to spec the uid in order to spec the gid? That's a bit clunky,
IMO. Not unworkable, but meh.
On Mar 6, 2015 5:57 PM, "Arnaud Porterie" notifications@github.com wrote:

I agree with the use case, but I'd not add another command line flag.

What do you think about extending -u for this? So -u|--user could accept
values of the form uid[:gid[:extragrp1,extragrp2,extragrp3]].

—
Reply to this email directly or view it on GitHub
#10717 (comment).

[mrunalp]

@icecrime I agree with @thockin that this will require passing the user even when generally not necessary. However, if it gets this moving, I will work on an updated PR.

[tianon]

IMO the additional flag is a decent way to expose this feature, but I'm not the final say 👍

[tianon]

(but I do still think that optionally supporting named groups will be necessary for this to fly, since that's the interface we already expose via -u user:group, -u uid:group, -u user:gid, -u uid:gid)

[icecrime]

@thockin Meh indeed, I didn't think of that...
@crosbymichael Any opinion? Go for the flag?

[LK4D4]

Flag is okay for me. Names for groups makes sense.

[LK4D4]

ping @icecrime @tiborvass @tianon for decision and moving to code-review.

[tianon]

+1 for --group-add flag, and +1 for it supporting named groups as well as numeric groups for consistency

[icecrime]

Go for the flag, as described by @tianon: pushing to 2-needs-code-review.

[mrunalp]

Not clear where we agreed the lookup will be done though? On the host or in the container?

[tianon]

In the container, just like arguments to "-u/--user" are. 👍

[saad-ali]

Any traction on this?

[mrunalp]

@saad-ali Need to check with @thockin who opened the original issue. @thockin WDYT about lookup in the container?

[thockin]

Anyone who wants full control will just use numeric values, I guess, so I
don't much care where lookup happens (we won't use it :). In-container
makes sense.

On Mon, Apr 27, 2015 at 5:44 PM, Mrunal Patel notifications@github.com
wrote:

@saad-ali https://github.com/saad-ali Need to check with @thockin
https://github.com/thockin who opened the original isse. @thockin
https://github.com/thockin WDYT about lookup in the container?

—
Reply to this email directly or view it on GitHub
#10717 (comment).

[mrunalp]

Alright, everyone agrees :) I will update libcontainer to change the lookup and send an update PR.

[mrunalp]

I have a PR open against libcontainer for the lookup in the container.

[LK4D4]

ping @mrunalp

[mrunalp]

Updated the PR. I believe the janky failure is a teardown issue and not related to the changes in the PR.
I am working on updating the docs. Meanwhile comments on the code welcome.

[cpuguy83]

LGTM

[mrunalp]

Docs added as well. Also, all the builds passed :)

[cpuguy83]

Oh but rebase is required now :(

[mrunalp]

@cpuguy83 Pushed rebase.

[mrunalp]

teardown issue on janky again.

[mrunalp]

experimental failures look like they are unrelated to this PR.

[mrunalp]

@LK4D4 @icecrime Ready for review :)

[cpuguy83]

@mrunalp and another rebase, sorry.

[mrunalp]

Rebased.

[icecrime]

LGTM

Ping @ewindisch @NathanMcCauley.

[mrunalp]

Rebased.

[jessfraz]

that --group-add audio im sooooo using that LGTM

[mrunalp]

@jfrazelle Thanks for the merge :)

[thaJeztah]

Oh wow, merged! Thanks for being so patient @mrunalp!

[jglick]

Beware that if the Dockerfile has a USER someone directive, and you run with --group-add 999, id from the ENTRYPOINT will correctly include group 999, but if you docker-enter the container and su - jenkins, id will not include it (I presume since /etc/group is untouched). In other words, the feature works, but your attempts to debug problems with it may be confusing.

[cpuguy83]

@jglick Why not use docker exec?