yonahd / kor

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

feat: find unused networkpolicies #296

Closed tthvo closed 3 months ago

tthvo commented 3 months ago

What this PR does / why we need it?

This PR added a new functionality to kor to find unused NetworkPolicies. A NetworkPolicy is considered unused when its label selector selects 0 pod or it has label "kor/used"="false".

kor version: vdev

  _  _____  ____
 | |/ / _ \|  _ \
 | ' / | | | |_) |
 | . \ |_| |  _ <
 |_|\_\___/|_| \_\

Unused resources in namespace: "default"
+---+---------------+---------------------+
| # | RESOURCE TYPE |    RESOURCE NAME    |
+---+---------------+---------------------+
| 1 | NetworkPolicy | test-network-policy |
+---+---------------+---------------------+

PR Checklist

GitHub Issue

Closes #279

Notes for your reviewers

This could be extended to check pods' status (e.g. Succeeded) but I am keeping it simple for now. Please let me know what you think^^

codecov-commenter commented 3 months ago

Codecov Report

Attention: Patch coverage is 47.25275% with 48 lines in your changes missing coverage. Please review.

Project coverage is 41.38%. Comparing base (107c885) to head (9c7a0b7).

Files Patch % Lines
pkg/kor/networkpolicies.go 62.26% 13 Missing and 7 partials :warning:
pkg/kor/all.go 0.00% 10 Missing :warning:
cmd/kor/networkpolicies.go 0.00% 9 Missing :warning:
pkg/kor/delete.go 0.00% 7 Missing :warning:
pkg/kor/multi.go 0.00% 2 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 #296 +/- ## ========================================== + Coverage 41.21% 41.38% +0.17% ========================================== Files 59 61 +2 Lines 3113 3204 +91 ========================================== + Hits 1283 1326 +43 - Misses 1627 1668 +41 - Partials 203 210 +7 ```

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

doronkg commented 3 months ago

@yonahd we should address the following before merging the feature or in future releases:

  1. Consider mapping false-positive NetworkPolicies in multiple distros and supporting it in pkg/kor/exceptions/networkpolicies.
  2. Consider discovering second-level unused NetworkPolicies, for example, a NetworkPolicy with matching .spec.podSelector, but unmatching:
    • .spec.ingress.from.podSelector
    • .spec.ingress.from.namespaceSelector (unsuppoted yet, see #249)
    • .spec.egress.to.podSelector
    • .spec.egress.to.namespaceSelector (unsuppoted yet, see #249)
tthvo commented 3 months ago

Thanks for the reviews! I have addressed them in the latest commits.

@yonahd we should address the following before merging the feature or in future releases:

1. Consider mapping false-positive NetworkPolicies in multiple distros and supporting it in `pkg/kor/exceptions/networkpolicies`.

2. Consider discovering second-level unused NetworkPolicies, for example, a NetworkPolicy with matching `.spec.podSelector`, but unmatching:

* `.spec.ingress.from.podSelector`

* `.spec.ingress.from.namespaceSelector` (unsuppoted yet, see [#249](#249))

* `.spec.egress.to.podSelector`

* `.spec.egress.to.namespaceSelector` (unsuppoted yet, see [#249](#249))

I could help handling these cases. Just let me know if you prefer to address these in this PR or a separate one :D

yonahd commented 3 months ago

Thanks for the reviews! I have addressed them in the latest commits.

@yonahd we should address the following before merging the feature or in future releases:

1. Consider mapping false-positive NetworkPolicies in multiple distros and supporting it in `pkg/kor/exceptions/networkpolicies`.

2. Consider discovering second-level unused NetworkPolicies, for example, a NetworkPolicy with matching `.spec.podSelector`, but unmatching:

* `.spec.ingress.from.podSelector`

* `.spec.ingress.from.namespaceSelector` (unsuppoted yet, see [#249](#249))

* `.spec.egress.to.podSelector`

* `.spec.egress.to.namespaceSelector` (unsuppoted yet, see [#249](#249))

I could help handling these cases. Just let me know if you prefer to address these in this PR or a separate one :D

The cases in the second point should be addressed in this PR.

The exceptions can be a separate one.

doronkg commented 3 months ago

When rethinking it, it's irrelevant if unused namespaces are unsupported in kor at the moment, the label selector discovery is unrelated, hence it could be featured in this PR as well.

In addition, the second-level discovery could be tricky for the following reasons:

  1. Performance - each NetworkPolicy can be set with multiple ingress/egress rules hence a wide list of labels to iterate.
  2. False-positives - if a NetworkPolicy is set with multiple ingress/egress rules, and say only one label in the list doesn't match existing pods/namespaces, it could be mapped as unused even if the other rules do apply and potentially be deleted.

In cases where multiple label selectors / rules are set in a NetworkPolicy, they are "OR'd", meaning if at least one of them matches, the NetworkPolicy is applied and should be considered as used.

Therefore, a NetworkPolicy should be considered as second-level unused when .spec.podSelector matches existing pods, but none of the ingress/egress rules matches existing pods/namespaces. In other words, the NetworkPolicy handles no traffic to the matched pods.

@yonahd what do you think in terms of second-level definition and PR scope?

Multi-Rule NetworkPolicy Example ```console apiVersion: networking.k8s.io/v1 kind: NetworkPolicy metadata: name: complex-network-policy namespace: default spec: podSelector: matchLabels: app: myapp policyTypes: - Ingress ingress: - from: - podSelector: matchLabels: app: frontend - podSelector: matchLabels: app: backend ports: - protocol: TCP port: 80 - from: - podSelector: matchLabels: app: database - podSelector: matchLabels: app: cache ports: - protocol: TCP port: 6379 ```
tthvo commented 3 months ago

Therefore, a NetworkPolicy should be considered as second-level unused when .spec.podSelector matches existing pods, but none of the ingress/egress rules matches existing pods/namespaces. In other words, the NetworkPolicy handles no traffic to the matched pods.

This sounds good to me! I think that's what @yonahd mentioned in the issue #279 but I misunderstood/oversimplified. IF agreed, I will go into that direction.

Performance - each NetworkPolicy can be set with multiple ingress/egress rules hence a wide list of labels to iterate.

I guess we can use goroutines but I have been wondering what the conventions/tools for this these cases in kor are. Would you be able to give me some pointers?

yonahd commented 3 months ago

Therefore, a NetworkPolicy should be considered as second-level unused when .spec.podSelector matches existing pods, but none of the ingress/egress rules matches existing pods/namespaces. In other words, the NetworkPolicy handles no traffic to the matched pods.

This sounds good to me! I think that's what @yonahd mentioned in the issue #279 but I misunderstood/oversimplified. IF agreed, I will go into that direction.

Performance - each NetworkPolicy can be set with multiple ingress/egress rules hence a wide list of labels to iterate.

I guess we can use goroutines but I have been wondering what the conventions/tools for this these cases in kor are. Would you be able to give me some pointers?

We are currently running synchronously if that's what you're asking. I'll add to the backlog to checkout goroutines across kor

tthvo commented 3 months ago

Thanks for the reviews! I have addressed them in the latest commits.

@yonahd we should address the following before merging the feature or in future releases:

1. Consider mapping false-positive NetworkPolicies in multiple distros and supporting it in `pkg/kor/exceptions/networkpolicies`.

2. Consider discovering second-level unused NetworkPolicies, for example, a NetworkPolicy with matching `.spec.podSelector`, but unmatching:

* `.spec.ingress.from.podSelector`

* `.spec.ingress.from.namespaceSelector` (unsuppoted yet, see [#249](#249))

* `.spec.egress.to.podSelector`

* `.spec.egress.to.namespaceSelector` (unsuppoted yet, see [#249](#249))

I could help handling these cases. Just let me know if you prefer to address these in this PR or a separate one :D

The cases in the second point should be addressed in this PR.

The exceptions can be a separate one.

Thanks! I am happy to help out! Though, I think we still need to handle these cases? I can make a follow-up PR.

yonahd commented 3 months ago

Yes. Another PR would be great to cover these other cases