Add terminationMessagePolicy and required-scc annotation for OCP 4.21 conformance#411
Open
dustman9000 wants to merge 1 commit into
Open
Add terminationMessagePolicy and required-scc annotation for OCP 4.21 conformance#411dustman9000 wants to merge 1 commit into
dustman9000 wants to merge 1 commit into
Conversation
Contributor
WalkthroughThe changes add OpenShift security constraints and modify termination message behavior across two deployment configuration files. An annotation specifying the required Security Context Constraint is added to pod templates, and container termination message handling is configured to fall back to logs on error. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 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 |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dustman9000 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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #411 +/- ##
=======================================
Coverage 72.44% 72.44%
=======================================
Files 11 11
Lines 704 704
=======================================
Hits 510 510
Misses 173 173
Partials 21 21 🚀 New features to boost your workflow:
|
dustman9000
force-pushed
the
termination-message-policy
branch
from
c7623dc to
d1414d9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
deploy/operator.yaml (1)
30-48: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd explicit securityContext to align with restricted-v2 SCC.
While the
openshift.io/required-scc: restricted-v2annotation (line 15) declares the SCC requirement, the container spec should explicitly set securityContext fields to align with restricted-v2 constraints and best practices.🛡️ Proposed securityContext addition
- name: splunk-forwarder-operator # Replace this with the built image name image: REPLACE_IMAGE + securityContext: + runAsNonRoot: true + allowPrivilegeEscalation: false + capabilities: + drop: + - ALL + seccompProfile: + type: RuntimeDefault command: - splunk-forwarder-operatorAs per coding guidelines: "securityContext: runAsNonRoot, readOnlyRootFilesystem, allowPrivilegeEscalation: false" and "Drop ALL capabilities, add only what is required" for Kubernetes/OpenShift 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 `@deploy/operator.yaml` around lines 30 - 48, Add an explicit securityContext to the container spec for the splunk-forwarder-operator container: set runAsNonRoot: true, readOnlyRootFilesystem: true, allowPrivilegeEscalation: false, drop all capabilities and only add minimal required ones (if any), and set a non-root runAsUser/runAsGroup as appropriate; update the container with these fields (container name: splunk-forwarder-operator) so the manifest aligns with the openshift.io/required-scc: restricted-v2 policy and coding guidelines.
🧹 Nitpick comments (2)
deploy/operator.yaml (2)
30-48: ⚡ Quick winDefine resource limits for the container.
The container spec lacks resource limits (cpu, memory), which can lead to resource exhaustion and scheduling issues.
📊 Proposed resource limits
- name: splunk-forwarder-operator # Replace this with the built image name image: REPLACE_IMAGE + resources: + limits: + cpu: 200m + memory: 256Mi + requests: + cpu: 100m + memory: 128Mi command: - splunk-forwarder-operatorNote: Adjust the values based on actual operator resource requirements.
As per coding guidelines: "Resource limits (cpu, memory) on every container" for Kubernetes/OpenShift 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 `@deploy/operator.yaml` around lines 30 - 48, Add CPU and memory resource requests and limits to the splunk-forwarder-operator container spec to satisfy the guideline requiring resource limits; update the container with a resources block (requests and limits) for cpu and memory (e.g., set conservative defaults for requests and slightly higher limits) so the operator can be scheduled and bounded, and ensure this is applied to the container with name "splunk-forwarder-operator" (the same spec that currently defines env vars WATCH_NAMESPACE, POD_NAME, OPERATOR_NAME and terminationMessagePolicy).
30-48: ⚡ Quick winAdd liveness and readiness probes.
The container spec lacks health probes, which are necessary for Kubernetes to properly manage the operator lifecycle and detect failures.
🏥 Proposed health probes
- name: splunk-forwarder-operator # Replace this with the built image name image: REPLACE_IMAGE + livenessProbe: + httpGet: + path: /healthz + port: 8081 + initialDelaySeconds: 15 + periodSeconds: 20 + readinessProbe: + httpGet: + path: /readyz + port: 8081 + initialDelaySeconds: 5 + periodSeconds: 10 command: - splunk-forwarder-operatorNote: Adjust the probe endpoints and timings based on the operator's actual health check implementation.
As per coding guidelines: "Liveness + readiness probes defined" for Kubernetes/OpenShift 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 `@deploy/operator.yaml` around lines 30 - 48, Add Kubernetes livenessProbe and readinessProbe entries to the container spec for the splunk-forwarder-operator container: under the container with name "splunk-forwarder-operator" (and image REPLACE_IMAGE / command splunk-forwarder-operator) add a livenessProbe and readinessProbe blocks (e.g., HTTPGet or Exec probes pointing at your operator's health endpoint or probe command) with sensible defaults (initialDelaySeconds, periodSeconds, timeoutSeconds, failureThreshold) so Kubernetes can manage lifecycle; adjust probe type, path and timings to match the operator's actual health check implementation.
🤖 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 `@deploy/operator.yaml`:
- Around line 30-48: Add an explicit securityContext to the container spec for
the splunk-forwarder-operator container: set runAsNonRoot: true,
readOnlyRootFilesystem: true, allowPrivilegeEscalation: false, drop all
capabilities and only add minimal required ones (if any), and set a non-root
runAsUser/runAsGroup as appropriate; update the container with these fields
(container name: splunk-forwarder-operator) so the manifest aligns with the
openshift.io/required-scc: restricted-v2 policy and coding guidelines.
---
Nitpick comments:
In `@deploy/operator.yaml`:
- Around line 30-48: Add CPU and memory resource requests and limits to the
splunk-forwarder-operator container spec to satisfy the guideline requiring
resource limits; update the container with a resources block (requests and
limits) for cpu and memory (e.g., set conservative defaults for requests and
slightly higher limits) so the operator can be scheduled and bounded, and ensure
this is applied to the container with name "splunk-forwarder-operator" (the same
spec that currently defines env vars WATCH_NAMESPACE, POD_NAME, OPERATOR_NAME
and terminationMessagePolicy).
- Around line 30-48: Add Kubernetes livenessProbe and readinessProbe entries to
the container spec for the splunk-forwarder-operator container: under the
container with name "splunk-forwarder-operator" (and image REPLACE_IMAGE /
command splunk-forwarder-operator) add a livenessProbe and readinessProbe blocks
(e.g., HTTPGet or Exec probes pointing at your operator's health endpoint or
probe command) with sensible defaults (initialDelaySeconds, periodSeconds,
timeoutSeconds, failureThreshold) so Kubernetes can manage lifecycle; adjust
probe type, path and timings to match the operator's actual health check
implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a59e8fb9-9a04-4815-af64-794caa35b323
📒 Files selected for processing (2)
deploy/operator.yamldeploy_pko/Deployment-splunk-forwarder-operator.yaml.gotmpl
🚧 Files skipped from review as they are similar to previous changes (1)
- deploy_pko/Deployment-splunk-forwarder-operator.yaml.gotmpl
dustman9000
force-pushed
the
termination-message-policy
branch
from
577c5ed to
e844766
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
deploy/operator.yaml (3)
31-47: 🏗️ Heavy liftAdd resource limits to prevent resource exhaustion.
The operator container lacks CPU and memory resource limits, which could lead to unbounded resource consumption.
⚙️ Suggested resource limits
containers: - name: splunk-forwarder-operator # Replace this with the built image name image: REPLACE_IMAGE command: - splunk-forwarder-operator imagePullPolicy: Always + resources: + limits: + cpu: 200m + memory: 256Mi + requests: + cpu: 100m + memory: 128Mi env:Note: Adjust values based on actual operator resource usage patterns.
As per coding guidelines: "Resource limits (cpu, memory) 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 `@deploy/operator.yaml` around lines 31 - 47, The operator container spec for the "splunk-forwarder-operator" container is missing resource requests/limits; add a resources block under that container (resources.requests and resources.limits) and set sensible CPU and memory values (e.g., small requests and stricter limits) to prevent unbounded consumption — update the container definition for splunk-forwarder-operator to include resources.requests.cpu, resources.requests.memory, resources.limits.cpu and resources.limits.memory (tune values as appropriate for your operator).
31-47: ⚡ Quick winAdd liveness and readiness probes for better health monitoring.
The operator container lacks health probes, which are important for Kubernetes to monitor and manage the operator lifecycle.
🏥 Suggested health probes
containers: - name: splunk-forwarder-operator # Replace this with the built image name image: REPLACE_IMAGE command: - splunk-forwarder-operator imagePullPolicy: Always + livenessProbe: + httpGet: + path: /healthz + port: 8081 + initialDelaySeconds: 15 + periodSeconds: 20 + readinessProbe: + httpGet: + path: /readyz + port: 8081 + initialDelaySeconds: 5 + periodSeconds: 10 env:Note: Verify the operator exposes
/healthzand/readyzendpoints on port 8081, or adjust accordingly.As per coding guidelines: "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 `@deploy/operator.yaml` around lines 31 - 47, The container spec for splunk-forwarder-operator is missing Kubernetes health probes; add livenessProbe and readinessProbe entries under the container (the one with name splunk-forwarder-operator and command splunk-forwarder-operator) that hit the operator HTTP endpoints (default paths /healthz for liveness and /readyz for readiness) on port 8081 (or change port/path if the operator exposes different endpoints), include sensible initialDelaySeconds, periodSeconds and failureThreshold values, and ensure probe type is an HTTP GET to allow the kubelet to monitor and restart/traffic-route accordingly.
31-47: ⚡ Quick winConsider adding explicit securityContext fields for clarity and portability.
While the
restricted-v2SCC will enforce security constraints at runtime, explicitly declaring them in the manifest improves clarity and ensures the deployment is portable to non-OpenShift Kubernetes clusters.🔒 Suggested securityContext addition
containers: - name: splunk-forwarder-operator # Replace this with the built image name image: REPLACE_IMAGE command: - splunk-forwarder-operator imagePullPolicy: Always + securityContext: + runAsNonRoot: true + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: + - ALL env:As per coding guidelines: "securityContext: runAsNonRoot, readOnlyRootFilesystem, allowPrivilegeEscalation: false" and "Drop 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 `@deploy/operator.yaml` around lines 31 - 47, The manifest omits explicit securityContext settings for the operator container (splunk-forwarder-operator); add a pod-level podSecurityContext (e.g., runAsNonRoot: true, runAsUser: 1000, fsGroup: 1000) under spec.template.spec and add a container-level securityContext inside the splunk-forwarder-operator container with runAsNonRoot: true, runAsUser matching the pod, readOnlyRootFilesystem: true, allowPrivilegeEscalation: false, capabilities: drop: ["ALL"] and only add specific capabilities if required, and optionally set seccompProfile to RuntimeDefault to make the manifest self-describing and portable across clusters.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@deploy/operator.yaml`:
- Around line 31-47: The operator container spec for the
"splunk-forwarder-operator" container is missing resource requests/limits; add a
resources block under that container (resources.requests and resources.limits)
and set sensible CPU and memory values (e.g., small requests and stricter
limits) to prevent unbounded consumption — update the container definition for
splunk-forwarder-operator to include resources.requests.cpu,
resources.requests.memory, resources.limits.cpu and resources.limits.memory
(tune values as appropriate for your operator).
- Around line 31-47: The container spec for splunk-forwarder-operator is missing
Kubernetes health probes; add livenessProbe and readinessProbe entries under the
container (the one with name splunk-forwarder-operator and command
splunk-forwarder-operator) that hit the operator HTTP endpoints (default paths
/healthz for liveness and /readyz for readiness) on port 8081 (or change
port/path if the operator exposes different endpoints), include sensible
initialDelaySeconds, periodSeconds and failureThreshold values, and ensure probe
type is an HTTP GET to allow the kubelet to monitor and restart/traffic-route
accordingly.
- Around line 31-47: The manifest omits explicit securityContext settings for
the operator container (splunk-forwarder-operator); add a pod-level
podSecurityContext (e.g., runAsNonRoot: true, runAsUser: 1000, fsGroup: 1000)
under spec.template.spec and add a container-level securityContext inside the
splunk-forwarder-operator container with runAsNonRoot: true, runAsUser matching
the pod, readOnlyRootFilesystem: true, allowPrivilegeEscalation: false,
capabilities: drop: ["ALL"] and only add specific capabilities if required, and
optionally set seccompProfile to RuntimeDefault to make the manifest
self-describing and portable across clusters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: eece075e-37f5-4f3f-b320-c289543f72ef
⛔ Files ignored due to path filters (3)
build/Dockerfileis excluded by!build/**build/Dockerfile.olm-registryis excluded by!build/**deploy_pko/.test-fixtures/default/Deployment-splunk-forwarder-operator.yamlis excluded by!**/.test-fixtures/**
📒 Files selected for processing (2)
deploy/operator.yamldeploy_pko/Deployment-splunk-forwarder-operator.yaml.gotmpl
🚧 Files skipped from review as they are similar to previous changes (1)
- deploy_pko/Deployment-splunk-forwarder-operator.yaml.gotmpl
Contributor
|
@dustman9000: 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. |
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
2 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
terminationMessagePolicy: FallbackToLogsOnErrorto the operator container spec in both OLM (deploy/) and PKO (deploy_pko/) deployment manifestsopenshift.io/required-scc: restricted-v2annotation to pod template metadata in both deployment manifestsThese are required for OCP 4.21 conformance. The
terminationMessagePolicyensures container termination messages capture log output on error, and therequired-sccannotation explicitly declares the security context constraint the pod needs.Test plan
terminationMessagePolicyappears in pod spec viaoc get pod -o yamlrequired-sccannotation appears in pod metadataSummary by CodeRabbit