[POA-155] Capture client/server timeouts as well as Agent parsing errors #245
Conversation
| @@ -314,6 +333,32 @@ func (c *tcpStream) Accept(tcp *layers.TCP, _ gopacket.CaptureInfo, dir reassemb | |||
| } | |||
| } | |||
|
|
|||
| // One of the flows initiated a connection close request | |||
| if tcp.FIN { | |||
There was a problem hiding this comment.
I do think it is important to handle RST too, or at least leave a FIXME about abnormal termination.
If the server crashes hard (a kernel failure) or there is a load balancer problem directing traffic to the wrong server, then the client will get a RST back instead of a FIN. Of course, we might not be still around to see the traffic in that case.
The server getting a RST is a bit more obscure.
The comments here do not make it clear how we handle the FIN-ACK from the side that did not initiate the shutdown.
…e timeout case, and remove the message field from server and client timeouts
…o client and server shutdown metadata
| firstPacketSeen bool | ||
|
|
||
| // Indicates who initiated this flow (client or server) | ||
| initiator *tcpFlowInitiator |
There was a problem hiding this comment.
Consider using go-utils.optional here instead
| func (f *tcpFlow) TcpFlowInitiator() tcpFlowInitiator { | ||
| return *f.initiator | ||
| } |
There was a problem hiding this comment.
This is one of the reasons to worry about using *T instead of optional[T], the dereference here is unchecked.
(And might be the nil pointer derefs you are getting in unit tests? I didn't investigate.)
| // and no packets have yet arrived on the reverse flow | ||
| // this would indicate a timeout on the current side | ||
| if currFlow.TcpFlowInitiator() == TCP_FLOW_INITIATOR_CLIENT { | ||
| c.outChan <- currFlow.toPNT(ac.GetCaptureInfo().Timestamp, ac.GetCaptureInfo().Timestamp, akinet.ClientShutdowntMetadata{ |
There was a problem hiding this comment.
"Shutdownt" => "Shutdown"?
| // The current flow is the one that initiated the connection close request | ||
| // and no packets have yet arrived on the reverse flow | ||
| // this would indicate a timeout on the current side |
There was a problem hiding this comment.
I really thing we need to do the design work here of showing the different scenarios and listing out the possibilities to be sure we're covering these correctly.
Client shutdown:
client -> SYN
SYN-ACK <- server
client -> SYN
client -> HTTP request
ACK <- server
...
client -> FIN
FIN-ACK <- server
Are we called on every TCP frame, or only those with payloads? If every TCP frame, then wouldn't revFlow.FirstPacketSeen() be true? We probably need to set it only when there are bytes in the stream.
Server shutdown:
as above until
...
FIN <- server
client -> FIN-ACK
It seems to me that currFlow.TcpFlowInitiator() is not set, because we have not selected a parser.
These are subtle enough that we need unit tests for this code.
JIRA: https://postmanlabs.atlassian.net/browse/POA-155
Confluence: https://postmanlabs.atlassian.net/l/cp/EtJHwV3u
Changes
In this PR, we have added two sets of changes
Acceptfunction for this purpose. This function gets called on every packet captured fromAssembleWithContextfunction of tcpAssembly utility.TODO