NETOBSERV-1471 gRPC export for packet capture #291

jpinsonneau · 2024-03-06T17:10:36Z

Description

Add gRPC export capability for packet capture.
Since I moved flow grpc to flowgrpc package, we will need to update FLP at the same time

I also took the opportunity to reuse the target host / port from flow config since it doesn't make sense to have both here.

Dependencies

n/a

Checklist

If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.

jotak · 2024-03-11T16:54:50Z

pkg/agent/config.go

@@ -163,8 +164,6 @@ type Config struct {
 	// PCAFilters set the filters to determine packets to filter using Packet Capture Agent (PCA). It is a comma separated set.
 	// The format is [protocol], [port number] Example: PCA_FILTER = "tcp,80". Currently, we support 'tcp','udp','sctp' for protocol.
 	PCAFilters string `env:"PCA_FILTER"`
-	// PCAServerPort is the port PCA Server starts at, when ENABLE_PCA variable is set to true.
-	PCAServerPort int `env:"PCA_SERVER_PORT" envDefault:"9990"`


I remember we had this discussion with Shachee previously, that reusing the FLOWS_TARGET_PORT env for pcap was misleading, and for two reasons: first because it has "FLOWS" in the name, and second because in the case of the TCP impl, if I remember correctly, this isn't a target port (ie. a remote host port) but it's a server port, uses to listen for clients who want to poll data. So that's why we ended up with this new env.

Well in that case we can remove both flows and target mentions. The final output will be:

// Host is the host name or IP of the Flow collector Host string `env:"HOST"` // Port is the port of the Flow collector Port int `env:"PORT"`

There is no point keeping these separated unless we plan to have both packets + flows export at the same time. WDYT ?

FYI I addressed this in that way in:
8cbecaf
3e86b43

idk it still bother me to mix server and client config into a single configuration ; you kept "target" which IMO is still misleading for a server configuration, it really looks like we are configuring a client. idk if it's just me, @msherif1234 what do you think? I defer to you :-)

well if we get rid of pcap w/ tcp , hence we have no server mode anyway, then I guess it answers the question

As mention above I changed the config to get rid of these mentions. That's more generic for future implementations.
On top of that I removed tcp export for packet capture.

Let me know if the current state match your expectations

Well, sorry but, now that it's clearly only a client connection there's no reason to remove "target" from the names? It was inconsistent if we were mixing client & server, but now that we don't, it's fine and more precise than just "host" / "port"
Anyway I don't want to block this PR indefinitely, we can do that later

codecov · 2024-03-12T11:48:07Z

Codecov Report

Attention: Patch coverage is 38.57143% with 172 lines in your changes are missing coverage. Please review.

Project coverage is 34.04%. Comparing base (6e00a92) to head (df212a7).

Files	Patch %	Lines
pkg/exporter/grpc_packets.go	0.00%	49 Missing ⚠️
pkg/pbpacket/packet.pb.go	52.08%	44 Missing and 2 partials ⚠️
pkg/exporter/packets_proto.go	0.00%	21 Missing ⚠️
pkg/agent/packets_agent.go	0.00%	16 Missing ⚠️
pkg/agent/config.go	8.33%	8 Missing and 3 partials ⚠️
pkg/pbpacket/packet_grpc.pb.go	70.00%	7 Missing and 2 partials ⚠️
pkg/grpc/packet/server.go	73.33%	6 Missing and 2 partials ⚠️
pkg/agent/agent.go	30.00%	7 Missing ⚠️
pkg/grpc/packet/client.go	68.75%	4 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #291      +/-   ##
==========================================
+ Coverage   32.79%   34.04%   +1.25%     
==========================================
  Files          41       47       +6     
  Lines        3653     3836     +183     
==========================================
+ Hits         1198     1306     +108     
- Misses       2379     2444      +65     
- Partials       76       86      +10

Flag	Coverage Δ
unittests	`34.04% <38.57%> (+1.25%)`	⬆️

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

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

jpinsonneau · 2024-03-13T13:37:49Z

/retest

examples/packetcapture-dump/README.md

pkg/agent/config.go

pkg/agent/packets_agent.go

pkg/exporter/grpc_packets.go

pkg/exporter/tcp_packets.go

msherif1234 · 2024-03-13T17:23:02Z

pkg/exporter/grpc_packets.go

+						CaptureLength: len(packetStream),
+						Length:        len(packetStream),
+					}
+					err := writeGRPCPacket(captureInfo, packetStream, p.clientConn)


I wonder if we can define grpc as array of pcap and write all the record at once instead of writing every packet this will probably be more efficient WDYT?

I can give a try indeed. What would be the maximum acceptable total size here ?

A single packet can be huge so I may need to check about the next(s) before sending

u can make the max configurable like we do today with flows IIRC for flows its 10000 do we send the entire packet or its just max of 256 bytes ? we should set an upper limit in ebpf code, I can provide a patch for this

For the collector side, it doesn't make sense to get half of a packet here. It would require extra work to sync and avoid concurrency

We should keep this simple and write packets entirely; but allowing array of them with a reasonnable upper limit

are we ok to limit the packet header size from the ebpf to say 256 bytes max if the packet is fragmented or has encrypted data it has no value copy all of that to user space WDYT ?

As we discussed today, it's still interesting to capture the payload for non encrypted packets. Giving the ability to get only the firsts 256 bytes is still interesting if wireshark manages that seamlessly but in that case, flows already cover these informations.

msherif1234 · 2024-03-13T17:23:56Z

proto/packet.proto

+
+// The request message containing the Packet
+message Packet {
+  google.protobuf.Any pcap = 1;


if we decided to send multiple pcaps then we need to add repeated at the definition above

Can we keep that for a followup improvment ? That raise more questions about the total size of the message.

The goal here was just to implement gRPC for security reasons to productize CLI.

sure sounds good to me!!

proto/packet.proto

jpinsonneau · 2024-03-14T16:55:36Z

vendor/github.com/netobserv/flowlogs-pipeline/pkg/pipeline/ingest/ingest_grpc.go

@@ -9,7 +9,7 @@ import (
 	"github.com/netobserv/flowlogs-pipeline/pkg/operational"
 	"github.com/netobserv/flowlogs-pipeline/pkg/pipeline/utils"
 	"github.com/netobserv/netobserv-ebpf-agent/pkg/decode"
-	"github.com/netobserv/netobserv-ebpf-agent/pkg/grpc"
+	grpc "github.com/netobserv/netobserv-ebpf-agent/pkg/grpc/flow"


👆 that cross import will require FLP update

jpinsonneau · 2024-03-22T09:50:59Z

Rebased + removed tcp packet capture
6e65e79

msherif1234 · 2024-03-22T11:32:15Z

pkg/agent/config.go

+	// Deprecated FlowsTargetPort replaced by TargetPort
+	FlowsTargetPort int `env:"FLOWS_TARGET_PORT"`
+	// Deprecated PCAServerPort replaced by TargetPort
+	PCAServerPort int `env:"PCA_SERVER_PORT"`


if we have case like we have both agent and pca running at the same node reaching out to host but using different port with the above proposed config that seems impossible right ?
i.e with this new config changes we can run either agent or pca but not both at the same time as it was b4 this change ?

You can't have both flow + pca capture as it rely on ENABLE_PCA bool config
https://github.com/netobserv/netobserv-ebpf-agent/blob/main/cmd/netobserv-ebpf-agent.go#L56

msherif1234 · 2024-03-22T11:33:24Z

pkg/agent/config.go

+}
+
+func manageDeprecatedConfigs(cfg *Config) {
+	if len(cfg.FlowsTargetHost) > 0 {


nit: u are doing !=0 below so why not be consistent ?

Host is a string whereas Port is a number

i meant len() != 0 as oppose to len() > 0

ok sure, 64c9c2a

msherif1234 · 2024-03-22T11:38:16Z

pkg/exporter/grpc_packets.go

+	"google.golang.org/protobuf/types/known/anypb"
+
+	"github.com/netobserv/netobserv-ebpf-agent/pkg/flow"
+	grpc "github.com/netobserv/netobserv-ebpf-agent/pkg/grpc/packet"


nit: this is the import order used in the agent
1- standard libs
2- netobserv pkg
3- external packages
this applies here and in all newly created files

Sure, 111c915

msherif1234 · 2024-03-22T11:50:24Z

pkg/exporter/grpc_packets.go

+				}
+				err := writeGRPCPacket(captureInfo, packetStream, p.clientConn)
+				if err != nil {
+					gplog.Error(err)


lets aggregate the errors here and return error in this function and use the return err from here in the caller not the err from StartGRPCPacketSend

I'm not sure to get your point here. Do you mean adding error as returned value of ExportGRPCPackets ?
I'm expecting to log with error each packet not sent here.

The approach is very similar to flows one here:
https://github.com/netobserv/netobserv-ebpf-agent/blob/main/pkg/exporter/grpc_proto.go#L59-L62

if u look at the caller it return error from startGRPC right but not reflecting errors because of export
so what I am suggesting return an error here and inside the loop just log and aggr the errors

is that what you had in mind ?

0ccc69b

msherif1234 · 2024-03-22T12:46:31Z

pkg/grpc/packet/grpc_test.go

+	"time"
+
+	"github.com/mariomac/guara/pkg/test"
+	"github.com/netobserv/netobserv-ebpf-agent/pkg/pbpacket"


nit: import order

done in beef4dd

msherif1234 · 2024-03-22T12:46:46Z

pkg/grpc/packet/server.go

+	"google.golang.org/grpc"
+	"google.golang.org/grpc/reflection"
+
+	"github.com/netobserv/netobserv-ebpf-agent/pkg/pbpacket"


done in beef4dd

msherif1234 · 2024-03-22T12:47:43Z

examples/packetcapture-dump/client/packetcapture-client.go

 	"os"
 	"time"
+
+	"github.com/google/gopacket/layers"


import order

done in beef4dd

msherif1234 · 2024-03-22T12:48:40Z

I am good with the changes few nit left other than that LGTM up to u fixing them
/LGTM

msherif1234 · 2024-03-22T13:54:30Z

/lgtm

msherif1234 · 2024-03-22T14:06:51Z

pls remember to update the operator
https://github.com/netobserv/network-observability-operator/blob/1cd8e7670e47ee248965c72e5fc9bc8709fd615f/controllers/ebpf/agent_controller.go#L35

msherif1234 · 2024-03-22T20:07:43Z

/approve

openshift-ci · 2024-03-22T20:07:49Z

[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

jpinsonneau · 2024-03-25T09:14:43Z

pls remember to update the operator https://github.com/netobserv/network-observability-operator/blob/1cd8e7670e47ee248965c72e5fc9bc8709fd615f/controllers/ebpf/agent_controller.go#L35

Here is the PR:
netobserv/network-observability-operator#600

As discussed here: netobserv#291 (comment) HOST and PORT are very generic and don't provide the meaning about what they are for; An uninformed user would probably think this would be for a server, of for the agent host itself, rather than a target of flow/packet collectors.

As discussed here: #291 (comment) HOST and PORT are very generic and don't provide the meaning about what they are for; An uninformed user would probably think this would be for a server, of for the agent host itself, rather than a target of flow/packet collectors.

jpinsonneau mentioned this pull request Mar 6, 2024

NETOBSERV-1471 capture using gRPC netobserv/network-observability-cli#3

Merged

jpinsonneau requested review from jotak, msherif1234 and OlivierCazade March 6, 2024 17:12

jotak reviewed Mar 11, 2024

View reviewed changes

jpinsonneau mentioned this pull request Mar 12, 2024

NETOBSERV-59 Add tcp flow stream exporter #224

Closed

11 tasks

jpinsonneau force-pushed the 1471 branch from 3b3c54e to eea42e8 Compare March 12, 2024 13:29

jpinsonneau closed this Mar 13, 2024

jpinsonneau reopened this Mar 13, 2024

msherif1234 reviewed Mar 13, 2024

View reviewed changes

jpinsonneau commented Mar 14, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase label Mar 15, 2024

jpinsonneau force-pushed the 1471 branch from fe58890 to 3e86b43 Compare March 15, 2024 13:08

openshift-merge-robot removed the needs-rebase label Mar 15, 2024

jpinsonneau added 8 commits March 22, 2024 10:24

grpc export for packet capture

b4514ac

add missing error check when write

85b7fb2

addressed feedback

a57a6dc

update pb doc + Makefile

7a12ac3

allow deprecated configs

4e50302

remove target mentions

e78f7d6

deprecated config fixes

de25f24

removed tcp packet capture and updated example

6e65e79

jpinsonneau force-pushed the 1471 branch from b42f36c to 6e65e79 Compare March 22, 2024 09:50

msherif1234 reviewed Mar 22, 2024

View reviewed changes

jpinsonneau added 2 commits March 22, 2024 13:28

updated import order

111c915

different than zero

64c9c2a

msherif1234 reviewed Mar 22, 2024

View reviewed changes

jpinsonneau added 3 commits March 22, 2024 14:34

aggregate errors

0ccc69b

import order 2

beef4dd

misc return nill

df212a7

openshift-ci bot assigned msherif1234 Mar 22, 2024

openshift-ci bot added the lgtm label Mar 22, 2024

jotak approved these changes Mar 22, 2024

View reviewed changes

openshift-ci bot assigned jotak Mar 22, 2024

openshift-ci bot added the approved label Mar 22, 2024

openshift-merge-bot bot merged commit 08030df into netobserv:main Mar 22, 2024
11 checks passed

jpinsonneau mentioned this pull request Mar 25, 2024

NETOBSERV-1471 eBPF env renaming netobserv/network-observability-operator#600

Merged

10 tasks

This was referenced Mar 25, 2024

NETOBSERV-1471 update eBPF dep netobserv/flowlogs-pipeline#642

Merged

NETOBSERV-1471 update FLP dep #302

Merged

This was referenced Mar 25, 2024

Change HOST/PORT env to TARGET_HOST / TARGET_PORT #303

Merged

Change HOST/PORT env to TARGET_HOST / TARGET_PORT netobserv/network-observability-operator#601

Merged

NETOBSERV-1471 gRPC export for packet capture #291

NETOBSERV-1471 gRPC export for packet capture #291

Conversation

jpinsonneau commented Mar 6, 2024 • edited

Description

Dependencies

Checklist

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

codecov bot commented Mar 12, 2024 • edited

Codecov Report

jpinsonneau commented Mar 13, 2024

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

jpinsonneau commented Mar 22, 2024

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

Choose a reason for hiding this comment

msherif1234 commented Mar 22, 2024 • edited

msherif1234 commented Mar 22, 2024

msherif1234 commented Mar 22, 2024

msherif1234 commented Mar 22, 2024

openshift-ci bot commented Mar 22, 2024

jpinsonneau commented Mar 25, 2024

jpinsonneau commented Mar 6, 2024 •

edited

codecov bot commented Mar 12, 2024 •

edited

msherif1234 commented Mar 22, 2024 •

edited