-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
x-pack/filebeat/input/http_endpoint: add support for request trace logging #36957
Conversation
1221079
to
4d7707c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
4d7707c
to
a812c05
Compare
Pinging @elastic/security-external-integrations (Team:Security-External Integrations) |
This pull request is now in conflicts. Could you fix it? 🙏
|
a812c05
to
35b49cf
Compare
This pull request is now in conflicts. Could you fix it? 🙏
|
35b49cf
to
4ab0649
Compare
This pull request is now in conflicts. Could you fix it? 🙏
|
4ab0649
to
eb4c687
Compare
[float] | ||
==== `tracer.filename` | ||
|
||
It is possible to log HTTP requests and responses in a CEL program to a local file-system for debugging configurations. |
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.
Needs tailoring to this input (mentions CEL).
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 would specify that only requests that have passed validation checks (e.g. auth, content-type, method, HMAC) are logged.
Perhaps we should be trace logging unconditionally, and annotating that the request was rejected (like with the http response body and status code). That would be useful for debugging HMAC issues.
@@ -167,6 +130,70 @@ func (rt *LoggingRoundTripper) RoundTrip(req *http.Request) (*http.Response, err | |||
return resp, err | |||
} | |||
|
|||
// LogRequest logs an HTTP request to the provided logger. | |||
// | |||
// Fields logged: |
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 would like to have the source.{ip,port}
in the log.
This comment was marked as outdated.
This comment was marked as outdated.
eb4c687
to
38001b7
Compare
// If we are logging, keep a copy of the body for the logger. | ||
// This is stashed in the r.Body field. This is only safe | ||
// because we are closing the original body in a defer and | ||
// r.Body is not otherwise referenced by the non-logging logic | ||
// after the call to getBodyReader 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.
This comment is not strictly speaking true. The current handling of gzip content breaks the invariant that request bodies are closed. This is a bug in the current code, so I won't fix it here, but the change is
diff --git a/x-pack/filebeat/input/http_endpoint/gzip.go b/x-pack/filebeat/input/http_endpoint/gzip.go
index 332b482497..5b3e7111f1 100644
--- a/x-pack/filebeat/input/http_endpoint/gzip.go
+++ b/x-pack/filebeat/input/http_endpoint/gzip.go
@@ -18,15 +18,16 @@ var gzipDecoderPool = sync.Pool{
type pooledGzipReader struct {
Reader *gzip.Reader
+ closer io.Closer
}
-func newPooledGzipReader(r io.Reader) (*pooledGzipReader, error) {
+func newPooledGzipReader(r io.ReadCloser) (*pooledGzipReader, error) {
gzipReader := gzipDecoderPool.Get().(*gzip.Reader)
if err := gzipReader.Reset(r); err != nil {
gzipDecoderPool.Put(gzipReader)
return nil, err
}
- return &pooledGzipReader{Reader: gzipReader}, nil
+ return &pooledGzipReader{Reader: gzipReader, closer: r}, nil
}
// Read implements io.Reader, reading uncompressed bytes from its underlying Reader.
@@ -34,13 +35,17 @@ func (r *pooledGzipReader) Read(b []byte) (int, error) {
return r.Reader.Read(b)
}
-// Close closes the Reader. It does not close the underlying io.Reader.
+// Close closes the Reader and the underlying source.
// In order for the GZIP checksum to be verified, the reader must be
// fully consumed until the io.EOF.
//
// After this call the reader should not be reused because it is returned to the pool.
func (r *pooledGzipReader) Close() error {
err := r.Reader.Close()
+ _err := r.closer.Close()
+ if err == nil {
+ err = _err
+ }
gzipDecoderPool.Put(r.Reader)
r.Reader = nil
return 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.
LGTM. Thanks for adding the source.{ip,port} and covering all the request paths with trace logging.
Proposed commit message
See title.
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Author's Checklist
How to test this PR locally
Run
go test -log-traces
in x-pack/filebeat/input/http_endpoint` and then check the resulting ndjson files in the trace_logs directory.Related issues
Use cases
Screenshots
Logs