docker / cli

The Docker CLI
Apache License 2.0
4.88k stars 1.92k forks source link

add support for DOCKER_CUSTOM_HEADERS env-var (experimental) #5098

Closed thaJeztah closed 2 months ago

thaJeztah commented 4 months ago

This environment variable allows for setting additional headers to be sent by the client. Headers set through this environment variable are added to headers set through the config-file (through the HttpHeaders field).

This environment variable can be used in situations where headers must be set for a specific invocation of the CLI, but should not be set by default, and therefore cannot be set in the config-file.

:warning: WARNING: If both config and environment-variable are set, the environment variable currently overrides all headers set in the configuration file. This behavior may change in a future update, as we are considering the environment variable to be appending to existing headers (and to only override headers with the same name).

- What I did

- How I did it

- How to verify it

- Description for the changelog

Add support for `DOCKER_CUSTOM_HEADERS` environment variable

This environment variable allows for setting additional headers to be sent by the client. Headers set through this environment variable are added to headers set through the config-file (through the HttpHeaders field).

This environment variable can be used in situations where headers must be set for a specific invocation of the CLI, but should not be set by default, and therefore cannot be set in the config-file.

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

codecov-commenter commented 4 months ago

Codecov Report

Attention: Patch coverage is 58.33333% with 10 lines in your changes missing coverage. Please review.

Project coverage is 61.48%. Comparing base (cd92610) to head (6638deb).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #5098 +/- ## ========================================== + Coverage 61.05% 61.48% +0.43% ========================================== Files 296 299 +3 Lines 20814 20845 +31 ========================================== + Hits 12707 12817 +110 + Misses 7196 7114 -82 - Partials 911 914 +3 ```
laurazard commented 4 months ago

I'm wondering about the utility of adding an env var that completely replaces headers set via over settings (such as through the config file) vs just appending/overriding headers with the same name. I feel like in most cases, adding headers might be more useful, and if in some cases there's a need to overwrite some values, that can be done on a per-header basis, rather than completely ignoring otherwise configured headers.

thaJeztah commented 4 months ago

I'm wondering about the utility of adding an env var that completely replaces headers set via over settings (such as through the config file) vs just appending/overriding headers with the same name. I feel like in most cases, adding headers might be more useful, and if in some cases there's a need to overwrite some values, that can be done on a per-header basis, rather than completely ignoring otherwise configured headers.

Yes, this is always the tricky one; override vs merge.

I guess in most situations, an env-var would override config (flag > env-var > config), but as this is a potential list of options, that tends to make things more of a grey area.

The tricky part with merging is that we would also need a way to unset env-vars; in some parts of our code, I think we have "empty value" meaning "remove", although that one was specifically for User-Agent (allowing removing the user-agent); https://github.com/moby/moby/blob/989426d303a334db4d97f72efe6637d64220d01a/client/request.go#L244-L266

thaJeztah commented 2 months ago

I'm wondering about the utility of adding an env var that completely replaces headers set via over settings (such as through the config file) vs just appending/overriding headers with the same name. I feel like in most cases, adding headers might be more useful, and if in some cases there's a need to overwrite some values, that can be done on a per-header basis, rather than completely ignoring otherwise configured headers.

Yes, this is always the tricky one; override vs merge.

I guess in most situations, an env-var would override config (flag > env-var > config), but as this is a potential list of options, that tends to make things more of a grey area.

The tricky part with merging is that we would also need a way to unset env-vars; in some parts of our code, I think we have "empty value" meaning "remove", although that one was specifically for User-Agent (allowing removing the user-agent); https://github.com/moby/moby/blob/989426d303a334db4d97f72efe6637d64220d01a/client/request.go#L244-L266

I updated the PR and added a WARNING to the GoDoc to mention that the behavior may change (headers to be appended in future, and only overwrite same-name ones)