Report captured packet in live-traffic Traceflow and support capturing only dropped packet #2029

jianjuns · 2021-04-05T03:18:37Z

For live-traffic Traceflow, the sender will collect the captured packet and update to the Traceflow CR (Traceflow.Status.CapturedPacket).
Add support for capturing only the packet dropped by NetworkPolicy or Antrea NetworkPolicy in live-traffic Traceflow. In this case, only the Node (can be either the sender or the receiver Node) the packet is dropped will update the Traceflow CR Status. For example, a Traceflow can be created to capture the dropped TCP packet from a Pod to a Service within 10 minutes.
Extended antctl Traceflow command to support printing the captured packet, and support creating a Traceflow for capturing dropped packet.

Issue: #2030

jianjuns · 2021-04-05T03:23:29Z

Example command

# Capture the first dropped TCP packet from Pod client to Service web-server within 10m.
$ antctl traceflow -S client -D web-server --live-traffic -f tcp -t 10m --dropped-only
name: client-web-server-to-any-5sb65mnq
phase: Succeeded
source: client
destination: web-server
results:
- node: k8s2
  timestamp: 1617591772
  observations:
  - component: Forwarding
    componentInfo: Classification
    action: Received
  - component: NetworkPolicy
    componentInfo: IngressDefaultRule
    action: Dropped
capturedPacket:
  srcIP: 172.100.0.16
  dstIP: 172.100.1.13
  length: 60
  ipHeader:
    protocol: 6
    ttl: 62
    flags: 2
  tranportHeader:
    tcp:
      srcport: 47950
      dstport: 80
      flags: 2

codecov-io · 2021-04-05T03:33:38Z

Codecov Report

Merging #2029 (196d367) into main (3b294ea) will decrease coverage by 6.15%.
The diff coverage is 57.71%.

@@            Coverage Diff             @@
##             main    #2029      +/-   ##
==========================================
- Coverage   61.71%   55.56%   -6.16%     
==========================================
  Files         262      266       +4     
  Lines       19560    19859     +299     
==========================================
- Hits        12072    11035    -1037     
- Misses       6236     7652    +1416     
+ Partials     1252     1172      -80

Flag	Coverage Δ
kind-e2e-tests	`41.74% <48.66%> (-11.69%)`	⬇️
unit-tests	`41.58% <35.42%> (+0.15%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/agent/controller/networkpolicy/packetin.go	`6.08% <0.00%> (-62.87%)`	⬇️
pkg/ovs/openflow/ofctrl_packetin.go	`35.08% <35.08%> (ø)`
pkg/antctl/raw/traceflow/command.go	`25.42% <52.00%> (-0.92%)`	⬇️
pkg/agent/controller/traceflow/packetin.go	`60.26% <59.45%> (-0.22%)`	⬇️
pkg/controller/traceflow/controller.go	`69.69% <64.00%> (-0.37%)`	⬇️
...agent/controller/traceflow/traceflow_controller.go	`73.28% <100.00%> (+0.30%)`	⬆️
pkg/agent/openflow/client.go	`52.39% <100.00%> (-7.45%)`	⬇️
pkg/agent/openflow/pipeline.go	`66.58% <100.00%> (-3.55%)`	⬇️
pkg/apis/controlplane/helper.go	`25.00% <0.00%> (-75.00%)`	⬇️
pkg/apis/controlplane/v1beta2/helper.go	`25.00% <0.00%> (-75.00%)`	⬇️
... and 77 more

jianjuns · 2021-04-05T20:42:44Z

/test-ipv6-e2e

jianjuns · 2021-04-06T02:06:50Z

/test-ipv6-only-e2e

tnqn · 2021-04-06T17:02:44Z

pkg/agent/controller/traceflow/packetin.go

+		// Report the captured dropped packet, if the Traceflow is for
+		// the dropped packet only; otherwise only the sender reports
+		// the first captured packet.
+		if tfState.isSender || tfState.droppedOnly {


If droppedOnly is set and the packet is dropped on receiver, could the sender and the receiver override the field that have been written by the other one? Is it expected?

Not really. For droppedOnly, I changed to flow to send to controller only when the packet is dropped, so only one of sender or receiver should come to here.

tnqn

LGTM overall

pkg/agent/openflow/pipeline.go

pkg/apis/crd/v1alpha1/types.go

antoninbas

I left comments on a unit test, but then realized you just moved it and it wasn't part of the PR really. So we can punt the changes to a subsequent PR.

Can you open a follow-up PR to add live traffic info to the antctl doc?

BTW, I was testing the feature locally and ran into the following issue:
If I open a TCP connection between Pods (with ncat), then initiate the live traffic capture, then send some data between the Pods, the TF request times out. However, if I initiate the capture before opening the connection, then it will behave as expected. Is it because of some bypass in the OVS pipeline for connections committed by conntrack? Is it something you expected? I think we should document this somewhere (maybe in the antctl doc).

antoninbas · 2021-04-06T21:26:15Z

pkg/ovs/openflow/ofctrl_packetin_test.go

+			if (err != nil) != tt.wantErr {
+				t.Errorf("getPacketInfo() error = %v, wantErr %v", err, tt.wantErr)
+			}


I suggest:

if tt.wantErr { require.Error(t, err, "Expected getPacketInfo() to return an error") return // stop test early } else { require.NoError(t, err, "Expected getPacketInfo() not to return an error") }

I feel GetTCPHeaderData() is not possible to return an error, at least not possible with the test (unless we construct binary packet data), so I removed wantErr. Hope it is fine to you.

pkg/ovs/openflow/ofctrl_packetin_test.go

antoninbas · 2021-04-06T21:29:35Z

pkg/ovs/openflow/ofctrl_packetin_test.go

+			assert.Equal(t, tt.args.expectTCPSrcPort, tcpSrcPort, "Expect to retrieve exact TCP src port while differed")
+			assert.Equal(t, tt.args.expectTCPDstPort, tcpDstPort, "Expect to retrieve exact TCP dst port while differed")
+			assert.Equal(t, tt.args.expectTCPSeqNum, tcpSeqNum, "Expect to retrieve exact TCP seq num while differed")
+			assert.Equal(t, tt.args.expectTCPAckNum, tcpAckNum, "Expect to retrieve exact TCP ack num while differed")
+			assert.Equal(t, tt.args.expectTCPCode, tcpCode, "Expect to retrieve exact TCP code while differed")


nit: the error messages don't seem grammatically to me. I also don't think they are needed since assert.Equal will display an informative message in case of mismatch

Agreed. Removed them.

antoninbas · 2021-04-06T21:30:46Z

pkg/ovs/openflow/ofctrl_packetin_test.go

+
+			tcpSrcPort, tcpDstPort, tcpSeqNum, tcpAckNum, tcpCode, err := GetTCPHeaderData(pktIn)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("getPacketInfo() error = %v, wantErr %v", err, tt.wantErr)


there is actually no call to getPacketInfo()?

antoninbas · 2021-04-06T21:41:53Z

test/e2e/traceflow_test.go

@@ -1906,4 +1920,7 @@ func runTestTraceflow(t *testing.T, data *TestData, tc testcase) {
 			}
 		}
 	}
+	if tc.expectedPktCap != nil && !reflect.DeepEqual(tc.expectedPktCap, tf.Status.CapturedPacket) {
+		t.Fatal(fmt.Errorf("captured packet should be: %+v, but got: %+v", tc.expectedPktCap, tf.Status.CapturedPacket))


there is no need for fmt.Errorf here. t.Fatal / t.Fatalf support positional arguments for the error message.

Sure. Changed.

antoninbas

LGTM

antoninbas · 2021-04-07T02:40:50Z

pkg/ovs/openflow/ofctrl_packetin_test.go

+			if err != nil {
+				t.Errorf("GetTCPHeaderData() returned error: %v", err)
+			}


nit:

require.NoError(t, err, "GetTCPHeaderData() returned an error")

tnqn

LGTM

pkg/apis/crd/v1alpha1/types.go

jianjuns · 2021-04-07T16:20:02Z

/test-all

jianjuns · 2021-04-07T18:58:46Z

/test-all

jianjuns · 2021-04-07T20:59:32Z

/test-all

jianjuns · 2021-04-07T21:37:41Z

/test-all

jianjuns · 2021-04-07T21:39:55Z

/test-ipv6-e2e

antoninbas

LGTM

jianjuns · 2021-04-08T00:53:10Z

/test-conformance

gran-vmv · 2021-04-08T01:41:58Z

pkg/apis/crd/v1alpha1/types.go

+	SrcIP string `json:"srcIP,omitempty"`
+	DstIP string `json:"dstIP,omitempty"`
+	// Length is the IP packet length (includes the IPv4 or IPv6 header length).
+	Length uint16 `json:"length,omitempty"`


What are these 3 properties used for?

They are for reporting information of the captured packet. See: #2029 (comment)

But I feel later we can support them for generating the injected packets too.

gran-vmv · 2021-04-09T02:22:53Z

test/e2e/traceflow_test.go

@@ -516,6 +518,12 @@ func TestTraceflowIntraNode(t *testing.T) {
 					},
 				},
 			},
+			expectedPktCap: &v1alpha1.Packet{
+				SrcIP:    node1IPs[0].ipv4.String(),


@jianjuns We'll get nil pointer panic here in IPv6 e2e.

vmwclabot added the cla-not-required label Apr 5, 2021

jianjuns requested a review from gran-vmv April 5, 2021 03:18

jianjuns force-pushed the tf-pkt-cap branch from 3df15b0 to 4527907 Compare April 5, 2021 03:29

jianjuns force-pushed the tf-pkt-cap branch 2 times, most recently from 41f485b to c089ab9 Compare April 5, 2021 16:10

jianjuns requested review from tnqn and antoninbas April 5, 2021 16:34

jianjuns mentioned this pull request Apr 5, 2021

Support tracing live traffic in Traceflow #2030

Closed

tnqn reviewed Apr 6, 2021

View reviewed changes

tnqn previously approved these changes Apr 6, 2021

View reviewed changes

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

pkg/apis/crd/v1alpha1/types.go Show resolved Hide resolved

antoninbas reviewed Apr 6, 2021

View reviewed changes

jianjuns dismissed tnqn’s stale review via a5f0c6a April 6, 2021 22:43

jianjuns force-pushed the tf-pkt-cap branch 2 times, most recently from a5f0c6a to 4ae6421 Compare April 6, 2021 22:46

jianjuns requested a review from lzhecheng April 7, 2021 00:45

antoninbas previously approved these changes Apr 7, 2021

View reviewed changes

jianjuns dismissed antoninbas’s stale review via 196d367 April 7, 2021 03:38

jianjuns force-pushed the tf-pkt-cap branch from 4ae6421 to 196d367 Compare April 7, 2021 03:38

tnqn previously approved these changes Apr 7, 2021

View reviewed changes

pkg/apis/crd/v1alpha1/types.go Show resolved Hide resolved

Report the captured packet in live-traffic Traceflow

7a86c23

jianjuns dismissed tnqn’s stale review via de0a7fd April 7, 2021 18:37

jianjuns force-pushed the tf-pkt-cap branch from 196d367 to de0a7fd Compare April 7, 2021 18:37

jianjuns force-pushed the tf-pkt-cap branch from de0a7fd to 7814c5b Compare April 7, 2021 20:59

Support capturing only the dropped packet in live-traffic Traceflow

14c6197

jianjuns force-pushed the tf-pkt-cap branch from 7814c5b to 14c6197 Compare April 7, 2021 21:14

jianjuns added this to the Antrea v1.0 release milestone Apr 8, 2021

antoninbas approved these changes Apr 8, 2021

View reviewed changes

gran-vmv reviewed Apr 8, 2021

View reviewed changes

jianjuns merged commit 4d92619 into antrea-io:main Apr 8, 2021

gran-vmv reviewed Apr 9, 2021

View reviewed changes

This was referenced Apr 9, 2021

Add IPv6 flow for traceflowConnectionTrackFlows #2054

Merged

Support AntreaNetworkPolicy reject action in Traceflow #2032

Merged

Fix Traceflow IPv6 e2e error #2059

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report captured packet in live-traffic Traceflow and support capturing only dropped packet #2029

Report captured packet in live-traffic Traceflow and support capturing only dropped packet #2029

jianjuns commented Apr 5, 2021 •

edited

Loading

jianjuns commented Apr 5, 2021 •

edited

Loading

codecov-io commented Apr 5, 2021 •

edited

Loading

jianjuns commented Apr 5, 2021

jianjuns commented Apr 6, 2021

tnqn Apr 6, 2021

jianjuns Apr 6, 2021

tnqn left a comment

antoninbas left a comment

antoninbas Apr 6, 2021

jianjuns Apr 6, 2021

antoninbas Apr 6, 2021

jianjuns Apr 6, 2021

antoninbas Apr 6, 2021

jianjuns Apr 6, 2021

antoninbas Apr 6, 2021

jianjuns Apr 6, 2021

antoninbas left a comment

antoninbas Apr 7, 2021

jianjuns Apr 7, 2021

tnqn left a comment

jianjuns commented Apr 7, 2021

jianjuns commented Apr 7, 2021

jianjuns commented Apr 7, 2021

jianjuns commented Apr 7, 2021

jianjuns commented Apr 7, 2021

antoninbas left a comment

jianjuns commented Apr 8, 2021

gran-vmv Apr 8, 2021

jianjuns Apr 8, 2021

gran-vmv Apr 9, 2021

gran-vmv Apr 9, 2021

Report captured packet in live-traffic Traceflow and support capturing only dropped packet #2029

Report captured packet in live-traffic Traceflow and support capturing only dropped packet #2029

Conversation

jianjuns commented Apr 5, 2021 • edited Loading

jianjuns commented Apr 5, 2021 • edited Loading

codecov-io commented Apr 5, 2021 • edited Loading

Codecov Report

jianjuns commented Apr 5, 2021

jianjuns commented Apr 6, 2021

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this 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

tnqn left a comment

Choose a reason for hiding this comment

jianjuns commented Apr 7, 2021

jianjuns commented Apr 7, 2021

jianjuns commented Apr 7, 2021

jianjuns commented Apr 7, 2021

jianjuns commented Apr 7, 2021

antoninbas left a comment

Choose a reason for hiding this comment

jianjuns commented Apr 8, 2021

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

jianjuns commented Apr 5, 2021 •

edited

Loading

jianjuns commented Apr 5, 2021 •

edited

Loading

codecov-io commented Apr 5, 2021 •

edited

Loading