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

thaJeztah · 2024-05-31T12:26:13Z

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: 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 · 2024-06-04T10:05:43Z

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

@@            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 · 2024-06-04T12:23:35Z

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.

krissetto · 2024-06-04T12:21:54Z

cli/command/cli_options.go

+		csvReader := csv.NewReader(strings.NewReader(value))
+		fields, err := csvReader.Read()
+		if err != nil {
+			return errdefs.InvalidParameter(errors.Wrapf(err, "failed to set custom headers from %s environment variable", envOverrideHTTPHeaders))


From the docs i see these three main error conditions on csvReader.Read():

If the record has an unexpected number of fields, Read returns the record along with the error ErrFieldCount.

Given we'll only have a single "row" of data, ErrFieldCount shouldn't really occur;

If the record contains a field that cannot be parsed, Read returns a partial record along with the parse error. The partial record contains all fields read before the error.

I don't think we want to manage partial records, so we should return when ending up on that error;

If there is no data left to be read, Read returns nil, io.EOF.

We shouldn't ever encounter an empty reader

Given the above, in this error msg shall we mention that the CSV passed in the DOCKER_CUSTOM_HEADERS is likely malformed? IIUC that'd be the most likely reason for encountering the error, and might be a bit more helpful to the user than this fairly generic message. WDYT?

Hm, yes, good point; I was thinking that the CSV error might be useful, but perhaps it's not, and we should just produce a generic "this is likely the wrong format, we expect a comma-separated list of key=value.

fair point, always try to make user facing errors "explicit" just in case (so they're in our control rather then depending on the library maintainers). No hard feelings if the error it returns is good enough👍

We should look indeed what the errors look like, and either "replace" all errors it produces, or if it's only a specific type; for that type.

I haven't found the time yet to try what they look like (perhaps we need to add a test-case for it)

I updated the error to not include the CSV error, and made it more generic

cli/command/cli_options.go

krissetto · 2024-06-04T12:28:54Z

cli/command/cli_options.go

+
+			if v == "" {
+				// Ignore empty values, and consider them to not be set
+				continue


I'm not totally sure we should ignore empty values, as it could add some confusion to the user (maybe they were expecting the header to be set but empty). Are there scenarios in which we might expect this to occur?

My somewhat anticipation here was for My-Header=${SOME_ENV_VAR}. I'd be ok with starting with this being an error condition though (which would still keep all options open)

OK, gave this some thinking, and it's probably fine to just set an empty value, and leave it an exercise to the user to handle My-Header=${SOME_ENV_VAR} in situations where $SOME_ENV_VAR is not set (they can use the magic of (Ba)sh to handle unset env-vars.

thaJeztah · 2024-06-04T12:58:47Z

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 · 2024-07-19T12:09:29Z

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)

krissetto

overall LGTM, some minor comments

krissetto · 2024-07-19T12:38:49Z

cli/command/cli_options.go

@@ -108,3 +113,107 @@ func WithAPIClient(c client.APIClient) CLIOption {
 		return nil
 	}
 }
+
+// envOverrideHTTPHeaders is the name of the environment variable that can be


nit: a bit pedantic, but should we go with only one way of writing these in the docstrings?

environment variable

environment-variable

env-var

Heh, yeah, I keep making that mistake. Changed to environment-variable which seems the most common use, but we should make a pass at looking through our use for consistency

cli/command/cli_options.go

krissetto · 2024-07-19T12:49:01Z

cli/command/cli_options.go

+			k = strings.TrimSpace(k)
+
+			if k == "" {
+				return errdefs.InvalidParameter(errors.Errorf("failed to set custom headers from %s environment variable: value contains a key=value pair with an empty key", envOverrideHTTPHeaders))


could it make sense to print out the wrong key-value in this error message like we do below, to help the user in spotting the bad part of the string? I'm imagining something similar to:

failed to set custom headers from DOCKER_CUSTOM_HEADERS environment variable: value contains a key=value pair with an empty key (' =somevalue')

where the error shown also contains the potential whitespace (so k before trimming the space)

ah, yes, probably works; let me look

Added; I decided to change the error-messages to use '%s' (not %q) to prevent double-escaped quotes in the output

cli/command/cli_options.go

krissetto · 2024-07-19T12:53:02Z

docs/reference/commandline/docker.md

 | `DOCKER_API_VERSION`          | Override the negotiated API version to use for debugging (e.g. `1.19`)                                                                                                                                                                                            |
 | `DOCKER_CERT_PATH`            | Location of your authentication keys. This variable is used both by the `docker` CLI and the [`dockerd` daemon](https://docs.docker.com/reference/cli/dockerd/)                                                                                                   |
 | `DOCKER_CONFIG`               | The location of your client configuration files.                                                                                                                                                                                                                  |
 | `DOCKER_CONTENT_TRUST_SERVER` | The URL of the Notary server to use. Defaults to the same URL as the registry.                                                                                                                                                                                    |
 | `DOCKER_CONTENT_TRUST`        | When set Docker uses notary to sign and verify images. Equates to `--disable-content-trust=false` for build, create, pull, push, run.                                                                                                                             |
 | `DOCKER_CONTEXT`              | Name of the `docker context` to use (overrides `DOCKER_HOST` env var and default context set with `docker context use`)                                                                                                                                           |
+| `DOCKER_CUSTOM_HEADERS`       | (Experimental) Configure [custom HTTP headers](#custom-http-headers) to be sent by the client. Headers must be provided as a comma-separated list of `name=value` pairs. This is the equivalent to the `HttpHeaders` field in the configuration file.             |


I think we should specify also here that, when the env var is populated, it's values will replace all values set via the configuration file

Yeah, I tried to shorten the description here, otherwise the whole table would be re-formatted 😂

I think we may need to do another pass at docs in a follow-up; I'm keeping this as-is for now, and we can do a follow-up to improve things if that's ok for you.

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: 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). Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah force-pushed the custom_headers_env_var branch 2 times, most recently from dc2c0f1 to 0d6b805 Compare June 4, 2024 10:04

thaJeztah mentioned this pull request Jun 4, 2024

[26.1 backport] add support for DOCKER_CUSTOM_HEADERS env-var #5112

Closed

thaJeztah added status/2-code-review area/context area/api labels Jun 4, 2024

krissetto reviewed Jun 4, 2024

View reviewed changes

thaJeztah force-pushed the custom_headers_env_var branch from 0d6b805 to af2dde4 Compare July 19, 2024 12:07

thaJeztah force-pushed the custom_headers_env_var branch from af2dde4 to 5713d6d Compare July 19, 2024 12:21

thaJeztah added process/cherry-pick/27.0 impact/changelog labels Jul 19, 2024

thaJeztah added this to the 28.0.0 milestone Jul 19, 2024

thaJeztah mentioned this pull request Jul 19, 2024

[27.1 backport] add support for DOCKER_CUSTOM_HEADERS env-var (experimental) #5271

Merged

thaJeztah marked this pull request as ready for review July 19, 2024 12:24

thaJeztah requested a review from a team as a code owner July 19, 2024 12:24

thaJeztah force-pushed the custom_headers_env_var branch from 5713d6d to 2f336a6 Compare July 19, 2024 12:28

thaJeztah changed the title ~~add support for DOCKER_CUSTOM_HEADERS env-var~~ add support for DOCKER_CUSTOM_HEADERS env-var (experimental) Jul 19, 2024

thaJeztah added the kind/experimental label Jul 19, 2024

vvoland approved these changes Jul 19, 2024

View reviewed changes

krissetto approved these changes Jul 19, 2024

View reviewed changes

thaJeztah force-pushed the custom_headers_env_var branch from 2f336a6 to 6638deb Compare July 19, 2024 13:04

thaJeztah merged commit 5568565 into docker:master Jul 19, 2024
89 checks passed

thaJeztah deleted the custom_headers_env_var branch July 19, 2024 14:43

thaJeztah added process/cherry-picked and removed process/cherry-pick/27.0 labels Jul 22, 2024

thaJeztah mentioned this pull request Sep 11, 2024

Unable to resolve docker context 'desktop-linux' with VisualStudio 2022 and Docker Desktop v4.34.0 docker/for-win#14295

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

thaJeztah commented May 31, 2024 •

edited

Loading

codecov-commenter commented Jun 4, 2024 •

edited

Loading

laurazard commented Jun 4, 2024

krissetto Jun 4, 2024

thaJeztah Jun 4, 2024

krissetto Jun 4, 2024

thaJeztah Jun 4, 2024

thaJeztah Jul 19, 2024

krissetto Jun 4, 2024

thaJeztah Jun 4, 2024

thaJeztah Jul 19, 2024

thaJeztah commented Jun 4, 2024

thaJeztah commented Jul 19, 2024

krissetto left a comment

krissetto Jul 19, 2024

thaJeztah Jul 19, 2024

krissetto Jul 19, 2024

thaJeztah Jul 19, 2024

thaJeztah Jul 19, 2024

krissetto Jul 19, 2024

thaJeztah Jul 19, 2024

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

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

Conversation

thaJeztah commented May 31, 2024 • edited Loading

codecov-commenter commented Jun 4, 2024 • edited Loading

Codecov Report

laurazard commented Jun 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Jun 4, 2024

thaJeztah commented Jul 19, 2024

krissetto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented May 31, 2024 •

edited

Loading

codecov-commenter commented Jun 4, 2024 •

edited

Loading