Skip to content

fix running a container with config.User #1994

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
crosbymichael merged 4 commits into from
Jan 12, 2018
Merged

fix running a container with config.User #1994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1167035
ctr: promote cOpts over opts, as oci.WithImageConfig requires snapshot
AkihiroSuda Jan 11, 2018
369d7c9
oci: fix err variable scope
AkihiroSuda Jan 11, 2018
b99dc56
oci: add TODO comments
AkihiroSuda Jan 11, 2018
1645d84
oci: simplify WithImageConfig
AkihiroSuda Jan 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion cmd/ctr/commands/run/run_unix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,10 @@ func newContainer(ctx gocontext.Context, client *containerd.Client, context *cli
if context.Bool("net-host") {
opts = append(opts, oci.WithHostNamespace(specs.NetworkNamespace), oci.WithHostHostsFile, oci.WithHostResolvconf)
}
cOpts = append([]containerd.NewContainerOpts{containerd.WithNewSpec(opts...)}, cOpts...)
// oci.WithImageConfig (WithUsername, WithUserID) depends on rootfs snapshot for resolving /etc/passwd.
// So cOpts needs to have precedence over opts.
// TODO: WithUsername, WithUserID should additionally support non-snapshot rootfs
cOpts = append(cOpts, []containerd.NewContainerOpts{containerd.WithNewSpec(opts...)}...)
Copy link
Copy Markdown
Member

@estesp estesp Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this fixes ctr, but seems like this leaves a hole in the overall client API that currently doesn't say anything about option "precedence." Are there other cases where behavior is different based on ordering?

Copy link
Copy Markdown
Contributor

@mlaventure mlaventure Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@crosbymichael did you ever implement your idea to take care of this somewhere?

Copy link
Copy Markdown
Member

@crosbymichael crosbymichael Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats the problem with ctr, the ordering is what we made it. As far as other clients go, the supported usecase of the API, the ordering/precedence is what they define.

Copy link
Copy Markdown
Member

@estesp estesp Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess my point is slightly different; if I create a new client for containerd using the published Go API, there is nothing that tells me the way I order my 'opts' when using containerd's API for NewContainer could affect outcome (e.g. error versus working container). Do we need some note and/or, if we know the "valid" orderings, should we/can we fix it on the API implementation side.

Copy link
Copy Markdown
Member

@crosbymichael crosbymichael Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how we can fix that. Since opts can be created by the user the best I know to do is give them the information they need to setup things correctly.

Copy link
Copy Markdown
Contributor

@dnephin dnephin Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't be too difficult. The problem is the dependency is not explicit from the function signature. So we can fix that by changing it to:

func WithUserID(uid uint32, snapshotter snapshots.Snapshotter, rootfs string)

Of course this is more work to use, but it does make the dependencies obvious.

A change like this would also potentially allow us to remove client Client from the args in SpecOpt, which I think is a good thing. It's rarely used (these 2 functions are the only place it's referenced) and it introduces problems like this (where dependencies aren't clear).

Any SpecOpt that actually needs client (or something from client), can accept that as an arg to the function which returns the SpecOpt (like the WithUserID example above).

Copy link
Copy Markdown
Member

@crosbymichael crosbymichael Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dnephin good idea.

Copy link
Copy Markdown
Member Author

@AkihiroSuda AkihiroSuda Jan 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, but it should be probably separate PR for 1.1

return client.NewContainer(ctx, id, cOpts...)
}

Expand Down
24 changes: 12 additions & 12 deletions oci/spec_opts_unix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,22 +95,25 @@ func WithImageConfig(image Image) SpecOpts {
s.Process.Env = append(s.Process.Env, config.Env...)
cmd := config.Cmd
s.Process.Args = append(config.Entrypoint, cmd...)
cwd := config.WorkingDir
if cwd == "" {
cwd = "/"
}
s.Process.Cwd = cwd
if config.User != "" {
// According to OCI Image Spec v1.0.0, the following are valid for Linux:
// user, uid, user:group, uid:gid, uid:group, user:gid
parts := strings.Split(config.User, ":")
switch len(parts) {
case 1:
v, err := strconv.Atoi(parts[0])
if err != nil {
// if we cannot parse as a uint they try to see if it is a username
if err := WithUsername(config.User)(ctx, client, c, s); err != nil {
return err
}
return err
Copy link
Copy Markdown
Contributor

@dnephin dnephin Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this change, I think the old code may have been wrong as well.

If WithUsername is successful, shouldn't it continue to run the code after the switch (setting cwd) ?

Alternatively may this can be refactored so that the CWD code is above this switch, which would let us keep this early return, and also let us return WithUserID(...) below, instead of checking err there as well.

Copy link
Copy Markdown
Member Author

@AkihiroSuda AkihiroSuda Jan 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed, thanks

}
if err := WithUserID(uint32(v))(ctx, client, c, s); err != nil {
return err
return WithUsername(config.User)(ctx, client, c, s)
}
return WithUserID(uint32(v))(ctx, client, c, s)
case 2:
// TODO: support username and groupname
v, err := strconv.Atoi(parts[0])
if err != nil {
return errors.Wrapf(err, "parse uid %s", parts[0])
Expand All @@ -125,11 +128,6 @@ func WithImageConfig(image Image) SpecOpts {
return fmt.Errorf("invalid USER value %s", config.User)
}
}
cwd := config.WorkingDir
if cwd == "" {
cwd = "/"
}
s.Process.Cwd = cwd
return nil
}
}
Expand Down Expand Up @@ -259,6 +257,7 @@ func WithUIDGID(uid, gid uint32) SpecOpts {
// uid, and not returns error.
func WithUserID(uid uint32) SpecOpts {
return func(ctx context.Context, client Client, c *containers.Container, s *specs.Spec) (err error) {
// TODO: support non-snapshot rootfs
if c.Snapshotter == "" {
return errors.Errorf("no snapshotter set for container")
}
Expand Down Expand Up @@ -307,6 +306,7 @@ func WithUserID(uid uint32) SpecOpts {
// does not exist, or the username is not found in /etc/passwd,
// it returns error.
func WithUsername(username string) SpecOpts {
// TODO: support non-snapshot rootfs
return func(ctx context.Context, client Client, c *containers.Container, s *specs.Spec) (err error) {
if c.Snapshotter == "" {
return errors.Errorf("no snapshotter set for container")
Expand Down