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

chown: use overflow id as fallback when chowning #1220

Merged
merged 3 commits into from Apr 28, 2022

Conversation

giuseppe
Copy link
Member

when chowning an image, fall back to the overflow ID when a UID or GID cannot be mapped to the target user namespace.

This ensures the chown driver works similar to what we do with idmapped mounts when it is supported for overlay.

It is needed for CRI-O to support user namespaces in Kubernetes since the Kubelet picks a static size for the user namespace and it might break some images using IDs outside the picked range.

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

@giuseppe giuseppe force-pushed the chown-to-overflow-id branch 3 times, most recently from 3c4d211 to 52b4926 Compare April 27, 2022 14:45
@rhatdan
Copy link
Member

rhatdan commented Apr 27, 2022

What happens if I have no UIDs in /etc/subuid, does this effectively do ignore_chown? Or is this different?

Copy link
Member

@rhatdan rhatdan left a comment

Choose a reason for hiding this comment

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

LGTM

@TomSweeneyRedHat
Copy link
Member

LGTM

@giuseppe
Copy link
Member Author

What happens if I have no UIDs in /etc/subuid, does this effectively do ignore_chown? Or is this different?

they happen in two different times.

ignore_chown is needed when a rootless user pulls an image in its initial user namespace (the podman unshare environment) and the image requires more IDs than are available for that user. So it won't be possible to represent it correctly in the local storage.

The use case I am trying to address here instead is when an image already present in the storage must be chowned before it can be used in a user namespace. In this case, both for rootful and rootless, the user specifies the user namespace mappings or size (through auto:size=N) and some IDs cannot be mapped to the target user namespace.

I think in this case we should not error out but do what the user asked to do. We need to fit the image in the specified mappings and if something cannot be mapped then use the overflow IDs.

@rhatdan
Copy link
Member

rhatdan commented Apr 27, 2022

Should we at least logrus.Warnf?

@giuseppe
Copy link
Member Author

Should we at least logrus.Warnf?

I'd expect that to be too verbose when used from CRI-O. Maybe Debugf?

@rhatdan
Copy link
Member

rhatdan commented Apr 28, 2022

Ok add debugf.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
add a new function ToHostOverflow() that instead of raising an error
when the mapping is not possible in the target user namespace, fall
back to using the overflow ID.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
when chowning an image, fall back to the overflow ID when a UID or GID
cannot be mapped to the target user namespace.

This ensures the chown driver works similar to what we do with
idmapped mounts when it is supported for overlay.

It is needed for CRI-O to support user namespaces in Kubernetes since
the Kubelet picks a static size for the user namespace and it might
break some images using IDs outside the picked range.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe
Copy link
Member Author

Ok add debugf.

added

@rhatdan rhatdan merged commit 754b868 into containers:main Apr 28, 2022
@nalind
Copy link
Member

nalind commented Apr 29, 2022

What's the intent of the graphtest: use unique names for the file system layers commit?

@giuseppe
Copy link
Member Author

giuseppe commented May 2, 2022

What's the intent of the graphtest: use unique names for the file system layers commit?

I've seen this test failing multiple times, and it wasn't clear where exactly it was failing, so the intent is to make it clearer.

If you don't like it, I can open a PR to revert the change

@nalind
Copy link
Member

nalind commented May 2, 2022

The intent wasn't documented, AFAICT, anywhere, which means there's nothing preventing another maintainer from concluding that it was done on a whim, and then quietly reverting it if we do another rebase onto from moby's version.

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.

None yet

4 participants