Add NetworkPolicy rule name in Traceflow observation #5667

Atish-iaf · 2023-11-03T12:57:11Z

Add NetworkPolicy rule name in Traceflow observation.

Update Traceflow e2e tests to verify "NetworkPolicy" and "NetworkPolicyRule"
in Traceflow observation.

Signed-off-by: Kumar Atish atish.iaf@gmail.com

gran-vmv

LGTM. Could you update e2e cases to verify this section?

rajnkamr · 2023-11-06T05:58:40Z

pkg/apis/crd/v1beta1/types.go

@@ -1135,6 +1135,8 @@ type Observation struct {
 	DstMAC string `json:"dstMAC,omitempty" yaml:"dstMAC,omitempty"`
 	// NetworkPolicy is the combination of Namespace and NetworkPolicyName.
 	NetworkPolicy string `json:"networkPolicy,omitempty" yaml:"networkPolicy,omitempty"`
+	// NetworkPolicyRule is the name of an ingress or an egress rule in NetworkPolicy.
+	NetworkPolicyRule string `json:"networkPolicyRule,omitempty" yaml:"networkPolicyRule,omitempty"`


Since we are introducing new attribute NetworkPolicyRule in "v1beta1" but not in "v1alpha1," it's essential to provide clear documentation about this difference to avoid confusion. Users of "v1alpha1" should be aware that the attribute is not available in that version, and they may need to upgrade their CRDs to "v1beta1" if they want to use the new feature.

We'll make it clear in the release notes. Is there any other documentation that you suggest updating?

Upstream side, updating release note is required.

Atish-iaf · 2023-11-06T11:21:36Z

LGTM. Could you update e2e cases to verify this section?

updated, PTAL.
Thanks.

pkg/agent/controller/traceflow/packetin.go

pkg/agent/controller/traceflow/packetin_test.go

luolanzone

LGTM overall, one nit.

pkg/agent/controller/traceflow/packetin_test.go

antoninbas

Some small comments, otherwise LGTM

It would have been better to separate 1) the e2e tests changes to replace usage of v1alpha1 with v1beta1, and 2) the addition of the new field to Traceflow (i.e., use 2 different PRs). I don't think there is a strong need to split that PR now, but something to keep in mind in the future.

antoninbas · 2023-11-13T20:12:52Z

pkg/agent/controller/traceflow/packetin_test.go

+		Class: openflow15.OXM_CLASS_PACKET_REGS,
+		Field: uint8(openflow.TFEgressConjIDField.GetRegID() / 2),
+		Value: &openflow15.ByteArrayField{
+			Data: xreg0,


It's confusing to use xreg0 as the data source here. The name xreg0 comes from the fact that we use it (above) to set the value of the openflow15.NXM_NX_REG0 register.
I suggest that you use a new byte array for the conj ID registers. Maybe conjData or some variable like that.

antoninbas · 2023-11-13T20:17:13Z

pkg/agent/controller/traceflow/packetin_test.go

+						Name: "acnp-1",
+					},
+				)
+				npQuerier.EXPECT().GetRuleByFlowID(gomock.Any()).Return(


nit: I think that it is pretty easy to know the expected ID here, given that we craft the register data ourselves?
applies to expectations below as well

antoninbas · 2023-11-13T20:19:58Z

pkg/apis/crd/v1beta1/types.go

@@ -1135,6 +1135,8 @@ type Observation struct {
 	DstMAC string `json:"dstMAC,omitempty" yaml:"dstMAC,omitempty"`
 	// NetworkPolicy is the combination of Namespace and NetworkPolicyName.
 	NetworkPolicy string `json:"networkPolicy,omitempty" yaml:"networkPolicy,omitempty"`
+	// NetworkPolicyRule is the name of an ingress or an egress rule in NetworkPolicy.
+	NetworkPolicyRule string `json:"networkPolicyRule,omitempty" yaml:"networkPolicyRule,omitempty"`


We'll make it clear in the release notes. Is there any other documentation that you suggest updating?

test/e2e/traceflow_test.go

antoninbas · 2023-11-13T20:28:23Z

test/e2e/traceflow_test.go

+							Component:         v1beta1.ComponentNetworkPolicy,
+							ComponentInfo:     "IngressMetric",
+							Action:            v1beta1.ActionDropped,
+							NetworkPolicy:     "AntreaNetworkPolicy:testtraceflow-.{8}/test-annp-deny-ingress",


Do we really need a regex here? We know what the name of the Namespace is by the time we declare the test cases.

antoninbas · 2023-11-13T20:31:07Z

test/e2e/traceflow_test.go

+							ComponentInfo:     "IngressMetric",
+							Action:            v1beta1.ActionDropped,
+							NetworkPolicy:     "AntreaNetworkPolicy:testtraceflow-.{8}/test-annp-deny-ingress",
+							NetworkPolicyRule: "ingress-drop-.{7}",


A similar comment here: we can use hardcoded rule names when we create the policies, and use that name here. We don't need to let Antrea auto-generate a rule name when testing Traceflow.

Update Traceflow e2e tests to verify "NetworkPolicy" and "NetworkPolicyRule" in Traceflow observation. Signed-off-by: Kumar Atish <atish.iaf@gmail.com>

tnqn

LGTM

tnqn · 2023-11-16T08:49:57Z

/test-all

antoninbas · 2023-11-16T16:59:48Z

/test-conformance

Atish-iaf force-pushed the add-rule-name-in-traceflow branch from 4621040 to b403390 Compare November 3, 2023 13:41

Atish-iaf marked this pull request as ready for review November 4, 2023 06:00

Atish-iaf requested a review from gran-vmv November 4, 2023 06:01

gran-vmv added this to the Antrea v1.15 release milestone Nov 6, 2023

gran-vmv added area/ops/traceflow Issues or PRs related to the Traceflow feature kind/feature Categorizes issue or PR as related to a new feature. labels Nov 6, 2023

gran-vmv assigned Atish-iaf Nov 6, 2023

gran-vmv reviewed Nov 6, 2023

View reviewed changes

rajnkamr reviewed Nov 6, 2023

View reviewed changes

rajnkamr added the action/release-note Indicates a PR that should be included in release notes. label Nov 6, 2023

Atish-iaf force-pushed the add-rule-name-in-traceflow branch from b403390 to b22e485 Compare November 6, 2023 10:07

Atish-iaf requested a review from gran-vmv November 6, 2023 11:21

luolanzone reviewed Nov 7, 2023

View reviewed changes

pkg/agent/controller/traceflow/packetin.go Show resolved Hide resolved

Atish-iaf force-pushed the add-rule-name-in-traceflow branch from b22e485 to 3cb61fd Compare November 8, 2023 11:14

Atish-iaf requested a review from luolanzone November 8, 2023 11:37

luolanzone reviewed Nov 9, 2023

View reviewed changes

pkg/agent/controller/traceflow/packetin_test.go Outdated Show resolved Hide resolved

Atish-iaf force-pushed the add-rule-name-in-traceflow branch from 3cb61fd to f39adb9 Compare November 9, 2023 07:25

Atish-iaf requested a review from luolanzone November 9, 2023 07:38

luolanzone reviewed Nov 9, 2023

View reviewed changes

pkg/agent/controller/traceflow/packetin_test.go Outdated Show resolved Hide resolved

luolanzone requested review from tnqn and antoninbas November 9, 2023 08:47

Atish-iaf mentioned this pull request Nov 10, 2023

Use Traceflow API v1beta1 #5689

Merged

antoninbas reviewed Nov 13, 2023

View reviewed changes

Atish-iaf force-pushed the add-rule-name-in-traceflow branch from f39adb9 to f14e6b5 Compare November 16, 2023 06:07

Add NetworkPolicy rule name in Traceflow observation

d2cc4a5

Update Traceflow e2e tests to verify "NetworkPolicy" and "NetworkPolicyRule" in Traceflow observation. Signed-off-by: Kumar Atish <atish.iaf@gmail.com>

Atish-iaf force-pushed the add-rule-name-in-traceflow branch from f14e6b5 to d2cc4a5 Compare November 16, 2023 06:11

Atish-iaf changed the title ~~Add NetworkPolicy rule name in traceflow observation~~ Add NetworkPolicy rule name in Traceflow observation Nov 16, 2023

tnqn approved these changes Nov 16, 2023

View reviewed changes

antoninbas approved these changes Nov 16, 2023

View reviewed changes

tnqn merged commit 68b5a02 into antrea-io:main Nov 17, 2023
49 of 55 checks passed

tnqn mentioned this pull request Jan 24, 2024

Remove invalid flow when WireGuard is enabled #5909

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add NetworkPolicy rule name in Traceflow observation #5667

Add NetworkPolicy rule name in Traceflow observation #5667

Atish-iaf commented Nov 3, 2023 •

edited

gran-vmv left a comment

rajnkamr Nov 6, 2023

antoninbas Nov 13, 2023

rajnkamr Nov 14, 2023

Atish-iaf commented Nov 6, 2023

luolanzone left a comment

antoninbas left a comment

antoninbas Nov 13, 2023

antoninbas Nov 13, 2023

antoninbas Nov 13, 2023

antoninbas Nov 13, 2023

antoninbas Nov 13, 2023

tnqn left a comment

tnqn commented Nov 16, 2023

antoninbas commented Nov 16, 2023

Add NetworkPolicy rule name in Traceflow observation #5667

Add NetworkPolicy rule name in Traceflow observation #5667

Conversation

Atish-iaf commented Nov 3, 2023 • edited

gran-vmv 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

Atish-iaf commented Nov 6, 2023

luolanzone left a comment

Choose a reason for hiding this comment

antoninbas 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

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Nov 16, 2023

antoninbas commented Nov 16, 2023

Atish-iaf commented Nov 3, 2023 •

edited