This repository was archived by the owner on Sep 12, 2018. It is now read-only.
Bypass the tarsum computation with docker version > 0.9.1#300
Merged
Bypass the tarsum computation with docker version > 0.9.1#300
Conversation
Docker-DCO-1.1-Signed-off-by: Sam Alba <sam.alba@gmail.com> (github: samalba)
registry/toolkit.py
Outdated
Contributor
There was a problem hiding this comment.
I think if there's no match you should raise an exception, otherwise this could pose problems in the future.
Contributor
Author
There was a problem hiding this comment.
If we do that, we enforce the client to have a UserAgent all the time...
Might be a good thing actually since we already get data from the user-agent for meta data associated to the layer.
Contributor
There was a problem hiding this comment.
We're free to handle the exception downstream in any way we want, but I think it's a mistake to silently ignore it.
Contributor
Author
There was a problem hiding this comment.
You convinced me.
Also spot another glitch in the code, I'll push an update
Docker-DCO-1.1-Signed-off-by: Sam Alba <sam.alba@gmail.com> (github: samalba)
…as of today) Docker-DCO-1.1-Signed-off-by: Sam Alba <sam.alba@gmail.com> (github: samalba)
Contributor
Author
|
Updated with previous comments. Since moby/moby#4945, the target will be 0.10. |
Docker-DCO-1.1-Signed-off-by: Sam Alba <sam.alba@gmail.com> (github: samalba)
Docker-DCO-1.1-Signed-off-by: Sam Alba <sam.alba@gmail.com> (github: samalba)
Contributor
Author
|
This can be merged now. @shin- and anyone else, please review |
Contributor
|
LGTM |
Contributor
Author
|
merging manually |
This was referenced May 20, 2014
wking
added a commit
to wking/docker-registry
that referenced
this pull request
Sep 14, 2014
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 X-Docker-Version header, or in the PUT payload, ...). I'm happy with either the URL approach (always explicit) or the X-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 X-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. 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.
9 tasks
wking
added a commit
to wking/docker-registry
that referenced
this pull request
Nov 16, 2014
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
wking
added a commit
to wking/docker-registry
that referenced
this pull request
Nov 16, 2014
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
wking
added a commit
to wking/docker-registry
that referenced
this pull request
Nov 16, 2014
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
added a commit
to wking/docker-registry
that referenced
this pull request
Dec 4, 2014
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
added a commit
to wking/docker-registry
that referenced
this pull request
Dec 4, 2014
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This depends on moby/moby#4945
Need to wait for next version of docker to be released (don't know the minimum version required yet for this patch to work).