New Tags API by sergeyfd · Pull Request #2169 · distribution/distribution

sergeyfd · 2017-01-21T23:18:01Z

This is a PR to cover first two first steps of the plan discussed at #1956 It introduces new TagAPI which allows deleting tags for a given image and also supports legacy Tags List URL /v2/<name>/tags/list by redirecting it to the new one /v2/<name>/tags/_list.

@stevvooe please take a look when you have a chance. It seems rather big but most of the changes are caused by renaming some functions to better reflect their new purpose.

Signed-off-by: Serge Dubrouski sergeyfd@gmail.com

stevvooe · 2017-01-23T23:17:12Z

docs/spec/api.md.tmpl

 for an image repository can be retrieved with the following request:

-    GET /v2/<name>/tags/list
+    GET /v2/<name>/tags/_list


This doesn't match was was added above. Please make sure to regenerate from the template.

I did not realize that one was generated from another so both were updated manually, What is the process to generate .md from .md.tpl?

make docs/spec/api.md

stevvooe · 2017-01-23T23:17:56Z

docs/spec/api.md.tmpl

+issued:
+
+    202 Accepted
+    Content-Length: None


Do we really do None for Content-Length? How about 0?

stevvooe · 2017-01-23T23:18:40Z

docs/spec/api.md.tmpl

+If the tag had already been deleted or did not exist, a `404 Not Found`
+response will be issued instead.    
+
+> **Note** Manifests can never be deleted using this call. If you delete all 


While writing for the API user helps, please target the implementer of the specification.

Do you mean no need for that note?

Well, just explain that the endpoint should not delete the manifest, but the reference from tag to the manifest, so something like that. This doesn't even need to be a note.

stevvooe · 2017-01-23T23:21:36Z

registry/handlers/tags_list.go

@@ -0,0 +1,62 @@
+package handlers


Let's not make a separate file for this.

Are you sure? These are two different Dispatchers, but I am fine either way.

That is okay to have two separate dispatchers in the same file. Its nice to have everything related to tags in the same file.

I'd also have a not about what is going on here (redirect) + some historical context.

stevvooe · 2017-01-23T23:23:05Z

registry/handlers/tags.go

-}
-
-// GetTags returns a json list of tags for a specific image name.
 func (th *tagsHandler) GetTags(w http.ResponseWriter, r *http.Request) {


GetTagCompat.

stevvooe · 2017-01-23T23:24:06Z

@sergeyfd Could you open a road map issue with the steps in the description to tie this all together?

dmcgowan · 2017-01-23T23:42:54Z

I am 👎 to migrating docker to /v2/<name>/tags/_list. This would have to be done on the clients by relying on a 404 fallback to /v2/<name>/tags/list, which has been a pain in the past. I think it would be best to define an Accept header and explicitly call out the behavior that without the Accept header, the list tag gets interpreted as the _list endpoint. Until we have some sort of api version negotiation with the client, we cannot easily just change the behavior. The other benefit is, DELETE never needs an Accept header and if we create a tag type in the future it makes sense to specify that type on GET.

sergeyfd · 2017-01-23T23:49:31Z

So what is the process of generating .md from that template? And what is a road map issue? Is it just a regular issue with the [ROADMAP] in the name or is it something special? Sorry for the novice questions.

aaronlehmann · 2017-01-24T00:06:12Z

docs/spec/api.md


+#### DELETE Tags
+
+Delete the that identified by `name` and `tag`.


Delete the tag?

stevvooe · 2017-01-24T00:54:02Z

@sergeyfd Just make an issue describing the steps with some checkboxes. Next to each step, include the related PRs. As far as generating the api.md goes, just use make docs/spec/api.md.

@dmcgowan Clients should be able to handle a simple redirect. Does that not work? As far as API versioning goes, this should be fully compatible. Old clients will redirect and new clients can try one then the other. This is better than version negotiation; it is active discovery.

sergeyfd · 2017-01-27T15:22:58Z

Does it look better now?

stevvooe · 2017-01-27T21:15:37Z

registry/handlers/tags.go

+func (th *tagsHandler) GetTagsCompat(w http.ResponseWriter, r *http.Request) {
+	ctxu.GetLogger(th).Debug("GetTags")
+
+	if th.Tag == "list" {


Provide a little comment about why.

stevvooe · 2017-01-27T21:15:49Z

registry/handlers/tags.go

 	Tags []string `json:"tags"`
 }

+func (th *tagsHandler) GetTagsCompat(w http.ResponseWriter, r *http.Request) {


Make sure to document the handler.

stevvooe · 2017-01-27T21:17:09Z

@sergeyfd This is looking better!

I think there needs to be a little work in the client. I am not quite sure how we want to handle fallback, quite yet but making the client handle it will help explore possibilities.

sergeyfd · 2017-01-29T15:28:18Z

@stevvooe I added a couple of comments to document new handler. I am not sure what changes are needed on the client side. With this change client will start using BuildTagsListURL, which returns new _list URL. By default http.client in Go follows all redirection so old list URL should work as well if somebody uses old version of the Distribution client. I had to explicitly disable default behavior in the test case. But if you have something in mind please do let me know.

stevvooe · 2017-01-30T20:27:21Z

With this change client will start using BuildTagsListURL, which returns new _list URL. By default http.client in Go follows all redirection so old list URL should work as well if somebody uses old version of the Distribution client. I had to explicitly disable default behavior in the test case. But if you have something in mind please do let me know

This needs to handle both cases. If _list does not exist, it needs to fallback to list.

sergeyfd · 2017-02-01T14:40:15Z

Makes sense. Please take a look at the latest commit.

stevvooe · 2017-02-01T21:28:19Z

registry/client/repository.go

+	var compatUrl bool
+
+	// This is needed to be compatible with old registries that use "list" instead of "_list"
+	_, compatUrl = ctx.Value("tags.url.compat").(bool)


Not sure I'm on board with this use of context. Just make a helper that takes the URL.

stevvooe · 2017-02-01T21:30:34Z

registry/client/repository_test.go

 		})
 		if i < 2 {
-			headers.Set("Link", "<"+s.URL+"/v2/"+repo.Name()+"/tags/list?n=1&last="+tagsList[i]+`>; rel="next"`)
+			headers.Set("Link", "<"+s.URL+"/v2/"+repo.Name()+"/tags/"+ref+"?n=1&last="+tagsList[i]+`>; rel="next"`)


Let's try and setup tests that have the following:

Older client hits tags/list and is redirected.

Newer client hits tags/_list against old registry and falls back.

Newer client hits tags/_list against new registry.

This is split between different test cases. 1) is covered in handlers/api_test.go and 2) and 3) are covered in client/repository_test.go

stevvooe · 2017-02-01T21:31:11Z

docs/spec/api.md.tmpl

@@ -1000,8 +1000,12 @@ to last response or be fully omitted, depending on the server implementation.
 It may be necessary to list all of the tags under a given repository. The tags


I missed this earlier: make sure to create a change entry at the top of the specification.

stevvooe · 2017-02-01T21:36:20Z

@sergeyfd Thanks for sticking with this one! It is a tricky change and we'll keep filling in everything to ensure that it will be a smooth transition.

dmcgowan · 2017-02-01T21:46:27Z

registry/client/repository.go

 		}
 		defer resp.Body.Close()

+		if resp.StatusCode == http.StatusNotFound && !compatURL {


I think this needs to cover a broader range of possible status codes. Possibly any 4xx except 401. We haven't defined what the behavior must be for undefined routes which in some cases could leave proxies to set the rules.

dmcgowan · 2017-02-01T21:48:05Z

registry/client/repository.go

+	var compatURL bool
+
+	// This is needed to be compatible with old registries that use "list" instead of "_list"
+	_, compatURL = ctx.Value(compatTagsURLKey{}).(bool)


Instead of using context, you can just define a func (t *tags) all(ctx context.Context, fallback bool) ([]string, error) method which All calls and will call itself recursively. Passing arguments through context should be avoided whenever possible.

@dmcgowan I suggested he has an unexported helper that takes the url.

The way @dmcgowan suggested seems as less intrusive change to the original code so I changed my code that way. I am not sure how to go with "helper that takes url" idea for this case.

dmcgowan · 2017-02-01T21:48:27Z

Design sounds good, marking as code review

mattmoor · 2017-02-02T14:43:21Z

docs/spec/api.md

 | GET | `/v2/` | Base | Check that the endpoint implements Docker Registry API V2. |
-| GET | `/v2/<name>/tags/list` | Tags | Fetch the tags under the repository identified by `name`. |
+| GET | `/v2/<name>/tags/_list` | Tags | Fetch the tags under the repository identified by `name`. |
+| GET | `/v2/<name>/tags/<reference>` | Tags | Redirect to tags list. |


I think <reference> is a typo, shouldn't it be list?

It is not. The route is used for DELETE operations and GET is just a special case where only 'list' reference supported for backward compatibility.

For DELETE sure, this is GET. Your change description specifically mentions redirecting /tags/list to /tags/_list:

It introduces new TagAPI which allows deleting tags for a given image and also supports legacy Tags List URL /v2//tags/list by redirecting it to the new one /v2//tags/_list.

So I am a bit confused by this line.

Sorry, just reread your comment. If only list is supported as <reference> why bother to call it reference?

That part of template is generated from /registry/api/v2/descriptors.go Over there it's just one route for GET and DELETE with uses <reference> to get correct tag. It works same way for DELETE and GET and if we were developing from scratches we would not implement GET at all. But we have to be backward compatible, hence the only <reference> supported in GET is list and that's just a special case. I see no reason to define a special route and handler for it.

I see, it wasn't clear that was coming from the auto-generation.

Signed-off-by: Serge Dubrouski <sergeyfd@gmail.com>

prune998 · 2018-10-01T17:59:04Z

still not merged. Will it be at some point ?

ayufan · 2018-10-01T18:00:28Z

Waiting for it too :(

…

On Mon, 1 Oct 2018 at 19:59, Prune Sebastien THOMAS < ***@***.***> wrote: still not merged. Will it be at some point ? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2169 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACTpQaKSkbt_VSLB95wKc5a5NhTBzNGaks5uglf4gaJpZM4LqNtM> .

timrizzi · 2019-05-01T12:38:37Z

Any update on this request?

shauneq2 · 2019-07-22T20:21:16Z

Any update on this? It is fairly critical for us because we have a registry cleanup script which purges old images. But if tag "old" points to the same image (manifest) as tag "new" then the new tag is removed as well as the old one. This will leave customers who reference the image by the new tag without an image to pull!!!

esantoro · 2019-12-18T15:27:45Z

Any update on this ?

esantoro · 2019-12-18T15:33:35Z

A little more than a month and then this issue will be three years old! 🎉
And still no api to delete a single tag! 🎉

thaJeztah · 2019-12-18T15:56:45Z

With the distribution spec now living elsewhere, this should likely now be opened as a proposal in https://github.com/opencontainers/distribution-spec

bmerry · 2019-12-19T10:22:10Z

For those looking for a workaround: you can push a new temporary manifest to the tag and then delete it again, which will ensure you don't delete anything else. I've implemented the following in my cleanup script:

Fetch the manifest.
Fetch the image config from it (the script only handles v2-2 manifests).
Append an extra, fake entry to the history section of the image config.
Push the new image config.
Push a new manifest, referencing this new image config, to the tag, and get its digest.
Delete this digest.

This will leave some extra detritus in the registry, but it should get cleaned up by garbage collection. Note that this process never removes any manifests that were originally in the registry, so to actually get space savings you need to use the garbage collector with --delete-untagged.

I've only just implemented this and haven't yet used it on a production registry, so caveat emptor.

dmcgowan · 2020-02-23T00:45:20Z

Additions to the specification are no longer being considered here. The specification has moved to https://github.com/opencontainers/distribution-spec

joaodrp · 2021-05-27T23:14:09Z

For anyone waiting for this, the tag delete API has been added to the OCI Distribution spec in v1.0.0. I've created a PR to implement it here: #3427

sergeyfd mentioned this pull request Jan 21, 2017

Untag manifests for DELETE API calls by tag #1956

Closed

dmcgowan added the status/1-design-review label Jan 23, 2017

stevvooe reviewed Jan 23, 2017

View reviewed changes

aaronlehmann reviewed Jan 24, 2017

View reviewed changes

docs/spec/api.md Outdated

#### DELETE Tags

Delete the that identified by `name` and `tag`.

Copy link
Copy Markdown

aaronlehmann Jan 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete the tag?

sergeyfd mentioned this pull request Jan 24, 2017

Introduce new Tags API #2170

Closed

3 tasks

stevvooe reviewed Jan 27, 2017

View reviewed changes

stevvooe reviewed Feb 1, 2017

View reviewed changes

dmcgowan reviewed Feb 1, 2017

View reviewed changes

dmcgowan added status/2-code-review and removed status/1-design-review labels Feb 1, 2017

mattmoor reviewed Feb 2, 2017

View reviewed changes

sergeyfd added 10 commits July 1, 2018 16:48

New Tags API

5f4b3c1

Signed-off-by: Serge Dubrouski <sergeyfd@gmail.com>

New Tags API

223229c

Signed-off-by: Serge Dubrouski <sergeyfd@gmail.com>

New Tags API

ec4ad54

Signed-off-by: Serge Dubrouski <sergeyfd@gmail.com>

New Tags API

9ecc152

Signed-off-by: Serge Dubrouski <sergeyfd@gmail.com>

New Tags API

d70ebfc

Signed-off-by: Serge Dubrouski <sergeyfd@gmail.com>

New Tags API

ceaefb8

Signed-off-by: Serge Dubrouski <sergeyfd@gmail.com>

New Tags API

96daa77

Signed-off-by: Serge Dubrouski <sergeyfd@gmail.com>

New Tags API

0885cae

Signed-off-by: Serge Dubrouski <sergeyfd@gmail.com>

New Tags API

dfeba7b

Signed-off-by: Serge Dubrouski <sergeyfd@gmail.com>

New Tags API

afce681

Signed-off-by: Serge Dubrouski <sergeyfd@gmail.com>

sergeyfd force-pushed the taglist branch from 1c7a110 to afce681 Compare July 1, 2018 22:52

nlamirault mentioned this pull request Jul 25, 2018

Delete image tag genuinetools/reg#100

Closed

jonjohnsonjr mentioned this pull request Aug 29, 2018

[RFP] replace catalog API functionality opencontainers/distribution-spec#22

Closed

dmcgowan modified the milestones: Registry/2.7, Registry/2.8 Sep 13, 2018

roman-vynar mentioned this pull request Feb 19, 2019

Delete tag instead of manifest? Quiq/registry-ui#29

Closed

davedoesdev mentioned this pull request May 27, 2019

del-alias: 405 Client Error: Method Not Allowed davedoesdev/dxf#25

Closed

dmcgowan closed this Feb 23, 2020

jamesorlakin mentioned this pull request Mar 23, 2020

proposal: Tag deletion/better tag API opencontainers/distribution-spec#114

Open

antoinetran mentioned this pull request Jul 23, 2021

docker registry: image deletion by name and tag distribution/distribution-library-image#119

Closed


		#### DELETE Tags

		Delete the that identified by `name` and `tag`.

		@@ -1000,8 +1000,12 @@ to last response or be fully omitted, depending on the server implementation.
		It may be necessary to list all of the tags under a given repository. The tags

Conversation

sergeyfd commented Jan 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevvooe commented Jan 23, 2017

Uh oh!

dmcgowan commented Jan 23, 2017

Uh oh!

sergeyfd commented Jan 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevvooe commented Jan 24, 2017

Uh oh!

sergeyfd commented Jan 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevvooe commented Jan 27, 2017

Uh oh!

sergeyfd commented Jan 29, 2017

Uh oh!

stevvooe commented Jan 30, 2017

Uh oh!

sergeyfd commented Feb 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevvooe commented Feb 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmcgowan commented Feb 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergeyfd commented Jan 23, 2017 •

edited

Loading