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 down the rbac permissions for schedualer #2024

Merged
merged 1 commit into from
Dec 19, 2024

Conversation

blumamir
Copy link
Collaborator

@blumamir blumamir commented Dec 18, 2024

In this PR:

  • move destinations and collectorsgroup rbac permission to be role (on odigos ns) instead of clusterrole. These live anyway only in our ns, but it looks better in rbac reviews.
  • removed unused create/delete/update/patch for the destinations, as we only read them.
  • currently the leader election role brings in configmaps permissions, but I also added the permissions we need for the reconcilers so not to mix unrelated things. we will need to also understand and address the leader election role later on.
  • removed the finalizers permissions, as we don't use finalizers and it's unused.
  • created some consts in cli files for better structure to the references
  • synced helm files with the changes
  • tested both helm and cli locally to make sure it works

InstrumentationConfig is left as a clusterrole, since they belong to the various namespaces where the sources reside.

Namespace: ns,
},
}
}

func NewSchedulerRoleBinding(ns string) *rbacv1.RoleBinding {
func NewSchedulerLeaderElectionRoleBinding(ns string) *rbacv1.RoleBinding {
return &rbacv1.RoleBinding{
Copy link
Contributor

Choose a reason for hiding this comment

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

could we just embed the role and rolebinding yamls and marshal them to the API types? Then we can keep them in one spot that's a little more readable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, they are currently duplicated between helm and cli. we sync them manually but need to find a better way to have them just once

Copy link
Contributor

Choose a reason for hiding this comment

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

Opened #2047 to track (gh issue since it's not high priority)

@blumamir blumamir merged commit 371c5ab into odigos-io:main Dec 19, 2024
30 of 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.

3 participants