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

overlay: drop check for mount_program AND force_mask #1970

Conversation

giuseppe
Copy link
Member

Using a mount_program is not a necessary requirement for users creating a shared store, as the store can be consumed by other users.

Stop enforcing this rule.

@giuseppe giuseppe force-pushed the drop-check-for-mount-program-and-force-mask branch from b0c1492 to 855c90c Compare June 13, 2024 13:15
@rhatdan
Copy link
Member

rhatdan commented Jun 15, 2024

LGTM
@saschagrunert @mtrmac @nalind PTAL

@mtrmac
Copy link
Collaborator

mtrmac commented Jun 17, 2024

Probably best if someone else reviews this…


Using a mount_program is not a necessary requirement for users creating a shared store, as the store can be consumed by other users.

I don’t understand how “shared store” and “mount_program” interact.

It seems to me that when creating a layer applying a diff, we can set the permissions ourselves while untarring. Fine.

What happens with read-write layers? AFAICS in that case we really need the filesystem driver (in-kernel or FUSE) to enforce the mask … but I also see nothing that passes the value to the mount operation.

So I don’t understand how any of this works even before this PR, and I’m just confused.

@giuseppe
Copy link
Member Author

What happens with read-write layers? AFAICS in that case we really need the filesystem driver (in-kernel or FUSE) to enforce the mask … but I also see nothing that passes the value to the mount operation.

yes a mount_program is still needed if you plan to use that same store, the issue though is that we require a mount_program even if the root user is not going to use that store to run containers, but only to share images to different users. The store can be made available also on a network file system, so users are not necessarily on the same machine. So we either require fuse-overlayfs to be installed, or provide a workaround with a dummy mount_program, since it doesn't matter when we pull an image.

@mtrmac
Copy link
Collaborator

mtrmac commented Jun 17, 2024

Would it make sense to refuse to create RW layers if force_mask is used without mount_program, then? (I don’t see how that actually works, I don’t know what I’m saying…)

giuseppe added 2 commits June 18, 2024 09:27
Using a mount_program is not a necessary requirement for users
creating a shared store, as the store can be consumed by other users.

Stop enforcing this rule for read-only layers, but check a
mount_program is specified when creating a container.

Signed-off-by: Giuseppe Scrivano <[email protected]>
Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe giuseppe force-pushed the drop-check-for-mount-program-and-force-mask branch from 855c90c to e6e9715 Compare June 18, 2024 07:28
@giuseppe
Copy link
Member Author

Would it make sense to refuse to create RW layers if force_mask is used without mount_program, then? (I don’t see how that actually works, I don’t know what I’m saying…)

sure we can do that. Added a check and submitted a new version

@cgwalters
Copy link
Contributor

Don't know this code really well, but looks sane to me
/approve

Copy link
Contributor

openshift-ci bot commented Jun 18, 2024

[APPROVALNOTIFIER] This PR is APPROVED

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

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

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

LGTM?

I don’t see how RW layers work with force_mask at all, but this is is at least not inconsistent with how I imagine it might be working :)

(I plan to leave this for 2 days, to give knowledgeable reviewers an opportunity, and then I default to merging.)

@mtrmac
Copy link
Collaborator

mtrmac commented Jun 20, 2024

/lgtm

as I threatened.

@openshift-ci openshift-ci bot added the lgtm label Jun 20, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit c3d0daa into containers:main Jun 20, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants