remote: use DeleteScope (with "delete" action) for manifest deletion#2266
Conversation
retryError consumed resp.Body with io.ReadAll but discarded the bytes, leaving the body closed. When the retry loop exhausted its attempts and called CheckError, that function tried to read the same body and found nothing, producing a generic "unexpected status code" error instead of the structured registry error (e.g. MANIFEST_UNKNOWN). After reading the body, restore it with io.NopCloser(bytes.NewReader(b)) so subsequent callers see the same bytes. Fixes google#2125 Signed-off-by: alliasgher <alliasgher123@gmail.com>
The OCI Distribution spec (§10.5) and Docker Registry API require HTTP 202 Accepted for successful chunk upload PATCH responses. The registry was returning 204 No Content, which caused clients that strictly validate the response code to fail or behave unexpectedly. Update the two PATCH response paths in blobs.go and the corresponding test expectations. Fixes google#2198 Signed-off-by: alliasgher <alliasgher123@gmail.com>
DeleteScope was defined as an alias for PushScope ("push,pull") with a
comment saying it was temporary. Registries that require an explicit
"delete" token action — such as IBM Cloud Container Registry — rejected
delete requests with DENIED because the token only requested "push,pull".
Changes:
- Update DeleteScope to "push,pull,delete" so the token request includes
the delete action.
- Add makeDeleteClient that creates an HTTP client using DeleteScope
rather than the shared push-scoped transport.
- Wire Pusher.Delete to use makeDeleteClient instead of the shared writer.
Fixes google#2184
Signed-off-by: alliasgher <alliasgher123@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2266 +/- ##
==========================================
+ Coverage 53.01% 53.03% +0.01%
==========================================
Files 165 165
Lines 11226 11237 +11
==========================================
+ Hits 5951 5959 +8
- Misses 4556 4557 +1
- Partials 719 721 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: alliasgher <alliasgher123@gmail.com>
|
Can you add tests to show that the delete scope is propagated? I don't see the value in my manual testing. |
…ction Add TestDeleteRequestsDeleteScope to delete_test.go. The test spins up a fake registry + token endpoint, wires all HTTP connections from the client to that server via a custom DialContext (so the non-loopback host name in the www-authenticate realm passes the bearer transport's private-address check), and asserts that: * transport.DeleteScope literally contains "delete" * the scope parameter on the token request contains "delete" This guards against accidentally reverting DeleteScope to push,pull and regresses registries like IBM Cloud Container Registry that require the explicit delete action. Signed-off-by: Ali <alliasgher123@gmail.com>
|
Added |
|
Looks good! Once the linter errors are resolved I'll approve and merge this change. |
The woke linter flags "Sanity" as potentially insensitive. Use "Quick check" per the linter's suggested alternatives. Signed-off-by: Ali <alliasgher123@gmail.com>
…oogle#2266) * transport: restore resp.Body in retryError so CheckError can parse it retryError consumed resp.Body with io.ReadAll but discarded the bytes, leaving the body closed. When the retry loop exhausted its attempts and called CheckError, that function tried to read the same body and found nothing, producing a generic "unexpected status code" error instead of the structured registry error (e.g. MANIFEST_UNKNOWN). After reading the body, restore it with io.NopCloser(bytes.NewReader(b)) so subsequent callers see the same bytes. Fixes google#2125 Signed-off-by: alliasgher <alliasgher123@gmail.com> * pkg/registry: return 202 Accepted for PATCH chunk uploads The OCI Distribution spec (§10.5) and Docker Registry API require HTTP 202 Accepted for successful chunk upload PATCH responses. The registry was returning 204 No Content, which caused clients that strictly validate the response code to fail or behave unexpectedly. Update the two PATCH response paths in blobs.go and the corresponding test expectations. Fixes google#2198 Signed-off-by: alliasgher <alliasgher123@gmail.com> * remote: use DeleteScope (with "delete" action) for manifest deletion DeleteScope was defined as an alias for PushScope ("push,pull") with a comment saying it was temporary. Registries that require an explicit "delete" token action — such as IBM Cloud Container Registry — rejected delete requests with DENIED because the token only requested "push,pull". Changes: - Update DeleteScope to "push,pull,delete" so the token request includes the delete action. - Add makeDeleteClient that creates an HTTP client using DeleteScope rather than the shared push-scoped transport. - Wire Pusher.Delete to use makeDeleteClient instead of the shared writer. Fixes google#2184 Signed-off-by: alliasgher <alliasgher123@gmail.com> * transport: use errors.As in test to fix errorlint Signed-off-by: alliasgher <alliasgher123@gmail.com> * remote: add test that Delete requests a token scope with the delete action Add TestDeleteRequestsDeleteScope to delete_test.go. The test spins up a fake registry + token endpoint, wires all HTTP connections from the client to that server via a custom DialContext (so the non-loopback host name in the www-authenticate realm passes the bearer transport's private-address check), and asserts that: * transport.DeleteScope literally contains "delete" * the scope parameter on the token request contains "delete" This guards against accidentally reverting DeleteScope to push,pull and regresses registries like IBM Cloud Container Registry that require the explicit delete action. Signed-off-by: Ali <alliasgher123@gmail.com> * remote: replace "Sanity-check" with "Quick check" in test comment The woke linter flags "Sanity" as potentially insensitive. Use "Quick check" per the linter's suggested alternatives. Signed-off-by: Ali <alliasgher123@gmail.com> --------- Signed-off-by: alliasgher <alliasgher123@gmail.com> Signed-off-by: Ali <alliasgher123@gmail.com>
|
How many registries was this tested against? Our registry rejects the new scope, and while we're happy to fix this, I am worried that this is going to break others 🤔 |
Description
Fixes #2184.
DeleteScopewas defined as an alias forPushScope("push,pull") with a comment "For now DELETE is PUSH". Registries that require an explicitdeletetoken action — such as IBM Cloud Container Registry — rejected delete requests because the token only asked forpush,pull.Changes
transport/scope.go: UpdateDeleteScopeto"push,pull,delete"to include the standarddeleteaction.write.go: AddmakeDeleteClientwhich creates anhttp.Clientwhose transport usesDeleteScopeinstead ofPushScope.pusher.go:Pusher.Deletenow callsmakeDeleteClientinstead of reusing the shared push writer. This ensures the token includes thedeleteaction regardless of whether a push client was previously created.Compatibility
The extra
deleteaction in the scope is harmless for registries that don't require it — they ignore unknown actions or grant the same token. No existing tests were broken.