OCPBUGS-99696: Add extension verification failure test cases OCP-89090 and OCP-89095 - #6333
OCPBUGS-99696: Add extension verification failure test cases OCP-89090 and OCP-89095#6333ptalgulk01 wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ptalgulk01 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
WalkthroughAdds reusable node command replacement helpers and two disruptive extension tests that simulate staged deployment and package verification failures, verify MachineConfigPool degradation and MCD logs, and recover the affected pool during cleanup. ChangesExtension failure testing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ExtensionTest
participant Node
participant MCD
participant MCP
ExtensionTest->>Node: replace rpm-ostree or rpm
ExtensionTest->>MCD: apply extension MachineConfig
ExtensionTest->>MCD: delete daemon pod
MCD->>MCP: report NodeDegraded condition
ExtensionTest->>MCP: verify failure message
ExtensionTest->>MCD: verify failure log
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/extended-priv/mco_extensions.go`:
- Line 156: The informational logs in test/extended-priv/mco_extensions.go at
lines 156-156 and 212-212 expose node hostnames through node.GetName(). Update
both logger.Infof calls to log only the customMCPName identifier; retain node
names only in assertion or error messages.
- Around line 159-180: Make fake-binary lifecycle cleanup failure-safe at
test/extended-priv/mco_extensions.go:159-180 and
test/extended-priv/mco_extensions.go:215-236: register rollback before mutating
rpm-ostree or rpm, ensure setup failures trigger restoration, and assert every
restoration command succeeds. Update the cleanup callbacks to check
DebugNodeWithChroot errors and avoid masking umount failures with trailing
commands; do not ignore any Go error returns.
🪄 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: Pro Plus
Run ID: efcbdd0c-0d7c-4447-950f-23f8a5fbaefe
📒 Files selected for processing (1)
test/extended-priv/mco_extensions.go
|
/test bootstrap-unit |
| customMCPName := fmt.Sprintf("test-%s-infra", testID) | ||
| customMCP, err := CreateCustomMCP(oc.AsAdmin(), customMCPName, 1) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "Could not create a new custom MCP") | ||
| node := customMCP.GetSortedNodesOrFail()[0] | ||
| logger.Infof("Custom MCP: %s, node: %s", customMCPName, node.GetName()) |
There was a problem hiding this comment.
We can use GetCompactCompatibleOrCustomPool instead, so that the test is compatible with SNO and Compact clusters too.
| fakeRpmOstreeScript := `printf '#!/bin/bash\nif [[ "$@" == *"install"* ]] || [[ "$@" == *"override"* ]]; then\n echo "Installing package (fake)"\n exit 0\nfi\nexec /var/tmp/rpm-ostree "$@"\n' > /var/tmp/rpm-ostree-fake.sh && ` + | ||
| `chmod +x /var/tmp/rpm-ostree-fake.sh && ` + | ||
| `cp /usr/bin/rpm-ostree /var/tmp/rpm-ostree && ` + | ||
| `nsenter --mount=/proc/1/ns/mnt mount --bind /var/tmp/rpm-ostree-fake.sh /usr/bin/rpm-ostree && ` + | ||
| `restorecon -v /usr/bin/rpm-ostree && echo done` | ||
| out, err := node.DebugNodeWithChroot("bash", "-c", fakeRpmOstreeScript) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "Failed to setup fake rpm-ostree on node %s: %s", node.GetName(), out) |
There was a problem hiding this comment.
We already have similar code in the method node.BreakRebaseInNode
It seems that several test cases need this action, and likely more test cases will need it in the future.
I think it would be a good idea to create a function ReplaceRpmostree() accepting an existing script in the node and replacing the rpm-ostree with it
We would need a RestoreRpmostree() too, to rever the changes executed by ReplaceRpmostree().
Like that all tests can reuse the same code. We need to refactor BreakRebaseInNode so that it uses the new RestoreRpmostree fucntion too.
Optionally, we can store the script itself as a template, the same way BreakRebaseInNode does. If you prefer to leave it as a string I'm ok too, ti is a very short script.
| restoreScript := `nsenter --mount=/proc/1/ns/mnt umount -l /usr/bin/rpm-ostree 2>/dev/null; ` + | ||
| `rm -f /var/tmp/rpm-ostree-fake.sh /var/tmp/rpm-ostree; echo done` |
There was a problem hiding this comment.
We shold encapsulate this in a RestoreRpmostree fucntion.
Use the node.FixRebaseInNode method as a model.
Note the "kill -9" statement in this method. We need to kill the running rpm-ostree before undoing the ReplaceRpmostree actions.
We need to do it to avoid a "devide busy" error when unmounting.
|
|
||
| exutil.By("Wait for MCP to degrade with missing staged deployment error") | ||
| expectedNDMessage := regexp.QuoteMeta("no staged deployment found after applying extensions") | ||
| checkDegraded(customMCP, expectedNDMessage, "", "NodeDegraded", false, 1) |
There was a problem hiding this comment.
I think it would be simpler to avoid using this function. Jut check the status and the message in the mcp.
I think it would be easier to read. We can check the machine-config co status
| customMCPName := fmt.Sprintf("test-%s-infra", testID) | ||
| customMCP, err := CreateCustomMCP(oc.AsAdmin(), customMCPName, 1) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "Could not create a new custom MCP") |
There was a problem hiding this comment.
Same, we can use the function to make it compatible with SNO and Compact clusters.
| exutil.By("Restore rpm, delete MC, recover MCP, and delete custom MCP") | ||
| restoreScript := `nsenter --mount=/proc/1/ns/mnt umount -l /usr/bin/rpm 2>/dev/null; ` + | ||
| `rm -f /var/tmp/rpm-fake.sh /var/tmp/rpm-real; echo done` | ||
| node.DebugNodeWithChroot("bash", "-c", restoreScript) |
There was a problem hiding this comment.
Same, let's encapsulate it in a recover function so that we can reuse it. Let's not forget the "kill -9" part.
The test should test the recover. We have 2 options: we can add the recover code as steps and make the defer section best effort. Or we can test it in the defer section and then we need to check all errors in the defer so that it fails if we can't recover.
Same in the previous test, I guess.
Actually the defer sections are all different. We should reach an agreement in the team and define how do we want the defer sections to be coded exactly.
| exutil.By("Restart MCD pod on the node to pick up fake rpm-ostree") | ||
| mcdPod := node.GetMachineConfigDaemon() | ||
| err = NewNamespacedResource(oc.AsAdmin(), "pod", MachineConfigNamespace, mcdPod).Delete() | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "Failed to delete MCD pod %s", mcdPod) | ||
| logger.Infof("Deleted MCD pod %s to trigger re-sync with fake rpm-ostree", mcdPod) | ||
| logger.Infof("OK!\n") |
There was a problem hiding this comment.
Why do we restart the pod in both tests?
I remember it was only necessary in only one of the, wasn't it?
There was a problem hiding this comment.
in early version I had to delete MCD pod to reproduce the issue, without deleting the restart used to not trigger and MCP not used to get degrade So here we need to delete the pod for both the TC.
| exutil.By("Restart MCD pod on the node to pick up fake rpm") | ||
| mcdPod := node.GetMachineConfigDaemon() | ||
| err = NewNamespacedResource(oc.AsAdmin(), "pod", MachineConfigNamespace, mcdPod).Delete() | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "Failed to delete MCD pod %s", mcdPod) | ||
| logger.Infof("Deleted MCD pod %s to trigger re-sync", mcdPod) | ||
| logger.Infof("OK!\n") |
There was a problem hiding this comment.
Do we need to restart the pod in both tests? or only in one of them?
There was a problem hiding this comment.
It depends, in early version I had to delete MCD pod to reproduce the issue. So here we need to delete the pod for both the TC.
|
|
||
| exutil.By("Wait for MCP to degrade with extension verification error") | ||
| expectedNDMessage := "extension package verification failed" | ||
| checkDegraded(customMCP, expectedNDMessage, "", "NodeDegraded", false, 1) |
There was a problem hiding this comment.
Same. Checking the mcp with eventually and HaveCondition should be enough.
| reapplyScript := `nsenter --mount=/proc/1/ns/mnt mount --bind /var/tmp/rpm-fake.sh /usr/bin/rpm && ` + | ||
| `restorecon -v /usr/bin/rpm && echo done` |
There was a problem hiding this comment.
Let's try to keep this logic encapsulated if possible. If possible, I don't know if we can reapply it without breaking the encapsulation, but let's try. If not possible, or too ugly, we break the encapsulation.
38deb8f to
c8df918
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/extended-priv/node.go (1)
1617-1624: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUnquoted
scriptPathconcatenated into shell command string.
scriptPathis spliced directly into thesh -ccommand inReplaceRpmOstree/ReplaceRpmwithout quoting. Currently all call sites use hardcoded literal paths, so this isn't exploitable today, but it's a fragile pattern that path instructions flag for injection prevention — any future caller passing a path with whitespace or shell metacharacters would break or hijack the command.🔒 Proposed fix
- "nsenter --mount=/proc/1/ns/mnt mount --bind "+scriptPath+" /usr/bin/rpm-ostree && "+ + "nsenter --mount=/proc/1/ns/mnt mount --bind '"+scriptPath+"' /usr/bin/rpm-ostree && "+Apply the equivalent quoting in
ReplaceRpm.As per path instructions, injection prevention guidance states "Command: no shell=True, os.system, or backtick exec with user input" — unquoted string concatenation into a shell command is the same anti-pattern.
Also applies to: 1638-1645
🤖 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/node.go` around lines 1617 - 1624, Update ReplaceRpmOstree and ReplaceRpm to safely quote or escape scriptPath before interpolating it into the sh -c command, preserving paths containing whitespace and preventing shell interpretation of metacharacters. Keep the existing command behavior and hardcoded call-site paths unchanged.Source: Path instructions
🤖 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/node.go`:
- Around line 1617-1624: Update the shell command chains in both
ReplaceRpmOstree and ReplaceRpm to join cp, mount --bind, and restorecon with &&
instead of semicolons, ensuring any failed setup step short-circuits and its
error is returned to the caller.
---
Nitpick comments:
In `@test/extended-priv/node.go`:
- Around line 1617-1624: Update ReplaceRpmOstree and ReplaceRpm to safely quote
or escape scriptPath before interpolating it into the sh -c command, preserving
paths containing whitespace and preventing shell interpretation of
metacharacters. Keep the existing command behavior and hardcoded call-site paths
unchanged.
🪄 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: Pro Plus
Run ID: df6f3dd6-1510-4042-816d-05b218147844
📒 Files selected for processing (2)
test/extended-priv/mco_extensions.gotest/extended-priv/node.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/extended-priv/mco_extensions.go
c8df918 to
df566f6
Compare
ptalgulk01
changed the title
openshift-ci-robot
added
jira/severity-moderate
|
@ptalgulk01: This pull request references Jira Issue OCPBUGS-65645, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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-robot
added
the
jira/invalid-bug
|
/jira refresh |
|
@ptalgulk01: This pull request references Jira Issue OCPBUGS-65645, which is invalid:
Comment 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. |
ptalgulk01
changed the title
openshift-ci-robot
added
jira/severity-low
|
@ptalgulk01: This pull request references Jira Issue OCPBUGS-99696, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/test bootstrap-unit |
|
/test security |
|
/cherry-pick release-4.22 |
|
@ptalgulk01: once the present PR merges, I will cherry-pick it on top of 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 kubernetes-sigs/prow repository. |
|
/test bootstrap-unit |
| o.Expect(err).NotTo(o.HaveOccurred(), "Failed to create fake rpm-ostree script on node %s: %s", node.GetName(), out) | ||
|
|
||
| exutil.By("Replace rpm-ostree with fake script") | ||
| o.Expect(ReplaceRpmOstree(node, fakeRpmOstreePath)).To(o.Succeed(), |
There was a problem hiding this comment.
Let's encapsulate it even more.
We can use
node.ReplaceRpmOstree(node, fakeRpmOstreeScript)Let the ReplaceRpmOstree handle the copy of the file to the node, so that it is easier to use by someone creating a test. Use a constant in the node file to define the path were the fake rpmostree will be copied.
Using print can interfere whit the special characters too. We can copy the string to a local file and copy the local file to the node.
Or we can store the script in a template, and make the node.RpelaceRmpOstree accept a node and a local file instead.
| defer func() { | ||
| exutil.By("Restore rpm-ostree, delete MC, and recover MCP") | ||
| RestoreRpmOstree(node) | ||
| node.DebugNodeWithChroot("sh", "-c", "rm -f "+fakeRpmOstreePath) |
There was a problem hiding this comment.
No need to do that, I think, it's just temp file.
But if you want to do it we should do it in the RestoreRpmOstree function.
| mc.skipWaitForMcp = true | ||
| defer func() { | ||
| exutil.By("Restore rpm-ostree, delete MC, and recover MCP") | ||
| RestoreRpmOstree(node) |
There was a problem hiding this comment.
Let's o.Expect(RestoreRpmOstree).To(o.Succeed()...
| o.Expect(err).NotTo(o.HaveOccurred(), "Failed to create fake rpm script on node %s: %s", node.GetName(), out) | ||
|
|
||
| exutil.By("Replace rpm with fake script") | ||
| o.Expect(ReplaceRpm(node, fakeRpmPath)).To(o.Succeed(), |
There was a problem hiding this comment.
Same, let's encapsulate the file creation inside the function too.
Using print to create the file will give problems in the future if we need to use special characters, since we will have to escape them. It is better to create a local file with the string, or directly provide a template name to the function. Like that no escape has to bee done to create the script.
| defer func() { | ||
| exutil.By("Restore rpm, delete MC, and recover MCP") | ||
| RestoreRpm(node) | ||
| node.DebugNodeWithChroot("sh", "-c", "rm -f "+fakeRpmPath) |
| func ReplaceRpmOstree(node *Node, scriptPath string) error { | ||
| logger.Infof("Replacing rpm-ostree with %s on node %s", scriptPath, node.GetName()) | ||
| _, err := node.DebugNodeWithChroot("sh", "-c", | ||
| "cp /usr/bin/rpm-ostree /var/tmp/rpm-ostree && "+ |
| func ReplaceRpm(node *Node, scriptPath string) error { | ||
| logger.Infof("Replacing rpm with %s on node %s", scriptPath, node.GetName()) | ||
| _, err := node.DebugNodeWithChroot("sh", "-c", | ||
| "cp /usr/bin/rpm /var/tmp/rpm-real && "+ |
df566f6 to
273cd5e
Compare
|
@ptalgulk01: This pull request references Jira Issue OCPBUGS-99696, which is valid. 3 validation(s) were run on this bug
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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/extended-priv/testdata/files/rpm-ostree-fake-install-noop.sh`:
- Around line 5-8: Update the command detection in the fake rpm-ostree script to
inspect exact argument tokens rather than joining "$@" and using substring
matching. Only treat invocations whose command token is exactly “install” or
“override” as successful fake operations, while leaving unrelated commands and
package names on the failure path.
🪄 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: Pro Plus
Run ID: 0ae9c87b-20ce-4c0b-8ac1-548cfb9d7f2d
📒 Files selected for processing (4)
test/extended-priv/mco_extensions.gotest/extended-priv/node.gotest/extended-priv/testdata/files/rpm-fake-usbguard-missing.shtest/extended-priv/testdata/files/rpm-ostree-fake-install-noop.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- test/extended-priv/mco_extensions.go
- test/extended-priv/node.go
|
@ptalgulk01: 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. |
|
/hold Holding to allow the Kube rebase to land in #6321. Please ensure this will not cause merge conflicts for the Kube rebase before unholding this PR. |
openshift-ci
Bot
added
the
do-not-merge/hold
5 participants
Summary
deployment found after applying extensions" error.
"extension package verification failed" error.
Both tests use a custom MCP with 1 node, bind-mount fake binaries via nsenter, and validate degradation via checkDegraded. Cleanup restores original binaries, deletes MC, recovers MCP, and
deletes custom MCP — all in defer.
Test plan
Summary by CodeRabbit