Conversation
…e http client 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 |
📝 WalkthroughWalkthroughReplaces the global ChangesHttpClientsFactory Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🧹 Nitpick comments (2)
controllers/workspace/http.go (1)
123-134: 💤 Low valueConsider setting explicit TLS MinVersion for defense-in-depth.
While Go defaults to TLS 1.2 for clients, explicitly setting
MinVersion: tls.VersionTLS12(or TLS 1.3 if server compatibility permits) documents the security intent and guards against future default changes.Suggested improvement
transport.TLSClientConfig = &tls.Config{ RootCAs: h.getCaCertPool(certsCM), + MinVersion: tls.VersionTLS12, }🤖 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 123 - 134, In the createHttpClient method of the DefaultHttpClientsFactory struct, add an explicit MinVersion field to the TLSClientConfig initialization. Set it to tls.VersionTLS12 (or tls.VersionTLS13 if server compatibility permits) within the tls.Config struct that is assigned to transport.TLSClientConfig. This documents security intent and ensures the code is protected against future changes to Go's default TLS version behavior.Source: Linters/SAST tools
controllers/workspace/http_test.go (1)
158-179: ⚡ Quick winAssertions inside goroutines may not fail the test properly.
When
assert.NotNilfails inside a goroutine, it logs the failure but doesn't stop the test or causet.FailNow(). The test could pass despite assertion failures. Consider collecting results via a channel or usingrequirewith proper synchronization.Suggested fix using error collection
t.Run("safe for concurrent access", func(t *testing.T) { factory := newTestFactory(t) routingConfigs := []*controller.RoutingConfig{nil, routingConfigWithProxy("http://proxy:80", "", "")} var wg sync.WaitGroup + errCh := make(chan string, 50) for i := 0; i < 50; i++ { wg.Add(1) go func(idx int) { defer wg.Done() routingConfig := routingConfigs[idx%len(routingConfigs)] client := getClient(factory, routingConfig) - assert.NotNil(t, client) + if client == nil { + errCh <- "client was nil" + return + } if routingConfig != nil { - assert.NotNil(t, client.Transport.(*http.Transport).Proxy) + if client.Transport.(*http.Transport).Proxy == nil { + errCh <- "proxy was nil when routingConfig was set" + } } }(i) } wg.Wait() + close(errCh) + for err := range errCh { + t.Error(err) + } })🤖 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 158 - 179, The assertions inside the goroutine in the "safe for concurrent access" test using assert.NotNil will not properly fail the test if they fail, since assertions inside goroutines don't trigger test failure. Refactor the test to collect the results or errors from each goroutine into a thread-safe collection (such as a channel or a slice protected by a mutex), then perform the assertions after the wg.Wait() call completes. This ensures that assertion failures properly fail the test instead of being silently logged.
🤖 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/status.go`:
- Around line 213-221: The checkServerStatus function currently uses
healthCheckHttpClient.Get() which discards context, preventing proper request
cancellation when the reconciler is stopped. Update the checkServerStatus
function signature to accept a context parameter, then replace the
healthCheckHttpClient.Get(healthz.String()) call with http.NewRequestWithContext
combined with the Do method to preserve context propagation throughout the
request lifecycle. Additionally, update the call site in
devworkspace_controller.go to pass the appropriate context when invoking
checkServerStatus.
---
Nitpick comments:
In `@controllers/workspace/http_test.go`:
- Around line 158-179: The assertions inside the goroutine in the "safe for
concurrent access" test using assert.NotNil will not properly fail the test if
they fail, since assertions inside goroutines don't trigger test failure.
Refactor the test to collect the results or errors from each goroutine into a
thread-safe collection (such as a channel or a slice protected by a mutex), then
perform the assertions after the wg.Wait() call completes. This ensures that
assertion failures properly fail the test instead of being silently logged.
In `@controllers/workspace/http.go`:
- Around line 123-134: In the createHttpClient method of the
DefaultHttpClientsFactory struct, add an explicit MinVersion field to the
TLSClientConfig initialization. Set it to tls.VersionTLS12 (or tls.VersionTLS13
if server compatibility permits) within the tls.Config struct that is assigned
to transport.TLSClientConfig. This documents security intent and ensures the
code is protected against future changes to Go's default TLS version 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: 4b8feaaa-4724-43b4-8891-db8d5932a8b4
📒 Files selected for processing (4)
controllers/workspace/devworkspace_controller.gocontrollers/workspace/http.gocontrollers/workspace/http_test.gocontrollers/workspace/status.go
| healthCheckHttpClient := httpClientsFactory.GetHealthCheckHttpClient(workspace.Config.Routing) | ||
| resp, err := healthCheckHttpClient.Get(healthz.String()) | ||
| if err != nil { | ||
| return false, nil, &dwerrors.RetryError{Err: err, Message: "Failed to check server status", RequeueAfter: 1 * time.Second} | ||
| } | ||
|
|
||
| defer func() { | ||
| _ = resp.Body.Close() | ||
| }() |
There was a problem hiding this comment.
Use http.NewRequestWithContext to preserve context cancellation.
The noctx linter correctly identifies that client.Get() discards context, so request cancellation (e.g., when the reconciler is stopped) won't work. Using NewRequestWithContext + Do propagates cancellation properly.
Suggested fix
The function signature would need to accept a context parameter. If checkServerStatus doesn't currently have access to a context, this would require a minor signature change:
-func checkServerStatus(workspace *common.DevWorkspaceWithConfig) (ok bool, responseCode *int, err error) {
+func checkServerStatus(ctx context.Context, workspace *common.DevWorkspaceWithConfig) (ok bool, responseCode *int, err error) {
mainUrl := workspace.Status.MainUrl
if mainUrl == "" {
// Support DevWorkspaces that do not specify an mainUrl
return true, nil, nil
}
healthz, err := url.Parse(mainUrl)
if err != nil {
return false, nil, err
}
healthz.Path = path.Join(healthz.Path, "healthz")
healthCheckHttpClient := httpClientsFactory.GetHealthCheckHttpClient(workspace.Config.Routing)
- resp, err := healthCheckHttpClient.Get(healthz.String())
+ req, err := http.NewRequestWithContext(ctx, http.MethodGet, healthz.String(), nil)
+ if err != nil {
+ return false, nil, err
+ }
+ resp, err := healthCheckHttpClient.Do(req)
if err != nil {And update the call site in devworkspace_controller.go:
- serverReady, serverStatusCode, err := checkServerStatus(clusterWorkspace)
+ serverReady, serverStatusCode, err := checkServerStatus(ctx, clusterWorkspace)📝 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.
| healthCheckHttpClient := httpClientsFactory.GetHealthCheckHttpClient(workspace.Config.Routing) | |
| resp, err := healthCheckHttpClient.Get(healthz.String()) | |
| if err != nil { | |
| return false, nil, &dwerrors.RetryError{Err: err, Message: "Failed to check server status", RequeueAfter: 1 * time.Second} | |
| } | |
| defer func() { | |
| _ = resp.Body.Close() | |
| }() | |
| healthCheckHttpClient := httpClientsFactory.GetHealthCheckHttpClient(workspace.Config.Routing) | |
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, healthz.String(), nil) | |
| if err != nil { | |
| return false, nil, err | |
| } | |
| resp, err := healthCheckHttpClient.Do(req) | |
| if err != nil { | |
| return false, nil, &dwerrors.RetryError{Err: err, Message: "Failed to check server status", RequeueAfter: 1 * time.Second} | |
| } | |
| defer func() { | |
| _ = resp.Body.Close() | |
| }() |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 214-214: (*net/http.Client).Get must not be called. use (*net/http.Client).Do(*http.Request)
(noctx)
🤖 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/status.go` around lines 213 - 221, The
checkServerStatus function currently uses healthCheckHttpClient.Get() which
discards context, preventing proper request cancellation when the reconciler is
stopped. Update the checkServerStatus function signature to accept a context
parameter, then replace the healthCheckHttpClient.Get(healthz.String()) call
with http.NewRequestWithContext combined with the Do method to preserve context
propagation throughout the request lifecycle. Additionally, update the call site
in devworkspace_controller.go to pass the appropriate context when invoking
checkServerStatus.
Source: Linters/SAST tools
PR Review: Fix initialization HTTP clients with merged DevWorkspaceOperatorConfig📋 SummaryThis PR refactors HTTP client initialization in the DevWorkspace Operator to use a thread-safe factory pattern that properly merges DevWorkspaceOperatorConfig settings. The changes address an issue where HTTP clients were only initialized with global config, missing workspace-specific routing configurations. Key Changes:
Files Changed: 4 files, +447/-67 lines
🔍 Code Review✅ Strengths
|
1 participant
What does this PR do?
This PR changes the HTTP client initialization to use the merged DevWorkspaceOperatorConfig instead of only the global config.
What issues does this PR fix or reference?
eclipse-che/che#23870
eclipse-che/che-operator#2137
Is it tested? How?
Followed eclipse-che/che-operator#2137
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
Chores