Skip to content

build: make proxy build-arg override check case-insensitive#3697

Open
crazy-max wants to merge 1 commit intodocker:masterfrom
crazy-max:build-arg-proxy
Open

build: make proxy build-arg override check case-insensitive#3697
crazy-max wants to merge 1 commit intodocker:masterfrom
crazy-max:build-arg-proxy

Conversation

@crazy-max
Copy link
Member

fixes #3636

Buildx currently checks whether a proxy default should be injected using a case-sensitive map lookup, so --build-arg no_proxy=... does not block injecting NO_PROXY from ~/.docker/config.json.

That can result in both build-arg:no_proxy and build-arg:NO_PROXY being sent to BuildKit and because BuildKit resolves proxy args case-insensitively from a Go map, the winning value can vary by map iteration order.

With this patch, Buildx uses a case-insensitive key existence check before injecting proxy defaults, so an explicit CLI proxy arg consistently takes precedence. This aligns behavior with user expectation that explicit --build-arg values should deterministically override config defaults.

Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com>
Comment on lines +174 to +175
name: "case insensitive match",
proxyArgs: map[string]string{"no_proxy": "cli"},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know in the CLI and daemon we only consider NO_PROXY and no_proxy, but don't consider nO_pRoXy; perhaps worth considering.

(I can dig up a link to the code if needed)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g.;

https://github.com/docker/cli/blob/5927d80c76b3ce5cf782be818922966e8a0d87a3/cli/config/configfile/file.go#L218-L253

Also possibly relevant; if either HTTP_PROXY or http_proxy is set, we set both (because upper/lowercase depends on implementation, and there's no official standard)
https://github.com/moby/moby/blob/83bca512aa7ffc1bb4f37ce1107e0d3e3489ad43/daemon/command/daemon.go#L1119-L1132

func configureProxyEnv(ctx context.Context, cfg config.Proxies) {
	if p := cfg.HTTPProxy; p != "" {
		overrideProxyEnv(ctx, "HTTP_PROXY", p)
		overrideProxyEnv(ctx, "http_proxy", p)
	}
	if p := cfg.HTTPSProxy; p != "" {
		overrideProxyEnv(ctx, "HTTPS_PROXY", p)
		overrideProxyEnv(ctx, "https_proxy", p)
	}
	if p := cfg.NoProxy; p != "" {
		overrideProxyEnv(ctx, "NO_PROXY", p)
		overrideProxyEnv(ctx, "no_proxy", p)
	}
}

@tonistiigi
Copy link
Member

I guess we should also fix it in proxyEnvFromBuildArgs where the undeterministic map iteration is. I'm not sure what is the preferred order but main thing would be to just not be random.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Non-deterministic precedence between --build-arg no_proxy=... and ~/.docker/config.json proxy defaults

3 participants