23870 2

[tolusha]

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

• E2E tests pass (when PR is ready, comment /test v8-devworkspace-operator-e2e, v8-che-happy-path to trigger)
  • v8-devworkspace-operator-e2e: DevWorkspace e2e test
  • v8-che-happy-path: Happy path for verification integration with Che

Summary by CodeRabbit

• Bug Fixes

  • Improved HTTP health check handling with proper resource cleanup.

• Refactor

  • Redesigned HTTP client management for better concurrency safety and configurability.
  • Enhanced proxy settings and TLS certificate handling with dynamic reconfiguration support.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tolusha
Once this PR has been reviewed and has the lgtm label, please assign dkwon17 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: dfbf8066-ff92-4418-a4c7-5814dee5e3bc

📥 Commits

Reviewing files that changed from the base of the PR and between 7b6ff69 and 271ee31.

📒 Files selected for processing (5)

• controllers/workspace/devworkspace_controller.go
• controllers/workspace/http.go
• controllers/workspace/http_clients_holder_test.go
• controllers/workspace/http_test.go
• controllers/workspace/status.go

---

📝 Walkthrough

Walkthrough

The PR replaces package-level httpClient and healthCheckHttpClient globals with an HttpClientsHolder interface backed by DefaultHttpClientsHolder. The new implementation is concurrency-safe, supports dynamic rebuilding of clients when proxy settings or a TLS certificate ConfigMap's ResourceVersion changes, and centralizes both initialization and certificate loading. All callers in the reconciler and status check are updated accordingly.

Changes

HTTP Clients Holder Refactor

Layer / File(s)	Summary
HttpClientsHolder interface and DefaultHttpClientsHolder implementation controllers/workspace/http.go	Introduces HttpClientsHolder interface and DefaultHttpClientsHolder struct with mutex-protected GetHttpClient/GetHealthCheckHttpClient accessors. ConfigureHttpClients reads proxy and TLS cert ConfigMap references from RoutingConfig, determines via shouldRebuildClients whether proxy settings or the ConfigMap ResourceVersion changed, and rebuilds clients accordingly. The main client loads CA certificates from the ConfigMap; the health-check client uses InsecureSkipVerify. SetupHttpClients loads the system cert pool at startup. Removes former InjectCertificates, setupHttpClients, and package-level client globals.
Controller and status check wiring controllers/workspace/devworkspace_controller.go, controllers/workspace/status.go	Reconcile calls httpClientsHolder.ConfigureHttpClients(ctx, config.Routing) before workspace logging; flattenHelpers.HttpClient is sourced from httpClientsHolder.GetHttpClient(). SetupWithManager calls SetupHttpClients(...) and propagates its error. checkServerStatus uses httpClientsHolder.GetHealthCheckHttpClient() and adds a deferred resp.Body close.
Tests and test helper controllers/workspace/http_clients_holder_test.go, controllers/workspace/http_test.go	Adds TestHttpClientsHolder and SetupHttpClientsForTesting for wiring a mock client holder in integration tests. Adds nine ConfigureHttpClients unit tests covering nil/empty config initialization, proxy configuration on both transports, TLS cert loading from a ConfigMap, rebuild skip and trigger conditions (proxy change rebuilds both; cert ConfigMap change rebuilds only the main client), and graceful handling of invalid cert data and a missing ConfigMap.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐇 A hop through the certs and a skip past the proxy,
No more globals to guard — the holder's less foxy.
With mutex in paw and a ConfigMap key,
I rebuild my clients when changes I see.
The health-check skips TLS, root CAs stay true —
This bunny ships safer HTTP for you! 🔒

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 23870-2

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}