-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add option to enable sparse indexes #3536
Conversation
One particular aspect of this is the requirement that a sparse index / manifest list has the same digest as the full index / manifest list. |
Hi Daniel!
Yes, absolutely, but from a PR perspective I would consider that to be a client problem. I've got a PR open for skopeo [1] as an example client implementation and a PR open for the image library [2] to enable a stronger "don't change the digest" option on copies, which once merged could be used by skopeo and oc image mirror. |
2a72348
to
fa5d3cb
Compare
fa5d3cb
to
166fcf2
Compare
For what it's worth, we allow "sparse indexes" to be on the registry today via some convoluted means of copying the entire set of images and index, and then deleting the unneeded manifests:
This does create some weird issues though, like not being able to push the same manifest with a different tag. And many clients will report the 404 error on pulling the platform manifest while that tag is listed in API's and probably most GUI's, leading to confusing behavior for users. Allowing a sparse index is going to create some issues for tooling that isn't expecting this, e.g. tools that do a full copy of an index and images. I'd also question how vulnerability scanners should react if parts of the image cannot be pulled to scan. Overall, I'm interested in this feature and would be very likely to add it to regclient/regsync for more efficient image mirroring. But I'd want to move cautiously considering the potential issues it will create. |
I agree, its one of the reasons I thought this should go in as an optional config option, even though I think it makes sense as the default behaviour. On my list next is to go around the clients (cmdline and CRI) and see how they respond, and raise bugs/prs where the UX isn't as good as it could be. I didn't want to raise those without a supporting registry, and preferably this too: opencontainers/distribution-spec#310
Two good points here, I hadn't considered vuln scanners (will have to try those) and copying when the index is sparse. I already raised something for skopeo to enable sparse copying, but will need to look at another PR to handle copying when the index is already sparse.
👍 |
166fcf2
to
f89f05b
Compare
Hi to all subscribed. Based on feedback around the configuration options, I've revisited this one to change the language in the config and to add the suggestion from @SteveLasker to validate a subset of platforms. Also converted to draft as I haven't finished writing the tests yet, but wanted to get it out for an initial look to make sure people agree with the approach. |
f89f05b
to
311ea2e
Compare
52a020c
to
52bc959
Compare
This took me longer to get back to than I hoped, but bringing this back again for review. I've extended the functionality to be able to validate a subset of platforms. Because the commit is substantially different I resolved all the previous conversations too. Skopeo from 1.6.0 includes the command-line option ( |
21d8e19
to
fd8909b
Compare
Codecov Report
@@ Coverage Diff @@
## main #3536 +/- ##
==========================================
+ Coverage 57.43% 57.59% +0.15%
==========================================
Files 105 105
Lines 10839 10889 +50
==========================================
+ Hits 6225 6271 +46
- Misses 3930 3934 +4
Partials 684 684
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
I thought I'd look at the client output for pulling a sparse manifest list where the platform image is missing. I copied the latest alpine index and s390x image to my registry, leaving out the other platforms, and am now attempting to use the manifest list on amd64. Overall, everything fails in sensible ways but the error messages could be improved in the clients. skopeo inspect - error message could be clearer but you can tell its looking for the "target platform"
skopeo copy - It will fail on the first missing image when trying to copy them all, would be better to copy those that exist. Doesn't mention platform when pulling based on system, which could be improved.
docker pull - error is different, but doens't mention platform or a manifest list.
docker build - as per docker pull
podman pull - as per skopeo copy
podman build - as per podman pull
buildah - as per skopeo copy
grype - doesn't mention platforms in the error messagegrype doesn't seem to have options specifically for manifest lists anyway, and will only scan the system image. There are calls to improve already: anchore/grype#485
|
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.
i've added some comments on the proposed api, but i should note that i am not a contributor to this repo, so i don't expect my input to carry any particular weight, i may be overlooking general best practices/guidance/philosophies that this repo follows for its apis.
Thanks for the review, right now I'm just trying to get something that feels right so any input is valued! |
daeef49
to
3f33878
Compare
As you've been active recently, @thaJeztah and @davidspek I'd love a review on this one :) |
4e5267a
to
337d133
Compare
Hey @neersighted, would you mind weighing into this PR, please? |
This needs a rebase @Jamstah also, friendly ping to @neersighted 😄 |
337d133
to
165b247
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.
This LGTM and I feel would be valuable to end users, but I've some configuration UX/stylistic questions/suggestions we could discuss before we move forward with this.
Curious what you think @Jamstah
I'm still personally not experienced with distribution/distribution's code to say whether or not this is a good idea for this codebase... I think that as long as the maintainers don't forsee integrity/architectural issues arising from intentionally allowing sparse indexes (something that can already arise organically today, as I understand it) that this very much makes sense and is in demand by a small but significant minority of users. |
The key in adding this in is, it's entirely optional by the operators themselves. It's not enforced - so if say user X makes a deliberate decision to do this, knowing that the integrity of the store will not be maintained, then IMHO they should be ok to make that decision. We need to make sure this is properly highlighted in the docs In other words: this isn't forced upon anyone - it's entirely optional. |
2b3ce73
to
cac474e
Compare
@milosgajdos ready for review again now I finally got to the config reshuffle. |
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.
I had a look at this today and left some comments. Sorry for dragging this @Jamstah. I'm all in for this feature to get in, just to make it easier to understand 😄
@@ -74,7 +76,7 @@ func (ms *manifestListHandler) Put(ctx context.Context, manifestList distributio | |||
func (ms *manifestListHandler) verifyManifest(ctx context.Context, mnfst distribution.Manifest, skipDependencyVerification bool) error { | |||
var errs distribution.ErrManifestVerification | |||
|
|||
if !skipDependencyVerification { | |||
if ms.validateImagesExist && !skipDependencyVerification { |
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.
suggestion: should we swap these two so we break the logic circuit on skipDependencyVerification
by default
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.
For what purpose? I'm not sure it matters.
@@ -101,3 +107,20 @@ func (ms *manifestListHandler) verifyManifest(ctx context.Context, mnfst distrib | |||
|
|||
return nil | |||
} | |||
|
|||
func (ms *manifestListHandler) platformMustExist(descriptor distribution.Descriptor) bool { |
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.
I'm finding it a bit hard to follow the boolean logic in the flow of verifyManifest
that calls this func
but maybe it's the naming of the functions and the boolean logic I must do when reading the the func flow that slightly confuse me.
I thought the validateImagesExistPlatforms
list would work as follows:
if validateImagesExistPlatforms
is empty then I'd expect we are saying no platform needs to exist in which case I'd expect the if len(validateImagesExistPlatforms) == 0
to return false
which would then lead to !ms.platformMustExist(manifestDescriptor)
skipping the validation 🤔
Maybe renaming this func
to platformSkipExist
and changing the logic would make it easier to read/understand to me personally
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.
I think switching it to platformSkipExist would make it even more confusing.
I've added a comment and changed the check to not use continue, which I think helps with clarity. wdyt?
@milosgajdos thakns for the further review, somehow I completely missed it. Good suggestions, mostly implemented, as above. No hurry, I know you're out at the moment :) |
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.
This change looks good to me and I don't want to keep blocking this PR. PTAL @thaJeztah
CC: @thaJeztah @squizzi @corhere you folks mind having a look at this? It's been sitting here a while; would be good put it into the beta release |
@thaJeztah @corhere @squizzi would appreciate some help to get this one moving please |
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.
I remember looking at this a few times in the past and am happy to finally have this unblocked.
Rebased and squashed. |
docs/content/about/configuration.md
Outdated
validation: | ||
manifests: | ||
indexes: | ||
platforms: [All|None|List] |
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 small thing I noticed: these options shouldnt be capitalized here 🤔
Enable configuration options that can selectively disable validation that dependencies exist within the registry before the image index is uploaded. This enables sparse indexes, where a registry holds a manifest index that could be signed (so the digest must not change) but does not hold every referenced image in the index. The use case for this is when a registry mirror does not need to mirror all platforms, but does need to maintain the digests of all manifests either because they are signed or because they are pulled by digest. The registry administrator can also select specific image architectures that must exist in the registry, enabling a registry operator to select only the platforms they care about and ensure all image indexes uploaded to the registry are valid for those platforms. Signed-off-by: James Hewitt <james.hewitt@uk.ibm.com>
Enable configuration options that can selectively disable validation that dependencies exist within the registry before the image index is uploaded.
This enables sparse indexes, where a registry holds a manifest index that could be signed (so the digest must not change) but does not hold every referenced image in the index. The use case for this is when a registry mirror does not need to mirror all platforms, but does need to maintain the digests of all manifests either because they are signed or because they are pulled by digest.
The registry administrator can also select specific image architectures that must exist in the registry, enabling a registry operator to select only the platforms they care about and ensure all image indexes uploaded to the registry are valid for those platforms.
Closes #3628