Skip to content
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

http: Report unmatched requests or responses #6794

Merged
merged 4 commits into from Apr 17, 2018

Conversation

Projects
None yet
4 participants
@adriansr
Copy link
Member

commented Apr 6, 2018

Until now, unmatched requests or responses are silently discarded by the http plugin.

This patch makes two changes to this behavior:

  • When an HTTP response is received without a matching request, an event with Error status is generated.

  • When an HTTP stream times out, any pending request or response that has received complete headers is also reported with Error status.

@@ -50,6 +50,13 @@ type UDPPlugin interface {
ParseUDP(pkt *Packet)
}

type ExpirationAwareTCPPlugin interface {

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 6, 2018

exported type ExpirationAwareTCPPlugin should have comment or be unexported

@adriansr adriansr force-pushed the adriansr:feature/http_timeout branch from ff77555 to add0f1f Apr 6, 2018

@adriansr

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2018

Partially addressing #257, as it only works with HTTP.

@adriansr adriansr force-pushed the adriansr:feature/http_timeout branch from add0f1f to 63cd215 Apr 9, 2018

@andrewkroh andrewkroh requested review from urso and andrewkroh Apr 9, 2018

func (http *httpPlugin) correlate(conn *httpConnectionData) {
// drop responses with missing requests
if conn.requests.empty() {
func (http *httpPlugin) correlate(conn *httpConnectionData, flush bool) {

This comment has been minimized.

Copy link
@urso

urso Apr 10, 2018

Collaborator

Will the correlate always report all active messages? Depending on where packetbeat is running (passive sniffing), a request might be received after the corresponding resource. This make correlation quite tricky in general.

Is there a chance of flushRequests (at the end of the function) is prematurely flushing events? In which situation will the flush flag be set? Always on shutdown?

How about extracting the local functions into methods on the httpPlugin. The correlate can re-use them and we can remove the flush flag from correlate. Whenever we would set the flush flag, we would call flushAll, which calls flushResponses and flushRequests.

}
}

// drop responses with missing requests

This comment has been minimized.

Copy link
@urso

urso Apr 10, 2018

Collaborator

s/drop/publish/

@urso

This comment has been minimized.

Copy link
Collaborator

commented Apr 10, 2018

LGTM. Let's pull change the local closures to httpPlugin methods and get rid of the flush flag on correlate.

@adriansr adriansr force-pushed the adriansr:feature/http_timeout branch 3 times, most recently from a20195b to f49c939 Apr 11, 2018

@adriansr

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2018

Thanks @urso, I've addressed your comment.

Also, I've included a fix for a race condition when expired streams are notified. libbeat's common.Cache uses a separate goroutine for the janitor, and removal notifications from the cache are reported from this goroutine. I've refactored it so that plugins are notified about this event from the same goroutine that handles the packet processing. Give me your opinion on it, it's all on the first commit, TCP plugins can be notified of stream expirations

@andrewkroh
Copy link
Member

left a comment

LGTM

@@ -415,14 +415,31 @@ func (http *httpPlugin) handleHTTP(
}
}

func (http *httpPlugin) flushResponses(conn *httpConnectionData) {
for !conn.responses.empty() {
debugf("Response from unknown transaction. Reporting error.")

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Apr 12, 2018

Member

If there's any identifying information available about the flow it would likely be beneficial to include a few details in the log. The same in flushRequests.

This comment has been minimized.

Copy link
@adriansr

adriansr Apr 16, 2018

Author Member

Done

adriansr added some commits Apr 5, 2018

TCP plugins can be notified of stream expirations
Added a new interface: ExpirationAwareTCPPlugin. Protocol parsers
implementing this interface will be notified when a TCP stream expires
and have the oportunity to run some cleanup tasks and deliver
pending events. This is an opt-in feature. Plugins conforming only
to TCPPlugin will keep the existing behavior.
packetbeat: cleanup pcaps used in system tests
A couple of pcap files used in packetbeat's system tests include extra
HTTP streams with unmatched HTTP responses that are unnecessary for the
actual tests.

This will result in a test failure when packetbeat starts reporting
unmatched HTTP transactions.

@adriansr adriansr force-pushed the adriansr:feature/http_timeout branch from f49c939 to da30fc5 Apr 13, 2018

adriansr added some commits Apr 6, 2018

Packetbeat: System tests for HTTP unmatched requests
Added tests for the HTTP unmatched requests and responses.

- test_unmatched_response: Checks that a response received out of order
  inside a stream is reported as an Error.

- test_unmatched_request: Tests that a single stream containing only one
  request is reported after the stream times out. This tests can't be
  run with packetbeat's -t flag as it needs to wait 10s for the stream to
  expire. This timeout is currently not configurable.
http: Report unmatched requests or responses
Until now, unmatched requests or responses are silently discarded by the
http plugin.

This patch makes two changes to this behavior:

- When an HTTP response is received without a matching request, an event
  with Error status is generated.

- When an HTTP stream times out, any pending request or response that
  has received complete headers is also reported with Error status.

@adriansr adriansr force-pushed the adriansr:feature/http_timeout branch from da30fc5 to 53d7c49 Apr 16, 2018

@andrewkroh andrewkroh merged commit f3aa089 into elastic:master Apr 17, 2018

6 checks passed

CLA Commit author is a member of Elasticsearch
Details
Hound No violations found. Woof!
beats-ci Build finished.
Details
codecov/patch No report found to compare against
Details
codecov/project No report found to compare against
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.