Add regexp matching of HTTP response headers to the http probe #419

gvsmirnov · 2019-02-16T13:33:23Z

I needed to add the verification of header values for some of the targets probed by the blackbox exporter, but apparently matching is currently only supported for body content.

This pull request adds two new config options: fail_if_header_matches_regexp and fail_if_header_not_matches_regexp, mirroring the existing fail_if_matches_regexp and fail_if_not_matches_regexp for the response body. I considered renaming these ones to fail_if_body_matches_regexp and fail_if_body_not_matches_regexp for consistency, but that would be a breaking change and thus does not seem like a good idea.

I am fairly new to golang, so I just tried to follow the style and apparent conventions in the rest of this project. Please let me know if something should be changed.

I also tested this extensively manually, it worked in all the corner cases I tried.

Signed-off-by: Gleb Smirnov <[email protected]>

gvsmirnov · 2019-02-16T13:48:09Z

Oh dear, I just checked the list of open pull requests and found not one, but TWO others with the same changes: #62 and #332 😕 Apparently they were abandoned by their authors before all the comments were addressed.

Looking at the comments from @brian-brazil, it seems like all of them are addressed in this PR now. The "check that header is missing" can be achieved by

fail_if_header_matches_regexp:
  - header: Not-Supposed-To-Be-Here
    allow_missing: true
    regexp: '.*'

gvsmirnov · 2019-02-16T13:52:37Z

config/config.go

+	Method                       string                  `yaml:"method,omitempty"`
+	Headers                      map[string]string       `yaml:"headers,omitempty"`
+	FailIfMatchesRegexp          []string                `yaml:"fail_if_matches_regexp,omitempty"`
+	FailIfNotMatchesRegexp       []string                `yaml:"fail_if_not_matches_regexp,omitempty"`


Should these two above be renamed to FailIfBodyMatchesRegexp and FailIfBodyNotMatchesRegexp ?

Yes, I would rename them.

SuperQ · 2019-02-17T17:02:49Z

Thanks for picking up where those other PRs went stale.

Per your above comment, I think you want the regexp to match .+ in order to enforce some header text. Or maybe I'm misunderstanding.

fail_if_header_matches_regexp:
  - header: Not-Supposed-To-Be-Here
    allow_missing: true
    regexp: '.+'

EDIT: Are headers with no value after the header name valid?

Signed-off-by: Gleb Smirnov <[email protected]>

gvsmirnov · 2019-02-17T18:31:48Z

I think you want the regexp to match .+ in order to enforce some header text.
EDIT: Are headers with no value after the header name valid?

If I read RFC 7230 section 3.2 correctly, header value is not optional, but an empty value is valid. That seems to be the generally accepted interpretation, judging by the discussions on the Internet that I found. However, http.Header.Get() returns an empty string ("") if the header is not set, which is ambiguous. Thankfully, the mapping can be accessed directly, so it's possible to just do something like len(textproto.MIMEHeader(header)[key]) > 0.

This leads to another consideration: as per section section 3.2.2, there may be multiple header fields with the same field name. Judging by the doc on http.Response.Header, they may be concatenated with comma delimiters. It is not clear to me in which cases it would be populated by the http client as a list with multiple string values, and in which cases it would be in a single string value as a comma-separated list.

I would propose the following behaviour:

Verify the allow_missing by checking if the header with given name is present at least once. If one or more values are empty, do not consider it to be missing.
Verify the regexp against all the header values for a given header name. For "fail if matches", at least one match fails the probe. For "fail if does not match", at least one mismatch fails the probe.
If multiple values are concatenated into a single comma-separated string, do not do any special handling. Let the user deal with such cases on their own.

I have doubts about the behaviour in (2) for the "fail if does not match" for multi-value headers. "fail if none match" may be more desirable than "fail if any does not match".

What do you think?

Signed-off-by: Gleb Smirnov <[email protected]>

gvsmirnov · 2019-02-17T21:03:34Z

After some consideration, the most common use of multi-value headers is probably Set-Cookie (the RFC even mentions it explicitly). The following use cases seem to be plausible:

Verify that a specific cookie is set: the user would want the probe to fail if a desired cookie is not present, but would not want the probe to fail if there are other cookies:

fail_if_header_not_matches_regexp:
  header: Set-Cookie
  regexp: '.*Domain=\.example\.com.*'

Verify that no cookies are set

fail_if_header_matches_regexp:
  header: Set-Cookie
  allow_missing: true
  regexp: '.*'

Another common multi-value header is Accept-Encoding, but in practice it's mostly represented as a single comma-separated value, not as multiple values.

I pushed the changes implementing the behaviour described above. Please let me know if you think it is reasonable.

brian-brazil

for consistency, but that would be a breaking change and thus does not seem like a good idea.

It's probably worth it.

We can always reconsider handling of multiple headers if there's use cases.

brian-brazil · 2019-02-18T09:17:40Z

prober/http.go

@@ -173,6 +236,11 @@ func ProbeHTTP(ctx context.Context, target string, module config.Module, registr
 			Help: "Indicates if probe failed due to regex",
 		})

+		probeFailedDueToHeaders = prometheus.NewGauge(prometheus.GaugeOpts{
+			Name: "probe_failed_due_to_headers",


probe_http_

falied_due_regex should already cover this

probe_http

Thee are several other labels that do not have the http: probe_ssl_earliest_cert_expiry and probe_failed_due_to_regex. Should they be changed, too? Or can I just ditch the newly introduced probe_failed_due_to_headers and keep the rest as is?

EDIT: formatting

They should be changed, but at least the ssl one is commonly used for alerting so we need to be a little careful so not now.

OK, I will not touch the existing labels.

But I am not sure if representing the failed header matches with probe_failed_due_to_regex is the right way. It would be helpful if the user could see immediately from the metrics where the failure originated from. I do not see any drawbacks to this either (the extra disk usage would be negligible).

If you do insist, I can reuse probe_failed_due_to_regex. Do you insist?

I'd suggest reusing, either that or have two more clearly named metrics.

OK, reused existing metric. Renaming existing ones would be a more dangerous breaking change indeed.

brian-brazil · 2019-02-18T09:27:04Z

prober/http.go

+		}
+	}
+	for _, headerMatchSpec := range httpConfig.FailIfHeaderNotMatchesRegexp {
+		values := textproto.MIMEHeader(header)[headerMatchSpec.Header]


This won't canonicalise the key

Fixed and added tests

Signed-off-by: Gleb Smirnov <[email protected]>

gvsmirnov · 2019-02-18T12:28:37Z

It's probably worth it.

Alright, done. The breaking change would be caught at the configuration parsing stage, so it's a fail-fast scenario.

brian-brazil · 2019-02-18T13:24:35Z

config/config.go

+	Headers                      map[string]string       `yaml:"headers,omitempty"`
+	FailIfBodyMatchesRegexp      []string                `yaml:"fail_if_body_matches_regexp,omitempty"`
+	FailIfBodyNotMatchesRegexp   []string                `yaml:"fail_if_body_not_matches_regexp,omitempty"`
+	FailIfHeaderMatchesRegexp    []HeaderMatch           `yaml:"fail_if_header_matches_regexp,omitempty"`


This isn't a string, so I'd remove the _regexp from the name to avoid confusion.

Where would the confusion arise from? Is there a convention that properties ending with _regexp are always strings that represent regular expression?

I could rename the properties so it would be like this:

# ... fail_if_body_matches_regexp: - 'Thou shall not pass' fail_if_header_matches: - header: Host regexp: example.com

It would lose the consistency between property names for header and body, though. Is it worth it?

Yes, the data structures are different

Signed-off-by: Gleb Smirnov <[email protected]>

gvsmirnov · 2019-02-19T10:45:55Z

@brian-brazil @SuperQ is there anything else I can do to get this merged? I would love to use the upstream version instead of own fork.

brian-brazil · 2019-02-19T11:15:23Z

CONFIGURATION.md

    [ - <regex>, ... ]

+  # Probe fails if response header matches regex. For headers with multiple values, fails if *at least one* matches


Added here and in other places

brian-brazil · 2019-02-19T11:15:34Z

CONFIGURATION.md

+  # Probe fails if response header matches regex. For headers with multiple values, fails if *at least one* matches
+  fail_if_header_matches:
+    [ - [ header: <string>,
+          [ regex: <regex>, ]


Indentation seems off here

Yeah, does not look quite good. I got it from the prometheus config documentation, e.g. here when the same level contains both optional and non-optional parameters, then it looks like this (comments stripped):

job_name: <job_name> [ scrape_interval: <duration> | default = <global_config.scrape_interval> ]

In here, though, the regex should be named regexp and be made non-optional, so it would look like this:

# Probe fails if response header matches regex. For headers with multiple values, fails if *at least one* matches. fail_if_header_matches: [ - [ header: <string>, regexp: <regex>, [ allow_missing: <boolean> | default = false ] ], ... ]

Would that be OK?

Looking around, the only similar one we have is some of pagerduty for the AM. There we break it out as a different type.

Another example right in this repo is query_response in tcp_probe, it does not break it out as a separate type. That's why I did not do it here. Can extract both or keep inlined like above

For query_response they're all optional though.

OK, extracted only the http header match spec.

brian-brazil · 2019-02-19T11:16:03Z

prober/http.go

+				level.Error(logger).Log("msg", "Missing required header", "header", headerMatchSpec.Header)
+				return false
+			} else {
+				continue // no need to match any regex on missing headers


"No", and full stop

Fixed here and in the other place.

Signed-off-by: Gleb Smirnov <[email protected]>

gvsmirnov · 2019-02-19T11:58:46Z

Huh, travis build failed when fetching dependencies. Is it possible to rerun it?

gvsmirnov · 2019-02-20T10:52:48Z

@brian-brazil @SuperQ sorry to pester, but I ask again: anything else I can do to get this merged?

brian-brazil · 2019-02-20T13:21:44Z

CONFIGURATION.md

+
+```yml
+header: <string>,
+regexp: <regex>,


This is optional

Why is it optional? It is not possible to match a header without a regex to match against.

Unless you allow it to be missing

But if you allow it to be missing, then what is the point of having a match rule at all?

fail_if_header_matches: - header: Transfer-Encoding allow_missing: true fail_if_header_not_matches: - header: Transfer-Encoding allow_missing: true

Neither of these rules make sense to me. But looking at the checks made in config.go, apparently they used to. So I think I should change the config check, so it always required regexp to be set, regardless of allow_missing.

Would that be OK?

In a stroke of speculative execution, did just that.

Signed-off-by: Gleb Smirnov <[email protected]>

brian-brazil · 2019-02-21T13:28:59Z

Thanks!

gvsmirnov · 2019-02-21T16:00:52Z

Thanks for a thorough code review! I appreciate it.

Is there any estimate of when the next version will be released? I’m fine building from upstream source, asking for the sake of completeness.

brian-brazil · 2019-02-21T16:07:30Z

It's not going to be today, and there's a breaking change in there so Friday is a bad idea. Probably early next week.

piotrkochan · 2019-03-11T07:18:23Z

So now example.yml should be fixed, because for "http" prober option "fail_if_body_matches_regexp" fails and this is very confusing:

msg="Error loading config" err="Error parsing config file: yaml: unmarshal errors:\n  line 8: field fail_if_body_matches_regexp not found in type config.plain"

gvsmirnov · 2019-03-11T08:46:16Z

@piotrkochan how should it be fixed? example.yml is in line with the renamed config parameter. It used to be fail_if_matches_regexp, now it is fail_if_body_matches_regexp.

Where do you get this error? I would guess that you are running the old version of blackbox_exporter with the new version of config. However, the new version of blackbox_exporter has not yet been released, so the documentation is out of sync with the available distributions.

@brian-brazil any way I can help with making a release? This is indeed confusing now.

brian-brazil · 2019-03-11T10:47:36Z

#382 (comment) is what's holding a release up.

gvsmirnov added 2 commits February 16, 2019 21:34

Add regexp matching of HTTP response headers to the http probe

f6cdd9c

Signed-off-by: Gleb Smirnov <[email protected]>

Fixed a copy-paste error in the documentation

2c2a8ee

Signed-off-by: Gleb Smirnov <[email protected]>

gvsmirnov changed the title ~~Verify http headers~~ Add regexp matching of HTTP response headers to the http probe Feb 16, 2019

Remove potentially secret header values from logs

aa18657

Signed-off-by: Gleb Smirnov <[email protected]>

gvsmirnov commented Feb 16, 2019

View reviewed changes

SuperQ requested a review from brian-brazil February 17, 2019 17:04

Rename two fields in HTTPProbe config for clarity and consistency

b5c2f94

Signed-off-by: Gleb Smirnov <[email protected]>

Add support for empty header values and multi-value headers

4bf57e1

Signed-off-by: Gleb Smirnov <[email protected]>

brian-brazil reviewed Feb 18, 2019

View reviewed changes

gvsmirnov added 2 commits February 18, 2019 20:18

Canonicalize header names when matching

6abb43e

Signed-off-by: Gleb Smirnov <[email protected]>

Breaking change: rename config parametes of HTTP probe for consistency

4e794b3

Signed-off-by: Gleb Smirnov <[email protected]>

brian-brazil reviewed Feb 18, 2019

View reviewed changes

gvsmirnov added 2 commits February 18, 2019 23:35

Rename header match config properties to avoid a confusing postfix

7c95fb3

Signed-off-by: Gleb Smirnov <[email protected]>

Reused existing prometheus metrics for failed http header matches

0ed9994

Signed-off-by: Gleb Smirnov <[email protected]>

brian-brazil reviewed Feb 19, 2019

View reviewed changes

Grammar, style and formatting

3527651

Signed-off-by: Gleb Smirnov <[email protected]>

brian-brazil reviewed Feb 20, 2019

View reviewed changes

Always require regexp for http header matching

e7e7afb

Signed-off-by: Gleb Smirnov <[email protected]>

brian-brazil merged commit c453ee3 into prometheus:master Feb 21, 2019

brian-brazil mentioned this pull request Feb 21, 2019

Add HTTP response headers checker #62

Closed

brian-brazil mentioned this pull request Feb 21, 2019

HTTP response header regex matching #332

Closed

gvsmirnov deleted the verify-http-headers branch February 21, 2019 16:00

		[ - <regex>, ... ]

		# Probe fails if response header matches regex. For headers with multiple values, fails if at least one matches

Add regexp matching of HTTP response headers to the http probe #419

Add regexp matching of HTTP response headers to the http probe #419

Conversation

gvsmirnov commented Feb 16, 2019

gvsmirnov commented Feb 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperQ commented Feb 17, 2019 • edited Loading

gvsmirnov commented Feb 17, 2019

gvsmirnov commented Feb 17, 2019 • edited Loading

brian-brazil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvsmirnov Feb 18, 2019 • edited Loading

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

gvsmirnov commented Feb 18, 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

gvsmirnov commented Feb 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvsmirnov Feb 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvsmirnov Feb 19, 2019 • edited Loading

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

gvsmirnov commented Feb 19, 2019

gvsmirnov commented Feb 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvsmirnov Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brian-brazil commented Feb 21, 2019

gvsmirnov commented Feb 21, 2019

brian-brazil commented Feb 21, 2019

piotrkochan commented Mar 11, 2019 • edited Loading

gvsmirnov commented Mar 11, 2019

brian-brazil commented Mar 11, 2019

gvsmirnov commented Feb 16, 2019 •

edited

Loading

SuperQ commented Feb 17, 2019 •

edited

Loading

gvsmirnov commented Feb 17, 2019 •

edited

Loading

gvsmirnov Feb 18, 2019 •

edited

Loading

gvsmirnov Feb 19, 2019 •

edited

Loading

gvsmirnov Feb 19, 2019 •

edited

Loading

gvsmirnov Feb 20, 2019 •

edited

Loading

piotrkochan commented Mar 11, 2019 •

edited

Loading