Improve proxy handling for builds#9741
Conversation
Signed-off-by: Jonas Wood <jw@ti.com>
jonaswood01
requested review from
ColorfulRhino and
schwar3kat
and removed request for
a team
📝 WalkthroughWalkthroughThe changes enhance proxy environment variable handling and DNS resolution across multiple configuration and tool execution functions. Configuration now auto-derives Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
github-actions
Bot
added
size/small
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/functions/general/oci-oras.sh (1)
93-102:⚠️ Potential issue | 🟠 MajorForward the full proxy set into ORAS, not just
HTTPS_PROXY.These calls also run under
env -i. Preserving onlyHTTPS_PROXYstill breaks lowercase-only proxy setups, dropsHTTP_PROXYfor--plain-httpregistries, and losesNO_PROXYfor internal registries that should bypass the proxy.🔧 Suggested change
declare ORAS_HOME="${HOME:-"${TMPDIR}"}" # oras _requires_ a HOME to work atleast in 1.2+ + declare -a oras_proxy_env=( + "http_proxy=${http_proxy:-${HTTP_PROXY:-}}" + "https_proxy=${https_proxy:-${HTTPS_PROXY:-}}" + "HTTP_PROXY=${HTTP_PROXY:-${http_proxy:-}}" + "HTTPS_PROXY=${HTTPS_PROXY:-${https_proxy:-}}" + "no_proxy=${no_proxy:-${NO_PROXY:-}}" + "NO_PROXY=${NO_PROXY:-${no_proxy:-}}" + ) display_alert "Running ORAS ${ACTUAL_VERSION}" "HOME='${ORAS_HOME}'; retries='${retries:-1}'; cmdline: $*" "debug" if [[ "${retries:-1}" -gt 1 ]]; then display_alert "Calling ORAS with retries ${retries}" "$*" "debug" - sleep_seconds="30" do_with_retries "${retries}" env -i "HOME=${ORAS_HOME}" "HTTPS_PROXY=${HTTPS_PROXY:-}" "${ORAS_BIN}" "$@" + sleep_seconds="30" do_with_retries "${retries}" env -i "HOME=${ORAS_HOME}" "${oras_proxy_env[@]}" "${ORAS_BIN}" "$@" else @@ - env -i "HOME=${ORAS_HOME}" "HTTPS_PROXY=${HTTPS_PROXY:-}" "${ORAS_BIN}" "$@" + env -i "HOME=${ORAS_HOME}" "${oras_proxy_env[@]}" "${ORAS_BIN}" "$@" fiBased on learnings: The Armbian build framework uses lowercase proxy environment variables (
http_proxy,https_proxy) across build tooling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/functions/general/oci-oras.sh` around lines 93 - 102, Summary: proxy envs are stripped by env -i and only HTTPS_PROXY is forwarded, breaking lowercase proxies, HTTP_PROXY and NO_PROXY. Fix: when invoking ORAS (both the do_with_retries call and the direct env -i invocation around ORAS_BIN) preserve all proxy-related variables by exporting/forwarding HTTP_PROXY, HTTPS_PROXY, http_proxy, https_proxy, NO_PROXY and no_proxy (e.g., add "HTTP_PROXY=${HTTP_PROXY:-}" "HTTPS_PROXY=${HTTPS_PROXY:-}" "http_proxy=${http_proxy:-}" "https_proxy=${https_proxy:-}" "NO_PROXY=${NO_PROXY:-}" "no_proxy=${no_proxy:-}" to the env -i invocation), keep HOME=ORAS_HOME and preserve the existing no-args branch behavior; modify the lines that call ORAS_BIN and the do_with_retries wrapper to include these variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/functions/configuration/main-config.sh`:
- Around line 455-459: The code currently derives APT_PROXY_ADDR from proxy env
vars and then logs the raw value, which can include credentials; update the
logic that sets APT_PROXY_ADDR (the assignment using echo | sed) to strip any
user:pass@ credentials so APT_PROXY_ADDR contains only host[:port], and change
the call to display_alert("Derived APT proxy address from proxy env vars", ...)
to log the sanitized/masked value (or just the hostname:port) instead of the
original environment string; ensure the credential-stripping is applied before
any use of APT_PROXY_ADDR so functions like display_alert and runners.sh never
see or log the user info.
In `@lib/functions/general/python-tools.sh`:
- Around line 129-133: Update the two isolated pip invocations that call
run_host_command_logged so they propagate both uppercase and lowercase proxy
vars and the no_proxy bypass, and ensure arrays are passed using the `@Q` form;
specifically, replace the env -i argument list to include
"HTTP_PROXY=${HTTP_PROXY:-}" "HTTPS_PROXY=${HTTPS_PROXY:-}"
"http_proxy=${http_proxy:-}" "https_proxy=${https_proxy:-}"
"NO_PROXY=${NO_PROXY:-}" "no_proxy=${no_proxy:-}", and call arrays as
${PYTHON3_VARS[@]@Q} and "${pip3_extra_args[@]@Q}" when invoking
"${PYTHON3_INFO[BIN]}", "${PYTHON3_INFO[GET_PIP_BIN]}", pip3 install and the -r
"${python3_pip_dependencies_path}" step so
run_host_command_logged/run_host_command_logged env invocations preserve proxy
settings consistently.
---
Outside diff comments:
In `@lib/functions/general/oci-oras.sh`:
- Around line 93-102: Summary: proxy envs are stripped by env -i and only
HTTPS_PROXY is forwarded, breaking lowercase proxies, HTTP_PROXY and NO_PROXY.
Fix: when invoking ORAS (both the do_with_retries call and the direct env -i
invocation around ORAS_BIN) preserve all proxy-related variables by
exporting/forwarding HTTP_PROXY, HTTPS_PROXY, http_proxy, https_proxy, NO_PROXY
and no_proxy (e.g., add "HTTP_PROXY=${HTTP_PROXY:-}"
"HTTPS_PROXY=${HTTPS_PROXY:-}" "http_proxy=${http_proxy:-}"
"https_proxy=${https_proxy:-}" "NO_PROXY=${NO_PROXY:-}" "no_proxy=${no_proxy:-}"
to the env -i invocation), keep HOME=ORAS_HOME and preserve the existing no-args
branch behavior; modify the lines that call ORAS_BIN and the do_with_retries
wrapper to include these variables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ca27b929-77df-4fa1-abce-8a0c4b63c67b
📒 Files selected for processing (4)
lib/functions/configuration/main-config.shlib/functions/general/oci-oras.shlib/functions/general/python-tools.shlib/functions/host/docker.sh
| # Derive APT_PROXY_ADDR from proxy env vars if unset, which runners.sh uses inside chroot | ||
| if [[ -z "${APT_PROXY_ADDR}" && -n "${http_proxy:-${https_proxy:-${HTTP_PROXY:-${HTTPS_PROXY:-}}}}" ]]; then | ||
| APT_PROXY_ADDR="$(echo "${http_proxy:-${https_proxy:-${HTTP_PROXY:-${HTTPS_PROXY:-}}}}" | sed -E 's|https?://([^/]+).*|\1|')" | ||
| display_alert "Derived APT proxy address from proxy env vars" "${APT_PROXY_ADDR}" "info" | ||
| fi |
There was a problem hiding this comment.
Avoid logging credential-bearing proxy values.
If the source proxy env is something like http://user:pass@proxy:port, Line 458 writes secrets into the build log, and the existing log at Line 460 repeats it. That is a pretty likely corporate-proxy setup.
🔒 Suggested change
# Derive APT_PROXY_ADDR from proxy env vars if unset, which runners.sh uses inside chroot
if [[ -z "${APT_PROXY_ADDR}" && -n "${http_proxy:-${https_proxy:-${HTTP_PROXY:-${HTTPS_PROXY:-}}}}" ]]; then
APT_PROXY_ADDR="$(echo "${http_proxy:-${https_proxy:-${HTTP_PROXY:-${HTTPS_PROXY:-}}}}" | sed -E 's|https?://([^/]+).*|\1|')"
- display_alert "Derived APT proxy address from proxy env vars" "${APT_PROXY_ADDR}" "info"
+ display_alert "Derived APT proxy address from proxy env vars" "${APT_PROXY_ADDR##*@}" "info"
fi
- [[ -n "${APT_PROXY_ADDR}" ]] && display_alert "Using custom apt proxy address" "APT_PROXY_ADDR=${APT_PROXY_ADDR}" "info"
+ [[ -n "${APT_PROXY_ADDR}" ]] && display_alert "Using custom apt proxy address" "APT_PROXY_ADDR=${APT_PROXY_ADDR##*@}" "info"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/functions/configuration/main-config.sh` around lines 455 - 459, The code
currently derives APT_PROXY_ADDR from proxy env vars and then logs the raw
value, which can include credentials; update the logic that sets APT_PROXY_ADDR
(the assignment using echo | sed) to strip any user:pass@ credentials so
APT_PROXY_ADDR contains only host[:port], and change the call to
display_alert("Derived APT proxy address from proxy env vars", ...) to log the
sanitized/masked value (or just the hostname:port) instead of the original
environment string; ensure the credential-stripping is applied before any use of
APT_PROXY_ADDR so functions like display_alert and runners.sh never see or log
the user info.
| run_host_command_logged env -i "HTTP_PROXY=${HTTP_PROXY:-}" "HTTPS_PROXY=${HTTPS_PROXY:-}" "${PYTHON3_VARS[@]@Q}" "${PYTHON3_INFO[BIN]}" "${PYTHON3_INFO[GET_PIP_BIN]}" "${pip3_extra_args[@]}" "pip==${pip3_version_number}" | ||
|
|
||
| # Install the dependencies | ||
| display_alert "Installing Python dependencies" "from ${python3_pip_dependencies_path}" "info" | ||
| run_host_command_logged env -i "${PYTHON3_VARS[@]@Q}" "${PYTHON3_INFO[BIN]}" -m pip install "${pip3_extra_args[@]}" -r "${python3_pip_dependencies_path}" | ||
| run_host_command_logged env -i "HTTP_PROXY=${HTTP_PROXY:-}" "HTTPS_PROXY=${HTTPS_PROXY:-}" "${PYTHON3_VARS[@]@Q}" "${PYTHON3_INFO[BIN]}" -m pip install "${pip3_extra_args[@]}" -r "${python3_pip_dependencies_path}" |
There was a problem hiding this comment.
Propagate lowercase and bypass proxy vars in these isolated pip steps.
Because both commands run under env -i, a host configured only with http_proxy / https_proxy / no_proxy still reaches these installs with an empty proxy config. That keeps the Python bootstrap path inconsistent with the Docker launcher and can still fail in the proxy setups this PR is trying to fix.
🔧 Suggested change
+ declare -a python_proxy_env=(
+ "http_proxy=${http_proxy:-${HTTP_PROXY:-}}"
+ "https_proxy=${https_proxy:-${HTTPS_PROXY:-}}"
+ "HTTP_PROXY=${HTTP_PROXY:-${http_proxy:-}}"
+ "HTTPS_PROXY=${HTTPS_PROXY:-${https_proxy:-}}"
+ "no_proxy=${no_proxy:-${NO_PROXY:-}}"
+ "NO_PROXY=${NO_PROXY:-${no_proxy:-}}"
+ )
- run_host_command_logged env -i "HTTP_PROXY=${HTTP_PROXY:-}" "HTTPS_PROXY=${HTTPS_PROXY:-}" "${PYTHON3_VARS[@]@Q}" "${PYTHON3_INFO[BIN]}" "${PYTHON3_INFO[GET_PIP_BIN]}" "${pip3_extra_args[@]}" "pip==${pip3_version_number}"
+ run_host_command_logged env -i "${python_proxy_env[@]@Q}" "${PYTHON3_VARS[@]@Q}" "${PYTHON3_INFO[BIN]}" "${PYTHON3_INFO[GET_PIP_BIN]}" "${pip3_extra_args[@]}" "pip==${pip3_version_number}"
@@
- run_host_command_logged env -i "HTTP_PROXY=${HTTP_PROXY:-}" "HTTPS_PROXY=${HTTPS_PROXY:-}" "${PYTHON3_VARS[@]@Q}" "${PYTHON3_INFO[BIN]}" -m pip install "${pip3_extra_args[@]}" -r "${python3_pip_dependencies_path}"
+ run_host_command_logged env -i "${python_proxy_env[@]@Q}" "${PYTHON3_VARS[@]@Q}" "${PYTHON3_INFO[BIN]}" -m pip install "${pip3_extra_args[@]}" -r "${python3_pip_dependencies_path}"Based on learnings: The Armbian build framework uses lowercase proxy environment variables in tooling, and arrays passed through run_host_command_logged should use ${array[@]@Q}.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/functions/general/python-tools.sh` around lines 129 - 133, Update the two
isolated pip invocations that call run_host_command_logged so they propagate
both uppercase and lowercase proxy vars and the no_proxy bypass, and ensure
arrays are passed using the `@Q` form; specifically, replace the env -i argument
list to include "HTTP_PROXY=${HTTP_PROXY:-}" "HTTPS_PROXY=${HTTPS_PROXY:-}"
"http_proxy=${http_proxy:-}" "https_proxy=${https_proxy:-}"
"NO_PROXY=${NO_PROXY:-}" "no_proxy=${no_proxy:-}", and call arrays as
${PYTHON3_VARS[@]@Q} and "${pip3_extra_args[@]@Q}" when invoking
"${PYTHON3_INFO[BIN]}", "${PYTHON3_INFO[GET_PIP_BIN]}", pip3 install and the -r
"${python3_pip_dependencies_path}" step so
run_host_command_logged/run_host_command_logged env invocations preserve proxy
settings consistently.
|
how does this contrast with #8811 and |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me gather details on PR 🏁 Script executed: Length of output: 1075 🏁 Script executed: Length of output: 6123
What PR
|
MANAGE_ACNG value |
Effect in runners.sh (chroot apt) |
Effect in rootfs-create.sh (debootstrap mirror) |
|---|---|---|
yes |
Uses http://${APT_PROXY_ADDR:-localhost:3142} as apt proxy |
Prefixes mirror with http://localhost:3142/ |
<URL> (from #8811) |
Not matched (== "yes" check fails) → falls to APT_PROXY_ADDR elif |
Prefixes debootstrap mirror with the custom URL |
no |
No apt proxy | No prefix |
So there's already a subtle gap introduced by #8811: MANAGE_ACNG=<url> sets the debootstrap mirror prefix correctly, but does not propagate to the apt-get proxy used inside the chroot via runners.sh — that still depends on APT_PROXY_ADDR.
What APT_PROXY_ADDR does (pre-existing)
APT_PROXY_ADDR is the variable runners.sh uses to route chroot apt-get operations through a proxy. It pre-dates #8811. With MANAGE_ACNG=yes, APT_PROXY_ADDR overrides the default localhost:3142. With MANAGE_ACNG not yes, it acts as a standalone apt proxy (the elif branch).
What PR #9741 adds
PR #9741 auto-derives APT_PROXY_ADDR (host:port, scheme stripped) from HTTP_PROXY/http_proxy/HTTPS_PROXY/https_proxy only when APT_PROXY_ADDR is not already set. It also forwards APT_PROXY_ADDR into the Docker container via --env.
This targets users in corporate proxy environments where the proxy is already set as a standard env var — without this, chroot apt-get operations would fail since env -i environments don't inherit those vars.
How they contrast
MANAGE_ACNGis an explicit, opt-in mechanism for caching (localapt-cacher-ngdaemon or a custom acng URL per#8811). It requires deliberate user configuration.APT_PROXY_ADDR(auto-derived by#9741) is an implicit, environment-driven mechanism — it bridges the gap for users whose proxy is already declared in the shell but not known to the build system.
There is no direct conflict: MANAGE_ACNG=yes takes precedence in runners.sh, and APT_PROXY_ADDR can even customize the localhost:3142 address in that path. For MANAGE_ACNG=<url> (the #8811 case), the chroot apt proxy falls through to APT_PROXY_ADDR anyway, so the auto-derivation in #9741 would fill that gap for debootstrap-external acng users who are also behind a proxy.
One thing worth scrutinising in #9741: when a user has MANAGE_ACNG=<url> (an acng instance) and HTTP_PROXY set to a different corporate proxy, the auto-derivation will set APT_PROXY_ADDR to the corporate proxy rather than the acng URL. This could be surprising — APT_PROXY_ADDR should arguably be explicitly set by the user in that case, or the auto-derivation logic should be skipped when MANAGE_ACNG is a URL.
🧠 Learnings used
Learnt from: EvilOlaf
Repo: armbian/build PR: 8281
File: lib/functions/host/docker.sh:412-418
Timestamp: 2025-06-09T06:59:27.323Z
Learning: The Armbian build framework uses lowercase proxy environment variables (http_proxy, https_proxy) in GPG keyserver operations within lib/functions/general/downloads.sh, and many other tools like curl, wget, python/pip also respect lowercase proxy variables.
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-08-19T04:55:45.648Z
Learning: In Armbian build configurations, HTTP is preferred over HTTPS for APT repository URLs because: 1) APT package signing provides sufficient tamper protection through cryptographic verification, 2) HTTPS complicates package caching with tools like Squid, and 3) this is an established pattern across their repository configurations.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub API (https://api.github.com/repos/armbian/build/pulls/{pr_number}/files) to get the complete picture of what files are being added or modified, especially for U-Boot patches that will be applied during the build process.
|
@coderabbitai yes, this suggests that we should |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me pull up the exact code to draft a concrete guard. 🏁 Script executed: Length of output: 1970 🏁 Script executed: Length of output: 3129
A clean runtime abort covering both, placed in # Derive n from proxy env vars if unset, which runners.sh uses inside chroot
declare _n_auto_derived="no"
if [[ -z "${n}" && -n "${http_proxy:-${https_proxy:-${HTTP_PROXY:-${HTTPS_PROXY:-}}}}" ]]; then
n="$(echo "${http_proxy:-${https_proxy:-${HTTP_PROXY:-${HTTPS_PROXY:-}}}}" | sed -E 's|https?://([^/]+).*|\1|')"
_n_auto_derived="yes"
display_alert "Derived APT proxy address from proxy env vars" "${n}" "info"
fi
[[ -n "${n}" ]] && display_alert "Using custom apt proxy address" "n=${n}" "info"
# Guard: MANAGE_ACNG and n (APT proxy) must not conflict.
# When MANAGE_ACNG is a URL, it controls debootstrap but NOT chroot apt-get; n controls chroot apt-get.
# Having both set to different targets produces split behaviour that is almost never intended.
# When MANAGE_ACNG=yes, n overrides the acng address; auto-deriving it from a corporate proxy
# would silently redirect apt-cacher-ng traffic to the wrong host.
if [[ -n "${MANAGE_ACNG}" && "${MANAGE_ACNG}" != "no" && -n "${n}" ]]; then
if [[ "${MANAGE_ACNG}" =~ ^https?:// ]]; then
exit_with_error \
"MANAGE_ACNG is set to a URL ('${MANAGE_ACNG}') and APT proxy (n='${n}') is also set." \
"These apply to different operations (debootstrap vs chroot apt-get) and will conflict." \
"Unset one of them, or set APT_PROXY_ADDR explicitly if you need both."
elif [[ "${MANAGE_ACNG}" == "yes" && "${_n_auto_derived}" == "yes" ]]; then
exit_with_error \
"MANAGE_ACNG=yes but APT proxy was auto-derived from HTTP_PROXY/http_proxy ('${n}')." \
"This would silently replace the apt-cacher-ng address with your corporate proxy." \
"Set APT_PROXY_ADDR explicitly (e.g. APT_PROXY_ADDR=localhost:3142) to suppress this error."
fi
fiThe 🧠 Learnings used |
|
@jonaswood01 any thoughts on |
igorpecovnik
added
08
3 participants
Description
Have faced issues when building inside TI proxy for long time now, finally have time to PR to help fix/improve so I don't need proxy hacks any longer. Should also help others inside their own separate proxy, and shoud not affect others outside proxy.
How Has This Been Tested?
Built using
sk-am62bonvendoron same PC with Ubuntu 22.04Checklist:
Summary by CodeRabbit
Improvements