diff --git a/pkg/featureflags/client_capability.go b/pkg/featureflags/client_capability.go new file mode 100644 index 0000000000..f8f5c1c7f0 --- /dev/null +++ b/pkg/featureflags/client_capability.go @@ -0,0 +1,117 @@ +package featureflags + +import ( + "context" + "mime" + "net/http" + "strings" + + "connectrpc.com/connect" + "github.com/go-kit/log/level" + "github.com/grafana/dskit/middleware" + "github.com/grafana/pyroscope/pkg/util" + "google.golang.org/grpc" + "google.golang.org/grpc/metadata" +) + +const ( + // Capability names - update parseClientCapabilities below when new capabilities added + allowUtf8LabelNamesCapabilityName string = "allow-utf8-labelnames" +) + +// Define a custom context key type to avoid collisions +type contextKey struct{} + +type ClientCapabilities struct { + AllowUtf8LabelNames bool +} + +func WithClientCapabilities(ctx context.Context, clientCapabilities ClientCapabilities) context.Context { + return context.WithValue(ctx, contextKey{}, clientCapabilities) +} + +func GetClientCapabilities(ctx context.Context) (ClientCapabilities, bool) { + value, ok := ctx.Value(contextKey{}).(ClientCapabilities) + return value, ok +} + +func ClientCapabilitiesGRPCMiddleware() grpc.UnaryServerInterceptor { + return func( + ctx context.Context, + req interface{}, + info *grpc.UnaryServerInfo, + handler grpc.UnaryHandler, + ) (interface{}, error) { + // Extract metadata from context + md, ok := metadata.FromIncomingContext(ctx) + if !ok { + return handler(ctx, req) + } + + // Convert metadata to http.Header for reuse of existing parsing logic + httpHeader := make(http.Header) + for key, values := range md { + // gRPC metadata keys are lowercase, HTTP headers are case-insensitive + httpHeader[http.CanonicalHeaderKey(key)] = values + } + + // Reuse existing HTTP header parsing + clientCapabilities, err := parseClientCapabilities(httpHeader) + if err != nil { + return nil, connect.NewError(connect.CodeInvalidArgument, err) + } + + enhancedCtx := WithClientCapabilities(ctx, clientCapabilities) + return handler(enhancedCtx, req) + } +} + +// ClientCapabilitiesHttpMiddleware creates middleware that extracts and parses the +// `Accept` header for capabilities the client supports +func ClientCapabilitiesHttpMiddleware() middleware.Interface { + return middleware.Func(func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + clientCapabilities, err := parseClientCapabilities(r.Header) + if err != nil { + http.Error(w, "Invalid header format: "+err.Error(), http.StatusBadRequest) + return + } + + ctx := WithClientCapabilities(r.Context(), clientCapabilities) + next.ServeHTTP(w, r.WithContext(ctx)) + }) + }) +} + +func parseClientCapabilities(header http.Header) (ClientCapabilities, error) { + acceptHeaderValues := header.Values("Accept") + + var capabilities ClientCapabilities + + for _, acceptHeaderValue := range acceptHeaderValues { + if acceptHeaderValue != "" { + accepts := strings.Split(acceptHeaderValue, ",") + + for _, accept := range accepts { + if _, params, err := mime.ParseMediaType(accept); err != nil { + return capabilities, err + } else { + for k, v := range params { + switch k { + case allowUtf8LabelNamesCapabilityName: + if v == "true" { + capabilities.AllowUtf8LabelNames = true + } + default: + level.Debug(util.Logger).Log( + "msg", "unknown capability parsed from Accept header", + "acceptHeaderKey", k, + "acceptHeaderValue", v) + } + } + } + } + } + } + return capabilities, nil +} diff --git a/pkg/featureflags/client_capability_test.go b/pkg/featureflags/client_capability_test.go new file mode 100644 index 0000000000..85d284deca --- /dev/null +++ b/pkg/featureflags/client_capability_test.go @@ -0,0 +1,239 @@ +package featureflags + +import ( + "net/http" + "testing" + + "github.com/stretchr/testify/require" +) + +func Test_parseClientCapabilities(t *testing.T) { + tests := []struct { + Name string + Header http.Header + Want ClientCapabilities + WantError bool + ErrorMessage string + }{ + { + Name: "empty header returns default capabilities", + Header: http.Header{}, + Want: ClientCapabilities{AllowUtf8LabelNames: false}, + }, + { + Name: "no Accept header returns default capabilities", + Header: http.Header{ + "Content-Type": []string{"application/json"}, + }, + Want: ClientCapabilities{AllowUtf8LabelNames: false}, + }, + { + Name: "empty Accept header value returns default capabilities", + Header: http.Header{ + "Accept": []string{""}, + }, + Want: ClientCapabilities{AllowUtf8LabelNames: false}, + }, + { + Name: "simple Accept header without capabilities", + Header: http.Header{ + "Accept": []string{"application/json"}, + }, + Want: ClientCapabilities{AllowUtf8LabelNames: false}, + }, + { + Name: "Accept header with utf8 label names capability true", + Header: http.Header{ + "Accept": []string{"*/*; allow-utf8-labelnames=true"}, + }, + Want: ClientCapabilities{AllowUtf8LabelNames: true}, + }, + { + Name: "Accept header with utf8 label names capability false", + Header: http.Header{ + "Accept": []string{"*/*; allow-utf8-labelnames=false"}, + }, + Want: ClientCapabilities{AllowUtf8LabelNames: false}, + }, + { + Name: "Accept header with utf8 label names capability invalid value", + Header: http.Header{ + "Accept": []string{"*/*; allow-utf8-labelnames=invalid"}, + }, + Want: ClientCapabilities{AllowUtf8LabelNames: false}, + }, + { + Name: "Accept header with unknown capability", + Header: http.Header{ + "Accept": []string{"*/*; unknown-capability=true"}, + }, + Want: ClientCapabilities{AllowUtf8LabelNames: false}, + }, + { + Name: "Accept header with multiple capabilities", + Header: http.Header{ + "Accept": []string{"*/*; allow-utf8-labelnames=true; unknown-capability=false"}, + }, + Want: ClientCapabilities{AllowUtf8LabelNames: true}, + }, + { + Name: "multiple Accept header values", + Header: http.Header{ + "Accept": []string{"application/json", "*/*; allow-utf8-labelnames=true"}, + }, + Want: ClientCapabilities{AllowUtf8LabelNames: true}, + }, + { + Name: "multiple Accept header values with different capabilities", + Header: http.Header{ + "Accept": []string{ + "application/json; allow-utf8-labelnames=false", + "*/*; allow-utf8-labelnames=true", + }, + }, + Want: ClientCapabilities{AllowUtf8LabelNames: true}, + }, + { + Name: "Accept header with quality values", + Header: http.Header{ + "Accept": []string{"text/html; q=0.9; allow-utf8-labelnames=true"}, + }, + Want: ClientCapabilities{AllowUtf8LabelNames: true}, + }, + { + Name: "complex Accept header", + Header: http.Header{ + "Accept": []string{ + "text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8;allow-utf8-labelnames=true", + }, + }, + Want: ClientCapabilities{AllowUtf8LabelNames: true}, + }, + { + Name: "multiple Accept header entries", + Header: http.Header{ + "Accept": []string{ + "application/json", + "text/plain; allow-utf8-labelnames=true", + "*/*; q=0.1", + }, + }, + Want: ClientCapabilities{AllowUtf8LabelNames: true}, + }, + { + Name: "invalid mime type in Accept header", + Header: http.Header{ + "Accept": []string{"invalid/mime/type/format"}, + }, + Want: ClientCapabilities{}, + WantError: true, + ErrorMessage: "mime: unexpected content after media subtype", + }, + { + Name: "Accept header with invalid syntax", + Header: http.Header{ + "Accept": []string{"text/html; invalid-parameter-syntax"}, + }, + Want: ClientCapabilities{}, + WantError: true, + ErrorMessage: "mime: invalid media parameter", + }, + { + Name: "mixed valid and invalid Accept header values", + Header: http.Header{ + "Accept": []string{ + "application/json", + "invalid/mime/type/format", + }, + }, + Want: ClientCapabilities{}, + WantError: true, + ErrorMessage: "mime: unexpected content after media subtype", + }, + { + // Parameter names are case-insensitive in mime.ParseMediaType + Name: "case sensitivity test for capability name", + Header: http.Header{ + "Accept": []string{"*/*; Allow-Utf8-Labelnames=true"}, + }, + Want: ClientCapabilities{AllowUtf8LabelNames: true}, + }, + { + Name: "whitespace handling in Accept header", + Header: http.Header{ + "Accept": []string{" application/json ; allow-utf8-labelnames=true "}, + }, + Want: ClientCapabilities{AllowUtf8LabelNames: true}, + }, + } + + for _, tt := range tests { + t.Run(tt.Name, func(t *testing.T) { + t.Parallel() + + got, err := parseClientCapabilities(tt.Header) + + if tt.WantError { + require.Error(t, err) + if tt.ErrorMessage != "" { + require.Contains(t, err.Error(), tt.ErrorMessage) + } + return + } + + require.NoError(t, err) + require.Equal(t, tt.Want, got) + }) + } +} + +func Test_parseClientCapabilities_MultipleCapabilities(t *testing.T) { + // This test specifically checks that when the same capability appears + // multiple times with different values, the last "true" value wins + tests := []struct { + Name string + Header http.Header + Want ClientCapabilities + }{ + { + Name: "capability appears multiple times - last true wins", + Header: http.Header{ + "Accept": []string{ + "application/json; allow-utf8-labelnames=false", + "text/plain; allow-utf8-labelnames=true", + }, + }, + Want: ClientCapabilities{AllowUtf8LabelNames: true}, + }, + { + Name: "capability appears multiple times - last false loses to earlier true", + Header: http.Header{ + "Accept": []string{ + "application/json; allow-utf8-labelnames=true", + "text/plain; allow-utf8-labelnames=false", + }, + }, + Want: ClientCapabilities{AllowUtf8LabelNames: true}, + }, + { + Name: "capability appears multiple times - all false", + Header: http.Header{ + "Accept": []string{ + "application/json; allow-utf8-labelnames=false", + "text/plain; allow-utf8-labelnames=false", + }, + }, + Want: ClientCapabilities{AllowUtf8LabelNames: false}, + }, + } + + for _, tt := range tests { + t.Run(tt.Name, func(t *testing.T) { + t.Parallel() + + got, err := parseClientCapabilities(tt.Header) + require.NoError(t, err) + require.Equal(t, tt.Want, got) + }) + } +} diff --git a/pkg/frontend/readpath/queryfrontend/query_frontend_test.go b/pkg/frontend/readpath/queryfrontend/query_frontend_test.go index b6ee685f90..c88f641b1b 100644 --- a/pkg/frontend/readpath/queryfrontend/query_frontend_test.go +++ b/pkg/frontend/readpath/queryfrontend/query_frontend_test.go @@ -4,7 +4,9 @@ import ( "context" "fmt" "testing" + "time" + "connectrpc.com/connect" "github.com/go-kit/log" "github.com/grafana/dskit/user" "github.com/stretchr/testify/assert" @@ -13,8 +15,11 @@ import ( profilev1 "github.com/grafana/pyroscope/api/gen/proto/go/google/v1" metastorev1 "github.com/grafana/pyroscope/api/gen/proto/go/metastore/v1" + querierv1 "github.com/grafana/pyroscope/api/gen/proto/go/querier/v1" queryv1 "github.com/grafana/pyroscope/api/gen/proto/go/query/v1" + typesv1 "github.com/grafana/pyroscope/api/gen/proto/go/types/v1" "github.com/grafana/pyroscope/pkg/block/metadata" + "github.com/grafana/pyroscope/pkg/featureflags" "github.com/grafana/pyroscope/pkg/tenant" "github.com/grafana/pyroscope/pkg/test/mocks/mockfrontend" "github.com/grafana/pyroscope/pkg/test/mocks/mockmetastorev1" @@ -224,3 +229,251 @@ func createProfile(t *testing.T) []byte { require.NoError(t, err) return bytes } + +func Test_QueryFrontend_LabelNames_WithFiltering(t *testing.T) { + tests := []struct { + name string + allowUtf8LabelNames bool + setCapabilities bool + backendLabelNames []string + expectedLabelNames []string + }{ + { + name: "UTF8 labels allowed when enabled", + allowUtf8LabelNames: true, + setCapabilities: true, + backendLabelNames: []string{"foo", "bar", "世界"}, + expectedLabelNames: []string{"foo", "bar", "世界"}, + }, + { + name: "UTF8 labels filtered when disabled", + allowUtf8LabelNames: false, + setCapabilities: true, + backendLabelNames: []string{"foo", "bar", "世界"}, + expectedLabelNames: []string{"foo", "bar"}, + }, + { + name: "invalid labels pass through when UTF8 enabled", + allowUtf8LabelNames: true, + setCapabilities: true, + backendLabelNames: []string{"valid_name", "123invalid", "invalid-hyphen", "世界"}, + expectedLabelNames: []string{"valid_name", "123invalid", "invalid-hyphen", "世界"}, + }, + { + name: "invalid labels filtered when UTF8 disabled", + allowUtf8LabelNames: false, + setCapabilities: true, + backendLabelNames: []string{"valid_name", "123invalid", "invalid-hyphen", "世界"}, + expectedLabelNames: []string{"valid_name"}, + }, + { + name: "filtering enabled when no capabilities set", + setCapabilities: false, + backendLabelNames: []string{"valid_name", "123invalid", "世界"}, + expectedLabelNames: []string{"valid_name"}, + }, + { + name: "labels with dots pass through", + allowUtf8LabelNames: false, + setCapabilities: true, + backendLabelNames: []string{"service.name", "app.version"}, + expectedLabelNames: []string{"service.name", "app.version"}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + mockQueryBackend := mockqueryfrontend.NewMockQueryBackend(t) + mockQueryBackend.On("Invoke", mock.Anything, mock.Anything).Return(&queryv1.InvokeResponse{ + Reports: []*queryv1.Report{ + { + ReportType: queryv1.ReportType_REPORT_LABEL_NAMES, + LabelNames: &queryv1.LabelNamesReport{ + LabelNames: tc.backendLabelNames, + }, + }, + }, + }, nil) + + mockLimits := mockfrontend.NewMockLimits(t) + mockLimits.On("MaxQueryLookback", "test-tenant").Return(time.Duration(0)) + mockLimits.On("MaxQueryLength", "test-tenant").Return(time.Duration(0)) + mockLimits.On("QuerySanitizeOnMerge", "test-tenant").Return(true) + mockMetadataClient := new(mockmetastorev1.MockMetadataQueryServiceClient) + mockMetadataClient.On("QueryMetadata", mock.Anything, mock.Anything).Return(&metastorev1.QueryMetadataResponse{ + Blocks: []*metastorev1.BlockMeta{{Id: "test-block"}}, + }, nil) + + qf := NewQueryFrontend( + log.NewNopLogger(), + mockLimits, + mockMetadataClient, + nil, + mockQueryBackend, + nil, + ) + + ctx := tenant.InjectTenantID(context.Background(), "test-tenant") + if tc.setCapabilities { + ctx = featureflags.WithClientCapabilities(ctx, featureflags.ClientCapabilities{ + AllowUtf8LabelNames: tc.allowUtf8LabelNames, + }) + } + + req := connect.NewRequest(&typesv1.LabelNamesRequest{ + Start: 1000, + End: 2000, + }) + + resp, err := qf.LabelNames(ctx, req) + require.NoError(t, err) + require.Equal(t, tc.expectedLabelNames, resp.Msg.Names) + }) + } +} + +func Test_QueryFrontend_Series_WithLabelNameFiltering(t *testing.T) { + tests := []struct { + name string + allowUtf8LabelNames bool + setCapabilities bool + requestLabelNames []string + backendLabelNames []string // For empty request case + expectedQueryRequest []string // What should be passed to backend + }{ + { + name: "all label names pass through when UTF8 enabled", + allowUtf8LabelNames: true, + setCapabilities: true, + requestLabelNames: []string{"valid_name", "123invalid", "invalid-hyphen", "世界"}, + expectedQueryRequest: []string{"valid_name", "123invalid", "invalid-hyphen", "世界"}, + }, + { + name: "invalid label names filtered when UTF8 disabled", + allowUtf8LabelNames: false, + setCapabilities: true, + requestLabelNames: []string{"valid_name", "123invalid", "invalid-hyphen", "世界"}, + expectedQueryRequest: []string{"valid_name"}, + }, + { + name: "UTF8 labels filtered when UTF8 disabled", + allowUtf8LabelNames: false, + setCapabilities: true, + requestLabelNames: []string{"foo", "bar", "世界", "日本語"}, + expectedQueryRequest: []string{"foo", "bar"}, + }, + { + name: "filtering enabled when no capabilities set", + setCapabilities: false, + requestLabelNames: []string{"foo", "123invalid", "世界"}, + expectedQueryRequest: []string{"foo"}, + }, + { + name: "all valid labels pass through", + allowUtf8LabelNames: false, + setCapabilities: true, + requestLabelNames: []string{"foo", "bar", "service_name"}, + expectedQueryRequest: []string{"foo", "bar", "service_name"}, + }, + { + name: "labels with dots pass through", + allowUtf8LabelNames: false, + setCapabilities: true, + requestLabelNames: []string{"service.name", "app.version"}, + expectedQueryRequest: []string{"service.name", "app.version"}, + }, + { + name: "empty label names with UTF8 disabled queries and filters all labels", + allowUtf8LabelNames: false, + setCapabilities: true, + requestLabelNames: []string{}, + backendLabelNames: []string{"foo", "bar", "世界"}, + expectedQueryRequest: []string{"foo", "bar"}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + var capturedLabelNames []string + + mockQueryBackend := mockqueryfrontend.NewMockQueryBackend(t) + + // For empty label names case, we need to mock the LabelNames query first + if len(tc.requestLabelNames) == 0 { + mockQueryBackend.On("Invoke", mock.Anything, mock.MatchedBy(func(req *queryv1.InvokeRequest) bool { + return len(req.Query) > 0 && req.Query[0].QueryType == queryv1.QueryType_QUERY_LABEL_NAMES + })).Return(&queryv1.InvokeResponse{ + Reports: []*queryv1.Report{ + { + ReportType: queryv1.ReportType_REPORT_LABEL_NAMES, + LabelNames: &queryv1.LabelNamesReport{ + LabelNames: tc.backendLabelNames, + }, + }, + }, + }, nil).Once() + } + + // Mock the Series query specifically + mockQueryBackend.On("Invoke", mock.Anything, mock.MatchedBy(func(req *queryv1.InvokeRequest) bool { + return len(req.Query) > 0 && req.Query[0].QueryType == queryv1.QueryType_QUERY_SERIES_LABELS + })).Run(func(args mock.Arguments) { + invReq := args.Get(1).(*queryv1.InvokeRequest) + if len(invReq.Query) > 0 && invReq.Query[0].SeriesLabels != nil { + capturedLabelNames = invReq.Query[0].SeriesLabels.LabelNames + if capturedLabelNames == nil { + capturedLabelNames = []string{} + } + } + }).Return(&queryv1.InvokeResponse{ + Reports: []*queryv1.Report{ + { + ReportType: queryv1.ReportType_REPORT_SERIES_LABELS, + SeriesLabels: &queryv1.SeriesLabelsReport{ + SeriesLabels: []*typesv1.Labels{}, + }, + }, + }, + }, nil).Once() + + mockLimits := mockfrontend.NewMockLimits(t) + mockLimits.On("MaxQueryLookback", "test-tenant").Return(time.Duration(0)) + mockLimits.On("MaxQueryLength", "test-tenant").Return(time.Duration(0)) + mockLimits.On("QuerySanitizeOnMerge", "test-tenant").Return(true) + mockMetadataClient := new(mockmetastorev1.MockMetadataQueryServiceClient) + mockMetadataClient.On("QueryMetadata", mock.Anything, mock.Anything).Return(&metastorev1.QueryMetadataResponse{ + Blocks: []*metastorev1.BlockMeta{{Id: "test-block"}}, + }, nil) + + qf := NewQueryFrontend( + log.NewNopLogger(), + mockLimits, + mockMetadataClient, + nil, + mockQueryBackend, + nil, + ) + + ctx := tenant.InjectTenantID(context.Background(), "test-tenant") + if tc.setCapabilities { + ctx = featureflags.WithClientCapabilities(ctx, featureflags.ClientCapabilities{ + AllowUtf8LabelNames: tc.allowUtf8LabelNames, + }) + } + + req := connect.NewRequest(&querierv1.SeriesRequest{ + Matchers: []string{`{service_name="test"}`}, + LabelNames: tc.requestLabelNames, + Start: 1000, + End: 2000, + }) + + _, err := qf.Series(ctx, req) + require.NoError(t, err) + + // Verify that the label names were filtered correctly before being sent to backend + require.Equal(t, tc.expectedQueryRequest, capturedLabelNames, + "Expected label names sent to backend to be %v, but got %v", tc.expectedQueryRequest, capturedLabelNames) + }) + } +} diff --git a/pkg/frontend/readpath/queryfrontend/query_label_names.go b/pkg/frontend/readpath/queryfrontend/query_label_names.go index c430ebb310..18e2f0b777 100644 --- a/pkg/frontend/readpath/queryfrontend/query_label_names.go +++ b/pkg/frontend/readpath/queryfrontend/query_label_names.go @@ -4,10 +4,12 @@ import ( "context" "connectrpc.com/connect" + "github.com/go-kit/log/level" "github.com/grafana/dskit/tenant" queryv1 "github.com/grafana/pyroscope/api/gen/proto/go/query/v1" typesv1 "github.com/grafana/pyroscope/api/gen/proto/go/types/v1" + "github.com/grafana/pyroscope/pkg/featureflags" "github.com/grafana/pyroscope/pkg/validation" ) @@ -46,5 +48,20 @@ func (q *QueryFrontend) LabelNames( if report == nil { return connect.NewResponse(&typesv1.LabelNamesResponse{}), nil } - return connect.NewResponse(&typesv1.LabelNamesResponse{Names: report.LabelNames.LabelNames}), nil + + labelNames := report.LabelNames.LabelNames + if capabilities, ok := featureflags.GetClientCapabilities(ctx); !ok || !capabilities.AllowUtf8LabelNames { + // Filter out label names not passing legacy validation if utf8 label names not enabled + filteredLabelNames := make([]string, 0, len(labelNames)) + for _, labelName := range labelNames { + if _, _, ok := validation.SanitizeLegacyLabelName(labelName); !ok { + level.Debug(q.logger).Log("msg", "filtering out label", "label_name", labelName) + continue + } + filteredLabelNames = append(filteredLabelNames, labelName) + } + labelNames = filteredLabelNames + } + + return connect.NewResponse(&typesv1.LabelNamesResponse{Names: labelNames}), nil } diff --git a/pkg/frontend/readpath/queryfrontend/query_series_labels.go b/pkg/frontend/readpath/queryfrontend/query_series_labels.go index 41589017da..763ec92fe3 100644 --- a/pkg/frontend/readpath/queryfrontend/query_series_labels.go +++ b/pkg/frontend/readpath/queryfrontend/query_series_labels.go @@ -7,11 +7,55 @@ import ( "github.com/go-kit/log/level" "github.com/grafana/dskit/tenant" + typesv1 "github.com/grafana/pyroscope/api/gen/proto/go/types/v1" + querierv1 "github.com/grafana/pyroscope/api/gen/proto/go/querier/v1" queryv1 "github.com/grafana/pyroscope/api/gen/proto/go/query/v1" + "github.com/grafana/pyroscope/pkg/featureflags" "github.com/grafana/pyroscope/pkg/validation" ) +func (q *QueryFrontend) filterLabelNames( + ctx context.Context, + c *connect.Request[querierv1.SeriesRequest], + labelNames []string, + matchers []string, +) ([]string, error) { + if capabilities, ok := featureflags.GetClientCapabilities(ctx); ok && capabilities.AllowUtf8LabelNames { + return labelNames, nil + } + + toFilter := make([]string, len(labelNames)) + copy(toFilter, labelNames) + + // Filter out label names not passing legacy validation if utf8 label names not enabled + if len(labelNames) == 0 { + // Querying for all label names; must retrieve all label names to then filter out + response, err := q.LabelNames(ctx, connect.NewRequest(&typesv1.LabelNamesRequest{ + Start: c.Msg.Start, + End: c.Msg.End, + Matchers: matchers, + })) + + if err != nil { + return nil, err + } + if response != nil { + toFilter = response.Msg.Names + } + } + + filtered := make([]string, 0, len(labelNames)) + for _, name := range toFilter { + if _, _, ok := validation.SanitizeLegacyLabelName(name); !ok { + level.Debug(q.logger).Log("msg", "filtering out label", "label_name", name) + continue + } + filtered = append(filtered, name) + } + return filtered, nil +} + func (q *QueryFrontend) Series( ctx context.Context, c *connect.Request[querierv1.SeriesRequest], @@ -37,6 +81,12 @@ func (q *QueryFrontend) Series( if err != nil { return nil, connect.NewError(connect.CodeInvalidArgument, err) } + + labelNames, err := q.filterLabelNames(ctx, c, c.Msg.LabelNames, c.Msg.Matchers) + if err != nil { + return nil, err + } + report, err := q.querySingle(ctx, &queryv1.QueryRequest{ StartTime: c.Msg.Start, EndTime: c.Msg.End, @@ -44,7 +94,7 @@ func (q *QueryFrontend) Series( Query: []*queryv1.Query{{ QueryType: queryv1.QueryType_QUERY_SERIES_LABELS, SeriesLabels: &queryv1.SeriesLabelsQuery{ - LabelNames: c.Msg.LabelNames, + LabelNames: labelNames, }, }}, }) @@ -54,5 +104,6 @@ func (q *QueryFrontend) Series( if report == nil { return connect.NewResponse(&querierv1.SeriesResponse{}), nil } + return connect.NewResponse(&querierv1.SeriesResponse{LabelsSet: report.SeriesLabels.SeriesLabels}), nil } diff --git a/pkg/pyroscope/modules.go b/pkg/pyroscope/modules.go index b2bf670924..0a2db5e030 100644 --- a/pkg/pyroscope/modules.go +++ b/pkg/pyroscope/modules.go @@ -42,6 +42,7 @@ import ( "github.com/grafana/pyroscope/pkg/compactor" "github.com/grafana/pyroscope/pkg/distributor" "github.com/grafana/pyroscope/pkg/embedded/grafana" + "github.com/grafana/pyroscope/pkg/featureflags" "github.com/grafana/pyroscope/pkg/ingester" objstoreclient "github.com/grafana/pyroscope/pkg/objstore/client" "github.com/grafana/pyroscope/pkg/objstore/providers/filesystem" @@ -464,7 +465,10 @@ func (f *Pyroscope) initServer() (services.Service, error) { // see https://github.com/grafana/pyroscope/issues/231 f.Cfg.Server.DoNotAddDefaultHTTPMiddleware = true f.Cfg.Server.ExcludeRequestInLog = true // gRPC-specific. - f.Cfg.Server.GRPCMiddleware = append(f.Cfg.Server.GRPCMiddleware, util.RecoveryInterceptorGRPC) + f.Cfg.Server.GRPCMiddleware = append(f.Cfg.Server.GRPCMiddleware, + util.RecoveryInterceptorGRPC, + featureflags.ClientCapabilitiesGRPCMiddleware(), + ) if f.Cfg.V2 { f.Cfg.Server.MetricsNativeHistogramFactor = 1.1 // 10% increase from bucket to bucket @@ -483,7 +487,6 @@ func (f *Pyroscope) initServer() (services.Service, error) { f.Cfg.Server.HTTPServerReadTimeout = 2 * f.Cfg.Server.HTTPServerReadTimeout f.Cfg.Server.HTTPServerWriteTimeout = 2 * f.Cfg.Server.HTTPServerWriteTimeout } - var err error if f.Server, err = server.New(f.Cfg.Server); err != nil { return nil, err @@ -523,6 +526,7 @@ func (f *Pyroscope) initServer() (services.Service, error) { httpMetric, objstoreTracerMiddleware, httputil.K6Middleware(), + featureflags.ClientCapabilitiesHttpMiddleware(), } if f.Cfg.SelfProfiling.UseK6Middleware { defaultHTTPMiddleware = append(defaultHTTPMiddleware, httputil.K6Middleware()) diff --git a/pkg/querier/querier.go b/pkg/querier/querier.go index 2397092e71..cebf26a5f7 100644 --- a/pkg/querier/querier.go +++ b/pkg/querier/querier.go @@ -27,6 +27,8 @@ import ( "github.com/samber/lo" "golang.org/x/sync/errgroup" + "github.com/grafana/pyroscope/pkg/featureflags" + googlev1 "github.com/grafana/pyroscope/api/gen/proto/go/google/v1" ingestv1 "github.com/grafana/pyroscope/api/gen/proto/go/ingester/v1" querierv1 "github.com/grafana/pyroscope/api/gen/proto/go/querier/v1" @@ -271,6 +273,18 @@ func (q *Querier) LabelValues(ctx context.Context, req *connect.Request[typesv1. }), nil } +func filterLabelNames(labelNames []string) []string { + filtered := make([]string, 0, len(labelNames)) + // Filter out label names not passing legacy validation if utf8 label names not enabled + for _, labelName := range labelNames { + if _, _, ok := validation.SanitizeLegacyLabelName(labelName); !ok { + continue + } + filtered = append(filtered, labelName) + } + return filtered +} + func (q *Querier) LabelNames(ctx context.Context, req *connect.Request[typesv1.LabelNamesRequest]) (*connect.Response[typesv1.LabelNamesResponse], error) { sp, ctx := opentracing.StartSpanFromContext(ctx, "LabelNames") defer sp.Finish() @@ -288,8 +302,15 @@ func (q *Querier) LabelNames(ctx context.Context, req *connect.Request[typesv1.L if err != nil { return nil, err } + + labelNames := uniqueSortedStrings(responses) + if capabilities, ok := featureflags.GetClientCapabilities(ctx); !ok || !capabilities.AllowUtf8LabelNames { + level.Debug(q.logger).Log("msg", "filtering out non-valid labels") + labelNames = filterLabelNames(labelNames) + } + return connect.NewResponse(&typesv1.LabelNamesResponse{ - Names: uniqueSortedStrings(responses), + Names: labelNames, }), nil } @@ -336,8 +357,14 @@ func (q *Querier) LabelNames(ctx context.Context, req *connect.Request[typesv1.L return nil, err } + labelNames := uniqueSortedStrings(responses) + if capabilities, ok := featureflags.GetClientCapabilities(ctx); !ok || !capabilities.AllowUtf8LabelNames { + level.Debug(q.logger).Log("msg", "filtering out non-valid labels") + labelNames = filterLabelNames(labelNames) + } + return connect.NewResponse(&typesv1.LabelNamesResponse{ - Names: uniqueSortedStrings(responses), + Names: labelNames, }), nil } @@ -378,6 +405,42 @@ func (q *Querier) blockSelect(ctx context.Context, start, end model.Time) (block return results.blockPlan(ctx), nil } +func (q *Querier) filterLabelNames( + ctx context.Context, + req *connect.Request[querierv1.SeriesRequest], +) ([]string, error) { + if capabilities, ok := featureflags.GetClientCapabilities(ctx); ok && capabilities.AllowUtf8LabelNames { + return req.Msg.LabelNames, nil + } + + // Filter out label names not passing legacy validation if utf8 label names not enabled + toFilter := make([]string, len(req.Msg.LabelNames)) + copy(toFilter, req.Msg.LabelNames) + filtered := make([]string, 0, len(toFilter)) + + if len(req.Msg.LabelNames) == 0 { + // Querying for all label names; must retrieve all label names to then filter out + response, err := q.LabelNames(ctx, connect.NewRequest(&typesv1.LabelNamesRequest{ + Matchers: req.Msg.Matchers, + Start: req.Msg.Start, + End: req.Msg.End, + })) + if err != nil { + return nil, err + } + toFilter = response.Msg.Names + } + + for _, name := range toFilter { + if _, _, ok := validation.SanitizeLegacyLabelName(name); !ok { + level.Debug(q.logger).Log("msg", "filtering out label", "label_name", name) + continue + } + filtered = append(filtered, name) + } + return filtered, nil +} + func (q *Querier) Series(ctx context.Context, req *connect.Request[querierv1.SeriesRequest]) (*connect.Response[querierv1.SeriesResponse], error) { sp, ctx := opentracing.StartSpanFromContext(ctx, "Series") defer sp.Finish() @@ -390,6 +453,14 @@ func (q *Querier) Series(ctx context.Context, req *connect.Request[querierv1.Ser otlog.Int64("start", req.Msg.Start), otlog.Int64("end", req.Msg.End), ) + + // Update LabelNames + filteredLabelNames, err := q.filterLabelNames(ctx, req) + if err != nil { + return nil, err + } + req.Msg.LabelNames = filteredLabelNames + // no store gateways configured so just query the ingesters if q.storeGatewayQuerier == nil || !hasTimeRange { responses, err := q.seriesFromIngesters(ctx, &ingestv1.SeriesRequest{ @@ -451,7 +522,7 @@ func (q *Querier) Series(ctx context.Context, req *connect.Request[querierv1.Ser }) } - err := group.Wait() + err = group.Wait() if err != nil { return nil, err } diff --git a/pkg/querier/querier_test.go b/pkg/querier/querier_test.go index dd6c66ef27..a3c563193d 100644 --- a/pkg/querier/querier_test.go +++ b/pkg/querier/querier_test.go @@ -9,6 +9,7 @@ import ( "os" "path" "sort" + "sync" "testing" "time" @@ -31,6 +32,7 @@ import ( querierv1 "github.com/grafana/pyroscope/api/gen/proto/go/querier/v1" typesv1 "github.com/grafana/pyroscope/api/gen/proto/go/types/v1" "github.com/grafana/pyroscope/pkg/clientpool" + "github.com/grafana/pyroscope/pkg/featureflags" phlaremodel "github.com/grafana/pyroscope/pkg/model" objstoreclient "github.com/grafana/pyroscope/pkg/objstore/client" "github.com/grafana/pyroscope/pkg/objstore/providers/filesystem" @@ -170,6 +172,206 @@ func Test_QueryLabelNames(t *testing.T) { require.Equal(t, []string{"bar", "buzz", "foo"}, out.Msg.Names) } +func Test_filterLabelNames(t *testing.T) { + tests := []struct { + name string + labelNames []string + want []string + }{ + { + name: "empty slice", + labelNames: []string{}, + want: []string{}, + }, + { + name: "nil slice", + labelNames: nil, + want: []string{}, + }, + { + name: "all valid label names", + labelNames: []string{"foo", "bar", "service_name", "ServiceName123"}, + want: []string{"foo", "bar", "service_name", "ServiceName123"}, + }, + { + name: "all invalid label names", + labelNames: []string{"123service", "service-name", "service name", "世界"}, + want: []string{}, + }, + { + name: "mix of valid and invalid label names", + labelNames: []string{"foo", "123invalid", "bar", "invalid-name", "buzz", "世界"}, + want: []string{"foo", "bar", "buzz"}, + }, + { + name: "label names with dots (valid but get sanitized)", + labelNames: []string{"service.name", "app.version", "valid_label"}, + want: []string{"service.name", "app.version", "valid_label"}, + }, + { + name: "single valid label", + labelNames: []string{"service"}, + want: []string{"service"}, + }, + { + name: "single invalid label", + labelNames: []string{"123invalid"}, + want: []string{}, + }, + { + name: "labels with underscores", + labelNames: []string{"_", "__a", "__a__", "_service_name_"}, + want: []string{"_", "__a", "__a__", "_service_name_"}, + }, + { + name: "mixed valid labels with dots and underscores", + labelNames: []string{"a.b.c", "a_b_c", "a.b_c.d"}, + want: []string{"a.b.c", "a_b_c", "a.b_c.d"}, + }, + { + name: "labels starting with dots (valid, dots are sanitized)", + labelNames: []string{".abc", "..def", ".g.h.i"}, + want: []string{".abc", "..def", ".g.h.i"}, + }, + { + name: "labels starting with invalid characters", + labelNames: []string{"-abc", "0abc", "123xyz"}, + want: []string{}, + }, + { + name: "empty string in slice (invalid)", + labelNames: []string{"foo", "", "bar"}, + want: []string{"foo", "bar"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := filterLabelNames(tt.labelNames) + require.Equal(t, tt.want, got) + }) + } +} + +func Test_QueryLabelNames_WithFiltering(t *testing.T) { + req := connect.NewRequest(&typesv1.LabelNamesRequest{}) + + tests := []struct { + name string + allowUtf8LabelNames bool + setCapabilities bool + ingesterLabelNames map[string][]string + expectedLabelNames []string + }{ + { + name: "UTF8 labels allowed when enabled", + allowUtf8LabelNames: true, + setCapabilities: true, + ingesterLabelNames: map[string][]string{ + "1": {"foo", "bar", "世界"}, + "2": {"foo", "bar", "世界"}, + "3": {"foo", "bar", "世界"}, + }, + // UTF8 labels pass through when UTF8 is enabled + expectedLabelNames: []string{"bar", "foo", "世界"}, + }, + { + name: "UTF8 labels filtered when disabled", + allowUtf8LabelNames: false, + setCapabilities: true, + ingesterLabelNames: map[string][]string{ + "1": {"foo", "bar", "世界"}, + "2": {"foo", "bar", "世界"}, + "3": {"foo", "bar", "世界"}, + }, + // Only legacy-valid labels pass through (UTF8 filtered out) + expectedLabelNames: []string{"bar", "foo"}, + }, + { + name: "all labels pass through when UTF8 enabled including invalid ones", + allowUtf8LabelNames: true, + setCapabilities: true, + ingesterLabelNames: map[string][]string{ + "1": {"foo", "123invalid"}, + "2": {"foo", "123invalid"}, + "3": {"foo", "123invalid"}, + }, + // When UTF8 is enabled, NO filtering happens - even invalid labels pass through + expectedLabelNames: []string{"123invalid", "foo"}, + }, + { + name: "filtering enabled when no capabilities set", + setCapabilities: false, + ingesterLabelNames: map[string][]string{ + "1": {"valid_name", "123invalid", "世界"}, + "2": {"valid_name", "123invalid", "世界"}, + "3": {"valid_name", "123invalid", "世界"}, + }, + // No capabilities means legacy-only mode - UTF8 and invalid labels filtered + expectedLabelNames: []string{"valid_name"}, + }, + { + name: "all valid legacy labels pass through when filtering enabled", + allowUtf8LabelNames: false, + setCapabilities: true, + ingesterLabelNames: map[string][]string{ + "1": {"foo", "bar"}, + "2": {"bar", "buzz"}, + "3": {"buzz", "foo"}, + }, + expectedLabelNames: []string{"bar", "buzz", "foo"}, + }, + { + name: "labels with dots pass through in both modes", + allowUtf8LabelNames: false, + setCapabilities: true, + ingesterLabelNames: map[string][]string{ + "1": {"service.name", "app.version"}, + "2": {"host.name", "service.name"}, + "3": {"app.version", "host.name"}, + }, + // Dots are valid legacy characters (get sanitized to underscores) + expectedLabelNames: []string{"app.version", "host.name", "service.name"}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + querier, err := New(&NewQuerierParams{ + Cfg: Config{ + PoolConfig: clientpool.PoolConfig{ClientCleanupPeriod: 1 * time.Millisecond}, + }, + IngestersRing: testhelper.NewMockRing([]ring.InstanceDesc{ + {Addr: "1"}, + {Addr: "2"}, + {Addr: "3"}, + }, 3), + PoolFactory: &poolFactory{f: func(addr string) (client.PoolClient, error) { + q := newFakeQuerier() + if labelNames, ok := tc.ingesterLabelNames[addr]; ok { + q.On("LabelNames", mock.Anything, mock.Anything).Return( + connect.NewResponse(&typesv1.LabelNamesResponse{Names: labelNames}), nil) + } + return q, nil + }}, + Logger: log.NewLogfmtLogger(os.Stdout), + }) + require.NoError(t, err) + + ctx := context.Background() + if tc.setCapabilities { + ctx = featureflags.WithClientCapabilities(ctx, featureflags.ClientCapabilities{ + AllowUtf8LabelNames: tc.allowUtf8LabelNames, + }) + } + + out, err := querier.LabelNames(ctx, req) + require.NoError(t, err) + require.Equal(t, tc.expectedLabelNames, out.Msg.Names) + }) + } +} + func Test_Series(t *testing.T) { foobarlabels := phlaremodel.NewLabelsBuilder(nil).Set("foo", "bar") foobuzzlabels := phlaremodel.NewLabelsBuilder(nil).Set("foo", "buzz") @@ -191,10 +393,13 @@ func Test_Series(t *testing.T) { q := newFakeQuerier() switch addr { case "1": + q.On("LabelNames", mock.Anything, mock.Anything).Return(connect.NewResponse(&typesv1.LabelNamesResponse{Names: []string{"foo", "bar"}}), nil) q.On("Series", mock.Anything, mock.Anything).Return(ingesterResponse, nil) case "2": + q.On("LabelNames", mock.Anything, mock.Anything).Return(connect.NewResponse(&typesv1.LabelNamesResponse{Names: []string{"foo", "bar"}}), nil) q.On("Series", mock.Anything, mock.Anything).Return(ingesterResponse, nil) case "3": + q.On("LabelNames", mock.Anything, mock.Anything).Return(connect.NewResponse(&typesv1.LabelNamesResponse{Names: []string{"foo", "bar"}}), nil) q.On("Series", mock.Anything, mock.Anything).Return(ingesterResponse, nil) } return q, nil @@ -211,6 +416,139 @@ func Test_Series(t *testing.T) { }, out.Msg.LabelsSet) } +func Test_Series_WithLabelNameFiltering(t *testing.T) { + tests := []struct { + name string + allowUtf8LabelNames bool + setCapabilities bool + requestLabelNames []string + expectedLabelNames []string + }{ + { + name: "all label names pass through when UTF8 enabled", + allowUtf8LabelNames: true, + setCapabilities: true, + requestLabelNames: []string{"valid_name", "123invalid", "invalid-hyphen", "世界"}, + expectedLabelNames: []string{"valid_name", "123invalid", "invalid-hyphen", "世界"}, + }, + { + name: "invalid label names filtered when UTF8 disabled", + allowUtf8LabelNames: false, + setCapabilities: true, + requestLabelNames: []string{"valid_name", "123invalid", "invalid-hyphen", "世界"}, + expectedLabelNames: []string{"valid_name"}, + }, + { + name: "UTF8 labels filtered when UTF8 disabled", + allowUtf8LabelNames: false, + setCapabilities: true, + requestLabelNames: []string{"foo", "bar", "世界", "日本語"}, + expectedLabelNames: []string{"foo", "bar"}, + }, + { + name: "filtering enabled when no capabilities set", + setCapabilities: false, + requestLabelNames: []string{"foo", "123invalid", "世界"}, + expectedLabelNames: []string{"foo"}, + }, + { + name: "all valid labels pass through", + allowUtf8LabelNames: false, + setCapabilities: true, + requestLabelNames: []string{"foo", "bar", "service_name"}, + expectedLabelNames: []string{"foo", "bar", "service_name"}, + }, + { + name: "labels with dots pass through", + allowUtf8LabelNames: false, + setCapabilities: true, + requestLabelNames: []string{"service.name", "app.version"}, + expectedLabelNames: []string{"service.name", "app.version"}, + }, + { + name: "empty label names with UTF8 disabled queries all labels and filters", + allowUtf8LabelNames: false, + setCapabilities: true, + requestLabelNames: []string{}, + expectedLabelNames: []string{"bar", "foo"}, + }, + { + name: "empty label names with UTF8 enabled returns ingester labels without filtering", + allowUtf8LabelNames: true, + setCapabilities: true, + requestLabelNames: []string{}, + expectedLabelNames: []string{"日本語", "bar"}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + ingesterLabels := []string{"日本語", "bar"} + var capturedLabelNames []string + var capturedMutex sync.Mutex + + querier, err := New(&NewQuerierParams{ + Cfg: Config{ + PoolConfig: clientpool.PoolConfig{ClientCleanupPeriod: 1 * time.Millisecond}, + }, + IngestersRing: testhelper.NewMockRing([]ring.InstanceDesc{ + {Addr: "1"}, + {Addr: "2"}, + {Addr: "3"}, + }, 3), + PoolFactory: &poolFactory{f: func(addr string) (client.PoolClient, error) { + q := newFakeQuerier() + + q.On("Series", mock.Anything, mock.Anything).Run(func(args mock.Arguments) { + req := args.Get(1).(*connect.Request[ingestv1.SeriesRequest]) + capturedMutex.Lock() + defer capturedMutex.Unlock() + + // If no labels passed to ingester series request, + // ingester returns all ingester labels + if len(req.Msg.LabelNames) == 0 { + capturedLabelNames = ingesterLabels + } else { + capturedLabelNames = req.Msg.LabelNames + } + }).Return(connect.NewResponse(&ingestv1.SeriesResponse{}), nil) + + if len(tc.requestLabelNames) == 0 { + q.On("LabelNames", mock.Anything, mock.Anything).Return( + connect.NewResponse(&typesv1.LabelNamesResponse{Names: []string{"foo", "bar", "世界"}}), nil) + } + + return q, nil + }}, + Logger: log.NewLogfmtLogger(os.Stdout), + }) + require.NoError(t, err) + + ctx := context.Background() + if tc.setCapabilities { + ctx = featureflags.WithClientCapabilities(ctx, featureflags.ClientCapabilities{ + AllowUtf8LabelNames: tc.allowUtf8LabelNames, + }) + } + + req := connect.NewRequest(&querierv1.SeriesRequest{ + Matchers: []string{`{foo="bar"}`}, + LabelNames: tc.requestLabelNames, + }) + + _, err = querier.Series(ctx, req) + require.NoError(t, err) + + // Verify that the label names were filtered correctly before being sent to ingester + capturedMutex.Lock() + actualLabelNames := capturedLabelNames + capturedMutex.Unlock() + require.Equal(t, tc.expectedLabelNames, actualLabelNames, + "Expected label names sent to ingester to be %v, but got %v", tc.expectedLabelNames, actualLabelNames) + }) + } +} + func newBlockMeta(ulids ...string) *connect.Response[ingestv1.BlockMetadataResponse] { resp := &ingestv1.BlockMetadataResponse{} diff --git a/pkg/validation/validate.go b/pkg/validation/validate.go index a30bd49962..240c921088 100644 --- a/pkg/validation/validate.go +++ b/pkg/validation/validate.go @@ -146,7 +146,7 @@ func ValidateLabels(limits LabelValidationLimits, tenantID string, ls []*typesv1 if len(l.Value) > limits.MaxLabelValueLength(tenantID) { return NewErrorf(LabelValueTooLong, LabelValueTooLongErrorMsg, phlaremodel.LabelPairsString(ls), l.Value) } - if origName, newName, ok := SanitizeLabelName(l.Name); ok && origName != newName { + if origName, newName, ok := SanitizeLegacyLabelName(l.Name); ok && origName != newName { var err error ls, idx, err = handleSanitizedLabel(ls, idx, origName, newName) if err != nil { @@ -211,11 +211,12 @@ func handleSanitizedLabel(ls []*typesv1.LabelPair, origIdx int, origName, newNam return ls[:len(newSlice)], finalIdx, nil } -// SanitizeLabelName reports whether the label name is valid, -// and returns the sanitized value. +// SanitizeLegacyLabelName reports whether the label name is a valid legacy label name, +// and returns the sanitized value. Legacy label names are non utf-8 and contain characters +// [a-zA-Z0-9_.]. // -// The only change the function makes is replacing dots with underscores. -func SanitizeLabelName(ln string) (old, sanitized string, ok bool) { +// The only sanitization the function makes is replacing dots with underscores. +func SanitizeLegacyLabelName(ln string) (old, sanitized string, ok bool) { if len(ln) == 0 { return ln, ln, false } diff --git a/pkg/validation/validate_test.go b/pkg/validation/validate_test.go index 52c0a449dc..5baa5ab2fe 100644 --- a/pkg/validation/validate_test.go +++ b/pkg/validation/validate_test.go @@ -5,7 +5,6 @@ import ( "time" "github.com/prometheus/common/model" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" googlev1 "github.com/grafana/pyroscope/api/gen/proto/go/google/v1" @@ -135,17 +134,6 @@ func TestValidateLabels(t *testing.T) { {Name: "service_name", Value: "svc0"}, }, }, - { - name: "duplicates once sanitized with conflicting values", - lbs: []*typesv1.LabelPair{ - {Name: model.MetricNameLabel, Value: "qux"}, - {Name: "service.name", Value: "svc1"}, - {Name: "service_abc", Value: "def"}, - {Name: "service_name", Value: "svc0"}, - }, - expectedReason: DuplicateLabelNames, - expectedErr: "profile with labels '{__name__=\"qux\", service.name=\"svc1\", service_abc=\"def\", service_name=\"svc0\"}' has duplicate label name 'service_name' after label name sanitization from 'service.name'", - }, } { t.Run(tt.name, func(t *testing.T) { err := ValidateLabels(MockLimits{ @@ -527,40 +515,241 @@ func TestValidateFlamegraphMaxNodes(t *testing.T) { } } -func Test_SanitizeLabelName(t *testing.T) { - for _, tc := range []struct { - input string - expected string - valid bool +func Test_SanitizeLegacyLabelName(t *testing.T) { + tests := []struct { + Name string + LabelName string + WantOld string + WantSanitized string + WantOk bool }{ - {"", "", false}, - {".", "_", true}, - {".a", "_a", true}, - {"a.", "a_", true}, - {"..", "__", true}, - {"..a", "__a", true}, - {"a..", "a__", true}, - {"a.a", "a_a", true}, - {".a.", "_a_", true}, - {"..a..", "__a__", true}, - {"世界", "世界", false}, - {"界世_a", "界世_a", false}, - {"界世__a", "界世__a", false}, - {"a_世界", "a_世界", false}, - {"0.a", "0.a", false}, - {"0a", "0a", false}, - {"a.0", "a_0", true}, - {"a0", "a0", true}, - {"_", "_", true}, - {"__a", "__a", true}, - {"__a__", "__a__", true}, - } { - tc := tc - t.Run("", func(t *testing.T) { - origName, actual, valid := SanitizeLabelName(tc.input) - assert.Equal(t, tc.input, origName) - assert.Equal(t, tc.expected, actual) - assert.Equal(t, tc.valid, valid) + { + Name: "empty string is invalid", + LabelName: "", + WantOld: "", + WantSanitized: "", + WantOk: false, + }, + { + Name: "valid simple label name", + LabelName: "service", + WantOld: "service", + WantSanitized: "service", + WantOk: true, + }, + { + Name: "valid label with underscores", + LabelName: "service_name", + WantOld: "service_name", + WantSanitized: "service_name", + WantOk: true, + }, + { + Name: "valid label with numbers", + LabelName: "service123", + WantOld: "service123", + WantSanitized: "service123", + WantOk: true, + }, + { + Name: "valid mixed case label", + LabelName: "ServiceName", + WantOld: "ServiceName", + WantSanitized: "ServiceName", + WantOk: true, + }, + { + Name: "label with dots gets sanitized", + LabelName: "service.name", + WantOld: "service.name", + WantSanitized: "service_name", + WantOk: true, + }, + { + Name: "label with multiple dots gets sanitized", + LabelName: "service.name.type", + WantOld: "service.name.type", + WantSanitized: "service_name_type", + WantOk: true, + }, + { + Name: "label starting with number is invalid", + LabelName: "123service", + WantOld: "123service", + WantSanitized: "123service", + WantOk: false, + }, + { + Name: "label with hyphen is invalid", + LabelName: "service-name", + WantOld: "service-name", + WantSanitized: "service-name", + WantOk: false, + }, + { + Name: "label with space is invalid", + LabelName: "service name", + WantOld: "service name", + WantSanitized: "service name", + WantOk: false, + }, + { + Name: "label with special characters is invalid", + LabelName: "service@name", + WantOld: "service@name", + WantSanitized: "service@name", + WantOk: false, + }, + { + Name: "label with dots and invalid characters is invalid", + LabelName: "service.name@host", + WantOld: "service.name@host", + WantSanitized: "service.name@host", + WantOk: false, + }, + { + Name: "label starting with underscore", + LabelName: "_service", + WantOld: "_service", + WantSanitized: "_service", + WantOk: true, + }, + { + Name: "label with only underscores", + LabelName: "___", + WantOld: "___", + WantSanitized: "___", + WantOk: true, + }, + { + Name: "label ending with dot", + LabelName: "service.", + WantOld: "service.", + WantSanitized: "service_", + WantOk: true, + }, + { + Name: "label starting with dot gets sanitized", + LabelName: ".service", + WantOld: ".service", + WantSanitized: "_service", + WantOk: true, + }, + { + Name: "single dot", + LabelName: ".", + WantOld: ".", + WantSanitized: "_", + WantOk: true, + }, + { + Name: "double dots", + LabelName: "..", + WantOld: "..", + WantSanitized: "__", + WantOk: true, + }, + { + Name: "double dots with letter at end", + LabelName: "..a", + WantOld: "..a", + WantSanitized: "__a", + WantOk: true, + }, + { + Name: "letter with double dots at end", + LabelName: "a..", + WantOld: "a..", + WantSanitized: "a__", + WantOk: true, + }, + { + Name: "letter surrounded by dots", + LabelName: ".a.", + WantOld: ".a.", + WantSanitized: "_a_", + WantOk: true, + }, + { + Name: "letter surrounded by double dots", + LabelName: "..a..", + WantOld: "..a..", + WantSanitized: "__a__", + WantOk: true, + }, + { + Name: "letter with dot and number", + LabelName: "a.0", + WantOld: "a.0", + WantSanitized: "a_0", + WantOk: true, + }, + { + Name: "number with dot is invalid", + LabelName: "0.a", + WantOld: "0.a", + WantSanitized: "0.a", + WantOk: false, + }, + { + Name: "single underscore", + LabelName: "_", + WantOld: "_", + WantSanitized: "_", + WantOk: true, + }, + { + Name: "double underscore with letter", + LabelName: "__a", + WantOld: "__a", + WantSanitized: "__a", + WantOk: true, + }, + { + Name: "letter surrounded by double underscores", + LabelName: "__a__", + WantOld: "__a__", + WantSanitized: "__a__", + WantOk: true, + }, + { + Name: "unicode characters are invalid", + LabelName: "世界", + WantOld: "世界", + WantSanitized: "世界", + WantOk: false, + }, + { + Name: "mixed unicode with valid characters is invalid", + LabelName: "界世_a", + WantOld: "界世_a", + WantSanitized: "界世_a", + WantOk: false, + }, + { + Name: "mixed unicode with underscores is invalid", + LabelName: "界世__a", + WantOld: "界世__a", + WantSanitized: "界世__a", + WantOk: false, + }, + { + Name: "valid characters with unicode suffix is invalid", + LabelName: "a_世界", + WantOld: "a_世界", + WantSanitized: "a_世界", + WantOk: false, + }, + } + + for _, tt := range tests { + t.Run(tt.Name, func(t *testing.T) { + t.Parallel() + + gotOld, gotSanitized, gotOk := SanitizeLegacyLabelName(tt.LabelName) + require.Equal(t, tt.WantOld, gotOld) + require.Equal(t, tt.WantSanitized, gotSanitized) + require.Equal(t, tt.WantOk, gotOk) }) } }