Fix Issue #6976: Modified the logic to capture either source or destination traffic in PacketCapture CRD #6997

ifrah-ashraf · 2025-02-13T21:18:34Z

Description

This PR fixes Issue #6976 by ensuring that PacketCapture CRD properly validates source and destination separately.

Updated packetcapture.yaml schema to enforce oneOf validation.
Modified bpf.go to handle traffic capturing based on either source or destination.
Fixed packetcapture_controller.go logic to correctly process packet captures for either source or destination.

Source Pod traffic (egressing from a pod)

Destination Pod traffic (ingressing into a pod)

Note:
For destination pod traffic (ingress), I need to further verify whether externally sent traffic is properly captured at the destination. Initial tests with locally exposed pod service indicate that ingress traffic might be translated at the gateway, making it appear as if it's coming from another internal source pod. If any modifications are required in this regard, please let me know. Otherwise, I will update the e2e test suite accordingly to validate this scenario and share my findings.

antoninbas · 2025-02-14T21:58:13Z

build/charts/antrea/crds/packetcapture.yaml

+              oneOf:
                - properties:
                    source:
                      required: [pod]
+                  required: [source]
                - properties:
                    destination:
                      required: [pod]
+                  required: [destination]  


that doesn't seem right to me: if I define a PacketCapture with both .spec.source.pod and .spec.destination.pod, I believe validation will fail as both alternatives in the oneOf will match

I am also not sure I understand the rationale behind the change. I think the previous requirement (with the anyOf) as fine. What we are trying to ensure is that at least one one .spec.source.pod and .spec.destination.pod is provided. If at least one of them is provided, then of course at least one of .spec.source and .spec.destination is provided.

Thanks, @antoninbas, for your feedback!

I've updated the Packet Capture CRD to capture packets in the following scenarios:

Egress traffic – packets originating from a pod.

Ingress traffic – packets destined for a pod.

Pod-to-pod traffic – packets where at least one endpoint (source or destination) is a pod.

Let me know if any further improvements are needed!

antoninbas · 2025-02-14T22:01:08Z

pkg/agent/packetcapture/packetcapture_controller.go

+
+	var sourceIP, destIP net.IP
+	// Parse only one IP either source or destination
+	podIP, isSource, err := c.parseOneIP(ctx, pc)


both IPs can still be provided, so returning a single IP here is incorrect

it seems to me that the existing implementation of parseIPs is correct actually, and compatible with what we are trying to achieve with this PR

I've removed parseOneIP and kept the previous parseIPs logic.

Additionally, I made little changes in bpf.go to ensure that the instruction count
is correctly calculated based on the Pod CRD for all types of traffic.

… or dest pod traffic Signed-off-by: ifrah-ashraf <[email protected]>

…o-destination traffic Signed-off-by: ifrah-ashraf <[email protected]>

ifrah-ashraf · 2025-02-15T19:32:13Z

Hey @antoninbas, thanks for the feedback!

I've made the necessary changes to ensure correct packet capture and verified
that all conditions mentioned in issue #6976 are met.

Let me know if any further improvements are needed. Otherwise, I'll test it
locally and provide an update.

devc007 · 2025-02-17T16:10:27Z

build/charts/antrea/crds/packetcapture.yaml

+                        required: [pod]
+                  - properties:
+                      destination:
+                        required: [pod] 


@ifrah-ashraf
what if both src and dst is provided? Then doesn't you think then both src and dst must be pod?

Hey @devc007 , if both (source and dest) is specified then it will work as it was working before but either of them must be a pod as to store the pcapng file one pod must be specified and yeah if both source and destination are defined then by default it will store the file at source pod.

I hope it is clear now.

Thanks for clarification @ifrah-ashraf but i am still trying to understand the things you have pointed out. And thanks to explain me why one of them must be a pod.

I don't think I understand why we don't just have this:

required: - captureConfig anyOf: - properties: source: required: [pod] - properties: destination: required: [pod]

wouldn't that work?

antoninbas · 2025-02-24T18:59:16Z

build/charts/antrea/crds/packetcapture.yaml

+                        required: [pod]
+                  - properties:
+                      destination:
+                        required: [pod] 


I don't think I understand why we don't just have this:

required: - captureConfig anyOf: - properties: source: required: [pod] - properties: destination: required: [pod]

wouldn't that work?

antoninbas · 2025-02-24T18:59:46Z

pkg/agent/packetcapture/capture/bpf.go

@@ -76,7 +76,16 @@ func compareProtocol(protocol uint32, skipTrue, skipFalse uint8) bpf.Instruction
 // 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))
+	//size := uint8(calculateInstructionsSize(packetSpec))


please remove such commented out lines when you submit a PR for review

antoninbas · 2025-02-24T19:00:46Z

pkg/agent/packetcapture/capture/bpf.go

@@ -212,8 +221,16 @@ func calculateInstructionsSize(packet *crdv1alpha1.Packet) int {
 		}
 	}
 	// src and dst ip
-	count += 4
+	//count += 4


antoninbas · 2025-02-24T19:01:07Z

pkg/agent/packetcapture/capture/bpf.go


+	// generate traffic for source pod (egress)


I don't understand the new comments here. What does "generate traffic" mean here?

antoninbas · 2025-02-24T19:01:43Z

pkg/agent/packetcapture/packetcapture_controller.go

 	srcIP, dstIP, err := c.parseIPs(ctx, pc)
 	if err != nil {
 		return false, err
 	}

+	/* var sourceIP, destIP net.IP


again, remove all the commented out code that is not needed

antoninbas · 2025-02-24T19:01:50Z

pkg/agent/packetcapture/packetcapture_controller.go

@@ -447,11 +447,26 @@ func (c *Controller) performCapture(
 	file afero.File,
 	device string,
 ) (bool, error) {
+
+	// here is the IP parsing when both source and destination pod is specified


not a useful comment

antoninbas · 2025-02-24T19:02:02Z

pkg/agent/packetcapture/packetcapture_controller.go

@@ -447,11 +447,26 @@ func (c *Controller) performCapture(
 	file afero.File,
 	device string,
 ) (bool, error) {
+


don't add an empty line at the beginning of functions

antoninbas reviewed Feb 14, 2025

View reviewed changes

ifrah-ashraf added 2 commits February 16, 2025 00:33

Fix Issue antrea-io#6976: Modified the logic to capture either source…

6c095c6

… or dest pod traffic Signed-off-by: ifrah-ashraf <[email protected]>

modify packetcapture crd to capture all ingress , egress and source-t…

0710ce5

…o-destination traffic Signed-off-by: ifrah-ashraf <[email protected]>

ifrah-ashraf force-pushed the ifrah-fix-issue--6976 branch from 088808c to 0710ce5 Compare February 15, 2025 19:04

devc007 reviewed Feb 17, 2025

View reviewed changes

antoninbas reviewed Feb 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Issue #6976: Modified the logic to capture either source or destination traffic in PacketCapture CRD #6997

Fix Issue #6976: Modified the logic to capture either source or destination traffic in PacketCapture CRD #6997

ifrah-ashraf commented Feb 13, 2025

antoninbas Feb 14, 2025

ifrah-ashraf Feb 15, 2025

antoninbas Feb 14, 2025

ifrah-ashraf Feb 15, 2025

ifrah-ashraf commented Feb 15, 2025

devc007 Feb 17, 2025

ifrah-ashraf Feb 17, 2025

devc007 Feb 17, 2025

antoninbas Feb 24, 2025

antoninbas Feb 24, 2025

antoninbas Feb 24, 2025

antoninbas Feb 24, 2025

antoninbas Feb 24, 2025

antoninbas Feb 24, 2025

antoninbas Feb 24, 2025

antoninbas Feb 24, 2025

Fix Issue #6976: Modified the logic to capture either source or destination traffic in PacketCapture CRD #6997

Are you sure you want to change the base?

Fix Issue #6976: Modified the logic to capture either source or destination traffic in PacketCapture CRD #6997

Conversation

ifrah-ashraf commented Feb 13, 2025

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

ifrah-ashraf commented Feb 15, 2025

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

Choose a reason for hiding this comment