Add support for ANP and BANP in the explain command #188

Peac36 · 2024-01-14T16:21:31Z

Issue: #153

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 · 2024-01-14T16:21:37Z

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

Name	Link
🔨 Latest commit	`e9a2916`
🔍 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 · 2024-01-14T16:21:40Z

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. 😃

k8s-ci-robot · 2024-01-14T16:21:41Z

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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

astoycos · 2024-01-16T16:10:20Z

/ok-to-test
/assign @huntergregory

astoycos · 2024-01-16T16:10:44Z

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

cmd/policy-assistant/pkg/cli/analyze.go

mattfenwick · 2024-01-17T13:23:03Z

cmd/policy-assistant/pkg/matcher/explain.go

@@ -50,7 +50,7 @@ func (s *SliceBuilder) TargetsTableLines(targets []*Target, isIngress bool) {
 		ruleType = "Egress"
 	}
 	for _, target := range targets {
-		sourceRules := slice.Sort(target.SourceRules)


I recommend sorting all output so that it's:

deterministic

predictable for the reader

mattfenwick · 2024-01-17T13:23:38Z

cmd/policy-assistant/pkg/matcher/explain.go

 				case *IPPeerMatcher:
 					s.IPPeerMatcherTableLines(a)
 				case *PodPeerMatcher:
 					s.PodPeerMatcherTableLines(a, NewV1Effect(true))
 				default:
-					panic(errors.Errorf("invalid PeerMatcher type %T", a))
+					continue


this is an unexpected case, right? might be worth a warning at least

mattfenwick · 2024-01-17T13:28:15Z

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 · 2024-01-26T21:20:12Z

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

huntergregory

Thanks again @Peac36 for tackling this important visualization piece!

You found all the code pieces nicely. The only remaining functionality is handling SameLabels and NotSameLabels.

Also, thanks for looking to improve the formatting. We will need to make sure everything is clear for every case. Along that line, could you add UTs to make sure we have expected outputs for all cases? For NPv1, I'd suggest using the example policies from cyclonus analyze --use-example-policies (believe that's the correct arg but you can also check cyclonus analyze --help).

cmd/policy-assistant/pkg/matcher/peermatcherv2.go

cmd/policy-assistant/pkg/matcher/target.go

cmd/policy-assistant/pkg/matcher/explain.go

huntergregory · 2024-01-30T02:00:52Z

cmd/policy-assistant/pkg/kube/labelselector.go

@@ -107,14 +107,12 @@ func LabelSelectorTableLines(selector metav1.LabelSelector) string {
 	}
 	var lines []string
 	if len(selector.MatchLabels) > 0 {
-		lines = append(lines, "Match labels:")


Can we keep this and Match expressions: ? Similarly to my previous comment, leaving this will make the output clearer imo

Or feel free to change the wording, and we can assess outputs for both the MatchLabels case and MatchExpressions

Let's keep it that way. We can change it later when we are sure for the output layout.

cmd/policy-assistant/pkg/matcher/explain.go

Peac36 · 2024-01-30T17:15:29Z

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

huntergregory · 2024-01-30T19:28:58Z

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 · 2024-02-11T15:47:31Z

@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 · 2024-02-13T05:22:30Z

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 · 2024-02-20T18:04:22Z

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

huntergregory · 2024-02-20T19:56:46Z

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 · 2024-02-21T16:30:36Z

@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 · 2024-02-26T18:36:21Z

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

Here is a short preview.

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.

huntergregory

Thanks @Peac36 the screenshot looks really good. Can we flip the ordering for priorities in Action column? Highest priority (lowest pri number) coming first.

Had a few thoughts on simplifying implementation. My main goal is to keep parallels with NPv1 where possible, and to minimize change to NPv1 and functionality outside the summary table.

Also, thanks for adding tests. So I was thinking, to prevent regressions, it'd be nice to just have maybe one integration test comparing the output of cyclonus analyze --mode explain to an expected table (including NPv1).

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

tableOutput := policy.ExplainTable()
expectedTable := """---------------------------
| Name | Subject | Action |
| abc     | ns=x     | allow   |
---------------------------
require.Equal(t, expectedTable, tableOutput)
"""

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?

cmd/policy-assistant/examples/example.go

cmd/policy-assistant/pkg/matcher/builder.go

huntergregory · 2024-02-26T20:24:52Z

cmd/policy-assistant/pkg/cli/analyze.go

@@ -87,6 +89,8 @@ func SetupAnalyzeCommand() *cobra.Command {
 func RunAnalyzeCommand(args *AnalyzeArgs) {
 	// 1. read policies from kube
 	var kubePolicies []*networkingv1.NetworkPolicy
+	var kubeANPs []*v1alpha1.AdminNetworkPolicy


for a follow-up PR: set these via CLI (#201)

cmd/policy-assistant/pkg/matcher/explain.go

huntergregory · 2024-02-27T07:40:20Z

cmd/policy-assistant/pkg/matcher/explain.go

-				default:
-					panic(errors.Errorf("invalid PeerMatcher type %T", a))
+					s.PodPeerMatcherTableLines(t, NewV1Effect(true), "")
+				case *PeerMatcherAdmin:


simplification idea: before this function, combine all ANPs + BANPs sharing the same subject into one target using BuildV1AndV2NetPols(simplify=true). Then at this case in the switch statement, we know this is an admin target. Let's make a separate function call here to render the target's peers, and break out of the current for loop

Within this function call, we can group by PeerMatchers by Peer and Port

cmd/policy-assistant/pkg/matcher/builder_tests.go

Peac36 · 2024-03-05T13:00:29Z

@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.

huntergregory

Thanks for simplifying @Peac36, this will help us in the long run. And thanks for working on this quickly. Let's address a couple questions/nits and then we can get this merged. Would you be able to tackle these ASAP?

Seeing a few discrepancies between your last screenshot and the text in your integration test. Do you know why? Which is correct?

Before:

After:

cmd/policy-assistant/test/integration/explain_test.go

cmd/policy-assistant/pkg/matcher/explain.go

Peac36 · 2024-03-06T16:45:39Z

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.

Peac36 · 2024-03-07T17:00:43Z

@huntergregory - I've resolved your last feedback.

huntergregory

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.

huntergregory · 2024-03-08T06:59:46Z

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

/lgtm

/approve

k8s-ci-robot · 2024-03-08T06:59:53Z

[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~~ [huntergregory]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mattfenwick · 2024-03-12T12:40:10Z

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 · 2024-03-17T21:01:01Z

Congrats @Peac36 on the first policy assistant PR!

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

huntergregory · 2024-03-21T06:14:06Z

Fixed #153

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 14, 2024

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 14, 2024

k8s-ci-robot requested review from astoycos and mattfenwick January 14, 2024 16:21

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 14, 2024

k8s-ci-robot assigned huntergregory Jan 16, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 16, 2024

mattfenwick reviewed Jan 17, 2024

View reviewed changes

cmd/policy-assistant/pkg/cli/analyze.go Outdated Show resolved Hide resolved

mattfenwick reviewed Jan 17, 2024

View reviewed changes

huntergregory suggested changes Jan 30, 2024

View reviewed changes

add support for anp and banp in the explain command

0a9d295

Peac36 force-pushed the update/153 branch from 52f1f41 to 0a9d295 Compare February 5, 2024 06:15

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 5, 2024

Peac36 force-pushed the update/153 branch 3 times, most recently from 5c73100 to c5db850 Compare February 26, 2024 18:22

change the way the anps and banps are shown in the explain commnad

f9b3cab

Peac36 force-pushed the update/153 branch from c5db850 to f9b3cab Compare February 26, 2024 18:33

huntergregory suggested changes Feb 27, 2024

View reviewed changes

Peac36 force-pushed the update/153 branch 5 times, most recently from 8fdff90 to 82478cb Compare March 4, 2024 15:28

refactor explain command that handle anp and banp

ec0c4a4

Peac36 force-pushed the update/153 branch from 82478cb to ec0c4a4 Compare March 4, 2024 18:22

huntergregory reviewed Mar 5, 2024

View reviewed changes

reviewer feedback

e9a2916

Peac36 force-pushed the update/153 branch from c45de28 to e9a2916 Compare March 7, 2024 16:33

huntergregory approved these changes Mar 8, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 8, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 8, 2024

k8s-ci-robot merged commit 0368c76 into kubernetes-sigs:main Mar 8, 2024
8 checks passed

huntergregory mentioned this pull request Apr 18, 2024

[Policy Assistant] Summary Table for (B)ANP and NetPol #153

Closed

Add support for ANP and BANP in the explain command #188

Add support for ANP and BANP in the explain command #188

Conversation

Peac36 commented Jan 14, 2024 • edited Loading

netlify bot commented Jan 14, 2024 • edited Loading

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

k8s-ci-robot commented Jan 14, 2024

k8s-ci-robot commented Jan 14, 2024

astoycos commented Jan 16, 2024

astoycos commented Jan 16, 2024

mattfenwick Jan 17, 2024

Choose a reason for hiding this comment

mattfenwick Jan 17, 2024

Choose a reason for hiding this comment

mattfenwick commented Jan 17, 2024

huntergregory commented Jan 26, 2024

huntergregory left a comment

Choose a reason for hiding this comment

huntergregory Jan 30, 2024

Choose a reason for hiding this comment

huntergregory Jan 30, 2024

Choose a reason for hiding this comment

Peac36 Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Peac36 commented Jan 30, 2024

huntergregory commented Jan 30, 2024 • edited Loading

Implementation Ideas

Testing

Peac36 commented Feb 11, 2024

huntergregory commented Feb 13, 2024

huntergregory commented Feb 20, 2024

huntergregory commented Feb 20, 2024

Peac36 commented Feb 21, 2024 • edited Loading

Peac36 commented Feb 26, 2024

huntergregory left a comment • edited Loading

Choose a reason for hiding this comment

huntergregory Feb 26, 2024

Choose a reason for hiding this comment

huntergregory Feb 27, 2024

Choose a reason for hiding this comment

huntergregory Feb 27, 2024

Choose a reason for hiding this comment

Peac36 commented Mar 5, 2024

huntergregory left a comment

Choose a reason for hiding this comment

Peac36 commented Mar 6, 2024

Peac36 commented Mar 7, 2024

huntergregory left a comment • edited Loading

Choose a reason for hiding this comment

huntergregory commented Mar 8, 2024

k8s-ci-robot commented Mar 8, 2024

mattfenwick commented Mar 12, 2024

huntergregory commented Mar 17, 2024

huntergregory commented Mar 21, 2024

Peac36 commented Jan 14, 2024 •

edited

Loading

netlify bot commented Jan 14, 2024 •

edited

Loading

Peac36 Feb 26, 2024 •

edited

Loading

huntergregory commented Jan 30, 2024 •

edited

Loading

Peac36 commented Feb 21, 2024 •

edited

Loading

huntergregory left a comment •

edited

Loading

huntergregory left a comment •

edited

Loading