fix: preserve default proxy settings#1656
Conversation
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
tolusha
requested review from
akurinnoy,
btjd,
dkwon17,
ibuziuk and
rohanKanojia
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughHTTP client factory is refactored to decouple proxy configuration from per-workspace routing. Proxy behavior is now computed once globally during setup and stored on the factory; both regular and health-check clients use this single stored proxy function. The ChangesProxy decoupling from routing config
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Sequence DiagramsequenceDiagram
participant Init as Controller Init
participant Factory as HttpClientsFactory
participant GlobalConfig as Global Config
participant Transport as HTTP Transport
Init->>Factory: SetupHttpClientsFactory()
Factory->>GlobalConfig: getProxyFunc reads global routing config
GlobalConfig-->>Factory: HttpProxy, HttpsProxy, NoProxy values
Factory->>Factory: Compute proxyFunc from global config
alt proxyFunc is non-nil
Factory->>Transport: Set Proxy field with proxyFunc
else proxyFunc is nil
Factory->>Transport: Leave Proxy field as default
end
Factory->>Factory: Create and cache healthCheckHttpClient
Note over Factory: Later: Client requests
participant Client as Caller
Client->>Factory: GetHttpClient(routingConfig)
Factory->>Factory: createHttpClient uses factory.proxyFunc
Factory->>Transport: Conditionally assign Proxy
Transport-->>Client: Return configured client
Possibly related PRs
Suggested labels
Suggested reviewers
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 docstrings
🧪 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 |
|
Hi! I'm che-ai-assistant — I help with your pull requests. Available commands:
|
![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 `@controllers/workspace/http.go`:
- Around line 232-234: The condition checking ProxyConfig.HttpProxy and
ProxyConfig.HttpsProxy unconditionally dereferences these pointer fields without
first verifying they are not nil. Since both HttpProxy and HttpsProxy are
*string pointer fields with omitempty tag, they can legitimately be nil. Add
explicit nil-checks for both pointers before dereferencing them in the
condition, ensuring each pointer is not nil before accessing its value. Follow
the same pattern used in the existing code below at lines 237 and 240 which
already properly nil-check before dereferencing.
- Around line 232-234: In the proxy configuration validation check within the
ProxyConfig initialization, change the logical operator from `||` (OR) to `&&`
(AND) in the condition that evaluates *routingConfig.ProxyConfig.HttpProxy and
*routingConfig.ProxyConfig.HttpsProxy. This ensures the function only returns
nil when both proxy fields are empty, allowing configurations with at least one
proxy type set to be processed correctly instead of being skipped.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32ca01cb-c20c-44cf-be9d-63106cd185a6
📒 Files selected for processing (1)
controllers/workspace/http.go
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
|
/che-ai-assistant ok-pr-review Review is complete. Please check the review comments below. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@controllers/workspace/http_test.go`:
- Around line 81-100: The test "rebuilds client when proxy changes" is not
actually exercising the proxyFunc field that createHttpClient now uses in
production, instead it relies on routingConfig.ProxyConfig which is ignored. To
fix this, either seed the proxyFunc field in newTestFactory with a test proxy
function that can be verified in assertions, or replace the assert.NotNil checks
on client.Transport.(*http.Transport).Proxy with assertions that verify the
actual proxy URL being returned by the transport matches what was configured in
the routingConfig passed to GetHttpClient.
- Around line 166-173: The require.NotNil assertions on the client and tlsConfig
variables are being called from within a worker goroutine, which is unsafe
because require.NotNil calls FailNow() and must run on the test goroutine only.
Create a channel to collect validation results or errors from the worker
goroutine, then move these require.NotNil assertions outside the goroutine to
execute after wg.Wait() completes so test failures are reported reliably on the
main test goroutine.
In `@controllers/workspace/http.go`:
- Around line 188-209: The getProxyFunc function currently returns a proxy
closure built from an empty httpproxy.Config when ProxyConfig exists but both
HttpProxy and HttpsProxy fields are nil, which overrides the default proxy
behavior. Add a guard condition before returning the proxy function to check
that at least one of HttpProxy or HttpsProxy is non-nil; only return the proxy
closure when at least one explicit proxy field is configured, otherwise return
nil to preserve the default proxy 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e20ed13a-ee18-4abc-9bfc-8c380ebdbcb2
📒 Files selected for processing (3)
controllers/workspace/http.gocontrollers/workspace/http_test.gocontrollers/workspace/status.go
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
tolusha
left a comment
tolusha
left a comment
There was a problem hiding this comment.
Review Summary
This PR successfully fixes the proxy configuration bug on air-gapped clusters and significantly simplifies the HTTP client management code. The architectural decision to move from per-workspace to per-operator-lifecycle proxy configuration is sound.
Key Findings
Fix require.NotNil in concurrent test ()
The concurrent test uses require.NotNil(t, client) inside goroutines. require.NotNil calls t.FailNow() which invokes runtime.Goexit(). From a non-test goroutine, this doesn't propagate correctly to the test runner - it silently kills the goroutine without failing the test, potentially masking real failures. Should use assert.NotNil instead.
Document runtime proxy configuration behavior ()
The proxy function is computed once at operator startup. If an admin updates the DevWorkspaceOperatorConfig CR's proxy settings at runtime, the change won't take effect until the operator pod is restarted. Consider adding a code comment or documenting this in the operator configuration docs.
Clarify that routingConfig proxy fields are ignored ()
GetHttpClient still accepts *controller.RoutingConfig but only uses it for TLS certs - the ProxyConfig field is now silently ignored. Consider adding a comment clarifying that proxy config from routingConfig is intentionally ignored.
Add direct unit tests for getProxyFunc()
The getProxyFunc() function has multiple code paths (nil config, empty proxy URLs, HTTP-only, HTTPS-only, NoProxy-only) that aren't tested directly. Consider adding tests using config.SetGlobalConfigForTesting().
Verify NoProxy-only configuration ()
If only NoProxy is set (both HttpProxy and HttpsProxy are empty), getProxyFunc() returns nil, discarding the NoProxy setting. This is likely intentional but worth verifying against documented behavior.
Positive Observations
Excellent work on this fix:
- Significant simplification: Removing ~40 lines of complex proxy-tracking logic
- Correct nil-proxy semantics: Properly preserves Go's default
http.ProxyFromEnvironmentbehavior - Performance improvement: Health check client uses only read locks, eliminating contention
- Well-structured tests: Concurrency test exercises realistic cert-based variations
The architectural decision to move proxy configuration from per-workspace to per-operator-lifecycle is sound for this problem domain.
Generated by Claude Code PR Review
tolusha
left a comment
tolusha
left a comment
There was a problem hiding this comment.
Review Summary
This PR successfully fixes the proxy configuration bug on air-gapped clusters and significantly simplifies the HTTP client management code. The architectural decision to move from per-workspace to per-operator-lifecycle proxy configuration is sound.
Key Findings
Fix require.NotNil in concurrent test (http_test.go:163-164)
The concurrent test uses require.NotNil(t, client) inside goroutines. require.NotNil calls t.FailNow() which invokes runtime.Goexit(). From a non-test goroutine, this doesn't propagate correctly to the test runner - it silently kills the goroutine without failing the test, potentially masking real failures. Should use assert.NotNil instead.
Document runtime proxy configuration behavior (http.go:73-88)
The proxy function is computed once at operator startup. If an admin updates the DevWorkspaceOperatorConfig CR's proxy settings at runtime, the change won't take effect until the operator pod is restarted. Consider adding a code comment or documenting this in the operator configuration docs.
Clarify that routingConfig proxy fields are ignored (http.go:112-129)
GetHttpClient still accepts *controller.RoutingConfig but only uses it for TLS certs - the ProxyConfig field is now silently ignored. Consider adding a comment clarifying that proxy config from routingConfig is intentionally ignored.
Add direct unit tests for getProxyFunc()
The getProxyFunc() function has multiple code paths (nil config, empty proxy URLs, HTTP-only, HTTPS-only, NoProxy-only) that aren't tested directly. Consider adding tests using config.SetGlobalConfigForTesting().
Verify NoProxy-only configuration (http.go:202-204)
If only NoProxy is set (both HttpProxy and HttpsProxy are empty), getProxyFunc() returns nil, discarding the NoProxy setting. This is likely intentional but worth verifying against documented behavior.
Positive Observations
Excellent work on this fix:
- Significant simplification: Removing ~40 lines of complex proxy-tracking logic
- Correct nil-proxy semantics: Properly preserves Go's default http.ProxyFromEnvironment behavior
- Performance improvement: Health check client uses only read locks, eliminating contention
- Well-structured tests: Concurrency test exercises realistic cert-based variations
The architectural decision to move proxy configuration from per-workspace to per-operator-lifecycle is sound for this problem domain.
Generated by Claude Code PR Review
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dkwon17, tolusha The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
LGTM, thanks 👍 |
3 participants
What does this PR do?
Preserve default proxy settings
What issues does this PR fix or reference?
fix #1650
Is it tested? How?
Manually on disconnected cluster
PR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-pathto trigger)v8-devworkspace-operator-e2e: DevWorkspace e2e testv8-che-happy-path: Happy path for verification integration with CheSummary by CodeRabbit
Bug Fixes
Tests