libimage: load name from oci-dir annotations (per-manifest first; single-manifest fallback to top-level); add e2e test #26867

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Closed

shiavm006 wants to merge 2 commits into containers:main from shiavm006:fix/oci-dir-load-name

Contributor

shiavm006 commented Aug 20, 2025

Fixes incorrect behavior where images loaded from oci-dir had no name and were only addressable by ID.Aligns with behavior users expect from tar-based loads and preserves tags exported via podman save --format oci-dir.

Fixes : #26850

None

openshift-ci bot added the release-note-none label

Contributor

openshift-ci bot commented Aug 20, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: shiavm006
Once this PR has been reviewed and has the lgtm label, please assign mheon for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

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

packit-as-a-service bot commented Aug 20, 2025

[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore.

shiavm006 force-pushed the fix/oci-dir-load-name branch 2 times, most recently from b2a6c88 to ed7aeae Compare

August 20, 2025 09:33

Member

baude commented Aug 20, 2025

I think there is some debate about whether or not the original behavior was a bug (given OCI-DIR). It would be good to have @mtrmac comment here as well.

mtrmac reviewed

View reviewed changes

Contributor

mtrmac left a comment

I don’t have much of an opinion on the change; I don’t recall any reason to specifically not want to use annotations. Consistency would be nice, of course.

Originally the code comes from containers/common@5797a05 , and all the way back from a031b83, which is not very helpful. My best guess is that special-casing oci: just didn’t happen originally, and nobody thought to make it equivalent when we did add that special case.

The major concern is that this would break scripts, so it definitely needs to happen only in a major Podman version.

(Procedurally, the c/common changes need to be made in the c/common repo.)

vendor/github.com/containers/common/libimage/pull.go Outdated Show resolved Hide resolved

vendor/github.com/containers/common/libimage/pull.go

    
              			if err != nil {

              				return nil, nil, err

              		// Try to load a name from the OCI layout's index.json annotations first

              		if md, errMD := ociTransport.LoadManifestDescriptor(ref); errMD == nil {

Contributor

mtrmac Aug 20, 2025

I don’t know why we would silently ignore this error; if this fails, the copy is going to fail anyway, isn’t it?

I suppose that relates to the other question — I’m missing something.

vendor/github.com/containers/common/libimage/pull.go

    
              		// Try to load a name from the OCI layout's index.json annotations first

              		if md, errMD := ociTransport.LoadManifestDescriptor(ref); errMD == nil {

              			if annotatedName := nameFromAnnotations(md.Annotations); annotatedName != "" {

              				if named, errNorm := NormalizeName(annotatedName); errNorm == nil {

Contributor

mtrmac Aug 20, 2025

Same, why is silently ignoring errors a good idea? (Also in other places of the PR.)

Contributor Author

shiavm006 Aug 20, 2025

You're absolutely right about the silent error handling ....... it's poor thing to ignore errors without any indication.
i will make sure of it

vendor/github.com/containers/common/libimage/pull.go Outdated

    
              				_ = f.Close()

              			}

              		}

              		// Fallback to previous behavior if no annotated name was found/usable

Contributor

mtrmac Aug 20, 2025

(Absolutely non-blocking: “previous” is, long-term, not going to be a meaningful concept.)

Contributor Author

shiavm006 commented Aug 20, 2025

@mtrmac
what u think is better for know then
Keep the current approach but move the logic to c/image in a followup PR or Refactor this to use only LoadManifestDescriptor and handle top-level annotations differently

mtrmac requested changes

View reviewed changes

vendor/github.com/containers/common/libimage/pull.go Outdated Show resolved Hide resolved

Contributor Author

shiavm006 commented Aug 21, 2025 •

edited

Loading

@mtrmac
removed the top-level index.json annotation logic completely and now only using LoadManifestDescriptor to read per-manifest annotations which is OCI spec compliant. This ensures consistency with podman pull oci:$path:$name behavior and I've also removed the test that expected top level annotations while ensuring all error handling now includes proper debug logging. so know images loaded from oci-dir will have names when they contain per-manifest annotations and fall back to ID based naming when no annotations exist, with no OCI spec violations and consistent behavior across all Podman commands.
whats ur point on this approach ?


          libimage: fix oci-dir name loading to use only per-manifest annotations

dc0d91e

Fixes issue where images loaded from oci-dir had no name and were only
addressable by ID. Now reads image names from per-manifest annotations
in OCI layout, ensuring OCI spec compliance and consistency with
podman pull oci:/Users/shivammittal/.nvm/versions/node/v20.19.0/bin /opt/homebrew/bin /opt/homebrew/sbin /usr/local/bin /System/Cryptexes/App/usr/bin /usr/bin /bin /usr/sbin /sbin /var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/local/bin /var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/bin /var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/appleinternal/bin /Users/shivammittal/.nvm/versions/node/v20.19.0/bin /Users/shivammittal/.cargo/bin /Users/shivammittal/.cursor/extensions/ms-python.debugpy-2025.8.0-darwin-arm64/bundled/scripts/noConfigScripts: behavior.

Removes top-level index.json annotation logic that violated OCI spec
and created inconsistency across Podman commands. Includes proper
error logging and removes test expecting non-compliant behavior.

Fixes: containers#26850
Signed-off-by: shiavm006 <[email protected]>

shiavm006 force-pushed the fix/oci-dir-load-name branch from 34e5176 to dc0d91e Compare

August 21, 2025 06:07


          ci: trigger checks to re-run

5523a04

Signed-off-by: shiavm006 <[email protected]>

Contributor Author

shiavm006 commented Aug 25, 2025 •

edited

Loading

@Luap99
can u lookit into once ,..... could help in it or guide

mtrmac reviewed

View reviewed changes

Contributor

mtrmac left a comment

Thanks!

To reiterate: this must be tracked as a Podman 6 breaking change
… and it needs to primarily happen in the c/common repo

On the substance of the change, now looks OK.

LoadManifestDescriptor/NormalizeName failures should be reported and cause the whole operation to fail, not ignored (or only debug-logged)

openshift-merge-robot added the needs-rebase label

Collaborator

openshift-merge-robot commented Sep 3, 2025

PR needs rebase.

Details

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.

github-actions bot commented Oct 4, 2025

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

github-actions bot added the stale-pr label

shiavm006 closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-rebase release-note-none stale-pr