Skip to content
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.

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

OCI Store resolver returns plain descriptor #457

Closed
carabasdaniel opened this issue Mar 7, 2023 · 16 comments
Closed

OCI Store resolver returns plain descriptor #457

carabasdaniel opened this issue Mar 7, 2023 · 16 comments
Milestone

Comments

@carabasdaniel
Copy link

When resolving a descriptor from the OCI store, the returned descriptor is stripped down to contain only the media type, digest and size:

return descriptor.Plain(desc), nil

Is there any particular reason to not have a complete descriptor returned ?

@Wwwsylvia
Copy link
Member

Returning the complete descriptor could be tricky in some scenarios.

I would like to take the chance to discuss this use case:
Suppose there are two descriptors pointing to the same manifest with different tags (v1.0 and latest) in the index.json:

{
    "schemaVersion": 2,
    "mediaType": "application/vnd.oci.image.index.v1+json",
    "manifests": [
      {
        "mediaType": "application/vnd.oci.image.manifest.v1+json",
        "size": 7143,
        "digest": "sha256:e692418e4cbaf90ca69d05a66403747baa33ee08806650b51fab815ad7fc331f",
        "annotations": {
          "org.opencontainers.image.ref.name": "v1.0",
          "foo":"bar"
        }
      },
      {
        "mediaType": "application/vnd.oci.image.manifest.v1+json",
        "size": 7143,
        "digest": "sha256:e692418e4cbaf90ca69d05a66403747baa33ee08806650b51fab815ad7fc331f",
        "annotations": {
          "org.opencontainers.image.ref.name": "latest",
          "foo":"bar"
        }
      }
    ]
  }

If we resolve the descriptor by its digest:

desc, err := oci.Resolve(ctx, "sha256:e692418e4cbaf90ca69d05a66403747baa33ee08806650b51fab815ad7fc331f")

What should be the result of oci.Resolve()?

Should it be the descriptor tagged by v1.0?

     {
        "mediaType": "application/vnd.oci.image.manifest.v1+json",
        "size": 7143,
        "digest": "sha256:e692418e4cbaf90ca69d05a66403747baa33ee08806650b51fab815ad7fc331f",
        "annotations": {
          "org.opencontainers.image.ref.name": "v1.0",
          "foo":"bar"
        }
      },

Or the descriptor tagged by latest?

      {
        "mediaType": "application/vnd.oci.image.manifest.v1+json",
        "size": 7143,
        "digest": "sha256:e692418e4cbaf90ca69d05a66403747baa33ee08806650b51fab815ad7fc331f",
        "annotations": {
          "org.opencontainers.image.ref.name": "latest",
          "foo":"bar"
        }
      }

Or should we strip the annotation org.opencontainers.image.ref.name and keep others?

      {
        "mediaType": "application/vnd.oci.image.manifest.v1+json",
        "size": 7143,
        "digest": "sha256:e692418e4cbaf90ca69d05a66403747baa33ee08806650b51fab815ad7fc331f",
        "annotations": {
          "foo":"bar"
        }
      }

@carabasdaniel
Copy link
Author

Hi @Wwwsylvia,
Thanks for the quick reply.

Ideally all annotations should be kept and the resolver should return an array of descriptors for the requested digest.

To keep things simple it might make sense to strip the org.opencontainers.image.ref.name from annotations and keep everything else.

@gertd
Copy link

gertd commented Mar 14, 2023

@Wwwsylvia it is my understanding that in the current OCI model, it is a legal and common case that a sha256 can resolve to 1 to N number of tags. Not supporting that would introduce backward compatibility backlash in my opinion which is undesirable. Am I missing something?

@Wwwsylvia
Copy link
Member

@gertd Yes a digest can be associated with multiple tags, and all the tags are saved in index.json.

a sha256 can resolve to 1 to N number of tags.

Do you mean we should return a list of descriptors for Resolve(dgst) ? Or am I understanding it wrong?

Not supporting that would introduce backward compatibility backlash

Could you elaborate a bit more on "backward compatibility"?

@Wwwsylvia
Copy link
Member

Wwwsylvia commented Mar 17, 2023

Maybe we can add a function ResolveN(), which returns multiple descriptors for a digest reference and returns one descriptor for a tag reference.

func (s *Store) ResolveN(ctx context.Context, reference string) ([]ocispec.Descriptor, error)

Using the above example, if we call

descs, err := oci.ResolveN(ctx, "sha256:e692418e4cbaf90ca69d05a66403747baa33ee08806650b51fab815ad7fc331f")

The result would be

[
     {
        "mediaType": "application/vnd.oci.image.manifest.v1+json",
        "size": 7143,
        "digest": "sha256:e692418e4cbaf90ca69d05a66403747baa33ee08806650b51fab815ad7fc331f",
        "annotations": {
          "org.opencontainers.image.ref.name": "v1.0",
          "foo":"bar"
        }
      },
      {
        "mediaType": "application/vnd.oci.image.manifest.v1+json",
        "size": 7143,
        "digest": "sha256:e692418e4cbaf90ca69d05a66403747baa33ee08806650b51fab815ad7fc331f",
        "annotations": {
          "org.opencontainers.image.ref.name": "latest",
          "foo":"bar"
        }
    }
]

And if we call

descs, err := oci.ResolveN(ctx, "latest")

The result would be

[
     {
        "mediaType": "application/vnd.oci.image.manifest.v1+json",
        "size": 7143,
        "digest": "sha256:e692418e4cbaf90ca69d05a66403747baa33ee08806650b51fab815ad7fc331f",
        "annotations": {
          "org.opencontainers.image.ref.name": "latest",
          "foo":"bar"
        }
      }
]

@Wwwsylvia
Copy link
Member

For the original Resolve() function, how about returning a complete descriptor for a tag reference and returning a descriptor without org.opencontainers.image.ref.name for a digest reference? 🤔

Code:

desc, err := oci.Resolve(ctx, "latest")

Result:

    {
        "mediaType": "application/vnd.oci.image.manifest.v1+json",
        "size": 7143,
        "digest": "sha256:e692418e4cbaf90ca69d05a66403747baa33ee08806650b51fab815ad7fc331f",
        "annotations": {
          "foo":"bar",
          "org.opencontainers.image.ref.name": "latest",
        }
      }

Code:

desc, err := oci.Resolve(ctx, "sha256:e692418e4cbaf90ca69d05a66403747baa33ee08806650b51fab815ad7fc331f")

Result:

    {
        "mediaType": "application/vnd.oci.image.manifest.v1+json",
        "size": 7143,
        "digest": "sha256:e692418e4cbaf90ca69d05a66403747baa33ee08806650b51fab815ad7fc331f",
        "annotations": {
          "foo":"bar",
        }
      }

@Wwwsylvia
Copy link
Member

@shizhMSFT What do you think?

@shizhMSFT
Copy link
Contributor

Resolve() digest is ambiguous if the manifest has multiple tags along with annotations. In that case, we can have ResolveN() to reduce ambiguity.

My suggestion would be

  • If reference is a digest,
    • Resolve() returns a plain descriptor
    • ResolveN() return an array of descriptors with annotations
  • If reference is a tag,
    • Resolve() return a descriptor with annotations
    • ResolveN() works the same as Resolve() but return an array

@shizhMSFT shizhMSFT modified the milestones: v2.2.0, v2.1.0 Mar 22, 2023
@shizhMSFT shizhMSFT moved this to In Progress in ORAS Project Board-2023 Mar 22, 2023
Wwwsylvia pushed a commit that referenced this issue Mar 23, 2023
Based on discussion on
#457, this PR returns a
full descriptor on Resolve when the method it's called by tag and it
returns a plain descriptor when the method it's called by digest.

Signed-off-by: oanatmaria <oana@aserto.com>
@qweeah
Copy link
Contributor

qweeah commented Apr 23, 2023

This is a breaking change which introduces inconsistent behavior of resolver between remote and OCI store.

@Wwwsylvia
Copy link
Member

This is a breaking change which introduces inconsistent behavior of resolver between remote and OCI store.

@qweeah Could you elaborate a bit on this?

@qweeah
Copy link
Contributor

qweeah commented Apr 24, 2023

Well remote registry manifest resolver always returns a descriptor without annotation.

return ocispec.Descriptor{
MediaType: mediaType,
Digest: contentDigest,
Size: resp.ContentLength,
}, nil

With merging of e8225cb, resolve a manifest via tag from OCI layout will always get a descriptor with at least one org.opencontainers.image.ref.name annotation.

Noticed this because of the E2E failure in ORAS CLI:

  1. I copied a tagged manifest from remote registry to local OCI layout via tag
  2. I resolved the manifest via tag in OCI layout
  3. I compared the resolve result to what it is in registry.

The content doesn't match so the spec fails.

@qweeah
Copy link
Contributor

qweeah commented Apr 24, 2023

This might not be a critical change since oras-go doesn't make any promise to guarantee that the resolved descriptor from registry and OCI layout will be identical. But IMHO it's better to bring it to up for discussion, like whether it's a breaking change and shall we pick it out from upcoming V2 minor releases.

@Wwwsylvia
Copy link
Member

This might not be a critical change since oras-go doesn't make any promise to guarantee that the resolved descriptor from registry and OCI layout will be identical.

Yeah there is no a guarantee that OCI.Resolve() and Repository.Resolve() will return exactly the same result.
For OCI Target, the manifest descriptor is resolved from the records in index.json. It's intuitive that the resolved descriptor matches the record in index.json.
But there is no such index.json for remote repositories, and the descriptor is resolved from the HTTP response of the remote server.

But IMHO it's better to bring it to up for discussion, like whether it's a breaking change and shall we pick it out from upcoming V2 minor releases.

I personally don't think it's a breaking change for oras-go, as it's adding things (annotation, etc.) to the original result (plain descriptor), not changing or removing existing stuffs.
But it could be a breaking change from the ORAS CLI point of view since the output format changed. 🤔
IMHO we can update the ORAS E2E tests to accommodate this change.

@shizhMSFT Any thoughts?

@shizhMSFT
Copy link
Contributor

In general, Target.Resolve() can return any extra stuffs other than the digest, size, media type. Therefore, we should use content.Equal() to compare the returned descriptors instead of reflect.DeepEqual().

If ORAS CLI E2E tests has more context on the Target instead of a generic Target, the E2E test can check the returned descriptor in deep accordingly.

@shizhMSFT
Copy link
Contributor

Closed by #468

@github-project-automation github-project-automation bot moved this from In Progress to Done in ORAS Project Board-2023 May 4, 2023
@Wwwsylvia
Copy link
Member

Resolve() digest is ambiguous if the manifest has multiple tags along with annotations. In that case, we can have ResolveN() to reduce ambiguity.

My suggestion would be

  • If reference is a digest,

    • Resolve() returns a plain descriptor
    • ResolveN() return an array of descriptors with annotations
  • If reference is a tag,

    • Resolve() return a descriptor with annotations
    • ResolveN() works the same as Resolve() but return an array

The ResolveN function has not been implemented yet. Do we need to raise another issue to track this feature request?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

5 participants