Open jshlbrd opened 5 years ago
gconnell
commented
5 years ago I think that's an excellent idea!
That said, I'd probably prefer if this were just integrated directly into the stenographer binary instead of being a side-car, maybe flag-controlled to determine whether it exposes via the current method? Or just another endpoint?
jshlbrd
commented
5 years ago Agreed 100%. I have something I should be able to share soon for your review!
jshlbrd
commented
5 years ago @gconnell feel free to take a look at this branch whenever you have time: https://github.com/jshlbrd/stenographer/tree/feature/grpc. The gRPC server is implemented as a goroutine if the user supplies a specific config dictionary in steno.conf (see DESIGN.md in the branch for details) and it's designed to never crash stenotype et al. I have some ideas on how to iterate on this in the future (e.g. there are some stenoread queries clients should probably never try to execute, like "tcp" without time modifiers), but for v1, this is fully tested.
Here's a summary of the file changes:
Aside from any potential bugs in the code, I'm curious how the design of this update looks (especially the protobuf).
aeppert
commented
5 years ago This looks solid to me. The only outlying question I can immediately think of, having implemented something similar in the past, just not with gRPC as we discussed @jshlbrd - how does it handle a zero-length PCAP? So if stenographer doesn't find anything. If I recall correctly, the issue there is Stenographer will send back just the PCAP header, which is normally fine, but can cause problems otherwise. If there is an issue with this, I believe I have the changes I made and can submit a follow-up or side-saddle patch to take care of this situation. Basically, it returns a 404 instead of a header plus zero length PCAP data.
jshlbrd
commented
5 years ago @aeppert Good question -- it currently doesn't. However, there are multiple cases when gRPC will return a 0-byte stream: https://github.com/jshlbrd/stenographer/blob/feature/grpc/rpc/rpc.go#L36-L38 https://github.com/jshlbrd/stenographer/blob/feature/grpc/rpc/rpc.go#L56-L60 https://github.com/jshlbrd/stenographer/blob/feature/grpc/rpc/rpc.go#L61-L65
If we'd like to handle the zero-length PCAP (IIRC this is when the resulting PCAP file is 24bytes, right?), then I'd suggest we do it similarly to those ("return nil"). Would probably make the most sense to do it here, right before the PCAP is chunked for streaming: https://github.com/jshlbrd/stenographer/blob/feature/grpc/rpc/rpc.go#L67
An alternate solution to returning nothing is to always return at least one message with a status code that describes what happened. This would alter the protobuf, but would not be difficult to add as long as we could define the status codes (e.g. successful extraction, successful extraction with truncation, general failure, zero-length PCAP, etc.)
For now, I've addressed this in this commit: https://github.com/jshlbrd/stenographer/commit/49dee7774cfb8cd609c4800f8b3083a83d4a7b8b
jshlbrd
commented
5 years ago PR #202 is submitted, please feel free to review, comment, and make suggestions as you have time. Thanks guys!
aeppert
commented
5 years ago @gconnell This looks solid to me. I would say this meets a solid MVP and a nice step forward. Thoughts?
Wanted to get your interest in adding gRPC support for securely retrieving PCAP from stenoread. I have a demo of this functionality, though there might be a better way to integrate it (currently I run a sidecar gRPC server that calls stenographer via the OS and streams extracted PCAP back to the client), would be happy to PR. Here's a post on Medium that describes how we could achieve this without altering any existing utilities. Thanks!