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

[GEN-2110]: update RBAC permissions for UI #2058

Merged
merged 6 commits into from
Dec 24, 2024

Conversation

BenElferink
Copy link
Contributor

This pull request includes significant updates to the role and cluster role definitions in the cli/cmd/resources/ui.go file. The changes primarily involve refining the permissions for various resources to better align with the required operations.

Changes to Role and ClusterRole Definitions:

  • cli/cmd/resources/ui.go: Updated the NewUIRole function to refine the permissions for accessing configmaps, secrets, and odigos.io resources. This includes adding comments to clarify the necessity of each permission set.
  • cli/cmd/resources/ui.go: Updated the NewUIClusterRole function to refine the permissions for accessing namespaces, services, pods, replicasets, and odigos.io resources. Comments were added to explain the purpose of each permission set.

Copy link

Copy link
Collaborator

@blumamir blumamir left a comment

Choose a reason for hiding this comment

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

Thanks ben, really love the comments you've added to explain why each permission is used.

Also great that you merged few permissions into one to make it more compact and readable.

Left few more suggestions for additional improvement we can apply - to have minimal permissions, and ask for only what we need and use. this can help mitigate future bugs and looks better in security reviews.

The permissions modified here are for installing odigos with cli. for helm, we maintain these permissions in another directory which we currently need to sync manually. you can run kubectl get role -n odigos-system odigos-ui -o yaml and copy it (with minor changes to the metadata) to sync the helm template as well (do it also for clusterrole).

To test your changes, you can uninstall odigos and then run make helm-install from odigos root directory.

cli/cmd/resources/ui.go Outdated Show resolved Hide resolved
cli/cmd/resources/ui.go Show resolved Hide resolved
cli/cmd/resources/ui.go Outdated Show resolved Hide resolved
cli/cmd/resources/ui.go Outdated Show resolved Hide resolved
cli/cmd/resources/ui.go Outdated Show resolved Hide resolved
cli/cmd/resources/ui.go Outdated Show resolved Hide resolved
@BenElferink
Copy link
Contributor Author

Thanks @blumamir
I made changes based on all the comments 😃

@BenElferink BenElferink requested a review from blumamir December 24, 2024 07:29
@BenElferink BenElferink merged commit 94e1e05 into odigos-io:main Dec 24, 2024
32 checks passed
@BenElferink BenElferink deleted the gen-2110 branch January 6, 2025 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants