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

Feat: Discover unused RoleBindings #362

Merged
merged 16 commits into from
Oct 13, 2024

Conversation

nati-elmaliach
Copy link
Contributor

What this PR does / why we need it?

This is a partial PR as I ran into challenges validating whether User or Group subjects exist. As you likely know, Kubernetes doesn’t store user or group information in its resources. I could use your input—am I missing something? How can we reliably verify the existence of a user or group?

Currently, a RoleBinding is considered unused if it references a non-existent Role, ClusterRole, or if none of its ServiceAccount subjects are valid.

We might consider merging this PR as is and opening a separate issue to address User and Group validation—I'll leave that decision to you.

PR Checklist

  • This PR adds K8s exceptions (false positives)
  • This PR adds new code
  • This PR includes tests for new/existing code
  • This PR adds docs

GitHub Issue

Closes #334

Notes for your reviewers

rolebinding-feature

@codecov-commenter
Copy link

codecov-commenter commented Oct 1, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 46.08696% with 62 lines in your changes missing coverage. Please review.

Project coverage is 44.48%. Comparing base (3d63b69) to head (b8d9cf8).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/kor/rolebindings.go 60.00% 19 Missing and 13 partials ⚠️
pkg/kor/all.go 0.00% 10 Missing ⚠️
cmd/kor/rolebindings.go 0.00% 9 Missing ⚠️
pkg/kor/delete.go 0.00% 7 Missing ⚠️
pkg/kor/kor.go 71.42% 1 Missing and 1 partial ⚠️
pkg/kor/multi.go 0.00% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #362      +/-   ##
==========================================
+ Coverage   44.43%   44.48%   +0.05%     
==========================================
  Files          61       63       +2     
  Lines        3241     3356     +115     
==========================================
+ Hits         1440     1493      +53     
- Misses       1574     1622      +48     
- Partials      227      241      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@yonahd yonahd left a comment

Choose a reason for hiding this comment

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

Overall looks good.
Regarding the users and groups I agree with your implementation, we will now mark as used as we prefer this option over false positives

pkg/kor/rolebindings.go Outdated Show resolved Hide resolved
pkg/kor/rolebindings.go Outdated Show resolved Hide resolved
pkg/kor/rolebindings.go Show resolved Hide resolved
@doronkg
Copy link
Contributor

doronkg commented Oct 7, 2024

One comment from a quick glance, relating both cmd & multi -
although rb is quite intuitive, it's not an official kubectl shortname for RoleBindings.

For easier maintenance I'd suggest to remove it, see #300 for further reference.

@doronkg
Copy link
Contributor

doronkg commented Oct 7, 2024

As for users/groups - you're right, Kubernetes vanilla on its own doesn't store this data, as other distros like OpenShift do.

We'll address that separately as you suggested, also relating for the future PR to map unused ClusterRoleBindings.

@yonahd yonahd merged commit 673c21e into yonahd:main Oct 13, 2024
2 checks passed
@yonahd
Copy link
Owner

yonahd commented Oct 13, 2024

LGTM
Thanks for the contribution!

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.

feature: Discover unused RoleBindings
4 participants