feat(cli): add --http-header option

[mapleeit]

Signed-off-by: Zhaofeng Miao 522856232@qq.com

- What I did

Add --http-header option to CLI to allow adding custom http headers when doing docker command

- How I did it

See the diff

- How to verify it

I tested it locally, if you think it's good to continue, I'll add test for it and polish the code.

- Description for the changelog

Add --http-header option to CLI to allow specifying http headers. docker --http-header "key: value" version

- A picture of a cute animal (not mandatory but encouraged)

: )

[mapleeit]

@thaJeztah It's good to have your opinion here. Thanks in advance.

[thaJeztah]

Could you describe the use-case for this feature? The CLI already supports a HttpHeaders option in the cli configuration file (https://docs.docker.com/engine/reference/commandline/cli/#custom-http-headers), so I'm wondering if that would be an alternative to use.

[mapleeit]

Thanks for the quick response. @thaJeztah

We are treating docker daemon as a microservice in our system. So we need to append x-request-id http header to the each request, so that we can trace and route them as we need. And it will change every time apparently. Writing it to the config.json doesn't work in concurrent cases.

[thaJeztah]

And it will change every time apparently. Writing it to the config.json doesn't work in concurrent cases.

Ah, right, gotcha, yes, understand the use-case.

[mapleeit]

Do you think I should continue to add unit test and polish the code now? or still need consideration or discussion.

[thaJeztah]

Do you think I should continue to add unit test and polish the code now? or still need consideration or discussion.

I was writing down some first comments (let me post them below); overall, I'm not against adding this as a top-level flag (but of course things can still be discussed with other maintainers/reviewers as well).

[thaJeztah]

I have yet to have a closer look, but some quick thoughts;

• we should probably use = as separator (--http-header My-Header=myvalue)
• use strings.SplitN(.., ..., 2) (as we expect only a single separator); this prevents issues if the value would contain :, which could be if (just thinking out loud) for example, an IPv6 address is passed (Foo-Header=[::]:123)
• We probably need to do this in some other places as well (existing locations that is; outside of scope for this PR), but perhaps we should also use http. CanonicalHeaderKey() (after stripping whitespace), to prevent duplicate headers, and to prevent User-Agent from being overridden (which we don't want); it looks like the client already does normalising (I just notice it doesn't trim whitespace, and probably should though), but it doesn't hurt to prevent possible duplicates here

Overall, I still want to look if this function (newAPIClientFromEndpoint()) is the best place to set this; I'm considering if perhaps we should merge the config-file and flag options earlier, perhaps as part of DockerCLI.Initialize() ? (I'll have to have a look if that would work)

[mapleeit]

we should probably use = as separator (--http-header My-Header=myvalue)

I'm trying to keep consistent with curl since it has been widely accepted. And I'm not sure if there are traps if we use =

[mapleeit]

I'm considering if perhaps we should merge the config-file and flag options earlier, perhaps as part of DockerCLI.Initialize()

Agree. This will make the whole configing procedure more clear

[mapleeit]

use strings.SplitN(.., ..., 2)

Addressed

[thaJeztah]

w.r.t. the above (parsing) in mind I'm thinking if perhaps the value parsing should be part of a custom type (so that this would be an (e.g.) opts.HttpHeadersOpt with its own validation function, similar fo opts.ListOpts and opts. NamedListOpts

Thinking of which (sorry, bit hazy on that one), but I think the NamedListOpts and NamedOpts types were used for command-line flags that also have an equivalent in ~/.docker/config.json. Which (in this case) could be considered the case (the --http-headers being an equivalent for the HttpHeaders option in the config file)

(Hope this helps, but happy to do some further digging in this)

[mapleeit]

Basically, you want to use opts.NamedMapOpts and write according validators for it, right?

[mapleeit]

What should I do to push this PR forward? @thaJeztah