feat: support pushing multiple tags for a single manifest#3885
feat: support pushing multiple tags for a single manifest#3885andaaron merged 2 commits intoproject-zot:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3885 +/- ##
==========================================
+ Coverage 91.44% 91.50% +0.05%
==========================================
Files 195 195
Lines 27737 27915 +178
==========================================
+ Hits 25365 25544 +179
Misses 1535 1535
+ Partials 837 836 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
vrajashkr
left a comment
There was a problem hiding this comment.
Looks good overall, but left some comments and questions.
There was a problem hiding this comment.
Pull request overview
Adds support for OCI distribution-spec “digest push with tag query params” (draft PR #600), allowing a single manifest push-by-digest to atomically create multiple tag references and return them to the client.
Changes:
- Extends
ImageStore.PutImageManifestto acceptextraTags []stringand updates call sites/mocks accordingly. - Implements
?tag=handling onPUT /v2/{name}/manifests/{digest}including normalization, limits, andOCI-Tagresponse headers. - Updates the image store index update logic to add multiple
org.opencontainers.image.ref.namedescriptors for the same digest, plus adds targeted tests.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/test/mocks/image_store_mock.go | Updates mock PutImageManifest signature to include extraTags. |
| pkg/test/image-utils/write_test.go | Adjusts test mock callback signature for new extraTags arg. |
| pkg/test/image-utils/write.go | Passes nil for extraTags when writing test images to filesystem. |
| pkg/storage/types/types.go | Extends ImageStore interface with extraTags []string param. |
| pkg/storage/storage_test.go | Adds coverage for multi-tag digest push behavior and updates existing calls with nil. |
| pkg/storage/scrub_test.go | Updates PutImageManifest calls to include nil extraTags. |
| pkg/storage/s3/s3_test.go | Updates PutImageManifest calls to include nil extraTags. |
| pkg/storage/local/local_test.go | Updates PutImageManifest calls to include nil extraTags. |
| pkg/storage/imagestore/imagestore.go | Implements multi-tag index descriptor updates when extraTags is provided; emits events per tag. |
| pkg/storage/gcs/gcs_test.go | Updates PutImageManifest calls to include nil extraTags. |
| pkg/storage/gc/gc_test.go | Updates PutImageManifest calls to include nil extraTags. |
| pkg/storage/gc/gc_internal_test.go | Updates PutImageManifest calls to include nil extraTags. |
| pkg/storage/common/common_test.go | Updates PutImageManifest calls to include nil extraTags. |
| pkg/meta/hooks.go | Updates restore path to call PutImageManifest(..., nil) after MetaDB failure. |
| pkg/extensions/sync/sync_internal_test.go | Updates PutImageManifest calls to include nil extraTags. |
| pkg/extensions/sync/destination.go | Updates destination registry manifest copy to pass nil extraTags. |
| pkg/extensions/search/search_test.go | Updates mocks/usages to match new PutImageManifest signature. |
| pkg/api/routes_test.go | Updates route tests’ image store mocks to accept extraTags. |
| pkg/api/routes.go | Adds ?tag= parsing/normalization/limits, passes tags down to storage, emits OCI-Tag headers, and adjusts MetaDB update behavior for multi-tag pushes. |
| pkg/api/controller_test.go | Adds HTTP-level tests for multi-tag digest push, errors, dedupe, and max-tag behavior. |
| pkg/api/constants/consts.go | Introduces OCI-Tag response header constant and max tag-query limit. |
| errors/errors.go | Adds ErrEmptyManifestTagQuery for empty/invalid tag query values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fb59c36 to
6d5b35f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
we may need to upgrade swagger, etc also |
|
just fyi, cc:@sudo-bmitch |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
swagger/swagger.yaml:831
- The PUT /v2/{name}/manifests/{reference} endpoint can now return 414 (Request-URI Too Long) when more than constants.MaxManifestDigestQueryTags raw tag query parameters are provided (see pkg/api/routes.go:727-734), but the Swagger spec for this operation doesn't document a 414 response. Please add a 414 response entry so generated clients/docs reflect the new behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 33 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@sudo-bmitch does regclient support this today? so we can add it to our ci/cd. |
It's in edge/main: regclient/regclient#1062 The place you see it is when pushing a non-canonical digest. I've got an outstanding TODO to add pushing multiple tags via the library. |
See opencontainers/distribution-spec#600 Signed-off-by: Andrei Aaron <andreifdaaron@gmail.com>
Also godot mandates comments ending in dots, which produces bad results in the swagger generated files, see the extra ". which is now fixed below: ``` diff --git a/swagger/docs.go b/swagger/docs.go index 84b0827..fb2c45c 100644 --- a/swagger/docs.go +++ b/swagger/docs.go @@ -114,7 +114,7 @@ const docTemplate = `{ } }, "400": { - "description": "bad request\".", + "description": "bad request", "schema": { "type": "string" } @@ -200,7 +200,7 @@ const docTemplate = `{ } }, "400": { - "description": "bad request\".", + "description": "bad request", "schema": { "type": "string" } diff --git a/swagger/swagger.json b/swagger/swagger.json index cfeb390..247f95f 100644 --- a/swagger/swagger.json +++ b/swagger/swagger.json @@ -106,7 +106,7 @@ } }, "400": { - "description": "bad request\".", + "description": "bad request", "schema": { "type": "string" } @@ -192,7 +192,7 @@ } }, "400": { - "description": "bad request\".", + "description": "bad request", "schema": { "type": "string" } diff --git a/swagger/swagger.yaml b/swagger/swagger.yaml index 57641c2..09b30dc 100644 --- a/swagger/swagger.yaml +++ b/swagger/swagger.yaml @@ -310,7 +310,7 @@ paths: schema: type: string "400": - description: bad request". + description: bad request schema: type: string "500": @@ -366,7 +366,7 @@ paths: schema: type: string "400": - description: bad request". + description: bad request schema: type: string "500": ``` Signed-off-by: Andrei Aaron <andreifdaaron@gmail.com>
vrajashkr
left a comment
There was a problem hiding this comment.
Sorry for the late follow-up review. LGTM. Left a few tiny nits and questions for later.
| want := (8192 - 2048) / (len("tag=") + zreg.TagMaxLen + 1) | ||
|
|
||
| if constants.MaxManifestDigestQueryTags != want { | ||
| t.Fatalf("MaxManifestDigestQueryTags = %d, want %d", constants.MaxManifestDigestQueryTags, want) |
There was a problem hiding this comment.
nit: since we've used github.com/stretchr/testify/assert, it would be ideal to use that here too for consistency.
There was a problem hiding this comment.
I think we use it in a single file in an older test, I haven't noticed it until now. We mosty use goconvey which has its own assertions. Here we could have used github.com/stretchr/testify/assert since goconvey doesn't work well with t.Parallel() as far as I understand.
There was a problem hiding this comment.
Cool. Sounds good. I'm fine with either the standard testing library way of doing it or testify assert. It would be good to standardize on one though to help with consistency across tests.
| ctlr := makeController(conf, dir) | ||
| cm := test.NewControllerManager(ctlr) | ||
| cm.StartServer() | ||
| time.Sleep(1000 * time.Millisecond) |
There was a problem hiding this comment.
If I recall correctly, we had a way to test for server readiness so we could potentially wait only as long as needed. (Not sure if I'm getting this mixed up with the BATS tests).
There was a problem hiding this comment.
I don't think we really needed this here. Must be a leftover.
| if err != nil { | ||
| response.WriteHeader(http.StatusInternalServerError) | ||
| if len(digestQueryTags) > 0 { | ||
| err := meta.OnUpdateManifestDigestTags(request.Context(), name, digestQueryTags, mediaType, |
There was a problem hiding this comment.
q: do we log this error inside the functions for all error cases?
There was a problem hiding this comment.
Looks like I missed one, commented.
| ctlr := makeController(conf, dir) | ||
| cm := test.NewControllerManager(ctlr) | ||
| cm.StartServer() | ||
| time.Sleep(1000 * time.Millisecond) |
There was a problem hiding this comment.
I don't think we really needed this here. Must be a leftover.
| want := (8192 - 2048) / (len("tag=") + zreg.TagMaxLen + 1) | ||
|
|
||
| if constants.MaxManifestDigestQueryTags != want { | ||
| t.Fatalf("MaxManifestDigestQueryTags = %d, want %d", constants.MaxManifestDigestQueryTags, want) |
There was a problem hiding this comment.
I think we use it in a single file in an older test, I haven't noticed it until now. We mosty use goconvey which has its own assertions. Here we could have used github.com/stretchr/testify/assert since goconvey doesn't work well with t.Parallel() as far as I understand.
| if err != nil { | ||
| response.WriteHeader(http.StatusInternalServerError) | ||
| if len(digestQueryTags) > 0 { | ||
| err := meta.OnUpdateManifestDigestTags(request.Context(), name, digestQueryTags, mediaType, |
There was a problem hiding this comment.
Looks like I missed one, commented.
|
|
||
| priorTagManifests, err := priorTagManifestsFromMetaDB(ctx, metaDB, repo, tags) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Need to add a log here.
Signed-off-by: Andrei Aaron <andreifdaaron@gmail.com>
* fix: address code review comments in #3885 (review) Signed-off-by: Andrei Aaron <andreifdaaron@gmail.com> * fix: data race in GetPort() See https://github.com/project-zot/zot/actions/runs/24045271222/job/70126983674?pr=3942 Signed-off-by: Andrei Aaron <andreifdaaron@gmail.com> * fix(test): reuse ReadLogFileAndSearchString for auto-port log; throttle poll loop Signed-off-by: Andrei Aaron <andreifdaaron@gmail.com> --------- Signed-off-by: Andrei Aaron <andreifdaaron@gmail.com>
See opencontainers/distribution-spec#600
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.