Make daemon proxy settings configurable through daemon.json by aiordache · Pull Request #42647 · moby/moby

aiordache · 2021-07-16T11:49:57Z

Added proxy properties to daemon configuration (config file and dockerd flags).

- How to verify it

$ cat daemon.json 
{
    "http-proxy": "http://proxytest.example.com:80",
    "https-proxy": "https://proxytest.example.com:443"
}

$ dockerd --config-file daemon.json

$ docker pull busybox
Using default tag: latest
Error response from daemon: Get "https://registry-1.docker.io/v2/": proxyconnect tcp: dial tcp: lookup proxytest.example.com on 127.0.0.11:53: no such host


# docker build .     
Sending build context to Docker daemon  89.28MB
Step 1/3 : FROM golang:1.16-alpine AS base
Get "https://registry-1.docker.io/v2/": proxyconnect tcp: dial tcp: lookup proxytest.example.com on 127.0.0.11:53: no such host

thaJeztah · 2021-07-16T16:26:21Z

Hm.. but this only changes what's shown in docker info, but not actually configures the daemon to use these proxies? 🤔

FWIW, default behavior; https://github.com/golang/go/blob/912f0750472dd4f674b69ca1616bfaf377af1805/src/net/http/transport.go#L422-L441 / https://github.com/golang/go/blob/912f0750472dd4f674b69ca1616bfaf377af1805/src/net/http/transport.go#L43-L54

(and we are using https://github.com/docker/go-connections for some central/default configuration, which may be relevant)

thaJeztah · 2021-07-16T16:27:17Z

Ah, right. Guess this is the "magic" bit

Looking at this again, I think we should do this explicitly. Currently, setting the option relies on a side-effect of getEnvOrConfig() (and thus a side effect of daemon.SystemInfo(). I think it will be clearer to perform this step in the code that starts the daemon (after we did the initial validation of the daemon's configuration).

Signed-off-by: Anca Iordache <[email protected]>

tianon · 2021-07-16T23:55:40Z

Honestly having a really hard time understanding the justification here -- why not use the frankly pretty well standardized environment variables instead?

fredericdalleau

LGTM

thaJeztah

Thanks! Sorry for the late reply on this one. I started a review, and had a branch with changes, but didn't post it, and had to rebase my branch to still work against master. Let me know if you want to take my branch or if I should push it as a separate PR (I left some commits un-squashed to explain the steps, and should probably squash those); https://github.com/moby/moby/compare/master...thaJeztah:proxy_daemon_config_carry?expand=1

thaJeztah · 2021-08-31T12:44:30Z

+		HTTPProxy:          maskCredentials(getEnvOrConfig(daemon.configStore.HTTPProxy, "HTTP_PROXY", "http_proxy")),
+		HTTPSProxy:         maskCredentials(getEnvOrConfig(daemon.configStore.HTTPSProxy, "HTTPS_PROXY", "https_proxy")),


We should make sure that these values are sanitised in other places as well;

the daemon prints an error message when it finds conflicting options (options passed both on the command-line and in the daemon.json), which contains the values for both.

when reloading the daemon configuration (kill -s HUP ..), the daemon's active configuration is printed.

Unfortunately it's a bit more involved to do so;

the maskCredentials() utility is not exported, so can't be used in some of those locations

in the SIGHUP / reload case, we marshal the whole config as JSON; sanitising the value will alter the actual config, so we need to either do a deep copy of the config, or we can extract the proxy-configuration fields to an embedded struct, and override.

thaJeztah · 2021-08-31T12:56:05Z

+func getEnvOrConfig(config string, env ...string) string {
+	val := getEnvAny(env...)
+	if val != "" {
+		return val
+	}


The order in which we consider these options looks to be incorrect. I think the order of precedence is; (daemon.json | daemon flags) > env-vars. In other words; if a proxy is configured either as a command-line flag (--https-proxy) or through the daemon.json, it should override any env-var that's set.

thaJeztah · 2021-08-31T12:58:05Z

Looking at this again, I think we should do this explicitly. Currently, setting the option relies on a side-effect of getEnvOrConfig() (and thus a side effect of daemon.SystemInfo(). I think it will be clearer to perform this step in the code that starts the daemon (after we did the initial validation of the daemon's configuration).

tianon · 2021-08-31T14:45:32Z

Honestly having a really hard time understanding the justification here -- why not use the frankly pretty well standardized environment variables instead?

To follow up on this thought, the answer is Windows, where adding a variable to a service is anything but straightforward. 😬

thaJeztah · 2021-09-09T12:07:29Z

Carrying these changes with my review suggestions in #42835

thaJeztah reviewed Jul 16, 2021

View reviewed changes

ghost approved these changes Jul 16, 2021

View reviewed changes

aiordache force-pushed the proxy_daemon_config branch from e12b75b to 05e3527 Compare July 16, 2021 17:15

ghost approved these changes Jul 16, 2021

View reviewed changes

Add http proxy properties to daemon configuration

05e3527

Signed-off-by: Anca Iordache <[email protected]>

fredericdalleau approved these changes Aug 27, 2021

View reviewed changes

thaJeztah requested changes Aug 31, 2021

View reviewed changes

thaJeztah added area/daemon Core Engine docs/revisit impact/changelog status/2-code-review labels Aug 31, 2021

thaJeztah mentioned this pull request Sep 9, 2021

Add http(s) proxy properties to daemon configuration (carry 42647) #42835

Merged

thaJeztah closed this Sep 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make daemon proxy settings configurable through daemon.json#42647

Make daemon proxy settings configurable through daemon.json#42647
aiordache wants to merge 1 commit intomoby:masterfrom
aiordache:proxy_daemon_config

aiordache commented Jul 16, 2021 •

edited by thaJeztah

Loading

Uh oh!

thaJeztah Jul 16, 2021

Uh oh!

thaJeztah Jul 16, 2021

Uh oh!

thaJeztah Aug 31, 2021

Uh oh!

tianon commented Jul 16, 2021

Uh oh!

fredericdalleau left a comment

Uh oh!

thaJeztah left a comment

Uh oh!

thaJeztah Aug 31, 2021

Uh oh!

thaJeztah Aug 31, 2021

Uh oh!

thaJeztah Aug 31, 2021

Uh oh!

tianon commented Aug 31, 2021

Uh oh!

thaJeztah commented Sep 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		HTTPProxy: maskCredentials(getEnvOrConfig(daemon.configStore.HTTPProxy, "HTTP_PROXY", "http_proxy")),
		HTTPSProxy: maskCredentials(getEnvOrConfig(daemon.configStore.HTTPSProxy, "HTTPS_PROXY", "https_proxy")),

Conversation

aiordache commented Jul 16, 2021 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah Jul 16, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jul 16, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah Aug 31, 2021

Choose a reason for hiding this comment

Uh oh!

tianon commented Jul 16, 2021

Uh oh!

fredericdalleau left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Aug 31, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah Aug 31, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah Aug 31, 2021

Choose a reason for hiding this comment

Uh oh!

tianon commented Aug 31, 2021

Uh oh!

thaJeztah commented Sep 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aiordache commented Jul 16, 2021 •

edited by thaJeztah

Loading