NETOBSERV-1255 Adding Raw Packet export capability as Packet Capture Agent(PCA) #113

shach33 · 2023-04-19T18:58:42Z

This PR extends the agent to export raw packets through the network. We call it Packet Capture Agent(PCA).

This functionality is turned off by default. I have added 2 new environments to enable this.
set ENABLE_PCA=true to activate packet cpature.
set PCA_FILTER=[ipproto,port number] to indicate the filter for packet capture, e.g. export PCA_FILTER=tcp,22

The PCA_FILTER defaults to udp DNS(udp,53). If ENABLE_PCA is true, but filters are not specified, by default it captures DNS packets.

The captured packets are written to a socket in PCAP format.

eranra · 2023-04-20T11:12:05Z

@msherif1234, for awareness --- the idea behind this PR is to become the basis for both PANO (of course) but also for other Packet level network observability requirements, such as "selective" packet mirroring - functionality such as network tracing etc. So generalization and customization of the content are key ( hope this make sense to you )

codecov · 2023-04-24T12:51:39Z

Codecov Report

Patch coverage: 3.03% and project coverage change: -7.41% ⚠️

Comparison is base (7d31bf6) 39.56% compared to head (1f17d37) 32.16%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #113      +/-   ##
==========================================
- Coverage   39.56%   32.16%   -7.41%     
==========================================
  Files          31       36       +5     
  Lines        2391     2960     +569     
==========================================
+ Hits          946      952       +6     
- Misses       1388     1951     +563     
  Partials       57       57

Flag	Coverage Δ
unittests	`32.16% <3.03%> (-7.41%)`	⬇️

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

Files Changed	Coverage Δ
pkg/agent/packets_agent.go	`0.00% <0.00%> (ø)`
pkg/ebpf/tracer.go	`0.00% <0.00%> (ø)`
pkg/exporter/packets.go	`0.00% <0.00%> (ø)`
pkg/flow/packet_record.go	`0.00% <0.00%> (ø)`
pkg/flow/perfbuffer.go	`0.00% <0.00%> (ø)`
pkg/flow/tracer_perf.go	`0.00% <0.00%> (ø)`
pkg/agent/agent.go	`39.11% <58.06%> (+1.26%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

msherif1234

I took initial look
I see ur created a parallel agent instead of extending the current one after u check the new config knob can u explain the reasoning ?

bpf/flows.c

pkg/agent/ip.go

shach33 · 2023-05-16T05:29:33Z

I took initial look I see ur created a parallel agent instead of extending the current one after u check the new config knob can u explain the reasoning ?

The current agent works at the level of flows and flow metrics. With PANO, we wish to use a 'packet' as the unit of communication. The struct elements of flow metrics are no longer required. Hope this helps.

msherif1234 · 2023-05-16T10:39:48Z

I took initial look I see ur created a parallel agent instead of extending the current one after u check the new config knob can u explain the reasoning ?

The current agent works at the level of flows and flow metrics. With PANO, we wish to use a 'packet' as the unit of communication. The struct elements of flow metrics are no longer required. Hope this helps.

yeah I am aware of that as u know I went through the same trouble when I prototyped TCPlife span hook but was able to refactor the current agent so it can handle both probably something we need to poll the team on @jotak @eranra @praveingk wdyt ? will be great if we somehow abstract support to perf map as we have other use cases as well that wanted to use it like the one I mentioned above and more will come and will be nice to share this so all hooks need to generate perf event will reuse

memodi · 2023-05-17T16:37:58Z

/ok-to-test

dushyantbehl

@shach33 can you also remove the temp1.pcap file which I assume got pushed along with your commits.

bpf/flows.c

pkg/ebpf/tracer.go

pkg/exporter/ipfix.go

dushyantbehl · 2023-05-24T11:45:13Z

pkg/flow/packet_record.go

+}
+
+// ReadPacketFrom reads a PacketRecord from a binary source, in LittleEndian order
+func ReadRawPacketFrom(reader io.Reader) (*PacketRecord, error) {


Can you rename this to what it does ReadRawPacketFromBuffer or just ReadRawPacket

Updated to ReadRawPacket

bpf/flows.c

dushyantbehl · 2023-05-24T11:54:10Z

I belive yes we can extract out common code and enable one or both functionalities in the agents rather than extending the code which can create code duplication.

shach33 · 2023-05-31T09:17:23Z

Performed first level refactoring.

bpf/flows.c

bpf/flow.h

bpf/flows.c

pkg/agent/agent.go

dushyantbehl · 2023-07-19T06:24:51Z

This PR needs a code review. Waiting for @shach33 to commit final version before providing the review

jotak · 2023-07-25T09:37:40Z

.gitignore

@@ -4,3 +4,5 @@
 bin/
 e2e-logs
 ebpf-agent.tar
+*.o


this is unclear to me why we would ignore .o files, I think we shouldn't?

I believe its a good practice to not include object files in commits. We cant get their diffs, they always have merge problems. But if thats how we do things. I will revert.

jotak · 2023-07-25T09:50:27Z

cmd/netobserv-ebpf-agent.go

-	if err != nil {
-		logrus.WithError(err).Fatal("can't instantiate NetObserv eBPF Agent")
-	}
+	if config.EnablePCA {


so this means the agent is started either for flow logs or for PCA, but cannot do both at the same time, right?
Do you see a technical limitation for doing so, or is that just a choice and we could change that any time?

Yes. The flows and packets agent are disparate and only one is active at a time. The metric of transfer in flows agent is a flow struct, while with PCA is a packet. We could merge them but that would add multiple unnecessary if checks at every step.

If the team deems this to be a better way to go, I can later create another PR with the merge. As of now, the code is cleanly separated between packet capture and flows agent.

Do you think if we are going to keep agent either PCA or Flow logs we should specify that make a single environment like AGENT_TYPE which can be PCA or FlowLogs

We can discuss this in our calls and make a decision then. In this PR, I would suggest to let it stay the way it is.

sounds good

jotak · 2023-08-23T15:43:29Z

Performed Resource analysis on the PR with 6 different use cases.

1. base netobserv:main

2. PR with PCA disabled

3. PR with PCA enabled - invalid filter

4. PR with PCA enabled - filter tcp,80

5. PR with PCA enabled - filter udp,53

6. PR with PCA enabled - filter tcp,443

@shach33 if I understand correctly your perf measurements, the current ebpf agent (without your PR) consumes twice as much memory than with your PR with PCA disabled? (1=~280MB vs 2=~140MB). This would be good news but is quite unexpected, how can we explain that? Or it could be a wrong measurement?

jotak · 2023-08-23T16:32:39Z

For info, I did some tests on my cluster for memory usage: there's indeed an increase with this feature (even when disabled) , I guess essentially due to the map. On my cluster, that's around 10MB increase per pod. Since it is constant, the more loaded is the agent, the more negligible is this overhead.

shach33 · 2023-08-24T04:04:15Z

Kernel does not support BPF_F_NO_PREALLOC for LRU type maps.
cilium/ebpf#1072

msherif1234 · 2023-08-24T12:32:18Z

bpf/maps_definition.h

+//PerfEvent Array for Packet Payloads
+struct {
+    __uint(type, BPF_MAP_TYPE_PERF_EVENT_ARRAY);
+	__type(key, int);


pls use u32

msherif1234 · 2023-08-24T12:35:15Z

bpf/maps_definition.h

+	__type(key, __u32);
+	__type(value, __u32);
+} packet_record SEC(".maps");
+


BPF_F_NO_PREALLOC not avaliable for all map types

msherif1234 · 2023-08-24T12:41:39Z

bpf/pca.h

+        // Set flag's upper 32 bits with the size of the paylaod and the bpf_perf_event_output will 
+        // attach the specified amount of bytes from packet to the perf event
+        // https://github.com/xdp-project/xdp-tutorial/tree/9b25f0a039179aca1f66cba5492744d9f09662c1/tracing04-xdp-tcpdump       
+        flags |= (__u64)packetSize << 32;


isn't there a limit of how much hdr we can send with perf event ?

it seems there is no enforced limit by the kernel and if userspace can't hold it the event will be dropped

If the size is too big, the call to bpf_perf_event_output will fail and return -EFAULT.

msherif1234 · 2023-08-24T12:42:55Z

bpf/pca.h

+        meta.if_index = skb->ifindex;
+        meta.pkt_len = packetSize;
+        meta.timestamp = current_time;
+        if (bpf_perf_event_output(skb, &packet_record, flags, &meta, sizeof(meta))){


what is the point of checking the return code if either way u return ok ?

this was an oversight. Fixed it.

msherif1234 · 2023-08-24T12:43:55Z

bpf/pca.h

+        }
+        sourcePort = udp_header->source;
+        destPort = udp_header->dest;
+    }


this is not right , we can't assume if its not TCP its UDP, we have SCTP, ICMPv4 and ICMPv6 protocols support

Updated. Only checks for tcp/udp, for anything else, we return.

sctp will work very similar to ycp and udp unless we see no use case for it ? which isn't true because svc can use sctp protocol taht is why we added it

I added SCTP. ICMP packets dont really have port associated to them. So, it would require an update to the filter specifications, which I can do as part of an extension, but may be not with the current PR.

msherif1234 · 2023-08-24T12:46:06Z

bpf/pca.h

+    }
+
+    if (validate_pca_filter(ip->protocol, (void *)ip + sizeof(*ip), data_end )){
+        return attach_packet_payload(data, data_end, skb, current_time);


nit: u don't use current_time here so u can read it in attach_packet_payload and remove it from here

Sure. Fixed.

msherif1234 · 2023-08-24T12:47:23Z

bpf/pca.h

+    return export_packet_payload(skb);
+}
+
+#endif /* __PCA_H__ */


nit pls add line to the end

msherif1234 · 2023-08-24T12:50:04Z

bpf/pca.h

+    }
+
+    // Only IPv4 and IPv6 packets captured
+    if (eth->h_proto != bpf_htons(ETH_P_IP) && (eth->h_proto != bpf_htons(ETH_P_IPV6))) {


might be more efficient instruction wise to do

u16 ethType = bpf_ntohs(eth->h_proto); if (ethType != ETH_P_IP && ethType != ETH_P_IPV6) { }

msherif1234 · 2023-08-24T12:52:01Z

cmd/netobserv-ebpf-agent.go

+		canceler()
+	}()
+	return ctx
+}


why did u add this ? for standalone testing ?

This was to reuse the same code block for terminating both netobserv and PCA.

msherif1234 · 2023-08-24T12:54:49Z

cmd/netobserv-ebpf-agent.go

-	}
+	if config.EnablePCA {
+		if config.PCAFilters == "" {
+			logrus.Fatal("[PCA] NetObserv eBPF Agent instantiated without filters to identify packets. Please set environment variable PCA_FILTER.")


can't we treat that as capture everything instead of error ?

Should we treat this for capture everything? That might be extremely resource intensive. This was a design decision we made. This could be updated if the team deems this behavior correct.

wanted to share one of the PM customers asked use case is to do capture all (just learnt this now) cc'd @jpinsonneau

+1 it seems to be a use case now 🙃
Would it work if we capture everything expect our own netobserv traffic ? Let's say with unlimited resources

Updated to collect and export all packets if the filter (enviroment variable PCA_FILTER) is not set at all.
However if the filter is wrongly set, incorrect format/ non-supported protocols etc, then the agent doesnt export anything.

jotak

@shach33 : here's a suggestion as discussed on slack. This is to minimize the memory allocation when PCA isn't used.

jotak · 2023-08-24T13:13:13Z

pkg/ebpf/tracer.go

@@ -117,12 +124,19 @@ func NewFlowFetcher(cfg *FlowFetcherConfig) (*FlowFetcher, error) {
 		return nil, fmt.Errorf("rewriting BPF constants definition: %w", err)
 	}

+	//Deleting specs for PCA


Suggested change

//Deleting specs for PCA

//Deleting specs for PCA

// Always set pcaRecordsMap to the minimum in FlowFetcher - PCA and Flow Fetcher are mutually exclusive.

spec.Maps[pcaRecordsMap].MaxEntries = 1

pkg/ebpf/tracer.go

msherif1234 · 2023-08-24T13:53:47Z

bpf/pca.h

+        if (packetSize < skb->len){
+            packetSize = skb->len;
+            if (bpf_skb_pull_data(skb, skb->len)){
+                return TC_ACT_UNSPEC;


I think we come to this case for TCP DNS have u tried to capture DNS over TCP and measured the CPU usage ? u can simply do dig google.om +tcp to do DNS over TCP

I am not sure what is happening here, but when I experiment, the requests/response generated via dig google.com +tcp are also UDP packets.

no that is indeed tcp dns I have used it

Then, I am able to capture those.

msherif1234 · 2023-08-25T12:09:07Z

bpf/pca.h

+        u32 packetSize = (__u32)(data_end-data);
+
+        // Record the current time.
+        __u64 current_time = bpf_ktime_get_ns();


nit: we don't use __types in the agent code so probably need to stick to the same style

msherif1234 · 2023-08-25T12:09:33Z

bpf/pca.h

+        if (packetSize < skb->len){
+            packetSize = skb->len;
+            if (bpf_skb_pull_data(skb, skb->len)){
+                return TC_ACT_UNSPEC;


no that is indeed tcp dns I have used it

msherif1234 · 2023-08-25T12:10:03Z

bpf/pca.h

+        // Set flag's upper 32 bits with the size of the paylaod and the bpf_perf_event_output will 
+        // attach the specified amount of bytes from packet to the perf event
+        // https://github.com/xdp-project/xdp-tutorial/tree/9b25f0a039179aca1f66cba5492744d9f09662c1/tracing04-xdp-tcpdump       
+        flags |= (__u64)packetSize << 32;


it seems there is no enforced limit by the kernel and if userspace can't hold it the event will be dropped

msherif1234 · 2023-08-25T12:11:55Z

bpf/pca.h

+        }
+        sourcePort = udp_header->source;
+        destPort = udp_header->dest;
+    }


sctp will work very similar to ycp and udp unless we see no use case for it ? which isn't true because svc can use sctp protocol taht is why we added it

msherif1234 · 2023-08-25T12:14:32Z

pkg/agent/PacketsAgent.go

+	io.Closer
+	Register(iface ifaces.Interface) error
+
+	LookupAndDeleteMap() map[int][]*byte


you aren't using this interface ?

I am. This is the one being used to delete map.

msherif1234 · 2023-08-25T12:26:32Z

pkg/flow/packet_record.go

+	getLen := make([]byte, 4)
+	packetTimestamp := make([]byte, 8)
+	// Read IfIndex and discard it: To be used in other usecases
+	_ = binary.Read(reader, binary.LittleEndian, make([]byte, 4))


is there a way to show interface name with the collected flow, u already have if_index in thr perf hdr

iface, err := net.InterfaceByIndex(int(eventHdr.IfId)) ifcace.Name

PCA eventually only transmits a packet stream. How do you think we should include the interface information?

msherif1234 · 2023-08-25T12:28:14Z

pkg/flow/perfbuffer.go

+				logrus.WithField("packets", len(evictingEntries)).
+					Debug("evicting packets from userspace accounter after reaching cache max length")
+				c.evict(evictingEntries, out)
+			}


do u need the above logs won't some of this will flood the logs ?

It is a debug log. But removed it for now.

jotak

Thanks @shach33 , LGTM on my side, I'm leaving it in @msherif1234 / @dushyantbehl hands! :)

dushyantbehl · 2023-09-04T10:02:52Z

bpf/pca.h

+            destPort = sctp_header->dest;        
+    }
+    else 
+        return false;


nit: Please add {} here with else to keep the code structure same across all C code.

@shach33

dushyantbehl · 2023-09-04T10:04:12Z

bpf/pca.h

+    }
+    else 
+        return false;
+    if (sourcePort == bpf_htons(pca_port) || destPort == bpf_htons(pca_port)){


@shach33 nit: you can possibly do bpf_htons(pca_port) once.

dushyantbehl

Added 2 minor things for @shach33.
Let me know when they are done and I can add a lgtm here.

Leaving for @msherif1234 for taking a final view

jpinsonneau · 2023-09-14T14:47:35Z

@shach33 can we just rebase this ? then we are good to merge 😸
thanks !

msherif1234 · 2023-09-21T15:09:45Z

@shach33 why the changes in main is showing up in ur PR as new diffs ?

shach33 · 2023-09-22T09:14:23Z

@msherif1234 This is fixed now.

msherif1234 · 2023-09-22T09:40:14Z

/lgtm
/approve

openshift-ci · 2023-09-22T09:40:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msherif1234

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [msherif1234]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added the do-not-merge/work-in-progress label Apr 19, 2023

openshift-merge-robot added the needs-rebase label Apr 26, 2023

msherif1234 reviewed May 15, 2023

View reviewed changes

openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label May 17, 2023

github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label May 18, 2023

dushyantbehl self-requested a review May 23, 2023 17:52

shach33 changed the title ~~[WIP] Adding Raw Packet export capability for PANO~~ Adding Raw Packet export capability for PANO May 24, 2023

openshift-ci bot removed the do-not-merge/work-in-progress label May 24, 2023

dushyantbehl requested changes May 24, 2023

View reviewed changes

openshift-ci bot assigned dushyantbehl May 24, 2023

dushyantbehl reviewed Jun 26, 2023

View reviewed changes

bpf/flows.c Outdated Show resolved Hide resolved

dushyantbehl reviewed Jun 26, 2023

View reviewed changes

bpf/flow.h Outdated Show resolved Hide resolved

dushyantbehl reviewed Jun 26, 2023

View reviewed changes

bpf/flows.c Outdated Show resolved Hide resolved

dushyantbehl reviewed Jun 26, 2023

View reviewed changes

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

openshift-merge-robot added needs-rebase and removed needs-rebase labels Jun 28, 2023

openshift-merge-robot removed the needs-rebase label Jul 7, 2023

shach33 force-pushed the main branch from 7beb6a5 to 319e9d0 Compare July 14, 2023 07:15

openshift-merge-robot added the needs-rebase label Jul 17, 2023

shach33 changed the title ~~Adding Raw Packet export capability for PANO~~ NETOBSERV-349 Adding Raw Packet export capability as Packet Capture Agent(PCA) Jul 18, 2023

jotak reviewed Jul 25, 2023

View reviewed changes

msherif1234 reviewed Aug 24, 2023

View reviewed changes

jotak reviewed Aug 24, 2023

View reviewed changes

msherif1234 reviewed Aug 24, 2023

View reviewed changes

github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Aug 25, 2023

msherif1234 reviewed Aug 25, 2023

View reviewed changes

jotak previously approved these changes Aug 29, 2023

View reviewed changes

openshift-ci bot assigned jotak Aug 29, 2023

openshift-ci bot added the lgtm label Aug 29, 2023

dushyantbehl reviewed Sep 4, 2023

View reviewed changes

shach33 dismissed jotak’s stale review via e997578 September 5, 2023 05:19

openshift-ci bot removed the lgtm label Sep 5, 2023

openshift-merge-robot added needs-rebase and removed needs-rebase labels Sep 14, 2023

shach33 added 2 commits September 22, 2023 09:07

Packet Capture Agent added.

267eabb

PCA: Added vendors.

1f17d37

shach33 force-pushed the main branch from 1f1f0cc to 1f17d37 Compare September 22, 2023 09:09

openshift-ci bot assigned msherif1234 Sep 22, 2023

openshift-ci bot added the lgtm label Sep 22, 2023

openshift-ci bot added the approved label Sep 22, 2023

openshift-merge-robot merged commit 4297938 into netobserv:main Sep 22, 2023
8 checks passed

NETOBSERV-1255 Adding Raw Packet export capability as Packet Capture Agent(PCA) #113

NETOBSERV-1255 Adding Raw Packet export capability as Packet Capture Agent(PCA) #113

Conversation

shach33 commented Apr 19, 2023 • edited Loading

eranra commented Apr 20, 2023

codecov bot commented Apr 24, 2023 • edited Loading

Codecov Report

msherif1234 left a comment

Choose a reason for hiding this comment

shach33 commented May 16, 2023

msherif1234 commented May 16, 2023

memodi commented May 17, 2023

dushyantbehl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dushyantbehl commented May 24, 2023

shach33 commented May 31, 2023

dushyantbehl commented Jul 19, 2023 • edited Loading

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

shach33 Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jotak commented Aug 23, 2023 • edited Loading

jotak commented Aug 23, 2023

shach33 commented Aug 24, 2023 • edited Loading

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

msherif1234 Aug 24, 2023 • edited Loading

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

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

jotak 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

shach33 Aug 25, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jotak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shach33 commented Apr 19, 2023 •

edited

Loading

codecov bot commented Apr 24, 2023 •

edited

Loading

dushyantbehl commented Jul 19, 2023 •

edited

Loading

shach33 Aug 8, 2023 •

edited

Loading

jotak commented Aug 23, 2023 •

edited

Loading

shach33 commented Aug 24, 2023 •

edited

Loading

msherif1234 Aug 24, 2023 •

edited

Loading

shach33 Aug 25, 2023 •

edited

Loading

dushyantbehl left a comment •

edited

Loading