-
Notifications
You must be signed in to change notification settings - Fork 32
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.
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
NETOBSERV-1471 gRPC export for packet capture #291
Conversation
@@ -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"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well if we get rid of pcap w/ tcp , hence we have no server mode anyway, then I guess it answers the question
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
/retest |
pkg/exporter/grpc_packets.go
Outdated
CaptureLength: len(packetStream), | ||
Length: len(packetStream), | ||
} | ||
err := writeGRPCPacket(captureInfo, packetStream, p.clientConn) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
// The request message containing the Packet | ||
message Packet { | ||
google.protobuf.Any pcap = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we decided to send multiple pcaps then we need to add repeated
at the definition above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure sounds good to me!!
@@ -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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👆 that cross import will require FLP update
Rebased + removed tcp packet capture |
// Deprecated FlowsTargetPort replaced by TargetPort | ||
FlowsTargetPort int `env:"FLOWS_TARGET_PORT"` | ||
// Deprecated PCAServerPort replaced by TargetPort | ||
PCAServerPort int `env:"PCA_SERVER_PORT"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
pkg/agent/config.go
Outdated
} | ||
|
||
func manageDeprecatedConfigs(cfg *Config) { | ||
if len(cfg.FlowsTargetHost) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: u are doing !=0
below so why not be consistent ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Host is a string
whereas Port is a number
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i meant len() != 0 as oppose to len() > 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok sure, 64c9c2a
"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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, 111c915
pkg/exporter/grpc_packets.go
Outdated
} | ||
err := writeGRPCPacket(captureInfo, packetStream, p.clientConn) | ||
if err != nil { | ||
gplog.Error(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that what you had in mind ?
"time" | ||
|
||
"github.com/mariomac/guara/pkg/test" | ||
"github.com/netobserv/netobserv-ebpf-agent/pkg/pbpacket" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: import order
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in beef4dd
pkg/grpc/packet/server.go
Outdated
"google.golang.org/grpc" | ||
"google.golang.org/grpc/reflection" | ||
|
||
"github.com/netobserv/netobserv-ebpf-agent/pkg/pbpacket" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in beef4dd
"os" | ||
"time" | ||
|
||
"github.com/google/gopacket/layers" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import order
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in beef4dd
I am good with the changes few nit left other than that LGTM up to u fixing them |
/lgtm |
/approve |
[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:
Approvers can indicate their approval by writing |
Here is the PR: |
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.
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 timeI 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.