Add support for ANP and BANP in the explain command

Peac36 commented 5 months ago

Issue: https://github.com/kubernetes-sigs/network-policy-api/issues/153

Screenshot from 2024-01-14 18-09-48

These images are based on the following files:

Also made some modifications to the original requirements:

Add Ingress/Egress rule names in the action column.
Add some formatting to the subject column.

netlify[bot] commented 5 months ago

Deploy Preview for kubernetes-sigs-network-policy-api ready!

Name	Link
Latest commit	e9a29166b64dce26e465d504077da71b5a388e4c
Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-network-policy-api/deploys/65e9ec4316d4240008cace3d
Deploy Preview	https://deploy-preview-188--kubernetes-sigs-network-policy-api.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

k8s-ci-robot commented 5 months ago

Welcome @Peac36!

It looks like this is your first PR to kubernetes-sigs/network-policy-api 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/network-policy-api has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. :smiley:

k8s-ci-robot commented 5 months ago

Hi @Peac36. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

astoycos commented 5 months ago

/ok-to-test /assign @huntergregory

astoycos commented 5 months ago

Thanks for this @Peac36! We'll get to it ASAP

mattfenwick commented 5 months ago

nice one @Peac36 ! thanks for posting screenshots of the output 👍 👍 !!

I'll defer to @astoycos and @huntergregory <- they're the real approvers , please let me know if you'd like me to take a closer look at anything but overall I feel like this is a good patch!

huntergregory commented 5 months ago

Thanks for making this PR @Peac36 ! Have been OOF and then got a concussion, but will aim to review this early next week!

Peac36 commented 5 months ago

Thank you for the input. I'll try to resolve it as soon as possible.

huntergregory commented 5 months ago

EDIT: removed "verdict" idea below since traffic decisions could be made by overlapping peers.

About the Action column, I like the idea of adding the source there.

Taking it a step further, I think we could make it quicker to glance at and hold the user's hand a little more. Since a policy's rules share the same priority, we could consolidate a policy into a single line (so long as the rules share the same ports/protocols) and explicitly point out which actions are ineffective.

Example:

ANP:
    pri=15 (policy1): Allow (ineffective rules: Deny, Skip, Allow, Deny)
    pri=16 (policy3): Deny (ineffective rules: Skip)
    pri=35 (policy2): Skip
BANP: Deny (ineffective rules: Allow, Deny)

Another example:

ANP:
    pri=15 (policy1): Skip
BANP: Deny

Implementation Ideas

Find ANP with same subject, namespaces, and ports, then sort by priority

Testing

We should try to come up with a UT that covers these kinds of examples with multiple policies.

Also, a case where a policy has multiple rules that share the same subject and namespaces, but not the same ports to help convey that output behavior.

Peac36 commented 4 months ago

@huntergregory - I've resolved most of the initial feedback and started working on the new implementation idea but I have a question about Source rules. Should we list all ANPs that affect this subject or we can just drop the column and add the policy name in the action column?

huntergregory commented 4 months ago

Thanks @Peac36. I'm thinking to list all ANPs that affect this subject to keep in-line with the previous design. For context, here are those example policies I referenced before, and the previous output for NPv1:

$ ./cyclonus analyze --use-example-policies --mode explain
INFO[2024-02-11T16:40:59-08:00] log level set to 'info'
explained policies:
+---------+--------------------+---------------------------------------------------------------------+--------------------------+---------------------------+
|  TYPE   |       TARGET       |                            SOURCE RULES                             |           PEER           |       PORT/PROTOCOL       |
+---------+--------------------+---------------------------------------------------------------------+--------------------------+---------------------------+
| Ingress | namespace: default | default/accidental-and                                              | namespace: default       | all ports, all protocols  |
|         | Match labels:      | default/accidental-or                                               | pods: Match labels:      |                           |
|         |   a: b             |                                                                     |   role: client           |                           |
+         +                    +                                                                     +--------------------------+                           +
|         |                    |                                                                     | namespace: Match labels: |                           |
|         |                    |                                                                     |   user: alice            |                           |
|         |                    |                                                                     | pods: Match labels:      |                           |
|         |                    |                                                                     |   role: client           |                           |
+         +                    +                                                                     +--------------------------+                           +
|         |                    |                                                                     | namespace: Match labels: |                           |
|         |                    |                                                                     |   user: alice            |                           |
|         |                    |                                                                     | pods: all                |                           |
+         +--------------------+---------------------------------------------------------------------+--------------------------+---------------------------+
|         | namespace: default | default/allow-nothing-to-v2-all-web                                 | no pods, no ips          | no ports, no protocols    |
|         | Match labels:      |                                                                     |                          |                           |
|         |   all: web         |                                                                     |                          |                           |
+         +--------------------+---------------------------------------------------------------------+--------------------------+---------------------------+
|         | namespace: default | default/allow-specific-port-from-role-monitoring-to-app-apiserver   | namespace: default       | port 5000 on protocol TCP |
|         | Match labels:      |                                                                     | pods: Match labels:      |                           |
|         |   app: apiserver   |                                                                     |   role: monitoring       |                           |
+         +--------------------+---------------------------------------------------------------------+--------------------------+---------------------------+
|         | namespace: default | default/allow-from-app-bookstore-to-app-bookstore-role-api          | namespace: default       | all ports, all protocols  |
|         | Match labels:      |                                                                     | pods: Match labels:      |                           |
|         |   app: bookstore   |                                                                     |   app: bookstore         |                           |
|         |   role: api        |                                                                     |                          |                           |
+         +--------------------+---------------------------------------------------------------------+--------------------------+                           +
|         | namespace: default | default/allow-from-multiple-to-app-bookstore-role-db                | namespace: default       |                           |
|         | Match labels:      |                                                                     | pods: Match labels:      |                           |
|         |   app: bookstore   |                                                                     |   app: bookstore         |                           |
|         |   role: db         |                                                                     |   role: api              |                           |
+         +                    +                                                                     +--------------------------+                           +
|         |                    |                                                                     | namespace: default       |                           |
|         |                    |                                                                     | pods: Match labels:      |                           |
|         |                    |                                                                     |   app: bookstore         |                           |
|         |                    |                                                                     |   role: search           |                           |
+         +                    +                                                                     +--------------------------+                           +
|         |                    |                                                                     | namespace: default       |                           |
|         |                    |                                                                     | pods: Match labels:      |                           |
|         |                    |                                                                     |   app: inventory         |                           |
|         |                    |                                                                     |   role: web              |                           |
+         +--------------------+---------------------------------------------------------------------+--------------------------+---------------------------+
|         | namespace: default | default/allow-nothing                                               | no pods, no ips          | no ports, no protocols    |
|         | Match labels:      |                                                                     |                          |                           |
|         |   app: foo         |                                                                     |                          |                           |
+         +--------------------+---------------------------------------------------------------------+--------------------------+---------------------------+
|         | namespace: default | default/allow-all-to-app-web                                        | all pods, all ips        | all ports, all protocols  |
|         | Match labels:      | default/allow-all-to-version2-app-web                               |                          |                           |
|         |   app: web         | default/allow-all-to-version3-app-web                               |                          |                           |
|         |                    | default/allow-all-to-version4-app-web                               |                          |                           |
|         |                    | default/allow-from-anywhere-to-app-web                              |                          |                           |
|         |                    | default/allow-from-namespace-to-app-web                             |                          |                           |
|         |                    | default/allow-from-namespace-with-labels-type-monitoring-to-app-web |                          |                           |
|         |                    | default/allow-nothing-to-app-web                                    |                          |                           |
+         +--------------------+---------------------------------------------------------------------+--------------------------+                           +
|         | namespace: default | default/allow-all-within-namespace                                  | namespace: default       |                           |
|         | all pods           | default/allow-nothing-to-anything                                   | pods: all                |                           |
+---------+--------------------+---------------------------------------------------------------------+--------------------------+---------------------------+
|         |                    |                                                                     |                          |                           |
+---------+--------------------+---------------------------------------------------------------------+--------------------------+---------------------------+
| Egress  | namespace: default | default/allow-egress-on-port-app-foo                                | all pods, all ips        | port 53 on protocol TCP   |
|         | Match labels:      | default/allow-egress-to-all-namespace-from-app-foo-on-port-53       |                          | port 53 on protocol UDP   |
|         |   app: foo         | default/allow-no-egress-from-labels-app-foo                         |                          |                           |
|         |                    | default/allow-nothing                                               |                          |                           |
+         +--------------------+---------------------------------------------------------------------+--------------------------+---------------------------+
|         | namespace: default | default/allow-no-egress-from-namespace                              | no pods, no ips          | no ports, no protocols    |
|         | all pods           |                                                                     |                          |                           |
+---------+--------------------+---------------------------------------------------------------------+--------------------------+---------------------------+

huntergregory commented 4 months ago

@Peac36 btw would you mind continuing the PR in separate commits? Will make it easier to review/track changes

huntergregory commented 4 months ago

Also @Peac36, we're planning to record a demo in a week or so for the Policy Assistant. Would be awesome to showcase your work! I'd like to give you a shoutout in a slide for our upcoming KubeCon presentation, similar to slide 25 here from the Fall.

Feel free to join the Kubernetes Slack workspace -> sig-network-policy-api channel and DM me there!

Peac36 commented 4 months ago

@Peac36 btw would you mind continuing the PR in separate commits? Will make it easier to review/track changes

Sure.

Also @Peac36, we're planning to record a demo in a week or so for the Policy Assistant. Would be awesome to showcase your work! I'd like to give you a shoutout in a slide for our upcoming KubeCon presentation, similar to slide 25 here from the Fall.

Awesome. I'm almost ready with the grouping of the ANPs and after that, I'll take care of the BANPs and the tests. I will be ready with the whole thing probably on Monday. I hope this is OK for you guys.

Peac36 commented 4 months ago

Hey @huntergregory I'm ready with the feedback and implementation of the idea you gave.

Here is a short preview.

Screenshot from 2024-02-26 20-29-13

Added tests for BuildV1AndV2NetPols to assert that

Anps with the same target would be grouped together
Anps that have the same target,protocols, and peers would be together
Banps would be added to a target group if it has the same peers and protocols.

Let me know if you have more ideas for tests.

Peac36 commented 4 months ago

@huntergregory - Can you review it again? I refactor it based on your feedback. So instead of grouping in the parsing stage, I introduced a new struct that combines all anps and bansp before printing stage. The code is much simpler now

Also, we don't have a PR check for it yet. Could you run go test . in both test/integration/ and pkg/matcher/ directories to make sure previous tests are all green?

All green.

Pretty simple, just something like this as a regular UT (no need for gomega?) in a new file test/integration/explain_test.go:

Done. Added a test for printing anps and banps as well.

Peac36 commented 4 months ago

Thanks for the feedback @huntergregory. Already fixed some of them, however, I'll need some more - probably later tomorrow will be able to resolve them all. About the discrepancies - the issue came from here:

https://github.com/kubernetes-sigs/network-policy-api/pull/188/files#diff-0c40ee241d83fc2c8e730d1cfea6a20eccbfd5a0c99a346073bee43e6646c44dR194

I missed including the namespace selector when building the map key so the grouping was incorrect.

Here is the result after the fix. Please note that I made some changed to the Banp so the result should not be the same as the one from previous examples. Screenshot from 2024-03-06 18-42-13

Peac36 commented 4 months ago

@huntergregory - I've resolved your last feedback.

huntergregory commented 3 months ago

Andrew added me as owner actually, let’s see if this works 😆

/lgtm

/approve

k8s-ci-robot commented 3 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: huntergregory, Peac36

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[cmd/policy-assistant/OWNERS](https://github.com/kubernetes-sigs/network-policy-api/blob/main/cmd/policy-assistant/OWNERS)~~ [huntergregory] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

mattfenwick commented 3 months ago

Awesome thank you @Peac36 !!

/lgtm

@mattfenwick could we get your seal of approval? Also going to raise a PR to add myself to cmd/policy-assistant/OWNERS.

I know it's too late but I'll do it anyway!

/lgtm /approve

great work @Peac36 , I love where y'all are taking this!

huntergregory commented 3 months ago

Congrats @Peac36 on the first policy assistant PR!

Just messaged you on Slack about something for tomorrow (Monday) if you have time

huntergregory commented 3 months ago

Fixed #153

kubernetes-sigs / network-policy-api