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: add K3S exceptions #270

Merged
merged 8 commits into from
May 20, 2024
Merged

Conversation

Itaykal
Copy link
Contributor

@Itaykal Itaykal commented May 18, 2024

What this PR does / why we need it

Adds exceptions to all k3s default objects.

PR Checklist

  • This PR adds K8s exceptions (false positives)
  • This PR adds new code
  • This PR includes test for any new code

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
Copy link

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 ⚠️

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

Additional details and impacted files
@@            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     

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

@yonahd
Copy link
Owner

yonahd commented May 19, 2024

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
Copy link
Contributor Author

Itaykal commented May 19, 2024

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

Unfortunately it's opaque.

@yonahd
Copy link
Owner

yonahd commented May 19, 2024

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

@doronkg
Copy link
Contributor

doronkg commented May 19, 2024

@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
Copy link
Contributor Author

Itaykal commented May 19, 2024

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
Copy link
Owner

yonahd commented May 20, 2024

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
Copy link
Contributor Author

Itaykal commented May 20, 2024

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
Copy link
Owner

yonahd commented May 20, 2024

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

@yonahd yonahd merged commit 710239d into yonahd:main May 20, 2024
1 check passed
yonahd pushed a commit that referenced this pull request Jun 15, 2024
* fix(formatOutput): fix spacing between tables (#269)

* fix(formatOutput): fix spacing between tables

* fix

* refactor

* refactor

---------

Co-authored-by: Phil Brocker <phil.brocker@gmail.com>

* feat: added regex support

* feat: updated exceptions to regex

* fix: converted all existing exceptions to be regex compatible

* fix: added support for configmaps and serviceaccounts

* fix: Configmaps and serviceaccounts are now filtered using the same methods

* fix: exceptiontype

* fix: once again wrong file

* feat: add K3S exceptions (#270)

* feat: added clusterroles

* feat: added k3s configmaps

* feat: added k3s crds

* feat: added k3s secrets

* feat: added k3s StorageClass

* feat: added job resource exceptions

* feat: added job exceptions

* fix: importing embed

* fix: fixed sa test

* fix: regex-ified job exceptions

* fix: fixed resource exception for jobs

* fix: removed kube-root-ca from test

* fix: removed default from TestRetrieveUsedSA

* fix: added regex flag to jsons

* added MatchRegex flag

* sorted all exceptoins

* fix: wrong regex expressions

* feat: added resource exception test

* fix: removed binary

---------

Co-authored-by: Phil Brocker <5331286+pbr0ck3r@users.noreply.github.com>
Co-authored-by: Phil Brocker <phil.brocker@gmail.com>
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.

Map false unused resources: K3s
4 participants