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

Add body_size_limit option to http module #836

Merged
merged 2 commits into from
Oct 19, 2021

Conversation

mem
Copy link
Contributor

@mem mem commented Oct 15, 2021

This option limits the maximum body length that will be read from the
HTTP server. It's meant to prevent misconfigured servers from causing
the probe to use too many resources, even if temporarily. It's not an
additional check on the response, for that, use the resulting metrics
(probe_http_content_length, probe_http_uncompressed_body_length, etc).

Signed-off-by: Marcelo E. Magallon [email protected]

@mem mem requested a review from roidelapluie October 15, 2021 02:35
config/config.go Outdated
@@ -287,6 +288,11 @@ func (s *HTTPProbe) UnmarshalYAML(unmarshal func(interface{}) error) error {
if err := unmarshal((*plain)(s)); err != nil {
return err
}

if s.MaxResponseLength < 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

underlying go code threats that as valid, I think we should too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After looking at the body_size_limit code in Prometheus, for consistency's sake, I copied the behavior implemented there: anything less than 0 is reinterpreted as MaxInt64.

I'm not sure I like that, but it's consistent.

Copy link
Member

@roidelapluie roidelapluie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we match Prometheus behaviour? Prometheus looks at the uncompressed response.

# An uncompressed response body larger than this many bytes will cause the
# scrape to fail. 0 means no limit. Example: 100MB.
# This is an experimental feature, this behaviour could
# change or be removed in the future.
[ body_size_limit: <size> | default = 0 ]

CONFIGURATION.md Outdated Show resolved Hide resolved
@mem mem force-pushed the mem/add_http_max_response_length branch from 95cdc57 to 52427fb Compare October 15, 2021 22:59
@mem
Copy link
Contributor Author

mem commented Oct 15, 2021

Can we match Prometheus behaviour? Prometheus looks at the uncompressed response.

# An uncompressed response body larger than this many bytes will cause the
# scrape to fail. 0 means no limit. Example: 100MB.
# This is an experimental feature, this behaviour could
# change or be removed in the future.
[ body_size_limit: <size> | default = 0 ]

OK, I added that for consistency. I used the same name and syntax, too.

This option limits the maximum body length that will be read from the
HTTP server. It's meant to prevent misconfigured servers from causing
the probe to use too many resources, even if temporarily. It's not an
additional check on the response, for that, use the resulting metrics
(probe_http_content_length, probe_http_uncompressed_body_length, etc).

Signed-off-by: Marcelo E. Magallon <[email protected]>
@mem mem force-pushed the mem/add_http_max_response_length branch from 52427fb to 61c5cf6 Compare October 15, 2021 23:15
@mem mem requested a review from roidelapluie October 15, 2021 23:43
Copy link
Member

@electron0zero electron0zero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 👍🏼

ps: PR title is outdated after rename to body_size_limit

config/config.go Outdated Show resolved Hide resolved
Co-authored-by: Julien Pivotto <[email protected]>
@mem mem changed the title Add max_response_length option to http module Add body_size_limit option to http module Oct 18, 2021
@roidelapluie
Copy link
Member

roidelapluie commented Oct 18, 2021

Does it actually work with compressed data?

@roidelapluie
Copy link
Member

I wonder if we still store the entire body in memory when it is compressed.

@mem
Copy link
Contributor Author

mem commented Oct 19, 2021

Does it actually work with compressed data?

Do you mean whether it limits the amount of data that is read?

Yes, it does: the stack of readers that is built (body -> decompressor -> limiter -> byte counter) they all just forward their read calls to the next reader. The decompressor in particular decompresses on the fly and if the limiter gets more data than it's willing to accept, it will error out and this will cause a chain of Close calls.

@mem
Copy link
Contributor Author

mem commented Oct 19, 2021

I wonder if we still store the entire body in memory when it is compressed.

That depends on whether or not regular expressions are used. If no regular expressions are involved, then it only streams (decompresses on the fly). If regular expressions are involved, that part of the code does a full read before trying to apply the regular expression (because we allow for multiple regular expressions, so the body has to be buffered to try to match multiple times).

@mem mem merged commit e869809 into master Oct 19, 2021
@mem mem deleted the mem/add_http_max_response_length branch October 19, 2021 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants