Skip to content

feat: support configurable name validation (legacy or UTF-8 schemes) #6916

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
* [FEATURE] Compactor: Add support for percentage based sharding for compactors. #6738
* [FEATURE] Querier: Allow choosing PromQL engine via header. #6777
* [FEATURE] Querier: Support for configuring query optimizers and enabling XFunctions in the Thanos engine. #6873
* [FEATURE] Config: Name validation scheme for metric and label names can be set using the config file (`name_validation_scheme`) as well as a CLI flag (`-name.validation-scheme`)
* [FEATURE] Query Frontend: Add support /api/v1/format_query API for formatting queries. #6893
* [ENHANCEMENT] Querier: Support snappy and zstd response compression for `-querier.response-compression` flag. #6848
* [ENHANCEMENT] Tenant Federation: Add a # of query result limit logic when the `-tenant-federation.regex-matcher-enabled` is enabled. #6845
Expand Down
4 changes: 4 additions & 0 deletions docs/configuration/config-file-reference.md
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,10 @@ Where default_value is the value to use if the environment variable is undefined
# CLI flag: -http.prefix
[http_prefix: <string> | default = "/api/prom"]

# Set to "legacy" to enforce strict legacy-compatible name rules.
# CLI flag: -name.validation-scheme
[name_validation_scheme: <legacy | utf8> | default = "legacy"]

resource_monitor:
# Comma-separated list of resources to monitor. Supported values are cpu and
# heap, which tracks metrics from github.com/prometheus/procfs and
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ require (
github.com/prometheus/client_model v0.6.2
github.com/prometheus/common v0.63.0
// Prometheus maps version 2.x.y to tags v0.x.y.
github.com/prometheus/prometheus v0.303.1
github.com/prometheus/prometheus v1.99.0
github.com/segmentio/fasthash v1.0.3
github.com/sony/gobreaker v1.0.0
github.com/spf13/afero v1.11.0
Expand Down
13 changes: 7 additions & 6 deletions pkg/alertmanager/alertmanagerpb/alertmanager.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 4 additions & 3 deletions pkg/alertmanager/alertspb/alerts.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 0 additions & 8 deletions pkg/cortex/configinit/init.go

This file was deleted.

27 changes: 20 additions & 7 deletions pkg/cortex/cortex.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import (
"github.com/go-kit/log/level"
"github.com/pkg/errors"
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/common/model"
prom_config "github.com/prometheus/prometheus/config"
"github.com/prometheus/prometheus/promql"
prom_storage "github.com/prometheus/prometheus/storage"
"github.com/weaveworks/common/server"
Expand All @@ -31,7 +33,6 @@ import (
"github.com/cortexproject/cortex/pkg/configs"
configAPI "github.com/cortexproject/cortex/pkg/configs/api"
"github.com/cortexproject/cortex/pkg/configs/db"
_ "github.com/cortexproject/cortex/pkg/cortex/configinit"
"github.com/cortexproject/cortex/pkg/cortex/storage"
"github.com/cortexproject/cortex/pkg/cortexpb"
"github.com/cortexproject/cortex/pkg/distributor"
Expand Down Expand Up @@ -90,11 +91,12 @@ var (

// Config is the root config for Cortex.
type Config struct {
Target flagext.StringSliceCSV `yaml:"target"`
AuthEnabled bool `yaml:"auth_enabled"`
PrintConfig bool `yaml:"-"`
HTTPPrefix string `yaml:"http_prefix"`
ResourceMonitor configs.ResourceMonitor `yaml:"resource_monitor"`
Target flagext.StringSliceCSV `yaml:"target"`
AuthEnabled bool `yaml:"auth_enabled"`
PrintConfig bool `yaml:"-"`
HTTPPrefix string `yaml:"http_prefix"`
ResourceMonitor configs.ResourceMonitor `yaml:"resource_monitor"`
NameValidationScheme string `yaml:"name_validation_scheme"`

ExternalQueryable prom_storage.Queryable `yaml:"-"`
ExternalPusher ruler.Pusher `yaml:"-"`
Expand Down Expand Up @@ -146,6 +148,7 @@ func (c *Config) RegisterFlags(f *flag.FlagSet) {
f.BoolVar(&c.AuthEnabled, "auth.enabled", true, "Set to false to disable auth.")
f.BoolVar(&c.PrintConfig, "print.config", false, "Print the config and exit.")
f.StringVar(&c.HTTPPrefix, "http.prefix", "/api/prom", "HTTP path prefix for Cortex API.")
f.StringVar(&c.NameValidationScheme, "name.validation-scheme", "legacy", "Validation scheme for metric and label names. Set to utf8 to allow UTF-8 characters.")

c.API.RegisterFlags(f)
c.registerServerFlagsWithChangedDefaultValues(f)
Expand Down Expand Up @@ -181,6 +184,11 @@ func (c *Config) RegisterFlags(f *flag.FlagSet) {
// Validate the cortex config and returns an error if the validation
// doesn't pass
func (c *Config) Validate(log log.Logger) error {
switch c.NameValidationScheme {
case "", prom_config.LegacyValidationConfig, prom_config.UTF8ValidationConfig:
default:
return fmt.Errorf("invalid name validation scheme: %s", c.NameValidationScheme)
}
if err := c.validateYAMLEmptyNodes(); err != nil {
return err
}
Expand Down Expand Up @@ -349,7 +357,12 @@ func New(cfg Config) (*Cortex, error) {
}
os.Exit(0)
}

//nolint:staticcheck // SA1019: using deprecated NameValidationScheme intentionally as a temporary compatibility workaround for UTF-8 migration issues
if cfg.NameValidationScheme == prom_config.UTF8ValidationConfig {
model.NameValidationScheme = model.UTF8Validation
} else {
model.NameValidationScheme = model.LegacyValidation
}
// Swap out the default resolver to support multiple tenant IDs separated by a '|'
if cfg.TenantFederation.Enabled {
util_log.WarnExperimentalUse("tenant-federation")
Expand Down
37 changes: 37 additions & 0 deletions pkg/cortex/cortex_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"time"

"github.com/prometheus/client_golang/prometheus"
prom_config "github.com/prometheus/prometheus/config"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/weaveworks/common/server"
Expand Down Expand Up @@ -217,6 +218,42 @@ func TestConfigValidation(t *testing.T) {
},
expectedError: nil,
},
{
name: "should not fail validation for empty name validation scheme (use legacy by default)",
getTestConfig: func() *Config {
configuration := newDefaultConfig()
configuration.NameValidationScheme = ""
return configuration
},
expectedError: nil,
},
{
name: "should not fail validation for legacy name validation scheme",
getTestConfig: func() *Config {
configuration := newDefaultConfig()
configuration.NameValidationScheme = prom_config.LegacyValidationConfig
return configuration
},
expectedError: nil,
},
{
name: "should not fail validation for utf-8 name validation scheme",
getTestConfig: func() *Config {
configuration := newDefaultConfig()
configuration.NameValidationScheme = prom_config.UTF8ValidationConfig
return configuration
},
expectedError: nil,
},
{
name: "should fail validation for invalid name validation scheme",
getTestConfig: func() *Config {
configuration := newDefaultConfig()
configuration.NameValidationScheme = "invalid"
return configuration
},
expectedError: fmt.Errorf("invalid name validation scheme"),
},
} {
t.Run(tc.name, func(t *testing.T) {
err := tc.getTestConfig().Validate(nil)
Expand Down
5 changes: 3 additions & 2 deletions pkg/cortexpb/cortex.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

108 changes: 94 additions & 14 deletions pkg/distributor/distributor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ import (
"google.golang.org/grpc/status"

promchunk "github.com/cortexproject/cortex/pkg/chunk/encoding"
_ "github.com/cortexproject/cortex/pkg/cortex/configinit"
"github.com/cortexproject/cortex/pkg/cortexpb"
"github.com/cortexproject/cortex/pkg/ha"
"github.com/cortexproject/cortex/pkg/ingester"
Expand Down Expand Up @@ -2037,59 +2036,140 @@ func TestDistributor_Push_ShouldGuaranteeShardingTokenConsistencyOverTheTime(t *
}
}

// func TestDistributor_Push_LabelNameValidation(t *testing.T) {
// model.NameValidationScheme = model.LegacyValidation
// t.Parallel()
// inputLabels := labels.Labels{
// {Name: model.MetricNameLabel, Value: "foo"},
// {Name: "999.illegal", Value: "baz"},
// }
// ctx := user.InjectOrgID(context.Background(), "user")

// tests := map[string]struct {
// inputLabels labels.Labels
// skipLabelNameValidationCfg bool
// skipLabelNameValidationReq bool
// errExpected bool
// errMessage string
// }{
// "label name validation is on by default": {
// inputLabels: inputLabels,
// errExpected: true,
// errMessage: `sample invalid label: "999.illegal" metric "foo{999.illegal=\"baz\"}"`,
// },
// "label name validation can be skipped via config": {
// inputLabels: inputLabels,
// skipLabelNameValidationCfg: true,
// errExpected: false,
// },
// "label name validation can be skipped via WriteRequest parameter": {
// inputLabels: inputLabels,
// skipLabelNameValidationReq: true,
// errExpected: false,
// },
// }

// for testName, tc := range tests {
// tc := tc
// for _, histogram := range []bool{true, false} {
// histogram := histogram
// t.Run(fmt.Sprintf("%s, histogram=%s", testName, strconv.FormatBool(histogram)), func(t *testing.T) {
// t.Parallel()
// ds, _, _, _ := prepare(t, prepConfig{
// numIngesters: 2,
// happyIngesters: 2,
// numDistributors: 1,
// shuffleShardSize: 1,
// skipLabelNameValidation: tc.skipLabelNameValidationCfg,
// })
// req := mockWriteRequest([]labels.Labels{tc.inputLabels}, 42, 100000, histogram)
// req.SkipLabelNameValidation = tc.skipLabelNameValidationReq
// _, err := ds[0].Push(ctx, req)
// if tc.errExpected {
// fromError, _ := status.FromError(err)
// assert.Equal(t, tc.errMessage, fromError.Message())
// } else {
// assert.Nil(t, err)
// }
// })
// }
// }
// }
func TestDistributor_Push_LabelNameValidation(t *testing.T) {
t.Parallel()
inputLabels := labels.Labels{
{Name: model.MetricNameLabel, Value: "foo"},
{Name: "999.illegal", Value: "baz"},
}
ctx := user.InjectOrgID(context.Background(), "user")

tests := map[string]struct {
inputLabels labels.Labels
skipLabelNameValidationCfg bool
skipLabelNameValidationReq bool
useUTF8Validation bool
errExpected bool
errMessage string
}{
"label name validation is on by default": {
inputLabels: inputLabels,
inputLabels: labels.Labels{
{Name: model.MetricNameLabel, Value: "foo"},
{Name: "999.illegal", Value: "baz"},
},
errExpected: true,
errMessage: `sample invalid label: "999.illegal" metric "foo{999.illegal=\"baz\"}"`,
},
"label name validation can be skipped via config": {
inputLabels: inputLabels,
inputLabels: labels.Labels{
{Name: model.MetricNameLabel, Value: "foo"},
{Name: "999.illegal", Value: "baz"},
},
skipLabelNameValidationCfg: true,
errExpected: false,
},
"label name validation can be skipped via WriteRequest parameter": {
inputLabels: inputLabels,
inputLabels: labels.Labels{
{Name: model.MetricNameLabel, Value: "foo"},
{Name: "999.illegal", Value: "baz"},
},
skipLabelNameValidationReq: true,
errExpected: false,
},
"UTF-8 validation allows Unicode label names": {
inputLabels: labels.Labels{
{Name: model.MetricNameLabel, Value: "foo"},
{Name: "Cortex_😃", Value: "baz"},
},
useUTF8Validation: true,
errExpected: false,
},
}

for testName, tc := range tests {
tc := tc
for _, histogram := range []bool{true, false} {
histogram := histogram
t.Run(fmt.Sprintf("%s, histogram=%s", testName, strconv.FormatBool(histogram)), func(t *testing.T) {
t.Parallel()
t.Run(fmt.Sprintf("%s, histogram=%v", testName, histogram), func(t *testing.T) {
if tc.useUTF8Validation {
// nolint:staticcheck // SA1019: using deprecated NameValidationScheme intentionally for legacy validation testing
model.NameValidationScheme = model.UTF8Validation
} else {
// nolint:staticcheck // SA1019: using deprecated NameValidationScheme intentionally for legacy validation testing
model.NameValidationScheme = model.LegacyValidation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't be modifying model.NameValidationScheme directly in the test case. Let's use the new config we added instead.

}

ds, _, _, _ := prepare(t, prepConfig{
numIngesters: 2,
happyIngesters: 2,
numDistributors: 1,
shuffleShardSize: 1,
skipLabelNameValidation: tc.skipLabelNameValidationCfg,
})

req := mockWriteRequest([]labels.Labels{tc.inputLabels}, 42, 100000, histogram)
req.SkipLabelNameValidation = tc.skipLabelNameValidationReq

_, err := ds[0].Push(ctx, req)

if tc.errExpected {
require.Error(t, err)
fromError, _ := status.FromError(err)
assert.Equal(t, tc.errMessage, fromError.Message())
} else {
assert.Nil(t, err)
require.NoError(t, err)
}
})
}
Expand Down
3 changes: 2 additions & 1 deletion pkg/distributor/distributorpb/distributor.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading