diff --git a/CHANGELOG.md b/CHANGELOG.md index 119e330d94a..ab2295a85e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,7 @@ * [BUGFIX] metrics-generator: Fix active-series counter underflow in local series limiter when overflow series are deleted [#6568](https://github.com/grafana/tempo/pull/6568) (@carles-grafana) * [BUGFIX] fix: skip per-label limiter and sanitizer for target_info and host_info metrics in metrics-generator [#6660](https://github.com/grafana/tempo/pull/6660) (@electron0zero) * [BUGFIX] fix(traceql): err on division by zero [#6580](https://github.com/grafana/tempo/pull/6580) (@Proximyst) +* [BUGFIX] Return 400 instead of 500 when query_range or query_instant requests have unparseable start/end parameters [#6694](https://github.com/grafana/tempo/pull/6694) (@ruslan-mikhailov) ### 3.0 Cleanup diff --git a/pkg/api/http.go b/pkg/api/http.go index 8f3f69fb6fa..08d7acc576e 100644 --- a/pkg/api/http.go +++ b/pkg/api/http.go @@ -304,7 +304,10 @@ func ParseQueryInstantRequest(r *http.Request) (*tempopb.QueryInstantRequest, er req.Query = s } - start, end, _ := bounds(vals) + start, end, err := bounds(vals) + if err != nil { + return nil, httpgrpc.Error(http.StatusBadRequest, err.Error()) + } req.Start = uint64(start.UnixNano()) req.End = uint64(end.UnixNano()) @@ -329,7 +332,10 @@ func ParseQueryRangeRequest(r *http.Request) (*tempopb.QueryRangeRequest, error) req.QueryMode = s } - start, end, _ := bounds(vals) + start, end, err := bounds(vals) + if err != nil { + return nil, httpgrpc.Error(http.StatusBadRequest, err.Error()) + } req.Start = uint64(start.UnixNano()) req.End = uint64(end.UnixNano()) diff --git a/pkg/api/http_test.go b/pkg/api/http_test.go index 19ade3d366f..6886a9c1201 100644 --- a/pkg/api/http_test.go +++ b/pkg/api/http_test.go @@ -696,6 +696,41 @@ func Test_parseTimestamp(t *testing.T) { } } +func TestParseQueryRequestInvalidBounds(t *testing.T) { + tests := []struct { + name string + params map[string]string + errMsg string + }{ + { + name: "invalid start", + params: map[string]string{"q": "{} | rate()", "start": "abc", "end": "1700000000"}, + errMsg: "could not parse 'start' parameter", + }, + { + name: "invalid end", + params: map[string]string{"q": "{} | rate()", "start": "1700000000", "end": "bca"}, + errMsg: "could not parse 'end' parameter", + }, + } + + for _, tt := range tests { + t.Run("range/"+tt.name, func(t *testing.T) { + r := makeReq(tt.params) + _, err := ParseQueryRangeRequest(r) + require.Error(t, err) + assert.Contains(t, err.Error(), tt.errMsg) + }) + + t.Run("instant/"+tt.name, func(t *testing.T) { + r := makeReq(tt.params) + _, err := ParseQueryInstantRequest(r) + require.Error(t, err) + assert.Contains(t, err.Error(), tt.errMsg) + }) + } +} + func TestQueryRangeRoundtripEmpty(t *testing.T) { req := &tempopb.QueryRangeRequest{ Step: uint64(time.Second), // you can't actually roundtrip an empty query b/c Build/Parse will force a default step