-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 response header regex matching #332
HTTP response header regex matching #332
Conversation
cfe8a95
to
bbe2361
Compare
Signed-off-by: Richard Mitchell <[email protected]>
Signed-off-by: Richard Mitchell <[email protected]>
bbe2361
to
80a451d
Compare
Any thoughts on this, @SuperQ / @brian-brazil? :) |
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, good idea.
Signed-off-by: Richard Mitchell <[email protected]>
config/config.go
Outdated
type HTTPHeaderMatch struct { | ||
Header string `yaml:"header,omitempty"` | ||
Pattern string `yaml:"pattern,omitempty"` | ||
Required bool `yaml:"pattern,omitempty"` |
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.
pattern?
fail_if_header_not_matches_regexp: | ||
[ - header: <header-name> | ||
pattern: <regex> | ||
# Also fail if the header is missing from the response. |
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.
How would you assert that a header must be missing?
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 think that should be a separate configuration option altogether if required. Having functionality that checks for value matches also check for the complete absence of the key is hard to parse, semantically.
In theory empty header values are valid, but in practice one could probably achieve something close enough with:
- header: Server
pattern: ^$
required: false
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.
An empty header is not the same as no header
@@ -66,6 +66,22 @@ The other placeholders are specified separately. | |||
fail_if_not_matches_regexp: |
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 needs clarification that it's for the body
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 wanted to avoid backwards incompatibility in the configuration file format. If that's acceptable, would having fail_if_not_matches_regexp
map to fail_if_body_not_matches_regexp
with a deprecation warning logged be workable?
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 meant more the comment above it
prober/http.go
Outdated
@@ -169,9 +209,14 @@ func ProbeHTTP(ctx context.Context, target string, module config.Module, registr | |||
Help: "Returns the version of HTTP of the probe response", | |||
}) | |||
|
|||
probeFailedDueToRegex = prometheus.NewGauge(prometheus.GaugeOpts{ | |||
probeFailedDueToHeaderRegex = prometheus.NewGauge(prometheus.GaugeOpts{ | |||
Name: "probe_failed_due_to_header_regex", |
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 think one metric for this is fine, the debug logs will always have the full details
prober/http.go
Outdated
} | ||
} | ||
} else if matcher.Required { | ||
return false |
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.
Log when you return false so the user can know why it failed
Signed-off-by: Richard Mitchell <[email protected]>
Signed-off-by: Richard Mitchell <[email protected]>
Signed-off-by: Richard Mitchell <[email protected]>
ebfadbe
to
4c9caef
Compare
Updated and responded to a couple of those comments, @brian-brazil :) |
@@ -66,6 +66,22 @@ The other placeholders are specified separately. | |||
fail_if_not_matches_regexp: |
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 meant more the comment above it
fail_if_header_not_matches_regexp: | ||
[ - header: <header-name> | ||
pattern: <regex> | ||
# Also fail if the header is missing from the response. |
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.
An empty header is not the same as no header
[ - header: <header-name> | ||
pattern: <regex> | ||
# Also fail if the header is missing from the response. | ||
required: <bool> |
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 think this would be implied from the pattern match, so we don't need an extra option for it
if headerValues, ok := headers[key]; ok { | ||
re, err := regexp.Compile(matcher.Pattern) | ||
if err != nil { | ||
level.Error(logger).Log("msg", "Could not compile regular expression", "regexp", key, matcher.Pattern, "err", 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.
the key is not the regex
} | ||
for _, value := range headerValues { | ||
if re.MatchString(value) { | ||
level.Error(logger).Log("msg", "Header matched regular expression", "regexp", key, value, matcher.Pattern) |
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.
Let's not log the value, it could be a secret
Did you get any further with this? |
I've been away on holiday, I shall pick it up again this week, thanks. |
Did you make any progress since your holiday? |
Obsoleted by #419 |
Also enables validating if response headers exist or not.
Defaulting to require all headers to match/not match is something I'm not sure about, but seems like the least worst default. Having multiple headers with the same name is a fairly uncommon scenario.