Fix networkpolicy related antctl commands #6487

Dyanngg · 2024-06-26T05:05:04Z

This commit contains numerous fixes for antctl, including the get ovsflows and get networkpolicy commands:

Update the antctl get networkpolicy flag type=ANP to point the correct AdminNetworkPolicy type since ANP<->AntreaNetworkPolicy mapping has been deprecated.
Update the antctl get ovsflows command so that when dumping flows for a specific policy, a type can be specified to eliminate ambiguity.
Since v1.13 the antctl get ovsflows command will not work properly for dumping networkpolicy flows. This is due to the matcher for dumping flows include priority, which is not supported by openvswitch. This PR fixes this issue.

Dyanngg · 2024-06-26T06:26:46Z

Sample output:

root@kind-worker2:/# antctl get ovsflows -N cnp-deny-client-to-db --type ACNP

table=AntreaPolicyEgressRule, n_packets=0, n_bytes=0, idle_age=233, priority=14800,conj_id=4 actions=set_field:0x4->reg3,set_field:0x400/0x400->reg0,goto_table:EgressMetric
table=AntreaPolicyEgressRule, n_packets=0, n_bytes=0, idle_age=233, priority=14800,ip,nw_src=10.20.2.2 actions=conjunction(4,1/2)
table=AntreaPolicyEgressRule, n_packets=0, n_bytes=0, idle_age=233, priority=14800,ip,nw_src=10.20.2.8 actions=conjunction(4,1/2)
table=AntreaPolicyEgressRule, n_packets=0, n_bytes=0, idle_age=233, priority=14800,ip,nw_dst=10.20.1.8 actions=conjunction(4,2/2)
table=AntreaPolicyEgressRule, n_packets=0, n_bytes=0, idle_age=233, priority=14800,ip,nw_dst=10.20.1.6 actions=conjunction(4,2/2)
table=AntreaPolicyEgressRule, n_packets=0, n_bytes=0, idle_age=233, priority=14800,ip,nw_dst=10.20.2.5 actions=conjunction(4,2/2)
table=AntreaPolicyEgressRule, n_packets=0, n_bytes=0, idle_age=233, priority=14800,ip,nw_dst=10.20.2.6 actions=conjunction(4,2/2)


root@kind-worker2:/# antctl get ovsflows -N test-1 --type K8sNP -n default

table=IngressRule, n_packets=0, n_bytes=0, idle_age=22, priority=190,conj_id=5,ip actions=set_field:0x5->reg6,ct(commit,table=IngressMetric,zone=65520,exec(set_field:0x5/0xffffffff->ct_label))
table=IngressRule, n_packets=0, n_bytes=0, idle_age=22, priority=200,ip,nw_src=10.20.2.2 actions=conjunction(5,1/3)
table=IngressRule, n_packets=0, n_bytes=0, idle_age=22, priority=200,ip,nw_src=10.20.1.7 actions=conjunction(5,1/3)
table=IngressRule, n_packets=0, n_bytes=0, idle_age=22, priority=200,reg1=0x4 actions=conjunction(5,2/3)
table=IngressRule, n_packets=0, n_bytes=0, idle_age=473, priority=200,tcp,tp_dst=443 actions=conjunction(5,3/3),conjunction(2,3/3)
table=IngressDefaultRule, n_packets=0, n_bytes=0, idle_age=22, priority=200,reg1=0x4 actions=drop

pkg/agent/apiserver/handlers/networkpolicy/handler.go

pkg/querier/querier.go

pkg/ovs/openflow/utils.go

pkg/agent/apiserver/handlers/ovsflows/handler.go

pkg/agent/apiserver/handlers/networkpolicy/handler.go

pkg/agent/apiserver/handlers/ovsflows/handler.go

pkg/ovs/openflow/utils.go

antoninbas · 2024-07-22T22:14:53Z

pkg/antctl/antctl.go

@@ -399,9 +400,14 @@ $ antctl get podmulticaststats pod -n namespace`,
 						},
 						{
 							name:      "networkpolicy",
-							usage:     "NetworkPolicy name. If present, Namespace must be provided.",
+							usage:     "NetworkPolicy name. If present, type must be provided. Namespace must be provided for non-cluster-scoped policy types.",


If present, type must be provided

So with the latest version, there is no way to enforce that?

If accommodating for the missing policy type in the API is too cumbersome, I think it is ok to just require the policy type unconditionally at the API level. This is an Agent-specific API, which means it is hard to consume directly without executing antctl in the Agent Pod. I think it generally makes sense to strive to preserve backwards-compatibility, even for this API, but not if it means adding too much complexity (considering that again this is an Agent-specific API, only used for advanced troubleshooting and as far as we know exclusively consumed via antctl).

In the latest version I've updated the logic so that 1. type is not strictly required, just like before 2. if a type is provided, validation on whether that type requires a namespace is performed 3. if there's ambiguity, a BadRequest will be returned

Dyanngg · 2024-08-09T19:51:54Z

@antoninbas Could you take another look and see if you have additional comments?

docs/antctl.md

pkg/agent/apiserver/handlers/ovsflows/handler.go

pkg/agent/apiserver/handlers/ovsflows/handler_test.go

pkg/ovs/openflow/utils.go

pkg/ovs/openflow/utils_test.go

pkg/agent/apiserver/handlers/networkpolicy/handler.go

pkg/agent/apiserver/handlers/ovsflows/handler_test.go

pkg/ovs/openflow/utils.go

This commit contains numerous fixes for antctl, including the get ovsflows and get networkpolicy commands: 1. Update the `antctl get networkpolicy` flag type=ANP to point the correct AdminNetworkPolicy type since ANP<->AntreaNetworkPolicy mapping has been deprecated. 2. Update the `antctl get ovsflows` command so that when dumping flows for a specific policy, a type is now required to eliminate ambiguity. 3. Since v1.13 the `antctl get ovsflows` command will not work properly for dumping networkpolicy flows. This is due to the matcher for dumping flows include priority, which is not supported by openvswitch. This PR fixes this issue. Signed-off-by: Dyanngg <dingyang@vmware.com>

Signed-off-by: Dyanngg <dingyang@vmware.com>

pkg/agent/apiserver/handlers/ovsflows/handler_test.go

pkg/querier/querier.go

antoninbas · 2024-08-22T21:04:22Z

pkg/agent/apiserver/handlers/ovsflows/handler_test.go

+			policyTypeToReturn := tc.policyType
+			if policyTypeToReturn == "" {
+				// Test the case where policy type is omitted from the query but the result is not ambiguous
+				policyTypeToReturn = cpv1beta.AntreaNetworkPolicy
+			}


may be better to have policyType and policyTypeToReturn as fields in the test case definition, to avoid the special case here, but I don't feel very strongly about it

@Dyanngg just checking that you didn't address it intentionally?

Yes for now, since this is not functionality related. I've updated it locally. Once the CI passes for Jenkins, I'll update the PR again with the changes in UT

So to clarify I should wait before I approve & merge the PR?

Yes I'll mention you in this thread once that's done, thanks

@antoninbas Done. The jenkins tests were passing before I pushed a refactor in UT.

tnqn

LGTM

Dyanngg · 2024-09-16T17:38:42Z

/test-all

Signed-off-by: Dyanngg <dingyang@vmware.com>

antoninbas · 2024-09-16T21:00:31Z

/skip-all

This commit contains numerous fixes for antctl, including the get ovsflows and get networkpolicy commands: 1. Update the `antctl get networkpolicy` flag type=ANP to point the correct AdminNetworkPolicy type since ANP<->AntreaNetworkPolicy mapping has been deprecated. 2. Update the `antctl get ovsflows` command so that when dumping flows for a specific policy, a type is now required to eliminate ambiguity. 3. Since v1.13 the `antctl get ovsflows` command will not work properly for dumping networkpolicy flows. This is due to the matcher for dumping flows include priority, which is not supported by openvswitch. This PR fixes this issue. Signed-off-by: Dyanngg <dingyang@vmware.com>

Dyanngg requested review from antoninbas, tnqn and qiyueyao June 26, 2024 05:05

Dyanngg mentioned this pull request Jun 26, 2024

Add antctl query for policy conjunction ID #6470

Open

Dyanngg force-pushed the antctl-np-fix branch from 82ffe3e to 81da3d3 Compare June 26, 2024 06:22

Dyanngg requested a review from jianjuns June 26, 2024 17:04

antoninbas reviewed Jun 26, 2024

View reviewed changes

pkg/agent/apiserver/handlers/networkpolicy/handler.go Show resolved Hide resolved

pkg/querier/querier.go Show resolved Hide resolved

pkg/ovs/openflow/utils.go Outdated Show resolved Hide resolved

pkg/agent/apiserver/handlers/ovsflows/handler.go Outdated Show resolved Hide resolved

Dyanngg requested a review from antoninbas June 27, 2024 00:04

Dyanngg force-pushed the antctl-np-fix branch from 46035f6 to 8f2eceb Compare July 11, 2024 21:37

antoninbas reviewed Jul 22, 2024

View reviewed changes

Dyanngg force-pushed the antctl-np-fix branch 2 times, most recently from e753804 to 14ff27d Compare July 26, 2024 21:15

Dyanngg requested a review from antoninbas July 29, 2024 17:19

Dyanngg force-pushed the antctl-np-fix branch 3 times, most recently from 3912ce4 to e13237c Compare July 29, 2024 21:08

antoninbas reviewed Aug 9, 2024

View reviewed changes

Dyanngg force-pushed the antctl-np-fix branch 2 times, most recently from 34207d9 to 5693126 Compare August 13, 2024 19:01

Dyanngg requested a review from antoninbas August 13, 2024 19:03

tnqn reviewed Aug 14, 2024

View reviewed changes

Dyanngg added 2 commits August 15, 2024 11:04

Address comments

99eb8f3

Signed-off-by: Dyanngg <dingyang@vmware.com>

Dyanngg force-pushed the antctl-np-fix branch from 5693126 to 962d72f Compare August 15, 2024 18:18

Dyanngg requested a review from tnqn August 15, 2024 18:19

antoninbas reviewed Aug 19, 2024

View reviewed changes

pkg/agent/apiserver/handlers/ovsflows/handler_test.go Outdated Show resolved Hide resolved

pkg/agent/apiserver/handlers/ovsflows/handler_test.go Outdated Show resolved Hide resolved

pkg/agent/apiserver/handlers/ovsflows/handler_test.go Show resolved Hide resolved

antoninbas reviewed Aug 19, 2024

View reviewed changes

pkg/querier/querier.go Outdated Show resolved Hide resolved

Dyanngg force-pushed the antctl-np-fix branch from 962d72f to 381628b Compare August 22, 2024 17:21

Dyanngg force-pushed the antctl-np-fix branch from 381628b to bd03340 Compare August 22, 2024 18:43

Dyanngg requested a review from antoninbas August 22, 2024 18:56

antoninbas previously approved these changes Aug 22, 2024

View reviewed changes

tnqn reviewed Aug 27, 2024

View reviewed changes

Dyanngg requested a review from antoninbas September 16, 2024 17:38

Address more comments

7b798ac

Signed-off-by: Dyanngg <dingyang@vmware.com>

Dyanngg dismissed antoninbas’s stale review via 7b798ac September 16, 2024 19:53

Dyanngg force-pushed the antctl-np-fix branch from bd03340 to 7b798ac Compare September 16, 2024 19:53

antoninbas approved these changes Sep 16, 2024

View reviewed changes

antoninbas merged commit d3faddc into antrea-io:main Sep 16, 2024
52 of 58 checks passed

antoninbas added the action/release-note Indicates a PR that should be included in release notes. label Sep 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix networkpolicy related antctl commands #6487

Fix networkpolicy related antctl commands #6487

Dyanngg commented Jun 26, 2024 •

edited

Loading

Dyanngg commented Jun 26, 2024 •

edited

Loading

antoninbas Jul 22, 2024

antoninbas Jul 23, 2024

Dyanngg Jul 29, 2024

Dyanngg commented Aug 9, 2024

antoninbas Aug 22, 2024

antoninbas Sep 16, 2024

Dyanngg Sep 16, 2024

antoninbas Sep 16, 2024

Dyanngg Sep 16, 2024

Dyanngg Sep 16, 2024

tnqn left a comment

Dyanngg commented Sep 16, 2024

antoninbas commented Sep 16, 2024

Fix networkpolicy related antctl commands #6487

Fix networkpolicy related antctl commands #6487

Conversation

Dyanngg commented Jun 26, 2024 • edited Loading

Dyanngg commented Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dyanngg commented Aug 9, 2024

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

tnqn left a comment

Choose a reason for hiding this comment

Dyanngg commented Sep 16, 2024

antoninbas commented Sep 16, 2024

Dyanngg commented Jun 26, 2024 •

edited

Loading

Dyanngg commented Jun 26, 2024 •

edited

Loading