search

yonahd / kor

A Golang Tool to discover unused Kubernetes Resources
MIT License
974 stars 91 forks source link

feat(namespaces): add empty namespace detection and removal #249

Open isindir opened 5 months ago

isindir commented 5 months ago

fixes #92

This PR is still WIP, as it lacks parallel processing of the namespaces as well as unit tests, could you please review if in principle it is what you'd be happy to accept ?

yonahd commented 5 months ago

fixes #92

This PR is still WIP, as it lacks parallel processing of the namespaces as well as unit tests, could you please review if in principle it is what you'd be happy to accept ?

Hey @isindir I'll take a look at the code in the next couple of days

doronkg commented 5 months ago

Hi @isindir, In order for the new feature to be completed, several additional files should be modified accordingly.

Take a look at https://github.com/yonahd/kor/blob/main/CONTRIBUTING.md#repository-structure.

codecov-commenter commented 5 months ago

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

Codecov Report

Attention: Patch coverage is 43.31797% with 123 lines in your changes missing coverage. Please review.

Project coverage is 42.13%. Comparing base (181141d) to head (d5d757e). Report is 1 commits behind head on main.

Files Patch % Lines
pkg/kor/namespaces.go 47.15% 65 Missing :warning:
pkg/kor/delete.go 13.88% 31 Missing :warning:
cmd/kor/namespaces.go 0.00% 27 Missing :warning:

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

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #249 +/- ## ========================================== + Coverage 42.01% 42.13% +0.11% ========================================== Files 61 63 +2 Lines 3175 3387 +212 ========================================== + Hits 1334 1427 +93 - Misses 1633 1752 +119 Partials 208 208 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

yonahd commented 5 months ago

fixes #92

This PR is still WIP, as it lacks parallel processing of the namespaces as well as unit tests, could you please review if in principle it is what you'd be happy to accept ?

This is definitely a good direction(this is not a simple one). Definitely need to review this thoroughly but in principal this is good

isindir commented 5 months ago

@doronkg,

Hi @isindir, In order for the new feature to be completed, several additional files should be modified accordingly.

Take a look at https://github.com/yonahd/kor/blob/main/CONTRIBUTING.md#repository-structure.

Helm chart I think needs extra changes:

I'm looking to work on unit tests if time allows these weekends.

doronkg commented 4 months ago

Helm chart I think needs extra changes:

  • at the moment it is not possible to configure kor in a deployment for example to automatically remove any orphaned resources - rbac lacks such permissions.

Correct, currently the deployment is only used to export metrics, hence read-only RBAC, but we can consider going that way.

  • I think it would be more useful to split cronjobs so that several such jobs are configured with different schedules (may become handy when removing some resources and then namespaces in a separate run.

Interesting, especially when scanning unused resources in high-scaled clusters that might take more time than the interval set.

We can continue with progress on both comments in separated issues/discussions (or in our Discord channel) as they are out-of-scope for this PR, but should be dippen into. I'd like to futher discuss them.

Referring to https://github.com/yonahd/kor/blob/main/CONTRIBUTING.md#repository-structure was made to make sure you modify all required files, as adding any new unused resource support should address them.

isindir commented 4 months ago

@doronkg ,

Referring to https://github.com/yonahd/kor/blob/main/CONTRIBUTING.md#repository-structure was made to make sure you modify all required files, as adding any new unused resource support should address them.

Thank you, I think I covered most of the files except helm chart and some unit tests, atm I'm trying to figure out if I can use fake clients to reliably test a feature I'm implementing. I deliberately left chart untouched as the change already looks very big.

btw, for chart - it will be better to have fine-tuned RBAC - possibility to enable only those read/write/per namespace permissions per feature (resource).

isindir commented 3 months ago

@yonahd @luisdavim @doronkg, I finally found time and adding some more testing to the namespace removal - but I was not following if json configs with resources to skip from namespace evaluation for different k8s distros is fully implemented or not. Could you please suggest and review if my testing in pkg/kor/namespacesMoreTests_test.go is acceptable ? Thanks

yonahd commented 3 months ago

@yonahd @luisdavim @doronkg, I finally found time and adding some more testing to the namespace removal - but I was not following if json configs with resources to skip from namespace evaluation for different k8s distros is fully implemented or not. Could you please suggest and review if my testing in pkg/kor/namespacesMoreTests_test.go is acceptable ? Thanks

The resource exceptions are merged fully. Not sure we will use the namespaces exceptions as we worked around it

yonahd commented 3 months ago

Let me know when this is ready for review. This is a massive pr and will take quite a while to review

isindir commented 2 months ago

@yonahd I feel that I'll not be able to do 100% unit test coverage for my new code, please review this PR. Thanks.