fix: Use merged DWOC config for HTTP client certificate initialization

[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

Release Notes

• Refactor
  • Improved HTTP client handling by replacing global, one-time setup with a routing-aware client holder that creates the appropriate HTTP and health-check clients as needed.
  • Updated health checks to use the routing-specific health-check client instead of a shared instance.
  • Enhanced TLS/certificate behavior for outbound HTTPS by loading certificates from the configured source when building clients, with error logging if certificate retrieval fails.

[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

[tolusha]

Hi! I'm che-ai-assistant — I help with your pull requests.

Available commands:

• /che-ai-assistant generate-che-doc — Generate a documentation PR based on this PR's changes
• /che-ai-assistant ok-pr-review — Run a comprehensive PR review (summary, code review, deep review, impact analysis)
• /che-ai-assistant help — Show this help message

[tolusha]

/che-ai-assistant ok-pr-review

Review is complete. Please check the review comments below.

[tolusha]

Code Review Summary

Completed comprehensive review (standard + deep + impact analysis) of this PR.

Does This PR Solve the Issue?

✅ Yes - enables DWO to use merged DevWorkspaceOperatorConfig for TLS certificate initialization.

Critical Issues Identified

1. Transient ConfigMap failure silently strips custom certificates - When readCertCM fails, the client rebuilds without custom certs, breaking TLS trust cluster-wide
2. TOCTOU race between shouldRebuildClients and setNewClients - Concurrent reconciliations can both rebuild clients
3. Shared HTTP client with per-workspace config - Last reconciled workspace's config wins globally

Positive Highlights

• Clean HttpClientsHolder interface design
• Excellent test coverage (8 test cases)
• Fixed pre-existing response body leak in status.go
• Smart rebuild optimization logic

Recommendations

See inline comments for detailed findings and suggested fixes.

Verdict: 🔄 Request Changes - Critical #1 directly undermines the PR's purpose. Once fixed, this is a solid, well-tested change.

---

Review generated by Claude Code using /ok-pr-review suite

[tolusha]

Critical: Transient ConfigMap failure silently strips custom certificates

When readCertCM fails (transient API error, RBAC issue), newCertsCM becomes nil. The shouldRebuildClients method then detects a "change" and rebuilds the HTTP client with getCaCertPool(nil), stripping all custom CA certificates.

A single transient Kubernetes API error will break TLS trust cluster-wide for all subsequent workspace operations.

Suggested fix: On readCertCM failure, return early without rebuilding clients. Only rebuild when we successfully read a ConfigMap with a different ResourceVersion.

if routingConfig.TLSCertificateConfigmapRef != nil {
    certsCM, err := h.readCertCM(ctx, routingConfig.TLSCertificateConfigmapRef)
    if err != nil {
        h.logger.Error(err, "Failed to read TLS certificate ConfigMap")
        // Preserve existing HTTP clients on transient failure
        return
    }
    newCertsCM = certsCM
}

[tolusha]

Race condition: TOCTOU gap between shouldRebuildClients and setNewClients

ConfigureHttpClients calls shouldRebuildClients (RLock), then buildNewClients (no lock), then setNewClients (Lock). Between releasing the RLock and acquiring the Lock, another concurrent reconciliation could also pass the check and both would rebuild clients.

Suggested fix: Use a single Lock for the entire ConfigureHttpClients method to make check-and-set atomic.

[tolusha]

Comment contradicts actual behavior

The comment says "not an issue at all" but this IS an issue - the operator was configured to trust custom certificates. Rebuilding without them on transient failure breaks the trust chain.

Consider updating this comment after fixing the transient failure handling.

[tolusha]

System-level concern: Shared HTTP client with per-workspace config

The httpClientsHolder is shared across ALL concurrent reconciliations. If different workspaces have different merged TLSCertificateConfigmapRef values, the "last reconciled workspace wins" for HTTP client configuration.

Needs verification: Does DWOC merging always produce identical routing config regardless of workspace? Or can per-namespace overrides create different cert references?

If configs can differ, consider scoping the HTTP client holder per-namespace or per-config-hash.