Use layers digests for comparing podman images

[tkopecek]

Previous method checked default digest provided by podman. This digest is "local" and changed every time image is saved/load or at any other point manifest is modified. This doesn't mean that it is a different image.

Viable way for our purposes is to compare that all layers are identical and in the same order. Simple way to distill this into one value is to concatenate individual layers' digests in order of appearance in RootFS.

[pep8speaks]

Hello @tkopecek! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-12-18 13:26:28 UTC

[tkopecek]

@praiskup Do you think this is the viable approach for comparing images?

[tkopecek]

Proposal from slack is to also add checking of Config section. If we decide for this I can add e.g. checksum of sorted dict of Config.

[praiskup]

@praiskup Do you think this is the viable approach for comparing images?

Probably? I'm not the best person to judge :-) have you tried to contact someone from the Podman team?

[praiskup]

Tested with fedora-rawhide-x86_64 and

--- a/mock/py/mockbuild/buildroot.py
+++ b/mock/py/mockbuild/buildroot.py
@@ -285,7 +285,7 @@ class Buildroot(object):
                                   digest_expected)
                     digest = podman.get_layers_digest()
                     if digest != digest_expected:
-                        getLog().warning(
+                        raise BootstrapError(
                             f"Expected digest for image {podman.image} is"
                             f"{digest_expected}, but {digest} found.")

And it seems to work.

[praiskup]

Tested with centos-stream+epel-{9,10}-x86_64 and it works as well.

[praiskup]

Can you please rebase on top of the current main? That should make CI green.

Also note that there's https://github.com/rpm-software-management/mock/blob/main/behave/features/hermetic-build.feature, so the testing-farm:fedora-40-x86_64:mock CI task should be green first before we merge this.

It would be nice to mention the podman discussion I started somewhere in the code (or commit message).

[praiskup]

CI fail: ERROR: Config section of registry.fedoraproject.org/fedora:rawhide is missing.

[praiskup]

ERROR: RootFS section of quay.io/centos/centos:stream9 is missing.

[praiskup]

• we need to resolve the json versioning issue
• new release notes entry
• I would make it failing on checksum mismatch again, as this method is reliable
• please squash, and refer the commit to Howto rule out a MITM attack after re-importing the image-as-tarball? containers/podman#24818

Otherwise thank you, LGTM, nice one :)

[praiskup]

Seems to work, the CI failure was probably some cache problem / race condition (after re-run, it is green now) . What remains is to handle the JSON scheme reasonably. I think we could keep the old scheme as-is, and not rename the field. Just change the documentation for the field that it is not the built-in digest.

[praiskup]

Nit: See the comment 2, there's one pep8speaks issue.

[tkopecek]

Seems to work, the CI failure was probably some cache problem / race condition (after re-run, it is green now) . What remains is to handle the JSON scheme reasonably. I think we could keep the old scheme as-is, and not rename the field. Just change the documentation for the field that it is not the built-in digest.

So, do you think that also the method should stage get_image_digest or is get_layers_digest better?

[praiskup]

So, do you think that also the method should stage get_image_digest or is get_layers_digest better?

No strong opinion, but it's not "just layers", we include ".Config" too.

[praiskup]

There's this error though:

ERROR: Expected digest for image registry.fedoraproject.org/fedora:rawhide isTrue, but 953d47de95893c3502da491bb1100e401ddf23b321dec4f3d7c54e6e095b3252 found.

[tkopecek]

There's this error though:

ERROR: Expected digest for image registry.fedoraproject.org/fedora:rawhide isTrue, but 953d47de95893c3502da491bb1100e401ddf23b321dec4f3d7c54e6e095b3252 found.

Fixed, anyway - how you were able to trigger it? Older lockfile?

[tkopecek]

So, do you think that also the method should stage get_image_digest or is get_layers_digest better?

No strong opinion, but it's not "just layers", we include ".Config" too.

Nor it is an image digest (as understood in container world).

[praiskup]

Fixed, anyway - how you were able to trigger it? Older lockfile?

That was triggered by the testing-farm CI, the latest one is being run right now by Packit:
https://github.com/rpm-software-management/mock/pull/1514/checks?check_run_id=34539968098

[praiskup]

Nor it is an image digest (as understood in container world).

🤷‍♂️ yeah I agree, we can call it mock_oci_digest ? :) or pick whatever name you want ... I'd still prefer to expose this digest calculation as a separate from mockbuild.podman podman_mock_image_digest method (or alike). Just to have the method published "somewhere", so other users may start doing the thing we do here.. (and e.g., check that mock's digest is valid).

[praiskup]

LGTM, thank you.

[xsuchy]

+1