Fix tunnelDstIP field in traceflow on dual-stack #4529

Atish-iaf · 2023-01-03T16:16:38Z

On dual-stack testbed, Inter-Node traceflow observation for
IPv4 packet in encap mode has empty tunnelDstIP field because
isIPv6 variable is set.

Change the logic to set isIPv6 variable based on the packet
IPv4 or IPv6.

Fixes #4502

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

codecov · 2023-01-03T16:36:07Z

Codecov Report

Merging #4529 (6131384) into main (bc74667) will increase coverage by 4.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #4529      +/-   ##
==========================================
+ Coverage   65.52%   69.54%   +4.02%     
==========================================
  Files         390      400      +10     
  Lines       58147    58235      +88     
==========================================
+ Hits        38098    40498    +2400     
+ Misses      17361    14962    -2399     
- Partials     2688     2775      +87

Flag	Coverage Δ
e2e-tests	`38.22% <100.00%> (-0.12%)`	⬇️
integration-tests	`34.59% <ø> (-0.01%)`	⬇️
kind-e2e-tests	`47.05% <100.00%> (-0.96%)`	⬇️
unit-tests	`59.56% <100.00%> (+1.89%)`	⬆️

Impacted Files	Coverage Δ
pkg/agent/controller/traceflow/packetin.go	`67.46% <100.00%> (+0.08%)`	⬆️
pkg/agent/util/ethtool/ethtool_linux.go	`0.00% <0.00%> (-70.00%)`	⬇️
pkg/agent/flowexporter/exporter/exporter.go	`70.96% <0.00%> (-4.04%)`	⬇️
pkg/agent/consistenthash/consistenthash.go	`93.75% <0.00%> (-3.75%)`	⬇️
pkg/controller/networkpolicy/mutate.go	`63.47% <0.00%> (-3.48%)`	⬇️
...agent/flowexporter/connections/deny_connections.go	`87.09% <0.00%> (-3.23%)`	⬇️
pkg/controller/traceflow/controller.go	`74.72% <0.00%> (-2.53%)`	⬇️
...ntroller/networkpolicy/networkpolicy_controller.go	`70.66% <0.00%> (-2.48%)`	⬇️
...catesigningrequest/ipsec_csr_signing_controller.go	`61.65% <0.00%> (-2.46%)`	⬇️
pkg/agent/controller/networkpolicy/reject.go	`74.03% <0.00%> (-2.41%)`	⬇️
... and 81 more

Atish-iaf · 2023-01-03T17:31:16Z

/test-ipv6-e2e

gran-vmv · 2023-01-04T01:29:50Z

pkg/agent/controller/traceflow/packetin.go

+			isIPv6 = etherData.Ethertype == protocol.IPv6_MSG
+		} else {
+			isIPv6 = c.nodeConfig.NodeIPv6Addr != nil
+		}
 		if match := getMatchTunnelDstField(matchers, isIPv6); match != nil {


Maybe we'd better get both IPv4 tunnel dst and IPv6 tunnel dst, and set tunnelDstIP with valid one.

You can also add verification in traceflow_test.go:testTraceflowInterNode, to check if the traceflow observation has expected tunnelDstIP.

Hi,
Even if we get both IPv4 tunnel dst and IPv6 tunnel dst, tunnelDstIP value will be decided according to isIPv6. The current code sets the value of tunnelDstIP according to isIPv6 directly without getting both tunnel dst. So, why to get both tunnel dst explicitly?

testTraceflowInterNode currently runs in both encap mode and no encap mode. If tunnelDstIP is added then we have to make the test run only in encap mode. Is it ok to change this test to run only in encap mode?

whether it is dual stack / non dual stack, in both the cases handling can be done with isIPv6 flag with right packet type, With this logic isIPv6 flag remain false if packet is not of ipv6 type irrespective of stack and hence tunnelDstIP will be assigned with ipv4 and vice versa.

UT testing can be extended to see the right values in tunnelDstIP, if the mode is encap.

Hi, Even if we get both IPv4 tunnel dst and IPv6 tunnel dst, tunnelDstIP value will be decided according to isIPv6. The current code sets the value of tunnelDstIP according to isIPv6 directly without getting both tunnel dst. So, why to get both tunnel dst explicitly?

testTraceflowInterNode currently runs in both encap mode and no encap mode. If tunnelDstIP is added then we have to make the test run only in encap mode. Is it ok to change this test to run only in encap mode?

Because reading both ipv4/ipv6 tunnel dst is more simple to decide the tunnel dst, and it won't depend on the flow logic in OVS table.
Currently only IPv4 tunnel dst is used in DualStack env. Please check: https://github.com/antrea-io/antrea/blob/main/pkg/agent/openflow/client.go#L485

You can add the TunnelDstIP into testTraceflowInterNode like

node2TunnelDstIP = "" switch encapMode { case config.TrafficEncapModeEncap: node2TunnelDstIP = nodeIP(nodeIdx1) case config.TrafficEncapModeHybrid: // check more } ... { Component: v1alpha1.ComponentForwarding, ComponentInfo: "Output", Action: v1alpha1.ActionForwarded, TunnelDstIP: node2TunnelDstIP, },

And add the comparison into compareObservations

@wenyingd
Do we use peer Node IPv4 as tunnelDst for IPv4 packet and peer Node IPv6 as tunnelDst for IPv6 packet?

We prefer to use tunnel format IPv4OverIPv4 and IPv6OverIPv6.

Atish-iaf · 2023-01-06T17:44:04Z

/test-ipv6-e2e

Atish-iaf · 2023-01-07T05:06:05Z

/test-ipv6-e2e

gran-vmv · 2023-01-11T01:20:02Z

/test-all
/test-flexible-ipam-e2e
/test-ipv6-only-all
/test-ipv6-networkpolicy
/test-ipv6-conformance
/test-windows-all

gran-vmv · 2023-01-11T01:20:18Z

/test-multicluster-e2e

gran-vmv · 2023-01-11T01:20:47Z

/test-all

gran-vmv · 2023-01-11T01:59:27Z

/test-all

gran-vmv · 2023-01-11T10:03:51Z

/test-windows-containerd-networkpolicy
/test-windows-networkpolicy

Atish-iaf · 2023-01-11T16:05:38Z

/test-ipv6-only-e2e

Atish-iaf · 2023-01-18T04:33:00Z

/test-all-features-conformance
/test-hw-offload
/test-ipv6-only-e2e

wenyingd · 2023-02-06T08:53:59Z

pkg/agent/controller/traceflow/packetin.go

+		var isIPv6 bool
+		// On dual stack, decide according to packet.
+		if c.nodeConfig.NodeIPv4Addr != nil && c.nodeConfig.NodeIPv6Addr != nil {
+			isIPv6 = etherData.Ethertype == protocol.IPv6_MSG


Just confused that you already check the L3 protocol from the packet header, what this if-condition c.nodeConfig.NodeIPv4Addr != nil && c.nodeConfig.NodeIPv6Addr != nil is needed?

Thanks for review.
You are correct, this condition check isn't required. Changed to decide based on packet only for any(ipv4/ipv6/dual) stack cluster.

wenyingd · 2023-02-06T08:54:50Z

pkg/agent/controller/traceflow/packetin.go

+			isIPv6 = etherData.Ethertype == protocol.IPv6_MSG
+		} else {
+			isIPv6 = c.nodeConfig.NodeIPv6Addr != nil
+		}
 		if match := getMatchTunnelDstField(matchers, isIPv6); match != nil {


We prefer to use tunnel format IPv4OverIPv4 and IPv6OverIPv6.

Atish-iaf · 2023-02-06T14:52:59Z

/test-all
/test-flexible-ipam-e2e
/test-ipv6-only-all
/test-ipv6-networkpolicy
/test-ipv6-conformance
/test-windows-all

Atish-iaf · 2023-02-06T16:45:02Z

/test-ipv6-e2e

gran-vmv · 2023-03-02T07:14:16Z

test/e2e/traceflow_test.go

+		// check if node1 and node2 are in same subnet.
+		node2IP := net.ParseIP(nodeIP(nodeIdx1))
+		cmd := getPingCommand(pingCount, 0, "", &node2IP)
+		stdout, _, err := data.RunCommandFromPod(data.testNamespace, node1Pods[0], agnhostContainerName, cmd)


We can use antrea-agent Pod antrea-agent container on node1 to run the command.

tnqn · 2023-03-02T10:50:32Z

test/e2e/traceflow_test.go

+	case config.TrafficEncapModeEncap:
+		node2TunnelDstIPv4 = clusterInfo.nodes[nodeIdx1].ipv4Addr
+		node2TunnelDstIPv6 = clusterInfo.nodes[nodeIdx1].ipv6Addr
+		// TODO: Add case for Hybrid mode when Nodes are in different subnet.


I think the test in hybrid mode would fail if the nodes are not in the same subnet as the expectation is empty but the actual observation is not?

Yes, but we can keep it for now if our testbeds on which e2e tests run are in same subnet so that tunnelIP validation can be done for all cases except one i.e Nodes on different subnet in hybrid mode.

Edit : Will remove tunnelIP validation.

tnqn · 2023-03-02T12:50:06Z

/test-e2e
/test-ipv6-e2e
/test-ipv6-only-e2e
/skip-conformance
/skip-networkpolicy

Atish-iaf · 2023-03-02T13:10:26Z

/test-ipv6-e2e

Atish-iaf · 2023-03-02T13:41:15Z

/test-ipv6-e2e
/test-ipv6-only-e2e

Atish-iaf · 2023-03-02T17:37:44Z

/test-ipv6-e2e
/test-ipv6-only-e2e

jianjuns · 2023-03-02T21:43:49Z

/test-ipv6-e2e

jianjuns · 2023-03-02T21:44:05Z

/test-ipv6-only-e2e

Atish-iaf · 2023-03-03T01:13:56Z

/test-ipv6-e2e
/test-ipv6-only-e2e

tnqn · 2023-03-03T02:30:13Z

The test error may be related to #4673, which will be fixed by #4674

tnqn · 2023-03-03T04:36:32Z

#4674 has been merged, could you rebase to see if e2e-ipv6 can pass now?

On dual-stack testbed, Inter-Node traceflow observation for IPv4 packet in encap mode has empty tunnelDstIP field because isIPv6 variable is set. Change the logic to set isIPv6 variable based on the packet IPv4 or IPv6. Fixes antrea-io#4502 Signed-off-by: Kumar Atish <atish.iaf@gmail.com>

Atish-iaf · 2023-03-03T05:05:33Z

/test-e2e
/test-ipv6-e2e
/test-ipv6-only-e2e
/skip-conformance
/skip-networkpolicy

Atish-iaf · 2023-03-03T06:18:00Z

/test-e2e

tnqn · 2023-03-03T08:11:17Z

/skip-conformance

tnqn · 2023-03-03T08:25:07Z

/skip-networkpolicy

On dual-stack testbed, Inter-Node traceflow observation for IPv4 packet in encap mode has empty tunnelDstIP field because isIPv6 variable is set. Change the logic to set isIPv6 variable based on the packet IPv4 or IPv6. Fixes antrea-io#4502 Signed-off-by: Kumar Atish <atish.iaf@gmail.com>

Atish-iaf requested review from antoninbas, gran-vmv, jianjuns and tnqn January 3, 2023 18:42

gran-vmv reviewed Jan 4, 2023

View reviewed changes

Atish-iaf mentioned this pull request Jan 4, 2023

Egress on dual-stack testbed #4532

Open

Atish-iaf force-pushed the traceflow/dualstack/ipv4 branch from 05777e5 to d361404 Compare January 6, 2023 16:13

Atish-iaf force-pushed the traceflow/dualstack/ipv4 branch from d361404 to 41bf4f9 Compare January 7, 2023 04:43

Atish-iaf requested a review from gran-vmv January 7, 2023 05:07

wenyingd reviewed Feb 6, 2023

View reviewed changes

Atish-iaf force-pushed the traceflow/dualstack/ipv4 branch from 41bf4f9 to 6131384 Compare February 6, 2023 11:33

Atish-iaf requested a review from wenyingd February 6, 2023 16:52

Atish-iaf mentioned this pull request Feb 17, 2023

test-ipv6-e2e failed #4633

Closed

gran-vmv added this to the Antrea v1.11 release milestone Feb 24, 2023

gran-vmv added kind/bug Categorizes issue or PR as related to a bug. area/ops/traceflow Issues or PRs related to the Traceflow feature area/transit/ipv6 Issues or PRs related to IPv6. labels Feb 24, 2023

gran-vmv reviewed Mar 2, 2023

View reviewed changes

Atish-iaf force-pushed the traceflow/dualstack/ipv4 branch from 6131384 to c616b76 Compare March 2, 2023 07:58

Atish-iaf requested review from gran-vmv and tnqn March 2, 2023 09:16

Atish-iaf force-pushed the traceflow/dualstack/ipv4 branch from c616b76 to ef262de Compare March 2, 2023 09:19

tnqn reviewed Mar 2, 2023

View reviewed changes

Atish-iaf requested a review from tnqn March 2, 2023 11:31

Atish-iaf force-pushed the traceflow/dualstack/ipv4 branch from ef262de to 94c2b34 Compare March 2, 2023 11:47

tnqn previously approved these changes Mar 2, 2023

View reviewed changes

Atish-iaf dismissed tnqn’s stale review via 2d51095 March 3, 2023 05:03

Atish-iaf force-pushed the traceflow/dualstack/ipv4 branch from 94c2b34 to 2d51095 Compare March 3, 2023 05:03

tnqn approved these changes Mar 3, 2023

View reviewed changes

tnqn merged commit 9c95110 into antrea-io:main Mar 3, 2023

luolanzone mentioned this pull request Apr 11, 2023

Automated cherry pick of #4529: Fix tunnelDstIP field in traceflow on dual-stack #4840

Merged

Fix tunnelDstIP field in traceflow on dual-stack #4529

Fix tunnelDstIP field in traceflow on dual-stack #4529

Conversation

Atish-iaf commented Jan 3, 2023 • edited Loading

codecov bot commented Jan 3, 2023 • edited Loading

Codecov Report

Atish-iaf commented Jan 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gran-vmv Jan 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Atish-iaf commented Jan 6, 2023

Atish-iaf commented Jan 7, 2023

gran-vmv commented Jan 11, 2023

gran-vmv commented Jan 11, 2023

gran-vmv commented Jan 11, 2023

gran-vmv commented Jan 11, 2023

gran-vmv commented Jan 11, 2023

Atish-iaf commented Jan 11, 2023

Atish-iaf commented Jan 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Atish-iaf commented Feb 6, 2023

Atish-iaf commented Feb 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Atish-iaf Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

tnqn commented Mar 2, 2023

Atish-iaf commented Mar 2, 2023 • edited Loading

Atish-iaf commented Mar 2, 2023

Atish-iaf commented Mar 2, 2023

jianjuns commented Mar 2, 2023

jianjuns commented Mar 2, 2023

Atish-iaf commented Mar 3, 2023

tnqn commented Mar 3, 2023

tnqn commented Mar 3, 2023

Atish-iaf commented Mar 3, 2023

Atish-iaf commented Mar 3, 2023

tnqn commented Mar 3, 2023

tnqn commented Mar 3, 2023

Atish-iaf commented Jan 3, 2023 •

edited

Loading

codecov bot commented Jan 3, 2023 •

edited

Loading

gran-vmv Jan 4, 2023 •

edited

Loading

Atish-iaf Mar 2, 2023 •

edited

Loading

Atish-iaf commented Mar 2, 2023 •

edited

Loading