client: add WithUserAgent() option#45677
Conversation
thaJeztah
added
area/api
|
/cc @ndeloof |
|
nice. Reading moby codebase I was convinced preventing user agent override was intentional |
| } | ||
|
|
||
| if cli.userAgent != "" { | ||
| req.Header.Set("User-Agent", cli.userAgent) |
There was a problem hiding this comment.
I think cli.userAgent should be *string and *cli.userAgent == "" should just unset the User-Agent header.
There was a problem hiding this comment.
I was thinking about that, but couldn't come-up with a case where we would first "set" and then "clear" the User-Agent; did you have a specific situation in mind?
Other things I was contemplating;
- Should
WithUserAgent("")produce an error? - What user-agent should be set if
*cli.userAgent == ""? Should we set some custom default? (Would we potentially end-up with Golang's default user-agent?) https://github.com/golang/go/blob/c71cbd544e3da139badd4c03612af41b63711705/src/net/http/request.go#L519-L523
// NOTE: This is not intended to reflect the actual Go version being used.
// It was changed at the time of Go 1.1 release because the former User-Agent
// had ended up blocked by some intrusion detection systems.
// See https://codereview.appspot.com/7532043.
const defaultUserAgent = "Go-http-client/1.1"
@ndeloof I "it depends". The code you linked to looks to be to prevent a user-configured custom header from overriding the user-agent; But that code may also have been working around limitations of the code, or a working around a bug; For the Docker CLI we probably still want to prevent the CLI config-file from overriding the user-agent, but the "library code" from both this (moby/moby) and docker/cli repositories should give the consumer of the library code full control over this. After all, you don't want to develop "my-awesome_cli" to be advertising itself as For Docker Compose; we should discuss what user-agent we want to set (which could be a combination of both when run as a plugin). Either way, this PR is just a building-block in that; this PR should make it easier (and non-ambiguous) to control the User-Agent without having to mess with setting/unsetting/overriding headers in other places. But more work is still needed in the docker/cli code (the "docker context" code is overly complicated, so may still need to do some refactoring). |
|
@rumpl @AkihiroSuda ptal 🤗 |
|
Oh! Nevermind; I still had to update this one with @AkihiroSuda's suggestion.. ignore me |
thaJeztah
force-pushed
the
client_useragent
branch
from
d8fbc2c to
3b1bc12
Compare
When constructing the client, and setting the User-Agent, care must be taken to apply the header in the right location, as custom headers can be set in the CLI configuration, and merging these custom headers should not override the User-Agent header. This patch adds a dedicated `WithUserAgent()` option, which stores the user-agent separate from other headers, centralizing the merging of other headers, so that other parts of the (CLI) code don't have to be concerned with merging them in the right order. Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
client_useragent
branch
from
3b1bc12 to
a6048fc
Compare
|
Ah, LOL, mystery solved; looks like I was working on this, but had to clean up things Either way, this should be ready for review now 👍 PTAL |
4 participants
When constructing the client, and setting the User-Agent, care must be taken to apply the header in the right location, as custom headers can be set in the CLI configuration, and merging these custom headers should not override the User-Agent header.
This patch adds a dedicated
WithUserAgent()option, which stores the user-agent separate from other headers, centralizing the merging of other headers, so that other parts of the (CLI) code don't have to be concerned with merging them in the right order.- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)