From 6ae6df7d75aec4f40a8d22bc1d2ff4b0908bc9c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Pereira?= <484633+joaodrp@users.noreply.github.com> Date: Thu, 27 May 2021 23:13:59 +0100 Subject: [PATCH 1/5] Add tag delete API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: João Pereira <484633+joaodrp@users.noreply.github.com> --- docs/spec/api.md | 11 ++-- registry/api/v2/descriptors.go | 6 +-- registry/client/repository.go | 25 ++++++++- registry/client/repository_test.go | 37 +++++++++++++ registry/handlers/api_test.go | 87 ++++++++++++++++++++++++++++++ registry/handlers/manifests.go | 21 +++++++- registry/storage/tagstore.go | 11 +--- registry/storage/tagstore_test.go | 4 +- 8 files changed, 179 insertions(+), 23 deletions(-) diff --git a/docs/spec/api.md b/docs/spec/api.md index 1f7a4a7b..38689c4b 100644 --- a/docs/spec/api.md +++ b/docs/spec/api.md @@ -1113,7 +1113,7 @@ A list of methods and URIs are covered in the table below: | GET | `/v2//tags/list` | Tags | Fetch the tags under the repository identified by `name`. | | GET | `/v2//manifests/` | Manifest | Fetch the manifest identified by `name` and `reference` where `reference` can be a tag or digest. A `HEAD` request can also be issued to this endpoint to obtain resource information without receiving all data. | | PUT | `/v2//manifests/` | Manifest | Put the manifest identified by `name` and `reference` where `reference` can be a tag or digest. | -| DELETE | `/v2//manifests/` | Manifest | Delete the manifest identified by `name` and `reference`. Note that a manifest can _only_ be deleted by `digest`. | +| DELETE | `/v2//manifests/` | Manifest | Delete the manifest or tag identified by `name` and `reference` where `reference` can be a tag or digest. Note that a manifest can _only_ be deleted by digest. | | GET | `/v2//blobs/` | Blob | Retrieve the blob from the registry identified by `digest`. A `HEAD` request can also be issued to this endpoint to obtain resource information without receiving all data. | | DELETE | `/v2//blobs/` | Blob | Delete the blob identified by `name` and `digest` | | POST | `/v2//blobs/uploads/` | Initiate Blob Upload | Initiate a resumable blob upload. If successful, an upload location will be provided to complete the upload. Optionally, if the `digest` parameter is present, the request body will be used to complete the upload in a single request. | @@ -1142,7 +1142,8 @@ The error codes encountered via the API are enumerated in the following table: `MANIFEST_UNVERIFIED` | manifest failed signature verification | During manifest upload, if the manifest fails signature verification, this error will be returned. `NAME_INVALID` | invalid repository name | Invalid repository name encountered either during manifest validation or any API operation. `NAME_UNKNOWN` | repository name not known to registry | This is returned if the name used during an operation is unknown to the registry. - `PAGINATION_NUMBER_INVALID` | invalid number of results requested | Returned when the `n` parameter (number of results to return) is not an integer, or `n` is negative. + `PAGINATION_NUMBER_INVALID` | invalid number of results requested | Returned when the "n" parameter (number of results to return) is not an integer, or "n" is negative. + `RANGE_INVALID` | invalid content range | When a layer is uploaded, the provided range is checked against the uploaded chunk. This error is returned if the range is out of order. `SIZE_INVALID` | provided length did not match content length | When a layer is uploaded, the provided size will be checked against the uploaded content. If they do not match, this error will be returned. `TAG_INVALID` | manifest tag did not match URI | During a manifest upload, if the tag in the manifest does not match the uri tag, this error will be returned. `UNAUTHORIZED` | authentication required | The access controller was unable to authenticate the client. Often this will be accompanied by a Www-Authenticate HTTP response header indicating how to authenticate. @@ -2240,7 +2241,7 @@ The error codes that may be included in the response body are enumerated below: #### DELETE Manifest -Delete the manifest identified by `name` and `reference`. Note that a manifest can _only_ be deleted by `digest`. +Delete the manifest or tag identified by `name` and `reference` where `reference` can be a tag or digest. Note that a manifest can _only_ be deleted by digest. @@ -2475,7 +2476,7 @@ Content-Type: application/json } ``` -The specified `name` or `reference` are unknown to the registry and the delete was unable to proceed. Clients can assume the manifest was already deleted if this response is returned. +The specified `name` or `reference` are unknown to the registry and the delete was unable to proceed. Clients can assume the manifest or tag was already deleted if this response is returned. @@ -2494,7 +2495,7 @@ The error codes that may be included in the response body are enumerated below: 405 Method Not Allowed ``` -Manifest delete is not allowed because the registry is configured as a pull-through cache or `delete` has been disabled. +Manifest or tag delete is not allowed because the registry is configured as a pull-through cache or `delete` has been disabled. diff --git a/registry/api/v2/descriptors.go b/registry/api/v2/descriptors.go index 1dbe6823..b1090fc9 100644 --- a/registry/api/v2/descriptors.go +++ b/registry/api/v2/descriptors.go @@ -655,7 +655,7 @@ var routeDescriptors = []RouteDescriptor{ }, { Method: "DELETE", - Description: "Delete the manifest identified by `name` and `reference`. Note that a manifest can _only_ be deleted by `digest`.", + Description: "Delete the manifest or tag identified by `name` and `reference` where `reference` can be a tag or digest. Note that a manifest can _only_ be deleted by digest.", Requests: []RequestDescriptor{ { Headers: []ParameterDescriptor{ @@ -691,7 +691,7 @@ var routeDescriptors = []RouteDescriptor{ tooManyRequestsDescriptor, { Name: "Unknown Manifest", - Description: "The specified `name` or `reference` are unknown to the registry and the delete was unable to proceed. Clients can assume the manifest was already deleted if this response is returned.", + Description: "The specified `name` or `reference` are unknown to the registry and the delete was unable to proceed. Clients can assume the manifest or tag was already deleted if this response is returned.", StatusCode: http.StatusNotFound, ErrorCodes: []errcode.ErrorCode{ ErrorCodeNameUnknown, @@ -704,7 +704,7 @@ var routeDescriptors = []RouteDescriptor{ }, { Name: "Not allowed", - Description: "Manifest delete is not allowed because the registry is configured as a pull-through cache or `delete` has been disabled.", + Description: "Manifest or tag delete is not allowed because the registry is configured as a pull-through cache or `delete` has been disabled.", StatusCode: http.StatusMethodNotAllowed, ErrorCodes: []errcode.ErrorCode{ errcode.ErrorCodeUnsupported, diff --git a/registry/client/repository.go b/registry/client/repository.go index 00b31a48..a3379c0a 100644 --- a/registry/client/repository.go +++ b/registry/client/repository.go @@ -364,7 +364,30 @@ func (t *tags) Tag(ctx context.Context, tag string, desc distribution.Descriptor } func (t *tags) Untag(ctx context.Context, tag string) error { - panic("not implemented") + ref, err := reference.WithTag(t.name, tag) + if err != nil { + return err + } + u, err := t.ub.BuildManifestURL(ref) + if err != nil { + return err + } + + req, err := http.NewRequest("DELETE", u, nil) + if err != nil { + return err + } + + resp, err := t.client.Do(req) + if err != nil { + return err + } + defer resp.Body.Close() + + if SuccessStatus(resp.StatusCode) { + return nil + } + return HandleErrorResponse(resp) } type manifests struct { diff --git a/registry/client/repository_test.go b/registry/client/repository_test.go index bd08bbd9..2e8e978a 100644 --- a/registry/client/repository_test.go +++ b/registry/client/repository_test.go @@ -1425,6 +1425,43 @@ func TestManifestTags(t *testing.T) { // TODO(dmcgowan): Check for error cases } +func TestTagDelete(t *testing.T) { + tag := "latest" + repo, _ := reference.WithName("test.example.com/repo/delete") + newRandomSchemaV1Manifest(repo, tag, 1) + + var m testutil.RequestResponseMap + m = append(m, testutil.RequestResponseMapping{ + Request: testutil.Request{ + Method: "DELETE", + Route: "/v2/" + repo.Name() + "/manifests/" + tag, + }, + Response: testutil.Response{ + StatusCode: http.StatusAccepted, + Headers: map[string][]string{ + "Content-Length": {"0"}, + }, + }, + }) + + e, c := testServer(m) + defer c() + + r, err := NewRepository(repo, e, nil) + if err != nil { + t.Fatal(err) + } + ctx := context.Background() + ts := r.Tags(ctx) + + if err := ts.Untag(ctx, tag); err != nil { + t.Fatal(err) + } + if err := ts.Untag(ctx, tag); err == nil { + t.Fatal("expected error deleting unknown tag") + } +} + func TestObtainsErrorForMissingTag(t *testing.T) { repo, _ := reference.WithName("test.example.com/repo") diff --git a/registry/handlers/api_test.go b/registry/handlers/api_test.go index e0c30b36..695a87b7 100644 --- a/registry/handlers/api_test.go +++ b/registry/handlers/api_test.go @@ -844,6 +844,93 @@ func TestManifestAPI(t *testing.T) { testManifestAPIManifestList(t, env2, schema2Args) } +func TestManifestAPI_DeleteTag(t *testing.T) { + env := newTestEnv(t, false) + defer env.Shutdown() + + imageName, err := reference.WithName("foo/bar") + checkErr(t, err, "building image name") + + tag := "latest" + dgst := createRepository(env, t, imageName.Name(), tag) + + ref, err := reference.WithTag(imageName, tag) + checkErr(t, err, "building tag reference") + + u, err := env.builder.BuildManifestURL(ref) + checkErr(t, err, "building tag URL") + + resp, err := httpDelete(u) + m := "deleting tag" + checkErr(t, err, m) + defer resp.Body.Close() + + checkResponse(t, m, resp, http.StatusAccepted) + if resp.Body != http.NoBody { + t.Fatal("unexpected response body") + } + + msg := "checking tag no longer exists" + resp, err = http.Get(u) + checkErr(t, err, msg) + checkResponse(t, msg, resp, http.StatusNotFound) + + digestRef, err := reference.WithDigest(imageName, dgst) + checkErr(t, err, "building manifest digest reference") + + u, err = env.builder.BuildManifestURL(digestRef) + checkErr(t, err, "building manifest URL") + + msg = "checking manifest still exists" + resp, err = http.Head(u) + checkErr(t, err, msg) + checkResponse(t, msg, resp, http.StatusOK) +} + +func TestManifestAPI_DeleteTag_Unknown(t *testing.T) { + env := newTestEnv(t, false) + defer env.Shutdown() + + imageName, err := reference.WithName("foo/bar") + checkErr(t, err, "building named object") + + ref, err := reference.WithTag(imageName, "latest") + checkErr(t, err, "building tag reference") + + u, err := env.builder.BuildManifestURL(ref) + checkErr(t, err, "building tag URL") + + resp, err := httpDelete(u) + msg := "deleting unknown tag" + checkErr(t, err, msg) + defer resp.Body.Close() + + checkResponse(t, msg, resp, http.StatusNotFound) + checkBodyHasErrorCodes(t, msg, resp, v2.ErrorCodeManifestUnknown) +} + +func TestManifestAPI_DeleteTag_ReadOnly(t *testing.T) { + env := newTestEnv(t, false) + defer env.Shutdown() + env.app.readOnly = true + + imageName, err := reference.WithName("foo/bar") + checkErr(t, err, "building named object") + + ref, err := reference.WithTag(imageName, "latest") + checkErr(t, err, "building tag reference") + + u, err := env.builder.BuildManifestURL(ref) + checkErr(t, err, "building URL") + + resp, err := httpDelete(u) + msg := "deleting tag" + checkErr(t, err, msg) + defer resp.Body.Close() + + checkResponse(t, msg, resp, http.StatusMethodNotAllowed) +} + // storageManifestErrDriverFactory implements the factory.StorageDriverFactory interface. type storageManifestErrDriverFactory struct{} diff --git a/registry/handlers/manifests.go b/registry/handlers/manifests.go index 03209754..5a208cdd 100644 --- a/registry/handlers/manifests.go +++ b/registry/handlers/manifests.go @@ -17,6 +17,7 @@ import ( "github.com/distribution/distribution/v3/registry/api/errcode" v2 "github.com/distribution/distribution/v3/registry/api/v2" "github.com/distribution/distribution/v3/registry/auth" + "github.com/distribution/distribution/v3/registry/storage/driver" "github.com/gorilla/handlers" "github.com/opencontainers/go-digest" v1 "github.com/opencontainers/image-spec/specs-go/v1" @@ -481,15 +482,31 @@ func (imh *manifestHandler) applyResourcePolicy(manifest distribution.Manifest) } -// DeleteManifest removes the manifest with the given digest from the registry. +// DeleteManifest removes the manifest with the given digest or the tag with the given name from the registry. func (imh *manifestHandler) DeleteManifest(w http.ResponseWriter, r *http.Request) { dcontext.GetLogger(imh).Debug("DeleteImageManifest") - if imh.Tag != "" { + if imh.App.isCache { imh.Errors = append(imh.Errors, errcode.ErrorCodeUnsupported) return } + if imh.Tag != "" { + tagService := imh.Repository.Tags(imh.Context) + if err := tagService.Untag(imh.Context, imh.Tag); err != nil { + switch err.(type) { + case distribution.ErrTagUnknown: + case driver.PathNotFoundError: + imh.Errors = append(imh.Errors, v2.ErrorCodeManifestUnknown) + default: + imh.Errors = append(imh.Errors, errcode.ErrorCodeUnknown) + } + return + } + w.WriteHeader(http.StatusAccepted) + return + } + manifests, err := imh.Repository.Manifests(imh) if err != nil { imh.Errors = append(imh.Errors, err) diff --git a/registry/storage/tagstore.go b/registry/storage/tagstore.go index 7bbb2f01..9c16330b 100644 --- a/registry/storage/tagstore.go +++ b/registry/storage/tagstore.go @@ -112,16 +112,7 @@ func (ts *tagStore) Untag(ctx context.Context, tag string) error { return err } - if err := ts.blobStore.driver.Delete(ctx, tagPath); err != nil { - switch err.(type) { - case storagedriver.PathNotFoundError: - return nil // Untag is idempotent, we don't care if it didn't exist - default: - return err - } - } - - return nil + return ts.blobStore.driver.Delete(ctx, tagPath) } // linkedBlobStore returns the linkedBlobStore for the named tag, allowing one diff --git a/registry/storage/tagstore_test.go b/registry/storage/tagstore_test.go index dca486fd..c17137d3 100644 --- a/registry/storage/tagstore_test.go +++ b/registry/storage/tagstore_test.go @@ -98,8 +98,8 @@ func TestTagStoreUnTag(t *testing.T) { desc := distribution.Descriptor{Digest: "sha256:bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"} err := tags.Untag(ctx, "latest") - if err != nil { - t.Error(err) + if err == nil { + t.Error("expected error removing unknown tag") } err = tags.Tag(ctx, "latest", desc) From 22053f57b0580cb8eedbb105440aa625f0afdc7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Pereira?= <484633+joaodrp@users.noreply.github.com> Date: Thu, 27 May 2021 23:51:55 +0100 Subject: [PATCH 2/5] Fix listener tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: João Pereira <484633+joaodrp@users.noreply.github.com> --- notifications/listener_test.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/notifications/listener_test.go b/notifications/listener_test.go index 74439668..e34707d7 100644 --- a/notifications/listener_test.go +++ b/notifications/listener_test.go @@ -199,11 +199,20 @@ func checkExerciseRepository(t *testing.T, repository distribution.Repository, r t.Fatalf("mismatching digest from payload and put") } + if err := repository.Tags(ctx).Tag(ctx, tag, distribution.Descriptor{Digest: dgst}); err != nil { + t.Fatalf("unexpected error tagging manifest: %v", err) + } + _, err = manifests.Get(ctx, dgst) if err != nil { t.Fatalf("unexpected error fetching manifest: %v", err) } + err = repository.Tags(ctx).Untag(ctx, m.Tag) + if err != nil { + t.Fatalf("unexpected error deleting tag: %v", err) + } + err = manifests.Delete(ctx, dgst) if err != nil { t.Fatalf("unexpected error deleting blob: %v", err) @@ -216,11 +225,6 @@ func checkExerciseRepository(t *testing.T, repository distribution.Repository, r } } - err = repository.Tags(ctx).Untag(ctx, m.Tag) - if err != nil { - t.Fatalf("unexpected error deleting tag: %v", err) - } - err = remover.Remove(ctx, repository.Named()) if err != nil { t.Fatalf("unexpected error deleting repo: %v", err) From 1398d3b5c6cc2e5162b21c2f7319972035df2edf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Pereira?= <484633+joaodrp@users.noreply.github.com> Date: Fri, 28 May 2021 10:11:48 +0100 Subject: [PATCH 3/5] Remove unrelated spec update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: João Pereira <484633+joaodrp@users.noreply.github.com> --- docs/spec/api.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docs/spec/api.md b/docs/spec/api.md index 38689c4b..f3d97d94 100644 --- a/docs/spec/api.md +++ b/docs/spec/api.md @@ -1142,8 +1142,7 @@ The error codes encountered via the API are enumerated in the following table: `MANIFEST_UNVERIFIED` | manifest failed signature verification | During manifest upload, if the manifest fails signature verification, this error will be returned. `NAME_INVALID` | invalid repository name | Invalid repository name encountered either during manifest validation or any API operation. `NAME_UNKNOWN` | repository name not known to registry | This is returned if the name used during an operation is unknown to the registry. - `PAGINATION_NUMBER_INVALID` | invalid number of results requested | Returned when the "n" parameter (number of results to return) is not an integer, or "n" is negative. - `RANGE_INVALID` | invalid content range | When a layer is uploaded, the provided range is checked against the uploaded chunk. This error is returned if the range is out of order. + `PAGINATION_NUMBER_INVALID` | invalid number of results requested | Returned when the `n` parameter (number of results to return) is not an integer, or `n` is negative. `SIZE_INVALID` | provided length did not match content length | When a layer is uploaded, the provided size will be checked against the uploaded content. If they do not match, this error will be returned. `TAG_INVALID` | manifest tag did not match URI | During a manifest upload, if the tag in the manifest does not match the uri tag, this error will be returned. `UNAUTHORIZED` | authentication required | The access controller was unable to authenticate the client. Often this will be accompanied by a Www-Authenticate HTTP response header indicating how to authenticate. From 81f081f91b1cf1c3c1375cf225de8ee05b00fb6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Pereira?= <484633+joaodrp@users.noreply.github.com> Date: Fri, 28 May 2021 10:33:26 +0100 Subject: [PATCH 4/5] Group case values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: João Pereira <484633+joaodrp@users.noreply.github.com> --- registry/handlers/manifests.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/registry/handlers/manifests.go b/registry/handlers/manifests.go index 5a208cdd..8b365c70 100644 --- a/registry/handlers/manifests.go +++ b/registry/handlers/manifests.go @@ -495,8 +495,7 @@ func (imh *manifestHandler) DeleteManifest(w http.ResponseWriter, r *http.Reques tagService := imh.Repository.Tags(imh.Context) if err := tagService.Untag(imh.Context, imh.Tag); err != nil { switch err.(type) { - case distribution.ErrTagUnknown: - case driver.PathNotFoundError: + case distribution.ErrTagUnknown, driver.PathNotFoundError: imh.Errors = append(imh.Errors, v2.ErrorCodeManifestUnknown) default: imh.Errors = append(imh.Errors, errcode.ErrorCodeUnknown) From 033683d629401c4cbe9f3f44239982bf2a743672 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Pereira?= <484633+joaodrp@users.noreply.github.com> Date: Sun, 6 Jun 2021 19:53:34 +0100 Subject: [PATCH 5/5] apply feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: João Pereira <484633+joaodrp@users.noreply.github.com> --- registry/handlers/manifests.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/registry/handlers/manifests.go b/registry/handlers/manifests.go index 8b365c70..5af59aaf 100644 --- a/registry/handlers/manifests.go +++ b/registry/handlers/manifests.go @@ -492,13 +492,14 @@ func (imh *manifestHandler) DeleteManifest(w http.ResponseWriter, r *http.Reques } if imh.Tag != "" { + dcontext.GetLogger(imh).Debug("DeleteImageTag") tagService := imh.Repository.Tags(imh.Context) if err := tagService.Untag(imh.Context, imh.Tag); err != nil { switch err.(type) { case distribution.ErrTagUnknown, driver.PathNotFoundError: - imh.Errors = append(imh.Errors, v2.ErrorCodeManifestUnknown) + imh.Errors = append(imh.Errors, v2.ErrorCodeManifestUnknown.WithDetail(err)) default: - imh.Errors = append(imh.Errors, errcode.ErrorCodeUnknown) + imh.Errors = append(imh.Errors, errcode.ErrorCodeUnknown.WithDetail(err)) } return }