Skip to content

refactor(charts): Change generation of RBAC templates#1520

Merged
viccuad merged 7 commits intokubewarden:mainfrom
jvanz:issue1483
Mar 30, 2026
Merged

refactor(charts): Change generation of RBAC templates#1520
viccuad merged 7 commits intokubewarden:mainfrom
jvanz:issue1483

Conversation

@jvanz
Copy link
Copy Markdown
Member

@jvanz jvanz commented Feb 26, 2026

Description

Updates the controller-gen command used to generated the RBAC manifests into the charts/kubewarden-controller directory. This commit also adds the kubebuilder markers to add the missing permissions in the controller roles that was added using manully created roles definitions.

Fix #1483

@jvanz jvanz self-assigned this Feb 26, 2026
@jvanz jvanz added the ci-full Run all CI jobs for all languages. label Feb 26, 2026
@jvanz jvanz changed the title fix(charts): controller-gen output file into charts. chore(charts): controller-gen output file into charts. Feb 26, 2026
@jvanz jvanz removed the kind/bug label Feb 26, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 27, 2026

Codecov Report

❌ Patch coverage is 14.28571% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.52%. Comparing base (300ffd9) to head (de6d5f6).
⚠️ Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
internal/controller/policyserver_controller.go 0.00% 4 Missing and 4 partials ⚠️
internal/controller/policy_subreconciler.go 33.33% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1520   +/-   ##
=======================================
  Coverage   75.52%   75.52%           
=======================================
  Files         170      170           
  Lines       20902    20898    -4     
=======================================
- Hits        15786    15783    -3     
+ Misses       4904     4902    -2     
- Partials      212      213    +1     
Flag Coverage Δ
go-tests 57.49% <14.28%> (-0.04%) ⬇️
rust-tests 80.45% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jvanz jvanz marked this pull request as ready for review February 27, 2026 19:04
@jvanz jvanz requested a review from a team as a code owner February 27, 2026 19:04
Comment thread charts/kubewarden-controller/templates/NOTES.txt Outdated
@jvanz jvanz force-pushed the issue1483 branch 5 times, most recently from 91c845d to 4e819cd Compare February 27, 2026 23:29
Comment thread charts/kubewarden-controller/templates/controller-rbac-extras.yaml Outdated
Comment thread scripts/post-generate-manifests.sh Outdated
Comment thread scripts/post-generate-manifests.sh Outdated
@flavio
Copy link
Copy Markdown
Member

flavio commented Mar 2, 2026

Also, I got a bit confused by the names:

  • charts/kubewarden-controller/templates/controller-rbac-roles.yaml: this holds all the roles (cluster and not)
  • charts/kubewarden-controller/templates/controller-rbac-extras.yaml: this holds all the roles bindings and one ClusterRole
  • charts/kubewarden-controller/templates/audit-scanner-rbac.yaml: contains roles and bindings

I would like, if possible, to end up with something like sbomscanner:

In our case we would have 4 files:

  • audit-scanner-role.yaml
  • audit-scanner-rolebinding.yaml
  • controller-role.yaml
  • controller-rolebinding.yaml

@jvanz
Copy link
Copy Markdown
Member Author

jvanz commented Mar 2, 2026

Also, I got a bit confused by the names:

* `charts/kubewarden-controller/templates/controller-rbac-roles.yaml`: this holds all the roles (cluster and not)

* `charts/kubewarden-controller/templates/controller-rbac-extras.yaml`: this holds all the roles bindings and one `ClusterRole`

* `charts/kubewarden-controller/templates/audit-scanner-rbac.yaml`: contains roles and bindings

I would like, if possible, to end up with something like sbomscanner:

* a file with [roles](https://github.com/kubewarden/sbomscanner/blob/main/charts/sbomscanner/templates/controller/role.yaml)

* a file with [rolebindings](https://github.com/kubewarden/sbomscanner/blob/main/charts/sbomscanner/templates/controller/rolebinding.yaml)

In our case we would have 4 files:

* `audit-scanner-role.yaml`

* `audit-scanner-rolebinding.yaml`

* `controller-role.yaml`

* `controller-rolebinding.yaml`

I'm fine splitting role and rolebindings. I actually did that while I was working on this PR. But I've changed that because I would like to avoid two role files. The controller-rbac-roles.yaml has the roles generated by the controller-gen. It's overwritten every time we run make manifests. Then, instead of having two role files, one for the controller-gen output and another to the "metrics-reader" role, I've decided to rename the file to controller-rbac-extras.yaml. Where it has all the rolebindings and one additional role. Therefore, I avoid more custom code to merge all the roles into a single file.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 26 out of 28 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread e2e/main_test.go Outdated
Comment thread cmd/controller/main.go Outdated
Comment thread internal/controller/policyserver_controller.go Outdated
Comment thread charts/kubewarden-controller/values.yaml
@jvanz jvanz force-pushed the issue1483 branch 4 times, most recently from 4c1795a to 8892c00 Compare March 26, 2026 02:54
@jvanz jvanz requested a review from Copilot March 26, 2026 02:54
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 28 out of 30 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cmd/controller/main.go Outdated
Comment thread .github/workflows/ci.yml
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 29 out of 31 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cmd/controller/main.go Outdated
Comment thread Makefile
Comment on lines +152 to +153
paths="./api/policies/v1" paths="./internal/controller" paths="./cmd/controller" \
output:crd:artifacts:config=charts/kubewarden-crds/templates \
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make manifests now writes CRDs/RBAC into the Helm charts, but it no longer updates the existing kubebuilder/kustomize manifests under config/crd/bases and config/rbac (which are still referenced by config/**/kustomization.yaml). This can leave the kustomize install path stale/out-of-sync (e.g., config/rbac/role.yaml still lacks the webhook get verb). Either keep generating to config/ as well, or remove/deprecate the config/ kustomize overlays to avoid drift.

Suggested change
paths="./api/policies/v1" paths="./internal/controller" paths="./cmd/controller" \
output:crd:artifacts:config=charts/kubewarden-crds/templates \
paths="./api/policies/v1" paths="./internal/controller" paths="./cmd/controller" \
output:crd:artifacts:config=config/crd/bases \
output:crd:artifacts:config=charts/kubewarden-crds/templates \
output:rbac:artifacts:config=config/rbac \

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We plan to remove the config directory in the future. So, we can ignore this comment for now.

Comment thread e2e/main_test.go Outdated
@jvanz jvanz force-pushed the issue1483 branch 2 times, most recently from 6d68425 to 2519539 Compare March 26, 2026 12:56
Comment thread internal/controller/client_interceptor.go Outdated
Split the monolithic rbac.yaml into dedicated files for controller roles,
bindings, metrics, and audit scanner. Redirect controller-gen output
directly into chart templates with sed post-processing for Helm labels,
annotations, and namespace templating. Rename CRD files to include the
API group prefix and fix CRD comments.

Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>
Assited-by: Github Copilot
jvanz added 6 commits March 27, 2026 10:27
Clean up main.go imports and add kubebuilder RBAC markers for
controller-level permissions (leases, configmaps, events, tokenreviews,
subjectaccessreviews).

Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>
Assited-by: Github Copilot
Fix error handling in getPolicies() which was only returning errors for
IsNotFound, silently swallowing all other error types. Now all errors are
properly propagated. Add explicit namespace scoping to List calls for
ReplicaSets and Pods in isPolicyUniquelyReachable() to avoid unnecessary
cluster-wide lookups.

Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>
Assited-by: Github Copilot
The controller calls Get() on ValidatingWebhookConfiguration and
MutatingWebhookConfiguration during reconciliation and deletion, but the
kubebuilder RBAC markers were missing the 'get' verb. With caching enabled
this went unnoticed because list/watch cache served the reads. Add 'get'
to the markers and regenerate the chart RBAC manifest.

Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>
Assited-by: Github Copilot
Add helm unit tests that validate the generated CRD and RBAC manifests.
CRD tests verify resource type, name, and API group. RBAC tests verify
that ClusterRoles and Roles include the expected Helm labels and that
additionalLabels and additionalAnnotations are correctly propagated.

Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>
Assited-by: Github Copilot
Add a new additionalEnvironmentVariables Helm value that allows users to
inject arbitrary environment variables into the controller deployment.
Includes schema validation and helm unit tests.

Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>
Assited-by: Github Copilot
Pin the kwctl-installer GitHub Action to v4.6.1 for reproducible e2e
test environments. Add a CI step that runs 'make manifests' and verifies
the generated chart files are up to date. Fix autolabeler workflow
permissions to use job-level scoping.

Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>
Assited-by: Github Copilot
Copy link
Copy Markdown
Member

@viccuad viccuad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@viccuad viccuad merged commit 4f6ce34 into kubewarden:main Mar 30, 2026
47 of 49 checks passed
@viccuad viccuad changed the title chore(charts): controller-gen output file into charts. refactor(charts): Change generation of RBAC templates Apr 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-full Run all CI jobs for all languages. kind/chore

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rename CRD and RBAC files to match kubebuilder/controller-gen defaults

5 participants