Pass upstream client's user agent through to registry on image pulls · Pull Request #21306 · moby/moby

ghost · 2016-03-17T20:23:29Z

Summary: pass through to registry an upstream user-agent on image pull. Closes issue #20001.

What I did: People running Registry servers should be able to see what client initiated an image pull per issue #20001. For example, if Compose instructs Engine to pull from a local Registry, then the User-Agent header sent to that Registry server should be the following RFC 7231 compliant [1] string concatenation:

User-Agent:  [Docker Compose UA] upstream-ua/end; [Docker Engine UA]

upstream-ua/end; is just a delimiter. (Open to suggestions for a better one.)

How I did it: When Engine receives an image pull API request, I set a golang.org/x/net/context context inside the closure returned by NewUserAgentMiddleware() in api/server/middleware/user_agent.go. This context includes the upstream client's UA string (e.g., Compose's UA string). From there, I just plumbed the context variable all the way through the request processing chain until DockerUserAgent() is called. DockerUserAgent() was modified to accept an optional string argument, which is read back from the context by the caller.

How to verify it:

Run a Registry server on port 5000 of localhost:

docker run -d -p 5000:5000 --restart=always --name registry registry:2

See [2] for more information.

In a separate terminal, tail the registry server's logs:

docker logs -f [REGISTRY CONTAINER ID]

Put an image in your empty registry like this:

docker pull hello-world
docker tag hello-world localhost:5000/hello-world
docker push localhost:5000/hello-world

Do an image pull from your registry via Engine but using a custom User-Agent header:

curl -X POST -A "Goelzer/99.9 (Goelzer; en-us)" "http://127.0.0.1:2375/images/create?fromImage=localhost:5000/hello-world" -H "X-Registry-Auth: e30="

Observe this User-Agent string in the registry log:

172.17.42.1 - - [14/Mar/2016:23:09:09 +0000] "GET /v2/hello-world/manifests/latest HTTP/1.1" 200 708 "" "Goelzer/99.9 (Goelzer; en-us) upstream-ua/end; docker/1.11.0-dev go/go1.6 git-commit/xxxxxx-unsupported kernel/4.2.0-30-generic os/linux arch/amd64"

Refs:
[1] RFC 7231: http://tools.ietf.org/html/rfc7231#section-5.5.3
[2] https://github.com/docker/distribution/blob/master/docs/deploying.md

calavera · 2016-03-18T15:57:01Z

Design LGTM

calavera · 2016-03-18T16:03:22Z

daemon/daemon.go

I think we should use context.Background() if we're not going to handle the user agent in this case.

icecrime · 2016-03-18T17:34:35Z

Thanks! I haven't tested, but from a rapid overview code LGTM (I do think the dependency graph in this PR puts us in a better position to split out the server-side API code out of the repo). 👍

aaronlehmann · 2016-03-18T18:16:26Z

integration-cli/docker_cli_registry_user_agent_test.go

Could this check for the expected client UA? Perhaps with a permissing regexp so the test isn't tied to a particular client version?

aaronlehmann · 2016-03-18T18:21:22Z

I probably should have throught of this earlier, but wouldn't it make sense to pass through the client user agent on pushes, searches, and login operations as well? It feels like that would be a lot more consistent.

ghost · 2016-03-18T19:15:53Z

@aaronlehmann Yes, I want to do this in a future PR. This PR is intended as an incremental step to get to that point.

aaronlehmann · 2016-03-18T19:39:56Z

I understand the desire to make incremental progress, but wouldn't it be awkward for registry operators gathering these statistics if some Engine versions include the client user agent during all operations, and some only do it for pulls?

I'd be happy to help make these changes for the other operations if time is an issue.

aaronlehmann · 2016-03-18T21:46:47Z

I have a local branch that extends this to support push, login, and search: aaronlehmann@41f3d16. This also makes some improvements to the integration test.

I didn't go through the effort to support it in build. That's somewhat more complex because it requires plumbing a context all the way through the builder. One could argue, though, that the pull operation in this case isn't being triggered directly by a client, so it's reasonable to only include the daemon user agent.

Let's finalize this PR first (@mgoelzer, I know you're looking to tweak the User-Agent format), and then I can rebase my changes on top of what gets merged and open a separate PR.

ghost · 2016-03-18T23:40:35Z

I have updated the PR to use the following format (suggested to me by @stevvooe):

User-Agent:  [Docker Engine UA] UPSTREAM-CLIENT([Upstream Client UA])

where [Upstream Client UA] has backslash escaped each of these characters: ();

To parse it, one would presumably match on ".+ UPSTREAM-CLIENT([^)])", extract the right-hand side substring (stuff between the parens), unescape it and then process as needed.

Any feedback on the new format? @aaronlehmann @calavera

anusha-ragunathan · 2016-03-19T01:00:51Z

docker run, similar to docker build can also trigger pulling images. If we dont plan to distinguish a user-initiated interaction with registry vs an indirect interaction with registry, these cases should be handled.

ghost · 2016-03-19T02:13:11Z

@anusha-ragunathan Good point, I had not thought of that but I agree we should handle it. I think if we merge this PR and then merge @aaronlehmann 's (as of yet uncreated) PR for push, login and search, then I can fix PullOnBuild() and the "pull on run" case in a subsequent PR. What do you think? I am hopeful about getting at least regular pull, push, login and search in for Docker 1.11 even if it's not perfect yet.

ghost · 2016-03-19T02:15:26Z

I just updated the PR with a better test given that the format of the UA "concatenation" has changed to a backslash-escaped, embedded string.

I know the test is not perfect and only looks at the beginning of each string, but I don't want to make the integration tests too brittle. Open to suggestions if anyone has them.

@aaronlehmann
@calavera
@anusha-ragunathan

aaronlehmann · 2016-03-19T03:24:45Z

dockerversion/useragent.go

Typo: should be insertUpstreamUserAgent.

Thank you, fixed.

aaronlehmann · 2016-03-19T03:33:09Z

I know the test is not perfect and only looks at the beginning of each string, but I don't want to make the integration tests too brittle. Open to suggestions if anyone has them.

The main problem that I see with the test is that it will succeed even if the mock handler is never hit for some reason. I've corrected this in my followup PR, so don't worry about it in this one.

ghost · 2016-03-19T08:11:54Z

Hi @aaronlehmann, I see how your branch improves the test significantly, so I'm going to defer to your PR on that.

ghost · 2016-03-20T11:27:51Z

win2lin failing because no space left on device

08:20:01 Step 1 : FROM docker:c51be1c
08:20:01  ---> 196dc052f06d
08:20:01 Step 2 : RUN hack/make.sh binary
08:20:01  ---> Running in 9ba5097ee5e9
08:20:02 Removing intermediate container 9ba5097ee5e9
08:20:02 Cannot start container 9ba5097ee5e98a0cadf1f66fff59ef01d5b027b3d33ad5421bae2908230dd526: [9] System error: mkdir /var/run/docker/execdriver/native/9ba5097ee5e98a0cadf1f66fff59ef01d5b027b3d33ad5421bae2908230dd526: no space left on device

windowsTP4 failing because HCSShim::StartComputeSystem failed in Win32

08:20:57 Step 5 : COPY . /go/src/github.com/docker/docker
08:21:12  ---> e047932207ea
08:21:12 Removing intermediate container 3773633fe385
08:21:12 Successfully built e047932207ea
08:21:12 + lastec=0
08:21:12 + set +x
08:21:12 INFO: Building the test binary...
08:21:12 + docker run --rm -v 'd:\CI\CI-9d11227:c:\target' docker sh -c 'cd /c/go/src/github.com/docker/docker; \
08:21:12    hack/make.sh binary; \
08:21:12    ec=$?; \
08:21:12    if [ $ec -eq 0 ]; then \
08:21:12        robocopy /c/go/src/github.com/docker/docker/bundles/$(cat VERSION)/binary /c/target/binary; \
08:21:12    fi; \
08:21:12    exit $ec'
08:21:13 C:\Windows\system32\docker.exe: Error response from daemon: Cannot start container 85a5523803a71e6e33c3c79f362b060ec6632d0e70d5a3578c332334c6635d56: HCSShim::StartComputeSystem failed in Win32: The object identifier does not represent a valid object. (0x10d8) id=85a5523803a71e6e33c3c79f362b060ec6632d0e70d5a3578c332334c6635d56.

All other tests passing.

Changes how the Engine interacts with Registry servers on image pull. Previously, Engine sent a User-Agent string to the Registry server that included only the Engine's version information. This commit appends to that string the fields from the User-Agent sent by the client (e.g., Compose) of the Engine. This allows Registry server operators to understand what tools are actually generating pulls on their registries. Signed-off-by: Mike Goelzer <mgoelzer@docker.com>

icecrime · 2016-03-21T00:09:58Z

@mgoelzer Sorry for this, the win2lin issue is known and being worked on, the windowsTP4 unfortunately has no solution but wait for a better TP release. I'll rebuild, we'll get it green.

ghost · 2016-03-21T14:52:13Z

@icecrime, no problem. I'm just posting the output of failing tests so we have a log of what's failing. But since it seems to be sporadic, there's probably no point.

anusha-ragunathan · 2016-03-21T15:27:31Z

@mgoelzer : I'm okay with you fixing pull on build/run in a subsequent PR. It'd be nice to get that in 1.11 as well, but I understand if there are timing constraints.

ghost · 2016-03-21T18:25:23Z

@icecrime If we can get this merged today, @aaronlehmann has a second PR he will open that adds the User-Agent stuff to login, push and search. Can we just disregard the failing TP4 test?

aaronlehmann · 2016-03-21T20:03:44Z

LGTM

stevvooe · 2016-03-21T20:48:13Z

dockerversion/useragent.go

+		appended := false
+		for _, escapeableRune := range charsToEscape {
+			if currRune == escapeableRune {
+				ret += "\\" + string(currRune)


Why is this double escaped? If it encounters ;, it should emit \;. If it encounters \, it should emit \\.

Not sure what you mean by double escaped. The string literal "\" is a one-character string. So, encountering ; will cause it to append to ret \;, like you're saying.

EDIT: ironically, I can't say what I want because github also escapes backslashes :-)

Rather, use a bactic literal here for clarity.

To save CI churn, I'd be happy to make this change in my followup PR.

anusha-ragunathan · 2016-03-21T20:49:06Z

LGTM

stevvooe · 2016-03-21T21:15:31Z

LGTM

Pass upstream client's user agent through to registry on image pulls

aaronlehmann · 2016-03-21T21:34:57Z

Opened the companion PR to extend this to build, login, push, and search here: #21373

rogaha · 2016-05-13T10:52:42Z

👍

GordonTheTurtle added the status/0-triage label Mar 17, 2016

thaJeztah added status/1-design-review and removed status/0-triage labels Mar 18, 2016

calavera mentioned this pull request Mar 18, 2016

Pass upstream client's user agent through to registry on image pulls #21197

Closed

calavera changed the title ~~Alternative to PR #21197~~ Pass upstream client's user agent through to registry on image pulls Mar 18, 2016

calavera added status/2-code-review and removed status/1-design-review labels Mar 18, 2016

calavera reviewed Mar 18, 2016
View reviewed changes

icecrime added this to the 1.11.0 milestone Mar 18, 2016

aaronlehmann reviewed Mar 18, 2016
View reviewed changes

aaronlehmann reviewed Mar 19, 2016
View reviewed changes

icecrime assigned aaronlehmann Mar 21, 2016

stevvooe reviewed Mar 21, 2016
View reviewed changes

icecrime added status/4-merge and removed status/2-code-review labels Mar 21, 2016

icecrime pushed a commit that referenced this pull request Mar 21, 2016

Merge pull request #21306 from mgoelzer/issue20001-nodeps

278d396

Pass upstream client's user agent through to registry on image pulls

icecrime merged commit 278d396 into moby:master Mar 21, 2016

aaronlehmann mentioned this pull request Mar 21, 2016

Pass upstream client's user agent through to registry on operations beyond pulls #21373

Merged

ghost mentioned this pull request Mar 23, 2016

Add client-side element to the User-Agent string #20001

Closed

bfirsh mentioned this pull request Jun 3, 2016

Set user agent for Remote API calls docker/compose#3550

Closed

bfirsh mentioned this pull request Jul 24, 2016

Add user agent to API calls docker/compose#3760

Merged

thaJeztah mentioned this pull request Jan 17, 2023

containerd-integration: set correct user-agent #44828

Closed

Conversation

ghost commented Mar 17, 2016

Uh oh!

calavera commented Mar 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

icecrime commented Mar 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Mar 18, 2016

Uh oh!

ghost commented Mar 18, 2016

Uh oh!

aaronlehmann commented Mar 18, 2016

Uh oh!

aaronlehmann commented Mar 18, 2016

Uh oh!

ghost commented Mar 18, 2016

Uh oh!

anusha-ragunathan commented Mar 19, 2016

Uh oh!

ghost commented Mar 19, 2016

Uh oh!

ghost commented Mar 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Mar 19, 2016

Uh oh!

ghost commented Mar 19, 2016

Uh oh!

ghost commented Mar 20, 2016

Uh oh!

icecrime commented Mar 21, 2016

Uh oh!

ghost commented Mar 21, 2016

Uh oh!

anusha-ragunathan commented Mar 21, 2016

Uh oh!

ghost commented Mar 21, 2016

Uh oh!

aaronlehmann commented Mar 21, 2016

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!

anusha-ragunathan commented Mar 21, 2016

Uh oh!

stevvooe commented Mar 21, 2016

Uh oh!

aaronlehmann commented Mar 21, 2016

Uh oh!

rogaha commented May 13, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants