Add smoke tests for encryption CLI helpers

[mtrmac]

What type of PR is this?

/kind other

What this PR does / why we need it:

A non-nil but empty decryption configuration seems to be valid enough to trigger decryption in some configurations, per containers/podman#18196 .

Like in Skopeo and Podman, only decrypt when the user explicitly instructs us to (e.g. not triggering decryption based on environment variables).

How to verify it

Manually, I guess.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Does this PR introduce a user-facing change?

An explicit `--decryption-key` option is now mandatory to trigger image decryption, even if the environment implicitly points at usable keys.

[mtrmac]

Cc: @lumjjb

[flouthoc]

LGTM , this needs [NO NEW TESTS NEEDED]

[Luap99]

I am not a buildah maintainer so ultimately I do not really care but this sounds like a breaking change. If you now require the option explicitly this can break existing users that depend on it. We should avoid such changes at all costs.

[mtrmac]

Note #4746 as an alternative implementation approach.

---

this sounds like a breaking change. If you now require the option explicitly this can break existing users that depend on it.

Yes… but it was also, to my knowledge, never documented in Buildah to work this way. I don’t know for sure whether it was intended to work this way, or documented elsewhere to work this way.

I’ll defer to @lumjjb on that.

---

We should avoid such changes at all costs.

I don’t think we have a general obligation to be bug-for-bug-compatible, and I would fairly strongly push against being bug-for-bug-compatible “at all costs”.

Still, it’s a valid concern. I can imagine changing podman pull to auto-decrypt instead — although it feels unintuitive and vaguely possibly risky to me.

---

Most of the diagnosis and discussion has been in containers/podman#18196 , and there are two Buildah PRs in flight.

Let’s concentrate the design discussion in the Podman containers/podman#18196 issue, for now.

[mtrmac]

LGTM , this needs [NO NEW TESTS NEEDED]

Thanks for pointing that out.

Actually I would expect some of the existing tests to fail with a different error message, so bypassing that for now to see the real failures…

[rhatdan]

I am fine with making them consistent since buildah vendored into Podman means podman build works differently then podman pull.

[TomSweeneyRedHat]

Anyway to add a warning? I.e. if the environment points at keys, but the option is not specified, then print a warning message? IDK for sure that we would want to do that, but just wondering.

[mtrmac]

Anyway to add a warning? I.e. if the environment points at keys, but the option is not specified, then print a warning message? IDK for sure that we would want to do that, but just wondering.

If the image isn’t encrypted, we probably wouldn’t want to warn:

• Right now, we “enable” encryption for all buildah from cases, only it does nothing for unencrypted images
• And to avoid that warning, users would probably need to unset the environment variable… which is a hassle.

OTOH if the image is encrypted and we no longer decrypt by default, buildah from will fail (see the test suite updates in this PR), so we don’t need a warning. The error message in that case could certainly be improved — but that’s not this PR; it would probably be added in c/image.

[mtrmac]

This exit code changes because the error stack, ultimately, contains an exec.ExitError from invoking an untar subprocess; and the top level of Buildah checks for exec.ExitError, and returns that exit code.

One can read that as a failure of the Go error wrapping design… or, alternatively, as Builder’s main matching too loosely if it wants only specific subprocesses’ exit codes to be returned unmodified.

AFAICS the exit code from Buildah is, apart from buildah manifest exists, not documented, so this PR just changes tests to accept the status 1. If we do actually commit (whether in documentation in or in practice) to returning 125 on unspecific failures, and this needs to be made more precise, please let me know (and also define which subprocess invocations that could fail with ExitError should be propagated.)

[rhatdan]

We only document the exit codes for the run commands, I believe.

The question is do we take this PR and leave multiple implementation of the same code, or take mine and vendor the code into Podman.

[mtrmac]

I think the primary question is whether we want this; as @Luap99 points out, this might be a compatibility break.

If we do want this, I completely agree that sharing the code, per your PR, is the better approach — consider using the test updates from here, to make it pass.

[rhatdan]

I don't see this as an important breaking change, since having it two different ways within Podman is more important.
LGTM
I think we can merge this and then I will update my PR to pull this in.

[github-actions]

A friendly reminder that this PR had no activity for 30 days.

[mtrmac]

(I think the consensus is to prefer #4746 … except that it doesn’t build.)

[mtrmac]

#4746 has implemented the primary (and better) fix; this now just carries the remaining unit tests.

[rhatdan]

/approve
/lgtm
/hold

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mtrmac, 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:

• OWNERS [rhatdan]

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

[rhatdan]

/hold cancel