Add a CI verification check - #2567
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:
📝 WalkthroughWalkthroughThe change adds a Go RBAC checker and integrates it into repository verification. The checker loads scheduler ClusterRole permissions, scans production Go files under ChangesRBAC verification
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Verification as verify-all.sh
participant Wrapper as verify-rbac.sh
participant Checker as rbaccheck
participant ClusterRole as Scheduler ClusterRole
participant Source as Production Go files
Verification->>Wrapper: invoke RBAC verification
Wrapper->>Checker: run checker for pkg
Checker->>ClusterRole: load permissions
ClusterRole-->>Checker: resource and verb rules
Checker->>Source: parse Kubernetes client calls
Source-->>Checker: resource method calls
Checker-->>Wrapper: report violations and exit status
Wrapper-->>Verification: return verification result
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 `@hack/verify-all.sh`:
- Line 35: Resolve the node RBAC mismatch before enabling the verify-rbac
invocation in the verification flow: update the production Nodes().Update()
calls in device.go to use a semantically equivalent Patch operation, or grant
the scheduler’s ClusterRole update permission for nodes if full updates are
required. Ensure verify-rbac.sh can complete successfully during make verify.
In `@hack/verify-rbac.sh`:
- Around line 44-49: Change EXCLUDE_DIRS from a quoted string to a shell array
containing each find exclusion predicate and its argument separately, then
expand it with "${EXCLUDE_DIRS[@]}" in the find command inside the
DISALLOWED_VERBS loop so pkg/vendor and pkg/.git are actually excluded.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 79eaa672-58dc-4b02-b444-b1c4388e50e8
📒 Files selected for processing (2)
hack/verify-all.shhack/verify-rbac.sh
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@hack/verify-rbac.sh`:
- Around line 94-96: Update the matches scan in verify-rbac.sh to stop
suppressing find or grep failures: remove the stderr redirection and
unconditional success fallback, and ensure traversal or matching errors
propagate as a non-zero script result while preserving the existing file and
pattern filtering.
- Around line 93-94: Update both grep invocations in the verification flow using
the pattern from verb_to_pattern so they pass fixed-string mode with grep -F --,
ensuring literal Go call patterns are matched without regular-expression
interpretation.
- Around line 57-67: Extend verb_to_pattern and ALL_VERBS to cover NodeInterface
methods UpdateStatus, DeleteCollection, Apply, and ApplyStatus, mapping them to
update, deletecollection, and patch as specified. Add separate nodes/status
handling, and make the verification logic recognize method calls whose arguments
span multiple lines.
- Around line 93-96: Update the verb-to-pattern scanning flow in verify-rbac.sh
to use Go AST/type information rather than grep text matching, so it detects
line-broken method chains, node methods including UpdateStatus and
DeleteCollection, and generated Apply/ApplyStatus calls mapped to the patch
permission. If AST-based detection cannot be implemented, explicitly document
these limitations in the script.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 50df41d8-68d5-43c3-8234-b44e21be35e2
📒 Files selected for processing (1)
hack/verify-rbac.sh
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 `@hack/tools/rbaccheck/main.go`:
- Around line 97-117: Update the filepath.Walk call in the main traversal flow
to capture its returned error, and set exitCode to a nonzero value when
traversal fails. Preserve the existing per-file violation handling and
directory-skipping behavior.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 92776584-a191-4f3c-bd8f-7df37ae7f255
📒 Files selected for processing (2)
hack/tools/rbaccheck/main.gohack/verify-rbac.sh
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
This is being closed because it does not comply with the contribution guidelines. |
|
Hi @mesutoezdil , I have updated this commit and PR description. Please have a look, thanks! |
|
/reopen |
|
@jianzhangbjz: Reopened this PR. 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. |
we have some rules here: |
|
Hi @mesutoezdil , got it, thanks! I have updated it, please have a look, thanks! |
mesutoezdil
left a comment
mesutoezdil
left a comment
There was a problem hiding this comment.
checked this tool logic against current master. left two notes inline, first one is important.
|
and look at the messages from coderrabbit and resolve it pls |
I guess the CodeRabbit passed for the latest commit. |
|
Hi @mesutoezdil , I've updated it. Could you help have a look? Thanks! |
|
resolve the conflicts. |
1,require CoreV1() in rbaccheck call chain to avoid false positives 2,check RBAC against the role of the binary that runs the code Signed-off-by: Jian Zhang <jiazha666@gmail.com>
|
/lgtm |
|
Hi @Shouren , could you help approve it? Thanks! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, jianzhangbjz 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 |
3 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Add a CI verification tool (
hack/tools/rbaccheck/) that uses Go AST analysis to verify production code only calls Kubernetes API verbs that are granted by the scheduler ClusterRole.The existing bash-based verify-rbac.sh greps for
Nodes().Update(to catch one specific violation. The new Go tool:charts/hami/templates/scheduler/clusterrole.yamlto extract all(resource, verb) pairs granted by the scheduler ClusterRole
go/astto precisely match method call chainslike
CoreV1().<Resource>().<Verb>(...)Nodes
This catches violations that bash/grep misses — for example
Events().Update()inpkg/util/util.go:325, where the scheduler ClusterRole only grantscreate, get, liston events.Fixed PR: #2569
Which issue(s) this PR fixes:
Fixes #2329 (comment)
Special notes for your reviewer:
The tool runs via
go run ./hack/tools/rbaccheck/ ./pkg/as part ofmake verify. It depends ongopkg.in/yaml.v3which is already a direct dependency.Does this PR introduce a user-facing change?:
No — CI-only change.
This PR was Assisted by Claude Code.
Summary by CodeRabbit
Security
Quality