yonahd / kor

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

chore: find kor exceptions script #294

Closed doronkg closed 3 months ago

doronkg commented 4 months ago

What this PR does / why we need it?

This PR add a hack folder into kor, to include future useful scripts for contributors. It also includes the initial script find_exceptions.sh to discover false-positive default resources, the output of this script could be merged to pkg/kor/exceptions to be ignored by kor.

This script should be used in issues like #236, #240 for intsance.

Output Example ```console $ ./find_exceptions.sh Processing completed for ConfigMaps, output saved to exceptions/configmaps.json Processing completed for Secrets, output saved to exceptions/secrets.json Processing completed for ServiceAccounts, output saved to exceptions/serviceaccounts.json $ cat exceptions/configmaps.json { "exceptionConfigMaps": [ { "Namespace": "kube-system", "ResourceName": "bootstrap" }, { "Namespace": "kube-system", "ResourceName": "cluster-config-v1" }, { "Namespace": "kube-system", "ResourceName": "openshift-service-ca.crt" }, { "Namespace": "kube-system", "ResourceName": "root-ca" } ] } ```

NOTE: As kor outputs the resources in alphabet order (by Namespace and then by ResourceName), this script outputs the exception JSONs in a sorted way as well.

PR Checklist

GitHub Issue

[XX-XX]

Notes for your reviewers

The find_exceptions.sh does not squash resources that appear in multiple namespaces, for example, if a ConfigMap is created in every namespace, the output will not display as so:

    {
      "Namespace": "*",
      "ResourceName": "kube-root-ca.crt"
    }

The script eases the process to map the default resources, but requires manual intervention (regex, merging, etc).

codecov-commenter commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 40.96%. Comparing base (5b9c270) to head (d1bddbe). Report is 3 commits behind head on main.

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

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #294 +/- ## ======================================= Coverage 40.96% 40.96% ======================================= Files 58 58 Lines 2910 2910 ======================================= Hits 1192 1192 Misses 1530 1530 Partials 188 188 ```

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

yonahd commented 3 months ago

It's a cool script. Do you think that this will be used more than a couple more times to validate putting it in the codebase?

doronkg commented 3 months ago

It's a cool script. Do you think that this will be used more than a couple more times to validate putting it in the codebase?

Yes, I believe it'd be needed even more than a couple of times, for at least the following scenarios:

  1. Adding additional methods to discover unused resources (i.e. #283. #297).
  2. Adding support for new unused resources (i.e. NetworkPolicies #296).
  3. Adding support for new K8s distributions (i.e. OKD, Rancher, Tanzu, etc).
  4. Exceptions updates for new releases of all supported K8s distributions (new resources, removal of resources).

This is a very basic script that could be enhanced (for example: auto merging outputs to pkg/kor/exceptions/ folder), and we even can adopt its logic to apply in automatic workflows in the future. It also could be modified locally to test out specific resources rather than all if needed, and it'll save some manual overhead for contributors that copy-paste outputs into the JSON files, by outputing the already formatted JSON files.

Mapping K8s exceptions should be treated as an on-going maintenance, and we should come up with more manual/automatic methods to integrate it as part of the Definition-of-Done of multiple features.

yonahd commented 3 months ago

It's a cool script. Do you think that this will be used more than a couple more times to validate putting it in the codebase?

Yes, I believe it'd be needed even more than a couple of times, for at least the following scenarios:

  1. Adding additional methods to discover unused resources (i.e. Feat: add failed jobs as unused #283. fix(pdb): fix nil pointer in pdb when selector.matchLabels is nil #297).
  2. Adding support for new unused resources (i.e. NetworkPolicies feat: find unused networkpolicies #296).
  3. Adding support for new K8s distributions (i.e. OKD, Rancher, Tanzu, etc).
  4. Exceptions updates for new releases of all supported K8s distributions (new resources, removal of resources).

This is a very basic script that could be enhanced (for example: auto merging outputs to pkg/kor/exceptions/ folder), and we even can adopt its logic to apply in automatic workflows in the future. It also could be modified locally to test out specific resources rather than all if needed, and it'll save some manual overhead for contributors that copy-paste outputs into the JSON files, by outputing the already formatted JSON files.

Mapping K8s exceptions should be treated as an on-going maintenance, and we should come up with more manual/automatic methods to integrate it as part of the Definition-of-Done of multiple features.

I agree with most of the points and the cost is not high So we'll merge