OCPBUGS-99047: Fix flaky oc adm storage-admin test#31400
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
jira/valid-reference
|
@xueqzhan: This pull request references Jira Issue OCPBUGS-99047, 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: xueqzhan 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
|
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)
WalkthroughThe storage-admin CLI test now matches negative ChangesStorage-admin authorization assertions
Estimated code review effort: 1 (Trivial) | ~2 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
openshift-ci
Bot
added
the
ready-for-human-review
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@xueqzhan: This pull request references Jira Issue OCPBUGS-99047, which is valid. The bug has been moved to the POST state. 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. |
|
Scheduling required tests: |
|
@xueqzhan: 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. |
|
/jira refresh |
|
@xueqzhan: This pull request references Jira Issue OCPBUGS-99047, 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. |
| out, err = oc.Run("auth", "can-i").Args("--as=storage-adm", "create", "pods", "--all-namespaces").Output() | ||
| o.Expect(err).To(o.HaveOccurred()) | ||
| o.Expect(out).To(o.HaveSuffix("no")) | ||
| o.Expect(out).To(o.MatchRegexp(`(?m)^no`)) |
There was a problem hiding this comment.
What's the likelihood this will assert a false positive? Is the warning and the extra text always on two lines? Is the extra text after "no" always of the same pattern? If only the missing cluster role reference is the extra text I'd rather prefer to filter it out before the assertion.
There was a problem hiding this comment.
The output from oc auth can-i when denied has this format:
no - RBAC: clusterrole.rbac.authorization.k8s.io "basic-user2-pnjfn" not found
The warning and the diagnostic text are on the same line — no followed by - RBAC: .... There are no multi-line patterns here. The (?m)^no regex matches no at the start of a line, which is safe because:
oc auth can-ionly outputsyesornoas the first word on the response line. There is no scenario where a non-denial response starts withno.- The preceding
o.Expect(err).To(o.HaveOccurred())already asserts the command returned non-zero (denied), so we are only checking the output format at that point. - Warning lines (e.g., deprecation warnings on stderr) would not start with
no— they start withWarning:.
That said, your suggestion to filter the diagnostic suffix is also reasonable. We could use strings.HasPrefix(out, "no") or strings.Split(out, "\n") and check only the relevant line. However, (?m)^no achieves the same result more concisely since the denial response is always the first (and usually only) line starting with no.
3 participants
The [sig-cli] oc adm storage-admin test fails intermittently when the parallel role-selectors test creates and deletes a ClusterRoleBinding bound to system:authenticated at the same time. During cleanup, there is a brief window where the ClusterRoleBinding still exists but its referenced ClusterRole has already been deleted. If oc auth can-i runs during this window, the RBAC authorizer appends a diagnostic message to the denial output (e.g. no - RBAC: clusterrole.rbac.authorization.k8s.io "basic-user2-pnjfn" not found), causing the HaveSuffix("no") assertion to fail.
Replace HaveSuffix("no") with MatchRegexp("(?m)^no") so the assertion matches no at the start of any line, tolerating both warning lines before and RBAC diagnostic text after. The exit code check on the preceding line continues to validate that the command returned non-zero (denied).
Summary by CodeRabbit