Skip to content
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.

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

add additional-gids to runc exec #1608

Merged
merged 3 commits into from
Oct 16, 2017
Merged

Conversation

crosbymichael
Copy link
Member

Closes #1307

This is a carry of #1307

This flag allows specifying additional gids for the process.
Without this flag, the user will have to provide process.json which allows additional gids.
Closes opencontainers#1306

Signed-off-by: Sumit Sanghrajka <sumit.sanghrajka@gmail.com>
Signed-off-by: Sumit Sanghrajka <sumit.sanghrajka@gmail.com>
@crosbymichael
Copy link
Member Author

@cyphar how should this work in userns? See travis failures for context.


@test "runc exec --additional-gids" {
# --user can't work in rootless containers that don't have idmap.
[[ "$ROOTLESS" -ne 0 ]] && requires rootless_idmap
Copy link
Member

Choose a reason for hiding this comment

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

--additional-gids cannot ever work in a rootless container, because setgroups is disabled. This should just be requires root. In this case, the specconv error is correct here.

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>
@crosbymichael
Copy link
Member Author

Ok, updated to skip that test when rootless is running. Good to review now

@cyphar
Copy link
Member

cyphar commented Oct 15, 2017

LGTM.

Approved with PullApprove

@cyphar
Copy link
Member

cyphar commented Oct 15, 2017

As an aside, we probably should add (privileged) user namespace integration tests for cases like this. But we can work on that in the future.

@mrunalp
Copy link
Contributor

mrunalp commented Oct 16, 2017

LGTM

Approved with PullApprove

@mrunalp mrunalp merged commit d5fc10a into opencontainers:master Oct 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants