8000 Support OCI Image Manifests with artifactType and subject properties by brackendawson · Pull Request #3834 · distribution/distribution · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Support OCI Image Manifests with artifactType and subject properties #3834

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

Closed

Conversation

brackendawson
Copy link
Contributor
@brackendawson brackendawson commented Feb 3, 2023

This implements support for the previously ignored subject property on OCI Image manifests by creating a link to the referrer under the subject, which will be needed for the artifact listing API.

This pull request is based on #3833 which should be reviewed first and then this re-based.

Adresses #3716

@codecov-commenter
Copy link
codecov-commenter commented Feb 3, 2023

Codecov Report

Patch coverage: 41.66% and project coverage change: +0.02 🎉

Comparison is base (983358f) 56.89% compared to head (a883f79) 56.92%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3834      +/-   ##
==========================================
+ Coverage   56.89%   56.92%   +0.02%     
==========================================
  Files         106      107       +1     
  Lines       10684    10729      +45     
==========================================
+ Hits         6079     6107      +28     
- Misses       3933     3954      +21     
+ Partials      672      668       -4     
Impacted Files Coverage Δ
registry/storage/driver/storagedriver.go 0.00% <ø> (ø)
registry/storage/paths.go 65.53% <0.00%> (-5.20%) ⬇️
registry/storage/references.go 0.00% <0.00%> (ø)
registry/storage/ocimanifesthandler.go 64.51% <33.33%> (-2.53%) ⬇️
registry/handlers/manifests.go 56.70% <50.00%> (+2.53%) ⬆️
manifest/ocischema/manifest.go 76.31% <85.71%> (+2.12%) ⬆️
registry/storage/registry.go 88.57% <100.00%> (+0.27%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Jamstah Jamstah mentioned this pull request Mar 13, 2023
@brackendawson brackendawson changed the title Support OCI Image Manifests with subject property Support OCI Image Manifests with artifactType and subject properties May 2, 2023
@brackendawson brackendawson force-pushed the image-manifest-subjects branch 2 times, most recently from c0832a8 to 69b1547 Compare May 3, 2023 14:57
@brackendawson brackendawson marked this pull request as ready for review May 3, 2023 15:05
Copy link
Contributor
@bainsy88 bainsy88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM just had a minor nit pick on a comment

@milosgajdos
Copy link
Member

Needs a rebase.

Does not yet make referrers link from the subject to the referrer

Signed-off-by: Bracken Dawson <abdawson@gmail.com>
Subjects of referrers should link back to those referrers

Signed-off-by: Bracken Dawson <abdawson@gmail.com>
So that one can easily tell if it was not set or was set invalidly.

Signed-off-by: Bracken Dawson <abdawson@gmail.com>
Signed-off-by: Bracken Dawson <abdawson@gmail.com>
When an artifact is uploaded link its subject back to the artifact, even
if the subject does not exist yet.

Signed-off-by: Bracken Dawson <abdawson@gmail.com>
Future non-OCI artifacts would not be able to live in the same package

Signed-off-by: Bracken Dawson <abdawson@gmail.com>
Signed-off-by: Bracken Dawson <abdawson@gmail.com>
Deletion already works. Referrers links are left dangling so that
deletions don't have to try to parse the content. The artifact listing
API will need to validate each artifact and GC may want to clean up
dangling referrer links.

Signed-off-by: Bracken Dawson <abdawson@gmail.com>
If a manifest which cannot refer to a subject has a subject field then
we should ignore that field and not make a referrer link on that
subject.

Signed-off-by: Bracken Dawson <abdawson@gmail.com>
Signed-off-by: Bracken Dawson <abdawson@gmail.com>
Signed-off-by: Bracken Dawson <abdawson@gmail.com>
Signed-off-by: Bracken Dawson <abdawson@gmail.com>
It may likely be needed by garbage collect later.

Signed-off-by: Bracken Dawson <abdawson@gmail.com>
Signed-off-by: Bracken Dawson <abdawson@gmail.com>
Signed-off-by: Bracken Dawson <abdawson@gmail.com>
Signed-off-by: Bracken Dawson <abdawson@gmail.com>
We do no know that we were handling a PUT request here.

Signed-off-by: Bracken Dawson <abdawson@gmail.com>
Signed-off-by: Bracken Dawson <abdawson@gmail.com>
Artifact Manifest was deleted ¯\_(ツ)_/¯, Image manifest keeps the
subject field, gains an artifactType field, and the rules for what the
artifactType actually is change.

Keeping a lot of the framework that came of the Artifact Manifest
because I think it lead to some worthwhile abstractions.

Signed-off-by: Bracken Dawson <abdawson@gmail.com>
@brackendawson brackendawson force-pushed the image-manifest-subjects branch from 69b1547 to 27a9632 Compare May 23, 2023 10:58
Signed-off-by: Bracken Dawson <abdawson@gmail.com>
We need to keep supporting them, dismiss the linter.

Signed-off-by: Bracken Dawson <abdawson@gmail.com>
@brackendawson
Copy link
Contributor Author

Needs a rebase.

Rebased and comment addressed.

@brackendawson
Copy link
Contributor Author

I need to go back and do this again now because in the next release (presumably RC4) of image-spec, the OCI Index can also have a subject: opencontainers/image-spec#1020 (╯°□°)╯︵ ┻━┻

@milosgajdos
Copy link
Member

I need to go back and do this again now because in the next release (presumably RC4) of image-spec, the OCI Index can also have a subject: opencontainers/image-spec#1020 (╯°□°)╯︵ ┻━┻

OCI sadness intensifies

@davidspek
Copy link
Collaborator

@brackendawson @milosgajdos What's the status of this PR (other than that it needs another rebase)?

@milosgajdos
Copy link
Member

We shall wait until a new stable release is cut in https://github.com/opencontainers/distribution-spec.

Adding things that implement RCs has turned out to be a wrong move. No rush with this until then.

@brackendawson
Copy link
Contributor Author

I'm going to do some work on this soon, now that #3869 has been resolved this is unblocked, I should be able to do all the RC4 changes and also the new APIs now and we'll see what it would look like. It definitely will not b 6D40 e merged until the OCI release the final spec.

@davidspek
Copy link
Collaborator

Just thought I'd mention v1.1.0-rc5 was released a couple weeks ago.

return "", err
}

artifactType := "_" + url.QueryEscape(v.mediaType)
Copy link
@njucjc njucjc Jan 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about perform md5sum here to prevent artifactType from containing special characters or artifactType being too long?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

artifactType := fmt.Sprintf("%x", md5.Sum([]byte(v.mediaType)))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A hash is not a bad idea. @Jamstah and I did discuss this and decided to use underscore prefixed URL encoded artifactType.

When listing referrers, we have to read the referrer manifests anyway to get the annotations, so a hash is no less efficient in that respect.

MD5 would be a weird choice, it would light up linters as an insecure hash. Would a collision be a security issue? Listing referrers by artifactType could yield an artifact of a different type, might have downstream impact on clients? We could use sha256 but we'd probably have to do the <algorithm>/<digest> directory structure to support more algorithms when sha256 falls, and then we'd have to enumerate another directory when listing.

It would remove directory name length concerns, for the storage back ends which actually have directories and have limited name lengths.

It would obfuscate the artifactType from admins browsing the back end storage, that's quite a minimal concern.

I'm open to the idea if done right.

@brackendawson
Copy link
Contributor Author

v1.1.0-rc6 dropped. 🙃

I will finish this when v1.1.0 is released, I don't want to start again again.

@sajayantony
Copy link

@brackendawson @milosgajdos since this is now released in OCI (image/distribution) is there any interest or plan to land this?

@brackendawson
Copy link
Contributor Author

Yes, I need to finish it. It's the next thing on my backlog at work so I should be able to resume work on it after next week. I think there's a couple of days work left to get it completed for out of the box use then there is migration of existing manifests with subjects to work out.

@milosgajdos
Copy link
Member

This needs a wild rebase now @brackendawson 😄

@milosgajdos milosgajdos removed the v3.0.0 v3 release label Jul 17, 2024
teambush1 added a commit to alistair/distribution that referenced this pull request Oct 13, 2024
teambush1 added a commit to alistair/distribution that referenced this pull request Oct 13, 2024
Signed-off-by: Alistair Bush <alistair.bush@gmail.com>
Co-authored-by: Alistair Bush <alistair.bush@gmail.com>
Co-authored-by: Bracken Dawson <abdawson@gmail.com>
teambush1 added a commit to alistair/distribution that referenced this pull request Oct 13, 2024
alistair added a commit to alistair/distribution that referenced this pull request Feb 28, 2025
Signed-off-by: Alistair Bush <alistair.bush@gmail.com>
Co-authored-by: Alistair Bush <alistair.bush@gmail.com>
Co-authored-by: Bracken Dawson <abdawson@gmail.com>
@brackendawson
Copy link
Contributor Author

Closing in favor of the more progressed #4483

@brackendawson brackendawson closed this 80A9 Jul 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants
0