Fix Traceflow implementation for external IPs and gateway IP #1884

antoninbas · 2021-02-18T04:41:38Z

PR #1883 fixes a panic in libOpenflow triggered when OVS receives reply
traffic for a Traceflow request with a valid dataplane tag as the ToS
field and the Linux packet mark set. However, it should be noted that
reply packets for Traceflow requests are generally meaningless and
should be ignored. In encapMode, The Traceflow implementation should
also not timeout when a Traceflow request leaves the overlay: as soon as
the request is forwarded through the gateway port, we should consider
the request complete, and ignore any potential reply packet. So we
include the following changes:

add a new "ForwardedOutOfOverlay" Traceflow action when a request is
forwarded out of the network managed by Antrea in encapMode. The
Controller can then mark the request as "succeeded". In theory,
something similar could be done for other traffic modes, but it would
be much more complex.
add support for Traceflow requests for which the destination is the
gateway's IP, by reporting a "Delivered" action.
add an OVS flow in charge of dropping reply traffic for Traceflow
requests (using the conntrack state to match this traffic), thus
ensuring it is not sent to the Agent. In our testing, this is
especially useful when the destination IP is the local Node's IP, as
the IP ToS field seems to be preseved in that case, causing the reply
packet to be treated as a Traceflow request.

We add end-to-end tests for both cases (external destination IP and
Antrea gateway destination IP).

See #1878

gran-vmv · 2021-02-18T06:35:10Z

pkg/agent/controller/traceflow/packetin.go

 			ob.TunnelDstIP = tunnelDstIP
 			ob.Action = opsv1alpha1.Forwarded
+		} else if ipDst == gatewayIP.String() && outputPort == config.HostGatewayOFPort {
+			ob.Action = opsv1alpha1.Delivered
+		} else if c.networkConfig.TrafficEncapMode.SupportsEncap() && outputPort == config.HostGatewayOFPort {


Can we check if ipDst in Pod CIDR here? Current we support to trace inter-Node packet without encapsulation, but this behavior is changed in this PR.
Otherwise the e2e test will fail: https://github.com/vmware-tanzu/antrea/pull/1884/checks?check_run_id=1924336861

Thanks, I can see that I forgot an if case here. Added it back.

codecov-io · 2021-02-18T19:58:13Z

Codecov Report

❗ No coverage uploaded for pull request base (main@f389d2e). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1884   +/-   ##
=======================================
  Coverage        ?   53.48%           
=======================================
  Files           ?      200           
  Lines           ?    17272           
  Branches        ?        0           
=======================================
  Hits            ?     9238           
  Misses          ?     6873           
  Partials        ?     1161

Flag	Coverage Δ
e2e-tests	`43.52% <0.00%> (?)`
kind-e2e-tests	`50.12% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

gran-vmv · 2021-02-19T09:19:10Z

/test-all
/test-ipv6-all
/test-ipv6-only-all

jianjuns

The new behavior sound good to me.

jianjuns · 2021-02-19T21:46:39Z

pkg/agent/openflow/pipeline.go

+				Action().SendToController(uint8(PacketInReasonTF)).
+				Cookie(c.cookieAllocator.Request(category).Raw()).
+				Done())
+			// Only SendToController if output port is local gateway.


If we still output when kube-proxy is used, could we still trace the Service traffic?

In theory, we do not support tracing Service traffic when AntreaProxy is disabled. It is documented (https://github.com/vmware-tanzu/antrea/blob/main/docs/traceflow-guide.md#prerequisites) and the following error will be logged by the agent: Error: using Service destination requires AntreaProxy feature enabled. However, it dates back to before we started using DSCP to carry the Traceflow dataplane tag. Now that we do, it may be possible to lift the restriction, since iptables will preserve the DSCP field. I commented out the error in the agent code, and this is what I get for a Traceflow request from a Pod to a Service, when AntreaProxy is disabled:

name: default-toolbox-jvmv5-to-kube-system-kube-dns-xs25d865 phase: Succeeded source: default/toolbox-jvmv5 destination: kube-system/kube-dns results: - node: k8s-node-worker-1 timestamp: 1613785297 observations: - component: Forwarding componentInfo: Classification action: Received - component: Forwarding componentInfo: Output action: Delivered - node: k8s-node-control-plane timestamp: 1613785297 observations: - component: SpoofGuard action: Forwarded - component: Forwarding componentInfo: Output action: Forwarded tunnelDstIP: 192.168.77.101 - node: k8s-node-control-plane timestamp: 1613785297 observations: - component: SpoofGuard action: Forwarded - component: Forwarding componentInfo: Output action: Forwarded

This is definitely not as straightforward as the one when AntreaProxy is enabled (although it is accurate):

name: default-toolbox-jvmv5-to-kube-system-kube-dns-m489kxwk phase: Succeeded source: default/toolbox-jvmv5 destination: kube-system/kube-dns results: - node: k8s-node-control-plane timestamp: 1613785535 observations: - component: SpoofGuard action: Forwarded - component: LB action: Forwarded pod: kube-system/coredns-74ff55c5b-vc7z7 translatedDstIP: 10.10.1.2 - component: Forwarding componentInfo: Output action: Forwarded tunnelDstIP: 192.168.77.101 - node: k8s-node-worker-1 timestamp: 1613785535 observations: - component: Forwarding componentInfo: Classification action: Received - component: Forwarding componentInfo: Output action: Delivered

I am personally not sure it is worth lifting the restriction, but I can open a PR to do it and update the documentation. I don't know if there is any edge case I am missing. @jianjuns what do you think?

You are right. Probably not worthwhile, given AntreaProxy is enabled by default now.

pkg/agent/openflow/pipeline.go

tnqn

LGTM, just a typo

test/e2e/framework.go

PR antrea-io#1883 fixes a panic in libOpenflow triggered when OVS receives reply traffic for a Traceflow request with a valid dataplane tag as the ToS field and the Linux packet mark set. However, it should be noted that reply packets for Traceflow requests are generally meaningless and should be ignored. In encapMode, The Traceflow implementation should also not timeout when a Traceflow request leaves the overlay: as soon as the request is forwarded through the gateway port, we should consider the request complete, and ignore any potential reply packet. So we include the following changes: * add a new "ForwardedOutOfOverlay" Traceflow action when a request is forwarded out of the network managed by Antrea in encapMode. The Controller can then mark the request as "succeeded". In theory, something similar could be done for other traffic modes, but it would be much more complex. * add support for Traceflow requests for which the destination is the gateway's IP, by reporting a "Delivered" action. * add an OVS flow in charge of dropping reply traffic for Traceflow requests (using the conntrack state to match this traffic), thus ensuring it is not set to the Agent. In our testing, this is especially useful when the destination IP is the local Node's IP, as the IP ToS field seems to be preseved in that case, causing the reply packet to be treated as a Traceflow request. We add end-to-end tests for both cases (external destination IP and Antrea gateway destination IP). See antrea-io#1878

Co-authored-by: Quan Tian <[email protected]>

tnqn

LGTM

antoninbas · 2021-02-25T03:10:42Z

/test-all
/test-ipv6-all
/test-ipv6-only-all

antoninbas · 2021-02-25T22:22:42Z

/test-conformance

vmwclabot added the cla-not-required label Feb 18, 2021

antoninbas requested review from tnqn, gran-vmv, jianjuns and lzhecheng February 18, 2021 04:41

gran-vmv reviewed Feb 18, 2021

View reviewed changes

jianjuns reviewed Feb 19, 2021

View reviewed changes

tnqn reviewed Feb 24, 2021

View reviewed changes

pkg/agent/openflow/pipeline.go Show resolved Hide resolved

tnqn previously approved these changes Feb 24, 2021

View reviewed changes

test/e2e/framework.go Outdated Show resolved Hide resolved

antoninbas dismissed tnqn’s stale review via a778c0a February 25, 2021 02:06

antoninbas and others added 4 commits February 24, 2021 18:08

Fix noEncap case

57f8fbf

Fix typo

ec54102

Co-authored-by: Quan Tian <[email protected]>

Add comment to clarify encap vs noEncap mode

95f43fd

antoninbas force-pushed the fix-traceflow-for-destination-external-IPs-and-gateway-IP branch from a778c0a to 95f43fd Compare February 25, 2021 02:12

antoninbas requested a review from tnqn February 25, 2021 02:13

tnqn approved these changes Feb 25, 2021

View reviewed changes

antoninbas merged commit 8038935 into antrea-io:main Feb 26, 2021

antoninbas deleted the fix-traceflow-for-destination-external-IPs-and-gateway-IP branch February 26, 2021 02:42

antoninbas mentioned this pull request May 6, 2021

Support Gateway IP as destination IP in Traceflow #1500

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Traceflow implementation for external IPs and gateway IP #1884

Fix Traceflow implementation for external IPs and gateway IP #1884

antoninbas commented Feb 18, 2021 •

edited

Loading

gran-vmv Feb 18, 2021

antoninbas Feb 18, 2021

codecov-io commented Feb 18, 2021 •

edited

Loading

gran-vmv commented Feb 19, 2021

jianjuns left a comment

jianjuns Feb 19, 2021

antoninbas Feb 20, 2021

jianjuns Feb 20, 2021

tnqn left a comment

tnqn left a comment

antoninbas commented Feb 25, 2021

antoninbas commented Feb 25, 2021

Fix Traceflow implementation for external IPs and gateway IP #1884

Fix Traceflow implementation for external IPs and gateway IP #1884

Conversation

antoninbas commented Feb 18, 2021 • edited Loading

gran-vmv Feb 18, 2021

Choose a reason for hiding this comment

antoninbas Feb 18, 2021

Choose a reason for hiding this comment

codecov-io commented Feb 18, 2021 • edited Loading

Codecov Report

gran-vmv commented Feb 19, 2021

jianjuns left a comment

Choose a reason for hiding this comment

jianjuns Feb 19, 2021

Choose a reason for hiding this comment

antoninbas Feb 20, 2021

Choose a reason for hiding this comment

jianjuns Feb 20, 2021

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

antoninbas commented Feb 25, 2021

antoninbas commented Feb 25, 2021

antoninbas commented Feb 18, 2021 •

edited

Loading

codecov-io commented Feb 18, 2021 •

edited

Loading