feat(Policy Assistant): data structures simulating connectivity for (B)ANP #159

huntergregory · 2023-10-18T04:42:34Z

Fixes #152. Related to #150.

This PR makes Cyclonus support ANP/BANP/NetPol v1 and their interactions. Simulated prober is showing correct connectivity matrix results for these.

This PR also introduces an integration/debugging test suite.

Implementation

Wraps/modifies existing types to support ANP/BANP. Bulk of changes is in the matcher pkg, starting at BuildV1AndV2NetPols().
Includes validation of ANP/BANP (e.g., exactly one of Pods or Namespaces field is set).

Testing

Integration tests (test/integration/integration_test.go)
No regressions to v1 NetPols. Can still:
- make cyclonus
- Table summary: cyclonus analyze --mode=explain
- Conformance tests: cyclonus generate ...

Example Connectivity Matrix

=== RUN   TestANPWithBANP/BANP_deny_all_after_ANP
    integration_test.go:1363: validated ingress:
        +--------+---------+---------+---------+---------+
        | TCP/80 |   X/A   |   X/B   |   Y/A   |   Y/B   |
        | TCP/81 |         |         |         |         |
        | UDP/80 |         |         |         |         |
        | UDP/81 |         |         |         |         |
        +--------+---------+---------+---------+---------+
        | x/a    | X X . X | X X X X | X X X X | X X X X |
        +--------+---------+---------+---------+---------+
        | x/b    | X X . X | X X X X | X X X X | X X X X |
        +--------+---------+---------+---------+---------+
        | y/a    | X X X X | X X X X | X X X X | X X X X |
        +--------+---------+---------+---------+---------+
        | y/b    | X X X X | X X X X | X X X X | X X X X |
        +--------+---------+---------+---------+---------+

Example 2

=== RUN   TestANPConnectivity/ingress_same_labels_port_range
    integration_test.go:1363: validated ingress:
        +--------+---------+---------+---------+---------+---------+---------+---------+---------+---------+
        | TCP/80 |   X/A   |   X/B   |   X/C   |   Y/A   |   Y/B   |   Y/C   |   Z/A   |   Z/B   |   Z/C   |
        | TCP/81 |         |         |         |         |         |         |         |         |         |
        | UDP/80 |         |         |         |         |         |         |         |         |         |
        | UDP/81 |         |         |         |         |         |         |         |         |         |
        +--------+---------+---------+---------+---------+---------+---------+---------+---------+---------+
        | x/a    | X X . . | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . |
        +--------+---------+---------+---------+---------+---------+---------+---------+---------+---------+
        | x/b    | X X . . | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . |
        +--------+---------+---------+---------+---------+---------+---------+---------+---------+---------+
        | x/c    | X X . . | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . |
        +--------+---------+---------+---------+---------+---------+---------+---------+---------+---------+
        | y/a    | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . |
        +--------+---------+---------+---------+---------+---------+---------+---------+---------+---------+
        | y/b    | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . |
        +--------+---------+---------+---------+---------+---------+---------+---------+---------+---------+
        | y/c    | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . |
        +--------+---------+---------+---------+---------+---------+---------+---------+---------+---------+
        | z/a    | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . |
        +--------+---------+---------+---------+---------+---------+---------+---------+---------+---------+
        | z/b    | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . |
        +--------+---------+---------+---------+---------+---------+---------+---------+---------+---------+
        | z/c    | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . | . . . . |
        +--------+---------+---------+---------+---------+---------+---------+---------+---------+---------+

netlify · 2023-10-18T04:42:40Z

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

Name	Link
🔨 Latest commit	`0c79736`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-network-policy-api/deploys/6543f0993a1eba0008e11152
😎 Deploy Preview	https://deploy-preview-159--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 · 2023-10-18T04:42:43Z

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

huntergregory · 2023-10-18T04:46:03Z

cmd/cyclonus/pkg/cli/analyze.go

@@ -23,7 +22,6 @@ import (
 const (
 	ParseMode        = "parse"
 	ExplainMode      = "explain"
-	LintMode         = "lint"


Removing Lint mode since it had compile issues with new structures. This mode flagged Pods (Targets) with NetworkPolicies that fail certain checks e.g. all ingress denied or NetPol doesn’t define the protocol (defaulting to TCP))

huntergregory · 2023-10-18T04:47:56Z

cmd/cyclonus/pkg/connectivity/probe/table.go

@@ -26,6 +29,73 @@ type Table struct {
 	Wrapped *TruthTable
 }

+func NewTableWithDefaultConnectivity(r *Resources, ingress, egress Connectivity) *Table {


Additions to the probe pkg are just utils for integration testing and debugging

…x for ANP/BANP

astoycos · 2023-10-20T17:27:00Z

/ok-to-test

mattfenwick · 2023-10-23T13:41:16Z

Hi @huntergregory thanks for picking this up , I'm excited about where you're taking this!

I'll take some time over the next couple of days to dig into this PR; if there's anything else I can do to help please let me know!

astoycos

Took a first pass here and it LGTM I know this is just the beginning of a larger development task.

Thanks for the work @huntergregory!! Once these small comments are addressed I'll approve just barring any changes @mattfenwick Want's to see 👍

One thing to think about though is let's start thinking about

Documentation
Automatic artifact deploying
Automated CI

most likely best done in follow ups.

astoycos · 2023-10-27T03:16:40Z

cmd/cyclonus/pkg/connectivity/probe/resources.go

@@ -16,6 +16,8 @@ type Resources struct {
 	Namespaces map[string]map[string]string
 	Pods       []*Pod
 	//ExternalIPs []string


nit: Do we need all these erroneous externelIP comments? If so let's make an issue for any work that needs to be added for external IP support (Although there may be none and we may be able to just delete these comments)

Created #169. Seems like it could have value

astoycos · 2023-10-27T03:19:54Z

cmd/cyclonus/pkg/connectivity/stepresult.go

@@ -3,6 +3,8 @@ package connectivity
 import (
 	"github.com/mattfenwick/cyclonus/pkg/connectivity/probe"
 	"github.com/mattfenwick/cyclonus/pkg/matcher"


Should these be vendored from our bits internally?

I'll circle back on this. Currently getting the following error when running go mod tidy:

go: github.com/mattfenwick/cyclonus/cmd/policy-assistant imports sigs.k8s.io/network-policy-api/cmd/policy-assistant/pkg/cli: module sigs.k8s.io/network-policy-api@latest found (v0.1.2), but does not contain package sigs.k8s.io/network-policy-api/cmd/policy-assistant/pkg/cli

OK please make an issue 👍

Tracking here: #170

cmd/cyclonus/pkg/connectivity/stepresult.go

cmd/cyclonus/pkg/matcher/builder.go

cmd/cyclonus/pkg/matcher/explain.go

astoycos · 2023-10-27T21:51:11Z

cmd/cyclonus/README.md

@@ -1,51 +1,16 @@
-# Cyclonus
+# NetworkPolicy Assistant (derived from Cyclonus)


Should we just make this Policy Assistant since it's gonna support NP/ANP/BANP etc?

Can you change the cmd/cyclonus dir to cmd/policy-assistant

Also can you change all the make targets / binary name to policy-assistent as well?

Policy Assistant is much better! Changed the dir to cmd/policy-assistant.

Still need to adjust Makefile and rename cmd/policy-assistant/cmd/cyclonus/. Seems there are 37 non-go files that reference "cyclonus", including some artifacts and CI.

Since the PR is already so large, I'd recommend making those changes in another PR to limit the scope of this one

Yep follow up!

Tracking here: #171

huntergregory · 2023-11-02T18:58:54Z

Hi @huntergregory thanks for picking this up , I'm excited about where you're taking this!

I'll take some time over the next couple of days to dig into this PR; if there's anything else I can do to help please let me know!

Hey @mattfenwick, gentle ping here. Any thoughts/ideas on this code or this project's trajectory?

mattfenwick · 2023-11-06T20:33:54Z

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

+// This matcher is modeled after the confusing behavior of v1 NetPol's Port field.
+// We map NetworkPolicy v2 to this matcher despite v2's better approach to distinguishing named ports from a port number.
+// This matcher requires that you specify the Protocol for a NamedPort.
+// In NetPol v1, there is the pathological case where the user can define a NetworkPolicy rule allowing a NamedPort on the wrong Protocol.


Interesting, I didn't realize this. Glad you pointed this out! 😄

mattfenwick · 2023-11-06T20:44:48Z

Big hat tip to @huntergregory , I'm on board with these changes ! This looks to me like an effective way to move forward with supporting ANP/BANP.

I'm curious to hear your opinion on whether any parts feel excessively clunky, or if this all feels like something that will fit together well. I personally don't see any risks but just want to hear your opinion on this.

One thing I always struggled with in Cyclonus was naming. Since ANP/BANP has new concepts, this of course means netpol-assistant (sorry: I'm not sure what the new name is?) needs more naming: I like the new Effect, Verdict, SubjectV1, and SubjectAdmin names => and just want to say that I've always found naming to be high-leverage so 👍

I also like the distinction between v1 and v2 netpols, that was a tricky one and I appreciate the way you've handled that.

tl,dr;

Anything in particular you'd like me to look at more closely?

mattfenwick · 2023-11-06T21:37:24Z

/approve

astoycos · 2023-11-06T21:55:40Z

/lgtm
/approve

@huntergregory Asked for like one other issue but otherwise let's keep moving forward!

Thanks for the review @mattfenwick!

k8s-ci-robot · 2023-11-06T21:55:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: astoycos, huntergregory, mattfenwick

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:

~~OWNERS~~ [astoycos]

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

huntergregory · 2023-11-06T23:09:54Z

Thanks @mattfenwick for that review and your perspective!

Anything in particular you'd like me to look at more closely?

Nothing in mind. Thanks!

I'm curious to hear your opinion on whether any parts feel excessively clunky, or if this all feels like something that will fit together well. I personally don't see any risks but just want to hear your opinion on this.

No concerns on my side. I think this design will allow us to simulate connectivity and run conformance tests agnostic to the Policy used (NetPol, ANP, or BANP). Visualizations (e.g. explaining a policy) should be fairly simple too.

Right now, there are a couple type checks for whether a matcher is v1 or v2. Hopefully we can keep that number low and introduce interface methods if needed.

…ernetes-sigs#240 (building policies) Signed-off-by: Hunter Gregory <42728408+huntergregory@users.noreply.github.com>

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 18, 2023

k8s-ci-robot requested review from astoycos and mattfenwick October 18, 2023 04:42

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 Oct 18, 2023

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 18, 2023

huntergregory changed the title ~~feat(Netpol Assistant): data structures simulating connectivity matrix for ANP/BANP~~ feat(Netpol Assistant): data structures simulating connectivity for (B)ANP Oct 18, 2023

huntergregory commented Oct 18, 2023

View reviewed changes

feat(Netpol Assistant): data structures simulating connectivity matri…

4649ace

…x for ANP/BANP

huntergregory force-pushed the data-structures-final branch from f7208d9 to dfb7871 Compare October 18, 2023 23:05

style: PeerMatcherAdmin using only PodPeerMatcher + minor UI changes

1978a22

huntergregory force-pushed the data-structures-final branch from dfb7871 to 1978a22 Compare October 18, 2023 23:08

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 Oct 20, 2023

astoycos self-assigned this Oct 20, 2023

astoycos reviewed Oct 27, 2023

View reviewed changes

huntergregory changed the title ~~feat(Netpol Assistant): data structures simulating connectivity for (B)ANP~~ feat(Policy Assistant): data structures simulating connectivity for (B)ANP Oct 31, 2023

huntergregory mentioned this pull request Oct 31, 2023

huntergregory as contributor to network-policy-api #167

Closed

huntergregory added 4 commits November 2, 2023 10:20

style: fix names in errors and remove/clarify FIXMEs

e325e01

style: rename cmd/cyclonus to cmd/policy-assistant

f3ad772

style: make fmt

0d5d247

docs: readme says Policy instead of NetPol Assistant

0c79736

huntergregory mentioned this pull request Nov 2, 2023

REQUEST: New membership for huntergregory kubernetes/org#4573

Closed

9 tasks

mattfenwick reviewed Nov 6, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 6, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 6, 2023

k8s-ci-robot merged commit 6774f36 into kubernetes-sigs:main Nov 6, 2023
3 checks passed

This was referenced Nov 6, 2023

[Policy Assistant] use internal vendoring instead of vendoring from Cyclonus #170

Closed

[Policy Assistant] update binary names to policy-assistant instead of cyclonus #171

Closed

huntergregory mentioned this pull request Oct 21, 2024

[Policy Assistant] update binary names to policy-assistant instead of cyclonus #254

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Policy Assistant): data structures simulating connectivity for (B)ANP #159

feat(Policy Assistant): data structures simulating connectivity for (B)ANP #159

huntergregory commented Oct 18, 2023 •

edited

Loading

netlify bot commented Oct 18, 2023 •

edited

Loading

k8s-ci-robot commented Oct 18, 2023

huntergregory Oct 18, 2023

huntergregory Oct 18, 2023

astoycos commented Oct 20, 2023

mattfenwick commented Oct 23, 2023

astoycos left a comment

astoycos Oct 27, 2023

huntergregory Nov 2, 2023

astoycos Oct 27, 2023

huntergregory Nov 2, 2023

astoycos Nov 6, 2023

huntergregory Nov 6, 2023

astoycos Oct 27, 2023

astoycos Oct 27, 2023

huntergregory Nov 2, 2023

astoycos Nov 6, 2023

huntergregory Nov 6, 2023

huntergregory commented Nov 2, 2023

mattfenwick Nov 6, 2023

mattfenwick commented Nov 6, 2023

mattfenwick commented Nov 6, 2023

astoycos commented Nov 6, 2023 •

edited

Loading

k8s-ci-robot commented Nov 6, 2023

huntergregory commented Nov 6, 2023

		@@ -1,51 +1,16 @@
		# Cyclonus
		# NetworkPolicy Assistant (derived from Cyclonus)

feat(Policy Assistant): data structures simulating connectivity for (B)ANP #159

feat(Policy Assistant): data structures simulating connectivity for (B)ANP #159

Conversation

huntergregory commented Oct 18, 2023 • edited Loading

Implementation

Testing

Example Connectivity Matrix

Example 2

netlify bot commented Oct 18, 2023 • edited Loading

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

k8s-ci-robot commented Oct 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astoycos commented Oct 20, 2023

mattfenwick commented Oct 23, 2023

astoycos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huntergregory commented Nov 2, 2023

Choose a reason for hiding this comment

mattfenwick commented Nov 6, 2023

mattfenwick commented Nov 6, 2023

astoycos commented Nov 6, 2023 • edited Loading

k8s-ci-robot commented Nov 6, 2023

huntergregory commented Nov 6, 2023

huntergregory commented Oct 18, 2023 •

edited

Loading

netlify bot commented Oct 18, 2023 •

edited

Loading

astoycos commented Nov 6, 2023 •

edited

Loading