Skip to content
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

Condense RBAC rules autogenerated by kubebuilder #1771

Closed
1 of 2 tasks
mflendrich opened this issue Aug 27, 2021 · 5 comments
Closed
1 of 2 tasks

Condense RBAC rules autogenerated by kubebuilder #1771

mflendrich opened this issue Aug 27, 2021 · 5 comments
Assignees
Labels
area/maintenance Cleanup, refactoring, and other maintenance improvements that don't change functionality. pending author feedback priority/low stale Will be closed unless advocated for within 7 days

Comments

@mflendrich
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues

Problem Statement

See if the RBAC rules generated by kubebuilder here could be condensed into fewer lines for more readability.

Proposed Solution

TBD

Additional information

No response

Acceptance Criteria

  • Improved readability/auditability of k8s RBAC rules that we install in user's cluster
@mflendrich
Copy link
Contributor Author

@hbagdi says:

You don’t need permissions on events in Role resource. ClusterRole resource covers them already.

@mflendrich
Copy link
Contributor Author

Also @hbagdi says:

If we don’t use lease resource, why do we have rbac for it? I couldn’t find the lease resource in code or in the cluster. I could be very wrong here. Does controller runtime make use of it? Why do I not see it in the cluster? Maybe I’ll if I deploy using postgres?

@shaneutt shaneutt self-assigned this Aug 27, 2021
@shaneutt shaneutt added this to the Blockers for KIC 2.0 GA milestone Aug 27, 2021
@shaneutt shaneutt added area/maintenance Cleanup, refactoring, and other maintenance improvements that don't change functionality. priority/low labels Aug 27, 2021
@hbagdi
Copy link
Member

hbagdi commented Aug 27, 2021

Here are my condensed versions: https://gist.github.com/hbagdi/4aba056bb8c96c29d368a9c6eb997a8a
https://gist.github.com/hbagdi/1e9810283c153b20d70cd336b182fcca
Feel free to copy them. Please verify if you do so.

@shaneutt
Copy link
Contributor

shaneutt commented Aug 30, 2021

Breaking down all the files from this directory, since the entire directory was linked in the description:

Auth Proxy

These RBAC rules (and the relevant Service) are automatically generated when a kubebuilder init initializes a new project, and they provide access controls to restrict usage of the controller manager metrics endpoints.

Ultimately since they're part of kustomize deployment which we use only for simple tests and examples these are mostly for posterity, but that posterity is important and I don't see any reason to change these.

Viewer and Editor Roles

Each one of the <api-type>_viewer_role.yaml and <api-type>_editor_role.yaml RBAC rules provided are one-time generated manifests from kubebuilder which are meant for cluster operator consumption (e.g. an operator can give fine-grained permissions).

I see no reason to change these from what kubebuilder provides as they seem simple and well named.

Leader Election Role

This role and binding are also generated by kubebuilder init in order to support the controller manager.

@hbagdi you suggested a condensed version which looked nearly identical to what's currently present, minus a namespace. Similar to above, these RBAC rules are mostly used for posterity and we reference them chart deployments:

https://github.com/Kong/charts/blob/main/charts/kong/templates/controller-rbac-resources.yaml#L44

There we specify the templated namespace.

I don't see any compelling reason to make changes here: as with most of our kustomize configs this is for example and we derive from it in the chart, and the rules do specifically help make leadership election possible.

Manager Role

The main manager role could be condensed as suggested above but with current controller-gen this will mean that we no longer get this file automatically generated for us when we run make manifests, nor will it be lint check-able in CI (the rbac options for controller-gen take no option to compile all rules into a condensed form as shown above).

I do not feel we should be manually maintaining this file for multiple reasons:

  • we've already invested significant time to getting the generators for this right
  • this manifest is not of significant consequence to end-users: they deploy it but it's an implementation detail of the controller manager, not a role they are meant to specifically bind to or interact with

I don't see the impetus for change in these or any of the existing roles. If condensing them could be done via the automation we have in place to create and maintain them, that would be fine. We could make upstream improvements to controller-gen to make the emitted manager role cleaner, for instance, however I don't think this should be a blocker for KIC 2.0 if anything just a "nice to have" for some time when we have free time.

I propose the following new action items to consider this issue resolved, as opposed to making any changes to the files:

  • we spin up an upstream issue for improvements to controller-gen to produce a single condensed section for rbac roles as an option instead of the historical separate sections
  • we add a README.md to the config/rbac directory which provides some of the above context to help readers of the repository navigate this directory and understand where/why/how these files are used and created.

@stale
Copy link

stale bot commented Sep 15, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Will be closed unless advocated for within 7 days label Sep 15, 2021
@stale stale bot closed this as completed Sep 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/maintenance Cleanup, refactoring, and other maintenance improvements that don't change functionality. pending author feedback priority/low stale Will be closed unless advocated for within 7 days
Projects
None yet
Development

No branches or pull requests

3 participants