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

chore: narrow the RBAC permissions used by the autoscaler #2039

Merged
merged 7 commits into from
Dec 20, 2024

Conversation

blumamir
Copy link
Collaborator

This reduces the permissions we request to just those we actually use.

Most resources are moved from cluster role to role. only resource we need in cluster level is instrumentationconfigs to update the datacollection configmaps with file patterns to scrape for logs.

Updated the selectors in controller runtime cache.

Tested several flows with CLI and helm.

@@ -2,12 +2,22 @@ apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
name: odigos-autoscaler
namespace: {{ .Release.Namespace }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this removed?
All the permissions here are only for our ns, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

helm will automatically apply the resource in the configured namespace, but added it back so it's aligned with other resource in the helm template

@@ -22,7 +32,6 @@ rules:
- apps
resources:
- daemonsets
- deployments
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need different permissions for deployments and daemonseta, or is it a separation just for readability?

Copy link
Collaborator Author

@blumamir blumamir Dec 20, 2024

Choose a reason for hiding this comment

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

It's align with how to CLI sets up these permissions, I just fixed in CLI and copied the content from kubectl get role -o yaml

We can merge them in both CLI and helm, but prefer not to do it as part of the current effort

@blumamir blumamir merged commit 6497397 into odigos-io:main Dec 20, 2024
32 checks passed
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