CNTRLPLANE-3624: add tls security profile configuration for the control-plane-pki-operator - #8768
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@ricardomaraschini: This pull request references CNTRLPLANE-3624 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. |
|
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:
📝 WalkthroughWalkthroughA new 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/v2/assets/control-plane-pki-operator/deployment.yaml`:
- Around line 26-27: Remove the unrecognized `--files` flag from the deployment
arguments in the control-plane-pki-operator deployment. The `--files` argument
is not a valid flag for the control-plane-pki-operator binary and will be
silently ignored at runtime. Delete the line containing
`--files=/var/run/configmaps/control-plane-pki-operator-config/config.yaml` and
keep only the `--config` flag argument on the preceding line.
🪄 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: 9e6e14d1-6237-4d4a-b260-12932e20c314
⛔ Files ignored due to path filters (15)
control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/AROSwift/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/AROSwift/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/AROSwift/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/GCP/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/GCP/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/GCP/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_deployment.yamlis excluded by!**/testdata/**
📒 Files selected for processing (5)
control-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/controller-config.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/deployment.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/configmap.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/configmap_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8768 +/- ##
==========================================
+ Coverage 41.84% 41.87% +0.02%
==========================================
Files 759 760 +1
Lines 94073 94138 +65
==========================================
+ Hits 39361 39416 +55
- Misses 51956 51959 +3
- Partials 2756 2763 +7
... and 3 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
4322832 to
ec63a44
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)
control-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/deployment.yaml (2)
21-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit non-root container hardening in
securityContext(Line 21 onward).The container currently runs without explicit
runAsNonRoot,allowPrivilegeEscalation: false,readOnlyRootFilesystem, and dropped Linux capabilities. Please enforce these defaults in the manifest.Suggested patch
- args: - operator @@ image: controlplane-pki-operator name: control-plane-pki-operator + securityContext: + runAsNonRoot: true + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: + - ALL ports:As per coding guidelines: “securityContext: runAsNonRoot, readOnlyRootFilesystem, allowPrivilegeEscalation: false” and “Drop ALL capabilities, add only what is required.”
Also applies to: 45-61
🤖 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/assets/control-plane-pki-operator/deployment.yaml` around lines 21 - 27, Add a securityContext block to the container specification in the deployment manifest to enforce non-root execution and filesystem hardening. The container running the operator with args starting with "operator", "--namespace", and other flags requires a securityContext that sets runAsNonRoot to true, allowPrivilegeEscalation to false, readOnlyRootFilesystem to true, and drops ALL Linux capabilities. Apply these same security hardening settings to all container specifications in this manifest, including the containers section starting at line 21 and any additional containers referenced in the "Also applies to" section.Sources: Coding guidelines, Linters/SAST tools
47-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDefine container limits and health probes (Line 47 onward).
resources.limitsand liveness/readiness probes are missing. This weakens runtime safety and rollout behavior.Suggested patch
ports: - containerPort: 8443 name: metrics protocol: TCP resources: requests: cpu: 10m memory: 80Mi + limits: + cpu: 100m + memory: 256Mi + readinessProbe: + tcpSocket: + port: metrics + initialDelaySeconds: 5 + periodSeconds: 10 + livenessProbe: + tcpSocket: + port: metrics + initialDelaySeconds: 15 + periodSeconds: 20As per coding guidelines: “Resource limits (cpu, memory) on every container” and “Liveness + 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/deployment.yaml` around lines 47 - 55, The container definition in the control-plane-pki-operator deployment.yaml is missing resource limits and health probes. Currently only resources.requests is defined with cpu: 10m and memory: 80Mi, but resources.limits is absent. Add a limits section under resources with appropriate cpu and memory values, and add livenessProbe and readinessProbe sections to the container spec (after the resources section) to define proper health check behavior for the metrics endpoint on port 8443.Source: Coding guidelines
🤖 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
`@control-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/deployment.yaml`:
- Around line 21-27: Add a securityContext block to the container specification
in the deployment manifest to enforce non-root execution and filesystem
hardening. The container running the operator with args starting with
"operator", "--namespace", and other flags requires a securityContext that sets
runAsNonRoot to true, allowPrivilegeEscalation to false, readOnlyRootFilesystem
to true, and drops ALL Linux capabilities. Apply these same security hardening
settings to all container specifications in this manifest, including the
containers section starting at line 21 and any additional containers referenced
in the "Also applies to" section.
- Around line 47-55: The container definition in the control-plane-pki-operator
deployment.yaml is missing resource limits and health probes. Currently only
resources.requests is defined with cpu: 10m and memory: 80Mi, but
resources.limits is absent. Add a limits section under resources with
appropriate cpu and memory values, and add livenessProbe and readinessProbe
sections to the container spec (after the resources section) to define proper
health check behavior for the metrics endpoint on port 8443.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e1653be0-4481-44ea-9398-fe9bb48b2800
⛔ Files ignored due to path filters (15)
control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/AROSwift/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/AROSwift/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/AROSwift/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/GCP/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/GCP/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/GCP/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_deployment.yamlis excluded by!**/testdata/**
📒 Files selected for processing (5)
control-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/controller-config.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/deployment.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/configmap.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/configmap_test.go
✅ Files skipped from review due to trivial changes (1)
- control-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/controller-config.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- control-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/component.go
- control-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/configmap.go
- control-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/configmap_test.go
ec63a44 to
9c00ee3
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)
control-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/deployment.yaml (2)
21-61:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden container
securityContextexplicitly.The container spec currently lacks explicit hardening controls (for example
runAsNonRoot,allowPrivilegeEscalation: false,readOnlyRootFilesystem, and dropped capabilities). This weakens workload isolation for the PKI operator pod.Suggested patch
containers: - args: - operator - --namespace - $(HOSTED_CONTROL_PLANE_NAMESPACE) - --config=/var/run/configmaps/control-plane-pki-operator-config/config.yaml - --terminate-on-files=/var/run/configmaps/control-plane-pki-operator-config/config.yaml + securityContext: + runAsNonRoot: true + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: + - ALL command: - /usr/bin/control-plane-pki-operatorAs per coding guidelines, Kubernetes manifests must set these securityContext hardening fields and avoid privilege-escalation 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/deployment.yaml` around lines 21 - 61, The control-plane-pki-operator container lacks explicit security hardening controls in its specification. Add a securityContext section to the container spec that includes runAsNonRoot set to true, allowPrivilegeEscalation set to false, readOnlyRootFilesystem set to true, and a capabilities section that drops ALL capabilities. This securityContext should be added as a sibling to other container fields like image, name, ports, resources, and volumeMounts to enforce proper workload isolation and follow Kubernetes security best practices.Sources: Coding guidelines, Linters/SAST tools
51-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd CPU/memory limits for this container.
At Line 51, only resource requests are set. Missing limits can let this control-plane component overconsume node resources during spikes.
Suggested patch
resources: requests: cpu: 10m memory: 80Mi + limits: + cpu: 100m + memory: 160MiAs per coding guidelines, every container should define resource limits in Kubernetes manifests.
🤖 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/assets/control-plane-pki-operator/deployment.yaml` around lines 51 - 54, In the resources block where cpu and memory requests are defined with 10m and 80Mi respectively, add a limits section at the same indentation level as the requests block. This limits block should specify the maximum cpu and memory values this container can consume, ensuring the control-plane-pki-operator component cannot overconsume node resources during load spikes.Source: Coding guidelines
🤖 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
`@control-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/deployment.yaml`:
- Around line 21-61: The control-plane-pki-operator container lacks explicit
security hardening controls in its specification. Add a securityContext section
to the container spec that includes runAsNonRoot set to true,
allowPrivilegeEscalation set to false, readOnlyRootFilesystem set to true, and a
capabilities section that drops ALL capabilities. This securityContext should be
added as a sibling to other container fields like image, name, ports, resources,
and volumeMounts to enforce proper workload isolation and follow Kubernetes
security best practices.
- Around line 51-54: In the resources block where cpu and memory requests are
defined with 10m and 80Mi respectively, add a limits section at the same
indentation level as the requests block. This limits block should specify the
maximum cpu and memory values this container can consume, ensuring the
control-plane-pki-operator component cannot overconsume node resources during
load spikes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 918479f3-b90a-450c-b647-6845e918c852
⛔ Files ignored due to path filters (15)
control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/AROSwift/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/AROSwift/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/AROSwift/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/GCP/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/GCP/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/GCP/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_deployment.yamlis excluded by!**/testdata/**
📒 Files selected for processing (5)
control-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/controller-config.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/deployment.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/configmap.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/configmap_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- control-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/controller-config.yaml
- control-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/configmap.go
- control-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/configmap_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/component.go
9c00ee3 to
f7516d8
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 (3)
control-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/deployment.yaml (3)
21-61:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden container security context and capabilities.
This container is missing explicit hardening controls (
runAsNonRoot,allowPrivilegeEscalation: false,readOnlyRootFilesystem, dropping capabilities), which are required by the repo’s manifest policy.Suggested patch
containers: - args: - operator @@ image: controlplane-pki-operator name: control-plane-pki-operator + securityContext: + runAsNonRoot: true + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: + - ALL ports: - containerPort: 8443As per coding guidelines, Kubernetes/OpenShift manifests must set
runAsNonRoot,readOnlyRootFilesystem,allowPrivilegeEscalation: false, and drop capabilities unless 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/deployment.yaml` around lines 21 - 61, The control-plane-pki-operator container in the deployment manifest is missing required security hardening controls. Add a securityContext field to the container specification that sets runAsNonRoot to true, allowPrivilegeEscalation to false, readOnlyRootFilesystem to true, and includes a capabilities section that drops ALL capabilities. This should be added as a peer to the existing fields like image, name, ports, and volumeMounts within the container specification.Source: Coding guidelines
51-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd container resource limits (not only requests).
The manifest defines requests but no CPU/memory limits, which violates the manifest requirements and can cause noisy-neighbor risk.
Suggested patch
resources: requests: cpu: 10m memory: 80Mi + limits: + cpu: 100m + memory: 160MiAs per coding guidelines, resource limits (
cpu,memory) should be set on every container.🤖 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/assets/control-plane-pki-operator/deployment.yaml` around lines 51 - 54, The deployment manifest for the control-plane-pki-operator container defines resource requests (cpu: 10m, memory: 80Mi) but is missing resource limits. Add a limits section under the resources block with appropriate CPU and memory limit values set for the container. This ensures compliance with manifest requirements and prevents noisy-neighbor issues. Both requests and limits should be defined for every container as per coding guidelines.Source: Coding guidelines
45-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDefine readiness and liveness probes for the operator container.
No health probes are configured, so kubelet cannot distinguish slow start from unhealthy state or recover stuck processes reliably.
As per coding guidelines, Kubernetes/OpenShift manifests should define both liveness and readiness probes.
🤖 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/assets/control-plane-pki-operator/deployment.yaml` around lines 45 - 50, The control-plane-pki-operator container lacks readiness and liveness probes which prevents kubelet from detecting unhealthy states or performing automatic recovery. Add both livenessProbe and readinessProbe fields to the container specification for control-plane-pki-operator. Configure these probes to use HTTP checks against port 8443 (the metrics port already defined) with an appropriate health check endpoint such as /healthz or /metrics. Set reasonable initial delay, timeout, period, and failure threshold values suitable for operator startup and health monitoring.Source: Coding guidelines
🤖 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
`@control-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/deployment.yaml`:
- Around line 21-61: The control-plane-pki-operator container in the deployment
manifest is missing required security hardening controls. Add a securityContext
field to the container specification that sets runAsNonRoot to true,
allowPrivilegeEscalation to false, readOnlyRootFilesystem to true, and includes
a capabilities section that drops ALL capabilities. This should be added as a
peer to the existing fields like image, name, ports, and volumeMounts within the
container specification.
- Around line 51-54: The deployment manifest for the control-plane-pki-operator
container defines resource requests (cpu: 10m, memory: 80Mi) but is missing
resource limits. Add a limits section under the resources block with appropriate
CPU and memory limit values set for the container. This ensures compliance with
manifest requirements and prevents noisy-neighbor issues. Both requests and
limits should be defined for every container as per coding guidelines.
- Around line 45-50: The control-plane-pki-operator container lacks readiness
and liveness probes which prevents kubelet from detecting unhealthy states or
performing automatic recovery. Add both livenessProbe and readinessProbe fields
to the container specification for control-plane-pki-operator. Configure these
probes to use HTTP checks against port 8443 (the metrics port already defined)
with an appropriate health check endpoint such as /healthz or /metrics. Set
reasonable initial delay, timeout, period, and failure threshold values suitable
for operator startup and health monitoring.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: aa1b5698-4d6c-4e16-94cf-5dc62eab6c03
⛔ Files ignored due to path filters (15)
control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/AROSwift/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/AROSwift/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/AROSwift/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/GCP/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/GCP/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/GCP/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_deployment.yamlis excluded by!**/testdata/**
📒 Files selected for processing (5)
control-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/controller-config.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/deployment.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/configmap.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/configmap_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- control-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/component.go
- control-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/controller-config.yaml
- control-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/configmap.go
- control-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/configmap_test.go
f7516d8 to
5d4d5c3
Compare
configure the control-plane-pki-operator to use the tls security profile settings from the hostedcontrolplane resource. this ensures the operator's metrics endpoint uses ciphers and minimum tls version that match the cluster's security requirements. implementation: - add configmap adapter to generate genericcontrollerconfig with tls settings derived from hcp.spec.configuration.tlssecurityprofile - mount the config and uses --config flag on the operator deployment - reuse existing config.ciphersuites() and config.mintlsversion() helper functions for consistency with other control plane components this implementation is very similar to the registry operator implementation.
we have changed the control plane pki operator deployment and added a new config map to the equation. we need to regenerate the testdata to incorporate these changes.
5d4d5c3 to
4937212
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 (1)
control-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/deployment.yaml (1)
22-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd mandatory container
securityContexthardening.This Deployment still allows insecure defaults (root-capable runtime and privilege escalation by default). Please set
runAsNonRoot,readOnlyRootFilesystem,allowPrivilegeEscalation: false, and drop all capabilities forcontrol-plane-pki-operator.Suggested patch
- args: - operator - --namespace - $(HOSTED_CONTROL_PLANE_NAMESPACE) - --config=/var/run/configmaps/control-plane-pki-operator-config/config.yaml - --terminate-on-files=/var/run/configmaps/control-plane-pki-operator-config/config.yaml command: - /usr/bin/control-plane-pki-operator + securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true + readOnlyRootFilesystem: true + capabilities: + drop: + - ALL env:As per coding guidelines, Kubernetes/OpenShift manifests must enforce
runAsNonRoot,readOnlyRootFilesystem,allowPrivilegeEscalation: false, and dropALLcapabilities.Also applies to: 59-61
🤖 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/assets/control-plane-pki-operator/deployment.yaml` around lines 22 - 27, Add a securityContext specification to the control-plane-pki-operator container to enforce security hardening. Within the container specification (where the args are defined), add a securityContext block that sets runAsNonRoot to true, readOnlyRootFilesystem to true, allowPrivilegeEscalation to false, and includes a capabilities section that drops ALL. Apply the same securityContext hardening to the other container referenced at lines 59-61.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
`@control-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/deployment.yaml`:
- Around line 22-27: Add a securityContext specification to the
control-plane-pki-operator container to enforce security hardening. Within the
container specification (where the args are defined), add a securityContext
block that sets runAsNonRoot to true, readOnlyRootFilesystem to true,
allowPrivilegeEscalation to false, and includes a capabilities section that
drops ALL. Apply the same securityContext hardening to the other container
referenced at lines 59-61.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7ae58a56-9d32-4fa6-b8a1-e77b2699a161
⛔ Files ignored due to path filters (15)
control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/AROSwift/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/AROSwift/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/AROSwift/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/GCP/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/GCP/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/GCP/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_deployment.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_config_configmap.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_controlplanecomponent.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/control-plane-pki-operator/zz_fixture_TestControlPlaneComponents_control_plane_pki_operator_deployment.yamlis excluded by!**/testdata/**
📒 Files selected for processing (5)
control-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/controller-config.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/deployment.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/configmap.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/configmap_test.go
✅ Files skipped from review due to trivial changes (1)
- control-plane-operator/controllers/hostedcontrolplane/v2/assets/control-plane-pki-operator/controller-config.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- control-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/configmap.go
- control-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/component.go
- control-plane-operator/controllers/hostedcontrolplane/v2/pkioperator/configmap_test.go
|
/lgtm |
|
@kaleemsiddiqu: 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. |
|
Scheduling tests matching the |
Test Resultse2e-aws
e2e-aks
|
|
/retest |
|
Now I have the full picture. The PR adds TLS security profile configuration to the Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe After the churn settled, all 3 final pods ( The secondary failure ( This failure is unrelated to PR #8768. The PR changes are scoped to:
None of these changes affect the Recommendations
Evidence
|
jparrill
left a comment
There was a problem hiding this comment.
Dropped some comments. Thanks!
| ServingInfo: configv1.HTTPServingInfo{ | ||
| ServingInfo: configv1.ServingInfo{ | ||
| BindAddress: "0.0.0.0:8443", | ||
| BindNetwork: "tcp4", |
There was a problem hiding this comment.
Hey Ricardo — I noticed you're adding BindNetwork: "tcp4" but the registryoperator (which you reference as the baseline) doesn't set it. In the repo, only KAS uses "tcp4". Is there a specific reason to force IPv4-only on the PKI operator? If not, dropping it would let it work on dual-stack clusters out of the box.
Related: "0.0.0.0:8443" also forces IPv4. The registry uses ":60000" (no IP). Could you use ":8443" for consistency?
Note: the base asset template (controller-config.yaml, lines 10-11) also has these IPv4-only values. While the adapter overwrites the template entirely at runtime, it'd be good to keep them consistent.
There was a problem hiding this comment.
Thanks for this comment. This difference was indeed based on the APIServer config generation (which seems to enforce 0.0.0.0 and tcp4). The long story short is that, differently from the Registry Operator, we need to specify an IP and a port as per validation here (the slightly longer story tells that the registry predates this whole library-go tooling). Failing to do so makes the pki operator not to start.
I assumed that this was OK as if the API Server does not come up nothing else would.
Please advice.
There was a problem hiding this comment.
For sake of easier understanding, this is what happens if the IP isn't set: https://go.dev/play/p/dkPew1VSLjd
There was a problem hiding this comment.
Humm you're right, what about:
0.0.0.0 + tcp4 forces IPv4-only, which would break on dual-stack or IPv6-only clusters. Maybe we can use [::] (the IPv6 unspecified address) instead:
BindAddress: "[::]:8443",
// drop BindNetwork entirely
This should work because:
- Passes library-go validation:
net.SplitHostPort("[::]:8443")→host="::", andnet.ParseIP("::")returns a valid IP. - Dual-stack safe: with no BindNetwork set, CreateListener in
k8s.io/apiserverdefaults to "tcp", and tcp +[::]listens on both IPv4 and IPv6. - IPv4-only clusters still work: Linux maps IPv4 connections onto IPv6 sockets by default
(net.ipv6.bindv6only=0).
The asset template (controller-config.yaml) should match:
servingInfo:
bindAddress: "[::]:8443"
WDYT?
There was a problem hiding this comment.
OK, I will do as instructed.
There was a problem hiding this comment.
Discussed this on Slack. Ricardo were right initially because the config is for the inner pod not the network layer. The KAS is configured in the same way. The network layer will do the translation from IPv6 to IPv4, so no issues.
| shouldPreserveKey: "other-key", | ||
| }, | ||
| { | ||
| name: "When TLS profile is nil it should use Intermediate profile", |
There was a problem hiding this comment.
Nit: the "When TLS profile is nil" case sets Configuration and APIServer as non-nil with just TLSSecurityProfile = nil. But in production HCP.Spec.Configuration itself can be nil (it's an omitempty pointer field). Same for APIServer == nil.
GetTLSSecurityProfile() handles both safely (it has a nil-receiver guard), but it'd be nice to have a couple of extra test cases verifying those paths — something like:
{
name: "When Configuration is nil it should default to Intermediate profile",
// don't set Configuration on the HCP
},Not blocking, but if you're aiming to cover all nil paths like the registry does, this would round it out.
| "sigs.k8s.io/yaml" | ||
| ) | ||
|
|
||
| func Test_adaptControllerConfig(t *testing.T) { |
There was a problem hiding this comment.
Minor nit: would you mind adding t.Parallel() to the test function and subtests? The cases are independent (each creates its own HCP and ConfigMap). The registry doesn't do it either, so not blocking — just following repo conventions.
| expectedMinTLS: configv1.TLSProfiles[configv1.TLSProfileIntermediateType].MinTLSVersion, | ||
| }, | ||
| { | ||
| name: "When TLS profile is Modern it should use TLS 1.3 with empty cipher list", |
There was a problem hiding this comment.
FYI: the Modern profile produces an empty cipher suite slice (because OpenSSLToIANACipherSuites drops TLS 1.3 ciphers). The test assumes the field gets omitted from the YAML thanks to the omitempty struct tag upstream. If upstream ever changes those tags, this assertion would break silently.
I don't think anything needs to change right now — just leaving a note in case this test ever fails for no apparent reason.
|
/approve |
|
/retest-required |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jparrill, ricardomaraschini 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 |
|
@ricardomaraschini: 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. |
What this PR does / why we need it:
Configure the
control-plane-pki-operatorto use the TLS security profile settings from the HostedCluster resource. This ensures the operator's metric endpoint uses ciphers and minimum TLS version that match the cluster's security requirements.Implementation:
Note
This follows what has been done for the Image Registry Operator, almost verbatim.
configv1.GenericControllerConfigwith TLS settings derived fromhcp.spec.configuration.apiServer.tlsSecurityProfile.--configflag on the operator deployment.config.CipherSuites()andconfig.minTLSVersion()helper functions for consistency with other control plane components.Checklist:
Summary by CodeRabbit