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

Run logout,manifest,source,unmount without userns #5752

Closed
wants to merge 1 commit into from

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Sep 23, 2024

Fixes: #5750

What type of PR is this?

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake
/kind other

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?


@rhatdan rhatdan added the No New Tests Allow PR to proceed without adding regression tests label Sep 23, 2024
Copy link
Contributor

openshift-ci bot commented Sep 23, 2024

[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

@rhatdan
Copy link
Member Author

rhatdan commented Sep 23, 2024

@nalind
Copy link
Member

nalind commented Sep 23, 2024

I'm skeptical that this will work for manifest push.

@rhatdan
Copy link
Member Author

rhatdan commented Sep 24, 2024

@AdamWill could you see if buildah manifest push fails when not in the user namespace. Since this follows the similar path to buildah push, most likely @nalind is correct.

@rhatdan
Copy link
Member Author

rhatdan commented Sep 24, 2024

$ ./bin/buildah manifest create quay.io/rhatdan/mymanifest
ff08b4da30d4c9474dcbebde7d61e03864f404541c44f5b2890798d7dbcae3ca
$ ./bin/buildah manifest push quay.io/rhatdan/mymanifest
Getting image list signatures
Copying 0 images generated from 0 images in list
Writing manifest list to image destination
Storing list signatures

This worked.

@nalind
Copy link
Member

nalind commented Sep 24, 2024

I'm doubtful that many of the relevant code paths were invoked there, since there was no image rootfs being pushed.

@AdamWill
Copy link

I'm not sure this is safe. login used to be in the list, but you took it out in #3278 because of #3259 .

If adding all these other things to the list means #3259 won't be an issue any more, great, but I'm worried maybe that's not the case :)

@rhatdan
Copy link
Member Author

rhatdan commented Sep 24, 2024

Well it looks like login, logout and manifest are likely to require UserNS then.

@AdamWill
Copy link

well the thinking behind my suggestion (make it an arg) was that what you can't do is "cross the streams", right? run some things in userns but some things outside it. but (at least as I understood it) it should in theory be okay to run only non-container-build-y commands, all outside userns. so my idea was to provide a somewhat-hidden way to do that, for this specific use case. if a command-line argument is too prominent it could be an env var or something, I guess.

@rhatdan
Copy link
Member Author

rhatdan commented Sep 25, 2024

I am fine with adding a hidden option --unshare and forcing the user to need to deal with it.

@rhatdan
Copy link
Member Author

rhatdan commented Sep 25, 2024

@nalind Thoughts?

@nalind
Copy link
Member

nalind commented Sep 25, 2024

Would it actually solve the problem? Attempting to push layers without configuring a user namespace can very easily trip over permissions problems.

@AdamWill
Copy link

I can try and test my patch later, I didn't have time when I wrote it.

@TomSweeneyRedHat
Copy link
Member

I'd be curious to hear @giuseppe 's opinion here. The code change looks fine for what you want to do, I'll let others decide if that's an appropriate approach. The hidden --unshare option is appealing.

Looks like a rebase is needed.

@giuseppe
Copy link
Member

I'd be curious to hear @giuseppe 's opinion here. The code change looks fine for what you want to do, I'll let others decide if that's an appropriate approach. The hidden --unshare option is appealing.

if we are accessing the storage as @nalind pointed out, it won't work without a user namespace since we need CAP_DAC_OVERRIDE to access some of the files in an exploded rootfs

@AdamWill
Copy link

AdamWill commented Oct 2, 2024

sorry for not testing this; after some extensive discussion we decided to try having the image uploader produce its own multi-arch manifest and push it with skopeo, to avoid the need for buildah or podman. I'm just finishing up that code now (adapted from https://pagure.io/koji-flatpak/blob/main/f/koji_flatpak/plugins/flatpak_builder_plugin.py#_586 ). If it works, we'll probably not care about this so much (though it'd still be nice if this could be addressed somehow in skopeo or buildah or podman so we could drop the manifest creation code from our tool).

@AdamWill
Copy link

AdamWill commented Oct 8, 2024

Update - we changed our uploader tool to build the manifest itself, and it works, so we'll just go with that for now. Bit more code than we really want to carry/maintain, but hey. If there's ever a way to use podman or buildah or skopeo to construct the manifest that doesn't run through this userns stuff, we'll try it.

@rhatdan rhatdan closed this Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved No New Tests Allow PR to proceed without adding regression tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

buildah commands that don't involve building containers at all run through unshare
5 participants