Fix an issue with podman save trying to store signatures

[fgiloux]

Issue
Running the following command:

podman save registry.redhat.io/openshift-logging/cluster-logging-operator-bundle@sha256:70d57b8062d855c6e1f38d99c796fdf06ddbc8070447770453f7ac37db5e93f8 -o ./tmp/cluster-logging.5.0.7-27/cluster-logging.5.0.7-27.tar

I get:

Getting image source signatures
Checking if image destination supports signatures
Error: Can not copy signatures to docker-archive:./tmp/cluster-logging.5.0.7-27/cluster-logging.5.0.7-27.tar: Storing signatures for docker tar files is not supported

This is similar to what was raised in this issue and should have been fixed with this PR

Applying this one line code change fixed the issue for me however I am not knowledgeable enough about podman code base to be confident that it is the right way of doing it.

[rhatdan]

@mtrmac @vrothberg PTAL

[rhatdan]

I think I have seen something like this in the CI/CD system when run locally as well.

[vrothberg]

Thank you! The changes work perfectly fine here but it does not catch all execution paths. I think we should flip the switch in libimage. @mtrmac WDYT?

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fgiloux, vrothberg

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:

• OWNERS [vrothberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[mtrmac]

Thank you! The changes work perfectly fine here but it does not catch all execution paths. I think we should flip the switch in libimage. @mtrmac WDYT?

History: If I’m reading the history correctly, #7956 did hard-code --remove-signatures at the API/engine abstraction level (which seems more work than any of the other options), and it was undone/lost in #10147 . I can’t see that the latter change was intentional.

Tests: … but I also wonder why the podman save remove signature test introduced in the first PR didn’t start failing. (Or rather, I wonder if/how it ever worked, the "save", "remove-signatures=true", without -- before --remove looks suspicious, and I can’t see that PR hooking a CLI flag up. I didn’t try actually running the test, to be fair.)

API: Isn’t pkg/domain/entities.ImageSaveOptions, with RemoveSignatures defaulting to false, a committed ABI by now? If so, I don’t think libimage can just decide to hard-code RemoveSignatures to true (or maybe it would require the caller to opt-in via something like ImageSaveOptions.RemoveSignaturesIfNecessary).

Behavior: I could conceptually see the case for either adding a --remove-signature flag that the user would have to affirmatively opt in to acknowledge that signatures won’t be represented, or just defaulting to that for formats that don’t support signatures (which is currently all of them, but that might not be the case forever), on the theory that the user would rather get something than just a failure, and missing signatures would end up a… teachable moment? The discussion in #7956 strongly leaned towards the latter, although it didn’t discuss the possible format evolution.

I guess if we don’t ask for an explicit opt-in, it would be cleaner for the knowledge about which formats don’t support signatures to be captured captured in libimage, not every single caller of that library. (Or maybe directly in c/image; right now c/image provides that information on a formed ImageDestination, which can be more informative but is a bad fit for image save; this should be a Can[Ever]SupportSignatures property of a transport — and we can’t change the ImageTransport interface directly, so that would involve adding a bit of infrastructure.)

So I think ideally the Podman CLI should set a new RemoveSignaturesIfNecessary boolean that gets forwarded all the way to libimage, and libimage should call a new c/image facility (c/image/transports.CanSupportSignatures(types.ImageTransport) -> {yes, no,unknown} — or maybe “unknown” should map to ”yes”). And I can quite understand the objection that this would be too much work.

[vrothberg]

@mtrmac @rhatdan how about removing signatures for save for now (i.e., main branch and 3.4). I begin to see it as a regression as podman save ubi8 (and other RH images) started to break.

Once we're back to the status quo, we can improve on that?

[rhatdan]

SGTM

[vrothberg]

Thanks for kicking things off, @fgiloux! Given the v3.4 release is just around the corner, I will take over and kick it over the finish line.

[fgiloux]

Very good! I am very impressed by your reactivity and the thoughts that have been given into this. You rock.

[mtrmac]

@mtrmac @rhatdan how about removing signatures for save for now (i.e., main branch and 3.4).

Sure. (Basically, a comment to the extent that in the future there may be formats that support signatures, and we wouldn’t want to remove signatures in those cases, is all I’d like to have at this point.)

[mtrmac]

or just defaulting to that for formats that don’t support signatures (which is currently all of them, but that might not be the case forever)

Actually docker-dir supports signatures right now.