feat: verify chart RBAC does not drift from config/rbac

[tejassinghbhati]

Description

Follows up on @ajaysundark's question in #351 about whether hack/verify-chart-drift.sh could cover more than CRDs. This extends it to RBAC.

The script previously diffed only the bundled CRD, so nothing caught the chart granting different permissions than the controller declares. #350 was exactly that, and it went unnoticed precisely because nothing compares the two.

RBAC is the case most likely to drift. config/rbac/role.yaml is generated by controller-gen from the kubebuilder markers, so it updates itself whenever the controller's permissions change. The chart's ClusterRole is hand written, so it only updates when someone remembers. Every future permission change is another chance for the chart to fall behind, and the failure is silent apart from whatever the missing grant covered.

A plain diff does not work here. The generated roles carry short names and kustomize labels, the chart templates the name from the release name and adds Helm labels, so the files never match textually even when the permissions are identical. The check therefore renders the chart, matches each Role and ClusterRole by name ignoring the release prefix, and compares only the rules.

Rules are normalised before comparison, sorting the entries within each rule and then the rules themselves, because the order of apiGroups, resources and verbs means nothing to the API server and should not be reported as drift. Without that the check would be noisy enough that people would learn to ignore it.

Output points at the offending permission rather than dumping both role definitions:

RBAC drift between config/rbac and the Helm chart:

ClusterRole "manager-role" has different rules
  only in config: apiGroups=["",events.k8s.io] resources=[events] verbs=[create,patch]
  only in chart:  apiGroups=[""] resources=[events] verbs=[create,patch]

The chart grants different permissions than the controller declares.
Update charts/nrr-controller/templates/rbac.yaml to match config/rbac.

A role the chart renders that has no counterpart in config is reported as a note rather than a failure, since the chart may legitimately ship something kustomize does not.

Merge order. This depends on #351. Run against main as it stands today the check fails, because it correctly finds the missing events.k8s.io grant that #351 fixes. That failure is the proof it works, but it does mean this should land after #351, or CI on main will flag a bug that already has a fix in flight.

Scope. Roles and ClusterRoles only, not bindings, subjects, the Deployment, Services or webhook configuration. I checked all of those by hand while reviewing #351 and they currently agree, so RBAC seemed the right place to start given it is the one that has already bitten us. Easy to extend later if that is wanted.

Related Issue

Fixes #371

Type of Change

/kind feature

Testing

Verified both directions against a real drift rather than a synthetic one.

Against main, where the chart is still missing the events grant, the check fails with the output shown above and exits 1. With #351's template change applied, it passes:

RBAC in the Helm chart matches config/rbac (7 roles compared)

All seven roles are compared: manager-role, leader-election-role, metrics-auth-role, metrics-reader, and the nodereadinessrule admin, editor and viewer roles.

golangci-lint v2.12.1 reports 0 issues across the repo including the new tool, gofmt is clean, and the boilerplate header matches hack/boilerplate/boilerplate.go.txt exactly. bash -n passes on the modified script.

One note on local verification: I could not run the script end to end on Windows, because make manifests fails there with no Go files in E:\... from controller-gen's path handling. That reproduces identically on a clean checkout of main, so it is pre-existing and unrelated to this change. I ran the RBAC section verbatim instead, and CI runs on ubuntu-latest where the CRD step works.

The Helm workflow already sets up Helm, Go and Python before invoking this script, so no workflow change is needed. If helm is missing locally the script now fails with a message pointing at make ensure-helm-install rather than skipping silently.

Checklist

• make test passes
• make lint passes

Does this PR introduce a user-facing change?

NONE

[netlify]

✅ Deploy Preview for node-readiness-controller ready!

Name	Link
🔨 Latest commit	49635d5
🔍 Latest deploy log	https://app.netlify.com/projects/node-readiness-controller/deploys/6a84b1044a411e00082b1153
😎 Deploy Preview	https://deploy-preview-372--node-readiness-controller.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[kubernetes-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tejassinghbhati
Once this PR has been reviewed and has the lgtm label, please assign haircommander for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[kubernetes-prow]

Hi @tejassinghbhati. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

[ajaysundark]

is there a reason why a simple diff based approach will not work? is it necessary for us to implement and maintain this binary?

[tejassinghbhati]

Fair, binary is gone.

Plain diff did not work because the two files differ in name, labels and namespace, and the chart wrote its rules in flow style while controller-gen emits block style. So nothing lined up textually even when the rules were the same.

Fixed that by having the chart render its rules from a copy of the generated file, like the CRD already does. Now it is just a diff in verify-chart-drift.sh, no parsing.

Deleting events.k8s.io from the copy reproduces #350 and the check fails pointing at the missing group.

[ajaysundark]

why it has to be a separate file and be rendered? IIUC, the issue is that kube-builder creates multi-line list and this has single line arrays? cant we have the verbatim copy here instead to keep it simple?

[tejassinghbhati]

Yeah that was it, block style vs single line arrays. Separate file was so the check could be a straight file diff.

Inlined it now. The script pulls the block back out of the template and diffs that instead. Still catches #350.

[tejassinghbhati]

Rebased on main, no conflicts with the chart changes that landed this week. Still just the one diff in verify-chart-drift.sh, no binary.

Both your comments are addressed, so this needs an /ok-to-test whenever you get to it.

[ajaysundark]

can you fix the path changes. I am still not convinced by the awk / sub solution. is there any a cleaner way to achieve this?

[ajaysundark]

This was renamed recently, ref: #407, could you help update the path?

[ajaysundark]

this is very brittle, and likely we will be spending more time fixing this than actually catching the drift :(

[tejassinghbhati]

Path is already done, 67ad417 rebuilt the whole thing on charts/node-readiness-controller. I pushed that a few hours after your comment so you likely saw the old diff.

On brittle, you are right and I do not have a clever answer. The two are linked: keeping the verbatim copy inline in the template is what forces the awk to get it back out. A separate file makes the check a plain diff of two files with no parsing at all, which is what I had before you asked me to inline it. I can have a simple template or a simple check, not both.

Also worth weighing: rbac_test.yaml is already on main from #351 and pins all six rules including the events one, so someone editing the chart RBAC wrongly gets caught today. What that misses is role.yaml gaining a permission from a new marker and the chart never following, which is exactly how #350 happened.

So, three ways: go back to the separate file and a plain diff, keep the awk, or close this and accept that gap. Given you have pushed back on it three times I am happy to just close it. Your call, no strong feelings.

[tejassinghbhati]

Rebuilt on top of the chart rename in #407, the old path was conflicting. Same change as before, just against charts/node-readiness-controller now.

Re-checked it after the move: no drift on main, and dropping events.k8s.io from the copy still fails the check with a diff pointing at the missing group. Chart tests 31/31.

[ajaysundark]

@tejassinghbhati did you consider kyaml? It seems like kyaml(left) == kyaml(right) check could help here?https://kubernetes.io/blog/2026/08/11/how-to-pretty-print-kubernetes-yaml-as-kyaml/#option-2-kubernetes-yamlfmt

Do you have time to evaluate this?

[tejassinghbhati]

I looked into kyaml / yamlfmt. While yamlfmt normalises YAML syntax (flow vs block arrays), it formats entire YAML documents. Because the chart template and config/rbac/role.yaml intentionally differ in metadata (Helm release name template, labels, namespace), a direct whole-file comparison still wouldn't work without an extraction step to isolate .rules.

Instead of introducing an external Go binary dependency for extraction, I've updated the approach to use explicit # BEGIN GENERATED RBAC RULES / # END GENERATED RBAC RULES sentinel comments in rbac.yaml.

The verify script simply extracts the block between those markers using sed and diffs against config/rbac/role.yaml. This eliminates the brittle positional awk logic completely while keeping the check simple, robust, and dependency-free.