Skip to content

🐛 (helm/v1-alpha): Use namePrefix from config/default/kustomization.yaml as prefix for RBAC rules and project name only if this value cannot be found. #4571

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

camilamacedo86
Copy link
Member

Closes: #4566

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 20, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 20, 2025
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 20, 2025
@camilamacedo86
Copy link
Member Author

HI @mkarlheim

Could you please check this one?

@camilamacedo86 camilamacedo86 force-pushed the use-prefix-name branch 2 times, most recently from 03a08a9 to 8775274 Compare February 20, 2025 10:49
@camilamacedo86 camilamacedo86 changed the title 🐛 (helm/v1-alpha): Use namePrefix from kustomization.yaml instead of project name in RBAC and Helm templates helm/v1-alpha): Use namePrefix from config/default/kustomization.yaml as prefix for RBAC rules and project name only if this value cannot be found. Feb 20, 2025
@camilamacedo86 camilamacedo86 changed the title helm/v1-alpha): Use namePrefix from config/default/kustomization.yaml as prefix for RBAC rules and project name only if this value cannot be found. 🐛 (helm/v1-alpha): Use namePrefix from config/default/kustomization.yaml as prefix for RBAC rules and project name only if this value cannot be found. Feb 20, 2025
@mkarlheim
Copy link

HI @mkarlheim

Could you please check this one?

Awesome, thanks! I'll try to do it shortly.

@mkarlheim
Copy link

mkarlheim commented Mar 6, 2025

Hi @camilamacedo86,

using the namePrefix by default and the project name as fallback is an improvement, but the core issue still exists in my opinion:
Executing kubebuilder create api --group testgroup --version v1 --kind Test leads to newly generated ClusterRoles named

  • name: testgroup-test-admin-role
  • name: testgroup-test-editor-role
  • name: testgroup-test-viewer-role

So these generated ClusterRoles are not considered with adding a prefix at all. There must be something like a regex replace like .*-editor-role as the names of the ClusterRoles can differ depending on the apis and apigroups in the kubebuilder project.

@camilamacedo86
Copy link
Member Author

camilamacedo86 commented Mar 6, 2025

Hi @mkarlheim

Executing kubebuilder create api --group testgroup --version v1 --kind Test leads to newly generated ClusterRoles named

When you change your project you need to re-run the helm plugin
The chart will not be updated by itself.

Also, ensure that you run make manifests and make generate before re-run the helm plugin.
The Chart is only updated when you say for it be done.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 6, 2025
@camilamacedo86
Copy link
Member Author

Closing because I think we will need to fix it another way, by changing the scaffolds and adding a prefix in the chart values instead.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2025
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 26, 2025
@camilamacedo86
Copy link
Member Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 26, 2025
@camilamacedo86
Copy link
Member Author

camilamacedo86 commented May 26, 2025

Hi @mkarlheim

Can you please verify this one now?
It seems that it addresses your need for now.

However, ultimately, we will need to change the Helm plugin implementation to generate the chart from the kustomize install, using YAML instead. See; #4833

@camilamacedo86 camilamacedo86 requested a review from mkarlheim May 26, 2025 17:54
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 27, 2025
…l as prefix for RBAC rules and project name only if this value cannot be found.
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 27, 2025
@mkarlheim
Copy link

mkarlheim commented Jun 4, 2025

Hi @camilamacedo86,

many thanks for your efforts!

Unfortunately, it is still not the expected behaviour. All roles that are created with the Kustomize/v2 plugin, don't have a prefix in their generated yaml by default.

All the roles are prefixed in the moment when the Kustomize build (make build-installer) is executed as you described in #4833.

The function copyFileWithHelmLogic only considers the roles with fixed names like the metrics, election and leader roles, but not the crd-roles (admin, editor, viewer). The name of these roles is not fixed as it contains resource group and kind (in multgroup projects).

I was also thinking about using the Kustomize build output as the base for the Helm chart generation, as this would adress the issue with the namePrefix.

Another argument for your proposal would be:
In the config/rbac/kustomization.yaml there is a comment saying You can comment the following lines if you do not want those helpers be installed with your Project. Those types of changes wouldn't be considered as well, if someone commented out some roles (e.g.). Using the Kustomize build output as base for the Helm chart generation would also address those types of changes.

Best
Michael

@camilamacedo86
Copy link
Member Author

/test pull-kubebuilder-e2e-k8s-1-33-0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(helm/v1-alpha) Nameprefix is missing in editor/viewer ClusterRoles after scaffolding
3 participants