Remove workaround from 2.1.1 for faulty 2.1.0 manifest links #3365
Conversation
|
Originally added in #892 - I see some tickets linking to that PR (which may be worth checking to see if there's still a valid reason to keep the patch, or at least may require an entry in the change-log). From #3122 (comment)
Let me /cc @milosgajdos for visibility (I see there's an internal Docker Hub pull request linked to #892) |
|
I would put in a change log note in regardless, something to the effect of "Tags which were pushed to distribution 2.1.0 which have not been overwritten by subsequent versions will not be accessible." I'm not sure how distribution tracks the change log? |
|
I applied the |
There was a problem hiding this comment.
Maybe a little bit of a nitpick, but the TestLinkPathFuncs test here https://github.com/distribution/distribution/blob/main/registry/storage/manifeststore_test.go#L548 now makes even more sense to be in registry/storage/linkedblobstore_test.go corresponding to the file in which the link path functions are defined.
Good spot, definitely agree that test should live in linkedblobstore_test.go. Is it worth burying those lines' git history under an "I moved this" commit and using this PR, currently only a revert, to do it? |
Good point and probably not. I'll make a separate PR to move that. |
|
Sorry, been busy. From the first look of it, this looks nice. I'd like to go over this properly again hopefully by the end of this week or the weekend at the latest. Thanks for your patience! |
|
@milosgajdos sorry to ping, but can we consider this for the upcoming 2.7.2 release? We want the change because it makes the UX a little better for users who mistakenly use the config digest in place of the manifest digest, 404 rather than 500. We see users do this pretty frequently. |
|
Thanks for the ping @brackendawson :) The delay in release is my bad, indeed. It's back on the agenda for the next CNCF call thanks to @joaodrp, so I'm hoping to restart the efforts and finally getting it over the finish line soon. I need to refresh my memory about this PR first, though. |
|
@milosgajdos @thaJeztah Looks like buildx is pushing up an OCI Image Index with layers under the Merging this issue would cause the validation step for manifests list to fail, since the existence check currently falls back to the We've recently had some trouble with these manifests ourselves at GitLab: https://gitlab.com/gitlab-org/container-registry/-/issues/404 Ideally, this step should fail since these manifests are invalid according to the OCI Image Index Spec, but this behavior has been allowed to happen for some time now. CC: @joaodrp |
|
@brackendawson can you rebase so we can rekick the build? |
This reverts commit 06a098c This changes the function of linkedBlobStatter.Clear(). It was either removing the first of two possible manifest links or returning nil if none were found. Now it once again it removes only the valid manifest link or returns an error if none are found. Signed-off-by: Bracken Dawson <abdawson@gmail.com>
brackendawson
force-pushed
the
3122-remove-workaround
branch
from
7684bd2
to
b2b3f86
Compare
|
@milosgajdos done |
|
@milosgajdos: Can this one move forward? I ran into the issue this is fixing and the current behavior seems very incorrect. |
|
Sorry for the delay everyone! Thanks for the PR @brackendawson |
This file is specifically not published to the site anyway
This reverts commit 06a098c (pull request: #864)
This changes the function of linkedBlobStatter.Clear(). It was either removing the first of two possible manifest links or returning nil if none were found. Now it once again it removes only the valid manifest link or returns an error if none are found.
This adresses #3122
Signed-off-by: Bracken Dawson abdawson@gmail.com