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

rootless: allow single ID mappings #2604

Merged

Conversation

giuseppe
Copy link
Member

we were playing safe and not allowed any container to have less than 65536 mappings. There are a couple of reasons to change it:

  • it blocked libpod to work in an environment where newuidmap/newgidmap are not available, or not configured.

  • not allowed to use different partitions of subuids, where each user has less than 65536 ids available.

Hopefully this change in containers/storage:

containers/storage#303

will make error clearers if there are not enough IDs for the image that is being used.

Closes: #1651

Signed-off-by: Giuseppe Scrivano [email protected]

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 11, 2019
print an error if there is any failure pulling an image.

Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe giuseppe force-pushed the allow-single-mappings branch 2 times, most recently from 71bbfc8 to 360beae Compare March 11, 2019 10:44
@@ -237,46 +220,15 @@ func BecomeRootInUserNS() (bool, int, error) {
return false, -1, errors.Errorf("cannot re-exec process")
}

allowSingleIDMapping := os.Getenv("PODMAN_ALLOW_SINGLE_ID_MAPPING_IN_USERNS") != ""

var uids, gids []idtools.IDMap
username := os.Getenv("USER")
if username == "" {
user, err := user.LookupId(fmt.Sprintf("%d", os.Getuid()))
Copy link
Member

Choose a reason for hiding this comment

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

I'm going back and forth so I'll throw it up for consideration. Any value to doing a logru.debugf here and after line 231 to show the error that was hit?

Copy link
Member Author

Choose a reason for hiding this comment

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

that means the UID we are running with is not present in the users DB. I don't think it should matter, but I can add a Debugf if that can help

@TomSweeneyRedHat
Copy link
Member

One question for cosideration, otherwise LGTM

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, jamescassell

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

we were playing safe and not allowed any container to have less than
65536 mappings.  There are a couple of reasons to change it:

- it blocked libpod to work in an environment where
  newuidmap/newgidmap are not available, or not configured.

- not allowed to use different partitions of subuids, where each user
  has less than 65536 ids available.

Hopefully this change in containers/storage:

containers/storage#303

will make error clearers if there are not enough IDs for the image
that is being used.

Closes: containers#1651

Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe giuseppe force-pushed the allow-single-mappings branch from 360beae to af3e734 Compare March 11, 2019 13:38
@giuseppe
Copy link
Member Author

@jamescassell does the PR work for you?

@TomSweeneyRedHat Pushed a new version with the Debugf statement

@rhatdan
Copy link
Member

rhatdan commented Mar 11, 2019

LGTM

@mheon
Copy link
Member

mheon commented Mar 11, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 11, 2019
@openshift-merge-robot openshift-merge-robot merged commit 9b42577 into containers:master Mar 11, 2019
@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 27, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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
Development

Successfully merging this pull request may close these issues.

8 participants