-
Notifications
You must be signed in to change notification settings - Fork 366
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Report captured packet in live-traffic Traceflow and support capturing only dropped packet #2029
Conversation
Example command
|
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
41f485b
to
c089ab9
Compare
/test-ipv6-e2e |
/test-ipv6-only-e2e |
// 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
if (err != nil) != tt.wantErr { | ||
t.Errorf("getPacketInfo() error = %v, wantErr %v", err, tt.wantErr) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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")
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Removed them.
|
||
tcpSrcPort, tcpDstPort, tcpSeqNum, tcpAckNum, tcpCode, err := GetTCPHeaderData(pktIn) | ||
if (err != nil) != tt.wantErr { | ||
t.Errorf("getPacketInfo() error = %v, wantErr %v", err, tt.wantErr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is actually no call to getPacketInfo()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
test/e2e/traceflow_test.go
Outdated
@@ -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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no need for fmt.Errorf
here. t.Fatal
/ t.Fatalf
support positional arguments for the error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Changed.
a5f0c6a
to
4ae6421
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
if err != nil { | ||
t.Errorf("GetTCPHeaderData() returned error: %v", err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
require.NoError(t, err, "GetTCPHeaderData() returned an error")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/test-all |
/test-all |
/test-all |
/test-all |
/test-ipv6-e2e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/test-conformance |
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"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these 3 properties used for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -516,6 +518,12 @@ func TestTraceflowIntraNode(t *testing.T) { | |||
}, | |||
}, | |||
}, | |||
expectedPktCap: &v1alpha1.Packet{ | |||
SrcIP: node1IPs[0].ipv4.String(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jianjuns We'll get nil pointer panic here in IPv6 e2e.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix: #2059
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