Drop the old tarsum computation for Docker <0.10 by wking · Pull Request #570 · docker-archive/docker-registry

wking · 2014-09-14T16:12:28Z

Fixes #375.

This landed in 82021b4 (Merging bypass-tarsum, 2014-04-07, #300), but
those Dockers are now ancient history. Additional details in the
commit message. @samalba thought this should land with Docker 1.0
1, which is a while back now. @dmp42 recently pushed this back to
docker-registry 1.0 2.

I'm just floating this now, because I was working on some Storage API
cleanup, and I didn't want to bother with those old code paths. I'm
happy to just let this PR (and any based on it) cook until the powers
that be feel comfortable dropping support for the older clients.

dmp42 · 2014-09-16T17:42:48Z

Pretty much, here we are:

6.  docker/0.9.1->push   1,873(3.41%) 1,000(5.41%)

Add to that about half a percent of even older clients.

Trend looks like that:

I would give it another month for these to flat out to something we can really ignore - that would match with 1.0 timeline hopefully.

Thanks for this Mr. wking :-)

dmp42 · 2014-11-13T20:53:01Z

Ok, time to revisit this.

I'm ok removing these bits, but I want in place a mechanism to refuse push from clients < 0.10 (exact http code to be devised - 406? 417? none are technically exact here, but they are specific enough to distinguish from other errors).

Do you want to take it from here?

wking · 2014-11-15T16:54:43Z

On Thu, Nov 13, 2014 at 12:53:03PM -0800, Olivier Gambier wrote:

I'm ok removing these bits, but I want in place a mechanism to
refuse push from clients < 0.10 (exact http code to be devised -
406? 417? none are technically exact here, but they are specific
enough to distinguish from other errors).

The reason there's no correct response for “you have a User-Agent I
don't like” is because we're not supposed to be switching on the
User-Agent value in the first place ;). Ideally we'd have versioned
the API URLs so we didn't overload /v1/… with incompatible stuff, and
old clients would just get a 404 once we dropped the old URLs. If you
wanted a nice client-side error message, we could always set an:

X-Docker-Versions: 2.4, 3.1, ...

header listing the semantically versioned APIs that the registry did
support, and the client could check that its version was supported
whenever it hit an endpoint (even if that endpoint claimed success,
see 1).

Is there an alternate way to get the existing clients to report a
clear error message (“I'm too old for that registry”)? I'm not
familiar with the client code, so maybe we do have an existing
mechanism for that.

wking · 2014-11-15T20:07:39Z

On Thu, Nov 13, 2014 at 12:53:03PM -0800, Olivier Gambier wrote:

exact http code to be devised - 406? 417? none are technically exact
here, but they are specific enough to distinguish from other errors

Joe Liversedge suggests “412 Precondition Failed” 1.

wking · 2014-11-15T20:15:34Z

On Sat, Nov 15, 2014 at 08:54:36AM -0800, W. Trevor King wrote:

X-Docker-Versions: 2.4, 3.1, ...

And it looks like X-* headers are no longer recommended 1, so
perhaps Docker-Versions: … 1. But a header returned from the
registry doesn't allow clients to pick which API version they'd like
to use. In that case, using a request version (in the URI, accepted
MIME type, or a separate header), and having the server respond with
that version makes sense. I'm not sure about endpoint deprecation
though. Does the server have to remember all the previous endpoint
patterns so it can 412 them (or whatever)? I guess that's easy enough
if you don't need to distinguish between “that's a version I don't
support anymore” and “that was never valid.”

dmp42 · 2014-11-15T22:40:01Z

Unfortunately, client side error reporting is not good (for past and present docker versions), and it's (very) unlikely that old docker versions are going to get an update - while I do agree that none of my suggestions are technically "the right thing to do", we need a reasonably "visible" way to give feedback to older clients pointing to that specific issue.

Now, for the future: #734

wking · 2014-11-15T23:03:15Z

On Sat, Nov 15, 2014 at 02:40:03PM -0800, Olivier Gambier wrote:

we need a reasonably "visible" way to give feedback to older clients
pointing to that specific issue.

So what are our options? I'm not sure what the client sees, or if the
connections are even from the command-line client (I think they're
from the Docker daemon/engine). I'm fine jumping through a per-client
hoop here until we fix #734, but I'm not familiar enough with the
client/engine system to know what hoop I should jump through ;).

dmp42 · 2014-11-15T23:24:37Z

So what are our options?

I believe for the targeted versions (<0.10) using an HTTP status code that is not used already for other error conditions is the only way (+ informative error message in the body can't hurt).

Connections are made by the daemon - these will show in daemon logs.

This landed in 82021b4 (Merging bypass-tarsum, 2014-04-07, docker-archive#300), but those Dockers are now ancient history. I'm against this sort of user-agent-dependent behavior in general. If we want to version the API, it makes sense to have version checks in one place (e.g. in the URL, or in an explicit Docker-Version header, or in the PUT payload, ...). I'm happy with either the URL approach (always explicit) or the Docker-Version approach (defaulting to the current version). If we make some small change to the API (such as not storing layer tarsums), and want to add per-feature headers or registry-side options to enable/disable this behaviour, that's fine. If we can auto-detect new features (e.g. if Docker-Checksum-Payload exists, it must be using the new SHA-256 semantics), that's fine too. But regardless of how we version the API, conflating the API version with the user-agent version just seems confusing. Olivier wants us to warn older clients instead of just failing when their tarsum doesn't match our checksum [1], so we still need to use their user-agent string to decide which clients get warned. I've softened the old DockerVersion into a more relaxed docker_client_version, which no longer rejects non-Docker clients [2]. If we can't version the API, we can at least use a blacklist to deny old implementations instead of a whitelist that requires other clients to fake the Docker-daemon User-Agent string. I'd like to remove all the user-agent-dependent behavior, but we're currently also using it to setup some tag metadata in docker_registry.tags.put_tag. I don't touch that here, since I haven't tracked down the reasoning behind that behavior (although I suspect it's also a bad idea ;). I also haven't touched the user-agent bits under tests/, since I haven't checked to see if they're designed to test this check, or to test the put_tags check, or just to bypass either of those checks. [1]: docker-archive#570 (comment) [2]: docker-archive#375

This landed in 82021b4 (Merging bypass-tarsum, 2014-04-07, docker-archive#300), but those Dockers are now ancient history. I'm against this sort of user-agent-dependent behavior in general. If we want to version the API, it makes sense to have version checks in one place (e.g. in the URL, or in an explicit Docker-Version header, or in the PUT payload, ...). I'm happy with either the URL approach (always explicit) or the Docker-Version approach (defaulting to the current version). If we make some small change to the API (such as not storing layer tarsums), and want to add per-feature headers or registry-side options to enable/disable this behaviour, that's fine. If we can auto-detect new features (e.g. if Docker-Checksum-Payload exists, it must be using the new SHA-256 semantics), that's fine too. But regardless of how we version the API, conflating the API version with the user-agent version just seems confusing. Olivier wants us to warn older clients instead of just failing when their tarsum doesn't match our checksum [1], so we still need to use their user-agent string to decide which clients get warned. I've softened the old DockerVersion into a more relaxed docker_client_version, which no longer rejects non-Docker clients [2]. If we can't version the API, we can at least use a blacklist to deny old implementations instead of a whitelist that requires other clients to fake the Docker-daemon User-Agent string. For the 'PUT /v1/images/<image_id>/checksum' endpoint, we don't need to rely on the user-agent, because the old X-Docker-Checksum was replaced by the new X-Docker-Checksum-Payload. If the client isn't setting X-Docker-Checksum-Payload, just say that in the error message and hint that it might be due to an outdated client. I'd like to remove all the user-agent-dependent behavior, but we're currently also using it to setup some tag metadata in docker_registry.tags.put_tag. I don't touch that here, since I haven't tracked down the reasoning behind that behavior (although I suspect it's also a bad idea ;). I also haven't touched the user-agent bits under tests/, since I haven't checked to see if they're designed to test this check, or to test the put_tags check, or just to bypass either of those checks. [1]: docker-archive#570 (comment) [2]: docker-archive#375

wking · 2014-11-16T01:28:34Z

On Sat, Nov 15, 2014 at 03:24:39PM -0800, Olivier Gambier wrote:

I believe for the targeted versions (<0.10) using an HTTP status
code that is not used already for other error conditions is the only
way (+ informative error message in the body can't hurt).

Should be done in ad9a8cc (which I've rebased onto the current master
too). I used docker_registry.toolkit.api_error for the error, so the
body will be:

{'error': ''}

dmp42 · 2014-12-02T22:28:55Z

@wking something is not green it seems :)

This landed in 82021b4 (Merging bypass-tarsum, 2014-04-07, docker-archive#300), but those Dockers are now ancient history. I'm against this sort of user-agent-dependent behavior in general. If we want to version the API, it makes sense to have version checks in one place (e.g. in the URL, or in an explicit Docker-Version header, or in the PUT payload, ...). I'm happy with either the URL approach (always explicit) or the Docker-Version approach (defaulting to the current version). If we make some small change to the API (such as not storing layer tarsums), and want to add per-feature headers or registry-side options to enable/disable this behaviour, that's fine. If we can auto-detect new features (e.g. if Docker-Checksum-Payload exists, it must be using the new SHA-256 semantics), that's fine too. But regardless of how we version the API, conflating the API version with the user-agent version just seems confusing. Olivier wants us to warn older clients instead of just failing when their tarsum doesn't match our checksum [1], so we still need to use their user-agent string to decide which clients get warned. I've softened the old DockerVersion into a more relaxed docker_client_version, which no longer rejects non-Docker clients [2]. If we can't version the API, we can at least use a blacklist to deny old implementations instead of a whitelist that requires other clients to fake the Docker-daemon User-Agent string. For the 'PUT /v1/images/<image_id>/checksum' endpoint, we don't need to rely on the user-agent, because the old X-Docker-Checksum was replaced by the new X-Docker-Checksum-Payload. If the client isn't setting X-Docker-Checksum-Payload, just say that in the error message and hint that it might be due to an outdated client. I'd like to remove all the user-agent-dependent behavior, but we're currently also using it to setup some tag metadata in docker_registry.tags.put_tag. I don't touch that here, since I haven't tracked down the reasoning behind that behavior (although I suspect it's also a bad idea ;). I also haven't touched the user-agent bits under tests/, since I haven't checked to see if they're designed to test this check, or to test the put_tags check, or just to bypass either of those checks. [1]: docker-archive#570 (comment) [2]: docker-archive#375

wking · 2014-12-04T22:47:28Z

On Tue, Dec 02, 2014 at 02:28:58PM -0800, Olivier Gambier wrote:

@wking something is not green it seems :)

Sloppy me. Should be fixed now.

dmp42 · 2014-12-04T23:22:24Z

LGTM

Thanks a lot for this!

Drop the old tarsum computation for Docker <0.10

dmp42 added this to the 1.0 milestone Sep 16, 2014

wking mentioned this pull request Sep 16, 2014

Refcount images and cleanup orphans #409

Closed

wking mentioned this pull request Nov 15, 2014

Proposal: JSON Registry API V2.1 moby/moby#9015

Closed

9 tasks

dmp42 mentioned this pull request Nov 15, 2014

NG: deprecation path and client messaging #734

Closed

wking force-pushed the drop-checksum-version-checks branch from 09a63ef to 6a73d6e Compare November 16, 2014 00:40

wking force-pushed the drop-checksum-version-checks branch from 6a73d6e to ffa493d Compare November 16, 2014 00:57

wking force-pushed the drop-checksum-version-checks branch from ffa493d to ad9a8cc Compare November 16, 2014 01:26

wking force-pushed the drop-checksum-version-checks branch from ad9a8cc to 4f929ce Compare December 4, 2014 22:03

wking force-pushed the drop-checksum-version-checks branch from 4f929ce to f245656 Compare December 4, 2014 22:12

dmp42 added a commit that referenced this pull request Dec 4, 2014

Merge pull request #570 from wking/drop-checksum-version-checks

f04bce0

Drop the old tarsum computation for Docker <0.10

dmp42 merged commit f04bce0 into docker-archive:master Dec 4, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop the old tarsum computation for Docker <0.10#570

Drop the old tarsum computation for Docker <0.10#570
dmp42 merged 1 commit intodocker-archive:masterfrom
wking:drop-checksum-version-checks

wking commented Sep 14, 2014

Uh oh!

dmp42 commented Sep 16, 2014

Uh oh!

dmp42 commented Nov 13, 2014

Uh oh!

wking commented Nov 15, 2014

Uh oh!

wking commented Nov 15, 2014

Uh oh!

wking commented Nov 15, 2014

Uh oh!

dmp42 commented Nov 15, 2014

Uh oh!

wking commented Nov 15, 2014

Uh oh!

dmp42 commented Nov 15, 2014

Uh oh!

wking commented Nov 16, 2014

Uh oh!

dmp42 commented Dec 2, 2014

Uh oh!

wking commented Dec 4, 2014

Uh oh!

dmp42 commented Dec 4, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

wking commented Sep 14, 2014

Uh oh!

dmp42 commented Sep 16, 2014

Uh oh!

dmp42 commented Nov 13, 2014

Uh oh!

wking commented Nov 15, 2014

Uh oh!

wking commented Nov 15, 2014

Uh oh!

wking commented Nov 15, 2014

Uh oh!

dmp42 commented Nov 15, 2014

Uh oh!

wking commented Nov 15, 2014

Uh oh!

dmp42 commented Nov 15, 2014

Uh oh!

wking commented Nov 16, 2014

Uh oh!

dmp42 commented Dec 2, 2014

Uh oh!

wking commented Dec 4, 2014

Uh oh!

dmp42 commented Dec 4, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants