-
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
Support matching JSON body with CEL expressions #1255
base: master
Are you sure you want to change the base?
Conversation
45e8b9c
to
2f2d0fb
Compare
@roidelapluie @mem Ping :) |
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.
A couple of minor comments.
@@ -297,6 +351,11 @@ func ProbeHTTP(ctx context.Context, target string, module config.Module, registr | |||
Help: "Indicates if probe failed due to regex", | |||
}) | |||
|
|||
probeFailedDueToCel = prometheus.NewGauge(prometheus.GaugeOpts{ |
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'm thinking this should be added conditionally, if the configuration asks for validation using CEL expressions.
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 was following the same logic that is applied to regex checks, following your logic that should also be conditionally added, no?
Thank you for this, this seems useful! Question, I have no experience with CEL... is it possible to use it to match HTML, too? I see in the code that Eval takes a The reason I'm asking is because I'm trying to figure out if it's possible to extend this to more inputs, or if it needs to be restricted to JSON. |
prober/http.go
Outdated
} | ||
|
||
if httpConfig.FailIfBodyJSONMatchesCel != nil { | ||
result, details, err := httpConfig.FailIfBodyJSONMatchesCel.Eval(evalPayload) |
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.
Question: shouldn't this be EvalContext instead? I'm not clear about the guarantees that the evaluation will terminate, and the fact that EvalContext exists suggests that there's a benefit from imposing a deadline.
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.
You're probably right, I've changed it to use ContextEval
0c9b9a0
to
f9a0036
Compare
I atleast do not know how CEL could be used to match HTML in any sane way. You might want to check this out https://github.com/google/cel-spec/blob/master/doc/langdef.md#json-data-conversion Thanks for your comments and suggestions so far. Really hoping we can get this merged. I think that having the ability to validate JSON responses would be an awesome addition to blackbox exporter. :) |
@mem kindly ping :) we are interested about this feature and would like to see it as well |
f9a0036
to
b9b903b
Compare
fbb6791
to
9131539
Compare
9131539
to
3101080
Compare
Signed-off-by: Juho Majasaari <[email protected]>
Signed-off-by: Juho Majasaari <[email protected]>
Signed-off-by: Juho Majasaari <[email protected]>
3101080
to
2b1939d
Compare
@mem @roidelapluie any comments to this PR? |
@electron0zero @mem I haven't heard from any of the maintainers in a while. I fixed or clarified everything that was mentioned in the review comments. The initial message I got was that this seems useful, but it seems that it's forgotten now. Any update? I'd love to get this merged in order to avoid maintaining my own fork. :) |
This PR implements parsing a JSON response from the body and checking the result using a CEL query. Looking for feedback on this.
Sample usage:
If response is
{"foo": { "bar": "qux" } }
the probe fails. fail_if_body_json_matches_cel is also implemented and should work as expected.