Include CRD viewer and editor roles in kustomization file?

[joelanford]

What do you want to happen?

When I create a new API, kubebuilder automatically generates viewer and editor roles that could be used to give end users access to the API.

However, it doesn't look like Kubebuilder adds references to these new files in the ./config/rbac/kustomization.yaml file, which means they don't end up being applied as part of the kustomize build + kubectl command.

It would be nice if Kubebuilder automatically added these to the kustomization file when it creates them.

Extra Labels

No response

[camilamacedo86]

Hi @joelanford,

Thank you for bringing this to our attention.

Upon initial consideration, I saw merit in the suggestion as a means to ensure syntax correctness during test executions. However, after a moment of reflection, I chose to delete my previous comment and remove the label to prevent any confusion.

The purpose of those scaffolds

Those rules are merely helpers; they are not utilized by the manager itself. Indeed, they can be removed from the project. The individuals who could benefit from them are those operating the solution. More information about its motivation can be found at #886.

So, why should we add those to the kustomize configuration for the Manager? What advantages would it bring for the users to have these applied when we deploy the manager? What is the use case?

To be honest, I also do not know how much value these rules bring, since their purpose is not very clear. I wonder if we should actually not remove them in a future version. WDYT?

[joelanford]

I had the same understanding as the stated purpose you mention. I'm not opposed to removing them in a future version, but I also see the value in them.

If these scaffolds were in the kustomization file and did get applied as part of the installation, there would be one less step for cluster admins to provide access to the APIs of a newly installed controller. An admin would just have to create (cluster)rolebindings that reference them.

Without these, an admin would have to also manually create them or derive them some other way.

I would propose one of the following:

1. Create and reference them in the kustomization file during create api, fully enabled and ready to go.
2. Create and reference them in the kustomization file during create api but commented out with an explanation about why a developer might want to uncomment them (or delete them).
3. Stop creating them in the first place.

If we choose options 1 or 2, we could explain the purpose of them in the kustomization file or as a comment in the RBAC YAML.

[camilamacedo86]

Hi @joelanford

Yep, I go along with your thoughts.

Honest, I cannot think how option 1 could bring any harm.
Maybe, apply what is not necessary to the cluster?
Did you see any cons/downsides beyond that?

Furthermore, I think it would be great to have comments in the kustomize where those are applied to try to bring clarity about their purpose. Therefore, I think I am fine either with us moving forward with option 1:

• a) Add it to be applied by default via the kustomize (so that we will start to cover at least the syntax in our e2e tests as well)
• b) Then, ensure that we have a nice comment explaining its purpose/motivation: Generate suggested Roles for CR users #886

It seems a good one for the first good issues, too.
I am adding the label.

[lunarwhite]

Hi @camilamacedo86, I'd like to help with this issue, and want to comfirm:

How/When should *_viewer_role.yaml and *_editor_role.yaml be added to rbac/kustomization file?

a. By init. One way is to directly modify kustomizeRBACTemplate:

kubebuilder/pkg/plugins/common/kustomize/v2/scaffolds/internal/templates/config/rbac/kustomization.go

Lines 45 to 63 in 8afeb40

	const kustomizeRBACTemplate = `resources:
	# All RBAC will be applied under this service account in
	# the deployment namespace. You may comment out this resource
	# if your manager will use a service account that exists at
	# runtime. Be sure to update RoleBinding and ClusterRoleBinding
	# subjects if changing service account names.
	- service_account.yaml
	- role.yaml
	- role_binding.yaml
	- leader_election_role.yaml
	- leader_election_role_binding.yaml
	# Comment the following 4 lines if you want to disable
	# the auth proxy (https://github.com/brancz/kube-rbac-proxy)
	# which protects your /metrics endpoint.
	- auth_proxy_service.yaml
	- auth_proxy_role.yaml
	- auth_proxy_role_binding.yaml
	- auth_proxy_client_clusterrole.yaml
	`

b. By create api. We might implement it after CRDEditorRole and CRDViewerRole are generated:

kubebuilder/pkg/plugins/common/kustomize/v2/scaffolds/api.go

Lines 64 to 104 in 8afeb40

	func (s *apiScaffolder) Scaffold() error {
	log.Println("Writing kustomize manifests for you to edit...")
	// Initialize the machinery.Scaffold that will write the files to disk
	scaffold := machinery.NewScaffold(s.fs,
	machinery.WithConfig(s.config),
	machinery.WithResource(&s.resource),
	)
	// Keep track of these values before the update
	if s.resource.HasAPI() {
	if err := scaffold.Execute(
	&samples.CRDSample{Force: s.force},
	&rbac.CRDEditorRole{},
	&rbac.CRDViewerRole{},
	&crd.Kustomization{},
	&crd.KustomizeConfig{},
	); err != nil {
	return fmt.Errorf("error scaffolding kustomize API manifests: %v", err)
	}
	// If the gvk is non-empty
	if s.resource.Group != "" || s.resource.Version != "" || s.resource.Kind != "" {
	if err := scaffold.Execute(&samples.Kustomization{}); err != nil {
	return fmt.Errorf("error scaffolding manifests: %v", err)
	}
	}
	kustomizeFilePath := "config/default/kustomization.yaml"
	err := pluginutil.UncommentCode(kustomizeFilePath, "#- ../crd", `#`)
	if err != nil {
	hasCRUncommented, err := pluginutil.HasFragment(kustomizeFilePath, "- ../crd")
	if !hasCRUncommented || err != nil {
	log.Errorf("Unable to find the target #- ../crd to uncomment in the file "+
	"%s.", kustomizeFilePath)
	}
	}
	}
	return nil
	}

Please correct me if I misunderstood. Any additional suggestions would be appreciated.

[camilamacedo86]

Hi @lunarwhite,

First, that is great. We appreciate your help.
The view/editor roles are created for each API, so we create them when we create the APIs.
Therefore, it seems the place where we should do that.

[lunarwhite]

/assign