[WIP] Add optional cluster-wide proxy support for snc.sh#1237
[WIP] Add optional cluster-wide proxy support for snc.sh#1237praveenkumar wants to merge 1 commit into
Conversation
Allow provisioning behind an HTTP/HTTPS proxy via SNC_USE_PROXY, SNC_HTTP_PROXY, and SNC_HTTPS_PROXY. When enabled, patch install-config with proxy URLs and a noProxy list that includes the internal API (api-int.<cluster>.<baseDomain>) plus standard cluster-local entries.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThis PR adds HTTP/HTTPS proxy configuration support to the cluster installation pipeline. A new ChangesHTTP/HTTPS Proxy Configuration
🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 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. Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@snc-library.sh`:
- Around line 127-129: The three YQ invocations (${YQ} eval --inplace
".proxy.httpProxy = \"${SNC_HTTP_PROXY}\"" "${install_config}", etc.) are
executed under global xtrace so embedded proxy credentials can leak to CI logs;
wrap these updates in a temporary xtrace-suppressed section (turn off xtrace
before the three ${YQ} calls and re-enable it afterwards) or use a safe write
method that reads proxy values from a file or environment without echoing (e.g.,
pass masked values via stdin or a here-doc) so that SNC_HTTP_PROXY,
SNC_HTTPS_PROXY and no_proxy are never printed; ensure you modify the script
around the ${YQ} eval lines (reference the ${YQ} eval, SNC_HTTP_PROXY,
SNC_HTTPS_PROXY, no_proxy and install_config symbols) to disable tracing only
for the sensitive operations and then restore the original xtrace state.
- Around line 118-120: The current check treats any non-"enabled" value as
silent disablement; update the SNC_USE_PROXY validation to explicitly allow only
"enabled" or "disabled": if SNC_USE_PROXY=="enabled" enable proxy logic, if
"disabled" return 0, otherwise print a clear error to stderr mentioning the
invalid SNC_USE_PROXY value and exit non‑zero (or return non‑zero) so
misconfigurations fail fast; reference the SNC_USE_PROXY variable and the
existing enabling/return branch when implementing this change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5df13681-0bb6-4fee-83a3-3b3b0d546dac
📒 Files selected for processing (3)
README.mdsnc-library.shsnc.sh
| if [[ ${SNC_USE_PROXY} != "enabled" ]]; then | ||
| return 0 | ||
| fi |
There was a problem hiding this comment.
Fail fast on invalid SNC_USE_PROXY values.
Line 118 silently disables proxy for any unexpected value (e.g., true, yes, typo), which makes misconfiguration hard to detect.
Proposed fix
function configure_install_config_proxy() {
local install_config=$1
- if [[ ${SNC_USE_PROXY} != "enabled" ]]; then
- return 0
- fi
+ case "${SNC_USE_PROXY}" in
+ enabled) ;;
+ disabled) return 0 ;;
+ *)
+ echo "Invalid SNC_USE_PROXY='${SNC_USE_PROXY}'. Expected 'enabled' or 'disabled'."
+ exit 1
+ ;;
+ esac📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [[ ${SNC_USE_PROXY} != "enabled" ]]; then | |
| return 0 | |
| fi | |
| function configure_install_config_proxy() { | |
| local install_config=$1 | |
| case "${SNC_USE_PROXY}" in | |
| enabled) ;; | |
| disabled) return 0 ;; | |
| *) | |
| echo "Invalid SNC_USE_PROXY='${SNC_USE_PROXY}'. Expected 'enabled' or 'disabled'." | |
| exit 1 | |
| ;; | |
| esac |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@snc-library.sh` around lines 118 - 120, The current check treats any
non-"enabled" value as silent disablement; update the SNC_USE_PROXY validation
to explicitly allow only "enabled" or "disabled": if SNC_USE_PROXY=="enabled"
enable proxy logic, if "disabled" return 0, otherwise print a clear error to
stderr mentioning the invalid SNC_USE_PROXY value and exit non‑zero (or return
non‑zero) so misconfigurations fail fast; reference the SNC_USE_PROXY variable
and the existing enabling/return branch when implementing this change.
| ${YQ} eval --inplace ".proxy.httpProxy = \"${SNC_HTTP_PROXY}\"" "${install_config}" | ||
| ${YQ} eval --inplace ".proxy.httpsProxy = \"${SNC_HTTPS_PROXY}\"" "${install_config}" | ||
| ${YQ} eval --inplace ".proxy.noProxy = \"${no_proxy}\"" "${install_config}" |
There was a problem hiding this comment.
Proxy credentials are exposed in logs.
Line 127-Line 129 run under global xtrace (set -x), so proxy URLs (including embedded credentials) will be printed to CI logs.
Proposed fix
function configure_install_config_proxy() {
local install_config=$1
@@
local api_int="api-int.${SNC_PRODUCT_NAME}.${BASE_DOMAIN}"
local no_proxy="localhost,.cluster.local,.svc,127.0.0.1,${api_int}"
+ set +x
${YQ} eval --inplace ".proxy.httpProxy = \"${SNC_HTTP_PROXY}\"" "${install_config}"
${YQ} eval --inplace ".proxy.httpsProxy = \"${SNC_HTTPS_PROXY}\"" "${install_config}"
${YQ} eval --inplace ".proxy.noProxy = \"${no_proxy}\"" "${install_config}"
+ set -x
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@snc-library.sh` around lines 127 - 129, The three YQ invocations (${YQ} eval
--inplace ".proxy.httpProxy = \"${SNC_HTTP_PROXY}\"" "${install_config}", etc.)
are executed under global xtrace so embedded proxy credentials can leak to CI
logs; wrap these updates in a temporary xtrace-suppressed section (turn off
xtrace before the three ${YQ} calls and re-enable it afterwards) or use a safe
write method that reads proxy values from a file or environment without echoing
(e.g., pass masked values via stdin or a here-doc) so that SNC_HTTP_PROXY,
SNC_HTTPS_PROXY and no_proxy are never printed; ensure you modify the script
around the ${YQ} eval lines (reference the ${YQ} eval, SNC_HTTP_PROXY,
SNC_HTTPS_PROXY, no_proxy and install_config symbols) to disable tracing only
for the sensitive operations and then restore the original xtrace state.
|
@praveenkumar: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
Allow provisioning behind an HTTP/HTTPS proxy via SNC_USE_PROXY, SNC_HTTP_PROXY, and SNC_HTTPS_PROXY. When enabled, patch install-config with proxy URLs and a noProxy list that includes the internal API (api-int..) plus standard cluster-local entries.
Summary by CodeRabbit