fix: preserve default proxy settings#1656
Open
tolusha wants to merge 5 commits into
Open
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
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tolusha 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 |
📝 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: Read routing proxy settings
GlobalConfig-->>Factory: HttpProxy, HttpsProxy 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(cert only)
Factory->>Transport: Use cached proxyFunc
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 |
Contributor
Author
|
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>
Contributor
Author
|
/che-ai-assistant ok-pr-review |
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
Comment on lines
+81
to
+100
| t.Run("rebuilds client when proxy changes", func(t *testing.T) { | ||
| factory := newTestFactory(t) | ||
| routingConfig1 := routingConfigWithProxy("http://proxy:80", "", "") | ||
| routingConfig2 := routingConfigWithProxy("http://proxy:90", "", "") | ||
|
|
||
| client1 := factory.GetHttpClient(context.Background(), routingConfig1) | ||
|
|
||
| assert.NotNil(t, client1.Transport.(*http.Transport).Proxy) | ||
|
|
||
| client2 := factory.GetHttpClient(context.Background(), routingConfig2) | ||
|
|
||
| assert.NotNil(t, client2.Transport.(*http.Transport).Proxy) | ||
| assert.NotSame(t, client1, client2) | ||
|
|
||
| client3 := factory.GetHttpClient(context.Background(), nil) | ||
|
|
||
| assert.NotSame(t, client2, client3) | ||
|
|
||
| // Default proxy config is not nil as well | ||
| assert.NotNil(t, client3.Transport.(*http.Transport).Proxy) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the proxy test exercise proxyFunc, not routingConfig.ProxyConfig.
After the production change, createHttpClient ignores routingConfig.ProxyConfig and only reads factory.proxyFunc; newTestFactory leaves that field nil. The Proxy != nil assertions can pass just because http.DefaultTransport.Clone() keeps the default proxy function, so this test can miss the proxy-reset regression. Seed proxyFunc in the test factory or assert the actual proxy URL returned by the transport.
Also applies to: 217-225
🤖 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 `@controllers/workspace/http_test.go` around lines 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.
Comment on lines
+166
to
+173
| client := factory.GetHttpClient(context.Background(), routingConfig) | ||
|
|
||
| require.NotNil(t, client) | ||
|
|
||
| assert.NotNil(t, client) | ||
| if routingConfig != nil { | ||
| assert.NotNil(t, client.Transport.(*http.Transport).Proxy) | ||
| tlsConfig := client.Transport.(*http.Transport).TLSClientConfig | ||
| require.NotNil(t, tlsConfig) | ||
| if routingConfig != nil && routingConfig.TLSCertificateConfigmapRef != nil { | ||
| assert.NotNil(t, tlsConfig.RootCAs) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect goroutine-local assertions in the concurrent test.
# Expected: no require/assert calls directly inside spawned goroutines after the fix.
sed -n '159,178p' controllers/workspace/http_test.go
rg -n -C4 'go func|require\.|assert\.' controllers/workspace/http_test.goRepository: devfile/devworkspace-operator
Length of output: 4469
Move require assertions out of worker goroutines.
require.NotNil calls FailNow, which should run on the test goroutine. Collect failures from workers through a channel and assert after wg.Wait() so failures are reported reliably.
🤖 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 `@controllers/workspace/http_test.go` around lines 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.
Comment on lines
188
to
+209
| // getProxyFunc returns a proxy function based on the proxy settings in routingConfig. | ||
| // Returns nil if no proxy is configured; a nil proxy func causes the HTTP transport to | ||
| // use the default proxy settings from environment variables. | ||
| func (h *DefaultHttpClientsFactory) getProxyFunc(routingConfig *controller.RoutingConfig) func(*http.Request) (*url.URL, error) { | ||
| if routingConfig == nil || routingConfig.ProxyConfig == nil { | ||
| return nil | ||
| } | ||
| func getProxyFunc() func(*http.Request) (*url.URL, error) { | ||
| globalConfig := config.GetGlobalConfig() | ||
|
|
||
| proxyConfig := httpproxy.Config{} | ||
| if routingConfig.ProxyConfig.HttpProxy != nil { | ||
| proxyConfig.HTTPProxy = *routingConfig.ProxyConfig.HttpProxy | ||
| } | ||
| if routingConfig.ProxyConfig.HttpsProxy != nil { | ||
| proxyConfig.HTTPSProxy = *routingConfig.ProxyConfig.HttpsProxy | ||
| } | ||
| if routingConfig.ProxyConfig.NoProxy != nil { | ||
| proxyConfig.NoProxy = *routingConfig.ProxyConfig.NoProxy | ||
| } | ||
| if globalConfig.Routing != nil && globalConfig.Routing.ProxyConfig != nil { | ||
| proxyConf := httpproxy.Config{} | ||
| if globalConfig.Routing.ProxyConfig.HttpProxy != nil { | ||
| proxyConf.HTTPProxy = *globalConfig.Routing.ProxyConfig.HttpProxy | ||
| } | ||
| if globalConfig.Routing.ProxyConfig.HttpsProxy != nil { | ||
| proxyConf.HTTPSProxy = *globalConfig.Routing.ProxyConfig.HttpsProxy | ||
| } | ||
| if globalConfig.Routing.ProxyConfig.NoProxy != nil { | ||
| proxyConf.NoProxy = *globalConfig.Routing.ProxyConfig.NoProxy | ||
| } | ||
|
|
||
| return func(req *http.Request) (*url.URL, error) { | ||
| return proxyConfig.ProxyFunc()(req.URL) | ||
| return func(req *http.Request) (*url.URL, error) { | ||
| return proxyConf.ProxyFunc()(req.URL) | ||
| } | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the nil/non-nil proxy function contract and proxy field optionality.
# Expected: getProxyFunc only returns non-nil when an explicit HTTP/HTTPS proxy override exists.
sed -n '77,96p;139,209p' controllers/workspace/http.go
rg -n -C3 'type Proxy struct|HttpProxy|HttpsProxy|NoProxy' apis/controller pkg/config controllers/workspaceRepository: devfile/devworkspace-operator
Length of output: 19601
Preserve defaults when ProxyConfig is present but no proxy field is set.
When ProxyConfig exists but all proxy fields (HttpProxy and HttpsProxy) are nil, the function returns a non-nil proxy closure built from an empty httpproxy.Config{}. This still overrides the cloned DefaultTransport.Proxy (lines 80–83 and 141–143), resetting the default proxy path that the PR aims to preserve. Only return a non-nil proxy function when at least one explicit HTTP or HTTPS proxy is configured; treat empty strings as explicit "disable" overrides but allow nil fields to preserve defaults.
Minimal guard
if globalConfig.Routing != nil && globalConfig.Routing.ProxyConfig != nil {
+ proxyConfig := globalConfig.Routing.ProxyConfig
+ if proxyConfig.HttpProxy == nil && proxyConfig.HttpsProxy == nil {
+ return nil
+ }
+
proxyConf := httpproxy.Config{}
- if globalConfig.Routing.ProxyConfig.HttpProxy != nil {
- proxyConf.HTTPProxy = *globalConfig.Routing.ProxyConfig.HttpProxy
+ if proxyConfig.HttpProxy != nil {
+ proxyConf.HTTPProxy = *proxyConfig.HttpProxy
}
- if globalConfig.Routing.ProxyConfig.HttpsProxy != nil {
- proxyConf.HTTPSProxy = *globalConfig.Routing.ProxyConfig.HttpsProxy
+ if proxyConfig.HttpsProxy != nil {
+ proxyConf.HTTPSProxy = *proxyConfig.HttpsProxy
}
- if globalConfig.Routing.ProxyConfig.NoProxy != nil {
- proxyConf.NoProxy = *globalConfig.Routing.ProxyConfig.NoProxy
+ if proxyConfig.NoProxy != nil {
+ proxyConf.NoProxy = *proxyConfig.NoProxy
}🤖 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 `@controllers/workspace/http.go` around lines 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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