chore: Configure DWI with tlsCertificateConfigmapRef when certificate…

[tolusha]

…s imported to che-operator

What does this PR do?

Screenshot/screencast of this PR

N/A

What issues does this PR fix or reference?

eclipse-che/che#23870

How to test this PR?

1. Deploy the operator:

OpenShift

./build/scripts/olm/test-catalog-from-sources.sh

or

build/scripts/docker-run.sh /bin/bash -c "
  oc login \
    --token=<...> \
    --server=<...> \
    --insecure-skip-tls-verify=true && \
  build/scripts/olm/test-catalog-from-sources.sh
"

on Minikube

./build/scripts/minikube-tests/test-operator-from-sources.sh

Common Test Scenarios

• Deploy Eclipse Che
• Start an empty workspace
• Open terminal and build/run an image
• Stop a workspace
• Check operator logs for reconciliation errors or infinite reconciliation loops

PR Checklist

As the author of this Pull Request I made sure that:

• The Eclipse Contributor Agreement is valid
• Code produced is complete
• Code builds without errors
• Tests are covering the bugfix
• The repository devfile is up to date and works
• Relevant user documentation updated
• Relevant contributing documentation updated
• CI/CD changes implemented, documented and communicated

Reviewers

Reviewers, please comment how you tested the PR when approving it.

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[openshift-ci]

[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.

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]

Comprehensive PR Review: Configure DWI with tlsCertificateConfigmapRef

I've completed a comprehensive review including standard code review, deep design analysis, and system-level impact assessment. Overall, this is a well-implemented PR with thorough test coverage that correctly addresses the linked issue.

✅ Summary

The PR successfully configures DevWorkspace Operator to trust CA certificates imported by che-operator by:

• Adding updateTLSCertificateConfigmapRef to set TLSCertificateConfigmapRef when ca-certs-merged exists with data
• Adding the controller.devfile.io/watch-configmap=true label so DWO can watch certificate changes
• Adding SetControllerReference to 4 ConfigMaps for proper garbage collection

📋 Findings

⚠️ Items to Verify Before Merge

1. Owner reference on OpenShift CA bundle ConfigMap (certificates.go:177)

   • Adding SetControllerReference means the ca-bundle ConfigMap will be garbage-collected when the CheCluster CR is deleted
   • On OpenShift, this ConfigMap may have existed before CheCluster and might be expected to survive deletion
   • Please verify this is the desired behavior

2. No watch on ca-certs-merged for che-operator (dev_workspace_config.go:103)

   • Certificate changes won't propagate to DWO config until the next unrelated reconciliation trigger
   • Consider adding a watch/informer for timely propagation or verify this delay is acceptable

💡 Suggestions (Non-blocking)

3. Test masking may hide cross-feature interference (dev_workspace_config_scheduling_test.go:803)

   • Four test functions use cmpopts.IgnoreFields(controllerv1alpha1.RoutingConfig{}, "TLSCertificateConfigmapRef")
   • While routing tests cover TLS thoroughly, this means tests won't detect if TLS accidentally interferes with other features
   • Consider assert.Equal on specific fields instead of broad ignore patterns

4. Check for ConfigMapEnsureLabels dead code (diffs.go:44)

   • DefaultsLabelKeys was removed but ConfigMapEnsureLabels may still exist
   • Verify with: grep -r "ConfigMapEnsureLabels" --include="*.go"

5. Add error-path test (dev_workspace_config.go:103)

   • Test case where GetIgnoreNotFound returns an error would improve coverage

6. Document TypeMeta removal rationale

   • TypeMeta setting was removed in multiple places (certificates.go lines 131-134, 240-243, 337-340)
   • A comment explaining that ClientWrapper.Sync handles this would help future maintainers

7. Clarify test expectations (dev_workspace_config_routing_test.go:347)

   • Inconsistency between nil and &RoutingConfig{} expectations in "clear" tests
   • Comment explaining the implicit behavior would be helpful

🎉 Excellent Work

• Comprehensive test coverage: 8 TLS test cases + 4 proxy composition tests
• Clean refactoring: 3328-line test file split into 6 focused files
• Thoughtful design: GetLabelsAndAnnotations is a genuine improvement
• Operational improvement: Proper garbage collection via controller references
• Integration detail: DevWorkspaceWatchConfigMapLabel enables DWO to react to updates

Verdict

💬 Comment - Ready to merge after verifying items 1 and 2 above. The suggestions are non-blocking improvements.