Early Request Validation in Query Frontend #10093
Conversation
| @@ -186,26 +186,6 @@ func TestTripperware_InstantQuery(t *testing.T) { | |||
| }, res) | |||
| }) | |||
|
|
|||
| t.Run("specific time param with form being already parsed", func(t *testing.T) { | |||
There was a problem hiding this comment.
these tests are for old code that was removed as it is more robustly handled in the prometheus codec and associated utils.
The tests continued to pass by chance that we weren't using this particular request parsing middleware twice.
We have moved away from trying to go back and fix the situation if the request body is consumed by our own middleware. Instead everything that reads a request body just needs to use ParseRequestFormWithoutConsumingBody introduced in #5524.
| queryrange = NewMetricsQueryRequestValidationRoundTripper(codec, queryrange) | ||
| instant = NewMetricsQueryRequestValidationRoundTripper(codec, instant) | ||
| labels = NewLabelsQueryRequestValidationRoundTripper(codec, labels) | ||
| series = NewLabelsQueryRequestValidationRoundTripper(codec, series) |
There was a problem hiding this comment.
Series Request validation is also supported by the codec for Label Names and Label Values Requests - a separate PR exists to update the naming for the codecs to express this
…r; expand metrics query tests to cover instant queries
francoposa
force-pushed
the
francoposa/query-frontend-request-validation
branch
from
0e296ea to
6c336d2
Compare
There was a problem hiding this comment.
You are in a maze of handlers, middlewares, roundtrippers, and tripperwares, all alike.
This change LGTM and is a definite improvement.
However, it feels weird that we're parsing/decoding requests multiple times. There's some existing functionality that adds QueryDetails to the context of a request. WDYT about doing something similar, adding the parsed request from the validation roundtrippers to the context of each request so that every subsequent roundtripper doesn't need to repeat the parsing process in a follow-up PR? It's fine if the answer to that is "that'd just make things more complicated"
@56quarters I will scratch that out and see how it looks - the multiple parse is definitely something we want to get away from and this context approach could be a good band-aid in the mean time. There's still a bit of work to get to a point where we can have roundtrippers that can take a |
This reverts commit 3251e86.
* WIP sketch out validation roundtripper with existing metrics and labels query codecs * lint: license header * separate series request roundtrippers * introduce request validation roundtripper tests * lint import order * introduce & test cardinality query validation in frontend roundtripper; expand metrics query tests to cover instant queries * CHANGELOG
What this PR does
This PR introduces early request validation for the majority of request types supported by the query-frontend.
This specifically does not introduce any new request parsing or validation logic, but it ensures that the validation that does exist is called in the first roundtripper, before any other work is done and that a standardized 400 Bad Request APIError is returned.
The tests are largely focused on making sure we get a status 400 API Error when we expect one - each request validator roundtripper relies on existing validation codecs which have their own extensive test suites.
Done so far are the ones that have clearly defined validation code already in Mimir:
Still to be done, likely saved for a subsequent PR are:
Which issue(s) this PR fixes or relates to
In previous incidents we have seen unexpected issues when parsing an invalid requests errors out in a part of the stack which is not set up to propagate the errors properly back up the stack and prevent breaking connections or crashing goroutines.
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.