Rules API: Make response more verbose #235
Conversation
Signed-off-by: Matej Gera <[email protected]>
saswatamcode
left a comment
saswatamcode
left a comment
There was a problem hiding this comment.
Thanks! This makes sense! 🙂
Just need to fix lint! But we ran across this again. Worth making an issue?
Signed-off-by: Matej Gera <[email protected]>
matej-g
force-pushed
the
rules-api-var-adjustments
branch
from
33be189 to
c0514a7
Compare
api/metrics/v1/rules.go
Outdated
| if err != nil { | ||
| level.Error(rh.logger).Log("msg", "could not unmarshal rules", "err", err.Error()) | ||
| http.Error(w, "error unmarshaling rules", http.StatusInternalServerError) | ||
| http.Error(w, fmt.Sprintf("error unmarshaling rules: %v", err), http.StatusInternalServerError) |
There was a problem hiding this comment.
We normally don't want to return arbitrary errors to the user from API. We don't do this (AFAIK) anywhere in Observatorium or other projects because it raises the possibility of injection attacks where users trigger a known error to get the API to construct a reply with an injected value, which can later be exploited.
There was a problem hiding this comment.
In other words, it's our convention to not echo untrusted values back to users
There was a problem hiding this comment.
If there is any convention, then I wasn't aware of it, we seem to return unknown error in at least a couple other places in the API (e.g. in labels enforcer). But I understand this can have potential security implications. I'm happy to defer this until #234, in which (in ideal case) we should be able to establish cause of error and potentially return more details to user, since current message is not very helpful.
There was a problem hiding this comment.
Done 👍 I removed this change.
There was a problem hiding this comment.
Done 👍 I removed this change.
| .PHONY: test-e2e | ||
| test-e2e: container-test | ||
| CGO_ENABLED=1 GO111MODULE=on go test -v -race -short --tags integration ./test/e2e | ||
| @rm -rf test/e2e/e2e_* |
Signed-off-by: Matej Gera <[email protected]>
| "github.com/efficientgo/tools/core/pkg/testutil" | ||
| ) | ||
|
|
||
| const recordingRuleYamlTpl = ` |
There was a problem hiding this comment.
I think to make the e2e tests pass we need now to update this const as in here
after that LGTM :)
There was a problem hiding this comment.
Whoops, missed this during merge conflict, thank you 🙇
Signed-off-by: Matej Gera <[email protected]>
4 participants
When working with rules API, the response message is not always very useful for the user. I suggest to: