CNTRLPLANE-3778: Migrate openshift-test-private kcm cases to OTE#943
CNTRLPLANE-3778: Migrate openshift-test-private kcm cases to OTE#943gangwgr wants to merge 1 commit into
Conversation
|
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 complete e2e test suite for the kube-controller-manager operator with an ChangesKCM e2e test suite and infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 6❌ Failed checks (6 warnings)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
gangwgr
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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: 4
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (25)
test/testdata/workloads/cronjob_54196.yaml-16-26 (1)
16-26:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmbedded CronJob template object needs explicit secure defaults.
The templated CronJob omits required
securityContextsettings and container resource limits; this should be part of the template so every rendered instance is compliant.As per coding guidelines, Kubernetes/OpenShift manifests must set
securityContext(runAsNonRoot,readOnlyRootFilesystem,allowPrivilegeEscalation: false), dropALLcapabilities, and define CPU/memory limits 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 `@test/testdata/workloads/cronjob_54196.yaml` around lines 16 - 26, The CronJob template in the containers spec is missing required security defaults. For the container named "hello", add a securityContext object with runAsNonRoot set to true, readOnlyRootFilesystem set to true, and allowPrivilegeEscalation set to false. Additionally add a capabilities section under securityContext that drops ALL capabilities. Finally, add a resources object with defined limits for both CPU and memory to ensure resource constraints are enforced on every rendered CronJob instance.Source: Coding guidelines
test/testdata/workloads/cronjob75375.yaml-19-24 (1)
19-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing required pod/container hardening in
pi75375CronJob.Add explicit pod/container
securityContextand CPU/memory requests/limits to align this fixture with restricted policies.As per coding guidelines, Kubernetes/OpenShift manifests must set
securityContext(runAsNonRoot,readOnlyRootFilesystem,allowPrivilegeEscalation: false), dropALLcapabilities, and define CPU/memory limits 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 `@test/testdata/workloads/cronjob75375.yaml` around lines 19 - 24, The CronJob definition for pi75375 is missing required security hardening and resource constraints. Add a securityContext at the pod spec level with runAsNonRoot set to true, and add a securityContext to the pi container with readOnlyRootFilesystem set to true, allowPrivilegeEscalation set to false, and capabilities.drop set to ALL. Additionally, add resources section to the pi container with both requests (cpu and memory) and limits (cpu and memory) to comply with restricted security policies and ensure proper resource management.Sources: Coding guidelines, Linters/SAST tools
test/testdata/workloads/cronjob54195notz.yaml-10-19 (1)
10-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis CronJob fixture also lacks required security and resource controls.
Please add explicit pod/container hardening (
allowPrivilegeEscalation: false,runAsNonRoot,readOnlyRootFilesystem, dropALLcapabilities) and container CPU/memory limits.As per coding guidelines, Kubernetes/OpenShift manifests must set
securityContext(runAsNonRoot,readOnlyRootFilesystem,allowPrivilegeEscalation: false), dropALLcapabilities, and define CPU/memory limits 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 `@test/testdata/workloads/cronjob54195notz.yaml` around lines 10 - 19, Add security hardening and resource constraints to the container specification in the cronjob54195notz.yaml CronJob fixture. Within the container definition for the "hello" container (after the imagePullPolicy field), add a securityContext block that sets runAsNonRoot to true, readOnlyRootFilesystem to true, allowPrivilegeEscalation to false, and includes capabilities with drop set to ALL. Additionally, add a resources section with limits for both CPU and memory to enforce resource constraints on the container. These additions should be at the same indentation level as the command and imagePullPolicy fields.Sources: Coding guidelines, Linters/SAST tools
test/testdata/workloads/cronjob75375.yaml-22-22 (1)
22-22:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse an immutable image reference instead of
image: perl.Using a mutable tag introduces non-reproducible test behavior and avoidable supply-chain risk. Pin this image by digest, as done in the other workload fixtures.
🤖 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 `@test/testdata/workloads/cronjob75375.yaml` at line 22, Replace the mutable image reference `image: perl` in the cronjob75375.yaml file with an immutable reference using a specific image digest (SHA256 hash). Instead of using just the tag `perl`, pin the image by appending `@sha256:` followed by the specific digest, matching the pattern used in other workload fixture files in the same directory. This ensures reproducible test behavior and reduces supply-chain risk.test/testdata/workloads/cronjob54195ic.yaml-11-20 (1)
11-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply the same mandatory pod/container hardening in this CronJob fixture.
The container spec is missing explicit
securityContextcontrols and resource limits required by repo policy.As per coding guidelines, Kubernetes/OpenShift manifests must set
securityContext(runAsNonRoot,readOnlyRootFilesystem,allowPrivilegeEscalation: false), dropALLcapabilities, and define CPU/memory limits 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 `@test/testdata/workloads/cronjob54195ic.yaml` around lines 11 - 20, In the container spec for the "hello" container within the cronjob54195ic.yaml fixture, add mandatory security hardening controls and resource limits according to repo policy. Add a securityContext section specifying runAsNonRoot as true, readOnlyRootFilesystem as true, and allowPrivilegeEscalation as false. Include a capabilities drop directive to drop ALL capabilities. Additionally, add a resources section with defined CPU and memory limits for the container. These hardening controls must match the security standards applied elsewhere in the repository to ensure consistent pod/container security across all Kubernetes/OpenShift manifests.Sources: Coding guidelines, Linters/SAST tools
test/testdata/workloads/cronjob56176.yaml-18-23 (1)
18-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCronJob pod template is missing required security hardening and limits.
The container runs without explicit security controls and no resource requests/limits are defined.
As per coding guidelines, Kubernetes/OpenShift manifests must set
securityContext(runAsNonRoot,readOnlyRootFilesystem,allowPrivilegeEscalation: false), dropALLcapabilities, and define CPU/memory limits 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 `@test/testdata/workloads/cronjob56176.yaml` around lines 18 - 23, The container named "pi" in the CronJob pod template is missing required security hardening and resource constraints. Add a securityContext block to the container spec with runAsNonRoot set to true, readOnlyRootFilesystem set to true, and allowPrivilegeEscalation set to false. Also add a capabilities section under securityContext that drops ALL capabilities. Additionally, add a resources section with both limits and requests defined for CPU and memory (e.g., limits and requests for cpu and memory fields) to ensure proper resource allocation and prevent resource exhaustion.Sources: Coding guidelines, Linters/SAST tools
test/e2e/kcm_cronjob.go-45-47 (1)
45-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
oc debug cronjobassertion contradicts the test’s expected failure behavior.This case is named as a failure-path test, but it currently expects no error from
oc debug. That inverts the test intent and can mask regressions in the failure contract.Suggested patch
debugErr, err := oc.Run("debug").Args("cronjob/cronjob56176").Output() -o.Expect(err).NotTo(o.HaveOccurred()) +o.Expect(err).To(o.HaveOccurred()) o.Expect(debugErr).NotTo(o.ContainSubstring("v1.CronJob is not supported by debug"))🤖 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 `@test/e2e/kcm_cronjob.go` around lines 45 - 47, The test assertions for the oc debug cronjob operation contradict the expected failure behavior. In the debugErr and err handling block near the cronjob/cronjob56176 operation, invert the assertions: change the first o.Expect(err).NotTo(o.HaveOccurred()) to expect an error to occur, and change the o.Expect(debugErr).NotTo(o.ContainSubstring("v1.CronJob is not supported by debug")) to expect that the error message DOES contain the unsupported message. This aligns the test to properly validate the failure path for unsupported cronjob debug operations.test/testdata/workloads/cronjob54195.yaml-11-20 (1)
11-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden the CronJob pod/container security settings explicitly.
This manifest omits required hardening controls (
runAsNonRoot,readOnlyRootFilesystem,allowPrivilegeEscalation: false, dropped capabilities) and container resource limits.Suggested patch
spec: schedule: '* * * * *' timeZone: Asia/Calcutta jobTemplate: spec: template: spec: + automountServiceAccountToken: false containers: - name: hello image: quay.io/openshifttest/busybox@sha256:c5439d7db88ab5423999530349d327b04279ad3161d7596d2126dfb5b02bfd1f imagePullPolicy: IfNotPresent + securityContext: + runAsNonRoot: true + readOnlyRootFilesystem: true + allowPrivilegeEscalation: false + capabilities: + drop: ["ALL"] + resources: + requests: + cpu: 10m + memory: 32Mi + limits: + cpu: 100m + memory: 128Mi command: - /bin/sh - -c - date; echo Hello from the CronJob restartPolicy: OnFailureAs per coding guidelines, Kubernetes/OpenShift manifests must set
securityContext(runAsNonRoot,readOnlyRootFilesystem,allowPrivilegeEscalation: false), dropALLcapabilities, and define CPU/memory limits 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 `@test/testdata/workloads/cronjob54195.yaml` around lines 11 - 20, The CronJob manifest is missing required security hardening controls in the container specification. Add a securityContext block to the hello container with the following settings: set runAsNonRoot to true, readOnlyRootFilesystem to true, and allowPrivilegeEscalation to false, and include a capabilities section that drops ALL capabilities. Additionally, add a resources section with defined CPU and memory limits to the container specification. These settings should be applied at the container level within the containers array in the spec.template.spec section of the CronJob manifest.Sources: Coding guidelines, Linters/SAST tools
test/e2e/kcm_cronjob.go-132-134 (1)
132-134:⚠️ Potential issue | 🟠 MajorReplace
regexp.MatchStringwithstrings.Containsto avoid ignoring errors.The
stringsContainshelper ignores the error return fromregexp.MatchString. Since the function is only used for literal substring searches (looking for "cronjob54195" and "cronjob54196" in output), usestrings.Containsinstead, which is safer and simpler.Suggested patch
import ( "path/filepath" - "regexp" + "strings" "time" @@ func stringsContains(s, sub string) bool { - matched, _ := regexp.MatchString(sub, s) - return matched + return strings.Contains(s, sub) }Per Go security guidelines, never ignore error returns.
🤖 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 `@test/e2e/kcm_cronjob.go` around lines 132 - 134, The stringsContains function is using regexp.MatchString but ignoring its error return value, which violates Go security guidelines. Since this function is only performing literal substring searches (not regex pattern matching), replace the regexp.MatchString call with strings.Contains instead. This eliminates the error that is being ignored and provides a simpler, safer implementation for the actual use case of searching for literal strings like "cronjob54195" and "cronjob54196".Source: Coding guidelines
test/e2e/kcm_operator.go-164-173 (1)
164-173:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftGate the 70-replica StatefulSet stress case by cluster topology
Line 170 applies
statefulset-76130.yaml, which uses a fixedreplicas: 70workload, but this test has no topology-aware guard. On SNO/two-node/HyperShift footprints, this can turn into deterministic scheduling timeouts unrelated to KCM restart behavior.As per coding guidelines, deployment/operator/controller logic should account for OpenShift topologies (SNO, Two-Node Fixed, Two-Node with Arbiter, HyperShift) and flag replica-count assumptions not protected by topology-aware checks.
🤖 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 `@test/e2e/kcm_operator.go` around lines 164 - 173, The testKCMNoRestartParallelStatefulSet function applies a StatefulSet workload with a fixed 70-replica count from statefulset-76130.yaml without checking the cluster topology first, which causes scheduling timeouts on resource-constrained topologies like SNO, two-node, and HyperShift. Before creating the StatefulSet with the Run and Execute calls, add a topology-aware guard to skip or modify this workload based on the cluster footprint. Check the cluster topology (SNO, two-node fixed, two-node with arbiter, or HyperShift) and either skip the creation of this specific workload or adjust the replica count accordingly to ensure the test only runs the 70-replica stress case on topologies that can handle it.Source: Coding guidelines
test/e2e/kcm_operator.go-109-109 (1)
109-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not ignore restore errors in deferred kube-apiserver manifest rollback
Line 109 defers
testlib.DebugNodeWithChroot(...)without checking its return. If rollback fails, the node can remain in a broken state and cascade failures into later tests.Suggested patch
- defer testlib.DebugNodeWithChroot(oc, leaderKcm, "mv", "/home/kube-apiserver-pod.yaml", "/etc/kubernetes/manifests/") + defer func() { + _, restoreErr := testlib.DebugNodeWithChroot(oc, leaderKcm, "mv", "/home/kube-apiserver-pod.yaml", "/etc/kubernetes/manifests/") + o.Expect(restoreErr).NotTo(o.HaveOccurred()) + }()As per coding guidelines,
**/*.gomust "Never ignore error returns".🤖 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 `@test/e2e/kcm_operator.go` at line 109, The deferred call to testlib.DebugNodeWithChroot is ignoring its error return value, which violates the Go coding guidelines that require checking all error returns. Wrap the testlib.DebugNodeWithChroot call in a deferred anonymous function that captures and checks the error return value, and ensure any returned error is properly logged or handled to prevent silent failures during kube-apiserver manifest rollback that could leave the node in a broken state.Sources: Coding guidelines, Linters/SAST tools
test/e2e/kcm_operator.go-210-213 (1)
210-213:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake deferred node-manifest restoration fail loudly on errors
Line 210-213 swallows
Execute()errors during cleanup (_ = ...). In a disruptive test, silent cleanup failures can leave control-plane nodes mutated and destabilize the suite.Suggested patch
defer func() { for _, node := range masterNodes { - _ = oc.AsAdmin().WithoutNamespace().Run("debug").Args("node/"+node, "-n", operatorclient.TargetNamespace, "--", "chroot", "/host", "mv", "/etc/kubernetes/kube-controller-manager-pod.yaml", "/etc/kubernetes/manifests/").Execute() + err := oc.AsAdmin().WithoutNamespace().Run("debug").Args("node/"+node, "-n", operatorclient.TargetNamespace, "--", "chroot", "/host", "mv", "/etc/kubernetes/kube-controller-manager-pod.yaml", "/etc/kubernetes/manifests/").Execute() + o.Expect(err).NotTo(o.HaveOccurred()) } }()As per coding guidelines,
**/*.gomust "Never ignore error returns".🤖 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 `@test/e2e/kcm_operator.go` around lines 210 - 213, The deferred cleanup code in the node-manifest restoration loop is ignoring the error returned by Execute() using the blank identifier pattern. Remove the blank identifier assignment and instead capture the error return value from the Execute() call on the chained command (oc.AsAdmin().WithoutNamespace().Run("debug").Args(...).Execute()). Check the error and log it appropriately to ensure that any cleanup failures during node restoration are visible rather than silently swallowed, preventing the test suite from being destabilized by undetected control-plane mutations.Source: Coding guidelines
test/testdata/workloads/statefulset-79105.yaml-24-41 (1)
24-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winComplete the container/pod security controls instead of empty stubs
Line 24-41 currently uses empty
resources/securityContextstubs and misses required controls (allowPrivilegeEscalation: false, capability drop,runAsNonRoot,readOnlyRootFilesystem, probes). This leaves the fixture out of compliance with repo policy.Suggested patch
spec: + automountServiceAccountToken: false + securityContext: + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault containers: - image: quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83 imagePullPolicy: IfNotPresent name: hello-pod ports: - containerPort: 8080 protocol: TCP - resources: {} + resources: + requests: + cpu: 10m + memory: 64Mi + limits: + cpu: 100m + memory: 128Mi securityContext: - capabilities: {} + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: ["ALL"] privileged: false + readinessProbe: + tcpSocket: + port: 8080 + livenessProbe: + tcpSocket: + port: 8080As per coding guidelines,
**/*.{yaml,yml}manifests must enforce non-root/read-only/anti-priv-esc controls, drop capabilities, set resource limits, and define liveness/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 `@test/testdata/workloads/statefulset-79105.yaml` around lines 24 - 41, The container named hello-pod in this StatefulSet YAML has incomplete security controls with empty stubs for resources and securityContext, and lacks required probes. Replace the empty resources block with proper memory and CPU limits/requests, populate the securityContext with allowPrivilegeEscalation set to false, add runAsNonRoot: true, readOnlyRootFilesystem: true, drop all capabilities, and add livenessProbe and readinessProbe sections to the container spec to comply with repository security and operational guidelines.Sources: Coding guidelines, Linters/SAST tools
test/testdata/workloads/statefulset-76130.yaml-31-40 (1)
31-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden the StatefulSet pod/container security baseline
Line 31-40 defines a container without the required pod/container hardening controls (
runAsNonRoot,readOnlyRootFilesystem,allowPrivilegeEscalation: false, capability drop, resource limits, probes). This currently violates the repo’s Kubernetes manifest policy and leaves the fixture with avoidable security drift.Suggested patch
spec: template: metadata: labels: app: hello-openshift spec: + automountServiceAccountToken: false + securityContext: + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault containers: - name: hello-openshift image: quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83 + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: ["ALL"] + resources: + requests: + cpu: 10m + memory: 64Mi + limits: + cpu: 100m + memory: 128Mi + readinessProbe: + httpGet: + path: / + port: web + livenessProbe: + httpGet: + path: / + port: web ports: - containerPort: 80 name: webAs per coding guidelines,
**/*.{yaml,yml}manifests must setsecurityContext: runAsNonRoot, readOnlyRootFilesystem, allowPrivilegeEscalation: false, drop capabilities, define resource limits, and include liveness/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 `@test/testdata/workloads/statefulset-76130.yaml` around lines 31 - 40, The hello-openshift container specification lacks the required security hardening controls mandated by the repository's Kubernetes manifest policy. Add a securityContext block to the container with runAsNonRoot set to true, readOnlyRootFilesystem set to true, and allowPrivilegeEscalation set to false. Include a capabilities section to drop unnecessary capabilities, add a resources section with limits and requests for CPU and memory, and include livenessProbe and readinessProbe configurations to establish proper health check baselines for the container.Sources: Coding guidelines, Linters/SAST tools
test/testdata/workloads/kubejobfailed-73886.yaml-8-13 (1)
8-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winJob container is missing required security context and resource constraints.
Please add
securityContexthardening (runAsNonRoot,readOnlyRootFilesystem,allowPrivilegeEscalation: false, capabilities dropALL) plus CPU/memory limits for the container.As per coding guidelines, "securityContext: runAsNonRoot, readOnlyRootFilesystem, allowPrivilegeEscalation: false", "Drop ALL capabilities", and "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 `@test/testdata/workloads/kubejobfailed-73886.yaml` around lines 8 - 13, The container named "fail" is missing required security hardening and resource constraints. Add a securityContext block to the container with runAsNonRoot set to true, readOnlyRootFilesystem set to true, allowPrivilegeEscalation set to false, and capabilities with drop set to ALL. Additionally, add a resources block with limits specifying both CPU and memory limits to the container specification to ensure proper resource management.Sources: Coding guidelines, Linters/SAST tools
test/testdata/workloads/deployment-73887.yaml-15-23 (1)
15-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winContainer hardening settings are missing in this fixture manifest.
Add
securityContext(runAsNonRoot: true,readOnlyRootFilesystem: true,allowPrivilegeEscalation: false, dropALLcapabilities) and include CPU resources alongside memory to satisfy policy and avoid admission/test-environment drift.As per coding guidelines, "securityContext: runAsNonRoot, readOnlyRootFilesystem, allowPrivilegeEscalation: false", "Drop ALL capabilities", and "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 `@test/testdata/workloads/deployment-73887.yaml` around lines 15 - 23, The busybox container in the deployment manifest is missing essential security hardening settings and CPU resource specifications. Add a securityContext block to the container spec that includes runAsNonRoot set to true, readOnlyRootFilesystem set to true, allowPrivilegeEscalation set to false, and a capabilities field with drop containing ALL. Additionally, augment the existing resources section with CPU requests and limits alongside the memory specifications to ensure complete resource constraints are defined for the container.Sources: Coding guidelines, Linters/SAST tools
test/e2e/kcm_controller.go-232-234 (1)
232-234:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
masters[0]can panic on topologies with no master-labeled nodes.Guard
len(masters)before indexing and skip/branch appropriately for non-traditional control-plane topologies.As per coding guidelines, topology-aware logic should account for SNO/Two-Node/HyperShift variants and avoid assumptions tied to specific node-role layouts.
🤖 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 `@test/e2e/kcm_controller.go` around lines 232 - 234, The code accesses masters[0] without verifying that the masters slice contains at least one element, which will cause a panic on clusters with no master-labeled nodes (such as SNO, Two-Node, or HyperShift topologies). Add a length check on the masters slice returned from testlib.GetClusterNodesBy before indexing into it, and implement appropriate branching logic to skip the patch operation or select an alternative node when no master nodes are found. This ensures the test handles topology-aware scenarios correctly.Source: Coding guidelines
test/testdata/workloads/project-69072.yaml-5-5 (1)
5-5:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftHard-coded worker node selector is not topology-safe for all OpenShift footprints.
This fixture assumes schedulable worker nodes exist. In SNO/Two-Node/HyperShift-style environments, that assumption can make this test unschedulable. Gate this by topology checks in test logic or parameterize selector behavior.
As per coding guidelines, "verify topology-aware scheduling compatibility ... account for ... SNO, Two-Node Fixed, Two-Node with Arbiter, and HyperShift" and flag assumptions about dedicated worker nodes unless topology-aware checks exist.
🤖 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 `@test/testdata/workloads/project-69072.yaml` at line 5, The hard-coded worker node selector in the openshift.io/node-selector annotation assumes dedicated worker nodes exist, which is not valid for SNO, Two-Node, or HyperShift OpenShift footprints. Either remove or conditionally apply this node selector based on topology checks in the test logic (to verify worker nodes are available), or parameterize the selector so it can be configured differently depending on the cluster topology being tested. Ensure the fixture can run successfully on all supported OpenShift deployment models without assuming dedicated worker node availability.Source: Coding guidelines
test/e2e/kcm_alerts.go-48-54 (1)
48-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCleanup command errors are dropped in disruptive test paths.
Ignoring
uncordon/delete errors can leave nodes/resources dirty and cascade failures into subsequent specs. Capture and assert these errors (including indeferblocks).As per coding guidelines, "Never ignore error returns".
Also applies to: 65-65, 78-78
🤖 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 `@test/e2e/kcm_alerts.go` around lines 48 - 54, The cleanup code in the defer block is ignoring error returns from the uncordon operations and the testlib.CheckNodeUncordoned function calls by using underscore assignments. Capture the error returns from both the Execute() method call on the uncordon command and the CheckNodeUncordoned() function calls instead of discarding them with underscores, then add proper error handling or assertions to ensure these cleanup operations succeed. This applies to all instances in the disruptive test paths (including the locations at lines 65 and 78) where cleanup operations are performed.Sources: Coding guidelines, Linters/SAST tools
test/library/helpers.go-132-138 (1)
132-138:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
CheckMetricmasks command failures and only surfaces a generic timeout.When
RemoteShPoderrors, the poll callback returns(false, nil), hiding auth/network failures for up totimeoutSec. Return the underlying error (or wrap it) so failures are actionable.🤖 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 `@test/library/helpers.go` around lines 132 - 138, The CheckMetric function's wait.Poll callback masks actual errors from RemoteShPod by returning (false, nil) when an error occurs, causing the poll to retry until timeout instead of exposing the underlying issue. Modify the callback function to return the actual error from RemoteShPod (either directly or wrapped with additional context) in the second return value instead of nil, so auth/network failures and other errors are surfaced immediately rather than hidden behind a generic timeout message.Source: Linters/SAST tools
test/library/oc.go-83-83 (1)
83-83:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCommand logging currently leaks sensitive CLI arguments (including bearer-token commands).
run()prints full args. Cross-file,CheckMetric()builds a command containingAuthorization:Bearer <token>, so this can expose credentials in test logs.🤖 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 `@test/library/oc.go` at line 83, The run() function currently logs all command arguments without sanitization, which exposes sensitive information like bearer tokens in test logs. Modify the logging statement where fmt.Fprintf is called to filter out or mask sensitive CLI arguments (such as Authorization headers and bearer tokens) before printing the command. Create a helper function that removes or redacts sensitive arguments from the command slice, then use this filtered version in the fmt.Fprintf call while keeping the original full command for actual execution.test/e2e/kcm_controller.go-90-90 (1)
90-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeferred cleanup errors are ignored across multiple disruptive specs.
These paths should assert cleanup success; otherwise taints/projects/resources can leak and destabilize later tests.
As per coding guidelines, "Never ignore error returns".
Also applies to: 105-105, 129-129, 139-139, 196-196, 224-224, 227-227
🤖 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 `@test/e2e/kcm_controller.go` at line 90, The deferred cleanup operations in the test file are ignoring errors returned from the Execute() method calls on the oc.WithoutNamespace().Run() chains. You need to capture the error returned by Execute() in each defer statement and assert or check that it is nil to ensure cleanup succeeds. Apply this fix to all the deferred cleanup statements mentioned: lines 90, 105, 129, 139, 196, 224, and 227, where each defer statement contains an Execute() call that currently ignores its return value. This ensures that if cleanup fails, the test will fail rather than allowing resource leaks to destabilize subsequent tests.Sources: Coding guidelines, Linters/SAST tools
test/library/helpers.go-217-223 (1)
217-223:⚠️ Potential issue | 🟠 Major
time.LoadLocation("")returns UTC, not the local timezone.The function uses
time.LoadLocation("")which returns the UTC location, not the host's local timezone. Usetime.Localdirectly instead:currentTime := time.Now().In(time.Local).🤖 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 `@test/library/helpers.go` around lines 217 - 223, In the GetTimeFromTimezone function, the call to time.LoadLocation("") returns UTC location instead of the host's local timezone. Remove the time.LoadLocation("") call and the associated error handling, and instead directly use time.Local when calling time.Now().In() to properly get the current time in the local timezone.test/library/oc.go-77-86 (1)
77-86:⚠️ Potential issue | 🟠 MajorAdd context timeout/cancellation to
occommand execution.The
exec.Commandcall at line 84 lacks timeout protection and can hang tests indefinitely. Useexec.CommandContextwith a bounded context instead per Go security guidelines.c := exec.CommandContext(ctx, "oc", args...)Note: Similar patterns exist elsewhere in test code (e.g.,
test/library/helpers.go:113) that should also be addressed.🤖 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 `@test/library/oc.go` around lines 77 - 86, The ocCommand.run() method uses exec.Command without any timeout protection, which can cause tests to hang indefinitely. Replace the exec.Command call with exec.CommandContext, passing a properly bounded context with a timeout as the first parameter. The context should be created before the CommandContext call to ensure all command execution is subject to a timeout duration that prevents indefinite hangs during test execution.Sources: Coding guidelines, Linters/SAST tools
test/library/helpers.go-111-116 (1)
111-116:⚠️ Potential issue | 🟠 MajorAdd context and timeout to
oc debugexecution.The
exec.Commandcall lacks cancellation/timeout handling. If theoc debugcommand stalls, it will block the entire test process. Switch toexec.CommandContextwith an appropriate timeout.🤖 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 `@test/library/helpers.go` around lines 111 - 116, The DebugNodeWithChroot function uses exec.Command without timeout handling, which can cause the test to hang if the oc debug command stalls. Replace the exec.Command call with exec.CommandContext, passing a context created with context.WithTimeout to provide an appropriate timeout duration. This ensures the command will be automatically cancelled if it exceeds the timeout, preventing the test process from being blocked indefinitely.Sources: Coding guidelines, Linters/SAST tools
🟡 Minor comments (1)
test/library/helpers.go-247-255 (1)
247-255:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFile close/remove errors are ignored in template application flow.
If
tmp.Close()fails,oc applymay consume incomplete content; ignoredos.Removefailures can leak temp files and hide I/O issues.As per coding guidelines, "Never ignore error returns".
🤖 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 `@test/library/helpers.go` around lines 247 - 255, In the template application flow, the errors returned by tmp.Close() and os.Remove() are being ignored. Check the error returned by tmp.Close() after closing the temporary file and handle it appropriately (this error indicates if the file contents were successfully written). Additionally, capture and handle the error returned by os.Remove() in the defer statement to prevent silent failures that could leak temporary files and hide I/O issues. Both error checks should follow the "Never ignore error returns" coding guideline by either logging them or returning them as part of the function's error handling.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 `@test/e2e/kcm_alerts.go`:
- Around line 27-33: The namespace `ns` is being retrieved via `oc.Namespace()`
before the project is initialized, resulting in an empty namespace value being
used in the subsequent `create` deployment and `create` poddisruptionbudget
commands. Call `SetupProject()` before calling `oc.Namespace()` to ensure the
namespace is properly initialized, or alternatively use an explicitly defined
namespace string instead of relying on `oc.Namespace()` at this point in the
test execution.
In `@test/e2e/kcm_controller.go`:
- Around line 243-251: The testTerminatingPodRemovedFromEndpoints function reads
the namespace via oc.Namespace() before initializing a project, resulting in an
empty namespace value that is passed to WaitForAvailableRsRunning and causes
misleading failures. After creating the OC client with testlib.NewOC(),
initialize a new project first (typically using oc.Run("new-project") or similar
project creation logic) before calling oc.Namespace() to ensure the namespace is
properly populated, then use that initialized namespace consistently in the
WaitForAvailableRsRunning call.
In `@test/library/oc.go`:
- Around line 93-99: The randomString() function generates deterministic values
because it uses os.Getpid() which remains constant throughout a single test
process, causing collisions across multiple SetupProject() calls. Replace the
current implementation that indexes into the chars string using
(os.Getpid()+i)%len(chars) with actual random number generation using the
math/rand or crypto/rand package to ensure each call generates a unique string,
preventing AlreadyExists errors from oc new-project calls.
In `@test/testdata/workloads/deploy_duplicatepodsrs.yaml`:
- Around line 27-35: The YAML file contains critical indentation errors where
the `requests`, `env`, and `ports` fields are at the same indentation level as
`containers` instead of being nested under the container item specification. Fix
this by properly indenting these three sections (resources with requests, env,
and ports) to be direct children of the container object, ensuring they are
indented further than the container item they belong to. This will allow the
YAML to parse correctly and apply these configurations to the container.
---
Major comments:
In `@test/e2e/kcm_alerts.go`:
- Around line 48-54: The cleanup code in the defer block is ignoring error
returns from the uncordon operations and the testlib.CheckNodeUncordoned
function calls by using underscore assignments. Capture the error returns from
both the Execute() method call on the uncordon command and the
CheckNodeUncordoned() function calls instead of discarding them with
underscores, then add proper error handling or assertions to ensure these
cleanup operations succeed. This applies to all instances in the disruptive test
paths (including the locations at lines 65 and 78) where cleanup operations are
performed.
In `@test/e2e/kcm_controller.go`:
- Around line 232-234: The code accesses masters[0] without verifying that the
masters slice contains at least one element, which will cause a panic on
clusters with no master-labeled nodes (such as SNO, Two-Node, or HyperShift
topologies). Add a length check on the masters slice returned from
testlib.GetClusterNodesBy before indexing into it, and implement appropriate
branching logic to skip the patch operation or select an alternative node when
no master nodes are found. This ensures the test handles topology-aware
scenarios correctly.
- Line 90: The deferred cleanup operations in the test file are ignoring errors
returned from the Execute() method calls on the oc.WithoutNamespace().Run()
chains. You need to capture the error returned by Execute() in each defer
statement and assert or check that it is nil to ensure cleanup succeeds. Apply
this fix to all the deferred cleanup statements mentioned: lines 90, 105, 129,
139, 196, 224, and 227, where each defer statement contains an Execute() call
that currently ignores its return value. This ensures that if cleanup fails, the
test will fail rather than allowing resource leaks to destabilize subsequent
tests.
In `@test/e2e/kcm_cronjob.go`:
- Around line 45-47: The test assertions for the oc debug cronjob operation
contradict the expected failure behavior. In the debugErr and err handling block
near the cronjob/cronjob56176 operation, invert the assertions: change the first
o.Expect(err).NotTo(o.HaveOccurred()) to expect an error to occur, and change
the o.Expect(debugErr).NotTo(o.ContainSubstring("v1.CronJob is not supported by
debug")) to expect that the error message DOES contain the unsupported message.
This aligns the test to properly validate the failure path for unsupported
cronjob debug operations.
- Around line 132-134: The stringsContains function is using regexp.MatchString
but ignoring its error return value, which violates Go security guidelines.
Since this function is only performing literal substring searches (not regex
pattern matching), replace the regexp.MatchString call with strings.Contains
instead. This eliminates the error that is being ignored and provides a simpler,
safer implementation for the actual use case of searching for literal strings
like "cronjob54195" and "cronjob54196".
In `@test/e2e/kcm_operator.go`:
- Around line 164-173: The testKCMNoRestartParallelStatefulSet function applies
a StatefulSet workload with a fixed 70-replica count from statefulset-76130.yaml
without checking the cluster topology first, which causes scheduling timeouts on
resource-constrained topologies like SNO, two-node, and HyperShift. Before
creating the StatefulSet with the Run and Execute calls, add a topology-aware
guard to skip or modify this workload based on the cluster footprint. Check the
cluster topology (SNO, two-node fixed, two-node with arbiter, or HyperShift) and
either skip the creation of this specific workload or adjust the replica count
accordingly to ensure the test only runs the 70-replica stress case on
topologies that can handle it.
- Line 109: The deferred call to testlib.DebugNodeWithChroot is ignoring its
error return value, which violates the Go coding guidelines that require
checking all error returns. Wrap the testlib.DebugNodeWithChroot call in a
deferred anonymous function that captures and checks the error return value, and
ensure any returned error is properly logged or handled to prevent silent
failures during kube-apiserver manifest rollback that could leave the node in a
broken state.
- Around line 210-213: The deferred cleanup code in the node-manifest
restoration loop is ignoring the error returned by Execute() using the blank
identifier pattern. Remove the blank identifier assignment and instead capture
the error return value from the Execute() call on the chained command
(oc.AsAdmin().WithoutNamespace().Run("debug").Args(...).Execute()). Check the
error and log it appropriately to ensure that any cleanup failures during node
restoration are visible rather than silently swallowed, preventing the test
suite from being destabilized by undetected control-plane mutations.
In `@test/library/helpers.go`:
- Around line 132-138: The CheckMetric function's wait.Poll callback masks
actual errors from RemoteShPod by returning (false, nil) when an error occurs,
causing the poll to retry until timeout instead of exposing the underlying
issue. Modify the callback function to return the actual error from RemoteShPod
(either directly or wrapped with additional context) in the second return value
instead of nil, so auth/network failures and other errors are surfaced
immediately rather than hidden behind a generic timeout message.
- Around line 217-223: In the GetTimeFromTimezone function, the call to
time.LoadLocation("") returns UTC location instead of the host's local timezone.
Remove the time.LoadLocation("") call and the associated error handling, and
instead directly use time.Local when calling time.Now().In() to properly get the
current time in the local timezone.
- Around line 111-116: The DebugNodeWithChroot function uses exec.Command
without timeout handling, which can cause the test to hang if the oc debug
command stalls. Replace the exec.Command call with exec.CommandContext, passing
a context created with context.WithTimeout to provide an appropriate timeout
duration. This ensures the command will be automatically cancelled if it exceeds
the timeout, preventing the test process from being blocked indefinitely.
In `@test/library/oc.go`:
- Line 83: The run() function currently logs all command arguments without
sanitization, which exposes sensitive information like bearer tokens in test
logs. Modify the logging statement where fmt.Fprintf is called to filter out or
mask sensitive CLI arguments (such as Authorization headers and bearer tokens)
before printing the command. Create a helper function that removes or redacts
sensitive arguments from the command slice, then use this filtered version in
the fmt.Fprintf call while keeping the original full command for actual
execution.
- Around line 77-86: The ocCommand.run() method uses exec.Command without any
timeout protection, which can cause tests to hang indefinitely. Replace the
exec.Command call with exec.CommandContext, passing a properly bounded context
with a timeout as the first parameter. The context should be created before the
CommandContext call to ensure all command execution is subject to a timeout
duration that prevents indefinite hangs during test execution.
In `@test/testdata/workloads/cronjob_54196.yaml`:
- Around line 16-26: The CronJob template in the containers spec is missing
required security defaults. For the container named "hello", add a
securityContext object with runAsNonRoot set to true, readOnlyRootFilesystem set
to true, and allowPrivilegeEscalation set to false. Additionally add a
capabilities section under securityContext that drops ALL capabilities. Finally,
add a resources object with defined limits for both CPU and memory to ensure
resource constraints are enforced on every rendered CronJob instance.
In `@test/testdata/workloads/cronjob54195.yaml`:
- Around line 11-20: The CronJob manifest is missing required security hardening
controls in the container specification. Add a securityContext block to the
hello container with the following settings: set runAsNonRoot to true,
readOnlyRootFilesystem to true, and allowPrivilegeEscalation to false, and
include a capabilities section that drops ALL capabilities. Additionally, add a
resources section with defined CPU and memory limits to the container
specification. These settings should be applied at the container level within
the containers array in the spec.template.spec section of the CronJob manifest.
In `@test/testdata/workloads/cronjob54195ic.yaml`:
- Around line 11-20: In the container spec for the "hello" container within the
cronjob54195ic.yaml fixture, add mandatory security hardening controls and
resource limits according to repo policy. Add a securityContext section
specifying runAsNonRoot as true, readOnlyRootFilesystem as true, and
allowPrivilegeEscalation as false. Include a capabilities drop directive to drop
ALL capabilities. Additionally, add a resources section with defined CPU and
memory limits for the container. These hardening controls must match the
security standards applied elsewhere in the repository to ensure consistent
pod/container security across all Kubernetes/OpenShift manifests.
In `@test/testdata/workloads/cronjob54195notz.yaml`:
- Around line 10-19: Add security hardening and resource constraints to the
container specification in the cronjob54195notz.yaml CronJob fixture. Within the
container definition for the "hello" container (after the imagePullPolicy
field), add a securityContext block that sets runAsNonRoot to true,
readOnlyRootFilesystem to true, allowPrivilegeEscalation to false, and includes
capabilities with drop set to ALL. Additionally, add a resources section with
limits for both CPU and memory to enforce resource constraints on the container.
These additions should be at the same indentation level as the command and
imagePullPolicy fields.
In `@test/testdata/workloads/cronjob56176.yaml`:
- Around line 18-23: The container named "pi" in the CronJob pod template is
missing required security hardening and resource constraints. Add a
securityContext block to the container spec with runAsNonRoot set to true,
readOnlyRootFilesystem set to true, and allowPrivilegeEscalation set to false.
Also add a capabilities section under securityContext that drops ALL
capabilities. Additionally, add a resources section with both limits and
requests defined for CPU and memory (e.g., limits and requests for cpu and
memory fields) to ensure proper resource allocation and prevent resource
exhaustion.
In `@test/testdata/workloads/cronjob75375.yaml`:
- Around line 19-24: The CronJob definition for pi75375 is missing required
security hardening and resource constraints. Add a securityContext at the pod
spec level with runAsNonRoot set to true, and add a securityContext to the pi
container with readOnlyRootFilesystem set to true, allowPrivilegeEscalation set
to false, and capabilities.drop set to ALL. Additionally, add resources section
to the pi container with both requests (cpu and memory) and limits (cpu and
memory) to comply with restricted security policies and ensure proper resource
management.
- Line 22: Replace the mutable image reference `image: perl` in the
cronjob75375.yaml file with an immutable reference using a specific image digest
(SHA256 hash). Instead of using just the tag `perl`, pin the image by appending
`@sha256:` followed by the specific digest, matching the pattern used in other
workload fixture files in the same directory. This ensures reproducible test
behavior and reduces supply-chain risk.
In `@test/testdata/workloads/deployment-73887.yaml`:
- Around line 15-23: The busybox container in the deployment manifest is missing
essential security hardening settings and CPU resource specifications. Add a
securityContext block to the container spec that includes runAsNonRoot set to
true, readOnlyRootFilesystem set to true, allowPrivilegeEscalation set to false,
and a capabilities field with drop containing ALL. Additionally, augment the
existing resources section with CPU requests and limits alongside the memory
specifications to ensure complete resource constraints are defined for the
container.
In `@test/testdata/workloads/kubejobfailed-73886.yaml`:
- Around line 8-13: The container named "fail" is missing required security
hardening and resource constraints. Add a securityContext block to the container
with runAsNonRoot set to true, readOnlyRootFilesystem set to true,
allowPrivilegeEscalation set to false, and capabilities with drop set to ALL.
Additionally, add a resources block with limits specifying both CPU and memory
limits to the container specification to ensure proper resource management.
In `@test/testdata/workloads/project-69072.yaml`:
- Line 5: The hard-coded worker node selector in the openshift.io/node-selector
annotation assumes dedicated worker nodes exist, which is not valid for SNO,
Two-Node, or HyperShift OpenShift footprints. Either remove or conditionally
apply this node selector based on topology checks in the test logic (to verify
worker nodes are available), or parameterize the selector so it can be
configured differently depending on the cluster topology being tested. Ensure
the fixture can run successfully on all supported OpenShift deployment models
without assuming dedicated worker node availability.
In `@test/testdata/workloads/statefulset-76130.yaml`:
- Around line 31-40: The hello-openshift container specification lacks the
required security hardening controls mandated by the repository's Kubernetes
manifest policy. Add a securityContext block to the container with runAsNonRoot
set to true, readOnlyRootFilesystem set to true, and allowPrivilegeEscalation
set to false. Include a capabilities section to drop unnecessary capabilities,
add a resources section with limits and requests for CPU and memory, and include
livenessProbe and readinessProbe configurations to establish proper health check
baselines for the container.
In `@test/testdata/workloads/statefulset-79105.yaml`:
- Around line 24-41: The container named hello-pod in this StatefulSet YAML has
incomplete security controls with empty stubs for resources and securityContext,
and lacks required probes. Replace the empty resources block with proper memory
and CPU limits/requests, populate the securityContext with
allowPrivilegeEscalation set to false, add runAsNonRoot: true,
readOnlyRootFilesystem: true, drop all capabilities, and add livenessProbe and
readinessProbe sections to the container spec to comply with repository security
and operational guidelines.
---
Minor comments:
In `@test/library/helpers.go`:
- Around line 247-255: In the template application flow, the errors returned by
tmp.Close() and os.Remove() are being ignored. Check the error returned by
tmp.Close() after closing the temporary file and handle it appropriately (this
error indicates if the file contents were successfully written). Additionally,
capture and handle the error returned by os.Remove() in the defer statement to
prevent silent failures that could leak temporary files and hide I/O issues.
Both error checks should follow the "Never ignore error returns" coding
guideline by either logging them or returning them as part of the function's
error handling.
🪄 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: cc0e7fba-0897-49fe-95f5-54bfe210f35a
📒 Files selected for processing (23)
test/e2e/kcm_alerts.gotest/e2e/kcm_controller.gotest/e2e/kcm_cronjob.gotest/e2e/kcm_operator.gotest/library/helpers.gotest/library/oc.gotest/testdata/workloads/cronjob54195.yamltest/testdata/workloads/cronjob54195ic.yamltest/testdata/workloads/cronjob54195notz.yamltest/testdata/workloads/cronjob56176.yamltest/testdata/workloads/cronjob75375.yamltest/testdata/workloads/cronjob_54196.yamltest/testdata/workloads/daemonset-origin.yamltest/testdata/workloads/daemonset-update.yamltest/testdata/workloads/deploy_duplicatepodsrs.yamltest/testdata/workloads/deployment-69072.yamltest/testdata/workloads/deployment-73887.yamltest/testdata/workloads/deployment-with-shutdown-gracefully.yamltest/testdata/workloads/kubejobfailed-73886.yamltest/testdata/workloads/project-69072.yamltest/testdata/workloads/statefulset-76130.yamltest/testdata/workloads/statefulset-79105.yamltest/testdata/workloads/statefulset_63694.yaml
gangwgr
force-pushed
the
mugrate-otp-cases-ote
branch
from
3a430fc to
e63831d
Compare
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (9)
test/testdata/workloads/deployment-73887.yaml (1)
19-23: ⚡ Quick winAdd CPU limits to resource specification.
The container defines memory requests and limits, but is missing CPU limits. Add
limits.cputo complete the resource specification.📊 Suggested resource limits addition
resources: requests: + cpu: 100m memory: "2Gi" limits: + cpu: 100m memory: "2Gi"🤖 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 `@test/testdata/workloads/deployment-73887.yaml` around lines 19 - 23, In the resources section of the deployment specification where memory requests and limits are defined, add a cpu field under the limits section to complement the existing memory limits. The limits object should include both memory (which is already present at "2Gi") and cpu with an appropriate value (commonly "500m" or "1000m" depending on your resource requirements). This ensures complete resource constraints are specified for the container.Source: Coding guidelines
test/testdata/workloads/daemonset-update.yaml (1)
27-32: ⚡ Quick winAdd CPU limits to resource specification.
The container defines memory requests and limits, but is missing CPU limits. Add
limits.cputo complete the resource specification for predictable test resource consumption.📊 Suggested resource limits addition
resources: limits: + cpu: 100m memory: 200Mi requests: cpu: 100m memory: 200Mi🤖 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 `@test/testdata/workloads/daemonset-update.yaml` around lines 27 - 32, The resources specification in the container definition is missing CPU limits. While memory limits and both CPU and memory requests are defined, the limits section only contains memory. Add a cpu field under the limits section (alongside the existing memory: 200Mi) to complete the resource specification. This ensures both CPU and memory have explicit limits for predictable test resource consumption.Source: Coding guidelines
test/testdata/workloads/daemonset-origin.yaml (2)
27-32: ⚡ Quick winAdd CPU limits to resource specification.
The container defines memory requests and limits, but is missing CPU limits. Add
limits.cputo complete the resource specification for predictable test resource consumption.📊 Suggested resource limits addition
resources: limits: + cpu: 100m memory: 200Mi requests: cpu: 100m memory: 200Mi🤖 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 `@test/testdata/workloads/daemonset-origin.yaml` around lines 27 - 32, The resource specification in the DaemonSet definition is incomplete as it lacks CPU limits. Add a cpu field under the limits section in the resources specification, placing it alongside the existing memory limit. This will ensure the resource constraints are fully defined with both CPU and memory limits, providing predictable test resource consumption.Source: Coding guidelines
1-33: ⚡ Quick winConsider adding security context to test fixture.
Define a
securityContextwithallowPrivilegeEscalation: falseandrunAsNonRoot: trueto align with Kubernetes security best practices, even for test fixtures.🤖 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 `@test/testdata/workloads/daemonset-origin.yaml` around lines 1 - 33, The DaemonSet test fixture is missing security context configuration that aligns with Kubernetes security best practices. Add a securityContext block to the hello-openshift container spec that includes allowPrivilegeEscalation set to false and runAsNonRoot set to true. This securityContext should be defined at the same level as the name, image, and resources fields within the container specification.Source: Coding guidelines
test/testdata/workloads/cronjob56176.yaml (1)
1-23: ⚡ Quick winConsider adding security context and resource limits to test fixture.
While this is a test fixture, Kubernetes best practices recommend defining
securityContextwithallowPrivilegeEscalation: falseandrunAsNonRoot: true, plus resourcerequestsandlimitsfor predictable test behavior.🤖 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 `@test/testdata/workloads/cronjob56176.yaml` around lines 1 - 23, Add security context and resource limits to the test fixture cronjob56176 to follow Kubernetes best practices. In the container specification for the "pi" container, add a securityContext section with allowPrivilegeEscalation set to false and runAsNonRoot set to true. Additionally, add a resources section with both requests and limits for CPU and memory to ensure predictable test behavior. These should be added as sibling properties to the existing image and command properties under the pi container definition.Source: Coding guidelines
test/testdata/workloads/deployment-with-shutdown-gracefully.yaml (1)
41-41: ⚡ Quick winConsider adding security context and resource limits.
This test fixture should define
securityContextwithallowPrivilegeEscalation: falseand resource requests/limits, even though the focus is on shutdown behavior. This ensures consistent security posture across all test workloads.Also applies to: 47-47
🤖 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 `@test/testdata/workloads/deployment-with-shutdown-gracefully.yaml` at line 41, The test fixture in the deployment-with-shutdown-gracefully.yaml file has an empty resources object that should be populated with resource requests and limits, and is missing a securityContext definition. Replace the empty `resources: {}` with actual resource requests and limits for both CPU and memory, and add a `securityContext` section at the same indentation level as `resources` that includes `allowPrivilegeEscalation: false` to ensure consistent security posture across all test workloads, even though the primary focus of this test is shutdown behavior.Source: Coding guidelines
test/testdata/workloads/kubejobfailed-73886.yaml (1)
1-13: ⚡ Quick winConsider adding resource limits and security context.
This test fixture should define resource requests/limits and a
securityContextwithallowPrivilegeEscalation: falseto align with Kubernetes security best practices and provide consistent test resource constraints.🤖 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 `@test/testdata/workloads/kubejobfailed-73886.yaml` around lines 1 - 13, The Kubernetes Job manifest in this test fixture is missing resource constraints and security hardening settings. Add a resources section to the fail container with appropriate requests and limits (e.g., CPU and memory), and add a securityContext field to the container spec with allowPrivilegeEscalation set to false. These additions should be placed within the container specification alongside the existing name, image, and command fields to align with Kubernetes security best practices.Source: Coding guidelines
test/testdata/workloads/deployment-69072.yaml (2)
30-36: ⚖️ Poor tradeoffVerify topology spread constraints work with test cluster topology.
This Deployment uses
topologySpreadConstraintswithwhenUnsatisfiable: DoNotSchedule, which prevents scheduling if there aren't enough unique hostnames formaxSkew: 1. Verify this works reliably on the test cluster topology (SNO, two-node, etc.) or consider usingDoNotScheduleWhenUnsatisfiableonly if testing topology-aware scheduling.🤖 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 `@test/testdata/workloads/deployment-69072.yaml` around lines 30 - 36, Verify that the topologySpreadConstraints configuration in the deployment (with maxSkew: 1, topologyKey: kubernetes.io/hostname, and whenUnsatisfiable: DoNotSchedule) is compatible with all test cluster topologies your tests run against (single-node, two-node, multi-node). If the constraint is too strict for certain cluster configurations, consider adjusting the whenUnsatisfiable policy or maxSkew value to ensure the Deployment can be scheduled reliably across all test environments, or document which specific cluster topologies this test requires.Source: Coding guidelines
46-46: ⚡ Quick winAdd resource limits and security context.
The container uses
resources: {}andsecurityContext: {}, leaving both empty. Define resource requests/limits and a security context withallowPrivilegeEscalation: falsefor test reliability and security best practices.Also applies to: 52-52
🤖 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 `@test/testdata/workloads/deployment-69072.yaml` at line 46, The container definition in the deployment test file has empty resources and securityContext configurations that should be populated for security and reliability. Replace the empty `resources: {}` with actual resource requests and limits (define appropriate CPU and memory values), and replace the empty `securityContext: {}` with a proper security context that includes `allowPrivilegeEscalation: false` along with other necessary security settings. Apply these changes to both occurrences of empty resources and securityContext configurations in the file.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.
Inline comments:
In `@test/e2e/kcm_alerts.go`:
- Around line 49-55: The defer cleanup block is discarding errors from the
Execute() method call when uncordoning nodes and from the CheckNodeUncordoned()
function call by using blank identifiers instead of capturing them. Replace the
blank identifier assignments with actual error variable captures (e.g., err :=
instead of _ =) and handle each error appropriately by logging or returning it
so that cleanup failures do not silently contaminate subsequent tests. Apply
this fix to both the uncordon Execute() call and the CheckNodeUncordoned() call
in the defer block, as well as to the additional instances mentioned at lines 66
and 79.
In `@test/e2e/kcm_controller.go`:
- Line 90: The deferred cleanup operations are not checking the error returned
by the Execute() method calls, which can cause cleanup failures to go unnoticed
and leave resources like projects behind. For all deferred cleanup statements
(including the one with Run("delete").Args("project", "p43092-1").Execute() and
others at the mentioned lines), capture the error returned by Execute() and
handle it appropriately by checking if it is non-nil and logging or handling the
error to ensure cleanup failures are properly reported.
- Around line 232-234: The `masters[0]` indexing operation can cause a panic if
the slice returned by `testlib.GetClusterNodesBy(oc, "master")` is empty. Add a
guard condition immediately after the `masters` variable is assigned to check if
the slice has at least one element before attempting to access `masters[0]`. If
the slice is empty, handle it appropriately (such as returning an error or
skipping the test step).
In `@test/e2e/kcm_operator.go`:
- Line 109: The defer statement calling testlib.DebugNodeWithChroot for manifest
restoration is ignoring the error return value, which violates the coding
guideline to never ignore error returns. Capture the error returned by the
testlib.DebugNodeWithChroot function call and verify or assert it within the
defer block to ensure the restoration operation succeeds. Apply this same fix to
all similar deferred manifest restoration calls in the file (also at lines
210-213), ensuring no error returns are ignored during cleanup paths.
In `@test/library/helpers.go`:
- Around line 150-151: Replace the regexp.MatchString call in the matched
variable assignment with exact string equality comparison. After trimming
whitespace from both the expected and output values, compare them directly for
exact equality instead of using regex pattern matching. This ensures that
partial matches like "5" matching "15" do not occur, providing accurate replica
count comparisons.
- Line 247: In the template application code, the tmp.Close() call at line 247
and the deferred os.Remove() call at line 254 are ignoring error returns.
Capture the error returned by tmp.Close() and handle it appropriately (either
log it or return it). Additionally, modify the deferred os.Remove() to capture
and handle any errors it returns instead of silently ignoring them, ensuring
that write flush failures and temp-file cleanup problems are properly surfaced
rather than hidden.
- Around line 111-116: The DebugNodeWithChroot function uses exec.Command
without any timeout or cancellation mechanism, which can cause the test suite to
hang indefinitely if the oc debug command gets stuck. Replace the exec.Command
call with exec.CommandContext and pass it a context with an appropriate timeout
value. This will ensure that long-running or stuck oc debug commands are
properly cancelled and do not block the entire test suite.
In `@test/library/oc.go`:
- Line 85: The `run()` function logs the complete command line with
`fmt.Fprintf(g.GinkgoWriter, "running: %s\n", strings.Join(full, " "))`, which
exposes sensitive credentials like Bearer tokens that may be present in command
arguments built by `CheckMetric()`. Before logging the command, sanitize or
filter the `full` argument slice to remove or mask sensitive arguments
(specifically Authorization headers with Bearer tokens) so that credentials are
not exposed in test logs.
- Around line 86-88: The exec.Command call in the run() method can block
indefinitely without timeout protection. Replace exec.Command with
exec.CommandContext to enable cancellation and timeouts. Add a context.Context
parameter to the run() method, then update all callers of run() (the Output(),
Execute(), and Outputs() methods) to accept and pass through a context
parameter, ensuring the context is properly threaded through the entire call
chain for proper subprocess lifecycle management.
In `@test/testdata/workloads/cronjob54195.yaml`:
- Around line 11-20: The CronJob manifest is missing required security hardening
and resource constraints on the container. Add a securityContext block to the
"hello" container with runAsNonRoot set to true, readOnlyRootFilesystem set to
true, allowPrivilegeEscalation set to false, and a capabilities section that
drops ALL capabilities. Additionally, add a resources section with both requests
and limits specified for CPU and memory to ensure proper resource allocation and
prevent resource exhaustion.
In `@test/testdata/workloads/cronjob54195ic.yaml`:
- Around line 11-20: The container specification in the CronJob pod template is
missing required security hardening controls and resource limits. Add a
securityContext block to the hello container with runAsNonRoot set to true,
readOnlyRootFilesystem set to true, allowPrivilegeEscalation set to false, and
capabilities configured to drop ALL. Additionally, add a resources block with
both requests and limits specified for cpu and memory to ensure proper resource
allocation and prevent resource exhaustion. These additions should be placed at
the same indentation level as the command and imagePullPolicy fields within the
container specification.
In `@test/testdata/workloads/cronjob54195notz.yaml`:
- Around line 10-19: Add a securityContext configuration to the container named
hello in the pod template with the required hardening settings including
runAsNonRoot set to true, readOnlyRootFilesystem set to true, and
allowPrivilegeEscalation set to false. Additionally, add a capabilities section
under securityContext that drops ALL capabilities. Finally, add a resources
section to the container specifying both CPU and memory limits to ensure proper
resource constraints are enforced on this container.
In `@test/testdata/workloads/cronjob75375.yaml`:
- Line 22: The `image: perl` field on line 22 uses an untagged image reference
which can result in different versions being pulled on different test runs.
Replace the untagged `perl` image with a digest-pinned image reference in the
format `quay.io/openshifttest/perl@sha256:...` to match the pattern used in
other workload fixtures in the test suite and ensure consistent, reproducible
test behavior across runs.
In `@test/testdata/workloads/statefulset_63694.yaml`:
- Around line 15-21: The nginx container specification is missing required
hardening configurations and operational safeguards. Add a securityContext block
to the nginx container with runAsNonRoot set to true, readOnlyRootFilesystem set
to true, and allowPrivilegeEscalation set to false. Within the securityContext,
add a capabilities section that drops ALL capabilities. Additionally, add a
resources section with limits specified for both cpu and memory. Finally, add
both livenessProbe and readinessProbe blocks to define health checks for the
container, using appropriate probe handlers and timing parameters.
- Around line 5-11: The container specification in the StatefulSet is missing
required security controls and resource management configuration. Navigate to
the container spec section (around lines 16-20 as mentioned) and add the
following: a securityContext block with runAsNonRoot set to true,
readOnlyRootFilesystem set to true, and allowPrivilegeEscalation set to false; a
resources section defining both cpu and memory limits; and both livenessProbe
and readinessProbe definitions to ensure the container meets the required
security and operational guidelines.
In `@test/testdata/workloads/statefulset-76130.yaml`:
- Around line 33-40: The hello-openshift container specification in the
StatefulSet is missing required resource limits and health probes. Add a
resources section to specify cpu and memory limits for the container.
Additionally, add both livenessProbe and readinessProbe configurations to the
container spec to enable proper health monitoring and failure detection. These
probes should be appropriate for an HTTP service listening on port 80 (such as
httpGet probes targeting the web port).
- Around line 31-40: The container named hello-openshift in the containers array
is missing security hardening controls. Add a securityContext section to the
container spec that includes: runAsNonRoot set to true, readOnlyRootFilesystem
set to true, allowPrivilegeEscalation set to false, and capabilities with drop
set to ["ALL"]. These settings should be added as a direct child of the
container spec, at the same level as the existing name, image, ports, and
volumeMounts fields.
In `@test/testdata/workloads/statefulset-79105.yaml`:
- Around line 24-41: The pod and container specifications in the StatefulSet
manifest are missing required security hardening and operational baseline
configurations. Add the following to the hello-pod container securityContext:
set runAsNonRoot to true, add allowPrivilegeEscalation set to false, add
readOnlyRootFilesystem set to true, and add a capabilities section with drop set
to ["ALL"]. Additionally, add a resources section with requests and limits for
both cpu and memory to the container spec. Finally, add livenessProbe and
readinessProbe sections to the container definition with appropriate httpGet or
exec handlers pointing to the application's health check endpoint (port 8080 in
this case).
- Line 11: The revisionHistoryLimit field in the StatefulSet spec is set to -1,
which violates Kubernetes API validation that only allows zero or positive
integers for this field. Change the revisionHistoryLimit value from -1 to either
0 or a positive integer (such as 3 or 10) to comply with Kubernetes API
requirements.
---
Nitpick comments:
In `@test/testdata/workloads/cronjob56176.yaml`:
- Around line 1-23: Add security context and resource limits to the test fixture
cronjob56176 to follow Kubernetes best practices. In the container specification
for the "pi" container, add a securityContext section with
allowPrivilegeEscalation set to false and runAsNonRoot set to true.
Additionally, add a resources section with both requests and limits for CPU and
memory to ensure predictable test behavior. These should be added as sibling
properties to the existing image and command properties under the pi container
definition.
In `@test/testdata/workloads/daemonset-origin.yaml`:
- Around line 27-32: The resource specification in the DaemonSet definition is
incomplete as it lacks CPU limits. Add a cpu field under the limits section in
the resources specification, placing it alongside the existing memory limit.
This will ensure the resource constraints are fully defined with both CPU and
memory limits, providing predictable test resource consumption.
- Around line 1-33: The DaemonSet test fixture is missing security context
configuration that aligns with Kubernetes security best practices. Add a
securityContext block to the hello-openshift container spec that includes
allowPrivilegeEscalation set to false and runAsNonRoot set to true. This
securityContext should be defined at the same level as the name, image, and
resources fields within the container specification.
In `@test/testdata/workloads/daemonset-update.yaml`:
- Around line 27-32: The resources specification in the container definition is
missing CPU limits. While memory limits and both CPU and memory requests are
defined, the limits section only contains memory. Add a cpu field under the
limits section (alongside the existing memory: 200Mi) to complete the resource
specification. This ensures both CPU and memory have explicit limits for
predictable test resource consumption.
In `@test/testdata/workloads/deployment-69072.yaml`:
- Around line 30-36: Verify that the topologySpreadConstraints configuration in
the deployment (with maxSkew: 1, topologyKey: kubernetes.io/hostname, and
whenUnsatisfiable: DoNotSchedule) is compatible with all test cluster topologies
your tests run against (single-node, two-node, multi-node). If the constraint is
too strict for certain cluster configurations, consider adjusting the
whenUnsatisfiable policy or maxSkew value to ensure the Deployment can be
scheduled reliably across all test environments, or document which specific
cluster topologies this test requires.
- Line 46: The container definition in the deployment test file has empty
resources and securityContext configurations that should be populated for
security and reliability. Replace the empty `resources: {}` with actual resource
requests and limits (define appropriate CPU and memory values), and replace the
empty `securityContext: {}` with a proper security context that includes
`allowPrivilegeEscalation: false` along with other necessary security settings.
Apply these changes to both occurrences of empty resources and securityContext
configurations in the file.
In `@test/testdata/workloads/deployment-73887.yaml`:
- Around line 19-23: In the resources section of the deployment specification
where memory requests and limits are defined, add a cpu field under the limits
section to complement the existing memory limits. The limits object should
include both memory (which is already present at "2Gi") and cpu with an
appropriate value (commonly "500m" or "1000m" depending on your resource
requirements). This ensures complete resource constraints are specified for the
container.
In `@test/testdata/workloads/deployment-with-shutdown-gracefully.yaml`:
- Line 41: The test fixture in the deployment-with-shutdown-gracefully.yaml file
has an empty resources object that should be populated with resource requests
and limits, and is missing a securityContext definition. Replace the empty
`resources: {}` with actual resource requests and limits for both CPU and
memory, and add a `securityContext` section at the same indentation level as
`resources` that includes `allowPrivilegeEscalation: false` to ensure consistent
security posture across all test workloads, even though the primary focus of
this test is shutdown behavior.
In `@test/testdata/workloads/kubejobfailed-73886.yaml`:
- Around line 1-13: The Kubernetes Job manifest in this test fixture is missing
resource constraints and security hardening settings. Add a resources section to
the fail container with appropriate requests and limits (e.g., CPU and memory),
and add a securityContext field to the container spec with
allowPrivilegeEscalation set to false. These additions should be placed within
the container specification alongside the existing name, image, and command
fields to align with Kubernetes security best practices.
🪄 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: 44b45bcc-b116-4cb8-900e-a506a6d56065
📒 Files selected for processing (23)
test/e2e/kcm_alerts.gotest/e2e/kcm_controller.gotest/e2e/kcm_cronjob.gotest/e2e/kcm_operator.gotest/library/helpers.gotest/library/oc.gotest/testdata/workloads/cronjob54195.yamltest/testdata/workloads/cronjob54195ic.yamltest/testdata/workloads/cronjob54195notz.yamltest/testdata/workloads/cronjob56176.yamltest/testdata/workloads/cronjob75375.yamltest/testdata/workloads/cronjob_54196.yamltest/testdata/workloads/daemonset-origin.yamltest/testdata/workloads/daemonset-update.yamltest/testdata/workloads/deploy_duplicatepodsrs.yamltest/testdata/workloads/deployment-69072.yamltest/testdata/workloads/deployment-73887.yamltest/testdata/workloads/deployment-with-shutdown-gracefully.yamltest/testdata/workloads/kubejobfailed-73886.yamltest/testdata/workloads/project-69072.yamltest/testdata/workloads/statefulset-76130.yamltest/testdata/workloads/statefulset-79105.yamltest/testdata/workloads/statefulset_63694.yaml
✅ Files skipped from review due to trivial changes (1)
- test/testdata/workloads/cronjob_54196.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- test/testdata/workloads/project-69072.yaml
- test/testdata/workloads/deploy_duplicatepodsrs.yaml
- test/e2e/kcm_cronjob.go
| spec: | ||
| containers: | ||
| # image changed since publication (previously used registry "k8s.gcr.io") | ||
| - image: quay.io/openshifttest/nginx-alpine@sha256:0d0385b0bea406ff659154fa34a6dda34e1de62d7dd559076d5abcf0d39c553e | ||
| imagePullPolicy: IfNotPresent | ||
| name: nginx | ||
| updateStrategy: |
There was a problem hiding this comment.
Add required container security, resources, and probes.
The manifest currently omits required hardening and runtime safeguards for the container (allowPrivilegeEscalation: false, non-root/read-only fs posture, dropped capabilities, limits, and health probes).
As per coding guidelines, "securityContext: runAsNonRoot, readOnlyRootFilesystem, allowPrivilegeEscalation: false", "Drop ALL capabilities", "Resource limits (cpu, memory) on every container", and "Liveness + readiness probes defined" are 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 `@test/testdata/workloads/statefulset_63694.yaml` around lines 15 - 21, The
nginx container specification is missing required hardening configurations and
operational safeguards. Add a securityContext block to the nginx container with
runAsNonRoot set to true, readOnlyRootFilesystem set to true, and
allowPrivilegeEscalation set to false. Within the securityContext, add a
capabilities section that drops ALL capabilities. Additionally, add a resources
section with limits specified for both cpu and memory. Finally, add both
livenessProbe and readinessProbe blocks to define health checks for the
container, using appropriate probe handlers and timing parameters.
Source: Coding guidelines
There was a problem hiding this comment.
@gangwgr : Can we address this
There was a problem hiding this comment.
@YamunadeviShanmugam, here's an updated version of the container spec that addresses the flagged items (non-root, read-only root filesystem, dropped capabilities, resource limits, and probes):
containers:
# image changed since publication (previously used registry "k8s.gcr.io")
- image: quay.io/openshifttest/nginx-alpine@sha256:0d0385b0bea406ff659154fa34a6dda34e1de62d7dd559076d5abcf0d39c553e
imagePullPolicy: IfNotPresent
name: nginx
securityContext:
runAsNonRoot: true
readOnlyRootFilesystem: true
allowPrivilegeEscalation: false
capabilities:
drop:
- ALL
resources:
limits:
cpu: 200m
memory: 256Mi
requests:
cpu: 100m
memory: 128Mi
ports:
- containerPort: 80
name: web
livenessProbe:
tcpSocket:
port: web
initialDelaySeconds: 5
periodSeconds: 10
readinessProbe:
tcpSocket:
port: web
initialDelaySeconds: 5
periodSeconds: 10Note: since this file is only a test fixture used to exercise StatefulSet rolling-update behavior in the e2e suite (not a production deployment manifest), readOnlyRootFilesystem: true may need adjustment if the nginx image writes to its filesystem at startup (e.g., PID files, cache dirs) — if the test starts failing due to permission errors, drop that field or mount an emptyDir for the writable paths instead.
gangwgr
force-pushed
the
mugrate-otp-cases-ote
branch
2 times, most recently
from
32251b7 to
1d6c513
Compare
There was a problem hiding this comment.
Actionable comments posted: 15
♻️ Duplicate comments (5)
test/testdata/workloads/cronjob75375.yaml (1)
22-22:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin the container image by digest for reproducible tests.
image: perlis mutable and can change behavior across runs; use a digest-pinned image like other workload fixtures.
As per coding guidelines, "IMPORTANT: Your task is to ensure that each instruction is followed for each code segment you review."🤖 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 `@test/testdata/workloads/cronjob75375.yaml` at line 22, Replace the mutable container image reference `image: perl` at line 22 in cronjob75375.yaml with a digest-pinned image tag following the same pattern used in other workload fixtures. Instead of using just the image name, specify the full image reference including the digest hash (e.g., perl@sha256:...) to ensure reproducible test behavior across runs.Source: Coding guidelines
test/library/oc.go (1)
67-90:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftUse
exec.CommandContextand thread context throughOutput/Execute/Outputs.Line 89 still launches
ocwithout cancellation/timeout, so a hung subprocess can block the suite indefinitely.
As per coding guidelines, "context.Context for cancellation and timeouts".#!/bin/bash # Verify all subprocess launches and call chain sites that need context threading. rg -nP --type=go -C3 '\bexec\.Command\(' test/library/oc.go rg -nP --type=go -C3 '\bfunc\s+\(cmd\s+\*ocCommand\)\s+(Output|Execute|Outputs|run)\s*\(' test/library/oc.go🤖 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 `@test/library/oc.go` around lines 67 - 90, The run() method in ocCommand currently uses exec.Command without context support, preventing subprocess cancellation and timeouts. Replace the exec.Command call with exec.CommandContext and add a context.Context parameter to the run() method signature. Then update all three public methods (Output, Execute, and Outputs) to also accept a context.Context parameter and pass it through to the run() method. This enables proper cancellation and timeout handling for subprocess execution.Sources: Coding guidelines, Linters/SAST tools
test/testdata/workloads/cronjob54195notz.yaml (1)
10-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd required security context and resource limits to the container.
This pod template omits required hardening controls and CPU/memory requests+limits.
As per coding guidelines, "securityContext: runAsNonRoot, readOnlyRootFilesystem, allowPrivilegeEscalation: false", "Drop ALL capabilities, add only what is required", and "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 `@test/testdata/workloads/cronjob54195notz.yaml` around lines 10 - 19, The container named "hello" in the pod template is missing required security hardening controls and resource constraints. Add a securityContext field to the container spec with runAsNonRoot set to true, readOnlyRootFilesystem set to true, and allowPrivilegeEscalation set to false. Within the securityContext, add a capabilities section that drops all capabilities. Additionally, add a resources field with requests and limits for both cpu and memory to ensure proper resource constraints are enforced on the container.Sources: Coding guidelines, Linters/SAST tools
test/testdata/workloads/cronjob54195ic.yaml (1)
11-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply baseline security hardening and resource constraints.
This CronJob container still lacks mandatory hardening (
securityContext+ dropped capabilities) and CPU/memory requests+limits.
As per coding guidelines, "securityContext: runAsNonRoot, readOnlyRootFilesystem, allowPrivilegeEscalation: false", "Drop ALL capabilities, add only what is required", and "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 `@test/testdata/workloads/cronjob54195ic.yaml` around lines 11 - 20, The hello container in the CronJob spec is missing required security hardening and resource constraints. Add a securityContext block to the container with runAsNonRoot set to true, readOnlyRootFilesystem set to true, and allowPrivilegeEscalation set to false. Within the securityContext, add a capabilities block that drops ALL capabilities. Additionally, add a resources block with both requests and limits for cpu and memory to ensure proper resource management. These should be sibling properties to the image, imagePullPolicy, and command properties of the container specification.Sources: Coding guidelines, Linters/SAST tools
test/testdata/workloads/cronjob54195.yaml (1)
11-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd required container hardening and CPU/memory limits.
The pod template is missing
runAsNonRoot,readOnlyRootFilesystem,allowPrivilegeEscalation: false, droppingALLcapabilities, and container resource limits/requests.
As per coding guidelines, "securityContext: runAsNonRoot, readOnlyRootFilesystem, allowPrivilegeEscalation: false", "Drop ALL capabilities, add only what is required", and "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 `@test/testdata/workloads/cronjob54195.yaml` around lines 11 - 20, The container named "hello" in the CronJob pod template is missing critical security hardening and resource management configurations. Add a securityContext block to the container specification with runAsNonRoot set to true, readOnlyRootFilesystem set to true, allowPrivilegeEscalation set to false, and capabilities configured to drop ALL. Additionally, add a resources section defining both requests and limits for CPU and memory to ensure proper resource allocation and prevent resource exhaustion.Sources: Coding guidelines, Linters/SAST tools
🧹 Nitpick comments (1)
test/e2e/kcm_controller.go (1)
188-188: Migrate from deprecatedwait.PollImmediatetowait.PollUntilContextTimeout.
wait.PollImmediateis deprecated in favor of context-aware polling. Migrate towait.PollUntilContextTimeoutto support proper context-based cancellation and timeouts as required by Go security guidelines. Import thecontextpackage and pass a context (e.g.,context.Background()or a derived context with timeout) as the first argument to the replacement function. Note that error handling will change—usewait.Interrupted(err)instead of comparing againstErrWaitTimeout.🤖 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 `@test/e2e/kcm_controller.go` at line 188, Replace the deprecated wait.PollImmediate call with wait.PollUntilContextTimeout at the polling location in kcm_controller.go. First, ensure the context package is imported. Then change the function signature to accept a context as the first argument (such as context.Background() or a derived context), adjust the timeout parameter accordingly as the new function signature differs, and update the error handling logic to use wait.Interrupted(err) instead of any direct comparison against ErrWaitTimeout to properly evaluate polling interruption.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 `@test/e2e/kcm_controller.go`:
- Around line 143-144: The code reads ReplicaSet ownerReferences immediately
after the new-app command without waiting for the ReplicaSet to be created,
causing a race condition where .items[0] may not exist yet. Implement polling
logic to wait for the ReplicaSet to be available before executing the get
command with jsonpath filtering, ensuring .items[0].metadata.ownerReferences is
accessible before the assertion checks err.
- Around line 206-207: Replace the hard assertion that fails when nodeList is
empty with a topology-aware skip. In both locations where GetClusterNodesBy is
called (for "worker" and "master" roles), instead of using
o.Expect(nodeList).NotTo(o.BeEmpty()), check if the nodeList is empty and call
g.Skip with an appropriate message explaining that the test requires the
specific node role to be present in the cluster. This allows the test to
gracefully skip on valid cluster topologies that don't include those node roles
rather than failing.
- Around line 70-76: The poll callback is swallowing command execution errors by
returning false, nil when the oc.WithoutNamespace().Run("get") command fails,
which hides the actual root cause of failures. Replace the return statement that
returns false, nil when err is not nil with a return of false, err instead, so
that actual command errors are properly propagated to the poll loop for better
diagnostics. Apply this same fix pattern to all similar poll callbacks at the
other mentioned locations where command failures are being hidden.
In `@test/e2e/kcm_operator.go`:
- Around line 99-103: The code on line 101-102 uses brittle string splitting
with strings.Split(...)[1] to extract the API server host from whoami output,
which can panic if the output format is unexpected or doesn't contain the
expected delimiters. Replace this approach by parsing the API server URL safely
using a URL parsing library to extract the hostname, then construct the
internalLB variable using the parsed hostname. This will handle different URL
formats correctly and prevent panics on unexpected output while ensuring the
api-int host is built correctly regardless of the original URL structure.
In `@test/library/helpers.go`:
- Around line 134-138: In the CheckMetric function, replace the current approach
of building getCmd as a concatenated string and passing it to RemoteShPod with
"sh" and "-c" arguments with a direct argument-based approach. Instead of
passing the full curl command as a single string through sh -c, pass the curl
program name and its individual arguments (flags like -G, -k, -s, the
Authorization header, and the URL) directly as separate arguments to
RemoteShPod, eliminating the need for shell execution and reducing injection
risk.
In `@test/library/oc.go`:
- Around line 41-45: The AsAdmin() method sets the asAdmin field to true but
this flag is never actually used anywhere in the execution path, making it a
non-functional API. Either implement the admin behavior by consuming the asAdmin
flag in the run() method (or wherever the OC command is built and executed) to
apply privilege escalation, or remove the AsAdmin() method entirely if admin
functionality is not intended. Apply the same fix to any other similar
non-functional methods mentioned in the review comments (referenced at lines
82-90).
In `@test/testdata/workloads/cronjob56176.yaml`:
- Around line 19-23: Add a securityContext section to the pi container with
runAsNonRoot set to true, readOnlyRootFilesystem set to true,
allowPrivilegeEscalation set to false, and capabilities configured to drop ALL.
Additionally, add a resources section to the pi container that defines both
requests and limits for CPU and memory resources to ensure proper resource
constraints are in place. These additions should be made as sibling fields to
the existing name, image, and command fields in the container specification.
In `@test/testdata/workloads/cronjob75375.yaml`:
- Around line 20-24: The "pi" container specification is missing required
security hardening configurations and resource limits. Add a securityContext
block with runAsNonRoot set to true, readOnlyRootFilesystem set to true, and
allowPrivilegeEscalation set to false. Within the securityContext, add a
capabilities section that drops ALL capabilities (no specific capabilities need
to be added for this workload). Additionally, add a resources section with both
requests and limits for CPU and memory to ensure proper resource constraints on
the "pi" container.
In `@test/testdata/workloads/daemonset-origin.yaml`:
- Around line 20-33: The DaemonSet manifest for the hello-openshift container is
missing required workload hardening controls. In the container spec for
hello-openshift, add a securityContext block that includes runAsNonRoot set to
true, readOnlyRootFilesystem set to true, allowPrivilegeEscalation set to false,
and a capabilities section that drops ALL capabilities. Additionally, ensure the
resources section has both requests and limits fully specified for CPU and
memory, and add liveness and readiness probes to the container specification to
enable proper health monitoring.
In `@test/testdata/workloads/daemonset-update.yaml`:
- Around line 20-33: The DaemonSet update manifest for the hello-openshift
container is missing critical security and resource configuration. Add a
securityContext block to the container specification with runAsNonRoot set to
true, readOnlyRootFilesystem set to true, allowPrivilegeEscalation set to false,
and capabilities drop set to ALL. Additionally, add a cpu limit to the
resources.limits section (currently only memory limit exists) to enforce
complete resource constraints. Finally, add both livenessProbe and
readinessProbe definitions to the container to ensure health checks are properly
configured, using appropriate handlers and timing parameters for your workload.
In `@test/testdata/workloads/deployment-69072.yaml`:
- Around line 30-36: The topologySpreadConstraints configuration in the
deployment manifest uses whenUnsatisfiable: DoNotSchedule which can cause
scheduling failures on constrained OpenShift topologies (SNO, Two-Node, Arbiter,
and HyperShift). Either add topology-aware validation logic to conditionally
apply DoNotSchedule only when supported, or change the whenUnsatisfiable value
to ScheduleAnyway for the topologySpreadConstraints block containing the
kubernetes.io/hostname topologyKey and the matchLabels selector for app:
infinite-pod-creation-69072 to ensure compatibility across all topology types.
- Around line 37-53: The container definition for infinite-pod-creation has
empty security and resource configurations that need to be hardened. Replace the
empty `resources: {}` object with explicit CPU and memory limits and requests.
Replace the empty `securityContext: {}` at the container level with security
settings including runAsNonRoot set to true, readOnlyRootFilesystem set to true,
allowPrivilegeEscalation set to false, and a capabilities section that drops all
capabilities. Additionally, add livenessProbe and readinessProbe sections to the
container spec to monitor pod health, using appropriate initial delays and
timeout values for the application.
In `@test/testdata/workloads/deployment-73887.yaml`:
- Around line 15-23: The busybox container in the deployment manifest is missing
critical security configurations and incomplete resource constraints. Add a
securityContext section to the container with runAsNonRoot set to true,
readOnlyRootFilesystem set to true, allowPrivilegeEscalation set to false, and
securityContext.capabilities.drop set to drop all capabilities. Extend the
resources section to include CPU requests and limits in addition to the existing
memory constraints. Finally, add livenessProbe and readinessProbe sections to
the container to define health checks, using appropriate initial delays and
timeout values for the sleep command.
In `@test/testdata/workloads/deployment-with-shutdown-gracefully.yaml`:
- Around line 25-48: The container specification for hello-19922 is missing
critical security hardening and health checks required by coding guidelines. Add
a securityContext block within the container that sets runAsNonRoot to true,
readOnlyRootFilesystem to true, allowPrivilegeEscalation to false, and includes
a capabilities section with drop set to ALL. Additionally, add liveness and
readiness probes to the container using httpGet to probe port 8080 with
appropriate initial delays and periods. Finally, populate the currently empty
resources block with both requests and limits for CPU and memory to constrain
the workload appropriately.
In `@test/testdata/workloads/kubejobfailed-73886.yaml`:
- Around line 7-13: The Job fixture container named "fail" is missing required
security hardening and resource constraints. Add a securityContext block to the
container with runAsNonRoot set to true, readOnlyRootFilesystem set to true, and
allowPrivilegeEscalation set to false. Within the securityContext, add a
capabilities field that drops ALL capabilities. Additionally, add a resources
section with limits specifying both cpu and memory constraints to meet the pod
security and resource management requirements. Apply these settings to the
container specification in the spec.containers section.
---
Duplicate comments:
In `@test/library/oc.go`:
- Around line 67-90: The run() method in ocCommand currently uses exec.Command
without context support, preventing subprocess cancellation and timeouts.
Replace the exec.Command call with exec.CommandContext and add a context.Context
parameter to the run() method signature. Then update all three public methods
(Output, Execute, and Outputs) to also accept a context.Context parameter and
pass it through to the run() method. This enables proper cancellation and
timeout handling for subprocess execution.
In `@test/testdata/workloads/cronjob54195.yaml`:
- Around line 11-20: The container named "hello" in the CronJob pod template is
missing critical security hardening and resource management configurations. Add
a securityContext block to the container specification with runAsNonRoot set to
true, readOnlyRootFilesystem set to true, allowPrivilegeEscalation set to false,
and capabilities configured to drop ALL. Additionally, add a resources section
defining both requests and limits for CPU and memory to ensure proper resource
allocation and prevent resource exhaustion.
In `@test/testdata/workloads/cronjob54195ic.yaml`:
- Around line 11-20: The hello container in the CronJob spec is missing required
security hardening and resource constraints. Add a securityContext block to the
container with runAsNonRoot set to true, readOnlyRootFilesystem set to true, and
allowPrivilegeEscalation set to false. Within the securityContext, add a
capabilities block that drops ALL capabilities. Additionally, add a resources
block with both requests and limits for cpu and memory to ensure proper resource
management. These should be sibling properties to the image, imagePullPolicy,
and command properties of the container specification.
In `@test/testdata/workloads/cronjob54195notz.yaml`:
- Around line 10-19: The container named "hello" in the pod template is missing
required security hardening controls and resource constraints. Add a
securityContext field to the container spec with runAsNonRoot set to true,
readOnlyRootFilesystem set to true, and allowPrivilegeEscalation set to false.
Within the securityContext, add a capabilities section that drops all
capabilities. Additionally, add a resources field with requests and limits for
both cpu and memory to ensure proper resource constraints are enforced on the
container.
In `@test/testdata/workloads/cronjob75375.yaml`:
- Line 22: Replace the mutable container image reference `image: perl` at line
22 in cronjob75375.yaml with a digest-pinned image tag following the same
pattern used in other workload fixtures. Instead of using just the image name,
specify the full image reference including the digest hash (e.g.,
perl@sha256:...) to ensure reproducible test behavior across runs.
---
Nitpick comments:
In `@test/e2e/kcm_controller.go`:
- Line 188: Replace the deprecated wait.PollImmediate call with
wait.PollUntilContextTimeout at the polling location in kcm_controller.go.
First, ensure the context package is imported. Then change the function
signature to accept a context as the first argument (such as
context.Background() or a derived context), adjust the timeout parameter
accordingly as the new function signature differs, and update the error handling
logic to use wait.Interrupted(err) instead of any direct comparison against
ErrWaitTimeout to properly evaluate polling interruption.
🪄 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: b7a44e1e-15ce-4ebf-86b8-d52972c0d376
📒 Files selected for processing (23)
test/e2e/kcm_alerts.gotest/e2e/kcm_controller.gotest/e2e/kcm_cronjob.gotest/e2e/kcm_operator.gotest/library/helpers.gotest/library/oc.gotest/testdata/workloads/cronjob54195.yamltest/testdata/workloads/cronjob54195ic.yamltest/testdata/workloads/cronjob54195notz.yamltest/testdata/workloads/cronjob56176.yamltest/testdata/workloads/cronjob75375.yamltest/testdata/workloads/cronjob_54196.yamltest/testdata/workloads/daemonset-origin.yamltest/testdata/workloads/daemonset-update.yamltest/testdata/workloads/deploy_duplicatepodsrs.yamltest/testdata/workloads/deployment-69072.yamltest/testdata/workloads/deployment-73887.yamltest/testdata/workloads/deployment-with-shutdown-gracefully.yamltest/testdata/workloads/kubejobfailed-73886.yamltest/testdata/workloads/project-69072.yamltest/testdata/workloads/statefulset-76130.yamltest/testdata/workloads/statefulset-79105.yamltest/testdata/workloads/statefulset_63694.yaml
✅ Files skipped from review due to trivial changes (3)
- test/testdata/workloads/cronjob_54196.yaml
- test/testdata/workloads/deploy_duplicatepodsrs.yaml
- test/testdata/workloads/project-69072.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/kcm_alerts.go
- test/e2e/kcm_cronjob.go
| err = wait.Poll(5*time.Second, 30*time.Second, func() (bool, error) { | ||
| output, err = oc.WithoutNamespace().Run("get").Args("is", "-n", ns).Output() | ||
| if err != nil { | ||
| return false, nil | ||
| } | ||
| return strings.Contains(output, "No resources found"), nil | ||
| }) |
There was a problem hiding this comment.
Stop swallowing command failures inside poll loops.
Several poll callbacks convert non-nil command errors into false, nil, which hides root-cause failures and produces timeouts with low diagnostic value.
Suggested fix pattern
- output, err = oc.WithoutNamespace().Run("get").Args("is", "-n", ns).Output()
- if err != nil {
- return false, nil
- }
+ output, cmdErr := oc.WithoutNamespace().Run("get").Args("is", "-n", ns).Output()
+ if cmdErr != nil {
+ return false, cmdErr
+ }As per coding guidelines, **/*.go requires: “Never ignore error returns”.
Also applies to: 117-123, 155-162, 188-194, 282-288
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 73-73: error is not nil (line 72) but it returns nil
(nilerr)
🤖 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 `@test/e2e/kcm_controller.go` around lines 70 - 76, The poll callback is
swallowing command execution errors by returning false, nil when the
oc.WithoutNamespace().Run("get") command fails, which hides the actual root
cause of failures. Replace the return statement that returns false, nil when err
is not nil with a return of false, err instead, so that actual command errors
are properly propagated to the poll loop for better diagnostics. Apply this same
fix pattern to all similar poll callbacks at the other mentioned locations where
command failures are being hidden.
Sources: Coding guidelines, Linters/SAST tools
| topologySpreadConstraints: | ||
| - maxSkew: 1 | ||
| topologyKey: kubernetes.io/hostname | ||
| whenUnsatisfiable: DoNotSchedule | ||
| labelSelector: | ||
| matchLabels: | ||
| app: infinite-pod-creation-69072 |
There was a problem hiding this comment.
Reconcile topology spread policy with supported OpenShift topologies.
whenUnsatisfiable: DoNotSchedule should be guarded/validated for SNO, Two-Node, Arbiter, and HyperShift compatibility to avoid unschedulable behavior in constrained topologies.
As per coding guidelines, deployment manifests must be topology-aware and specifically flag topology spread with DoNotSchedule unless protected by topology-aware checks/hooks.
🤖 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 `@test/testdata/workloads/deployment-69072.yaml` around lines 30 - 36, The
topologySpreadConstraints configuration in the deployment manifest uses
whenUnsatisfiable: DoNotSchedule which can cause scheduling failures on
constrained OpenShift topologies (SNO, Two-Node, Arbiter, and HyperShift).
Either add topology-aware validation logic to conditionally apply DoNotSchedule
only when supported, or change the whenUnsatisfiable value to ScheduleAnyway for
the topologySpreadConstraints block containing the kubernetes.io/hostname
topologyKey and the matchLabels selector for app: infinite-pod-creation-69072 to
ensure compatibility across all topology types.
Source: Coding guidelines
| containers: | ||
| - name: busybox | ||
| image: quay.io/openshifttest/busybox@sha256:c5439d7db88ab5423999530349d327b04279ad3161d7596d2126dfb5b02bfd1f | ||
| command: ["sh", "-c", "sleep 3600"] | ||
| resources: | ||
| requests: | ||
| memory: "2Gi" | ||
| limits: | ||
| memory: "2Gi" |
There was a problem hiding this comment.
Add baseline pod security and complete resource constraints.
This container lacks explicit non-root/privilege settings, dropped capabilities, and health probes; it also defines only memory resources without CPU limits/requests.
As per coding guidelines, Kubernetes/OpenShift manifests must enforce runAsNonRoot, readOnlyRootFilesystem, allowPrivilegeEscalation: false, drop capabilities, define resource limits, and include liveness/readiness probes.
🧰 Tools
🪛 Checkov (3.3.1)
[medium] 1-23: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-23: Minimize the admission of root containers
(CKV_K8S_23)
🤖 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 `@test/testdata/workloads/deployment-73887.yaml` around lines 15 - 23, The
busybox container in the deployment manifest is missing critical security
configurations and incomplete resource constraints. Add a securityContext
section to the container with runAsNonRoot set to true, readOnlyRootFilesystem
set to true, allowPrivilegeEscalation set to false, and
securityContext.capabilities.drop set to drop all capabilities. Extend the
resources section to include CPU requests and limits in addition to the existing
memory constraints. Finally, add livenessProbe and readinessProbe sections to
the container to define health checks, using appropriate initial delays and
timeout values for the sleep command.
Sources: Coding guidelines, Linters/SAST tools
gangwgr
force-pushed
the
mugrate-otp-cases-ote
branch
from
1d6c513 to
2bbde02
Compare
|
@CodeRabbit Poll loops swallowing errors (kcm_controller.go) - Changed return false, nil to return false, err in 4 poll callbacks so command failures are propagated instead of producing uninformative timeouts. Left the one in kcm_operator.go line 118 as false, nil because that test intentionally disrupts KAS where errors are expected. ReplicaSet ownerReferences race (kcm_controller.go:143) - Added wait.Poll to wait for the ReplicaSet to appear before reading ownerReferences in testClusterScopedOwnerRefWarning(). Topology-aware skips (kcm_controller.go + kcm_operator.go) - Replaced o.Expect(nodeList).NotTo(o.BeEmpty()) with g.Skip(...) when worker/master nodes are absent, so tests skip gracefully on HyperShift/SNO topologies. Safe URL parsing (kcm_operator.go:99-103) - Replaced brittle strings.Split chain with url.Parse to safely extract the hostname and construct the api-int address. Avoid sh -c injection (helpers.go:CheckMetric) - Replaced shell-interpolated curl command via sh -c with direct argument passing to curl. AsAdmin() documented (oc.go) - Added comment explaining the no-op is intentional (e2e tests already run with admin kubeconfig). Skipped (testdata YAML hardening): All the security hardening comments on cronjob56176.yaml, cronjob75375.yaml, daemonset-origin.yaml, daemonset-update.yaml, deployment-69072.yaml, deployment-73887.yaml, deployment-with-shutdown-gracefully.yaml, kubejobfailed-73886.yaml - these are test fixtures specifically designed to test controller behavior with certain workload configurations. Adding securityContext, resource limits, and probes would change the test intent. |
|
Tip For best results, initiate chat on the files or code changes.
Regarding the testdata YAML hardening skip: that rationale is sound. These manifests exist to exercise specific controller behaviors (e.g., |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/kcm_operator.go`:
- Around line 122-126: The error handling in the wait.Poll callback function is
swallowing the error from the oc command execution. When the
Run("get").Args("co", "kube-controller-manager").Output() call fails with an
error, the code returns false, nil instead of propagating the actual error,
which masks the root cause and causes the poll to retry until timeout. Change
the error handling in the callback to return the actual error (return false,
err) instead of returning nil when the oc command fails, so that failures are
properly surfaced and the poll can fail fast.
In `@test/library/helpers.go`:
- Around line 147-151: The wait.Poll function in the helpers.go file is
suppressing errors by returning false, nil when the oc command execution fails.
Instead of ignoring the error, propagate it by returning false, err in the
conditional block where err != nil is checked. This applies to the error
handling in the wait.Poll callback where the
oc.AsAdmin().WithoutNamespace().Run() command is executed, as well as in similar
polling patterns throughout the file at the other affected locations mentioned
(lines 158-162 and 236-240). This ensures that actual command failures are
preserved in the error chain rather than being masked, which will improve suite
diagnosability and aid debugging.
🪄 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: b0b72e1f-b31c-47ab-9b9f-816840125a8b
📒 Files selected for processing (23)
test/e2e/kcm_alerts.gotest/e2e/kcm_controller.gotest/e2e/kcm_cronjob.gotest/e2e/kcm_operator.gotest/library/helpers.gotest/library/oc.gotest/testdata/workloads/cronjob54195.yamltest/testdata/workloads/cronjob54195ic.yamltest/testdata/workloads/cronjob54195notz.yamltest/testdata/workloads/cronjob56176.yamltest/testdata/workloads/cronjob75375.yamltest/testdata/workloads/cronjob_54196.yamltest/testdata/workloads/daemonset-origin.yamltest/testdata/workloads/daemonset-update.yamltest/testdata/workloads/deploy_duplicatepodsrs.yamltest/testdata/workloads/deployment-69072.yamltest/testdata/workloads/deployment-73887.yamltest/testdata/workloads/deployment-with-shutdown-gracefully.yamltest/testdata/workloads/kubejobfailed-73886.yamltest/testdata/workloads/project-69072.yamltest/testdata/workloads/statefulset-76130.yamltest/testdata/workloads/statefulset-79105.yamltest/testdata/workloads/statefulset_63694.yaml
✅ Files skipped from review due to trivial changes (1)
- test/testdata/workloads/project-69072.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- test/testdata/workloads/cronjob_54196.yaml
- test/e2e/kcm_alerts.go
- test/e2e/kcm_cronjob.go
- test/testdata/workloads/deploy_duplicatepodsrs.yaml
gangwgr
force-pushed
the
mugrate-otp-cases-ote
branch
4 times, most recently
from
8437363 to
73bf127
Compare
gangwgr
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/test e2e-aws-ovn |
4 similar comments
|
/test e2e-aws-ovn |
|
/test e2e-aws-ovn |
|
/test e2e-aws-ovn |
|
/test e2e-aws-ovn |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
/test e2e-aws-ovn |
|
better to add CI Pass job links. I could see there are 2 cases which got listed under flaky, is this expected? |
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| o.Expect(output).To(o.ContainSubstring("openshift.io/imagestreams 0 10")) | ||
|
|
||
| err = oc.WithoutNamespace().Run("new-app").Args("quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83", "-n", ns, "--import-mode=PreserveOriginal").Execute() |
There was a problem hiding this comment.
describe the image as constant above
| err = oc.WithoutNamespace().Run("delete").Args("is", "--all", "-n", ns).Execute() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| err = wait.Poll(5*time.Second, 30*time.Second, func() (bool, error) { |
There was a problem hiding this comment.
update with non-deprecated method
gangwgr
force-pushed
the
mugrate-otp-cases-ote
branch
from
0439b1d to
51a63aa
Compare
| if len(nodeList) == 0 { | ||
| g.Skip("no worker nodes available in cluster") | ||
| } | ||
| defer func() { |
There was a problem hiding this comment.
consider using g.AfterEach(func()
gangwgr
force-pushed
the
mugrate-otp-cases-ote
branch
4 times, most recently
from
73fbdfb to
45bfacf
Compare
|
|
||
| if len(workers) > 1 { | ||
| for i := 0; i < len(workers)-1; i++ { | ||
| err := oc.Run("adm").Args("cordon", workers[i]).Execute() |
There was a problem hiding this comment.
Why do we cordon workers for this test? This test doesn't have to be disruptive, we can just deploy a deployment that will struggle to become available.
| err := oc.Run("create").Args("-f", deployFile, "-n", "default").Execute() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| if !testlib.WaitForAvailableRsRunning(oc, "deployment", "replicas-mismatch", "default", "5") { |
There was a problem hiding this comment.
do we have to block for this check?
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| token := testlib.GetSAToken(oc) | ||
| testlib.CheckMetric(oc, testlib.PrometheusURL+` --data-urlencode 'query=ALERTS{alertname="KubeJobFailed"}'`, token, "fail-job", 1200) |
There was a problem hiding this comment.
As far as I can tell, the alert triggers only specific jobs: https://github.com/openshift/cluster-monitoring-operator/blob/55b33753fadc80167658a7fc96f7f38fc5b5d0a2/assets/control-plane/prometheus-rule.yaml#L193
does this test work?
| } | ||
| }) | ||
|
|
||
| g.It("[OTP][OCP-73886] Validate for alert KubeJobFailed", func() { |
There was a problem hiding this comment.
Also, shouldn't these tests be rather in cluster-monitoring-operator, since the alert is managed by them?
| err = oc.WithoutNamespace().Run("new-app").Args("quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83", "-n", "p43099", "--import-mode=PreserveOriginal").Execute() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| var refer string |
There was a problem hiding this comment.
| var refer string | |
| var reference string |
| return oc.WithoutNamespace().Run("apply").Args("-f", configFile).Execute() | ||
| } | ||
|
|
||
| func CreateDuplicatePodsRS(oc *OC, name, namespace, template string, replicas int) { |
There was a problem hiding this comment.
do we expect this function to be reused? Can we inline it instead?
| return false, err | ||
| } | ||
| return ds.Status.DesiredNumberScheduled > 0 && | ||
| ds.Status.NumberReady == ds.Status.DesiredNumberScheduled && |
There was a problem hiding this comment.
| ds.Status.NumberReady == ds.Status.DesiredNumberScheduled && | |
| ds.Status.NumberAvailable == ds.Status.DesiredNumberScheduled && |
| } | ||
| return ds.Status.DesiredNumberScheduled > 0 && | ||
| ds.Status.NumberReady == ds.Status.DesiredNumberScheduled && | ||
| ds.Status.UpdatedNumberScheduled == ds.Status.DesiredNumberScheduled, nil |
There was a problem hiding this comment.
| ds.Status.UpdatedNumberScheduled == ds.Status.DesiredNumberScheduled, nil | |
| ds.Status.UpdatedNumberScheduled == ds.Status.DesiredNumberScheduled && ds.Status.numberMisscheduled == 0, nil |
| } | ||
|
|
||
| func GetDaemonsetDesiredNum(oc *OC, namespace, name string) int { | ||
| out, err := oc.WithoutNamespace().Run("get").Args("daemonset", name, "-n", namespace, "-o=jsonpath={.status.desiredNumberScheduled}").Output() |
There was a problem hiding this comment.
Can we limit the use of oc and jsonpath for such simple operations? And just use typed client instead?
| if err != nil { | ||
| return false, err | ||
| } | ||
| return !strings.Contains(status, "unschedulable"), nil |
There was a problem hiding this comment.
same here (types) and in other places
gangwgr
force-pushed
the
mugrate-otp-cases-ote
branch
from
45bfacf to
c0918e9
Compare
|
/test e2e-gcp-operator-disruptive |
gangwgr
force-pushed
the
mugrate-otp-cases-ote
branch
2 times, most recently
from
619238d to
40aa910
Compare
gangwgr
force-pushed
the
mugrate-otp-cases-ote
branch
from
40aa910 to
7096079
Compare
|
/retest-required |
|
/lgtm |
gangwgr
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@gangwgr: This pull request references CNTRLPLANE-3778 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 task 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. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/hold cancel |
|
/test e2e-aws-ovn-upgrade |
1 similar comment
|
/test e2e-aws-ovn-upgrade |
|
@gangwgr: 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. |
|
/verified by ci runs |
openshift-ci-robot
added
the
verified
|
@gangwgr: 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. |
5 participants
Summary by CodeRabbit