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 fail_if_body_too_large http probe option #1276

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CONFIGURATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,9 @@ modules:
# Probe fails if SSL is not present.
[ fail_if_not_ssl: <boolean> | default = false ]

# Probe fails if a defined body_size_limit is exceeded.
[ fail_if_body_too_large: <boolean> | default = true ]
hashworks marked this conversation as resolved.
Show resolved Hide resolved

# Probe fails if response body matches regex.
fail_if_body_matches_regexp:
[ - <regex>, ... ]
Expand Down
8 changes: 8 additions & 0 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ type HTTPProbe struct {
NoFollowRedirects *bool `yaml:"no_follow_redirects,omitempty"`
FailIfSSL bool `yaml:"fail_if_ssl,omitempty"`
FailIfNotSSL bool `yaml:"fail_if_not_ssl,omitempty"`
FailIfBodyTooLarge *bool `yaml:"fail_if_body_too_large,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

any reason to use *bool here instead of just bool like FailIfNotSSL??

Method string `yaml:"method,omitempty"`
Headers map[string]string `yaml:"headers,omitempty"`
FailIfBodyMatchesRegexp []Regexp `yaml:"fail_if_body_matches_regexp,omitempty"`
Expand Down Expand Up @@ -455,6 +456,13 @@ func (s *HeaderMatch) UnmarshalYAML(unmarshal func(interface{}) error) error {
return nil
}

func (h HTTPProbe) GetFailIfBodyTooLarge() bool {
Copy link
Author

Choose a reason for hiding this comment

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

Alternatively I could do the nil check in UnmarshalYAML, but then it could panic if someone changes the variable under the hood later on.

Copy link
Member

@electron0zero electron0zero Dec 31, 2024

Choose a reason for hiding this comment

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

To preserve the current behaviour, FailIfBodyTooLarge should be set to true by default.

you should be able to set it to true by default in DefaultHTTPProbe at https://github.com/prometheus/blackbox_exporter/blob/master/config/config.go#L51

if we make this bool instead of *bool, we do't have to worry about nil, and can remove this whole function.

if h.FailIfBodyTooLarge == nil {
return true
}
return *h.FailIfBodyTooLarge
}

// isCompressionAcceptEncodingValid validates the compression +
// Accept-Encoding combination.
//
Expand Down
9 changes: 9 additions & 0 deletions example.yml
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,15 @@ modules:
compression: gzip
headers:
Accept-Encoding: gzip
http_endless_stream_example:
prober: http
http:
method: GET
fail_if_header_not_matches:
- header: content-type
regexp: "audio/.*"
body_size_limit: 512B
fail_if_body_too_large: false
tls_connect:
prober: tcp
timeout: 5s
Expand Down
2 changes: 1 addition & 1 deletion prober/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -549,7 +549,7 @@ func ProbeHTTP(ctx context.Context, target string, module config.Module, registr

if !requestErrored {
_, err = io.Copy(io.Discard, byteCounter)
if err != nil {
if err != nil && (errors.Is(err, &http.MaxBytesError{}) || httpConfig.GetFailIfBodyTooLarge()) {
Copy link
Author

Choose a reason for hiding this comment

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

Please check if this is okay or if I should move this to another if so it's clearer.

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer another if for clarity

logger.Info("Failed to read HTTP response body", "err", err)
success = false
}
Expand Down
27 changes: 27 additions & 0 deletions prober/http_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import (
"testing"
"time"

"github.com/alecthomas/units"
"github.com/andybalholm/brotli"
"github.com/prometheus/client_golang/prometheus"
pconfig "github.com/prometheus/common/config"
Expand Down Expand Up @@ -838,6 +839,32 @@ func TestFailIfNotSSL(t *testing.T) {
checkRegistryResults(expectedResults, mfs, t)
}

func TestFailIfBodySizeTooLarge(t *testing.T) {
bodySizeLimit := units.Base2Bytes(1)

ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
resp := bytes.Repeat([]byte{'A'}, int(bodySizeLimit)+1)

w.Header().Set("Content-Length", strconv.Itoa(len(resp)))
w.WriteHeader(http.StatusOK)
w.Write(resp)
}))
defer ts.Close()

for _, failIfBodyTooLarge := range []bool{true, false} {
Copy link
Member

Choose a reason for hiding this comment

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

can we turn into a proper table test with config, and the expectation, see TestFailIfNotSSLLogMsg for reference

registry := prometheus.NewRegistry()
testCTX, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
result := ProbeHTTP(testCTX, ts.URL,
config.Module{Timeout: time.Second, HTTP: config.HTTPProbe{IPProtocolFallback: true, BodySizeLimit: bodySizeLimit, FailIfBodyTooLarge: &failIfBodyTooLarge}}, registry, promslog.NewNopLogger())
if result && failIfBodyTooLarge {
t.Fatal("Fail if body size too large succeeded unexpectedly")
} else if !result && !failIfBodyTooLarge {
t.Fatal("Dont't fail if body too large failed unexpectedly")
}
}
}

type logRecorder struct {
msgs map[string]bool
next *slog.Logger
Expand Down
Loading