-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Support OCI Image Manifests with artifactType and subject properties #3834
Conversation
Codecov ReportPatch coverage:
❗ 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
☔ View full report in Codecov by Sentry. |
c0832a8
to
69b1547
Compare
There was a problem hiding this 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
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>
69b1547
to
27a9632
Compare
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>
Rebased and comment addressed. |
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 |
@brackendawson @milosgajdos What's the status of this PR (other than that it needs another rebase)? |
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. |
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. |
Just thought I'd mention v1.1.0-rc5 was released a couple weeks ago. |
return "", err | ||
} | ||
|
||
artifactType := "_" + url.QueryEscape(v.mediaType) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)))
There was a problem hiding this comment.
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.
v1.1.0-rc6 dropped. 🙃 I will finish this when v1.1.0 is released, I don't want to start again again. |
@brackendawson @milosgajdos since this is now released in OCI (image/distribution) is there any interest or plan to land this? |
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. |
This needs a wild rebase now @brackendawson 😄 |
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>
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>
Closing in favor of the more progressed #4483 |
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