yonahd / kor

A Golang Tool to discover unused Kubernetes Resources
MIT License
1.04k stars 96 forks source link

feat: add K3S exceptions #270

Closed Itaykal closed 6 months ago

Itaykal commented 6 months ago

What this PR does / why we need it

Adds exceptions to all k3s default objects.

PR Checklist

Github Issue

Fixes #259

Notes for your reviewers

Theres a default secret which is prefixed by the node's name. (e.g: nodename.node-password.k3s) As wildcards work only as an indicator for a suffix I can't really really ignore it at the current state.
I think the way forward will be one of the following options:

  1. Allow wildcards/regex in the exception handling.
  2. Allow * to be used as a prefix as well as a suffix.

I think option 1 has more risk of ignoring objects that aren't supposed to be ignored, so the second one seems more appealing to me. On the other hand, the second option is not as straightforward as well-known and used wildcards.

codecov-commenter commented 6 months ago

Codecov Report

Attention: Patch coverage is 20.00000% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 40.79%. Comparing base (6a391d3) to head (c4406e8). Report is 2 commits behind head on main.

Files Patch % Lines
pkg/kor/jobs.go 20.00% 2 Missing and 2 partials :warning:

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

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #270 +/- ## ========================================== - Coverage 40.83% 40.79% -0.04% ========================================== Files 58 58 Lines 2902 2907 +5 ========================================== + Hits 1185 1186 +1 - Misses 1531 1533 +2 - Partials 186 188 +2 ```

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

yonahd commented 6 months ago

Pull request looks good. To your point regarding nodename.node-password.k3s We were able to avoid regexes so far using the secret type(e.g. helm) Is there any chance these have a different type than the default opaque?

Otherwise we will need to consider regexes

Itaykal commented 6 months ago

Is there any chance these have a different type than the default opaque?

Unfortunately it's opaque.

yonahd commented 6 months ago

@doronkg we were looking at a similar issue with the openshift namespaces right?

doronkg commented 6 months ago

@doronkg we were looking at a similar issue with the openshift namespaces right?

Not sure which case you meant so I'll address both:

As for regex:

We were able to avoid regexes so far using the secret type(e.g. helm)

In OpenShift, we used type kubernetes.io/dockercfg to avoid this similar case.

In addition, as described in #262:

Basic OpenShift installation comes with 60+ namespaces beginning with openshift- prefix, which doesn't include additional namespaces created by OpenShift operators or customized installations, that would also be created with that prefix.

In this case we discussed excluding all the openshift- prefix namespaces, as they are loaded with default unused resources.

Itaykal commented 6 months ago

Well, the object resides in kube-system so ignoring it by namespace won't be possible, and as mentioned it is of type opaque so both solutions are not relevant for this case.

yonahd commented 6 months ago

Added a ticket for this. We can either merge this now and then we can put the information for the relevant resources on that ticket. Or we can wait

Itaykal commented 6 months ago

Added a ticket for this. We can either merge this now and then we can put the information for the relevant resources on that ticket. Or we can wait

I think merging this now will be easier to work with, changes in all wildcard exceptions will be already done in #277, sounds like an easier fix to me.

yonahd commented 6 months ago

OK. Just add this secret regex in #277 and use it as a test case