diff --git a/README.md b/README.md index 0485d0e7e..3ca468d74 100644 --- a/README.md +++ b/README.md @@ -1022,6 +1022,13 @@ Configure statistics output frequency with `STATS_FLUSH_INTERVAL`, where the typ To disable statistics entirely, set env var `DISABLE_STATS` to `true` +To sanitize `.` characters in descriptor keys and values before they are published as metrics, set env var +`SANITIZE_DESCRIPTOR_METRIC_DOTS` to `true` (default `false`). When enabled, each `.` in a descriptor key or value is +replaced with `_` in the emitted metric name. Because `.` is the statsd metric-hierarchy separator, a dotted value +(e.g. a gRPC path like `/helloworld.Greeter/SayHello`) would otherwise inject unintended extra hierarchy levels — +which breaks the Prometheus `statsd_exporter` metric-name-to-label mapping. This only affects metric names; rate limit +matching (which keys off the raw descriptor) is unchanged. It is disabled by default for backward compatibility. + Rate Limit Statistic Path: ``` diff --git a/src/config/config_impl.go b/src/config/config_impl.go index c2b48e3f9..82fffcdd8 100644 --- a/src/config/config_impl.go +++ b/src/config/config_impl.go @@ -12,6 +12,7 @@ import ( "gopkg.in/yaml.v2" "github.com/envoyproxy/ratelimit/src/stats" + "github.com/envoyproxy/ratelimit/src/utils" ) type yamlReplaces struct { @@ -443,6 +444,17 @@ func (this *rateLimitConfigImpl) Dump() string { return ret } +// maybeSanitize replaces '.' with '_' in a descriptor key or value fragment when the +// SANITIZE_DESCRIPTOR_METRIC_DOTS setting is enabled, so dotted values don't inject extra +// metric hierarchy levels. It must only be applied to fragments written into metric names, +// never to keys used for descriptor map lookups (that would break rate limit matching). +func (this *rateLimitConfigImpl) maybeSanitize(s string) string { + if this.statsManager.SanitizeDescriptorMetricDots() { + return utils.SanitizeStatKeyValue(s) + } + return s +} + func (this *rateLimitConfigImpl) GetLimit( ctx context.Context, domain string, descriptor *pb_struct.RateLimitDescriptor, ) *RateLimit { @@ -457,7 +469,7 @@ func (this *rateLimitConfigImpl) GetLimit( } if descriptor.GetLimit() != nil { - rateLimitKey := descriptorKey(domain, descriptor) + rateLimitKey := this.descriptorKey(domain, descriptor) rateLimitOverrideUnit := pb.RateLimitResponse_RateLimit_Unit(descriptor.GetLimit().GetUnit()) // When limit override is provided by envoy config, we don't want to enable shadow_mode rateLimit = NewRateLimit( @@ -494,8 +506,12 @@ func (this *rateLimitConfigImpl) GetLimit( // to check for a default value. finalKey := entry.Key + "_" + entry.Value + // Write the sanitized fragments (never finalKey itself, which is reused for the + // descriptor map lookup below and must stay verbatim for rate limit matching). detailedMetricFullKey.WriteString(".") - detailedMetricFullKey.WriteString(finalKey) + detailedMetricFullKey.WriteString(this.maybeSanitize(entry.Key)) + detailedMetricFullKey.WriteString("_") + detailedMetricFullKey.WriteString(this.maybeSanitize(entry.Value)) logger.Debugf("looking up key: %s", finalKey) nextDescriptor := descriptorsMap[finalKey] @@ -561,14 +577,14 @@ func (this *rateLimitConfigImpl) GetLimit( } // Write key and value (if any) - valueToMetricFullKey.WriteString(entry.Key) + valueToMetricFullKey.WriteString(this.maybeSanitize(entry.Key)) if valueToUse != "" { valueToMetricFullKey.WriteString("_") - valueToMetricFullKey.WriteString(valueToUse) + valueToMetricFullKey.WriteString(this.maybeSanitize(valueToUse)) } } else { // No next descriptor found; still append something deterministic - valueToMetricFullKey.WriteString(entry.Key) + valueToMetricFullKey.WriteString(this.maybeSanitize(entry.Key)) } if nextDescriptor != nil && nextDescriptor.limit != nil { @@ -635,15 +651,15 @@ func (this *rateLimitConfigImpl) GetLimit( for i, entry := range descriptor.Entries { shareThresholdMetricKey.WriteString(".") if i < len(rateLimit.ShareThresholdKeyPattern) && rateLimit.ShareThresholdKeyPattern[i] != "" { - shareThresholdMetricKey.WriteString(entry.Key) + shareThresholdMetricKey.WriteString(this.maybeSanitize(entry.Key)) shareThresholdMetricKey.WriteString("_") - shareThresholdMetricKey.WriteString(rateLimit.ShareThresholdKeyPattern[i]) + shareThresholdMetricKey.WriteString(this.maybeSanitize(rateLimit.ShareThresholdKeyPattern[i])) } else { // Include full key_value for entries without share_threshold - shareThresholdMetricKey.WriteString(entry.Key) + shareThresholdMetricKey.WriteString(this.maybeSanitize(entry.Key)) if entry.Value != "" { shareThresholdMetricKey.WriteString("_") - shareThresholdMetricKey.WriteString(entry.Value) + shareThresholdMetricKey.WriteString(this.maybeSanitize(entry.Value)) } } } @@ -676,15 +692,15 @@ func (this *rateLimitConfigImpl) IsEmptyDomains() bool { return len(this.domains) == 0 } -func descriptorKey(domain string, descriptor *pb_struct.RateLimitDescriptor) string { +func (this *rateLimitConfigImpl) descriptorKey(domain string, descriptor *pb_struct.RateLimitDescriptor) string { rateLimitKey := "" for _, entry := range descriptor.Entries { if rateLimitKey != "" { rateLimitKey += "." } - rateLimitKey += entry.Key + rateLimitKey += this.maybeSanitize(entry.Key) if entry.Value != "" { - rateLimitKey += "_" + entry.Value + rateLimitKey += "_" + this.maybeSanitize(entry.Value) } } return domain + "." + rateLimitKey diff --git a/src/settings/settings.go b/src/settings/settings.go index ffb547083..198f3a1e3 100644 --- a/src/settings/settings.go +++ b/src/settings/settings.go @@ -90,6 +90,7 @@ type Settings struct { ExtraTags map[string]string `envconfig:"EXTRA_TAGS" default:""` StatsFlushInterval time.Duration `envconfig:"STATS_FLUSH_INTERVAL" default:"10s"` DisableStats bool `envconfig:"DISABLE_STATS" default:"false"` + SanitizeDescriptorMetricDots bool `envconfig:"SANITIZE_DESCRIPTOR_METRIC_DOTS" default:"false"` UsePrometheus bool `envconfig:"USE_PROMETHEUS" default:"false"` PrometheusAddr string `envconfig:"PROMETHEUS_ADDR" default:":9090"` PrometheusPath string `envconfig:"PROMETHEUS_PATH" default:"/metrics"` diff --git a/src/stats/manager.go b/src/stats/manager.go index a63a7b5c9..688cfb8f7 100644 --- a/src/stats/manager.go +++ b/src/stats/manager.go @@ -20,13 +20,17 @@ type Manager interface { NewServiceStats() ServiceStats // Returns the stats.Store wrapped by the Manager. GetStatsStore() gostats.Store + // SanitizeDescriptorMetricDots reports whether '.' in descriptor keys/values should be + // replaced with '_' when building metric names (SANITIZE_DESCRIPTOR_METRIC_DOTS). + SanitizeDescriptorMetricDots() bool } type ManagerImpl struct { - store gostats.Store - rlStatsScope gostats.Scope - serviceStatsScope gostats.Scope - shouldRateLimitScope gostats.Scope + store gostats.Store + rlStatsScope gostats.Scope + serviceStatsScope gostats.Scope + shouldRateLimitScope gostats.Scope + sanitizeDescriptorMetricDots bool } // Stats for panic recoveries. diff --git a/src/stats/manager_impl.go b/src/stats/manager_impl.go index e629064c7..aa195f88c 100644 --- a/src/stats/manager_impl.go +++ b/src/stats/manager_impl.go @@ -11,10 +11,11 @@ import ( func NewStatManager(store gostats.Store, settings settings.Settings) *ManagerImpl { serviceScope := store.ScopeWithTags("ratelimit", settings.ExtraTags).Scope("service") return &ManagerImpl{ - store: store, - rlStatsScope: serviceScope.Scope("rate_limit"), - serviceStatsScope: serviceScope, - shouldRateLimitScope: serviceScope.Scope("call.should_rate_limit"), + store: store, + rlStatsScope: serviceScope.Scope("rate_limit"), + serviceStatsScope: serviceScope, + shouldRateLimitScope: serviceScope.Scope("call.should_rate_limit"), + sanitizeDescriptorMetricDots: settings.SanitizeDescriptorMetricDots, } } @@ -22,6 +23,10 @@ func (this *ManagerImpl) GetStatsStore() gostats.Store { return this.store } +func (this *ManagerImpl) SanitizeDescriptorMetricDots() bool { + return this.sanitizeDescriptorMetricDots +} + // Create new rate descriptor stats for a descriptor tuple. // @param key supplies the fully resolved descriptor tuple. // @return new stats. diff --git a/src/utils/utilities.go b/src/utils/utilities.go index 60aa516ce..1b8797346 100644 --- a/src/utils/utilities.go +++ b/src/utils/utilities.go @@ -87,6 +87,13 @@ func SanitizeStatName(s string) string { }) } +// SanitizeStatKeyValue replaces the statsd hierarchy separator '.' with '_' so that +// dots in a descriptor key or value do not create unintended metric hierarchy levels +// (which, for example, break the Prometheus statsd_exporter metric-name -> label mapping). +func SanitizeStatKeyValue(s string) string { + return strings.ReplaceAll(s, ".", "_") +} + type HitsAddend struct { Value uint64 IsNegative bool diff --git a/test/config/config_test.go b/test/config/config_test.go index 2bd009a99..19a9fe17c 100644 --- a/test/config/config_test.go +++ b/test/config/config_test.go @@ -2261,3 +2261,168 @@ func TestMetadata(t *testing.T) { // share_threshold parity (middle+trailing wildcards produce the same shared-counter // behaviour as trailing-only) is covered by TestShareThreshold Cases 5-7. + +// TestSanitizeDescriptorMetricDots verifies the SANITIZE_DESCRIPTOR_METRIC_DOTS behavior: +// when enabled, '.' in descriptor keys/values is replaced with '_' in emitted metric names, +// while rate-limit matching (which keys off the un-sanitized descriptor) is unaffected. +func TestSanitizeDescriptorMetricDots(t *testing.T) { + // A config whose leaf key/value are looked up by the un-sanitized descriptor. The + // descriptor value carries dots (an IP and a dotted host) to exercise the metric path. + basicCfg := func() []config.RateLimitConfigToLoad { + return []config.RateLimitConfigToLoad{{ + Name: "inline", + ConfigYaml: &config.YamlRoot{ + Domain: "domain", + Descriptors: []config.YamlDescriptor{ + { + Key: "source_ip", + Value: "10.0.0.1", + RateLimit: &config.YamlRateLimit{ + RequestsPerUnit: 5, + Unit: "minute", + }, + }, + }, + }, + }} + } + ipDescriptor := &pb_struct.RateLimitDescriptor{ + Entries: []*pb_struct.RateLimitDescriptor_Entry{{Key: "source_ip", Value: "10.0.0.1"}}, + } + + t.Run("flag off keeps dots (backward compatible)", func(t *testing.T) { + asrt := assert.New(t) + store := stats.NewStore(stats.NewNullSink(), false) + rlConfig := config.NewRateLimitConfigImpl(basicCfg(), mockstats.NewMockStatManagerWithSanitize(store, false), false) + rl := rlConfig.GetLimit(context.TODO(), "domain", ipDescriptor) + asrt.NotNil(rl) + asrt.Equal("domain.source_ip_10.0.0.1", rl.Stats.Key) + }) + + t.Run("flag on sanitizes dotted value", func(t *testing.T) { + asrt := assert.New(t) + store := stats.NewStore(stats.NewNullSink(), false) + rlConfig := config.NewRateLimitConfigImpl(basicCfg(), mockstats.NewMockStatManagerWithSanitize(store, true), false) + rl := rlConfig.GetLimit(context.TODO(), "domain", ipDescriptor) + asrt.NotNil(rl) + expectedKey := "domain.source_ip_10_0_0_1" + asrt.Equal(expectedKey, rl.Stats.Key) + + // The sanitized key is the one actually registered as a counter. + rl.Stats.TotalHits.Inc() + asrt.EqualValues(1, store.NewCounter(expectedKey+".total_hits").Value()) + }) + + t.Run("flag on sanitizes non-ipv4 dotted value", func(t *testing.T) { + asrt := assert.New(t) + store := stats.NewStore(stats.NewNullSink(), false) + cfg := []config.RateLimitConfigToLoad{{ + Name: "inline", + ConfigYaml: &config.YamlRoot{ + Domain: "domain", + Descriptors: []config.YamlDescriptor{ + { + Key: "host", + Value: "foo.bar", + RateLimit: &config.YamlRateLimit{ + RequestsPerUnit: 5, + Unit: "minute", + }, + }, + }, + }, + }} + rlConfig := config.NewRateLimitConfigImpl(cfg, mockstats.NewMockStatManagerWithSanitize(store, true), false) + rl := rlConfig.GetLimit(context.TODO(), "domain", + &pb_struct.RateLimitDescriptor{Entries: []*pb_struct.RateLimitDescriptor_Entry{{Key: "host", Value: "foo.bar"}}}) + asrt.NotNil(rl) + asrt.Equal("domain.host_foo_bar", rl.Stats.Key) + }) + + t.Run("flag on sanitizes dotted key", func(t *testing.T) { + asrt := assert.New(t) + store := stats.NewStore(stats.NewNullSink(), false) + cfg := []config.RateLimitConfigToLoad{{ + Name: "inline", + ConfigYaml: &config.YamlRoot{ + Domain: "domain", + Descriptors: []config.YamlDescriptor{ + { + Key: "a.b", + Value: "v", + RateLimit: &config.YamlRateLimit{ + RequestsPerUnit: 5, + Unit: "minute", + }, + }, + }, + }, + }} + rlConfig := config.NewRateLimitConfigImpl(cfg, mockstats.NewMockStatManagerWithSanitize(store, true), false) + rl := rlConfig.GetLimit(context.TODO(), "domain", + &pb_struct.RateLimitDescriptor{Entries: []*pb_struct.RateLimitDescriptor_Entry{{Key: "a.b", Value: "v"}}}) + asrt.NotNil(rl) + asrt.Equal("domain.a_b_v", rl.Stats.Key) + }) + + t.Run("matching still works with dotted value when flag on", func(t *testing.T) { + asrt := assert.New(t) + store := stats.NewStore(stats.NewNullSink(), false) + rlConfig := config.NewRateLimitConfigImpl(basicCfg(), mockstats.NewMockStatManagerWithSanitize(store, true), false) + // The descriptor still carries the un-sanitized dotted value; if sanitization had + // leaked into the map lookup key, this would fail to match and return nil. + rl := rlConfig.GetLimit(context.TODO(), "domain", ipDescriptor) + asrt.NotNil(rl) + asrt.EqualValues(5, rl.Limit.RequestsPerUnit) + }) + + t.Run("flag on sanitizes detailed_metric path", func(t *testing.T) { + asrt := assert.New(t) + store := stats.NewStore(stats.NewNullSink(), false) + cfg := []config.RateLimitConfigToLoad{{ + Name: "inline", + ConfigYaml: &config.YamlRoot{ + Domain: "domain", + Descriptors: []config.YamlDescriptor{ + { + Key: "source_ip", + DetailedMetric: true, + RateLimit: &config.YamlRateLimit{ + RequestsPerUnit: 5, + Unit: "minute", + }, + }, + }, + }, + }} + rlConfig := config.NewRateLimitConfigImpl(cfg, mockstats.NewMockStatManagerWithSanitize(store, true), false) + rl := rlConfig.GetLimit(context.TODO(), "domain", ipDescriptor) + asrt.NotNil(rl) + asrt.Equal("domain.source_ip_10_0_0_1", rl.Stats.Key) + }) + + t.Run("flag on sanitizes value_to_metric path", func(t *testing.T) { + asrt := assert.New(t) + store := stats.NewStore(stats.NewNullSink(), false) + cfg := []config.RateLimitConfigToLoad{{ + Name: "inline", + ConfigYaml: &config.YamlRoot{ + Domain: "domain", + Descriptors: []config.YamlDescriptor{ + { + Key: "source_ip", + ValueToMetric: true, + RateLimit: &config.YamlRateLimit{ + RequestsPerUnit: 5, + Unit: "minute", + }, + }, + }, + }, + }} + rlConfig := config.NewRateLimitConfigImpl(cfg, mockstats.NewMockStatManagerWithSanitize(store, true), false) + rl := rlConfig.GetLimit(context.TODO(), "domain", ipDescriptor) + asrt.NotNil(rl) + asrt.Equal("domain.source_ip_10_0_0_1", rl.Stats.Key) + }) +} diff --git a/test/integration/integration_test.go b/test/integration/integration_test.go index d7d2c512d..705e40592 100644 --- a/test/integration/integration_test.go +++ b/test/integration/integration_test.go @@ -209,6 +209,68 @@ func TestBasicConfig_ExtraTags(t *testing.T) { }) } +// TestSanitizeDescriptorMetricDots exercises the SANITIZE_DESCRIPTOR_METRIC_DOTS setting +// end-to-end: a descriptor value carrying '.' characters (a gRPC path) is sent through the +// real gRPC server, and we assert the emitted metric name has the dots replaced with '_' +// while rate limiting still works (the request is served, not rejected). +func TestSanitizeDescriptorMetricDots(t *testing.T) { + common.WithMultiRedis(t, []common.RedisConfig{ + {Port: 6383}, + }, func() { + s := makeSimpleRedisSettings(6383, 6380, false, 0) + s.SanitizeDescriptorMetricDots = true + runner := startTestRunner(t, s) + defer runner.Stop() + + assert := assert.New(t) + conn, err := grpc.Dial(fmt.Sprintf("localhost:%v", s.GrpcPort), grpc.WithInsecure()) + assert.NoError(err) + defer conn.Close() + c := pb.NewRateLimitServiceClient(conn) + + // The sanitize domain has a value_to_metric descriptor on key "grpc_path", so the + // runtime value flows into the metric name. The value contains dots. + grpcPath := "/helloworld.Greeter/SayHello" + response, err := c.ShouldRateLimit( + context.Background(), + common.NewRateLimitRequest("sanitize", [][][2]string{{{"grpc_path", grpcPath}}}, 1)) + assert.NoError(err) + assert.Equal(pb.RateLimitResponse_OK, response.OverallCode) + + runner.GetStatsStore().Flush() + + // Dots in the value are replaced with '_' in the emitted metric name. + sanitizedValue := utils.SanitizeStatKeyValue(grpcPath) + sanitizedCounter := runner.GetStatsStore().NewCounter( + fmt.Sprintf("ratelimit.service.rate_limit.sanitize.grpc_path_%s.total_hits", sanitizedValue)) + assert.Equal(1, int(sanitizedCounter.Value())) + + // The un-sanitized (dotted) metric name is NOT emitted. + unsanitizedCounter := runner.GetStatsStore().NewCounter( + fmt.Sprintf("ratelimit.service.rate_limit.sanitize.grpc_path_%s.total_hits", grpcPath)) + assert.Equal(0, int(unsanitizedCounter.Value())) + + // The sanitize domain also has a detailed_metric descriptor on key "detailed_path". + // detailed_metric folds the runtime value into the metric name via a different builder + // than value_to_metric, so exercise that path too. + detailedResponse, err := c.ShouldRateLimit( + context.Background(), + common.NewRateLimitRequest("sanitize", [][][2]string{{{"detailed_path", grpcPath}}}, 1)) + assert.NoError(err) + assert.Equal(pb.RateLimitResponse_OK, detailedResponse.OverallCode) + + runner.GetStatsStore().Flush() + + detailedSanitizedCounter := runner.GetStatsStore().NewCounter( + fmt.Sprintf("ratelimit.service.rate_limit.sanitize.detailed_path_%s.total_hits", sanitizedValue)) + assert.Equal(1, int(detailedSanitizedCounter.Value())) + + detailedUnsanitizedCounter := runner.GetStatsStore().NewCounter( + fmt.Sprintf("ratelimit.service.rate_limit.sanitize.detailed_path_%s.total_hits", grpcPath)) + assert.Equal(0, int(detailedUnsanitizedCounter.Value())) + }) +} + func TestBasicTLSConfig(t *testing.T) { t.Run("WithoutPerSecondRedisTLS", testBasicConfigAuthTLS(false, 0)) t.Run("WithPerSecondRedisTLS", testBasicConfigAuthTLS(true, 0)) diff --git a/test/integration/runtime/current/ratelimit/config/sanitize.yaml b/test/integration/runtime/current/ratelimit/config/sanitize.yaml new file mode 100644 index 000000000..b04131d1b --- /dev/null +++ b/test/integration/runtime/current/ratelimit/config/sanitize.yaml @@ -0,0 +1,12 @@ +domain: sanitize +descriptors: + - key: grpc_path + value_to_metric: true + rate_limit: + unit: minute + requests_per_unit: 50 + - key: detailed_path + detailed_metric: true + rate_limit: + unit: minute + requests_per_unit: 50 diff --git a/test/mocks/stats/manager.go b/test/mocks/stats/manager.go index 253926410..82e7e3821 100644 --- a/test/mocks/stats/manager.go +++ b/test/mocks/stats/manager.go @@ -9,13 +9,18 @@ import ( ) type MockStatManager struct { - store gostats.Store + store gostats.Store + sanitizeDescriptorMetricDots bool } func (m *MockStatManager) GetStatsStore() gostats.Store { return m.store } +func (m *MockStatManager) SanitizeDescriptorMetricDots() bool { + return m.sanitizeDescriptorMetricDots +} + func (m *MockStatManager) NewShouldRateLimitStats() stats.ShouldRateLimitStats { s := m.store.Scope("call.should_rate_limit") ret := stats.ShouldRateLimitStats{} @@ -61,3 +66,9 @@ func (m *MockStatManager) NewDomainStats(key string) stats.DomainStats { func NewMockStatManager(store gostats.Store) stats.Manager { return &MockStatManager{store: store} } + +// NewMockStatManagerWithSanitize builds a mock stat manager with the +// SANITIZE_DESCRIPTOR_METRIC_DOTS behavior toggled on/off. +func NewMockStatManagerWithSanitize(store gostats.Store, sanitizeDescriptorMetricDots bool) stats.Manager { + return &MockStatManager{store: store, sanitizeDescriptorMetricDots: sanitizeDescriptorMetricDots} +} diff --git a/test/stats/manager_impl_test.go b/test/stats/manager_impl_test.go index 793ce835e..3e4b293a0 100644 --- a/test/stats/manager_impl_test.go +++ b/test/stats/manager_impl_test.go @@ -54,3 +54,11 @@ func TestEscapingInvalidChartersInMetricName(t *testing.T) { }) } } + +func TestSanitizeDescriptorMetricDotsAccessor(t *testing.T) { + statsStore := gostats.NewStore(gostatsMock.NewSink(), false) + + assert.False(t, stats.NewStatManager(statsStore, settings.Settings{}).SanitizeDescriptorMetricDots(), + "defaults to off for backward compatibility") + assert.True(t, stats.NewStatManager(statsStore, settings.Settings{SanitizeDescriptorMetricDots: true}).SanitizeDescriptorMetricDots()) +} diff --git a/test/utils/utilities_test.go b/test/utils/utilities_test.go index cadd2983f..efc735b71 100644 --- a/test/utils/utilities_test.go +++ b/test/utils/utilities_test.go @@ -138,3 +138,21 @@ func TestExpirationSecondsNonMonthDoesNotUseTimeSource(t *testing.T) { seconds := utils.ExpirationSeconds(pb.RateLimitResponse_RateLimit_DAY, timeSource, true) assert.EqualValues(t, 60*60*24, seconds) } + +func TestSanitizeStatKeyValue(t *testing.T) { + cases := []struct { + in string + want string + }{ + {"a.b.c", "a_b_c"}, + {"nodots", "nodots"}, + {"", ""}, + {"10.0.0.1", "10_0_0_1"}, + {"foo.bar", "foo_bar"}, + {".leading", "_leading"}, + {"trailing.", "trailing_"}, + } + for _, c := range cases { + assert.Equal(t, c.want, utils.SanitizeStatKeyValue(c.in)) + } +}