diff --git a/api/traces/v1/trace_rbac.go b/api/traces/v1/trace_rbac.go index 2554446c0..789e89f2e 100644 --- a/api/traces/v1/trace_rbac.go +++ b/api/traces/v1/trace_rbac.go @@ -11,8 +11,8 @@ import ( "github.com/go-kit/log" "github.com/go-kit/log/level" - "github.com/golang/protobuf/jsonpb" // nolint:staticcheck - "github.com/golang/protobuf/proto" //nolint:staticcheck + "github.com/gogo/protobuf/jsonpb" + "github.com/gogo/protobuf/proto" "github.com/grafana/tempo/pkg/tempopb" commonv1 "github.com/grafana/tempo/pkg/tempopb/common/v1" tracev1 "github.com/grafana/tempo/pkg/tempopb/trace/v1" @@ -84,6 +84,9 @@ func WithTraceQLNamespaceSelectAndForbidOtherAPIs(enabled bool) func(http.Handle } } +// unmarshal and marshal use github.com/gogo/protobuf/jsonpb for the JSON +// path, since golang/protobuf/jsonpb drops empty repeated fields on +// round-trip for these gogo-generated tempopb types (TRACING-6841). func unmarshal(response *http.Response, body []byte, pb proto.Message) error { switch response.Header.Get(HeaderContentType) { case ContentTypeProtobuf: @@ -163,6 +166,11 @@ func responseRBACModifier(log log.Logger) func(response *http.Response) error { } case routeQueryV2.MatchString(request.URL.Path): + // unlike routeQueryV1 above, this path can use the shared + // unmarshal/marshal helpers directly: TraceByIDResponse + // wraps a *Trace plus Metrics/Status/Message fields, and + // (with the gogo/protobuf/jsonpb switch above) round-trips + // correctly without needing the JSONV1 special-casing. traceByIDResponse := &tempopb.TraceByIDResponse{} err = unmarshal(response, b, traceByIDResponse) if err != nil { diff --git a/api/traces/v1/trace_rbac_test.go b/api/traces/v1/trace_rbac_test.go index a277b5bea..e86085b24 100644 --- a/api/traces/v1/trace_rbac_test.go +++ b/api/traces/v1/trace_rbac_test.go @@ -546,6 +546,42 @@ func TestRBACSearchResult(t *testing.T) { } } +// TestSearchResponseEmptyTracesPreserved is a regression test for TRACING-6841: +// a zero-result Tempo search response must round-trip through the JSON +// unmarshal/marshal cycle with "traces": [] intact, not silently dropped. +func TestSearchResponseEmptyTracesPreserved(t *testing.T) { + resp := &http.Response{Header: http.Header{}} + + searchResponse := &tempopb.SearchResponse{} + in := []byte(`{"traces":[]}`) + require.NoError(t, unmarshal(resp, in, searchResponse)) + require.NotNil(t, searchResponse.Traces, "an explicit empty traces array must not collapse to nil on unmarshal") + + buf := &bytes.Buffer{} + require.NoError(t, marshal(resp, buf, searchResponse)) + assert.JSONEq(t, `{"traces":[]}`, buf.String()) +} + +// TestTraceByIDResponseEmptyResourceSpansPreserved is the routeQueryV2 +// counterpart to TestSearchResponseEmptyTracesPreserved: TraceByIDResponse is +// also gogo/protobuf-generated, and its embedded Trace.ResourceSpans is a +// repeated field subject to the exact same golang/protobuf/jsonpb round-trip +// bug (see the comment on unmarshal/marshal). Before this fix, unmarshal/ +// marshal used github.com/golang/protobuf/jsonpb for this type too. +func TestTraceByIDResponseEmptyResourceSpansPreserved(t *testing.T) { + resp := &http.Response{Header: http.Header{}} + + traceByIDResponse := &tempopb.TraceByIDResponse{} + in := []byte(`{"trace":{"resourceSpans":[]}}`) + require.NoError(t, unmarshal(resp, in, traceByIDResponse)) + require.NotNil(t, traceByIDResponse.Trace, "trace field must be present") + require.NotNil(t, traceByIDResponse.Trace.ResourceSpans, "an explicit empty resourceSpans array must not collapse to nil on unmarshal") + + buf := &bytes.Buffer{} + require.NoError(t, marshal(resp, buf, traceByIDResponse)) + assert.JSONEq(t, `{"trace":{"resourceSpans":[]}}`, buf.String()) +} + func contextWithAllowedNamespaces(t *testing.T, namespaces []string) context.Context { t.Helper() data := fmt.Sprintf(`{"matchers":[{"name":"namespace","value":"%s","type":1}]}`, url.QueryEscape(strings.Join(namespaces, "|"))) @@ -702,7 +738,9 @@ func TestResponseRBACModifier(t *testing.T) { ] }, "scopeSpans": [ - {"scope": {}, "spans": [{}]} + {"scope": {"attributes": []}, "spans": [ + {"attributes": [], "events": []} + ]} ] } ] @@ -756,6 +794,23 @@ func TestResponseRBACModifier(t *testing.T) { }`, string(body)) }) + t.Run("search endpoint with zero results keeps traces as an empty array", func(t *testing.T) { + // TRACING-6841: a zero-result Tempo search response must not lose its + // "traces" key while passing through RBAC filtering. + resp := makeResponse(ctx, http.StatusOK, "/api/search", `{ + "traces": [], + "metrics": {"inspectedBytes": "254867"} +}`, nil) + + require.NoError(t, modifier(resp)) + + body, _ := io.ReadAll(resp.Body) + assert.JSONEq(t, `{ + "traces": [], + "metrics": {"inspectedBytes": "254867"} +}`, string(body)) + }) + t.Run("search tag values endpoint is not modified", func(t *testing.T) { resp := makeResponse(ctx, http.StatusOK, "/api/v2/search/tag/resource.service.name/values", `{ "tagValues": [ diff --git a/go.mod b/go.mod index 174f9ebaf..d16858ee1 100644 --- a/go.mod +++ b/go.mod @@ -13,6 +13,7 @@ require ( github.com/go-kit/log v0.2.1 github.com/go-openapi/runtime v0.33.0 github.com/go-openapi/strfmt v0.27.0 + github.com/gogo/protobuf v1.3.2 github.com/golang-jwt/jwt/v4 v4.5.2 github.com/golang/protobuf v1.5.4 github.com/gorilla/websocket v1.5.4-0.20250319132907-e064f32e3674 @@ -94,7 +95,6 @@ require ( github.com/go-viper/mapstructure/v2 v2.5.0 // indirect github.com/gobwas/glob v0.2.3 // indirect github.com/goccy/go-json v0.10.6 // indirect - github.com/gogo/protobuf v1.3.2 // indirect github.com/golang-jwt/jwt/v5 v5.3.1 // indirect github.com/google/gnostic-models v0.7.0 // indirect github.com/google/go-cmp v0.7.0 // indirect