Skip to content

image build: add --image-pull-{proxy,noproxy}#86

Merged
chmeliik merged 2 commits intokonflux-ci:mainfrom
chmeliik:pull-proxy
Mar 24, 2026
Merged

image build: add --image-pull-{proxy,noproxy}#86
chmeliik merged 2 commits intokonflux-ci:mainfrom
chmeliik:pull-proxy

Conversation

@chmeliik
Copy link
Copy Markdown
Contributor

Sets the relevant environment variables in the 'buildah pull' environment to support the layer caching proxy 1.

Note that the Konflux buildah task also used to set ALL_PROXY in addition to HTTP_PROXY and HTTPS_PROXY. This has always been useless. ALL_PROXY is not a standard environment variable and the Go libraries used by buildah don't take it into account at all.

@chmeliik chmeliik requested a review from a team as a code owner March 20, 2026 10:08
@qodo-code-review
Copy link
Copy Markdown

qodo-code-review bot commented Mar 20, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 📐 Spec deviations (0)

Grey Divider


Remediation recommended

1. Duplicate proxy env entries 🐞 Bug ⚙ Maintainability
Description
BuildahCli.Pull appends proxy variables onto os.Environ() without removing existing
HTTP_PROXY/HTTPS_PROXY/NO_PROXY (and lowercase) entries, which can leave duplicate keys in
cmd.Env and make the effective environment harder to reason about. This is avoidable by merging
env with key replacement semantics before executing the command.
Code

pkg/cliwrappers/buildah.go[R324-328]

+	cmd := Cmd{Name: "buildah", Args: buildahArgs, LogOutput: true}
+	env := common.ProxyEnvVars(args.HttpProxy, args.NoProxy)
+	if len(env) > 0 {
+		cmd.Env = append(os.Environ(), env...)
+	}
Evidence
BuildahCli.Pull constructs cmd.Env by blindly appending new proxy variables to the current
process environment. The proxy helper always emits multiple proxy-related keys (upper + lower case),
and CliExecutor.Execute forwards the Env slice directly to exec.Cmd.Env, so any pre-existing
proxy keys remain alongside the new ones.

pkg/cliwrappers/buildah.go[324-328]
pkg/common/proxy_env.go[10-22]
pkg/cliwrappers/cli_executor.go[45-49]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`pkg/cliwrappers/buildah.go` builds `cmd.Env` via `append(os.Environ(), env...)`, which can produce duplicate environment-variable keys (e.g., `HTTP_PROXY` already present in `os.Environ()` plus a second `HTTP_PROXY=...` appended). This makes the command environment harder to reason about and unnecessarily bloats `cmd.Env`.

### Issue Context
`common.ProxyEnvVars()` emits both uppercase and lowercase variants, increasing the likelihood of duplicates when the parent environment already contains proxy settings.

### Fix Focus Areas
- Introduce an env-merge helper that **replaces** existing keys (case-sensitive match on the exact keys you generate) instead of allowing duplicates, and use it when setting `cmd.Env`.
- Ensure the merge logic handles all keys produced by `common.ProxyEnvVars` (`HTTP_PROXY`, `http_proxy`, `HTTPS_PROXY`, `https_proxy`, `NO_PROXY`, `no_proxy`).

### Fix Focus Areas (code references)
- pkg/cliwrappers/buildah.go[324-328]
- pkg/common/proxy_env.go[10-22]
- pkg/cliwrappers/cli_executor.go[45-49]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@snyk-io
Copy link
Copy Markdown

snyk-io bot commented Mar 20, 2026

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@chmeliik
Copy link
Copy Markdown
Contributor Author

Tested in chmeliik#19 (with chmeliik/build-definitions@8fc9d38)

@chmeliik
Copy link
Copy Markdown
Contributor Author

chmeliik commented Mar 20, 2026

@qodo-code-review

  1. Duplicate proxy env entries

IMO the "last value wins" behavior of exec.Cmd is pretty obvious, but added a comment about it

Comment on lines +447 to +448
HttpProxy: "http://proxy.example.com:8080",
NoProxy: "localhost,127.0.0.1",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure how much this test is testing, probably only the fact that the env vars were set.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's it. I didn't find any easy way to integration-test it.

One idea was to set those URLs to something that will cause the pull to fail, but then the test would spend 8 minutes retrying

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As an option, we can try to use golang stdlib: net/http/httputil to setup proxy just for test. I think it should be possible to even make sure the proxy got the request.

Regarding waiting time, will DisableRetryer=true help?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good call! I didn't know how simple it was to create a tunneling proxy (especially with Claude's help 😄 ). Replaced the two questionable unit tests with an integration test.

func (e *CliExecutor) Execute(c Cmd) (string, string, int, error) {
cmd := exec.Command(c.Name, c.Args...)
cmd.Dir = c.Dir
cmd.Env = c.Env
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is replacing of all env vars intentional?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The intention is to have the same semantics as exec.Cmd - leaving it nil inherits the parent process env, otherwise replace

Copy link
Copy Markdown
Member

@mmorhun mmorhun left a comment

Choose a reason for hiding this comment

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

The suggestions are optional, so up to you.

Assisted-by: Claude
Signed-off-by: Adam Cmiel <acmiel@redhat.com>
Sets the relevant environment variables in the 'buildah pull'
environment to support the layer caching proxy [1].

Note that the Konflux buildah task also used to set ALL_PROXY in
addition to HTTP_PROXY and HTTPS_PROXY. This has always been useless.
ALL_PROXY is not a standard environment variable and the Go libraries
used by buildah don't take it into account at all.

[1]: https://github.com/konflux-ci/architecture/blob/main/ADR/0047-caching-for-container-build-layers.md

Assisted-by: Claude
Signed-off-by: Adam Cmiel <acmiel@redhat.com>
@chmeliik chmeliik merged commit a7c1149 into konflux-ci:main Mar 24, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants