MCO-2211: MCO-2210: MCO-2234 MCO-Migrate MCO tests from openshift-tests-private#5902
MCO-2211: MCO-2210: MCO-2234 MCO-Migrate MCO tests from openshift-tests-private#5902ptalgulk01 wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@ptalgulk01: This pull request references MCO-2211 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target 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. |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (13)
WalkthroughAdds exported test utilities and helpers (logs, MachineConfig/MCP queries, node filtering, exec helpers, scaling), a new platform constant, multiple long-duration MCO Ginkgo test suites (drain, MCP lifecycle, prune), and new YAML test fixtures. Changes are test-focused and add helper APIs used by those tests. MCO tests & helpers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 7 | ❌ 5❌ Failed checks (5 warnings)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ptalgulk01 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: 9
🧹 Nitpick comments (1)
test/extended-priv/util.go (1)
1301-1318: Useerrors.Asfor more robust and idiomatic exec error detection.The current implementation manually unwraps one level and checks the type, which works for the current codebase structure but is less robust and idiomatic than using
errors.As. This pattern will handle arbitrary error wrapping depths and aligns with Go best practices.Proposed fix
func IsExecShellError(err error) bool { - if unwrapped := errors.Unwrap(err); unwrapped != nil { - _, ok := unwrapped.(*exec.ExitError) - return ok - } - _, ok := err.(*exec.ExitError) - return ok + var exitErr *exec.ExitError + return errors.As(err, &exitErr) } // UnwrapExecCode unwraps the error and extracts the stderr string if possible func UnwrapExecCode(err error) (int, error) { - if unwrapped := errors.Unwrap(err); unwrapped != nil { - exitError, ok := unwrapped.(*exec.ExitError) - if ok { - return exitError.ExitCode(), nil - } - } - return -1, fmt.Errorf("No exit code available in the provided error %s", err) + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + return exitErr.ExitCode(), nil + } + return -1, fmt.Errorf("no exit code available in error: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended-priv/util.go` around lines 1301 - 1318, The type checks in IsExecShellError and UnwrapExecCode only inspect one unwrap level; replace the manual unwrap logic with errors.As to robustly detect *exec.ExitError across arbitrary wrapping. In IsExecShellError use errors.As(err, &exitError) and return whether it matched; in UnwrapExecCode use errors.As to obtain the *exec.ExitError and return exitError.ExitCode(), otherwise return -1 and a formatted error mentioning the original err. Update references to IsExecShellError and UnwrapExecCode accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended-priv/machineconfig.go`:
- Around line 184-189: GetMCPRenderedMachineConfigsOrFail currently calls
GetRenderedMachineConfigForMaster, so it returns only master-rendered
MachineConfigs; change it to return MCP-specific rendered MachineConfigs by
either renaming the method to reflect the master-only behavior or, preferably,
implement and call an MCP-aware lookup (e.g., create/use
GetRenderedMachineConfigForMCP or a filter by MCP label/selector inside
GetMCPRenderedMachineConfigsOrFail) and ensure you pass the MCP identifier to
that lookup and update any callers accordingly; search for
GetMCPRenderedMachineConfigsOrFail and GetRenderedMachineConfigForMaster to
locate the code to modify.
In `@test/extended-priv/mco_drain.go`:
- Around line 88-94: The test currently assumes filterTimestampFromLogs returns
at least 3 timestamps and indexes timestamps[0..2], which can cause a panic if
the log format changes; update the test around filterTimestampFromLogs and
timestamps to first assert the slice length (e.g., require or
o.Expect(len(timestamps)).To(BeNumerically(">=", 3))) before accessing
timestamps[0], timestamps[1], timestamps[2], and then perform the existing
getTimeDifferenceInMinute checks so failures surface as test assertions rather
than panics.
In `@test/extended-priv/mco_machineconfigpool.go`:
- Around line 330-332: The call to workerNode.PatchDesiredConfig(desiredConfig)
is unchecked; capture its returned error (e.g., err :=
workerNode.PatchDesiredConfig(desiredConfig)) and handle it explicitly: if err
!= nil, log the failure with context using logger.Errorf("failed to patch
desiredConfig for worker %s: %v", workerNode.GetName(), err) and fail the test
or return the error from the enclosing function (use the existing test assertion
mechanism if applicable) so a patch failure surfaces immediately.
- Around line 99-102: The test is checking for "NotFound" in stdout but
oc.AsAdmin().WithoutNamespace().Run("get").Args("mcp/"+mcpName).Output() only
returns stdout; change the call to use a method that captures stderr (e.g.,
CombinedOutput()) and assert against that result (update the mcpOut variable
usage) so the "NotFound" message emitted on stderr is detected; locate the call
to oc.AsAdmin().WithoutNamespace().Run("get").Args("mcp/"+mcpName).Output() and
replace it with the CombinedOutput-equivalent and assert Combined output
contains "NotFound".
In `@test/extended-priv/mco_prune.go`:
- Around line 50-53: The current rendered-MC collection misses worker configs
because GetMCPRenderedMachineConfigsOrFail() delegates to the master-only helper
GetRenderedMachineConfigForMaster; update GetMCPRenderedMachineConfigsOrFail()
to iterate all MachineConfigPools and for each pool call the appropriate helper
based on pool role (use GetRenderedMachineConfigForMasterOrFail() for master
pools and the worker equivalent helper for worker pools, or a unified helper
that returns rendered config for any pool), then return the combined
master+worker rendered configs so the call site
(mcList.GetMCPRenderedMachineConfigsOrFail() in mco_prune.go) receives both
master and worker rendered machineconfigs.
- Around line 39-40: The call to mMcp.WaitImmediateForUpdatedStatus() is
currently ignored; capture its return value and fail the test if it indicates a
timeout/error before logging "OK!"; specifically, replace the bare call to
mMcp.WaitImmediateForUpdatedStatus() with code that assigns the result (e.g.,
ok, err, or bool) and assert/fail accordingly (using the test harness's
Expect/require/t.Fatalf as used elsewhere in this file) so downstream assertions
only run when WaitImmediateForUpdatedStatus() succeeded, then log via
logger.Infof("OK!\n") after the check.
- Around line 252-259: Unwrap the exit code into variables instead of passing
UnwrapExecCode(err) directly into Expect: call something like (code, unwrapErr)
:= UnwrapExecCode(err), assert unwrapErr is nil/DidNot(HaveOccurred()) before
asserting the numeric exit code with Expect(code).Replace both usages in the
prune tests (the lines using UnwrapExecCode(err) to check non-zero return code)
so the unwrap error is explicitly checked and the actual code is asserted
separately.
In `@test/extended-priv/testdata/files/create-pod.yaml`:
- Around line 23-29: The Pod-level securityContext currently includes
container-only fields (allowPrivilegeEscalation and capabilities) which are
ignored; move allowPrivilegeEscalation and capabilities into the container's
securityContext (the container spec for the pod's container), leaving
runAsNonRoot and seccompProfile (pod-level or keep under pod's securityContext
if intended) appropriately placed—update the manifest so the container entry has
a securityContext block containing allowPrivilegeEscalation: false and
capabilities: { drop: ["ALL"] } while leaving runAsNonRoot and seccompProfile
where you want pod-level behavior.
In `@test/extended-priv/util.go`:
- Around line 1321-1339: The timestamp parsing is brittle:
filterTimestampFromLogs uses an unescaped "." so it can match wrong separators
and getTimeDifferenceInMinute blindly indexes split results causing panics; fix
by updating the regex in filterTimestampFromLogs to use a literal dot (e.g.
"\.[0-9]{1,6}") and a stricter pattern for timestamps, and change
getTimeDifferenceInMinute to validate split lengths before indexing (check
len(oldTimeValues)>=3 and that splitting the seconds contains two parts), or
better yet use time.Parse with a matching layout to parse both oldTimestamp and
newTimestamp safely; also normalize fractional seconds to nanoseconds when
constructing time.Date so you don’t mis-handle 1–6 digit fractions.
---
Nitpick comments:
In `@test/extended-priv/util.go`:
- Around line 1301-1318: The type checks in IsExecShellError and UnwrapExecCode
only inspect one unwrap level; replace the manual unwrap logic with errors.As to
robustly detect *exec.ExitError across arbitrary wrapping. In IsExecShellError
use errors.As(err, &exitError) and return whether it matched; in UnwrapExecCode
use errors.As to obtain the *exec.ExitError and return exitError.ExitCode(),
otherwise return -1 and a formatted error mentioning the original err. Update
references to IsExecShellError and UnwrapExecCode accordingly.
🪄 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: a3281dcc-aa80-4b54-80e9-fdcf3bdaef06
📒 Files selected for processing (13)
test/extended-priv/const.gotest/extended-priv/controller.gotest/extended-priv/machineconfig.gotest/extended-priv/machineconfigpool.gotest/extended-priv/mco_drain.gotest/extended-priv/mco_machineconfigpool.gotest/extended-priv/mco_prune.gotest/extended-priv/node.gotest/extended-priv/testdata/files/add-mc-to-trigger-node-drain.yamltest/extended-priv/testdata/files/change-worker-ign-version.yamltest/extended-priv/testdata/files/create-pod.yamltest/extended-priv/testdata/files/pod-disruption-budget.yamltest/extended-priv/util.go
| // GetMachineConfigCreatedByMCPs returns a list of the machineconfigs that were created by a MCP | ||
| func (mcl *MachineConfigList) GetMCPRenderedMachineConfigsOrFail() []*MachineConfig { | ||
| renderedMcList, err := mcl.GetRenderedMachineConfigForMaster() | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "Error getting the list of the machineconfigs that were created by a MCP ") | ||
| return renderedMcList | ||
| } |
There was a problem hiding this comment.
GetMCPRenderedMachineConfigsOrFail returns the wrong data set.
This wrapper still delegates to the master-only lookup, so it cannot return rendered MachineConfigs for other MCPs. Either rename it to match the current behavior or point it at an MCP-specific filter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended-priv/machineconfig.go` around lines 184 - 189,
GetMCPRenderedMachineConfigsOrFail currently calls
GetRenderedMachineConfigForMaster, so it returns only master-rendered
MachineConfigs; change it to return MCP-specific rendered MachineConfigs by
either renaming the method to reflect the master-only behavior or, preferably,
implement and call an MCP-aware lookup (e.g., create/use
GetRenderedMachineConfigForMCP or a filter by MCP label/selector inside
GetMCPRenderedMachineConfigsOrFail) and ensure you pass the MCP identifier to
that lookup and update any callers accordingly; search for
GetMCPRenderedMachineConfigsOrFail and GetRenderedMachineConfigForMaster to
locate the code to modify.
There was a problem hiding this comment.
Cannot do since it is same in private-repo
There was a problem hiding this comment.
@ptalgulk01, understood — since this is a direct migration from openshift-tests-private and must remain in parity with the source, keeping the current implementation makes sense. I'll note this constraint for future reviews of migrated test code.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
| timestamps := filterTimestampFromLogs(podLogs, 3) | ||
| logger.Infof("Timestamps %s", timestamps) | ||
| // First 3 retries should be queued every 1 minute. We check 1 min < time < 2.7 min | ||
| o.Expect(getTimeDifferenceInMinute(timestamps[0], timestamps[1])).Should(o.BeNumerically("<=", 2.7)) | ||
| o.Expect(getTimeDifferenceInMinute(timestamps[0], timestamps[1])).Should(o.BeNumerically(">=", 1)) | ||
| o.Expect(getTimeDifferenceInMinute(timestamps[1], timestamps[2])).Should(o.BeNumerically("<=", 2.7)) | ||
| o.Expect(getTimeDifferenceInMinute(timestamps[1], timestamps[2])).Should(o.BeNumerically(">=", 1)) |
There was a problem hiding this comment.
Guard extracted timestamps before indexing.
This code assumes exactly 3 timestamp matches; if log format drifts, this will panic instead of producing a clear assertion failure.
💡 Proposed fix
timestamps := filterTimestampFromLogs(podLogs, 3)
+ o.Expect(timestamps).To(o.HaveLen(3), "Expected 3 timestamps in drain failure logs")
logger.Infof("Timestamps %s", timestamps)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| timestamps := filterTimestampFromLogs(podLogs, 3) | |
| logger.Infof("Timestamps %s", timestamps) | |
| // First 3 retries should be queued every 1 minute. We check 1 min < time < 2.7 min | |
| o.Expect(getTimeDifferenceInMinute(timestamps[0], timestamps[1])).Should(o.BeNumerically("<=", 2.7)) | |
| o.Expect(getTimeDifferenceInMinute(timestamps[0], timestamps[1])).Should(o.BeNumerically(">=", 1)) | |
| o.Expect(getTimeDifferenceInMinute(timestamps[1], timestamps[2])).Should(o.BeNumerically("<=", 2.7)) | |
| o.Expect(getTimeDifferenceInMinute(timestamps[1], timestamps[2])).Should(o.BeNumerically(">=", 1)) | |
| timestamps := filterTimestampFromLogs(podLogs, 3) | |
| o.Expect(timestamps).To(o.HaveLen(3), "Expected 3 timestamps in drain failure logs") | |
| logger.Infof("Timestamps %s", timestamps) | |
| // First 3 retries should be queued every 1 minute. We check 1 min < time < 2.7 min | |
| o.Expect(getTimeDifferenceInMinute(timestamps[0], timestamps[1])).Should(o.BeNumerically("<=", 2.7)) | |
| o.Expect(getTimeDifferenceInMinute(timestamps[0], timestamps[1])).Should(o.BeNumerically(">=", 1)) | |
| o.Expect(getTimeDifferenceInMinute(timestamps[1], timestamps[2])).Should(o.BeNumerically("<=", 2.7)) | |
| o.Expect(getTimeDifferenceInMinute(timestamps[1], timestamps[2])).Should(o.BeNumerically(">=", 1)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended-priv/mco_drain.go` around lines 88 - 94, The test currently
assumes filterTimestampFromLogs returns at least 3 timestamps and indexes
timestamps[0..2], which can cause a panic if the log format changes; update the
test around filterTimestampFromLogs and timestamps to first assert the slice
length (e.g., require or o.Expect(len(timestamps)).To(BeNumerically(">=", 3)))
before accessing timestamps[0], timestamps[1], timestamps[2], and then perform
the existing getTimeDifferenceInMinute checks so failures surface as test
assertions rather than panics.
There was a problem hiding this comment.
Not sure about this implementation, keeping the same
There was a problem hiding this comment.
@ptalgulk01, understood — no problem. Just a heads-up that if filterTimestampFromLogs ever returns fewer than 3 entries (e.g., due to a log format change or timing variability), the subsequent timestamps[0], timestamps[1], timestamps[2] accesses will panic rather than emit a clean Gomega assertion failure, which can make debugging harder. That said, the choice is yours. Feel free to revisit if you hit flaky failures down the line.
| mcpOut, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("mcp/" + mcpName).Output() | ||
| o.Expect(err).Should(o.HaveOccurred()) | ||
| o.Expect(mcpOut).Should(o.ContainSubstring("NotFound")) | ||
| logger.Infof("Custom mcp is deleted successfully!") |
There was a problem hiding this comment.
NotFound check is reading stdout instead of stderr.
Run(...).Output() returns stdout; for failed oc get, the NotFound message is typically in stderr, so this assertion can fail even when behavior is correct.
💡 Proposed fix
- mcpOut, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("mcp/" + mcpName).Output()
+ _, mcpErrOut, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("mcp/" + mcpName).Outputs()
o.Expect(err).Should(o.HaveOccurred())
- o.Expect(mcpOut).Should(o.ContainSubstring("NotFound"))
+ o.Expect(mcpErrOut).Should(o.ContainSubstring("NotFound"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mcpOut, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("mcp/" + mcpName).Output() | |
| o.Expect(err).Should(o.HaveOccurred()) | |
| o.Expect(mcpOut).Should(o.ContainSubstring("NotFound")) | |
| logger.Infof("Custom mcp is deleted successfully!") | |
| _, mcpErrOut, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("mcp/" + mcpName).Outputs() | |
| o.Expect(err).Should(o.HaveOccurred()) | |
| o.Expect(mcpErrOut).Should(o.ContainSubstring("NotFound")) | |
| logger.Infof("Custom mcp is deleted successfully!") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended-priv/mco_machineconfigpool.go` around lines 99 - 102, The test
is checking for "NotFound" in stdout but
oc.AsAdmin().WithoutNamespace().Run("get").Args("mcp/"+mcpName).Output() only
returns stdout; change the call to use a method that captures stderr (e.g.,
CombinedOutput()) and assert against that result (update the mcpOut variable
usage) so the "NotFound" message emitted on stderr is detected; locate the call
to oc.AsAdmin().WithoutNamespace().Run("get").Args("mcp/"+mcpName).Output() and
replace it with the CombinedOutput-equivalent and assert Combined output
contains "NotFound".
| sortedRenderedMCs := mcList.GetMCPRenderedMachineConfigsOrFail() | ||
| logger.Infof(" %s", sortedRenderedMCs) | ||
|
|
||
| sortedMCListMaster := mcList.GetRenderedMachineConfigForMasterOrFail() // to get master rendered machine config |
There was a problem hiding this comment.
This rendered-MC validation currently misses worker-rendered configs.
GetMCPRenderedMachineConfigsOrFail() currently calls the master-only helper (GetRenderedMachineConfigForMaster) per test/extended-priv/machineconfig.go:185-189, so worker prune coverage here is incomplete.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended-priv/mco_prune.go` around lines 50 - 53, The current
rendered-MC collection misses worker configs because
GetMCPRenderedMachineConfigsOrFail() delegates to the master-only helper
GetRenderedMachineConfigForMaster; update GetMCPRenderedMachineConfigsOrFail()
to iterate all MachineConfigPools and for each pool call the appropriate helper
based on pool role (use GetRenderedMachineConfigForMasterOrFail() for master
pools and the worker equivalent helper for worker pools, or a unified helper
that returns rendered config for any pool), then return the combined
master+worker rendered configs so the call site
(mcList.GetMCPRenderedMachineConfigsOrFail() in mco_prune.go) receives both
master and worker rendered machineconfigs.
There was a problem hiding this comment.
It is as per use case the func is created to fetch master mcp only
There was a problem hiding this comment.
@ptalgulk01, understood — thanks for the clarification. If GetMCPRenderedMachineConfigsOrFail() is intentionally scoped to master-only rendered configs for this use case, that's perfectly fine. You may want to consider updating the function's doc comment or name to make that intent explicit (e.g., GetMasterMCPRenderedMachineConfigsOrFail), so future readers aren't misled — but that's entirely up to you.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
| securityContext: | ||
| allowPrivilegeEscalation: false | ||
| capabilities: | ||
| drop: ["ALL"] | ||
| runAsNonRoot: true | ||
| seccompProfile: | ||
| type: "RuntimeDefault" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n test/extended-priv/testdata/files/create-pod.yamlRepository: openshift/machine-config-operator
Length of output: 1091
🌐 Web query:
Kubernetes securityContext allowPrivilegeEscalation capabilities pod vs container level
💡 Result:
In Kubernetes, securityContext is defined at both pod level (spec.securityContext as PodSecurityContext) and container level (spec.containers[].securityContext as SecurityContext). Container-level settings override pod-level settings for overlapping fields. allowPrivilegeEscalation (boolean, controls no_new_privs flag to prevent child processes gaining more privileges than parent): - Container level only: spec.containers[].securityContext.allowPrivilegeEscalation - Pod level: Not available (PodSecurityContext lacks this field) - Defaults to true unless explicitly set to false. Always true if container is privileged or has CAP_SYS_ADMIN. - Examples in official docs show it only in container securityContext. capabilities (add/drop Linux capabilities): - Container level only: spec.containers[].securityContext.capabilities - Pod level: Not available (PodSecurityContext lacks capabilities field) - PodSecurityContext has pod-wide fields like runAsUser, runAsGroup, fsGroup, runAsNonRoot, seccompProfile, seLinuxOptions, but not allowPrivilegeEscalation or capabilities. PodSecurity Standards (Restricted profile) enforce must be false/empty at container level for these fields, confirming container-level application. Best practice: Set allowPrivilegeEscalation: false and drop ALL capabilities (add only necessary) at container level for security hardening.
Citations:
- 1: https://www.kubernetes.io/docs/concepts/security/pod-security-standards
- 2: https://www.k8s.io/docs/concepts/workloads/pods/advanced-pod-config/
- 3: https://kubernetes.io/docs/tasks/configure-pod-container/security-context/
- 4: https://www.k8s.io/docs/concepts/security/pod-security-standards/
- 5: https://kubernetes.io/docs/concepts/security/pod-security-standards/
- 6: https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1
- 7: https://kubernetes.io/docs/tasks/configure-pod-container/security-context
- 8: https://www.manifests.io/kubernetes/1.31/io.k8s.api.core.v1.SecurityContext?linked=Pod.spec.containers.securityContext
Move container-only security settings under the container spec.
allowPrivilegeEscalation and capabilities are container-level fields only in Kubernetes; the PodSecurityContext does not support them. Placing them at the pod level (lines 24-26) means they are silently ignored, and the fixture does not actually enforce the restrictive container security posture it appears to define.
Suggested fix
spec:
containers:
- args:
- sleep
- 4h
image: quay.io/openshifttest/busybox@sha256:c5439d7db88ab5423999530349d327b04279ad3161d7596d2126dfb5b02bfd1f
imagePullPolicy: Always
name: "${NAME}"
+ securityContext:
+ allowPrivilegeEscalation: false
+ capabilities:
+ drop: ["ALL"]
nodeSelector:
kubernetes.io/hostname: "${HOSTNAME}"
restartPolicy: Never
securityContext:
- allowPrivilegeEscalation: false
- capabilities:
- drop: ["ALL"]
runAsNonRoot: true
seccompProfile:
type: "RuntimeDefault"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended-priv/testdata/files/create-pod.yaml` around lines 23 - 29, The
Pod-level securityContext currently includes container-only fields
(allowPrivilegeEscalation and capabilities) which are ignored; move
allowPrivilegeEscalation and capabilities into the container's securityContext
(the container spec for the pod's container), leaving runAsNonRoot and
seccompProfile (pod-level or keep under pod's securityContext if intended)
appropriately placed—update the manifest so the container entry has a
securityContext block containing allowPrivilegeEscalation: false and
capabilities: { drop: ["ALL"] } while leaving runAsNonRoot and seccompProfile
where you want pod-level behavior.
There was a problem hiding this comment.
Dont know if it is vaild
There was a problem hiding this comment.
@ptalgulk01 — Yes, the review comment is valid. Here's the confirmation:
The official Kubernetes documentation consistently shows allowPrivilegeEscalation placed under the container-level securityContext, not the pod-level one.
The standard correct pattern is to place allowPrivilegeEscalation: false and capabilities: { drop: ["ALL"] } under the container's securityContext, while runAsNonRoot: true and seccompProfile live at the pod-level securityContext.
Kubernetes supports capabilities.add/capabilities.drop on containers specifically, and allowPrivilegeEscalation: false flips the kernel no_new_privs bit for the container — these are container-level primitives.
Because PodSecurityContext (pod-level) does not define allowPrivilegeEscalation or capabilities fields, placing them under spec.securityContext means they are silently ignored by Kubernetes — the container does not actually have those restrictions enforced.
The fix from the suggested diff is correct — move allowPrivilegeEscalation and capabilities into the container's securityContext block:
spec:
containers:
- args:
- sleep
- 4h
image: quay.io/openshifttest/busybox@sha256:c5439d7db88ab5423999530349d327b04279ad3161d7596d2126dfb5b02bfd1f
imagePullPolicy: Always
name: "${NAME}"
securityContext:
allowPrivilegeEscalation: false
capabilities:
drop: ["ALL"]
nodeSelector:
kubernetes.io/hostname: "${HOSTNAME}"
restartPolicy: Never
securityContext:
runAsNonRoot: true
seccompProfile:
type: "RuntimeDefault"
ptalgulk01
force-pushed
the
migrate-prune-and-mcp-tests
branch
from
c1c47d6 to
f8ce746
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
test/extended-priv/mco_drain.go (1)
88-94:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard timestamp slice length before indexed access.
On Line 88–94,
timestamps[0..2]is accessed without asserting length. If log format changes, this panics instead of failing as an assertion.Proposed fix
timestamps := filterTimestampFromLogs(podLogs, 3) + o.Expect(timestamps).To(o.HaveLen(3), "Expected 3 timestamps in drain failure logs") logger.Infof("Timestamps %s", timestamps)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended-priv/mco_drain.go` around lines 88 - 94, The test accesses timestamps[0..2] without checking length; update the block after calling filterTimestampFromLogs to assert there are at least 3 entries (e.g., o.Expect(len(timestamps)).Should(o.BeNumerically(">=", 3))) before accessing timestamps[0], timestamps[1], timestamps[2]; keep the existing logger.Infof("Timestamps %s", timestamps) and then perform the existing getTimeDifferenceInMinute assertions only after that guard so the test fails with an assertion instead of panicking.test/extended-priv/mco_machineconfigpool.go (1)
99-102:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck
NotFoundfrom stderr, not stdout.On Line 99–102, the assertion inspects stdout (
Output()), butoc getfailures usually emitNotFoundon stderr.Proposed fix
- mcpOut, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("mcp/" + mcpName).Output() + _, mcpErrOut, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("mcp/" + mcpName).Outputs() o.Expect(err).Should(o.HaveOccurred()) - o.Expect(mcpOut).Should(o.ContainSubstring("NotFound")) + o.Expect(mcpErrOut).Should(o.ContainSubstring("NotFound"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended-priv/mco_machineconfigpool.go` around lines 99 - 102, The test is checking for "NotFound" on stdout (mcpOut) but oc get writes that message to stderr; replace the call to oc.AsAdmin().WithoutNamespace().Run("get").Args("mcp/"+mcpName).Output() with a call that captures stderr (for example CombinedOutput() or the library method that returns combined stdout+stderr) and assert the combined output (or stderr) contains "NotFound" instead of inspecting mcpOut; keep the existing expectation that an error occurred (err) and update the variable name if needed (e.g., combinedOut) when asserting the substring.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended-priv/mco_drain.go`:
- Around line 77-78: The code is currently ignoring errors returned by
mcc.GetFilteredLogsAsList when polling for drain failures; update the poll logic
around calls to GetFilteredLogsAsList (the calls that assign to variables named
logs and the second call near the later poll) to capture the returned error,
check it, and surface it (e.g., fail the test or log the error via
t.Fatalf/t.Errorf or the test logger) instead of discarding it; ensure you
reference mcc and workerNode.GetName() when forming the error message so
failures from GetFilteredLogsAsList are visible and cause the poll to stop with
the real error rather than a generic timeout.
- Around line 52-62: The test defers call pod.Delete(oc) twice which can cause
flaky teardown; remove the duplicate defer so the pod is deleted only once (keep
the first defer that immediately follows pod creation), and ensure the
MachineConfig cleanup still uses mc.DeleteWithWait() and mc.create() as-is;
locate the duplicate defer wrapped around pod.Delete(oc) near where mc is
created and delete that second defer line.
---
Duplicate comments:
In `@test/extended-priv/mco_drain.go`:
- Around line 88-94: The test accesses timestamps[0..2] without checking length;
update the block after calling filterTimestampFromLogs to assert there are at
least 3 entries (e.g., o.Expect(len(timestamps)).Should(o.BeNumerically(">=",
3))) before accessing timestamps[0], timestamps[1], timestamps[2]; keep the
existing logger.Infof("Timestamps %s", timestamps) and then perform the existing
getTimeDifferenceInMinute assertions only after that guard so the test fails
with an assertion instead of panicking.
In `@test/extended-priv/mco_machineconfigpool.go`:
- Around line 99-102: The test is checking for "NotFound" on stdout (mcpOut) but
oc get writes that message to stderr; replace the call to
oc.AsAdmin().WithoutNamespace().Run("get").Args("mcp/"+mcpName).Output() with a
call that captures stderr (for example CombinedOutput() or the library method
that returns combined stdout+stderr) and assert the combined output (or stderr)
contains "NotFound" instead of inspecting mcpOut; keep the existing expectation
that an error occurred (err) and update the variable name if needed (e.g.,
combinedOut) when asserting the substring.
🪄 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: efae0253-1a61-4846-b692-fe236a3879d2
📒 Files selected for processing (13)
test/extended-priv/const.gotest/extended-priv/controller.gotest/extended-priv/machineconfig.gotest/extended-priv/machineconfigpool.gotest/extended-priv/mco_drain.gotest/extended-priv/mco_machineconfigpool.gotest/extended-priv/mco_prune.gotest/extended-priv/node.gotest/extended-priv/testdata/files/add-mc-to-trigger-node-drain.yamltest/extended-priv/testdata/files/change-worker-ign-version.yamltest/extended-priv/testdata/files/create-pod.yamltest/extended-priv/testdata/files/pod-disruption-budget.yamltest/extended-priv/util.go
✅ Files skipped from review due to trivial changes (5)
- test/extended-priv/const.go
- test/extended-priv/testdata/files/change-worker-ign-version.yaml
- test/extended-priv/testdata/files/pod-disruption-budget.yaml
- test/extended-priv/testdata/files/add-mc-to-trigger-node-drain.yaml
- test/extended-priv/testdata/files/create-pod.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- test/extended-priv/machineconfig.go
- test/extended-priv/mco_prune.go
- test/extended-priv/util.go
- test/extended-priv/controller.go
ptalgulk01
force-pushed
the
migrate-prune-and-mcp-tests
branch
2 times, most recently
from
25fdfd3 to
3b1dd2c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
test/extended-priv/mco_drain.go (3)
52-62:⚠️ Potential issue | 🟠 MajorDuplicate pod cleanup defer causes teardown flakes.
pod.Delete(oc)is deferred twice (lines 52 and 61). The second call will fail or return an error because the pod is already gone, producing a spurious assertion failure in teardown.🤖 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/extended-priv/mco_drain.go` around lines 52 - 62, Remove the duplicated deferred pod cleanup: there are two identical defer calls to pod.Delete(oc) surrounding the MachineConfig creation; keep a single defer func() { o.Expect(pod.Delete(oc)).NotTo(o.HaveOccurred()) }() (the first one after pod.Create(oc) is fine) and delete the second duplicate before mc.create(), leaving the other defers for mc.DeleteWithWait and the MachineConfig creation intact (references: pod.Delete, pod.Create, NewMachineConfig, mc.DeleteWithWait, mc.create).
77-78:⚠️ Potential issue | 🟡 MinorErrors from
GetFilteredLogsAsListare silently discarded.Both poll closures (lines 77 and 98) use
logs, _ := mcc.GetFilteredLogsAsList(...), discarding the error. If log retrieval fails, the poll just keeps retrying until timeout, hiding the actual failure reason.🤖 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/extended-priv/mco_drain.go` around lines 77 - 78, The poll closures call mcc.GetFilteredLogsAsList and discard its error; change both closures to capture the returned error from GetFilteredLogsAsList (e.g., logs, err := mcc.GetFilteredLogsAsList(...)) and handle it immediately by logging the error and returning a failing condition (or propagate the error to break the poll) instead of silently continuing to retry; update the closures that reference workerNode.GetName() + ".*Drain failed" so any retrieval error is surfaced (logged/returned) to avoid hiding failures.
88-94:⚠️ Potential issue | 🟠 MajorMissing length guard before indexing
timestamps.
timestamps[0],timestamps[1], andtimestamps[2]are accessed unconditionally. IffilterTimestampFromLogsreturns fewer than 3 elements (log format drift, partial match, etc.), this panics instead of surfacing a clear assertion failure.🤖 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/extended-priv/mco_drain.go` around lines 88 - 94, The test indexes timestamps[0..2] without checking length, which can panic if filterTimestampFromLogs returns fewer than 3 entries; add a guard that asserts len(timestamps) >= 3 (using the test framework's expectation, e.g., o.Expect(len(timestamps)).Should(o.BeNumerically(">=", 3))) or otherwise fail with a clear message before calling getTimeDifferenceInMinute, then proceed to assert on getTimeDifferenceInMinute(timestamps[0], timestamps[1]) and getTimeDifferenceInMinute(timestamps[1], timestamps[2]); reference symbols: filterTimestampFromLogs, timestamps, getTimeDifferenceInMinute.
🤖 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/extended-priv/mco_drain.go`:
- Around line 152-157: The loop currently discards the error from
exutil.GetSpecificPodLogs causing real failures to be hidden; modify the
anonymous function passed to o.Eventually so it captures the returned error
(err) from GetSpecificPodLogs and when err != nil return err.Error() (or a
string that includes the error) instead of ignoring it, so failures surface in
the test output; update the function that calls GetSpecificPodLogs (inside the
o.Eventually closure around podAllLogs, initialCtrlerLogs, searchRegexp) to
propagate the error string.
- Around line 247-248: Replace the raw string literal "none" with the package
constant NonePlatform in the platform check so it matches the other check (which
uses NonePlatform); locate the conditional that reads if platform != "none" &&
platform != "" and change it to use NonePlatform (keeping the empty-string
check) so both branches consistently reference the NonePlatform constant when
deciding whether to call AddToAllMachineSets.
---
Duplicate comments:
In `@test/extended-priv/mco_drain.go`:
- Around line 52-62: Remove the duplicated deferred pod cleanup: there are two
identical defer calls to pod.Delete(oc) surrounding the MachineConfig creation;
keep a single defer func() { o.Expect(pod.Delete(oc)).NotTo(o.HaveOccurred())
}() (the first one after pod.Create(oc) is fine) and delete the second duplicate
before mc.create(), leaving the other defers for mc.DeleteWithWait and the
MachineConfig creation intact (references: pod.Delete, pod.Create,
NewMachineConfig, mc.DeleteWithWait, mc.create).
- Around line 77-78: The poll closures call mcc.GetFilteredLogsAsList and
discard its error; change both closures to capture the returned error from
GetFilteredLogsAsList (e.g., logs, err := mcc.GetFilteredLogsAsList(...)) and
handle it immediately by logging the error and returning a failing condition (or
propagate the error to break the poll) instead of silently continuing to retry;
update the closures that reference workerNode.GetName() + ".*Drain failed" so
any retrieval error is surfaced (logged/returned) to avoid hiding failures.
- Around line 88-94: The test indexes timestamps[0..2] without checking length,
which can panic if filterTimestampFromLogs returns fewer than 3 entries; add a
guard that asserts len(timestamps) >= 3 (using the test framework's expectation,
e.g., o.Expect(len(timestamps)).Should(o.BeNumerically(">=", 3))) or otherwise
fail with a clear message before calling getTimeDifferenceInMinute, then proceed
to assert on getTimeDifferenceInMinute(timestamps[0], timestamps[1]) and
getTimeDifferenceInMinute(timestamps[1], timestamps[2]); reference symbols:
filterTimestampFromLogs, timestamps, getTimeDifferenceInMinute.
🪄 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: ca6ce77b-725b-4712-9788-d2edbb9d0a78
📒 Files selected for processing (13)
test/extended-priv/const.gotest/extended-priv/controller.gotest/extended-priv/machineconfig.gotest/extended-priv/machineconfigpool.gotest/extended-priv/mco_drain.gotest/extended-priv/mco_machineconfigpool.gotest/extended-priv/mco_prune.gotest/extended-priv/node.gotest/extended-priv/testdata/files/add-mc-to-trigger-node-drain.yamltest/extended-priv/testdata/files/change-worker-ign-version.yamltest/extended-priv/testdata/files/create-pod.yamltest/extended-priv/testdata/files/pod-disruption-budget.yamltest/extended-priv/util.go
✅ Files skipped from review due to trivial changes (4)
- test/extended-priv/testdata/files/add-mc-to-trigger-node-drain.yaml
- test/extended-priv/testdata/files/pod-disruption-budget.yaml
- test/extended-priv/testdata/files/change-worker-ign-version.yaml
- test/extended-priv/const.go
🚧 Files skipped from review as they are similar to previous changes (8)
- test/extended-priv/controller.go
- test/extended-priv/testdata/files/create-pod.yaml
- test/extended-priv/machineconfig.go
- test/extended-priv/mco_prune.go
- test/extended-priv/mco_machineconfigpool.go
- test/extended-priv/machineconfigpool.go
- test/extended-priv/node.go
- test/extended-priv/util.go
ptalgulk01
force-pushed
the
migrate-prune-and-mcp-tests
branch
from
3b1dd2c to
6a863cb
Compare
Migrated 17 test cases from openshift-tests-private/test/extended/mco/ to machine-config-operator/test/extended-priv/: - mco_prune.go: 3 tests for prune renderedmachineconfigs functionality (73148, 73155, 74606) - mco_machineconfigpool.go: 10 tests for MCP operations (43048, 43064, 56131, 77354, 42390, 45318, 52373, 56123, 70125, 72007, 75149, 76108, 85073) - mco_drain.go: 4 tests for node drain behavior (43245, 51381, 49568, 49672) Added supporting helper functions: - MachineConfig: NewMachineConfigList, GetRenderedMachineConfigForMaster, GetRenderedMachineConfigForMasterOrFail, GetMCPRenderedMachineConfigsOrFail - MachineConfigPool: SetMaxUnavailable, RemoveMaxUnavailable, GetSortedUpdatedNodes, IsOCL, GetAllApplicableExtensionsToMCPOrFail - Controller: GetLogsAsList, GetFilteredLogsAsList - Node: FilterSchedulableNodesOrFail - Util: IsSNO, IsExecShellError, UnwrapExecCode, getTimeDifferenceInMinute, filterTimestampFromLogs, AddToAllMachineSets, checkUpdatedLists Added template files: - change-worker-ign-version.yaml - pod-disruption-budget.yaml - create-pod.yaml - add-mc-to-trigger-node-drain.yaml All tests build successfully and appear in test listing. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
ptalgulk01
force-pushed
the
migrate-prune-and-mcp-tests
branch
from
6a863cb to
1f8f339
Compare
|
@ptalgulk01: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Migrated 17 test cases from openshift-tests-private/test/extended/mco/ to machine-config-operator/test/extended-priv/:
Added supporting helper functions:
Added template files:
All tests build successfully and appear in test listing.
- What I did
- How to verify it
- Description for the changelog
Summary by CodeRabbit
Tests
Chores