Solve #717 Validating config should try to compile regexes #729
Conversation
|
Thanks for your PR, could we use the same approach as in https://github.com/prometheus/prometheus/blob/master/pkg/relabel/relabel.go ? |
…ile regexes Signed-off-by: sshota0809 <[email protected]>
Signed-off-by: sshota0809 <[email protected]>
…p struct Signed-off-by: sshota0809 <[email protected]>
Signed-off-by: sshota0809 <[email protected]>
|
@brian-brazil |
prober/tcp.go
Outdated
| level.Error(logger).Log("msg", "Could not compile into regular expression", "regexp", qr.Expect, "err", err) | ||
| return false | ||
| } | ||
| if qr.Expect.Regexp != nil && qr.Expect.Regexp.String() != "" { |
There was a problem hiding this comment.
Isn't Regexp always non-nil?
There was a problem hiding this comment.
Isn't Regexp always non-nil?
There is the case that config.Module instance is created in prober/tcp_test.go.
blackbox_exporter/prober/tcp_test.go
Lines 359 to 378 in 49ae740
And when Expect isn't explicitly specified as following line, it's possible that qr.Expect.Rege is nil.
blackbox_exporter/prober/tcp_test.go
Line 364 in 49ae740
But that occors only in the test senario. In this case, should I modify prober/tcp_test.go as following.
{Expect: config.MustNewRegexp(""), Send: "EHLO tls.prober"},
Could you give me an advice.
There was a problem hiding this comment.
You just need the first predicate here, only check for nil. Expecting an empty response is valid after all.
There was a problem hiding this comment.
You just need the first predicate here, only check for nil.
Thank you. I deleted the latter predicate.
config/config.go
Outdated
| @@ -303,7 +355,7 @@ func (s *HeaderMatch) UnmarshalYAML(unmarshal func(interface{}) error) error { | |||
| return errors.New("header name must be set for HTTP header matchers") | |||
| } | |||
|
|
|||
| if s.Regexp == "" { | |||
| if s.Regexp.Regexp == nil { | |||
There was a problem hiding this comment.
Isn't this never nil, and you should be checking for the empty string?
I think you're being inconsistent in how you're treating this.
There was a problem hiding this comment.
It's possible that s.Regexp.Regexp is nil when following case. (http.fail_if_header_not_matches.regexp isn't explicitly specified.)
modules:
http_headers:
prober: http
timeout: 5s
http:
fail_if_header_not_matches:
- header: Access-Control-Allow-Origin
allow_missing: false
And as you said I have to check for the empty string, so I added its condition.
Signed-off-by: sshota0809 <[email protected]>
config/config.go
Outdated
| @@ -289,6 +337,10 @@ func (s *QueryResponse) UnmarshalYAML(unmarshal func(interface{}) error) error { | |||
| if err := unmarshal((*plain)(s)); err != nil { | |||
| return err | |||
| } | |||
| if s.Expect.Regexp == nil { | |||
| s.Expect = MustNewRegexp("") | |||
There was a problem hiding this comment.
Not being set and being the empty string are different, if there's handling here it should be in the probe rather than here.
There was a problem hiding this comment.
I deleted this IF statement to accept nil.
prober/tcp.go
Outdated
| level.Error(logger).Log("msg", "Could not compile into regular expression", "regexp", qr.Expect, "err", err) | ||
| return false | ||
| } | ||
| if qr.Expect.Regexp != nil && qr.Expect.Regexp.String() != "" { |
There was a problem hiding this comment.
You just need the first predicate here, only check for nil. Expecting an empty response is valid after all.
Signed-off-by: sshota0809 <[email protected]>
|
@brian-brazil I'm done with modifying what you mentioned. |
|
Thanks! |
Hello, This is my first contribution.
I solved issue #717.
I modified Validating config so that it includes trying to compile regexes and then output error if there are invalid regexes. Also I added test related to them.
Please check if my commit is correct. Thank you for your consideration.!