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

Fix ClusterRole & ClusterRoleBinding hotreload #92

Merged
merged 21 commits into from
Nov 25, 2021

Conversation

vigohe
Copy link
Contributor

@vigohe vigohe commented Nov 23, 2021

Q A
Bug fix? yes
New feature? yes
API breaks? no
Deprecations? no
Related tickets fixes #91
License Apache 2.0

What's in this PR?

fixes #91 and deletes ClusterRoles when CustomGroup name is not present at the config

Why?

  • Hot Reload is not working as intended
  • When a CustomGroup it's deleted it should also delete the related ClusterRole and ClusterRoleBinding or RoleBinding

Checklist

  • Code meets the Developer Guide
  • User guide and development docs updated (if needed)
  • Related Helm chart(s) updated (if needed)

Signed-off-by: Victor  Godoy Hernández <[email protected]>
Signed-off-by: Victor  Godoy Hernández <[email protected]>
Signed-off-by: Victor  Godoy Hernández <[email protected]>
Signed-off-by: Victor  Godoy Hernández <[email protected]>
@pbalogh-sa
Copy link
Member

@vigohe Thanks for contributions. I will check it soon.

@vigohe
Copy link
Contributor Author

vigohe commented Nov 24, 2021

@vigohe Thanks for contributions. I will check it soon.

Nice!

Do you know a better way to delete as cascade all resources related to a ClusterRole, because the only way I found, was to label each resource ( ClusterRoleBinding & RoleBinding ) with the label crole. Then search by the label and delete them.

@pbalogh-sa
Copy link
Member

@vigohe I think you can set the ownerreference of rolebindings to the proper role or clusterrole. In this case when you delete the clusterrole or role, the rolebindings will be deleted. This doesn't work in the case of clusterscope resources if the K8s version is 1.21 or above.

Copy link
Member

@pbalogh-sa pbalogh-sa left a comment

Choose a reason for hiding this comment

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

@vigohe Using labels works in the case of cluster-scoped resources as well (for example clusterrolebindings). I think it's an universal solution . Thanks.
Please resolve conflicts otherwise LGTM.

Copy link
Member

@pbalogh-sa pbalogh-sa left a comment

Choose a reason for hiding this comment

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

Please resolve conflicts otherwise LGTM.

@vigohe
Copy link
Contributor Author

vigohe commented Nov 25, 2021

Please resolve conflicts otherwise LGTM.

Please resolve conflicts otherwise LGTM.

Did I fix it ?

@pbalogh-sa
Copy link
Member

@vigohe Some conflicts are found. Please resolve them.

@vigohe
Copy link
Contributor Author

vigohe commented Nov 25, 2021

@vigohe Some conflicts are found. Please resolve them.

Don't see any conflicts

@pbalogh-sa
Copy link
Member

@vigohe . I can squash and merge instead of rebasing.

@pbalogh-sa pbalogh-sa merged commit b18369b into banzaicloud:master Nov 25, 2021
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.

Hot reload of customGroups not working
2 participants