[DO NOT MERGE] Configurable digest support for build and push #27170
Conversation
Signed-off-by: Lokesh Mandvekar <lsm5@redhat.com>
This commit adds support for a --digest flag to artifact push, image push, and manifest push commands. Changes: - Add DigestAlgorithm field to pushOptionsWrapper structs in all push command files - Add --digest flag with validation (sha256, sha512) to all push commands - Add DigestAlgorithm field to entities.ImagePushOptions struct - Add digest algorithm handling logic in abi/images.go Push method Signed-off-by: Lokesh Mandvekar <lsm5@redhat.com>
Apply changes from commit 7f7ab07 to add digest algorithm configuration for build operations. Additions: - DigestAlgorithm field to BuildFlagsWrapper and BuildOptions structs - --digest flag with validation (sha256, sha512) for build commands - BuildWithDigest method that temporarily modifies store digest type - Proper digest algorithm handling throughout the build pipeline - Auto-detects SHA256 (64-char IDs) and SHA512 (128-char IDs) from image ID length - Maintains priority: explicit --digest flag > auto-detection > default SHA256 - Provides debug logging for transparency - Ensures consistent digest algorithms throughout build->push workflow Signed-off-by: Lokesh Mandvekar <lsm5@redhat.com>
openshift-ci
bot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lsm5 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 |
openshift-ci
bot
added
the
approved
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
| return "sha256:" + i.ImageSummary.ID | ||
|
|
||
| // Determine digest algorithm based on ID length | ||
| // SHA256 = 64 hex chars, SHA512 = 128 hex chars |
There was a problem hiding this comment.
If we add more algorithms, this will break. Is there a chance to avoid this guessing?
(As an intuitive guess:
- either the input IDs, as recorded in storage, should include an explicit algorithm specification (maybe in a new field of the image,
IDWithAlgoor something, throughout the ecosystem); - or the image IDs should be “meaningless hex values only compared for equality” and we should not be adding a digest indication to the formatted value (unless it is sha256).
But the “what is an image ID” question really starts with “how do we deduplicate images in c/storage”, and that discussion incl. various corner cases (a manifest with sha256 config+sha512 layers?) will probably mostly happen in containers/container-libs#358 .)
| var buildStore storage.Store | ||
|
|
||
| if digestAlgorithm != "" && digestAlgorithm != r.store.GetDigestType() { | ||
| // Temporarily modify the store's digest type for the build |
There was a problem hiding this comment.
This fairly strongly suggests that the “digest type” option should not be a global value in c/storage (also, in c/storage, look how the “digest type” is actually set in the container and image store at initialization time) — instead, the “digest type to produce” (to convert to, in case of pushes?) should be a per-operation option passed to Buildah / libimage / c/image.
And, quite possibly, we might want to expose be several distinct options with distinct semantics (“use $digest for pushing”, both in CLI and as a default in containers.conf; “use $digest for everything in local storage to avoid collisions”; “use $digest for creating new objects in local storage”, …) — almost certainly not all of these, it’s just that “digest to use” is a bundle of several different aspects, and not all of them might need to be set to the same thing concurrently.
E.g., I think it would be fairly attractive (though, to be explicit, definitely not a blocker) if users could retain their sha256-based workflow with no changes, and only end with podman push --digest=sha512 to generate a sha512 image on a registry. Similarly, maybe pull --convert-digest=sha256 to ensure all existing scripts running against c/storage continue to work because they never see sha512, or pull --convert-digest=sha512 to ensure sha512-level collision resistance - with the --convert-digest option stored in containers.conf or storage.conf so that it doesn’t need to be specified every time.
At this point, I have ~no opinion on what we need / want.
I think the single-operation push --digest option is all of
- an individually valuable feature
- fairly isolated (we’d need to build the digest conversion/recomputation code in c/image, but there would be no interactions with other codebases and no required config file options)
- possibly a valuable prerequisite for a larger feature like the
pull --convert-digest - somewhat of a prerequisite to us testing sha512, with very few images existing in the wild
so it might be an interesting starting point.
OTOH, it would mean adding a digest conversion implementation which is not required by the minimal version of “current users can transparently start using sha512 images, while most of the universe remains sha256, but they might need to adjust their scripts to always look up the image X by the digest used by image X” as discussed in containers/container-libs#358 .
Adds a --digest flag to build and push operations. Accepts sha512 and sha256.
Depends on a container-libs PR (which will have to be split into multiple PRs) and an upcoming buildah PR. The vendoring done in here was using a local copy of my fork, so ignore that issue.
Keeping it in Draft for now. Need to add tests, but any initial review comments welcome.
(Cursor assisted)
Does this PR introduce a user-facing change?