Skip to content

Commit

Permalink
Make name validation configurable
Browse files Browse the repository at this point in the history
  • Loading branch information
prymitive committed Jan 17, 2025
1 parent bb69aa3 commit c871eb7
Show file tree
Hide file tree
Showing 28 changed files with 205 additions and 81 deletions.
1 change: 1 addition & 0 deletions .github/spellcheck/wordlist.txt
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ unmarshal
uptime
URI
URIs
utf
UTF
validator
VCS
Expand Down
3 changes: 3 additions & 0 deletions cmd/pint/bench_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"testing"

"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/common/model"
"github.com/stretchr/testify/require"

"github.com/cloudflare/pint/internal/config"
Expand All @@ -27,6 +28,7 @@ func BenchmarkFindEntries(b *testing.B) {
[]string{"bench/rules"},
git.NewPathFilter(nil, nil, nil),
parser.PrometheusSchema,
model.UTF8Validation,
nil,
)
for n := 0; n < b.N; n++ {
Expand All @@ -41,6 +43,7 @@ func BenchmarkCheckRules(b *testing.B) {
[]string{"bench/rules"},
git.NewPathFilter(nil, nil, nil),
parser.PrometheusSchema,
model.UTF8Validation,
nil,
)
entries, err := finder.Find()
Expand Down
14 changes: 12 additions & 2 deletions cmd/pint/ci.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import (
"strings"
"time"

"github.com/prometheus/common/model"

"github.com/cloudflare/pint/internal/checks"
"github.com/cloudflare/pint/internal/config"
"github.com/cloudflare/pint/internal/discovery"
Expand Down Expand Up @@ -102,14 +104,15 @@ func actionCI(c *cli.Context) error {
)

schema := parseSchema(meta.cfg.Parser.Schema)
names := parseNames(meta.cfg.Parser.Names)
allowedOwners := meta.cfg.Owners.CompileAllowed()
var entries []discovery.Entry
entries, err = discovery.NewGlobFinder([]string{"*"}, filter, schema, allowedOwners).Find()
entries, err = discovery.NewGlobFinder([]string{"*"}, filter, schema, names, allowedOwners).Find()
if err != nil {
return err
}

entries, err = discovery.NewGitBranchFinder(git.RunGit, filter, baseBranch, meta.cfg.CI.MaxCommits, schema, allowedOwners).Find(entries)
entries, err = discovery.NewGitBranchFinder(git.RunGit, filter, baseBranch, meta.cfg.CI.MaxCommits, schema, names, allowedOwners).Find(entries)
if err != nil {
return err
}
Expand Down Expand Up @@ -359,3 +362,10 @@ func parseSchema(s string) parser.Schema {
}
return parser.PrometheusSchema
}

func parseNames(s string) model.ValidationScheme {
if s == config.NamesLegacy {
return model.LegacyValidation
}
return model.UTF8Validation
}
1 change: 1 addition & 0 deletions cmd/pint/lint.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ func actionLint(c *cli.Context) error {
config.MustCompileRegexes(meta.cfg.Parser.Relaxed...),
),
parseSchema(meta.cfg.Parser.Schema),
parseNames(meta.cfg.Parser.Names),
allowedOwners,
)
entries, err := finder.Find()
Expand Down
6 changes: 6 additions & 0 deletions cmd/pint/tests/0166_invalid_label.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
cmp stderr stderr.txt

-- stderr.txt --
level=INFO msg="Loading configuration file" path=.pint.hcl
level=INFO msg="Finding all rules to check" paths=["rules"]
level=INFO msg="Checking Prometheus rules" entries=2 workers=10 online=true
rules/1.yaml:7 Fatal: This rule is not a valid Prometheus rule: `invalid annotation name: {{ $value }}`. (yaml/parse)
Expand All @@ -25,3 +26,8 @@ groups:
expr: up == 0
labels:
"{{ $value }}": "down"

-- .pint.hcl --
parser {
names = "legacy"
}
11 changes: 7 additions & 4 deletions cmd/pint/watch.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"github.com/prometheus/client_golang/prometheus/collectors"
"github.com/prometheus/client_golang/prometheus/promhttp"
dto "github.com/prometheus/client_model/go"
"github.com/prometheus/common/model"
"github.com/urfave/cli/v2"
)

Expand Down Expand Up @@ -209,12 +210,13 @@ func actionWatch(c *cli.Context, meta actionMeta, f pathFinderFunc) error {
}

schema := parseSchema(meta.cfg.Parser.Schema)
names := parseNames(meta.cfg.Parser.Names)
allowedOwners := meta.cfg.Owners.CompileAllowed()

// start timer to run every $interval
ack := make(chan bool, 1)
mainCtx, mainCancel := context.WithCancel(context.WithValue(context.Background(), config.CommandKey, config.WatchCommand))
stop := startTimer(mainCtx, meta.workers, meta.isOffline, gen, schema, allowedOwners, interval, ack, collector)
stop := startTimer(mainCtx, meta.workers, meta.isOffline, gen, schema, names, allowedOwners, interval, ack, collector)

quit := make(chan os.Signal, 1)
signal.Notify(quit, os.Interrupt, syscall.SIGINT, syscall.SIGTERM)
Expand All @@ -238,7 +240,7 @@ func actionWatch(c *cli.Context, meta actionMeta, f pathFinderFunc) error {
return nil
}

func startTimer(ctx context.Context, workers int, isOffline bool, gen *config.PrometheusGenerator, schema parser.Schema, allowedOwners []*regexp.Regexp, interval time.Duration, ack chan bool, collector *problemCollector) chan bool {
func startTimer(ctx context.Context, workers int, isOffline bool, gen *config.PrometheusGenerator, schema parser.Schema, names model.ValidationScheme, allowedOwners []*regexp.Regexp, interval time.Duration, ack chan bool, collector *problemCollector) chan bool {
ticker := time.NewTicker(time.Second)
stop := make(chan bool, 1)
wasBootstrapped := false
Expand All @@ -252,7 +254,7 @@ func startTimer(ctx context.Context, workers int, isOffline bool, gen *config.Pr
ticker.Reset(interval)
wasBootstrapped = true
}
if err := collector.scan(ctx, workers, isOffline, gen, schema, allowedOwners); err != nil {
if err := collector.scan(ctx, workers, isOffline, gen, schema, names, allowedOwners); err != nil {
slog.Error("Got an error when running checks", slog.Any("err", err))
}
checkIterationsTotal.Inc()
Expand Down Expand Up @@ -310,7 +312,7 @@ func newProblemCollector(cfg config.Config, f pathFinderFunc, minSeverity checks
}
}

func (c *problemCollector) scan(ctx context.Context, workers int, isOffline bool, gen *config.PrometheusGenerator, schema parser.Schema, allowedOwners []*regexp.Regexp) error {
func (c *problemCollector) scan(ctx context.Context, workers int, isOffline bool, gen *config.PrometheusGenerator, schema parser.Schema, names model.ValidationScheme, allowedOwners []*regexp.Regexp) error {
paths, err := c.finder(ctx)
if err != nil {
return fmt.Errorf("failed to get the list of paths to check: %w", err)
Expand All @@ -325,6 +327,7 @@ func (c *problemCollector) scan(ctx context.Context, workers int, isOffline bool
config.MustCompileRegexes(c.cfg.Parser.Relaxed...),
),
schema,
names,
allowedOwners,
).Find()
if err != nil {
Expand Down
7 changes: 7 additions & 0 deletions docs/changelog.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
# Changelog

## v0.71.0

### Added

- Added `names` option to the `parser` block, which controls how does Prometheus validates
label names.

## v0.70.0

### Added
Expand Down
4 changes: 4 additions & 0 deletions docs/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ Syntax:
```js
parser {
schema = "prometheus|thanos"
names = "legacy|utf-8"
include = [ "(.*)", ... ]
exclude = [ "(.*)", ... ]
relaxed = [ "(.*)", ... ]
Expand All @@ -81,6 +82,9 @@ parser {
in [Thanos Rule](https://thanos.io/tip/components/rule.md/) docs, which currently allows for setting
an extra key on the rule group object - `partial_response_strategy`.
Default value is `prometheus`.
- `names` - validation scheme for label names. This control if Prometheus libraries are
allowing UTF-8 in label **names** or not. See [utf-8 guide](https://prometheus.io/docs/guides/utf8/).
Default value is `utf-8`.
- `include` - list of regexp patterns to check when running checks. Only files
matching those regexp rules will be checked, other modified files will be ignored.
- `exclude` - list of regexp patterns to ignore when running checks.
Expand Down
2 changes: 1 addition & 1 deletion internal/checks/base_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ func runTests(t *testing.T, testCases []checkTest) {
}

func parseContent(content string) (entries []discovery.Entry, err error) {
p := parser.NewParser(false, parser.PrometheusSchema)
p := parser.NewParser(false, parser.PrometheusSchema, model.UTF8Validation)
rules, err := p.Parse([]byte(content))
if err != nil {
return nil, err
Expand Down
3 changes: 2 additions & 1 deletion internal/checks/template_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package checks_test
import (
"testing"

"github.com/prometheus/common/model"
"github.com/stretchr/testify/require"

"github.com/cloudflare/pint/internal/checks"
Expand Down Expand Up @@ -105,7 +106,7 @@ func TestTemplatedRegexpExpand(t *testing.T) {
}

func newMustRule(content string) parser.Rule {
p := parser.NewParser(false, parser.PrometheusSchema)
p := parser.NewParser(false, parser.PrometheusSchema, model.UTF8Validation)
rules, err := p.Parse([]byte(content))
if err != nil {
panic(err)
Expand Down
3 changes: 2 additions & 1 deletion internal/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (

"github.com/gkampitakis/go-snaps/snaps"
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/common/model"
"github.com/stretchr/testify/require"

"github.com/cloudflare/pint/internal/checks"
Expand Down Expand Up @@ -159,7 +160,7 @@ func TestSetDisabledChecks(t *testing.T) {
}

func newRule(t *testing.T, content string) parser.Rule {
p := parser.NewParser(false, parser.PrometheusSchema)
p := parser.NewParser(false, parser.PrometheusSchema, model.UTF8Validation)
rules, err := p.Parse([]byte(content))
if err != nil {
t.Error(err)
Expand Down
18 changes: 18 additions & 0 deletions internal/config/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,14 @@ import (
const (
SchemaPrometheus = "prometheus"
SchemaThanos = "thanos"

NamesLegacy = "legacy"
NamesUTF8 = "utf-8"
)

type Parser struct {
Schema string `hcl:"schema,optional" json:"schema,omitempty"`
Names string `hcl:"names,optional" json:"names,omitempty"`
Relaxed []string `hcl:"relaxed,optional" json:"relaxed,omitempty"`
Include []string `hcl:"include,optional" json:"include,omitempty"`
Exclude []string `hcl:"exclude,optional" json:"exclude,omitempty"`
Expand All @@ -24,6 +28,13 @@ func (p Parser) getSchema() string {
return p.Schema
}

func (p Parser) getNames() string {
if p.Names == "" {
return NamesUTF8
}
return p.Names
}

func (p Parser) validate() error {
switch s := p.getSchema(); s {
case SchemaPrometheus:
Expand All @@ -32,6 +43,13 @@ func (p Parser) validate() error {
return fmt.Errorf("unsupported parser scheme: %s", s)
}

switch n := p.getNames(); n {
case NamesUTF8:
case NamesLegacy:
default:
return fmt.Errorf("unsupported parser names: %s", n)
}

for _, pattern := range p.Relaxed {
_, err := regexp.Compile(pattern)
if err != nil {
Expand Down
3 changes: 1 addition & 2 deletions internal/discovery/discovery.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ type Entry struct {
State ChangeType
}

func readRules(reportedPath, sourcePath string, r io.Reader, isStrict bool, schema parser.Schema, allowedOwners []*regexp.Regexp) (entries []Entry, err error) {
func readRules(reportedPath, sourcePath string, r io.Reader, p parser.Parser, allowedOwners []*regexp.Regexp) (entries []Entry, err error) {
content, err := parser.ReadContent(r)
if err != nil {
return nil, err
Expand Down Expand Up @@ -159,7 +159,6 @@ func readRules(reportedPath, sourcePath string, r io.Reader, isStrict bool, sche
return entries, nil
}

p := parser.NewParser(isStrict, schema)
rules, err := p.Parse(content.Body)
if err != nil {
slog.Warn(
Expand Down
6 changes: 4 additions & 2 deletions internal/discovery/discovery_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"strings"
"testing"

"github.com/prometheus/common/model"
"github.com/stretchr/testify/require"

"github.com/cloudflare/pint/internal/parser"
Expand All @@ -24,7 +25,7 @@ func (r failingReader) Read(_ []byte) (int, error) {

func TestReadRules(t *testing.T) {
mustParse := func(offset int, s string) parser.Rule {
p := parser.NewParser(false, parser.PrometheusSchema)
p := parser.NewParser(false, parser.PrometheusSchema, model.UTF8Validation)
r, err := p.Parse([]byte(strings.Repeat("\n", offset) + s))
if err != nil {
panic(fmt.Sprintf("failed to parse rule:\n---\n%s\n---\nerror: %s", s, err))
Expand Down Expand Up @@ -338,7 +339,8 @@ groups:
fmt.Sprintf("rPath=%s sPath=%s strict=%v title=%s", tc.reportedPath, tc.sourcePath, tc.isStrict, tc.title),
func(t *testing.T) {
r := tc.sourceFunc(t)
entries, err := readRules(tc.reportedPath, tc.sourcePath, r, tc.isStrict, parser.PrometheusSchema, nil)
p := parser.NewParser(tc.isStrict, parser.PrometheusSchema, model.UTF8Validation)
entries, err := readRules(tc.reportedPath, tc.sourcePath, r, p, nil)
if tc.err != "" {
require.EqualError(t, err, tc.err)
} else {
Expand Down
12 changes: 8 additions & 4 deletions internal/discovery/git_branch.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import (
"sort"
"strings"

"github.com/prometheus/common/model"

"github.com/cloudflare/pint/internal/git"
"github.com/cloudflare/pint/internal/output"
"github.com/cloudflare/pint/internal/parser"
Expand All @@ -20,6 +22,7 @@ func NewGitBranchFinder(
baseBranch string,
maxCommits int,
schema parser.Schema,
names model.ValidationScheme,
allowedOwners []*regexp.Regexp,
) GitBranchFinder {
return GitBranchFinder{
Expand All @@ -28,6 +31,7 @@ func NewGitBranchFinder(
baseBranch: baseBranch,
maxCommits: maxCommits,
schema: schema,
names: names,
allowedOwners: allowedOwners,
}
}
Expand All @@ -39,6 +43,7 @@ type GitBranchFinder struct {
allowedOwners []*regexp.Regexp
maxCommits int
schema parser.Schema
names model.ValidationScheme
}

func (f GitBranchFinder) Find(allEntries []Entry) (entries []Entry, err error) {
Expand All @@ -60,13 +65,13 @@ func (f GitBranchFinder) Find(allEntries []Entry) (entries []Entry, err error) {
}

for _, change := range changes {
p := parser.NewParser(!f.filter.IsRelaxed(change.Path.Before.Name), f.schema, f.names)
var entriesBefore, entriesAfter []Entry
entriesBefore, err = readRules(
change.Path.Before.EffectivePath(),
change.Path.Before.Name,
bytes.NewReader(change.Body.Before),
!f.filter.IsRelaxed(change.Path.Before.Name),
f.schema,
p,
nil,
)
if err != nil {
Expand All @@ -76,8 +81,7 @@ func (f GitBranchFinder) Find(allEntries []Entry) (entries []Entry, err error) {
change.Path.After.EffectivePath(),
change.Path.After.Name,
bytes.NewReader(change.Body.After),
!f.filter.IsRelaxed(change.Path.After.Name),
f.schema,
p,
f.allowedOwners,
)
if err != nil {
Expand Down
Loading

0 comments on commit c871eb7

Please sign in to comment.