rootless: don't create a namespace unless for containers-storage#653
rootless: don't create a namespace unless for containers-storage#653mtrmac merged 2 commits intocontainers:masterfrom
Conversation
|
This looks like a clever solution to me. @giuseppe PTAL |
|
thanks for the patch! Yes, I think it is a good fix, and should also address #652 |
|
You're welcome, thank you for the prompt review. FWIW I tried using a cleaner method (such as alltransports.ParseImageName) but I couldn't find the right predicate to test for containers-storage type. The proposed solution assumes all the containers-storage image reference start with the |
|
@mtrmac is there any cleaner way to test that? Are you fine with this approach? |
|
One way could be to use |
|
The issue with ParseImageName is that it may fail early with |
|
Ouch, sorry for missing that. In this case, we can mimic what parts := strings.SplitN(imgName, ":", 2)
if len(parts) != 2 {
continue
}
if parts[0] == storage.Transport.Name() { // we have storage reference (but can't validate it yet) } |
|
Done, thank you for the suggestion. |
|
LGTM |
Oh, wow. That’s… a mess. @nalind AFAICS this is essentially because if img, err := store.Image(possibleID); err == nil && img != nil && len(possibleID) >= minimumTruncatedIDLength && strings.HasPrefix(img.ID, possibleID)and the like. Is there any chance we could drop that functionality? (I guess not, but it would make dealing with this in callers so much simpler.) Alternatively, would it be reasonably possible for |
mtrmac
left a comment
There was a problem hiding this comment.
(Assuming that we can’t wiggle out of having to do this by changing c/storage or c/image/storage.)
cmd/skopeo/utils.go
Outdated
| // Check if container-storage are used before doing the maybeReexec | ||
| func needsRexec(c *cli.Context) error { | ||
| return maybeReexec() | ||
| for _, arg := range c.Args() { |
There was a problem hiding this comment.
This is way too imprecise. Instead:
- In the various commands, drop
Before: needsReexec - In each of the command handlers, run something like
maybeReexec(inputParameter)only for the relevant parameters, something like:possibly rearranging this so that theinputImageName := image.args[0] if err := maybeReexec(inputImageName); err != nil { return err } … := parseImageSource(ctx, opts.image, inputImageName) // or parseImage or alltransports.ParseImageName
maybeReexecs happen at the start of the command handlers, before any other real work gets done.
|
Just for another data point, we're temporarily running with a16489e applied in production and it seems to have solved #649 for us. Thanks @TristanCacqueray! |
|
@mtrmac Thank you for the suggestions, I think the last change implements them along with containers/image#631 for the TransportFromImageName procedure. Not sure what is the policy around vendoring thus I inlined the c/image change in that PR. As @fungi mentioned, the opendev.org infrastructure is using a continuously built version of skopeo which triggered the issue of using reexec in unprivileged environment like bubblewrap. |
mtrmac
left a comment
There was a problem hiding this comment.
ACK to the overall approach.
Not sure what is the policy around vendoring thus I inlined the c/image change in that PR.
Sure. Ideally, update vendor.conf, run make vendor, and commit that as a separate commit from the other changes, in the same PR.
81aeea4 to
a6ade2c
Compare
vendor.conf
Outdated
| github.com/kr/pretty v0.1.0 | ||
| github.com/kr/text v0.1.0 | ||
| github.com/containers/image ff926d3c79684793a2135666a2cb738f44ba33dc | ||
| github.com/containers/image 2d92e2533d517b6db4ae7d085a332915fec57867 |
There was a problem hiding this comment.
That commit does not actually exist yet, which would make make vendor not reproducible. After containers/image#631 is merged, please update this with the actual merge commit and re-vendor.
Depends-On: containers/image#631 Signed-off-by: Tristan Cacqueray <tdecacqu@redhat.com>
This change fixes skopeo usage in restricted environment such as bubblewrap where it doesn't need extra capabilities or user namespace to perform its action. Close containers#649 Signed-off-by: Tristan Cacqueray <tdecacqu@redhat.com>
|
LGTM. Thanks! |
This change adds a couple of tests to prevent further regression introduced by containers#653 Signed-off-by: Tristan Cacqueray <tdecacqu@redhat.com>
* Update system-config from branch 'master'
- Merge "Revert "Pin skopeo to unbreak skopeo+bubblewrap""
- Revert "Pin skopeo to unbreak skopeo+bubblewrap"
This reverts commit 0d370a285b09bd28c5b1cdfc6b89d2997f67da5d.
Fixed by containers/skopeo#653 so safe to
merge this once a new build appears in the PPA.
Change-Id: I858eee79d084016b6b71eec46a6118d78f68cafa
This change fixes skopeo usage in restricted environment such as
bubblewrap where it doesn't need extra capabilities or user namespace
to perform its action.
Close #649
Signed-off-by: Tristan Cacqueray tdecacqu@redhat.com