OCPCLOUD-2208: Add cluster-wide proxy support for CAPI controllers#517
OCPCLOUD-2208: Add cluster-wide proxy support for CAPI controllers#517lunarwhite wants to merge 3 commits intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
Skipping CI for Draft Pull Request. |
|
@lunarwhite: This pull request references OCPCLOUD-2208 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this: Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis change adds cluster-wide proxy configuration support to the installer and revision controllers. Controllers now read proxy settings from the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[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 |
| // Read cluster-wide proxy configuration | ||
| proxy, err := util.GetProxy(ctx, r.Client) | ||
| if err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("fetching proxy: %w", err) | ||
| } | ||
|
|
||
| proxyEnvVars := util.ProxyEnvVars(proxy) | ||
|
|
||
| if len(proxyEnvVars) > 0 { | ||
| log.Info("Injecting proxy configuration into provider deployments", | ||
| "httpProxy", proxy.Status.HTTPProxy, "httpsProxy", proxy.Status.HTTPSProxy, "noProxy", proxy.Status.NoProxy) | ||
| } | ||
|
|
There was a problem hiding this comment.
I haven't given a careful read but
We would probably want to do this before starting these controllers, so before setting up the controller-runtime manager.
Then inject that down to each controller to use it.
There was a problem hiding this comment.
Thanks for your review @damdo. Are you suggesting 'read once, inject at startup'? I thought that, in this case, proxy changes (can change at runtime) would remain invisible until the operator restarts. Or am I overlooking something?
I noticed #507 was landed these days so I remove the touch to capiinstaller entirely
lunarwhite
force-pushed
the
proxy
branch
4 times, most recently
from
7974ca8 to
84794ac
Compare
lunarwhite
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
pkg/util/proxy.go (2)
47-75: Verify uppercase-only proxy vars are sufficient.This helper emits only
HTTP_PROXY,HTTPS_PROXY, andNO_PROXY. If any controller image, wrapper script, or diagnostic tooling relies on the conventional lowercase aliases, proxy support will still be partial.Possible change
if proxy.Status.HTTPProxy != "" { - envVars = append(envVars, corev1.EnvVar{ - Name: "HTTP_PROXY", - Value: proxy.Status.HTTPProxy, - }) + envVars = append(envVars, + corev1.EnvVar{Name: "HTTP_PROXY", Value: proxy.Status.HTTPProxy}, + corev1.EnvVar{Name: "http_proxy", Value: proxy.Status.HTTPProxy}, + ) } if proxy.Status.HTTPSProxy != "" { - envVars = append(envVars, corev1.EnvVar{ - Name: "HTTPS_PROXY", - Value: proxy.Status.HTTPSProxy, - }) + envVars = append(envVars, + corev1.EnvVar{Name: "HTTPS_PROXY", Value: proxy.Status.HTTPSProxy}, + corev1.EnvVar{Name: "https_proxy", Value: proxy.Status.HTTPSProxy}, + ) } if proxy.Status.NoProxy != "" { - envVars = append(envVars, corev1.EnvVar{ - Name: "NO_PROXY", - Value: proxy.Status.NoProxy, - }) + envVars = append(envVars, + corev1.EnvVar{Name: "NO_PROXY", Value: proxy.Status.NoProxy}, + corev1.EnvVar{Name: "no_proxy", Value: proxy.Status.NoProxy}, + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/proxy.go` around lines 47 - 75, The ProxyEnvVars function currently emits only uppercase HTTP_PROXY/HTTPS_PROXY/NO_PROXY which can break tools expecting lowercase variants; update ProxyEnvVars to also append the lowercase aliases "http_proxy", "https_proxy", and "no_proxy" alongside their uppercase counterparts (ensuring you don't create duplicates when values are empty) so both variants are present for any consumer that expects lowercase; locate ProxyEnvVars in pkg/util/proxy.go and add the additional corev1.EnvVar entries for each proxy field (Status.HTTPProxy, Status.HTTPSProxy, Status.NoProxy).
82-160: Keep one proxy-env merge implementation.
InjectProxyEnvVarsIntoUnstructuredandInjectProxyEnvVarsIntoDeploymenteach reimplement the same de-dupe/append rules. Because one path feeds revisionContentIDgeneration and the other mutates typed Deployments, future drift here can turn into false content-ID mismatches or unnecessary rollouts. Consider extracting a shared merge routine and adapting both representations to it.Also applies to: 165-189
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/proxy.go` around lines 82 - 160, The two implementations (InjectProxyEnvVarsIntoUnstructured and InjectProxyEnvVarsIntoDeployment) duplicate the same de-duplication and append rules which can cause drift; extract a single shared merge routine (e.g., MergeEnvVars or mergeProxyEnvVars) that takes an existing env list and desired []corev1.EnvVar and returns a deterministic merged []map[string]interface{} or []corev1.EnvVar (preserving order and skipping existing names). Replace the logic inside injectEnvVarsIntoContainer (and the Deployment mutator) to call this shared function, and have injectEnvVarsAtPath and InjectProxyEnvVarsIntoDeployment adapt typed vs unstructured representations to the same merge routine so both paths use identical merging behavior.pkg/controllers/installer/installer_controller_test.go (1)
601-658: Add a proxy-change reconciliation spec.These cases only cover the proxy state at revision creation time. A spec that installs successfully first, then flips
configv1.Proxy/statusfrom empty→set (and ideally set→empty) would exercise the new Proxy watch path and catch regressions where existing Deployments do not roll to the updated env set.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/installer/installer_controller_test.go` around lines 601 - 658, Add a new test It in installer_controller_test.go that verifies proxy-change reconciliation by (1) creating an initial revision without proxy (use addRevision, makeDeploymentAvailable, waitForRevision), (2) updating the cluster Proxy status to a non-empty value (fetch configv1.Proxy via cl.Get, set Proxy.Status fields, cl.Status().Update), then wait for the controller to roll the Deployment and assert the pod template now contains HTTP_PROXY/HTTPS_PROXY/NO_PROXY env vars (read Deployment via cl.Get and check deploy.Spec.Template.Spec.Containers[0].Env similar to the existing test); also include the reverse case by clearing Proxy.Status (set empty/no-proxy and cl.Status().Update) and asserting the env vars are removed after the rollout. Use the same helper functions (addRevision, makeDeploymentAvailable, waitForRevision) and util.ProxyEnvVars/revisiongenerator.WithProxyConfig where needed to construct expected values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controllers/capiinstaller/capi_installer_controller.go`:
- Around line 130-133: The Info log in capi_installer_controller.go that calls
log.Info("Injecting proxy configuration into provider deployments", "httpProxy",
proxy.Status.HTTPProxy, ...) must not log raw proxy URLs as they may contain
credentials; update the code that logs proxy.Status.HTTPProxy,
proxy.Status.HTTPSProxy and proxy.Status.NoProxy (and the identical logging
sites in pkg/controllers/revision/revision_controller.go and
pkg/controllers/installer/installer_controller.go) to sanitize/redact any URL
userinfo before logging (e.g., parse with net/url, clear u.User or replace with
"<redacted>" and log only host/path or a presence indicator), handle empty/nil
values safely, and keep the same log keys so callers can find the logging call
(look for the log.Info invocation and the proxy.Status.* references to change).
In `@pkg/revisiongenerator/revision_test.go`:
- Around line 435-436: Remove the trailing whitespace/newline noise at the end
of the test file so the file ends with a single newline and no extra blank lines
or spaces; open pkg/revisiongenerator/revision_test.go, delete the extra
whitespace on the last lines (around the final closing brace) and ensure the
file terminates with exactly one newline (run gofmt/gofmt -w or trim trailing
whitespace in your editor before committing).
In `@pkg/util/proxy_test.go`:
- Around line 97-107: The file contains unformatted Go code around the test
table declaration (the for _, tc := range []struct { ... } block with fields
like wantEnvCount, wantContainers, wantInitEnvCount, wantInitContainers,
wantSkipInspect, wantEnvValues), causing golangci-lint to fail; fix by running
gofmt (or gofmt -w) / goimports on pkg/util/proxy_test.go to reformat the file
and commit the changes so the test table and surrounding code comply with gofmt
formatting rules.
---
Nitpick comments:
In `@pkg/controllers/installer/installer_controller_test.go`:
- Around line 601-658: Add a new test It in installer_controller_test.go that
verifies proxy-change reconciliation by (1) creating an initial revision without
proxy (use addRevision, makeDeploymentAvailable, waitForRevision), (2) updating
the cluster Proxy status to a non-empty value (fetch configv1.Proxy via cl.Get,
set Proxy.Status fields, cl.Status().Update), then wait for the controller to
roll the Deployment and assert the pod template now contains
HTTP_PROXY/HTTPS_PROXY/NO_PROXY env vars (read Deployment via cl.Get and check
deploy.Spec.Template.Spec.Containers[0].Env similar to the existing test); also
include the reverse case by clearing Proxy.Status (set empty/no-proxy and
cl.Status().Update) and asserting the env vars are removed after the rollout.
Use the same helper functions (addRevision, makeDeploymentAvailable,
waitForRevision) and util.ProxyEnvVars/revisiongenerator.WithProxyConfig where
needed to construct expected values.
In `@pkg/util/proxy.go`:
- Around line 47-75: The ProxyEnvVars function currently emits only uppercase
HTTP_PROXY/HTTPS_PROXY/NO_PROXY which can break tools expecting lowercase
variants; update ProxyEnvVars to also append the lowercase aliases "http_proxy",
"https_proxy", and "no_proxy" alongside their uppercase counterparts (ensuring
you don't create duplicates when values are empty) so both variants are present
for any consumer that expects lowercase; locate ProxyEnvVars in
pkg/util/proxy.go and add the additional corev1.EnvVar entries for each proxy
field (Status.HTTPProxy, Status.HTTPSProxy, Status.NoProxy).
- Around line 82-160: The two implementations
(InjectProxyEnvVarsIntoUnstructured and InjectProxyEnvVarsIntoDeployment)
duplicate the same de-duplication and append rules which can cause drift;
extract a single shared merge routine (e.g., MergeEnvVars or mergeProxyEnvVars)
that takes an existing env list and desired []corev1.EnvVar and returns a
deterministic merged []map[string]interface{} or []corev1.EnvVar (preserving
order and skipping existing names). Replace the logic inside
injectEnvVarsIntoContainer (and the Deployment mutator) to call this shared
function, and have injectEnvVarsAtPath and InjectProxyEnvVarsIntoDeployment
adapt typed vs unstructured representations to the same merge routine so both
paths use identical merging behavior.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 497d8be5-5c36-4bf9-9e54-ad77897accdf
📒 Files selected for processing (13)
pkg/controllers/capiinstaller/capi_installer_controller.gopkg/controllers/installer/helpers_test.gopkg/controllers/installer/installer_controller.gopkg/controllers/installer/installer_controller_test.gopkg/controllers/installer/revision_reconciler.gopkg/controllers/revision/helpers_test.gopkg/controllers/revision/revision_controller.gopkg/controllers/revision/revision_controller_test.gopkg/controllers/revision/suite_test.gopkg/revisiongenerator/revision.gopkg/revisiongenerator/revision_test.gopkg/util/proxy.gopkg/util/proxy_test.go
lunarwhite
force-pushed
the
proxy
branch
2 times, most recently
from
acb07cc to
9dce7ec
Compare
lunarwhite
force-pushed
the
proxy
branch
2 times, most recently
from
2eb145e to
8100100
Compare
CAPI provider workloads need HTTP_PROXY/HTTPS_PROXY/NO_PROXY env vars when the cluster is behind a proxy, otherwise they cannot reach external endpoints. Add pkg/util/proxy.go that reads the cluster-wide Proxy CR status and injects proxy env vars into Deployment containers.
Proxy env vars must be part of the rendered revision so that content ID reflects proxy changes and triggers a new rollout. Add a RevisionRenderOption that injects proxy env vars into Deployment containers during rendering.
Controllers need to read the Proxy CR and pass it to the revision generator so provider workloads get proxy env vars at deploy time. Both controllers now fetch the Proxy CR and forward env vars via WithProxyConfig. The revision controller watches Proxy to re-reconcile when proxy config changes.
|
@lunarwhite: This pull request references OCPCLOUD-2208 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
lunarwhite
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/ok-to-test |
openshift-ci
bot
added
the
ok-to-test
|
@lunarwhite: all tests passed! 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. |
|
/assign @mdbooth |
4 participants
Summary
When an OpenShift cluster is configured with a cluster-wide proxy (
oc get proxy cluster), CAPI provider Deployments need the proxy environment variables to reach external endpoints.This PR reads the proxy configuration from
proxy.Statusand injects env vars during manifest rendering:pkg/util/proxy.goto read the Proxy CR status and inject env vars into Deployment containers.WithProxyConfigrender option to the revision generator for proxy injection during manifest rendering.configv1.Proxyand triggers new revisions on proxy changes.What have been tested
capa-controller-managerandcapi-controller-managerindeed have proxy ENV being injected, and can dynamically update the value based onProxy.Status.