Skip to content

Support OCI Image Manifests with artifactType and subject properties#3834

Closed
brackendawson wants to merge 21 commits intodistribution:mainfrom
brackendawson:image-manifest-subjects
Closed

Support OCI Image Manifests with artifactType and subject properties#3834
brackendawson wants to merge 21 commits intodistribution:mainfrom
brackendawson:image-manifest-subjects

Conversation

@brackendawson
Copy link
Copy Markdown
Contributor

@brackendawson brackendawson commented Feb 3, 2023 •
edited
Loading

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
Copy Markdown

codecov-commenter commented Feb 3, 2023 •
edited
Loading

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.

brackendawson
brackendawson commented Feb 10, 2023
View reviewed changes
Comment thread manifest/ocischema/manifest.go Outdated
@Jamstah Jamstah mentioned this pull request Mar 13, 2023
Closed
@milosgajdos milosgajdos mentioned this pull request Apr 24, 2023
Open
@brackendawson brackendawson mentioned this pull request May 2, 2023
Closed
@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
@brackendawson brackendawson mentioned this pull request May 5, 2023
Merged
bainsy88
bainsy88 suggested changes May 12, 2023
View reviewed changes
Copy link
Copy Markdown
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

Comment thread manifest/ocischema/manifest.go Outdated
@milosgajdos
Copy link
Copy Markdown
Member

milosgajdos commented May 12, 2023

Needs a rebase.

@brackendawson
Parsing and storing of OCI Artifact Manifests
30015b3 
Does not yet make referrers link from the subject to the referrer

Signed-off-by: Bracken Dawson <[email protected]>
@brackendawson
Add failing test for artifact referrers links
758f8ae 
Subjects of referrers should link back to those referrers

Signed-off-by: Bracken Dawson <[email protected]>
@brackendawson
Artifact subject should be a pointer field
608a570 
So that one can easily tell if it was not set or was set invalidly.

Signed-off-by: Bracken Dawson <[email protected]>
@brackendawson
Implement the Referrers interface on Artifact
cd0c26c 
Signed-off-by: Bracken Dawson <[email protected]>
@brackendawson
Create referrer links on subjects of artifacts
a9fafba 
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 <[email protected]>
@brackendawson
Artifact manifest package should ba named OCI
3b4919b 
Future non-OCI artifacts would not be able to live in the same package

Signed-off-by: Bracken Dawson <[email protected]>
@brackendawson
Implement GET of artifact manifests
d655919 
Signed-off-by: Bracken Dawson <[email protected]>
@brackendawson
Test artifact manifest deletion
19033a3 
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 <[email protected]>
@brackendawson
Test subject field in a manifest that can't refer
92a5cff 
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 <[email protected]>
@brackendawson
Test additional artifact manifest cases
baa392b 
Signed-off-by: Bracken Dawson <[email protected]>
@brackendawson
Remove stutter from artifact filenames
2f078b7 
Signed-off-by: Bracken Dawson <[email protected]>
@brackendawson
Remove duplication of manifest.Unversioned
d9b2869 
Signed-off-by: Bracken Dawson <[email protected]>
@brackendawson
Remove unlinker for now
35c22d3 
It may likely be needed by garbage collect later.

Signed-off-by: Bracken Dawson <[email protected]>
@brackendawson
Document the reason for unversioned
27b6905 
Signed-off-by: Bracken Dawson <[email protected]>
@brackendawson
Algorithm in referrer link path is a parameter
db324a4 
Signed-off-by: Bracken Dawson <[email protected]>
@brackendawson
Include a test for an artifact with blobs
35eae60 
Signed-off-by: Bracken Dawson <[email protected]>
@brackendawson
OCI manifest verification error should be generic
eb2f108 
We do no know that we were handling a PUT request here.

Signed-off-by: Bracken Dawson <[email protected]>
@brackendawson
Support OCI Image Manifests with subject property
3b1947a 
Signed-off-by: Bracken Dawson <[email protected]>
@milosgajdos
Copy link
Copy Markdown
Member

milosgajdos commented Jun 16, 2023

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 davidspek mentioned this pull request Jul 11, 2023
Closed
@mtrmac mtrmac mentioned this pull request Jul 31, 2023
Open
@davidspek
Copy link
Copy Markdown
Collaborator

davidspek commented Aug 15, 2023

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

@milosgajdos
Copy link
Copy Markdown
Member

milosgajdos commented Aug 15, 2023

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
Copy Markdown
Contributor Author

brackendawson commented Aug 16, 2023

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 be merged until the OCI release the final spec.

@wy65701436 wy65701436 self-requested a review August 26, 2023 17:41
@wy65701436 wy65701436 added the v3.0.0 v3 release label Aug 29, 2023
@davidspek
Copy link
Copy Markdown
Collaborator

davidspek commented Oct 4, 2023

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

njucjc
njucjc reviewed Jan 31, 2024
View reviewed changes
Comment thread registry/storage/paths.go
return "", err
}

artifactType := "_" + url.QueryEscape(v.mediaType)
Copy link
Copy Markdown
Contributor

@njucjc njucjc Jan 31, 2024 •
edited
Loading

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
Copy Markdown
Contributor

@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.

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

Copy link
Copy Markdown
Contributor Author

@brackendawson brackendawson Jan 31, 2024

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
Copy Markdown
Contributor Author

brackendawson commented Jan 31, 2024

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
Copy Markdown

sajayantony commented Mar 7, 2024

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

@brackendawson
Copy link
Copy Markdown
Contributor Author

brackendawson commented Mar 7, 2024

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
Copy Markdown
Member

milosgajdos commented Jul 13, 2024

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
Migrate and upgrade distribution#3834
50f5b13
@alistair alistair mentioned this pull request Oct 13, 2024
Open
teambush1 added a commit to alistair/distribution that referenced this pull request Oct 13, 2024
@teambush1 @alistair @brackendawson
Migrate and upgrade distribution#3834
76a64b8 
Signed-off-by: Alistair Bush <[email protected]>
Co-authored-by: Alistair Bush <[email protected]>
Co-authored-by: Bracken Dawson <[email protected]>
teambush1 added a commit to alistair/distribution that referenced this pull request Oct 13, 2024
@teambush1
Migrate and upgrade distribution#3834
bb86888
alistair added a commit to alistair/distribution that referenced this pull request Feb 28, 2025
@teambush1 @alistair @brackendawson
Migrate and upgrade distribution#3834
8343b80 
Signed-off-by: Alistair Bush <[email protected]>
Co-authored-by: Alistair Bush <[email protected]>
Co-authored-by: Bracken Dawson <[email protected]>
@brackendawson
Copy link
Copy Markdown
Contributor Author

brackendawson commented Jul 7, 2025

Closing in favor of the more progressed #4483

alistair added a commit to alistair/distribution that referenced this pull request Jan 11, 2026
@teambush1 @alistair @brackendawson
Migrate and upgrade distribution#3834
91814e9 
Signed-off-by: Alistair Bush <[email protected]>
Co-authored-by: Alistair Bush <[email protected]>
Co-authored-by: Bracken Dawson <[email protected]>
alistair added a commit to alistair/distribution that referenced this pull request Jan 11, 2026
@teambush1 @alistair @brackendawson
Migrate and upgrade distribution#3834
1356fba 
Signed-off-by: Alistair Bush <[email protected]>
Co-authored-by: Alistair Bush <[email protected]>
Co-authored-by: Bracken Dawson <[email protected]>
lemonprogis pushed a commit to lemonprogis/distribution that referenced this pull request Mar 26, 2026
@teambush1 @alistair
@brackendawson @lemonprogis
Migrate and upgrade distribution#3834
946d4ac 
Signed-off-by: Alistair Bush <[email protected]>
Co-authored-by: Alistair Bush <[email protected]>
Co-authored-by: Bracken Dawson <[email protected]>
Signed-off-by: lemonprogis <[email protected]>
lemonprogis pushed a commit to lemonprogis/distribution that referenced this pull request Mar 26, 2026
@teambush1 @alistair
@brackendawson @lemonprogis
Migrate and upgrade distribution#3834
12fbb32 
Signed-off-by: Alistair Bush <[email protected]>
Co-authored-by: Alistair Bush <[email protected]>
Co-authored-by: Bracken Dawson <[email protected]>
Signed-off-by: lemonprogis <[email protected]>
Signed-off-by: Edward Briggler <[email protected]>
This was referenced Mar 26, 2026
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wy65701436 wy65701436 Awaiting requested review from wy65701436

2 more reviewers

@njucjc njucjc njucjc left review comments

@bainsy88 bainsy88 bainsy88 requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@brackendawson @codecov-commenter @milosgajdos @davidspek @sajayantony @bainsy88 @njucjc @wy65701436