STOR-2954: Inject TLS from CVO to operators, update hypershift TLS based on CVO#712
Open
dfajmon wants to merge 3 commits into
Open
STOR-2954: Inject TLS from CVO to operators, update hypershift TLS based on CVO#712dfajmon wants to merge 3 commits into
dfajmon wants to merge 3 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@dfajmon: This pull request references STOR-2594 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "5.0." or "openshift-5.0.", but it targets "openshift-4.21" instead. 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
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
|
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:
WalkthroughAdds a ChangesCSI Operator ConfigMap TLS injection
Sequence Diagram(s)sequenceDiagram
rect rgba(70, 130, 180, 0.5)
note over DeploymentController,ConfigMap: Standalone mode
DeploymentController->>APIServerLister: Get("cluster") for TLS profile
APIServerLister-->>DeploymentController: APIServer (or empty fallback)
DeploymentController->>DeploymentController: tlsSettingsFromAPIServer (Intermediate default)
DeploymentController->>DeploymentController: marshal GenericOperatorConfig YAML
DeploymentController->>ConfigMap: Apply cm.Data["config.yaml"]
end
rect rgba(60, 179, 113, 0.5)
note over HyperShiftController,MgmtConfigMap: HyperShift mode
HyperShiftController->>HostedControlPlane: Get HCP unstructured
HostedControlPlane-->>HyperShiftController: HCP (or empty on error)
HyperShiftController->>HyperShiftController: tlsSettingsFromHCP (OpenSSL→IANA, Intermediate default)
HyperShiftController->>HyperShiftController: marshal GenericOperatorConfig YAML
HyperShiftController->>MgmtConfigMap: Apply cm.Data["config.yaml"]
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
dfajmon
changed the title
Contributor
|
@dfajmon: This pull request references STOR-2954 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "5.0.0" version, but no target version was set. 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. |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dfajmon 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
assets/csidriveroperators/powervs-block/hypershift/mgmt/06_deployment.yaml (1)
29-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin the restricted security context in the HyperShift overlay.
This management-cluster deployment still relies on ambient SCC defaults, while
assets/csidriveroperators/powervs-block/standalone/06_deployment.yamlexplicitly setsreadOnlyRootFilesystem,allowPrivilegeEscalation: false,capabilities.drop: [ALL],runAsNonRoot, andseccompProfile. That leaves the HyperShift variant with weaker manifest-level guarantees for the same operator.As per coding guidelines, Kubernetes manifests should set
securityContext: runAsNonRoot, readOnlyRootFilesystem, allowPrivilegeEscalation: false, andDrop ALL capabilities, add only what is required.🤖 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 `@assets/csidriveroperators/powervs-block/hypershift/mgmt/06_deployment.yaml` around lines 29 - 99, The HyperShift management deployment is missing the restricted container securityContext present in the standalone variant; update the container named powervs-block-csi-driver-operator (and optionally the pod spec) to set container.securityContext with readOnlyRootFilesystem: true, allowPrivilegeEscalation: false, runAsNonRoot: true (and a non-zero runAsUser if used in standalone), capabilities.drop: ["ALL"], and seccompProfile: { type: "RuntimeDefault" } so the HyperShift overlay matches the hardened manifest-level guarantees in the standalone/06_deployment.yaml.Sources: Coding guidelines, Linters/SAST tools
🤖 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 `@assets/csidriveroperators/openstack-cinder/base/07_deployment.yaml`:
- Around line 81-84: The pod template for the Deployment that adds the new
containerPort 8443 (named metrics) must include the network-policy label
openshift.storage.network-policy.operator-metrics-range: allow so the metrics
endpoint is scrappable; edit the Deployment resource (the podTemplateSpec for
the openstack-cinder-csi-driver-operator deployment) and add that label under
metadata.labels in the spec.template section to match the other operators that
expose metrics.
In `@pkg/operator/csidriveroperator/hypershift_deployment_controller.go`:
- Around line 258-286: tlsSettingsFromHCP currently defaults empty or
unrecognized TLS profile types to Modern (TLS 1.3) which contradicts the
fallback behavior in getHostedControlPlaneTLSSettings; update tlsSettingsFromHCP
so that when profileType is empty it sets pt =
configv1.TLSProfileIntermediateType and when profileSpec lookup fails (ok ==
false or profileSpec == nil) it falls back to
configv1.TLSProfiles[configv1.TLSProfileIntermediateType] instead of Modern;
ensure you only change the defaulting logic inside tlsSettingsFromHCP
(references: getHostedControlPlaneTLSSettings and tlsSettingsFromHCP) so tests
expecting Intermediate become correct.
---
Outside diff comments:
In `@assets/csidriveroperators/powervs-block/hypershift/mgmt/06_deployment.yaml`:
- Around line 29-99: The HyperShift management deployment is missing the
restricted container securityContext present in the standalone variant; update
the container named powervs-block-csi-driver-operator (and optionally the pod
spec) to set container.securityContext with readOnlyRootFilesystem: true,
allowPrivilegeEscalation: false, runAsNonRoot: true (and a non-zero runAsUser if
used in standalone), capabilities.drop: ["ALL"], and seccompProfile: { type:
"RuntimeDefault" } so the HyperShift overlay matches the hardened manifest-level
guarantees in the standalone/06_deployment.yaml.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 737cfc09-102c-4580-9bb4-134faa48d093
⛔ Files ignored due to path filters (21)
assets/csidriveroperators/aws-ebs/hypershift/mgmt/generated/apps_v1_deployment_aws-ebs-csi-driver-operator.yamlis excluded by!**/generated/**assets/csidriveroperators/aws-ebs/hypershift/mgmt/generated/v1_configmap_aws-ebs-csi-driver-operator-config.yamlis excluded by!**/generated/**assets/csidriveroperators/aws-ebs/standalone/generated/apps_v1_deployment_aws-ebs-csi-driver-operator.yamlis excluded by!**/generated/**assets/csidriveroperators/aws-ebs/standalone/generated/v1_configmap_aws-ebs-csi-driver-operator-config.yamlis excluded by!**/generated/**assets/csidriveroperators/azure-disk/hypershift/mgmt/generated/apps_v1_deployment_azure-disk-csi-driver-operator.yamlis excluded by!**/generated/**assets/csidriveroperators/azure-disk/hypershift/mgmt/generated/v1_configmap_azure-disk-csi-driver-operator-config.yamlis excluded by!**/generated/**assets/csidriveroperators/azure-disk/standalone/generated/apps_v1_deployment_azure-disk-csi-driver-operator.yamlis excluded by!**/generated/**assets/csidriveroperators/azure-disk/standalone/generated/v1_configmap_azure-disk-csi-driver-operator-config.yamlis excluded by!**/generated/**assets/csidriveroperators/azure-file/hypershift/mgmt/generated/apps_v1_deployment_azure-file-csi-driver-operator.yamlis excluded by!**/generated/**assets/csidriveroperators/azure-file/hypershift/mgmt/generated/v1_configmap_azure-file-csi-driver-operator-config.yamlis excluded by!**/generated/**assets/csidriveroperators/azure-file/standalone/generated/apps_v1_deployment_azure-file-csi-driver-operator.yamlis excluded by!**/generated/**assets/csidriveroperators/azure-file/standalone/generated/v1_configmap_azure-file-csi-driver-operator-config.yamlis excluded by!**/generated/**assets/csidriveroperators/openstack-cinder/hypershift/mgmt/generated/apps_v1_deployment_openstack-cinder-csi-driver-operator.yamlis excluded by!**/generated/**assets/csidriveroperators/openstack-cinder/hypershift/mgmt/generated/v1_configmap_openstack-cinder-csi-driver-operator-config.yamlis excluded by!**/generated/**assets/csidriveroperators/openstack-cinder/standalone/generated/apps_v1_deployment_openstack-cinder-csi-driver-operator.yamlis excluded by!**/generated/**assets/csidriveroperators/openstack-cinder/standalone/generated/v1_configmap_openstack-cinder-csi-driver-operator-config.yamlis excluded by!**/generated/**assets/csidriveroperators/openstack-manila/hypershift/guest/generated/default_operator.openshift.io_v1_clustercsidriver_manila.csi.openstack.org.yamlis excluded by!**/generated/**assets/csidriveroperators/openstack-manila/hypershift/mgmt/generated/apps_v1_deployment_manila-csi-driver-operator.yamlis excluded by!**/generated/**assets/csidriveroperators/openstack-manila/hypershift/mgmt/generated/v1_configmap_manila-csi-driver-operator-config.yamlis excluded by!**/generated/**assets/csidriveroperators/openstack-manila/standalone/generated/openshift-cluster-csi-drivers_apps_v1_deployment_manila-csi-driver-operator.yamlis excluded by!**/generated/**assets/csidriveroperators/openstack-manila/standalone/generated/openshift-cluster-csi-drivers_v1_configmap_manila-csi-driver-operator-config.yamlis excluded by!**/generated/**
📒 Files selected for processing (38)
assets/csidriveroperators/aws-ebs/base/03_configmap.yamlassets/csidriveroperators/aws-ebs/base/09_deployment.yamlassets/csidriveroperators/aws-ebs/base/kustomization.yamlassets/csidriveroperators/azure-disk/base/03_configmap.yamlassets/csidriveroperators/azure-disk/base/08_deployment.yamlassets/csidriveroperators/azure-disk/base/kustomization.yamlassets/csidriveroperators/azure-file/base/03_configmap.yamlassets/csidriveroperators/azure-file/base/08_deployment.yamlassets/csidriveroperators/azure-file/base/kustomization.yamlassets/csidriveroperators/gcp-pd/03_configmap.yamlassets/csidriveroperators/gcp-pd/07_deployment.yamlassets/csidriveroperators/ibm-vpc-block/03_configmap.yamlassets/csidriveroperators/ibm-vpc-block/08_deployment.yamlassets/csidriveroperators/openstack-cinder/base/03_configmap.yamlassets/csidriveroperators/openstack-cinder/base/07_deployment.yamlassets/csidriveroperators/openstack-cinder/base/kustomization.yamlassets/csidriveroperators/openstack-manila/base/03_configmap.yamlassets/csidriveroperators/openstack-manila/base/07_deployment.yamlassets/csidriveroperators/openstack-manila/base/kustomization.yamlassets/csidriveroperators/powervs-block/hypershift/mgmt/03_configmap.yamlassets/csidriveroperators/powervs-block/hypershift/mgmt/06_deployment.yamlassets/csidriveroperators/powervs-block/standalone/03_configmap.yamlassets/csidriveroperators/powervs-block/standalone/06_deployment.yamlassets/csidriveroperators/vsphere/03_configmap.yamlassets/csidriveroperators/vsphere/08_deployment.yamlpkg/operator/csidriveroperator/csioperatorclient/aws.gopkg/operator/csidriveroperator/csioperatorclient/azure-disk.gopkg/operator/csidriveroperator/csioperatorclient/azure-file.gopkg/operator/csidriveroperator/csioperatorclient/cinder.gopkg/operator/csidriveroperator/csioperatorclient/gcp-pd.gopkg/operator/csidriveroperator/csioperatorclient/ibm-vpc-block.gopkg/operator/csidriveroperator/csioperatorclient/manila.gopkg/operator/csidriveroperator/csioperatorclient/powervs-block.gopkg/operator/csidriveroperator/csioperatorclient/types.gopkg/operator/csidriveroperator/csioperatorclient/vsphere.gopkg/operator/csidriveroperator/hypershift_deployment_controller.gopkg/operator/csidriveroperator/hypershift_deployment_controller_test.gopkg/operator/operator_starter.go
Comment on lines
+81
to
+84
| ports: | ||
| - containerPort: 8443 | ||
| name: metrics | ||
| protocol: TCP |
There was a problem hiding this comment.
Add the metrics NetworkPolicy allow-label with the new 8443 listener.
This deployment now exposes the metrics port, but unlike gcp-pd and ibm-vpc-block in the same PR, its pod template still lacks openshift.storage.network-policy.operator-metrics-range: allow. That leaves openstack-cinder-csi-driver-operator as the outlier and can block scraping of the new endpoint.
💡 Suggested fix
labels:
name: openstack-cinder-csi-driver-operator
+ openshift.storage.network-policy.operator-metrics-range: allow🤖 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 `@assets/csidriveroperators/openstack-cinder/base/07_deployment.yaml` around
lines 81 - 84, The pod template for the Deployment that adds the new
containerPort 8443 (named metrics) must include the network-policy label
openshift.storage.network-policy.operator-metrics-range: allow so the metrics
endpoint is scrappable; edit the Deployment resource (the podTemplateSpec for
the openstack-cinder-csi-driver-operator deployment) and add that label under
metadata.labels in the spec.template section to match the other operators that
expose metrics.
ed53370 to
9473ccf
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
dfajmon
force-pushed
the
STOR-2954-tls-compliance
branch
from
9473ccf to
55a8b32
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
assets/csidriveroperators/powervs-block/hypershift/mgmt/06_deployment.yaml (2)
64-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet CPU/memory limits for the operator container.
Line 64 currently sets only requests. Missing limits can allow noisy-neighbor behavior and reduce scheduling/runtime predictability on management clusters.
Suggested patch
resources: requests: memory: 50Mi cpu: 10m + limits: + memory: 250Mi + cpu: 200mAs per coding guidelines, every container in Kubernetes/OpenShift manifests should define CPU and memory resource limits.
🤖 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 `@assets/csidriveroperators/powervs-block/hypershift/mgmt/06_deployment.yaml` around lines 64 - 67, The resources section for the operator container currently defines only requests for memory and cpu, but is missing the corresponding limits. Add a limits subsection under resources that specifies both cpu and memory limits (in addition to the existing requests) to prevent noisy-neighbor behavior and ensure predictable scheduling and runtime behavior on management clusters, as required by the coding guidelines for Kubernetes/OpenShift manifests.Source: Coding guidelines
29-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden pod/container securityContext before merge.
Line 29 onward defines the operator container without explicit
allowPrivilegeEscalation: false,readOnlyRootFilesystem: true, dropped capabilities, andrunAsNonRootat pod level. This leaves a weaker security baseline than the other driver deployments.Suggested patch
containers: - args: @@ resources: requests: memory: 50Mi cpu: 10m + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: + - ALL @@ spec: + securityContext: + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault priorityClassName: hypershift-control-planeAs per coding guidelines, Kubernetes/OpenShift manifests must enforce
securityContexthardening (runAsNonRoot,allowPrivilegeEscalation: false, and least-privilege capability posture).🤖 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 `@assets/csidriveroperators/powervs-block/hypershift/mgmt/06_deployment.yaml` around lines 29 - 79, The container specification for powervs-block-csi-driver-operator in the deployment lacks required security hardening. Add a securityContext block at the pod spec level (before containers) with runAsNonRoot set to true, and add a securityContext block within the powervs-block-csi-driver-operator container specification with allowPrivilegeEscalation set to false, readOnlyRootFilesystem set to true, and capabilities drop set to ALL. This ensures the container runs with non-root privileges, prevents privilege escalation, uses a read-only root filesystem, and drops all unnecessary capabilities in alignment with security hardening standards.Sources: Coding guidelines, Linters/SAST tools
🤖 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.
Outside diff comments:
In `@assets/csidriveroperators/powervs-block/hypershift/mgmt/06_deployment.yaml`:
- Around line 64-67: The resources section for the operator container currently
defines only requests for memory and cpu, but is missing the corresponding
limits. Add a limits subsection under resources that specifies both cpu and
memory limits (in addition to the existing requests) to prevent noisy-neighbor
behavior and ensure predictable scheduling and runtime behavior on management
clusters, as required by the coding guidelines for Kubernetes/OpenShift
manifests.
- Around line 29-79: The container specification for
powervs-block-csi-driver-operator in the deployment lacks required security
hardening. Add a securityContext block at the pod spec level (before containers)
with runAsNonRoot set to true, and add a securityContext block within the
powervs-block-csi-driver-operator container specification with
allowPrivilegeEscalation set to false, readOnlyRootFilesystem set to true, and
capabilities drop set to ALL. This ensures the container runs with non-root
privileges, prevents privilege escalation, uses a read-only root filesystem, and
drops all unnecessary capabilities in alignment with security hardening
standards.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f694e5e1-4423-45b6-8f67-157b8e6babb3
⛔ Files ignored due to path filters (25)
assets/csidriveroperators/aws-ebs/hypershift/mgmt/generated/apps_v1_deployment_aws-ebs-csi-driver-operator.yamlis excluded by!**/generated/**assets/csidriveroperators/aws-ebs/hypershift/mgmt/generated/v1_configmap_aws-ebs-csi-driver-operator-config.yamlis excluded by!**/generated/**assets/csidriveroperators/aws-ebs/standalone/generated/apps_v1_deployment_aws-ebs-csi-driver-operator.yamlis excluded by!**/generated/**assets/csidriveroperators/aws-ebs/standalone/generated/v1_configmap_aws-ebs-csi-driver-operator-config.yamlis excluded by!**/generated/**assets/csidriveroperators/azure-disk/hypershift/mgmt/generated/apps_v1_deployment_azure-disk-csi-driver-operator.yamlis excluded by!**/generated/**assets/csidriveroperators/azure-disk/hypershift/mgmt/generated/v1_configmap_azure-disk-csi-driver-operator-config.yamlis excluded by!**/generated/**assets/csidriveroperators/azure-disk/standalone/generated/apps_v1_deployment_azure-disk-csi-driver-operator.yamlis excluded by!**/generated/**assets/csidriveroperators/azure-disk/standalone/generated/v1_configmap_azure-disk-csi-driver-operator-config.yamlis excluded by!**/generated/**assets/csidriveroperators/azure-file/hypershift/mgmt/generated/apps_v1_deployment_azure-file-csi-driver-operator.yamlis excluded by!**/generated/**assets/csidriveroperators/azure-file/hypershift/mgmt/generated/v1_configmap_azure-file-csi-driver-operator-config.yamlis excluded by!**/generated/**assets/csidriveroperators/azure-file/standalone/generated/apps_v1_deployment_azure-file-csi-driver-operator.yamlis excluded by!**/generated/**assets/csidriveroperators/azure-file/standalone/generated/v1_configmap_azure-file-csi-driver-operator-config.yamlis excluded by!**/generated/**assets/csidriveroperators/openstack-cinder/hypershift/mgmt/generated/apps_v1_deployment_openstack-cinder-csi-driver-operator.yamlis excluded by!**/generated/**assets/csidriveroperators/openstack-cinder/hypershift/mgmt/generated/v1_configmap_openstack-cinder-csi-driver-operator-config.yamlis excluded by!**/generated/**assets/csidriveroperators/openstack-cinder/hypershift/mgmt/generated/v1_service_openstack-cinder-csi-driver-operator-metrics.yamlis excluded by!**/generated/**assets/csidriveroperators/openstack-cinder/standalone/generated/apps_v1_deployment_openstack-cinder-csi-driver-operator.yamlis excluded by!**/generated/**assets/csidriveroperators/openstack-cinder/standalone/generated/v1_configmap_openstack-cinder-csi-driver-operator-config.yamlis excluded by!**/generated/**assets/csidriveroperators/openstack-manila/hypershift/mgmt/generated/apps_v1_deployment_manila-csi-driver-operator.yamlis excluded by!**/generated/**assets/csidriveroperators/openstack-manila/hypershift/mgmt/generated/rbac.authorization.k8s.io_v1_role_manila-csi-driver-operator-role.yamlis excluded by!**/generated/**assets/csidriveroperators/openstack-manila/hypershift/mgmt/generated/rbac.authorization.k8s.io_v1_rolebinding_manila-csi-driver-operator-rolebinding.yamlis excluded by!**/generated/**assets/csidriveroperators/openstack-manila/hypershift/mgmt/generated/v1_configmap_manila-csi-driver-operator-config.yamlis excluded by!**/generated/**assets/csidriveroperators/openstack-manila/hypershift/mgmt/generated/v1_service_manila-csi-driver-operator-metrics.yamlis excluded by!**/generated/**assets/csidriveroperators/openstack-manila/hypershift/mgmt/generated/v1_serviceaccount_manila-csi-driver-operator.yamlis excluded by!**/generated/**assets/csidriveroperators/openstack-manila/standalone/generated/openshift-cluster-csi-drivers_apps_v1_deployment_manila-csi-driver-operator.yamlis excluded by!**/generated/**assets/csidriveroperators/openstack-manila/standalone/generated/openshift-cluster-csi-drivers_v1_configmap_manila-csi-driver-operator-config.yamlis excluded by!**/generated/**
📒 Files selected for processing (43)
assets/csidriveroperators/aws-ebs/base/03_configmap.yamlassets/csidriveroperators/aws-ebs/base/09_deployment.yamlassets/csidriveroperators/aws-ebs/base/kustomization.yamlassets/csidriveroperators/aws-ebs/hypershift/mgmt/kustomization.yamlassets/csidriveroperators/azure-disk/base/03_configmap.yamlassets/csidriveroperators/azure-disk/base/08_deployment.yamlassets/csidriveroperators/azure-disk/base/kustomization.yamlassets/csidriveroperators/azure-disk/hypershift/mgmt/kustomization.yamlassets/csidriveroperators/azure-file/base/03_configmap.yamlassets/csidriveroperators/azure-file/base/08_deployment.yamlassets/csidriveroperators/azure-file/base/kustomization.yamlassets/csidriveroperators/azure-file/hypershift/mgmt/kustomization.yamlassets/csidriveroperators/gcp-pd/03_configmap.yamlassets/csidriveroperators/gcp-pd/07_deployment.yamlassets/csidriveroperators/ibm-vpc-block/03_configmap.yamlassets/csidriveroperators/ibm-vpc-block/08_deployment.yamlassets/csidriveroperators/openstack-cinder/base/03_configmap.yamlassets/csidriveroperators/openstack-cinder/base/07_deployment.yamlassets/csidriveroperators/openstack-cinder/base/kustomization.yamlassets/csidriveroperators/openstack-cinder/hypershift/mgmt/kustomization.yamlassets/csidriveroperators/openstack-manila/base/03_configmap.yamlassets/csidriveroperators/openstack-manila/base/07_deployment.yamlassets/csidriveroperators/openstack-manila/base/kustomization.yamlassets/csidriveroperators/openstack-manila/hypershift/mgmt/kustomization.yamlassets/csidriveroperators/powervs-block/hypershift/mgmt/03_configmap.yamlassets/csidriveroperators/powervs-block/hypershift/mgmt/06_deployment.yamlassets/csidriveroperators/powervs-block/standalone/03_configmap.yamlassets/csidriveroperators/powervs-block/standalone/06_deployment.yamlassets/csidriveroperators/vsphere/03_configmap.yamlassets/csidriveroperators/vsphere/08_deployment.yamlpkg/operator/csidriveroperator/csioperatorclient/aws.gopkg/operator/csidriveroperator/csioperatorclient/azure-disk.gopkg/operator/csidriveroperator/csioperatorclient/azure-file.gopkg/operator/csidriveroperator/csioperatorclient/cinder.gopkg/operator/csidriveroperator/csioperatorclient/gcp-pd.gopkg/operator/csidriveroperator/csioperatorclient/ibm-vpc-block.gopkg/operator/csidriveroperator/csioperatorclient/manila.gopkg/operator/csidriveroperator/csioperatorclient/powervs-block.gopkg/operator/csidriveroperator/csioperatorclient/types.gopkg/operator/csidriveroperator/csioperatorclient/vsphere.gopkg/operator/csidriveroperator/hypershift_deployment_controller.gopkg/operator/csidriveroperator/hypershift_deployment_controller_test.gopkg/operator/operator_starter.go
✅ Files skipped from review due to trivial changes (10)
- assets/csidriveroperators/aws-ebs/base/kustomization.yaml
- assets/csidriveroperators/vsphere/03_configmap.yaml
- pkg/operator/csidriveroperator/csioperatorclient/gcp-pd.go
- assets/csidriveroperators/openstack-manila/base/kustomization.yaml
- assets/csidriveroperators/openstack-cinder/base/kustomization.yaml
- pkg/operator/csidriveroperator/csioperatorclient/vsphere.go
- assets/csidriveroperators/openstack-manila/base/03_configmap.yaml
- pkg/operator/operator_starter.go
- assets/csidriveroperators/azure-file/base/kustomization.yaml
- pkg/operator/csidriveroperator/csioperatorclient/azure-file.go
🚧 Files skipped from review as they are similar to previous changes (25)
- assets/csidriveroperators/openstack-cinder/base/03_configmap.yaml
- assets/csidriveroperators/azure-file/base/03_configmap.yaml
- pkg/operator/csidriveroperator/csioperatorclient/ibm-vpc-block.go
- assets/csidriveroperators/azure-disk/base/kustomization.yaml
- assets/csidriveroperators/ibm-vpc-block/03_configmap.yaml
- assets/csidriveroperators/aws-ebs/base/03_configmap.yaml
- assets/csidriveroperators/openstack-cinder/base/07_deployment.yaml
- pkg/operator/csidriveroperator/csioperatorclient/cinder.go
- assets/csidriveroperators/ibm-vpc-block/08_deployment.yaml
- pkg/operator/csidriveroperator/csioperatorclient/types.go
- assets/csidriveroperators/azure-file/base/08_deployment.yaml
- assets/csidriveroperators/powervs-block/standalone/03_configmap.yaml
- assets/csidriveroperators/gcp-pd/03_configmap.yaml
- assets/csidriveroperators/azure-disk/base/03_configmap.yaml
- assets/csidriveroperators/powervs-block/hypershift/mgmt/03_configmap.yaml
- assets/csidriveroperators/azure-disk/base/08_deployment.yaml
- assets/csidriveroperators/gcp-pd/07_deployment.yaml
- assets/csidriveroperators/openstack-manila/base/07_deployment.yaml
- assets/csidriveroperators/vsphere/08_deployment.yaml
- pkg/operator/csidriveroperator/csioperatorclient/azure-disk.go
- assets/csidriveroperators/powervs-block/standalone/06_deployment.yaml
- pkg/operator/csidriveroperator/csioperatorclient/aws.go
- pkg/operator/csidriveroperator/hypershift_deployment_controller_test.go
- pkg/operator/csidriveroperator/csioperatorclient/powervs-block.go
- pkg/operator/csidriveroperator/hypershift_deployment_controller.go
Contributor
Author
|
/retest |
jsafrane
reviewed
Jun 17, 2026
Comment on lines
+71
to
+74
| ports: | ||
| - containerPort: 8443 | ||
| name: metrics | ||
| protocol: TCP |
Contributor
There was a problem hiding this comment.
why exactly is the port now exposed?
Contributor
Author
There was a problem hiding this comment.
From my understanding, it was just missing the definition here, but runs the metrics exposed anyway.
https://prow.ci.openshift.org/view/gs/test-platform-results/logs/periodic-ci-openshift-tls-scanner-main-periodic-tls13-adherence/2066954802713071616
aws-ebs-csi-driver-operator-6fdf8c8694-k7l8c: [TLS Profile] 10.129.0.49:8443 - ssl/tls expand_less 0s
{TLS Compliance Failed TLSComplianceCheck API Server TLS config is not compliant.}
Contributor
|
Looking at azure-file-csi test results, I can see both azure-file and azure-disk did not get their TLS injected into the ConfigMap (search for Is that expected? Does the feature need a feature gate? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/operator/csidriveroperator/deploymentcontroller.go (1)
319-339: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider adding unit tests for the standalone TLS derivation logic.
The
tlsSettingsFromAPIServerfunction implements critical TLS profile fallback behavior (defaulting to Intermediate for nil/empty/unknown profiles), but unit test coverage is not visible in this cohort. The HyperShift equivalenttlsSettingsFromHCPhas comprehensive table-driven tests inhypershift_deployment_controller_test.go(lines 45-95 in context snippets). Adding parallel test coverage for the standalone path would improve confidence and prevent regressions.🤖 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 `@pkg/operator/csidriveroperator/deploymentcontroller.go` around lines 319 - 339, Add unit test coverage for the tlsSettingsFromAPIServer function following the table-driven test pattern used in the existing tlsSettingsFromHCP tests. Create tests that verify the fallback behavior to the Intermediate profile when the TLSSecurityProfile is nil, empty, or invalid, as well as tests for custom profiles and predefined profile types to ensure the function correctly handles all conditional branches and returns appropriate TLS versions and cipher suites.
🤖 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 `@pkg/operator/csidriveroperator/deploymentcontroller.go`:
- Around line 210-215: The condition checking
`c.csiOperatorConfig.StandaloneOperatorConfigAsset` before calling
`c.reconcileOperatorConfigMap(ctx)` prevents TLS config injection for six of
nine CSI drivers because their StandaloneOperatorConfigAsset field is not set.
Populate the StandaloneOperatorConfigAsset configuration field for the missing
drivers (aws, azure-disk, azure-file, cinder, manila, and powervs-block) to
match the implementation of the three drivers that already have it set (gcp-pd,
ibm-vpc-block, and vsphere), ensuring reconcileOperatorConfigMap is called for
all drivers to properly inject TLS configuration.
---
Nitpick comments:
In `@pkg/operator/csidriveroperator/deploymentcontroller.go`:
- Around line 319-339: Add unit test coverage for the tlsSettingsFromAPIServer
function following the table-driven test pattern used in the existing
tlsSettingsFromHCP tests. Create tests that verify the fallback behavior to the
Intermediate profile when the TLSSecurityProfile is nil, empty, or invalid, as
well as tests for custom profiles and predefined profile types to ensure the
function correctly handles all conditional branches and returns appropriate TLS
versions and cipher suites.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: aba52edb-b6b5-4508-ae58-2a204276ef55
⛔ Files ignored due to path filters (5)
assets/csidriveroperators/aws-ebs/standalone/generated/v1_configmap_aws-ebs-csi-driver-operator-config.yamlis excluded by!**/generated/**assets/csidriveroperators/azure-disk/standalone/generated/v1_configmap_azure-disk-csi-driver-operator-config.yamlis excluded by!**/generated/**assets/csidriveroperators/azure-file/standalone/generated/v1_configmap_azure-file-csi-driver-operator-config.yamlis excluded by!**/generated/**assets/csidriveroperators/openstack-cinder/standalone/generated/v1_configmap_openstack-cinder-csi-driver-operator-config.yamlis excluded by!**/generated/**assets/csidriveroperators/openstack-manila/standalone/generated/openshift-cluster-csi-drivers_v1_configmap_manila-csi-driver-operator-config.yamlis excluded by!**/generated/**
📒 Files selected for processing (20)
assets/csidriveroperators/aws-ebs/base/03_configmap.yamlassets/csidriveroperators/azure-disk/base/03_configmap.yamlassets/csidriveroperators/azure-file/base/03_configmap.yamlassets/csidriveroperators/gcp-pd/03_configmap.yamlassets/csidriveroperators/ibm-vpc-block/03_configmap.yamlassets/csidriveroperators/openstack-cinder/base/03_configmap.yamlassets/csidriveroperators/openstack-manila/base/03_configmap.yamlassets/csidriveroperators/powervs-block/standalone/03_configmap.yamlassets/csidriveroperators/vsphere/03_configmap.yamlpkg/operator/csidriveroperator/csioperatorclient/aws.gopkg/operator/csidriveroperator/csioperatorclient/azure-disk.gopkg/operator/csidriveroperator/csioperatorclient/azure-file.gopkg/operator/csidriveroperator/csioperatorclient/cinder.gopkg/operator/csidriveroperator/csioperatorclient/gcp-pd.gopkg/operator/csidriveroperator/csioperatorclient/ibm-vpc-block.gopkg/operator/csidriveroperator/csioperatorclient/manila.gopkg/operator/csidriveroperator/csioperatorclient/powervs-block.gopkg/operator/csidriveroperator/csioperatorclient/types.gopkg/operator/csidriveroperator/csioperatorclient/vsphere.gopkg/operator/csidriveroperator/deploymentcontroller.go
💤 Files with no reviewable changes (9)
- assets/csidriveroperators/azure-disk/base/03_configmap.yaml
- assets/csidriveroperators/vsphere/03_configmap.yaml
- assets/csidriveroperators/azure-file/base/03_configmap.yaml
- assets/csidriveroperators/aws-ebs/base/03_configmap.yaml
- assets/csidriveroperators/openstack-cinder/base/03_configmap.yaml
- assets/csidriveroperators/powervs-block/standalone/03_configmap.yaml
- assets/csidriveroperators/ibm-vpc-block/03_configmap.yaml
- assets/csidriveroperators/gcp-pd/03_configmap.yaml
- assets/csidriveroperators/openstack-manila/base/03_configmap.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/operator/csidriveroperator/csioperatorclient/azure-disk.go
- pkg/operator/csidriveroperator/csioperatorclient/powervs-block.go
- pkg/operator/csidriveroperator/csioperatorclient/aws.go
- pkg/operator/csidriveroperator/csioperatorclient/azure-file.go
- pkg/operator/csidriveroperator/csioperatorclient/types.go
dfajmon
force-pushed
the
STOR-2954-tls-compliance
branch
from
9d96d5c to
86a594e
Compare
dfajmon
force-pushed
the
STOR-2954-tls-compliance
branch
2 times, most recently
from
078216b to
beb3552
Compare
CSO reads TLS settings (minTLSVersion, cipherSuites) from APIServer/cluster and writes them as a GenericOperatorConfig into each CSI driver operator's config ConfigMap on every reconcile. Operators load this config via --config. - Add StandaloneOperatorConfigAsset to CSIOperatorConfig (asset path for the config ConfigMap) - Add config ConfigMap assets for all standalone CSI drivers - Add --config arg and volume mount to all standalone operator deployment assets - Add reconcileOperatorConfigMap() + tlsSettingsFromAPIServer() to DeploymentController - Subscribe to APIServer informer to trigger resync on TLS profile changes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Same pattern as standalone: reads TLS settings from HostedControlPlane.spec.configuration.apiServer.tlsSecurityProfile and writes GenericOperatorConfig into the mgmt-cluster operator ConfigMap. - Add MgmtOperatorConfigAsset to CSIOperatorConfig (HyperShift-capable drivers only) - Add reconcileOperatorConfigMap() + tlsSettingsFromHCP() to HyperShiftDeploymentController - Unit tests for tlsSettingsFromHCP covering all TLS profile types Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dfajmon
force-pushed
the
STOR-2954-tls-compliance
branch
3 times, most recently
from
4dfab16 to
c0e905d
Compare
tlsSettingsFromAPIServer and tlsSettingsFromHCP both duplicated the same TLS profile -> minTLSVersion/cipherSuites conversion logic. Extract to: - tlsSettingsFromProfile: shared core, defaults to Intermediate - operatorConfigYAML: serializes GenericOperatorConfig with given TLS settings tlsSettingsFromAPIServer and tlsSettingsFromHCP now delegate to these. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dfajmon
force-pushed
the
STOR-2954-tls-compliance
branch
from
c0e905d to
0417c73
Compare
Contributor
Author
|
/retest-required |
jsafrane
reviewed
Jul 1, 2026
Comment on lines
+260
to
+261
| ciphers, _, _ := unstructured.NestedStringSlice(hcp.UnstructuredContent(), "spec", "configuration", "apiServer", "tlsSecurityProfile", "custom", "ciphers") | ||
| minVersion, _, _ := unstructured.NestedString(hcp.UnstructuredContent(), "spec", "configuration", "apiServer", "tlsSecurityProfile", "custom", "minTLSVersion") |
Contributor
There was a problem hiding this comment.
Please log returned errors, just in case.
Comment on lines
+243
to
+247
| hcp, err := c.getHostedControlPlane() | ||
| if err != nil { | ||
| klog.Warningf("Failed to get HostedControlPlane, falling back to Intermediate TLS profile: %v", err) | ||
| hcp = &unstructured.Unstructured{} | ||
| } |
Contributor
There was a problem hiding this comment.
I think we should surface the error above (and into Degraded condition) instead of blindly applying the defaults.
Comment on lines
+283
to
+287
| apiServer, err := c.apiServerLister.Get("cluster") | ||
| if err != nil { | ||
| klog.Warningf("Failed to get APIServer cluster, using Intermediate TLS profile: %v", err) | ||
| apiServer = &configv1.APIServer{} | ||
| } |
Contributor
There was a problem hiding this comment.
return error to the caller to Degrade the operator
Contributor
|
@dfajmon: The following tests 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes