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.

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

Jump to bottom

Add additionalGIDs from users in rootless mode #7871

Merged
merged 1 commit into from
Oct 2, 2020
Merged

Add additionalGIDs from users in rootless mode #7871

merged 1 commit into from
Oct 2, 2020

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Oct 1, 2020 •
edited
Loading

There is a risk here, that if the GID does not exists
within the User Namespace the container will fail to start.

This is only likely to happen in HPC Envioronments, and I think
we should add a field to disable it for this environment,
Added a FIXME for this issue.

We currently have this problem with running a rootfull container within
a user namespace, it will fail if the GID is not available.

I looked at potentially checking the usernamespace that you are assigned
to, but I believe this will be very difficult to code up and to figure out.

Fixes #7782

Signed-off-by: Daniel J Walsh [email protected]

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 1, 2020
@rhatdan
Add additionalGIDs from users in rootless mode
baede7c 
There is a risk here, that if the GID does not exists
within the User Namespace the container will fail to start.

This is only likely to happen in HPC Envioronments, and I think
we should add a field to disable it for this environment,
Added a FIXME for this issue.

We currently have this problem with running a rootfull container within
a user namespace, it will fail if the GID is not available.

I looked at potentially checking the usernamespace that you are assigned
to, but I believe this will be very difficult to code up and to figure out.

Signed-off-by: Daniel J Walsh <[email protected]>
@rhatdan
Copy link
Member Author

rhatdan commented Oct 1, 2020

@giuseppe PTAL

@giuseppe
Copy link
Member

giuseppe commented Oct 1, 2020

I had started looked at it, but haven't finished the patch yet.

Basically, we could reuse GetAvailableGids() from pkg/specgen/generate/oci.go to know how many gids are available. At least we can filter out gids that are outside of the 0 to GetAvailableGids() range

@rhatdan
Copy link
Member Author

rhatdan commented Oct 1, 2020

@giuseppe if you want to take this PR over, your welcome to it.

@rhatdan
Copy link
Member Author

rhatdan commented Oct 1, 2020

@containers/podman-maintainers PTAL
Lets get this in and then have @giuseppe add an update when he is ready.

@QiWang19
Copy link
Contributor

QiWang19 commented Oct 1, 2020

LGTM

@mheon
Copy link
Member

mheon commented Oct 1, 2020

LGTM as long as we don't cut a release before this is fixed.

@TomSweeneyRedHat
Copy link
Member

If we have more to do, can we add a TODO comment please?

@rhatdan
Copy link
Member Author

rhatdan commented Oct 1, 2020

There already is a FIXME in the code.

@TomSweeneyRedHat
Copy link
Member

@rhatdan , yeah there's a "FIXME", but not a "TODO". Dope slap here, I'll get myself before you get me. I'll add a /lgtm, but will do a /hold in case @mheon wants to wait on this by chance.

@TomSweeneyRedHat
Copy link
Member

/lgtm
/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 1, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 1, 2020
@QiWang19
Copy link
Contributor

QiWang19 commented Oct 2, 2020

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 2, 2020
@openshift-merge-robot openshift-merge-robot merged commit 54a9ecc into containers:master Oct 2, 2020
@giuseppe
Copy link
Member

giuseppe commented Oct 2, 2020

follow-up: #7882

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@TomSweeneyRedHat TomSweeneyRedHat

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Groups do not seem to be getting assigned by default in rootless mode versus rootful mode.
7 participants
@rhatdan @openshift-ci-robot @giuseppe @QiWang19 @mheon @TomSweeneyRedHat @openshift-merge-robot