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

chore: find kor exceptions script #294

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

doronkg
Copy link
Contributor

@doronkg doronkg commented Jun 3, 2024

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
$ ./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

  • 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

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

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

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

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

@yonahd
Copy link
Owner

yonahd commented Jun 6, 2024

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

doronkg commented Jun 7, 2024

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.

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.

Lgtm

@yonahd
Copy link
Owner

yonahd commented Jun 7, 2024

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

@yonahd yonahd merged commit 4f3d099 into yonahd:main Jun 7, 2024
1 check passed
@doronkg doronkg deleted the find-exceptions-script branch June 18, 2024 18:51
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.

None yet

3 participants