Add bidirectional packet capture #6882

AryanBakliwal · 2024-12-23T04:18:38Z

fixes: #6862

Added bidirectional field in packet capture CR spec.
For testing, I created two pods and pinged one from the other.

Screenshot of the .pcapng output file.

Signed-off-by: Aryan Bakliwal <[email protected]>

AryanBakliwal · 2024-12-23T04:20:04Z

@hangyan please let me know what you think about this

pkg/agent/packetcapture/packetcapture_controller.go

pkg/apis/crd/v1alpha1/types.go

…antrea-io#6877) Bumps the golang-org-x group with 1 update: [golang.org/x/net](https://github.com/golang/net). Updates `golang.org/x/net` from 0.32.0 to 0.33.0 - [Commits](golang/net@v0.32.0...v0.33.0) --- updated-dependencies: - dependency-name: golang.org/x/net dependency-type: direct:production update-type: version-update:semver-minor dependency-group: golang-org-x ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…oup (antrea-io#6880) Bumps the ginkgo group with 1 update: [github.com/onsi/ginkgo/v2](https://github.com/onsi/ginkgo). Updates `github.com/onsi/ginkgo/v2` from 2.22.0 to 2.22.1 - [Release notes](https://github.com/onsi/ginkgo/releases) - [Changelog](https://github.com/onsi/ginkgo/blob/master/CHANGELOG.md) - [Commits](onsi/ginkgo@v2.22.0...v2.22.1) --- updated-dependencies: - dependency-name: github.com/onsi/ginkgo/v2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: ginkgo ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Signed-off-by: Aryan Bakliwal <[email protected]>

AryanBakliwal · 2025-01-01T13:13:56Z

Hey @hangyan @antoninbas, I made the changes in the bpf code according to the one generated by tcpdump.

When I try to test the bidirectional packet capture, it fails with this log

E1229 11:44:58.723258       1 packetcapture_controller.go:424] "PacketCapture failed capturing packets" err="invalid argument" name="pc-test"

Could you please take a look and help me identify what might be going wrong?

Also, golangci-lint is giving me this error even though I have changed the Capture method everywhere.

===> Running golangci (linux) <===
===> Running golangci (windows) <===
cmd/antrea-agent/agent.go:64:2: could not import antrea.io/antrea/pkg/agent/packetcapture (-: # antrea.io/antrea/pkg/agent/packetcapture
pkg/agent/packetcapture/packetcapture_controller.go:170:23: cannot use capture (variable of type *capture.pcapCapture) as PacketCapturer value in assignment: *capture.pcapCapture does not implement PacketCapturer (wrong type for method Capture)
                have Capture(context.Context, string, int, "net".IP, "net".IP, *"antrea.io/antrea/pkg/apis/crd/v1alpha1".Packet) (chan gopacket.Packet, error)
                want Capture(context.Context, string, int, "net".IP, "net".IP, *"antrea.io/antrea/pkg/apis/crd/v1alpha1".Packet, bool) (chan gopacket.Packet, error)) (typecheck)
        "antrea.io/antrea/pkg/agent/packetcapture"
        ^
pkg/agent/packetcapture/capture_interface.go:1: : # antrea.io/antrea/pkg/agent/packetcapture [antrea.io/antrea/pkg/agent/packetcapture.test]
pkg/agent/packetcapture/packetcapture_controller.go:170:23: cannot use capture (variable of type *capture.pcapCapture) as PacketCapturer value in assignment: *capture.pcapCapture does not implement PacketCapturer (wrong type for method Capture)
                have Capture(context.Context, string, int, "net".IP, "net".IP, *"antrea.io/antrea/pkg/apis/crd/v1alpha1".Packet) (chan gopacket.Packet, error)
                want Capture(context.Context, string, int, "net".IP, "net".IP, *"antrea.io/antrea/pkg/apis/crd/v1alpha1".Packet, bool) (chan gopacket.Packet, error) (typecheck)
// Copyright 2024 Antrea Authors.
make: *** [Makefile:329: golangci] Error 1

hangyan · 2025-01-02T02:29:30Z

Hey @hangyan @antoninbas, I made the changes in the bpf code according to the one generated by tcpdump.

When I try to test the bidirectional packet capture, it fails with this log

E1229 11:44:58.723258       1 packetcapture_controller.go:424] "PacketCapture failed capturing packets" err="invalid argument" name="pc-test"

Could you please take a look and help me identify what might be going wrong?

Also, golangci-lint is giving me this error even though I have changed the Capture method everywhere.

===> Running golangci (linux) <===
===> Running golangci (windows) <===
cmd/antrea-agent/agent.go:64:2: could not import antrea.io/antrea/pkg/agent/packetcapture (-: # antrea.io/antrea/pkg/agent/packetcapture
pkg/agent/packetcapture/packetcapture_controller.go:170:23: cannot use capture (variable of type *capture.pcapCapture) as PacketCapturer value in assignment: *capture.pcapCapture does not implement PacketCapturer (wrong type for method Capture)
                have Capture(context.Context, string, int, "net".IP, "net".IP, *"antrea.io/antrea/pkg/apis/crd/v1alpha1".Packet) (chan gopacket.Packet, error)
                want Capture(context.Context, string, int, "net".IP, "net".IP, *"antrea.io/antrea/pkg/apis/crd/v1alpha1".Packet, bool) (chan gopacket.Packet, error)) (typecheck)
        "antrea.io/antrea/pkg/agent/packetcapture"
        ^
pkg/agent/packetcapture/capture_interface.go:1: : # antrea.io/antrea/pkg/agent/packetcapture [antrea.io/antrea/pkg/agent/packetcapture.test]
pkg/agent/packetcapture/packetcapture_controller.go:170:23: cannot use capture (variable of type *capture.pcapCapture) as PacketCapturer value in assignment: *capture.pcapCapture does not implement PacketCapturer (wrong type for method Capture)
                have Capture(context.Context, string, int, "net".IP, "net".IP, *"antrea.io/antrea/pkg/apis/crd/v1alpha1".Packet) (chan gopacket.Packet, error)
                want Capture(context.Context, string, int, "net".IP, "net".IP, *"antrea.io/antrea/pkg/apis/crd/v1alpha1".Packet, bool) (chan gopacket.Packet, error) (typecheck)
// Copyright 2024 Antrea Authors.
make: *** [Makefile:329: golangci] Error 1

this error was reported by golangci on windows, we have a pcap_unsupported.go for windows/... interface, it's an non-op since we didn't support this on windows yet, you should change that too.

pkg/agent/packetcapture/capture/bpf.go

Signed-off-by: Aryan Bakliwal <[email protected]>

AryanBakliwal · 2025-01-05T18:52:56Z

I’ve added the direction field with enum values (SourceToDestination, DestinationToSource, and Both) and set the default to SourceToDestination. Additionally, I’ve updated the YAML manifests, packet capture guide and tests accordingly.

Please let me know if any adjustments are needed or if there's anything else I should update or add.

Captured packets

ICMP (SourceToDestination)

ICMP (DestinationToSource)

ICMP (Both)

TCP (SourceToDestination)

TCP (DestinationToSource)

TCP (Both)

pkg/agent/packetcapture/capture/bpf.go

hangyan · 2025-01-08T09:02:13Z

pkg/agent/packetcapture/capture/bpf.go

@@ -134,18 +122,96 @@ func compilePacketFilter(packetSpec *crdv1alpha1.Packet, srcIP, dstIP net.IP) []
 		}
 	}

+	// source ip


In the future we may also support tcp flags and other layer4 configs, if that happens, we should consider make the current code structure more modularized, or it would be extremely hard to extend this function . This won't be easy but i suggest to review this part and see if we can do better.

not sure if we can separate the ip section and ports section apart, call sub functions to calculate their instruments size and sums up.

I'll try my best to improve the code structure and explore separating the IP and port sections as suggested

@hangyan I am thinking of refactoring the code as you suggested by modularizing the logic for IP and port comparison by adding helper functions for taking decisions based on direction and other parameters.
I’ve created a function compareIP to handle the conditional logic for building the instruction to compare IP address depending on the direction and which IP address is loaded for comparison (the boolean isSrc tells if I have loaded the source or destination IP address)

func compareIP(direction crdv1alpha1.CaptureDirection, skipTrue, skipFalse uint8, isSrc bool, srcAddrVal, dstAddrVal uint32) bpf.Instruction { if isSrc { if direction == crdv1alpha1.CaptureDirectionDestinationToSource { return bpf.JumpIf{Cond: bpf.JumpEqual, Val: dstAddrVal, SkipTrue: skipTrue, SkipFalse: skipFalse} } else { return bpf.JumpIf{Cond: bpf.JumpEqual, Val: srcAddrVal, SkipTrue: skipTrue, SkipFalse: skipFalse} } } else { if direction == crdv1alpha1.CaptureDirectionDestinationToSource { return bpf.JumpIf{Cond: bpf.JumpEqual, Val: srcAddrVal, SkipTrue: skipTrue, SkipFalse: skipFalse} } else { return bpf.JumpIf{Cond: bpf.JumpEqual, Val: dstAddrVal, SkipTrue: skipTrue, SkipFalse: skipFalse} } } }

Similarly a function comparePort builds instruction for comparing port
Additional functions to calculate number of SkipTrue and SkipFalse instructions dynamically

Let me know if this looks better or if you have any suggestions for further improvements.

pkg/agent/packetcapture/capture/bpf_test.go

pkg/apis/crd/v1alpha1/types.go

antoninbas

could we add an e2e test with the Both direction?

pkg/agent/packetcapture/capture/bpf.go

pkg/apis/crd/v1alpha1/types.go

build/yamls/antrea.yml

pkg/apis/crd/v1alpha1/types.go

Signed-off-by: Aryan Bakliwal <[email protected]>

AryanBakliwal · 2025-01-19T22:21:13Z

@antoninbas @hangyan I have refactored the compilePacketFilter function, added a test case for dstPort and Both in bpf_test.go, generated the manifests by running make manifest and added an e2e test with the Both direction.
Could you please review the changes? Let me know if there’s anything else I can improve.

antoninbas

thanks for your ongoing work on this

antoninbas · 2025-01-21T21:41:40Z

pkg/agent/packetcapture/capture/bpf.go

+		transPort := packet.TransportHeader
+		if transPort.TCP != nil {
+			// load Fragment Offset
+			count += 3
+			if transPort.TCP.SrcPort != nil {
+				count += 2
+			}
+			if transPort.TCP.DstPort != nil {
+				count += 2
+			}
+
+		} else if transPort.UDP != nil {
+			count += 3
+			if transPort.UDP.SrcPort != nil {
+				count += 2
+			}
+			if transPort.UDP.DstPort != nil {
+				count += 2
+			}
+		}


correct me if I'm wrong but this code looks like a perfect duplicate of the code above
you could do something like this:

transport := packet.TransportHeader portFiltersSize := func() int { count := 0 if transport.TCP != nil { // load Fragment Offset count += 3 if transport.TCP.SrcPort != nil { count += 2 } if transport.TCP.DstPort != nil { count += 2 } } else if transport.UDP != nil { count += 3 if transport.UDP.SrcPort != nil { count += 2 } if transport.UDP.DstPort != nil { count += 2 } } return count }

Note that I have changed the spelling of transPort to transport, as the current version makes no sense.

You're correct, it is indeed a copy. I'll replace it with this function to remove duplication.
Thanks for sharing this.

antoninbas · 2025-01-21T21:43:23Z

pkg/agent/packetcapture/capture/bpf.go

@@ -214,6 +332,31 @@ func calculateInstructionsSize(packet *crdv1alpha1.Packet) int {
 	// src and dst ip
 	count += 4

+	if direction == crdv1alpha1.CaptureDirectionBoth {
+		count += 3


can you add a comment for this one?

based on your function comment, in the Both direction case, we have to repeat the source host and destination host filters. Is that what this is for? This seems "small" to me, as I see the following above:

// src and dst ip count += 4

but I assume you validated it in your unit tests

Yes, this is for the IP check instructions. We only need 3 more in the Both case because the source IP address is already loaded and

// (004) ld [26] # Load 4B at 26 (source address)

we jump to compare it with the destination IP address.

I will add a comment for this.

antoninbas · 2025-01-21T21:44:12Z

pkg/agent/packetcapture/capture/bpf.go

+	if direction == crdv1alpha1.CaptureDirectionBoth {
+		count += 3
+
+		transPort := packet.TransportHeader


I notice that you are not checking for packet != nil here, which doesn't match the above code?

Thanks for pointing this out, I overlooked it. Will add this check.

antoninbas · 2025-01-21T21:46:37Z

pkg/agent/packetcapture/capture/bpf.go

+	if srcIP != nil {
+		inst = append(inst, loadIPv4SourceAddress)
+		// from here we need to check the inst length to calculate skipFalse. If no protocol is set, there will be no related bpf instructions.
+		inst = append(inst, compareIP(direction, 0, calculateSkipFalse(size, uint8(len(inst)), srcPort, dstPort, direction), true, srcAddrVal, dstAddrVal))
+	}
+	// dst ip
+	if dstIP != nil {


the code above assumes that srcIP and dstIP are never nil. There was a discussion about it, which was marked as resolved. We should be consistent. If this is a precondition for the function, then we should not include these nil checks.

You're right, I will remove these nil checks.

antoninbas · 2025-01-21T21:54:31Z

pkg/agent/packetcapture/capture/bpf.go

@@ -72,11 +72,67 @@ func compareProtocol(protocol uint32, skipTrue, skipFalse uint8) bpf.Instruction
 	return bpf.JumpIf{Cond: bpf.JumpEqual, Val: protocol, SkipTrue: skipTrue, SkipFalse: skipFalse}
 }

+func calculateSkipTrue(size, instlen uint8, direction crdv1alpha1.CaptureDirection, srcPort, dstPort uint16, isSrc bool) uint8 {


the way these helper functions are written doesn't make a lot of sense to me. The functions have a generic name, yet they have very specialized implementation that is dependent on a bunch of parameters. To me, the fact that direction is a parameter to this function is a bit of a red flag.

overall, I would expect the implementation to be more streamlined. There should be a helper function to generate IP + port matching. We can call the function for the SourceToDestination direction. We can also call the function for the DestinationToSource direction by swapping src / dst function parameters. For the Both direction, the function should just be called twice, and the caller should add the glue to make it work (or operator). Am I missing something? Does it have to be much more complex than that?

AryanBakliwal added 2 commits December 23, 2024 00:26

add bidirection flag

875dde4

Signed-off-by: Aryan Bakliwal <[email protected]>

add bidirectional capture

1307c17

Signed-off-by: Aryan Bakliwal <[email protected]>

antoninbas reviewed Dec 23, 2024

View reviewed changes

pkg/agent/packetcapture/packetcapture_controller.go Outdated Show resolved Hide resolved

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

dependabot bot and others added 3 commits January 1, 2025 18:39

update code and tests

79a26c0

Signed-off-by: Aryan Bakliwal <[email protected]>

hangyan reviewed Jan 2, 2025

View reviewed changes

pkg/agent/packetcapture/capture/bpf.go Outdated Show resolved Hide resolved

AryanBakliwal added 2 commits January 5, 2025 19:42

Merge branch 'main' into bidirection

6e5898c

add enum, tests and docs

e5ef842

Signed-off-by: Aryan Bakliwal <[email protected]>

AryanBakliwal requested review from antoninbas, hangyan and tnqn January 5, 2025 18:53

hangyan reviewed Jan 8, 2025

View reviewed changes

antoninbas reviewed Jan 8, 2025

View reviewed changes

pkg/agent/packetcapture/capture/bpf.go Show resolved Hide resolved

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

build/yamls/antrea.yml Show resolved Hide resolved

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

refactor and add e2e test

7eb34de

Signed-off-by: Aryan Bakliwal <[email protected]>

AryanBakliwal force-pushed the bidirection branch from 7b17816 to 7eb34de Compare January 19, 2025 22:07

Merge branch 'main' into bidirection

5d04e5d

AryanBakliwal requested review from hangyan and antoninbas January 19, 2025 22:21

antoninbas reviewed Jan 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bidirectional packet capture #6882

Add bidirectional packet capture #6882

AryanBakliwal commented Dec 23, 2024 •

edited

Loading

AryanBakliwal commented Dec 23, 2024

AryanBakliwal commented Jan 1, 2025

hangyan commented Jan 2, 2025

AryanBakliwal commented Jan 5, 2025

hangyan Jan 8, 2025

AryanBakliwal Jan 8, 2025

AryanBakliwal Jan 16, 2025 •

edited

Loading

antoninbas left a comment

AryanBakliwal commented Jan 19, 2025

antoninbas left a comment

antoninbas Jan 21, 2025

AryanBakliwal Jan 26, 2025

antoninbas Jan 21, 2025

AryanBakliwal Jan 26, 2025

antoninbas Jan 21, 2025

AryanBakliwal Jan 26, 2025

antoninbas Jan 21, 2025

AryanBakliwal Jan 26, 2025

antoninbas Jan 21, 2025

Add bidirectional packet capture #6882

Are you sure you want to change the base?

Add bidirectional packet capture #6882

Conversation

AryanBakliwal commented Dec 23, 2024 • edited Loading

AryanBakliwal commented Dec 23, 2024

AryanBakliwal commented Jan 1, 2025

hangyan commented Jan 2, 2025

AryanBakliwal commented Jan 5, 2025

Captured packets

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AryanBakliwal Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

AryanBakliwal commented Jan 19, 2025

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

Choose a reason for hiding this comment

AryanBakliwal commented Dec 23, 2024 •

edited

Loading

AryanBakliwal Jan 16, 2025 •

edited

Loading