STOR-2954: inject centralized TLS configuration for storage operators - #8887
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@ingvagabund: This pull request references STOR-2954 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
area/control-plane-operator
70b9d62 to
063ba79
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds serving configuration ConfigMaps and deployment mounts for the Cluster Storage Operator and CSI Snapshot Controller Operator. Controller configuration generation now uses a shared helper. New serving certificate manifests and PKI reconciliation functions create certificates with operator and localhost DNS names. Certificate reconciliation is enabled except on IBM Cloud and PowerVS platforms, with tests covering certificate fields and idempotence. Sequence Diagram(s)sequenceDiagram
participant HostedControlPlaneReconciler
participant PKIReconciler
participant ServingCertSecret
HostedControlPlaneReconciler->>HostedControlPlaneReconciler: evaluate storage and CSI platform gating
HostedControlPlaneReconciler->>PKIReconciler: reconcile operator serving certificate
PKIReconciler->>ServingCertSecret: create or update TLS certificate and key
ServingCertSecret-->>PKIReconciler: return certificate data
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
control-plane-operator/controllers/hostedcontrolplane/v2/snapshotcontroller/deployment.go (1)
41-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify: set
TypeMetadirectly instead of JSON→map→YAML roundtrip.
configv1.GenericControllerConfigembedsmetav1.TypeMetainline, soapiVersion/kindcan be set as struct fields and the whole config YAML-marshaled directly — no need for the JSON marshal/unmarshal-to-map dance. This also drops theencoding/jsonimport.This exact function body is duplicated verbatim in
storage/deployment.go. Consider extracting a shared helper (e.g. insupport/config) that both callers invoke with just theComponentName/BindAddress, to avoid maintaining two copies of this marshal logic.♻️ Proposed simplification
-func adaptControllerConfig(cpContext component.WorkloadContext, cm *corev1.ConfigMap) error { - profile := cpContext.HCP.Spec.Configuration.GetTLSSecurityProfile() - controllerConfig := configv1.GenericControllerConfig{ - ServingInfo: configv1.HTTPServingInfo{ - ServingInfo: configv1.ServingInfo{ - BindAddress: ":8443", - CipherSuites: config.CipherSuites(profile), - MinTLSVersion: config.MinTLSVersion(profile), - }, - }, - } - - asJSON, err := json.Marshal(controllerConfig) - if err != nil { - return fmt.Errorf("failed to json marshal config: %w", err) - } - - asMap := map[string]any{} - if err := json.Unmarshal(asJSON, &asMap); err != nil { - return fmt.Errorf("failed to json unmarshal config: %w", err) - } - - asMap["apiVersion"] = configv1.GroupVersion.String() - asMap["kind"] = "GenericControllerConfig" - - data, err := yaml.Marshal(asMap) +func adaptControllerConfig(cpContext component.WorkloadContext, cm *corev1.ConfigMap) error { + profile := cpContext.HCP.Spec.Configuration.GetTLSSecurityProfile() + controllerConfig := configv1.GenericControllerConfig{ + TypeMeta: metav1.TypeMeta{ + APIVersion: configv1.GroupVersion.String(), + Kind: "GenericControllerConfig", + }, + ServingInfo: configv1.HTTPServingInfo{ + ServingInfo: configv1.ServingInfo{ + BindAddress: ":8443", + CipherSuites: config.CipherSuites(profile), + MinTLSVersion: config.MinTLSVersion(profile), + }, + }, + } + + data, err := yaml.Marshal(controllerConfig) if err != nil { return fmt.Errorf("failed to yaml marshal config: %w", err) }I flagged this with `` since it touches the
openshift/api`GenericControllerConfig` struct shape; please confirm the embedded `TypeMeta` field name/tag matches what's assumed above.🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/snapshotcontroller/deployment.go` around lines 41 - 78, Simplify adaptControllerConfig by setting configv1.GenericControllerConfig.TypeMeta directly and YAML-marshaling the struct instead of converting through JSON and a map. Remove the unnecessary encoding/json roundtrip, keep the existing TLS profile-derived ServingInfo setup, and ensure apiVersion/kind are populated via the embedded TypeMeta on GenericControllerConfig. Since this logic is duplicated in storage/deployment.go, consider extracting a shared helper so both callers reuse the same config rendering path.
🤖 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.
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/snapshotcontroller/deployment.go`:
- Around line 41-78: Simplify adaptControllerConfig by setting
configv1.GenericControllerConfig.TypeMeta directly and YAML-marshaling the
struct instead of converting through JSON and a map. Remove the unnecessary
encoding/json roundtrip, keep the existing TLS profile-derived ServingInfo
setup, and ensure apiVersion/kind are populated via the embedded TypeMeta on
GenericControllerConfig. Since this logic is duplicated in
storage/deployment.go, consider extracting a shared helper so both callers reuse
the same config rendering path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: af393ddb-20c4-4046-b1ae-8da2c8c37591
⛔ Files ignored due to path filters (30)
control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/AROSwift/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/AROSwift/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/AROSwift/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/GCP/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/GCP/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/GCP/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/AROSwift/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/AROSwift/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/AROSwift/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/GCP/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/GCP/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/GCP/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_deployment.yamlis excluded by!**/testdata/**
📒 Files selected for processing (8)
control-plane-operator/controllers/hostedcontrolplane/v2/assets/cluster-storage-operator/controller-config.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/assets/cluster-storage-operator/deployment.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/assets/csi-snapshot-controller-operator/controller-config.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/assets/csi-snapshot-controller-operator/deployment.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/snapshotcontroller/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/snapshotcontroller/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/storage/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/storage/deployment.go
|
Running the CPO image locally: |
063ba79 to
b12aa98
Compare
|
Validated via #8912 (comment). Both operators are running |
|
/lgtm |
|
Scheduling tests matching the |
Test Resultse2e-aws
e2e-aks
|
|
I now have a complete picture. Here's the analysis: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe job failed due to a single flaky etcd chaos resilience test ( Root CauseThe root cause is a test sequencing/timing issue in the etcd chaos test suite, not a product bug or a regression from PR #8887. Sequence of events:
The etcd cluster was still recovering from the previous test's data corruption and recovery cycle. While the StatefulSet had converged (all pods running), the etcd cluster itself had not fully stabilized — members were still catching up or performing internal consistency checks. The PR #8887 is not involved: The PR adds TLS configuration for
Recommendations
Evidence
|
bryan-cox
left a comment
bryan-cox
left a comment
There was a problem hiding this comment.
Review: STOR-2954 — Inject centralized TLS configuration for storage operators
Overall the approach is correct — the WithManifestAdapter / WithAdaptFunction wiring follows the established CPOv2 pattern and the deployment changes (volume mounts, --config, --terminate-on-files) are sound. No NodePool config hash impact.
Two blocking items and a few suggestions inline. The core issues are DRY (the adaptControllerConfig function is now copy-pasted 4x across the codebase) and missing unit tests (the existing implementations all have dedicated configmap_test.go covering 6 TLS profile scenarios).
Questions
-
Are the storage operators consuming
--configtoday? The deployment assets add--config=/var/run/configmaps/config/config.yamlto both operators. Could you note in the PR description which OCP version introduces support for this flag, so reviewers can confirm compatibility? -
csi-snapshot-controller-operatorhas no--terminate-on-filesfor serving cert (unlikecluster-storage-operatorwhich already has two). Is this intentional because it doesn't mount a serving cert?
| cm.Data = map[string]string{} | ||
| } | ||
|
|
||
| cm.Data["config.yaml"] = string(data) |
There was a problem hiding this comment.
[blocking] DRY: adaptControllerConfig is duplicated 4x across the codebase
This function is character-for-character identical to snapshotcontroller/deployment.go, and nearly identical to registryoperator/configmap.go and pkioperator/configmap.go. The only varying parameters across all four copies are BindAddress and BindNetwork.
Consider extracting the shared logic into support/config/, which already owns CipherSuites() and MinTLSVersion():
// support/config/controllerconfig.go
func BuildGenericControllerConfigData(bindAddress, bindNetwork string, profile *configv1.TLSSecurityProfile) (string, error) {
// marshal/unmarshal/apiVersion/kind pipeline lives here once
}Each component's adaptControllerConfig becomes a 2-3 line wrapper. If this extraction is out of scope, a TODO comment tracking the debt would be acceptable.
| }) | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
[blocking] Missing unit tests for adaptControllerConfig
Both registryoperator/configmap_test.go and pkioperator/configmap_test.go have six test cases covering Intermediate, Modern, Old, Custom, nil-profile, and existing-data-preservation scenarios. This PR adds no unit tests for the new function — the fixture tests only exercise the default TLS profile.
Edge cases like Modern (TLS 1.3, no cipher suites) and Custom (user-specified ciphers) are where regressions hide. Please add configmap_test.go following the pattern in registryoperator/configmap_test.go.
[suggestion] SRP: split into its own configmap.go
Per established convention, registryoperator and pkioperator both place adaptControllerConfig in a dedicated configmap.go, keeping deployment.go focused on Deployment adaptation. Splitting to configmap.go + configmap_test.go would match the convention.
Same applies to snapshotcontroller/deployment.go.
There was a problem hiding this comment.
Unit tests added, with configmap.go + configmap_test.go pattern
| @@ -0,0 +1,6 @@ | |||
| apiVersion: v1 | |||
| kind: ConfigMap | |||
| metadata: | |||
There was a problem hiding this comment.
[suggestion] Asset ConfigMap should have a meaningful placeholder
The existing assets (control-plane-pki-operator/controller-config.yaml, cluster-image-registry-operator/controller-config.yaml) include a proper GenericControllerConfig placeholder:
data:
config.yaml: |-
apiVersion: config.openshift.io/v1
kind: GenericControllerConfig
servingInfo:
bindAddress: 0.0.0.0:8443This file just has config.yaml: "". The adapt function overwrites it at reconcile time, but a meaningful placeholder serves as documentation and provides a valid fallback if the adapt function were ever skipped. Same applies to csi-snapshot-controller-operator/controller-config.yaml.
| WithManifestAdapter( | ||
| "controller-config.yaml", | ||
| component.WithAdaptFunction(adaptControllerConfig), | ||
| ). |
There was a problem hiding this comment.
[praise] Correct CPOv2 wiring
The WithManifestAdapter("controller-config.yaml", component.WithAdaptFunction(adaptControllerConfig)) composition is exactly right and follows the established pattern. Deployment changes (--config, --terminate-on-files, read-only volume mount) are all sound. No NodePool config hash impact.
b12aa98 to
85141c5
Compare
openshift-ci
Bot
added
the
area/hypershift-operator
That depends on each operator. Most of the operators I have encounter set --terminate-on-files on the cert files too. I added the flag for the operator in openshift/cluster-csi-snapshot-controller-operator#276. Yet, if we are to keep setting this flag consistent we should examine all operators and set it for each. |
85141c5 to
6d9ab1f
Compare
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Dropped some comments. Thanks!
Additional note: pkioperator/configmap.go is the only one of the 4 adapters without its own unit test (configmap_test.go doesn't exist). The other 3 all have one. Since pkioperator uses bindNetwork: "tcp4" (the only one that doesn't pass ""), a test would verify that wiring is correct.
I found https://github.com/openshift/hypershift/blob/main/control-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/configmap_test.go. That seems to test the wiring in |
70a23bd to
fd07acf
Compare
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
`@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go`:
- Around line 1778-1783: Add unit-test coverage for isStorageAndCSIManaged
covering IBMCloudPlatform and PowerVSPlatform as false, plus a supported
platform as true. Use table-driven cases where appropriate and preserve the
function’s existing platform-gating behavior.
In
`@control-plane-operator/controllers/hostedcontrolplane/pki/cluster_storage_operator_test.go`:
- Around line 67-94: Add assertions in the certificate validation blocks for
cluster_storage_operator_test.go:67-94 and
csi_snapshot_controller_operator_test.go:67-94 to verify both client-auth and
server-auth extended key usages are present, matching the reconcilers’
X509UsageClientServerAuth contract. Preserve the existing DNS name, common name,
and organization assertions.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9015c96a-110c-4f1e-a0fc-c1eecee3ef64
⛔ Files ignored due to path filters (33)
control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/AROSwift/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/AROSwift/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/AROSwift/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/GCP/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/GCP/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/GCP/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/ModernTLS/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/ModernTLS/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/ModernTLS/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/zz_fixture_TestControlPlaneComponents_cluster_storage_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/AROSwift/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/AROSwift/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/AROSwift/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/GCP/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/GCP/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/GCP/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/ModernTLS/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/ModernTLS/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/ModernTLS/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/csi-snapshot-controller-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_csi_snapshot_controller_operator_deployment.yamlis excluded by!**/testdata/**
📒 Files selected for processing (7)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/manifests/clusterstorageoperator.gocontrol-plane-operator/controllers/hostedcontrolplane/manifests/csisnapshotcontrolleroperator.gocontrol-plane-operator/controllers/hostedcontrolplane/pki/cluster_storage_operator.gocontrol-plane-operator/controllers/hostedcontrolplane/pki/cluster_storage_operator_test.gocontrol-plane-operator/controllers/hostedcontrolplane/pki/csi_snapshot_controller_operator.gocontrol-plane-operator/controllers/hostedcontrolplane/pki/csi_snapshot_controller_operator_test.go
a99670d to
faba69f
Compare
bryan-cox
left a comment
bryan-cox
left a comment
There was a problem hiding this comment.
Second-round review — 2 suggestions, 1 nit. No blocking issues. All prior findings addressed.
| ). | ||
| WithManifestAdapter( | ||
| "controller-config.yaml", | ||
| component.WithAdaptFunction(component.NewGenericControllerConfigAdapter("0.0.0.0:8443", "")), |
There was a problem hiding this comment.
nit: confirm empty bindNetwork is intentional
Both storage and snapshotcontroller pass "" as bindNetwork, while the pre-existing pkioperator uses "tcp4". The fixture YAML confirms bindNetwork: "" renders. Since these are new TLS configs (not migrations of existing ones), this is likely intentional — just confirming it's a conscious choice rather than an omission.
There was a problem hiding this comment.
Yes, this is intentional.
Dedup a configmap builder with TLS injected configuration
Mount a configmap with an operator config injected with the HCP TLS security profile.
…uration Mount a configmap with an operator config injected with the HCP TLS security profile.
The newly mounted certificates secrets need to be reconciled
faba69f to
d681c26
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, ingvagabund The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/lgtm |
|
Scheduling tests matching the |
|
@ingvagabund: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest |
|
/verified by @jsafrane
/verified by @jsafrane |
openshift-ci-robot
added
the
verified
|
@jsafrane: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
1 similar comment
|
@jsafrane: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
7 participants
What this PR does / why we need it:
have cloud-storage-operator and csi-snapshot-controller-operator honor the centralized TLS configuration
Which issue(s) this PR fixes:
Fixes
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests