OCPBUGS-86044: Use generated names in oc adm policy SCC test#31221
OCPBUGS-86044: Use generated names in oc adm policy SCC test#31221tchap wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
jira/valid-reference
|
@tchap: This pull request references Jira Issue OCPBUGS-86044, 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughTests in test/extended/cli/admin.go now generate a user, serviceaccount, and group, add/remove the privileged SCC for those subjects while polling the privileged clusterrolebinding YAML to assert presence/absence, and run oc adm prune auth for the generated user and group. ChangesSCC Policy CLI Test Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@tchap: This pull request references Jira Issue OCPBUGS-86044, 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tchap 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: 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/cli/admin.go`:
- Around line 225-233: The polling callback passed to Eventually that runs
oc.Run("get").Args("clusterrolebinding/system:openshift:scc:privileged", "-o",
"yaml").Output() must treat a "not found" error as a terminal success: if the
get command returns an error and the error message indicates the
ClusterRoleBinding is missing (e.g. contains "NotFound" or "not found"), return
an empty string and nil so Eventually stops; otherwise propagate the error as
before. Update the anonymous func used in the Eventually call to detect the
missing-binding error and convert it into a successful (empty, nil) response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 83551d20-0e8b-46b7-91e3-5f5fe21302d8
📒 Files selected for processing (1)
test/extended/cli/admin.go
openshift-ci
Bot
added
the
ready-for-human-review
|
Scheduling required tests: |
| o.Expect(err).To(o.HaveOccurred()) | ||
| o.Expect(out).To(o.ContainSubstring("error: rolebinding custom found for role view, not other")) | ||
|
|
||
| o.Expect(oc.Run("adm", "policy", "add-scc-to-user").Args("privileged", "fake-user").Execute()).To(o.Succeed()) |
There was a problem hiding this comment.
I think it is better to keep the tests we have.
There was a problem hiding this comment.
Well, it does causes the race, I think, that's why it's being changed. The point of this change is to execute this change in a single oc command so that it's sent together.
There was a problem hiding this comment.
You are probably right to push back on this, because it's not clear why the test is failing very rarely. There is not enough data in the failing job regarding why this happens. It seems the data is written into etcd even though it should return 409, but I don't manage to find information detailed enough in CI jobs to be able to troubleshoot this 😐 So I ended up just trying to improve the test, which is not particularly clear.
There was a problem hiding this comment.
I think that the issue is that the test is not using randomly generated names and there are some leftovers from the previous test run, causing 409 not to happen because the change is actually already there, but I am gonna continue tomorrow. Thanks for pushing back.
The SCC section of this test operates on a cluster-scoped ClusterRoleBinding with hardcoded subject names. Unlike namespaced resources, cluster-scoped resources are not cleaned up between test retries. When the first attempt fails partway through the removes, leftover subjects cause the retry's adds to be no-ops at the etcd level, preventing resourceVersion from advancing and disabling 409 conflict detection on subsequent writes. Use unique generated names so each attempt is independent.
tchap
force-pushed
the
OCPBUGS-86044/fix-scc-policy-test-race
branch
from
02600b3 to
0f8e575
Compare
|
/retitle OCPBUGS-86044: Use generated names in oc adm policy SCC test |
openshift-ci
Bot
changed the title
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/cli/admin.go`:
- Around line 219-221: The test issues multiple separate oc invocations to
mutate SCC subjects, leaving cache-propagation race windows; instead, batch
subject mutations into single oc admin policy commands by combining multiple
subject arguments in one call (e.g., replace separate
oc.Run("adm","policy","add-scc-to-user").Args("privileged", fakeUser) and
oc.Run("adm","policy","add-scc-to-user").Args("privileged", "-z", fakeSA) with a
single oc.Run("adm","policy","add-scc-to-user").Args("privileged", fakeUser,
"-z", fakeSA).Execute(), and do the same for
oc.Run("adm","policy","add-scc-to-group").Args(...) instances referenced around
the same section) so all related user/sa/group updates occur in one command and
remove the inter-command race window.
- Around line 222-241: Replace the immediate post-mutation CRB assertions with
retries using Eventually to avoid stale-cache flakes: after the adm policy
remove-scc-from-* calls (the
oc.Run(...).Args("clusterrolebinding/system:openshift:scc:privileged", "-o",
"yaml").Output() call and subsequent checks that reference fakeUser, fakeSA,
fakeGroup), wrap the get+assert logic in an Eventually that polls until either
the CRB YAML no longer contains the removed fakeUser/fakeSA/fakeGroup or the get
returns the "not found" message; do the same replacement for the other similar
block that spans the later checks (the second occurrence noted at lines
244-252).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4ccfbaa4-7746-403c-9a7b-5eae0e1daac3
📒 Files selected for processing (1)
test/extended/cli/admin.go
|
Scheduling required tests: |
Batch user and service account subjects into single oc invocations and use Eventually to poll for cache propagation between adds and removes as additional hardening against watch cache races.
|
@ardaguclu CodeRabbit now asked me to put back what I removed based on your review, so we need to decide 🙂 I added what CR requested as a separate commit. I mean, it makes sense. But let's discuss. |
|
@tchap: This pull request references Jira Issue OCPBUGS-86044, which is valid. 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. |
|
Scheduling required tests: |
|
@tchap: 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. |
3 participants
The SCC section of this test operates on a cluster-scoped
ClusterRoleBinding with hardcoded subject names. Unlike namespaced
resources, cluster-scoped resources are not cleaned up between test
retries. When the first attempt fails partway through the removes,
leftover subjects cause the retry's adds to be no-ops at the etcd
level, preventing resourceVersion from advancing and disabling 409
conflict detection on subsequent writes.
Use unique generated names so each attempt is independent.
Summary by CodeRabbit
Note: Test-only changes; no user-facing functionality altered.