STOR-2954: feat: have CVO inject the centralized TLS configuration into the operator's config#276
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@ingvagabund: 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughThis PR annotates the operator ConfigMap for TLS injection, updates Deployment variants to mount the injected ConfigMap and serving certs, exposes a metrics port (8443), and passes --config plus --terminate-on-files args to restart on config/certificate changes; HyperShift patch now appends volumes instead of replacing arrays. ChangesOperator Configuration and Metrics Instrumentation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ingvagabund The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
manifests/07_deployment.yaml (1)
29-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd resource limits and health probes to this updated container spec.
This container still has requests only and no liveness/readiness probes. Since this PR is updating the pod wiring and exposing the metrics listener, please add cpu/memory limits plus probes here before merge.
As per coding guidelines,
**/*.{yaml,yml}:Resource limits (cpu, memory) on every containerandLiveness + readiness probes defined.🤖 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 `@manifests/07_deployment.yaml` around lines 29 - 58, The container "csi-snapshot-controller-operator" currently only defines resource requests and no probes; add a resources.limits block (cpu and memory) alongside existing requests and add both livenessProbe and readinessProbe on the container spec. Use the existing container name and metrics/containerPort (8443) as the probe port and choose an appropriate probe type (httpGet to a health endpoint such as /healthz or /ready, or tcpSocket if no HTTP endpoint) with sensible initialDelaySeconds, periodSeconds and failureThreshold values; ensure the probe fields are added under the same container definition that contains args like "start" and "--config=/var/run/configmaps/config/operator-config.yaml".
🤖 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 `@manifests/07_deployment.yaml`:
- Around line 29-58: The container "csi-snapshot-controller-operator" currently
only defines resource requests and no probes; add a resources.limits block (cpu
and memory) alongside existing requests and add both livenessProbe and
readinessProbe on the container spec. Use the existing container name and
metrics/containerPort (8443) as the probe port and choose an appropriate probe
type (httpGet to a health endpoint such as /healthz or /ready, or tcpSocket if
no HTTP endpoint) with sensible initialDelaySeconds, periodSeconds and
failureThreshold values; ensure the probe fields are added under the same
container definition that contains args like "start" and
"--config=/var/run/configmaps/config/operator-config.yaml".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 77dcc8f8-256c-4998-87cb-4219b8da56ae
📒 Files selected for processing (4)
manifests/03_configmap.yamlmanifests/07_deployment-hypershift.yamlmanifests/07_deployment-ibm-cloud-managed.yamlmanifests/07_deployment.yaml
|
/retest-required |
ingvagabund
force-pushed
the
cvo-injected-tls-configuration
branch
from
fa43efb to
378ae06
Compare
…ator's config Also, have the operator restart whenever the config changes.
ingvagabund
force-pushed
the
cvo-injected-tls-configuration
branch
from
378ae06 to
d22ce72
Compare
|
/retest-required |
1 similar comment
|
/retest-required |
{
"apiVersion": "v1",
"data": {
"operator-config.yaml": "apiVersion: operator.openshift.io/v1alpha1\nkind: GenericOperatorConfig\nservingInfo:\n cipherSuites:\n - TLS_AES_128_GCM_SHA256\n - TLS_AES_256_GCM_SHA384\n - TLS_CHACHA20_POLY1305_SHA256\n - TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256\n - TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384\n - TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256\n - TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256\n - TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384\n - TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256\n minTLSVersion: VersionTLS12\n"
},
"kind": "ConfigMap",
"metadata": {
"annotations": {
"capability.openshift.io/name": "CSISnapshot",
"config.openshift.io/inject-tls": "true",
"include.release.openshift.io/hypershift": "true",
"include.release.openshift.io/ibm-cloud-managed": "true",
"include.release.openshift.io/self-managed-high-availability": "true",
"include.release.openshift.io/single-node-developer": "true"
},
"creationTimestamp": "2026-06-01T09:09:09Z",
"labels": {
"app": "csi-snapshot-controller-operator"
},
"name": "csi-snapshot-controller-operator-config",
"namespace": "openshift-cluster-storage-operator",
"ownerReferences": [
{
"apiVersion": "config.openshift.io/v1",
"controller": true,
"kind": "ClusterVersion",
"name": "version",
"uid": "7e805fbf-b54f-4fb6-b96f-fd7fedd5c97a"
}
],
"resourceVersion": "1261",
"uid": "8fbde059-bead-450f-8398-bbac1e96e38d"
}
},TLS injected |
|
/retest-required |
|
/retest |
|
@ingvagabund: all tests passed! 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. |
3 participants
Also, have the operator restart whenever the config changes.
wip-docs: openshift/enhancements#2020
Summary by CodeRabbit
New Features
Improvements