[Heartbeat] Correctly store HTTP bodies with validation #14223

andrewvc · 2019-10-24T18:11:28Z

Currently, when using an HTTP body validator (either regexp or JSON) will break the storage of HTTP bodies with response.include_body (introduced in #13022).

The root cause is that both validation and reading the body for inclusion in the event share the same ReadCloser provided by *http.Response.

This patch looks at both validation and body settings to determine how much of the body to read, reads that much, then passes that to the validation and body inclusion code as a []byte.

Resolves #13751

elasticmachine · 2019-10-24T18:11:30Z

Pinging @elastic/uptime (:uptime)

heartbeat/monitors/active/http/respbody.go

ruflin · 2019-10-28T09:03:18Z

heartbeat/monitors/active/http/check.go

 	}

 	if len(config.RecvJSON) > 0 {
 		jsonChecks, err := checkJSON(config.RecvJSON)
 		if err != nil {
-			return nil, err
+			return multiValidator{}, err


Don't like too much that even on error, we return an empty object.

Hmmm, what would you propose? The alternative would be making multiValidator a pointer type *multiValidator so we can still return nil. Seems unnecessary to use a pointer type IMHO given that it's initialized once at creation and never mutated. I'd call this a bit of a smell in golang TBH.

I normally would go with *multiValidator but also see your point. Keep it.

ruflin · 2019-10-28T09:03:56Z

heartbeat/monitors/active/http/check_test.go

 	"github.com/stretchr/testify/require"

+	"github.com/elastic/beats/libbeat/common"


Thanks for sorting 😆

I'm (slowly) getting better!

ruflin · 2019-10-28T09:04:47Z

heartbeat/monitors/active/http/respbody.go

+// maxBufferBodyBytes sets a hard limit on how much we're willing to buffer for any reason internally.
+// since we must buffer the whole body for body validators this is effectively a cap on that.
+// 100MiB out to be enough for everybody.
+const maxBufferBodyBytes = 100 * 1024 * 1024


Would probably make sense to document this max value somewhere in the docs. At least 1 user will hit it :-)

ruflin · 2019-10-28T09:06:43Z

heartbeat/monitors/active/http/respbody.go

 // then closes the body (which closes the connection). It doesn't return any errors
 // but does log them. During an error case the return values will be (nil, -1).
 // The maxBytes params controls how many bytes will be returned in a string, not how many will be read.
 // We always read the full response here since we want to time downloading the full thing.
 // This may return a nil body if the response is not valid UTF-8
-func readResp(resp *http.Response, maxSampleBytes int) (bodySample string, bodySize int64, hashStr string, err error) {
-	if resp == nil {


This is not needed anymore because?

We actually already checked this here: https://github.com/elastic/beats/pull/14223/files#diff-759945e9c6b1377331063825c8e427daR231

ruflin · 2019-10-28T09:09:19Z

heartbeat/monitors/active/http/task.go

 	}
-	eventext.MergeEventFields(event, common.MapStr{"http": response})
-	// Download the body, close the response body, then attach all fields
-	err := handleRespBody(event, resp, responseConfig, errReason)


This is now handled inside the validator, correct?

Not quite, it's handled inside the processBody method mostly now, but now that function is less side-effecty, and doesn't modify the event, it just returns the new HTTP fields. https://github.com/elastic/beats/pull/14223/files#diff-759945e9c6b1377331063825c8e427daR247 is where the fields are finally merged now, which IMHO is an easier to understand flow.

ruflin · 2019-10-28T09:11:18Z

Forgot to add: Changelog missing.

…lidate

andrewvc · 2019-10-28T18:53:57Z

@ruflin pushed some updates. I believe I've addressed all comments now.

ruflin

LGTM. Did not test it manually.

ruflin · 2019-10-29T12:37:22Z

heartbeat/docs/heartbeat-options.asciidoc

@@ -511,7 +511,8 @@ Under `check.response`, specify these options:

 *`status`*:: The expected status code. 4xx and 5xx codes are considered `down` by default. Other codes are considered `up`.
 *`headers`*:: The required response headers.
-*`body`*:: A list of regular expressions to match the the body output. Only a single expression needs to match.
+*`body`*:: A list of regular expressions to match the the body output. Only a single expression needs to match. HTTP response
+bodies of up to 100MiB are supported.


Reading this sound like 100MiB is crazy for a Body, but well ...

Currently, when using an HTTP body validator (either regexp or JSON) will break the storage of HTTP bodies with response.include_body (introduced in elastic#13022). The root cause is that both validation and reading the body for inclusion in the event share the same ReadCloser provided by *http.Response. This patch looks at both validation and body settings to determine how much of the body to read, reads that much, then passes that to the validation and body inclusion code as a []byte. Resolves elastic#13751 (cherry picked from commit 6c6b396)

) Currently, when using an HTTP body validator (either regexp or JSON) will break the storage of HTTP bodies with response.include_body (introduced in #13022). The root cause is that both validation and reading the body for inclusion in the event share the same ReadCloser provided by *http.Response. This patch looks at both validation and body settings to determine how much of the body to read, reads that much, then passes that to the validation and body inclusion code as a []byte. Resolves #13751 (cherry picked from commit 6c6b396)

Currently, when using an HTTP body validator (either regexp or JSON) will break the storage of HTTP bodies with response.include_body (introduced in elastic#13022). The root cause is that both validation and reading the body for inclusion in the event share the same ReadCloser provided by *http.Response. This patch looks at both validation and body settings to determine how much of the body to read, reads that much, then passes that to the validation and body inclusion code as a []byte. Resolves elastic#13751

… (elastic#14310) Currently, when using an HTTP body validator (either regexp or JSON) will break the storage of HTTP bodies with response.include_body (introduced in elastic#13022). The root cause is that both validation and reading the body for inclusion in the event share the same ReadCloser provided by *http.Response. This patch looks at both validation and body settings to determine how much of the body to read, reads that much, then passes that to the validation and body inclusion code as a []byte. Resolves elastic#13751 (cherry picked from commit 46643b0)

andrewvc added 4 commits October 23, 2019 17:07

Checkpoint

c22d84b

Checkpoint

5f84674

Checkpoint

b3c69e0

Existing tests pass

f73b6ed

andrewvc added bug in progress Pull request is currently in progress. Heartbeat Team:obs-ds-hosted-services Label for the Observability Hosted Services team labels Oct 24, 2019

andrewvc requested a review from a team as a code owner October 24, 2019 18:11

andrewvc self-assigned this Oct 24, 2019

houndci-bot reviewed Oct 24, 2019

View reviewed changes

heartbeat/monitors/active/http/respbody.go Outdated Show resolved Hide resolved

houndci-bot reviewed Oct 24, 2019

View reviewed changes

heartbeat/monitors/active/http/respbody.go Outdated Show resolved Hide resolved

houndci-bot reviewed Oct 24, 2019

View reviewed changes

heartbeat/monitors/active/http/respbody.go Outdated Show resolved Hide resolved

andrewvc added 3 commits October 24, 2019 13:11

Revert heartbeat.yml

ad4cbf1

Cleanup for hound/goimports

53d58a9

Test for body content on reads, cleanup

162265a

andrewvc added review and removed in progress Pull request is currently in progress. labels Oct 24, 2019

andrewvc requested a review from ruflin October 24, 2019 18:56

make update

420e120

ruflin reviewed Oct 28, 2019

View reviewed changes

andrewvc added 2 commits October 28, 2019 13:51

Add docs and changelog

a61fe63

Merge remote-tracking branch 'origin/master' into store-body-after-va…

3a456e1

…lidate

ruflin approved these changes Oct 29, 2019

View reviewed changes

andrewvc merged commit 6c6b396 into elastic:master Oct 29, 2019

andrewvc deleted the store-body-after-validate branch October 29, 2019 13:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Heartbeat] Correctly store HTTP bodies with validation #14223

[Heartbeat] Correctly store HTTP bodies with validation #14223

andrewvc commented Oct 24, 2019 •

edited

Loading

elasticmachine commented Oct 24, 2019

ruflin Oct 28, 2019

andrewvc Oct 28, 2019

ruflin Oct 29, 2019

ruflin Oct 28, 2019

andrewvc Oct 28, 2019

ruflin Oct 28, 2019

andrewvc Oct 28, 2019

ruflin Oct 28, 2019

andrewvc Oct 28, 2019

ruflin Oct 28, 2019

andrewvc Oct 28, 2019

ruflin commented Oct 28, 2019

andrewvc commented Oct 28, 2019

ruflin left a comment

ruflin Oct 29, 2019

		"github.com/stretchr/testify/require"

		"github.com/elastic/beats/libbeat/common"

[Heartbeat] Correctly store HTTP bodies with validation #14223

[Heartbeat] Correctly store HTTP bodies with validation #14223

Conversation

andrewvc commented Oct 24, 2019 • edited Loading

elasticmachine commented Oct 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin commented Oct 28, 2019

andrewvc commented Oct 28, 2019

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewvc commented Oct 24, 2019 •

edited

Loading