Fix cardinality and label names/values requests handling on POST requests #5524
Conversation
…ests Signed-off-by: Marco Pracucci <marco@pracucci.com>
| origBody := r.Body | ||
| defer func() { _ = origBody.Close() }() |
There was a problem hiding this comment.
The ParseRequestFormWithoutConsumingBody() logic has been copied from handler.ServeHTTP(). However, previously we didn't close the original Body (in handler.ServeHTTP() we just close the replaced req.Body) but I think we actually have to close the original Body too, so I've added this.
There was a problem hiding this comment.
I think you really need to call Close() only if the resource would be leaked otherwise. Like a file descriptor or some pooled memory. Otherwise GC will just clean it up. Probably doesn't hurt to have an extra defer Close, although if Close is transitive, that means calling it twice and the interface says that "The behavior of Close after the first call is undefined."
There was a problem hiding this comment.
The reference to the original Body is lost, so I'm not sure there's a real risk of closing it twice. On the contrary, I think none else could close it if we don't close it here.
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
| value := req.FormValue(paramName) | ||
| func parseRequestTimeParam(values url.Values, paramName string, defaultValue int64) (int64, error) { | ||
| var value string | ||
| if len(values[paramName]) > 0 { |
There was a problem hiding this comment.
Note to reviewers: this is the same thing done by req.FormValue() (it picks the 1st one).
Signed-off-by: Marco Pracucci <marco@pracucci.com>
What this PR does
We found a bug introduced in #5212 and #5426 which caused POST requests returning wrong results. The problem is that, when the cardinality and/or label names/values result cache is enabled, the caching middleware consumes the
http.RequestBodyand so the body will be empty when later read to forward it to the query-scheduler or querier.In this PR I propose a fix. The fix consists in consuming the Body and then replacing it with the original content. It's not a new approach, in fact we were already doing it in
Handler.ServeHTTP()so I've re-used that code, moving it to a common utility function.Note to reviewers:
Which issue(s) this PR fixes or relates to
N/A
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]