Skip to content
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.

Sign up for GitHub

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

Add bidirectional packet capture #6882

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Add bidirectional packet capture #6882

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
875dde4
add bidirection flag
AryanBakliwal Dec 22, 2024
1307c17
add bidirectional capture
AryanBakliwal Dec 23, 2024
25d581b
Bump golang.org/x/net from 0.32.0 to 0.33.0 in the golang-org-x group…
dependabot[bot] Dec 24, 2024
60d3e7e
Bump github.com/onsi/ginkgo/v2 from 2.22.0 to 2.22.1 in the ginkgo gr…
dependabot[bot] Dec 24, 2024
79a26c0
update code and tests
AryanBakliwal Jan 1, 2025
6e5898c
Merge branch 'main' into bidirection
AryanBakliwal Jan 5, 2025
e5ef842
add enum, tests and docs
AryanBakliwal Jan 5, 2025
7eb34de
refactor and add e2e test
AryanBakliwal Jan 19, 2025
5d04e5d
Merge branch 'main' into bidirection
AryanBakliwal Jan 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion build/charts/antrea/crds/packetcapture.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,10 @@ spec:
type: integer
minimum: 1
maximum: 65535

direction:
type: string
enum: ["SourceToDestination", "DestinationToSource", "Both"]
default: "SourceToDestination"
timeout:
type: integer
minimum: 1
Expand Down
5 changes: 4 additions & 1 deletion build/yamls/antrea-aks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3035,7 +3035,10 @@ spec:
type: integer
minimum: 1
maximum: 65535

direction:
type: string
enum: ["SourceToDestination", "DestinationToSource", "Both"]
default: "SourceToDestination"
timeout:
type: integer
minimum: 1
Expand Down
5 changes: 4 additions & 1 deletion build/yamls/antrea-crds.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3008,7 +3008,10 @@ spec:
type: integer
minimum: 1
maximum: 65535

direction:
type: string
enum: ["SourceToDestination", "DestinationToSource", "Both"]
default: "SourceToDestination"
timeout:
type: integer
minimum: 1
Expand Down
5 changes: 4 additions & 1 deletion build/yamls/antrea-eks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3035,7 +3035,10 @@ spec:
type: integer
minimum: 1
maximum: 65535

direction:
type: string
enum: ["SourceToDestination", "DestinationToSource", "Both"]
default: "SourceToDestination"
timeout:
type: integer
minimum: 1
Expand Down
5 changes: 4 additions & 1 deletion build/yamls/antrea-gke.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3035,7 +3035,10 @@ spec:
type: integer
minimum: 1
maximum: 65535

direction:
type: string
enum: ["SourceToDestination", "DestinationToSource", "Both"]
default: "SourceToDestination"
timeout:
type: integer
minimum: 1
Expand Down
5 changes: 4 additions & 1 deletion build/yamls/antrea-ipsec.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3035,7 +3035,10 @@ spec:
type: integer
minimum: 1
maximum: 65535

direction:
type: string
enum: ["SourceToDestination", "DestinationToSource", "Both"]
default: "SourceToDestination"
timeout:
type: integer
minimum: 1
Expand Down
5 changes: 4 additions & 1 deletion build/yamls/antrea.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3035,7 +3035,10 @@ spec:
type: integer
minimum: 1
maximum: 65535

direction:
type: string
enum: ["SourceToDestination", "DestinationToSource", "Both"]
default: "SourceToDestination"
AryanBakliwal marked this conversation as resolved.
Show resolved Hide resolved
timeout:
type: integer
minimum: 1
Expand Down
3 changes: 3 additions & 0 deletions docs/packetcapture-guide.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ the target traffic flow:
* Destination Pod, or IP address
* Transport protocol (TCP/UDP/ICMP)
* Transport ports
* Direction (SourceToDestination/DestinationToSource/Both)

You can start a new packet capture by creating a `PacketCapture` CR. An optional `fileServer`
field can be specified to store the generated packets file. Before that,
Expand Down Expand Up @@ -74,6 +75,8 @@ spec:
pod:
namespace: default
name: backend
# Available options for direction: `SourceToDestination` (default), `DestinationToSource` or `Both`.
direction: SourceToDestination # optional to specify
packet:
ipFamily: IPv4
protocol: TCP # support arbitrary number values and string values in [TCP,UDP,ICMP] (case insensitive)
Expand Down
191 changes: 167 additions & 24 deletions pkg/agent/packetcapture/capture/bpf.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

if direction == crdv1alpha1.CaptureDirectionBoth {
if isSrc && (srcPort > 0 && dstPort == 0) {
return size - instlen - 3
} else if !isSrc {
return size - instlen - 3
}
}
return 0
}

func calculateSkipFalse(size, instlen uint8, srcPort, dstPort uint16, direction crdv1alpha1.CaptureDirection) uint8 {
if direction == crdv1alpha1.CaptureDirectionBoth {
if srcPort > 0 && dstPort > 0 {
return size - instlen - 13
} else if srcPort > 0 || dstPort > 0 {
return size - instlen - 11
} else {
return size - instlen - 6
}
}
return size - instlen - 2
}

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}
}
}
}

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

// compilePacketFilter compiles the CRD spec to bpf instructions. For now, we only focus on
// ipv4 traffic. Compared to the raw BPF filter supported by libpcap, we only need to support
// limited use cases, so an expression parser is not needed.
func compilePacketFilter(packetSpec *crdv1alpha1.Packet, srcIP, dstIP net.IP) []bpf.Instruction {
size := uint8(calculateInstructionsSize(packetSpec))
func compilePacketFilter(packetSpec *crdv1alpha1.Packet, srcIP, dstIP net.IP, direction crdv1alpha1.CaptureDirection) []bpf.Instruction {
size := uint8(calculateInstructionsSize(packetSpec, direction))

// ipv4 check
inst := []bpf.Instruction{loadEtherKind}
Expand All @@ -101,20 +157,8 @@ func compilePacketFilter(packetSpec *crdv1alpha1.Packet, srcIP, dstIP net.IP) []
}
}

// source ip
if srcIP != nil {
inst = append(inst, loadIPv4SourceAddress)
addrVal := binary.BigEndian.Uint32(srcIP[len(srcIP)-4:])
// 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, bpf.JumpIf{Cond: bpf.JumpEqual, Val: addrVal, SkipTrue: 0, SkipFalse: size - uint8(len(inst)) - 2})

}
// dst ip
if dstIP != nil {
inst = append(inst, loadIPv4DestinationAddress)
addrVal := binary.BigEndian.Uint32(dstIP[len(dstIP)-4:])
inst = append(inst, bpf.JumpIf{Cond: bpf.JumpEqual, Val: addrVal, SkipTrue: 0, SkipFalse: size - uint8(len(inst)) - 2})
}
srcAddrVal := binary.BigEndian.Uint32(srcIP[len(srcIP)-4:])
dstAddrVal := binary.BigEndian.Uint32(dstIP[len(dstIP)-4:])
AryanBakliwal marked this conversation as resolved.
Show resolved Hide resolved

// ports
var srcPort, dstPort uint16
Expand All @@ -134,18 +178,61 @@ func compilePacketFilter(packetSpec *crdv1alpha1.Packet, srcIP, dstIP net.IP) []
}
}

// source ip
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@AryanBakliwal AryanBakliwal Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

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 {
Comment on lines +182 to +188
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

inst = append(inst, loadIPv4DestinationAddress)
if direction == crdv1alpha1.CaptureDirectionBoth && (srcPort == 0 && dstPort == 0) {
inst = append(inst, compareIP(direction, size-uint8(len(inst))-3, size-uint8(len(inst))-2, false, srcAddrVal, dstAddrVal))
} else {
inst = append(inst, compareIP(direction, 0, size-uint8(len(inst))-2, false, srcAddrVal, dstAddrVal))
}
}

if srcPort > 0 || dstPort > 0 {
skipTrue := size - uint8(len(inst)) - 3
inst = append(inst, loadIPv4HeaderOffset(skipTrue)...)
if srcPort > 0 {
if (direction != crdv1alpha1.CaptureDirectionDestinationToSource && srcPort > 0) || (direction == crdv1alpha1.CaptureDirectionDestinationToSource && dstPort > 0) {
inst = append(inst, loadIPv4SourcePort)
inst = append(inst, bpf.JumpIf{Cond: bpf.JumpEqual, Val: uint32(srcPort), SkipTrue: 0, SkipFalse: size - uint8(len(inst)) - 2})
inst = append(inst, comparePort(direction, calculateSkipTrue(size, uint8(len(inst)), direction, srcPort, dstPort, true), size-uint8(len(inst))-2, true, srcPort, dstPort))
}
if dstPort > 0 {

if (direction != crdv1alpha1.CaptureDirectionDestinationToSource && dstPort > 0) || (direction == crdv1alpha1.CaptureDirectionDestinationToSource && srcPort > 0) {
inst = append(inst, loadIPv4DestinationPort)
inst = append(inst, bpf.JumpIf{Cond: bpf.JumpEqual, Val: uint32(dstPort), SkipTrue: 0, SkipFalse: size - uint8(len(inst)) - 2})
inst = append(inst, comparePort(direction, calculateSkipTrue(size, uint8(len(inst)), direction, srcPort, dstPort, false), size-uint8(len(inst))-2, false, srcPort, dstPort))
}
}

if direction == crdv1alpha1.CaptureDirectionBoth {
// src ip (return traffic)
if dstIP != nil {
inst = append(inst, compareIP(direction, 0, size-uint8(len(inst))-2, false, srcAddrVal, dstAddrVal))
}

// dst ip (return traffic)
if srcIP != nil {
inst = append(inst, loadIPv4DestinationAddress)
inst = append(inst, compareIP(direction, 0, size-uint8(len(inst))-2, true, srcAddrVal, dstAddrVal))
}

// return traffic ports
if srcPort > 0 || dstPort > 0 {
skipTrue := size - uint8(len(inst)) - 3
inst = append(inst, loadIPv4HeaderOffset(skipTrue)...)
if dstPort > 0 {
inst = append(inst, loadIPv4SourcePort)
inst = append(inst, bpf.JumpIf{Cond: bpf.JumpEqual, Val: uint32(dstPort), SkipTrue: 0, SkipFalse: size - uint8(len(inst)) - 2})
}
if srcPort > 0 {
inst = append(inst, loadIPv4DestinationPort)
inst = append(inst, bpf.JumpIf{Cond: bpf.JumpEqual, Val: uint32(srcPort), SkipTrue: 0, SkipFalse: size - uint8(len(inst)) - 2})
}
}
}

// return
Expand All @@ -169,16 +256,47 @@ func compilePacketFilter(packetSpec *crdv1alpha1.Packet, srcIP, dstIP net.IP) []
// (006) ld [30] # Load 4B at 30 (dest address)
// (007) jeq #0x7f000001 jt 8 jf 16 # If bytes match(127.0.0.1), goto #8, else #16
// (008) ldh [20] # Load 2B at 20 (13b Fragment Offset)
// (009) jset #0x1fff jt 16 jf 10 # Use 0x1fff as a mask for fragment offset; If fragment offset != 0, #10, else #16
// (009) jset #0x1fff jt 16 jf 10 # Use 0x1fff as a mask for fragment offset; If fragment offset != 0, #10, else #16
// (010) ldxb 4*([14]&0xf) # x = IP header length
// (011) ldh [x + 14] # Load 2B at x+14 (TCP Source Port)
// (012) jeq #0x7b jt 13 jf 16 # TCP Source Port: If 123, goto #13, else #16
// (012) jeq #0x7b jt 13 jf 16 # TCP Source Port: If 123, goto #13, else #16
// (013) ldh [x + 16] # Load 2B at x+16 (TCP dst port)
// (014) jeq #0x7c jt 15 jf 16 # TCP dst port: If 123, goto $15, else #16
// (014) jeq #0x7c jt 15 jf 16 # TCP dst port: If 123, goto $15, else #16
// (015) ret #262144 # MATCH
// (016) ret #0 # NOMATCH

func calculateInstructionsSize(packet *crdv1alpha1.Packet) int {
// When capturing return traffic also (i.e., both src -> dst and dst -> src), the filter might look like this:
// 'ip proto 6 and ((src host 10.244.1.2 and dst host 10.244.1.3 and src port 123 and dst port 124) or (src host 10.244.1.3 and dst host 10.244.1.2 and src port 124 and dst port 123))'
// And using `tcpdump -i <device> '<filter>' -d` will generate the following BPF instructions:
// (000) ldh [12] # Load 2B at 12 (Ethertype)
// (001) jeq #0x800 jt 2 jf 26 # Ethertype: If IPv4, goto #2, else #26
// (002) ldb [23] # Load 1B at 23 (IPv4 Protocol)
// (003) jeq #0x6 jt 4 jf 26 # IPv4 Protocol: If TCP, goto #4, #26
// (004) ld [26] # Load 4B at 26 (source address)
// (005) jeq #0xaf40102 jt 6 jf 15 # If bytes match(10.244.0.2), goto #6, else #15
// (006) ld [30] # Load 4B at 30 (dest address)
// (007) jeq #0xaf40103 jt 8 jf 26 # If bytes match(10.244.0.3), goto #8, else #26
// (008) ldh [20] # Load 2B at 20 (13b Fragment Offset)
// (009) jset #0x1fff jt 26 jf 10 # Use 0x1fff as a mask for fragment offset; If fragment offset != 0, #10, else #26
// (010) ldxb 4*([14]&0xf) # x = IP header length
// (011) ldh [x + 14] # Load 2B at x+14 (TCP Source Port)
// (012) jeq #0x7b jt 13 jf 26 # TCP Source Port: If 123, goto #13, else #26
// (013) ldh [x + 16] # Load 2B at x+16 (TCP dst port)
// (014) jeq #0x7c jt 25 jf 26 # TCP dst port: If 123, goto #25, else #26
// (015) jeq #0xaf40103 jt 16 jf 26 # If bytes match(10.244.0.3), goto #16, else #26
// (016) ld [30] # Load 4B at 30 (return traffic dest address)
// (017) jeq #0xaf40102 jt 18 jf 26 # If bytes match(10.244.0.2), goto #18, else #26
// (018) ldh [20] # Load 2B at 20 (13b Fragment Offset)
// (019) jset #0x1fff jt 26 jf 20 # Use 0x1fff as a mask for fragment offset; If fragment offset != 0, #20, else #26
// (020) ldxb 4*([14]&0xf) # x = IP header length
// (021) ldh [x + 14] # Load 2B at x+14 (TCP Source Port)
// (022) jeq #0x7c jt 23 jf 26 # TCP Source Port: If 124, goto #23, else #26
// (023) ldh [x + 16] # Load 2B at x+16 (TCP dst port)
// (024) jeq #0x7b jt 25 jf 26 # TCP dst port: If 123, goto #25, else #26
// (025) ret #262144 # MATCH
// (026) ret #0 # NOMATCH

func calculateInstructionsSize(packet *crdv1alpha1.Packet, direction crdv1alpha1.CaptureDirection) int {
count := 0
// load ethertype
count++
Expand Down Expand Up @@ -214,6 +332,31 @@ func calculateInstructionsSize(packet *crdv1alpha1.Packet) int {
// src and dst ip
count += 4

if direction == crdv1alpha1.CaptureDirectionBoth {
count += 3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


transPort := packet.TransportHeader
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
}
}
Comment on lines +338 to +357
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// ret command
count += 2
return count
Expand Down
Loading