diff --git a/.github/spellcheck/wordlist.txt b/.github/spellcheck/wordlist.txt index ba5e0629..ed9b61d7 100644 --- a/.github/spellcheck/wordlist.txt +++ b/.github/spellcheck/wordlist.txt @@ -39,6 +39,7 @@ promql PromQL PRs prymitive +rulefmt samber SNI symlink diff --git a/cmd/pint/scan.go b/cmd/pint/scan.go index 261f5716..031157ea 100644 --- a/cmd/pint/scan.go +++ b/cmd/pint/scan.go @@ -5,12 +5,9 @@ import ( "errors" "fmt" "log/slog" - "regexp" - "strconv" "sync" "time" - "github.com/prometheus/prometheus/model/rulefmt" "go.uber.org/atomic" "github.com/cloudflare/pint/internal/checks" @@ -22,41 +19,14 @@ import ( "github.com/cloudflare/pint/internal/reporter" ) -var ( - yamlErrRe = regexp.MustCompile("^yaml: line (.+): (.+)") - yamlUnmarshalErrRe = regexp.MustCompile("^yaml: unmarshal errors:\n line (.+): (.+)") - rulefmtGroupRe = regexp.MustCompile("^([0-9]+):[0-9]+: group \".+\", rule [0-9]+, (.+)") - rulefmtGroupnameRe = regexp.MustCompile("^([0-9]+):[0-9]+: (groupname: .+)") -) - const ( yamlParseReporter = "yaml/parse" ignoreFileReporter = "ignore/file" pintCommentReporter = "pint/comment" -) - -func tryDecodingYamlError(err error) (l int, s string) { - s = err.Error() - - werr := &rulefmt.WrappedError{} - if errors.As(err, &werr) { - if uerr := werr.Unwrap(); uerr != nil { - s = uerr.Error() - } - } - for _, re := range []*regexp.Regexp{yamlErrRe, yamlUnmarshalErrRe, rulefmtGroupRe, rulefmtGroupnameRe} { - parts := re.FindStringSubmatch(err.Error()) - if len(parts) > 2 { - line, err2 := strconv.Atoi(parts[1]) - if err2 != nil || line <= 0 { - return 1, s - } - return line, parts[2] - } - } - return 1, s -} + yamlDetails = `This Prometheus rule is not valid. +This usually means that it's missing some required fields.` +) func checkRules(ctx context.Context, workers int, isOffline bool, gen *config.PrometheusGenerator, cfg config.Config, entries []discovery.Entry) (summary reporter.Summary, err error) { if isOffline { @@ -224,7 +194,6 @@ func scanWorker(ctx context.Context, jobs <-chan scanJob, results chan<- reporte Owner: job.entry.Owner, } case job.entry.PathError != nil: - line, e := tryDecodingYamlError(job.entry.PathError) results <- reporter.Report{ Path: discovery.Path{ Name: job.entry.Path.Name, @@ -233,11 +202,11 @@ func scanWorker(ctx context.Context, jobs <-chan scanJob, results chan<- reporte ModifiedLines: job.entry.ModifiedLines, Problem: checks.Problem{ Lines: parser.LineRange{ - First: line, - Last: line, + First: 1, + Last: 1, }, Reporter: yamlParseReporter, - Text: fmt.Sprintf("YAML parser returned an error when reading this file: `%s`.", e), + Text: fmt.Sprintf("YAML parser returned an error when reading this file: `%s`.", job.entry.PathError), Details: `pint cannot read this file because YAML parser returned an error. This usually means that you have an indention error or the file doesn't have the YAML structure required by Prometheus for [recording](https://prometheus.io/docs/prometheus/latest/configuration/recording_rules/) and [alerting](https://prometheus.io/docs/prometheus/latest/configuration/alerting_rules/) rules. If this file is a template that will be rendered into valid YAML then you can instruct pint to ignore some lines using comments, see [pint docs](https://cloudflare.github.io/pint/ignoring.html). @@ -247,6 +216,10 @@ If this file is a template that will be rendered into valid YAML then you can in Owner: job.entry.Owner, } case job.entry.Rule.Error.Err != nil: + details := yamlDetails + if job.entry.Rule.Error.Details != "" { + details = job.entry.Rule.Error.Details + } results <- reporter.Report{ Path: discovery.Path{ Name: job.entry.Path.Name, @@ -261,8 +234,7 @@ If this file is a template that will be rendered into valid YAML then you can in }, Reporter: yamlParseReporter, Text: fmt.Sprintf("This rule is not a valid Prometheus rule: `%s`.", job.entry.Rule.Error.Err.Error()), - Details: `This Prometheus rule is not valid. -This usually means that it's missing some required fields.`, + Details: details, Severity: checks.Fatal, }, Owner: job.entry.Owner, diff --git a/cmd/pint/tests/0004_fail_invalid_yaml.txt b/cmd/pint/tests/0004_fail_invalid_yaml.txt index e10d8088..dd82ac6d 100644 --- a/cmd/pint/tests/0004_fail_invalid_yaml.txt +++ b/cmd/pint/tests/0004_fail_invalid_yaml.txt @@ -5,9 +5,9 @@ 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=ERROR msg="Failed to parse file content" err="yaml: line 4: did not find expected key" path=rules/bad.yaml lines=1-7 -rules/bad.yaml:4 Fatal: YAML parser returned an error when reading this file: `did not find expected key`. (yaml/parse) - 4 | +level=WARN msg="Failed to parse file content" err="error at line 4: did not find expected key" path=rules/bad.yaml lines=1-7 +rules/bad.yaml:1 Fatal: YAML parser returned an error when reading this file: `error at line 4: did not find expected key`. (yaml/parse) + 1 | xxx: rules/ok.yml:5 Fatal: Prometheus failed to parse the query with this PromQL error: unclosed left bracket. (promql/syntax) 5 | expr: sum(foo[5m) diff --git a/cmd/pint/tests/0010_syntax_check.txt b/cmd/pint/tests/0010_syntax_check.txt index 65344f3c..4ee0f2a7 100644 --- a/cmd/pint/tests/0010_syntax_check.txt +++ b/cmd/pint/tests/0010_syntax_check.txt @@ -5,9 +5,9 @@ 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=ERROR msg="Failed to parse file content" err="yaml: line 6: did not find expected '-' indicator" path=rules/1.yaml lines=1-12 -rules/1.yaml:6 Fatal: YAML parser returned an error when reading this file: `did not find expected '-' indicator`. (yaml/parse) - 6 | +level=WARN msg="Failed to parse file content" err="error at line 6: did not find expected '-' indicator" path=rules/1.yaml lines=1-12 +rules/1.yaml:1 Fatal: YAML parser returned an error when reading this file: `error at line 6: did not find expected '-' indicator`. (yaml/parse) + 1 | - alert: Good level=INFO msg="Problems found" Fatal=1 level=ERROR msg="Fatal error" err="found 1 problem(s) with severity Bug or higher" diff --git a/cmd/pint/tests/0067_relaxed.txt b/cmd/pint/tests/0067_relaxed.txt index 85d4613f..7143a0c7 100644 --- a/cmd/pint/tests/0067_relaxed.txt +++ b/cmd/pint/tests/0067_relaxed.txt @@ -5,8 +5,9 @@ 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"] -rules/strict.yml:2 Fatal: YAML parser returned an error when reading this file: `cannot unmarshal !!seq into rulefmt.RuleGroups`. (yaml/parse) - 2 | - alert: No Owner +level=WARN msg="Failed to parse file content" err="error at line 2: top level field must be a groups key, got YAML list" path=rules/strict.yml lines=1-4 +rules/strict.yml:1 Fatal: YAML parser returned an error when reading this file: `error at line 2: top level field must be a groups key, got YAML list`. (yaml/parse) + 1 | {%- raw %} # pint ignore/line level=INFO msg="Problems found" Fatal=1 level=ERROR msg="Fatal error" err="found 1 problem(s) with severity Bug or higher" diff --git a/cmd/pint/tests/0071_ci_owner.txt b/cmd/pint/tests/0071_ci_owner.txt index 6f015024..41fe90a8 100644 --- a/cmd/pint/tests/0071_ci_owner.txt +++ b/cmd/pint/tests/0071_ci_owner.txt @@ -56,6 +56,7 @@ groups: -- src/.pint.hcl -- ci { + include = [".+.yml"] baseBranch = "main" } repository { diff --git a/cmd/pint/tests/0074_strict_error.txt b/cmd/pint/tests/0074_strict_error.txt index fa8f3a9b..e2465cf0 100644 --- a/cmd/pint/tests/0074_strict_error.txt +++ b/cmd/pint/tests/0074_strict_error.txt @@ -4,8 +4,9 @@ cmp stderr stderr.txt -- stderr.txt -- level=INFO msg="Finding all rules to check" paths=["rules"] -rules/strict.yml:2 Fatal: YAML parser returned an error when reading this file: `field alert not found in type rulefmt.RuleGroup`. (yaml/parse) - 2 | - alert: Conntrack_Table_Almost_Full +level=WARN msg="Failed to parse file content" err="error at line 2: invalid group key alert" path=rules/strict.yml lines=1-9 +rules/strict.yml:1 Fatal: YAML parser returned an error when reading this file: `error at line 2: invalid group key alert`. (yaml/parse) + 1 | groups: level=INFO msg="Problems found" Fatal=1 level=ERROR msg="Fatal error" err="found 1 problem(s) with severity Bug or higher" diff --git a/cmd/pint/tests/0076_ci_group_errors.txt b/cmd/pint/tests/0076_ci_group_errors.txt index 147cb245..d131ff65 100644 --- a/cmd/pint/tests/0076_ci_group_errors.txt +++ b/cmd/pint/tests/0076_ci_group_errors.txt @@ -75,6 +75,7 @@ groups: -- src/.pint.hcl -- ci { baseBranch = "main" + include = [".+.yml"] } repository { bitbucket { diff --git a/cmd/pint/tests/0078_repeated_group.txt b/cmd/pint/tests/0078_repeated_group.txt index ce84f272..7525cd84 100644 --- a/cmd/pint/tests/0078_repeated_group.txt +++ b/cmd/pint/tests/0078_repeated_group.txt @@ -4,8 +4,9 @@ cmp stderr stderr.txt -- stderr.txt -- level=INFO msg="Finding all rules to check" paths=["rules"] -rules/strict.yml:4 Fatal: YAML parser returned an error when reading this file: `groupname: "foo" is repeated in the same file`. (yaml/parse) - 4 | - name: foo +level=WARN msg="Failed to parse file content" err="error at line 4: duplicated group name" path=rules/strict.yml lines=1-5 +rules/strict.yml:1 Fatal: YAML parser returned an error when reading this file: `error at line 4: duplicated group name`. (yaml/parse) + 1 | groups: level=INFO msg="Problems found" Fatal=1 level=ERROR msg="Fatal error" err="found 1 problem(s) with severity Bug or higher" diff --git a/cmd/pint/tests/0123_ci_owner_allowed.txt b/cmd/pint/tests/0123_ci_owner_allowed.txt index 2fd6f720..e290a69d 100644 --- a/cmd/pint/tests/0123_ci_owner_allowed.txt +++ b/cmd/pint/tests/0123_ci_owner_allowed.txt @@ -65,6 +65,7 @@ owners { } ci { baseBranch = "main" + include = [".+.yml"] } repository { bitbucket { diff --git a/cmd/pint/tests/0160_ci_comment_edit.txt b/cmd/pint/tests/0160_ci_comment_edit.txt index 7184052e..9b7e2c55 100644 --- a/cmd/pint/tests/0160_ci_comment_edit.txt +++ b/cmd/pint/tests/0160_ci_comment_edit.txt @@ -100,6 +100,7 @@ groups: -- src/.pint.hcl -- ci { baseBranch = "main" + include = [".+.yml"] } prometheus "prom" { uri = "http://127.0.0.1:7160" diff --git a/cmd/pint/tests/0161_ci_move_files.txt b/cmd/pint/tests/0161_ci_move_files.txt index 40b60761..b219250a 100644 --- a/cmd/pint/tests/0161_ci_move_files.txt +++ b/cmd/pint/tests/0161_ci_move_files.txt @@ -78,6 +78,7 @@ groups: -- src/.pint.hcl -- ci { baseBranch = "main" + include = [".+.yml"] } prometheus "prom1" { uri = "http://127.0.0.1:7161/1" diff --git a/cmd/pint/tests/0162_ci_deleted_dependency.txt b/cmd/pint/tests/0162_ci_deleted_dependency.txt index 277216d6..c9873d0f 100644 --- a/cmd/pint/tests/0162_ci_deleted_dependency.txt +++ b/cmd/pint/tests/0162_ci_deleted_dependency.txt @@ -47,6 +47,7 @@ groups: -- src/.pint.hcl -- ci { baseBranch = "main" + include = [".+.yml"] } prometheus "prom" { uri = "http://127.0.0.1:7162" diff --git a/cmd/pint/tests/0167_rule_duplicate_symlink.txt b/cmd/pint/tests/0167_rule_duplicate_symlink.txt index c992a505..5fedc881 100644 --- a/cmd/pint/tests/0167_rule_duplicate_symlink.txt +++ b/cmd/pint/tests/0167_rule_duplicate_symlink.txt @@ -28,6 +28,7 @@ cmp stderr ../stderrV1.txt -- stderrV1.txt -- level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check on current git branch" base=main +level=WARN msg="Failed to parse file content" err="error at line 1: top level field must be a groups key, got YAML scalar value" path=.pint.hcl lines=1-20 level=INFO msg="Configured new Prometheus server" name=prom1 uris=1 uptime=up tags=[] include=["^rules.yml$"] exclude=[] level=INFO msg="Configured new Prometheus server" name=prom2 uris=1 uptime=up tags=[] include=["^symlink.yml$"] exclude=[] -- stderrV2.txt -- diff --git a/cmd/pint/tests/0173_rule_duplicate_move.txt b/cmd/pint/tests/0173_rule_duplicate_move.txt index b2c55999..821e4739 100644 --- a/cmd/pint/tests/0173_rule_duplicate_move.txt +++ b/cmd/pint/tests/0173_rule_duplicate_move.txt @@ -25,6 +25,7 @@ cmp stderr ../stderr.txt -- stderr.txt -- level=INFO msg="Loading configuration file" path=.pint.hcl level=INFO msg="Finding all rules to check on current git branch" base=main +level=WARN msg="Failed to parse file content" err="error at line 1: top level field must be a groups key, got YAML scalar value" path=.pint.hcl lines=1-24 level=INFO msg="Configured new Prometheus server" name=prom1 uris=1 uptime=up tags=[] include=["^rules/alert.*$"] exclude=[] level=INFO msg="Configured new Prometheus server" name=prom2a uris=1 uptime=up tags=[] include=["^rules/record.*$"] exclude=[] level=INFO msg="Configured new Prometheus server" name=prom2b uris=1 uptime=up tags=[] include=["^rules/record.*$"] exclude=[] diff --git a/docs/changelog.md b/docs/changelog.md index 7b0a8cd8..b346da97 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -7,6 +7,10 @@ - Don't suggest using `humanize` when alert template is already using printf on format the `$value`. - Fixed git history parsing when running `pint ci` on a branch that include merge commits. +### Changed + +- Refactored YAML syntax checks to avoid using [rulefmt.Parse](https://pkg.go.dev/github.com/prometheus/prometheus@v0.53.0/model/rulefmt#Parse) and effectively parsing rules twice. Some error messages will have different formatting. + ## v0.60.0 ### Fixed diff --git a/internal/checks/base_test.go b/internal/checks/base_test.go index 2481f851..35a761ae 100644 --- a/internal/checks/base_test.go +++ b/internal/checks/base_test.go @@ -173,7 +173,7 @@ func runTests(t *testing.T, testCases []checkTest) { } func parseContent(content string) (entries []discovery.Entry, err error) { - p := parser.NewParser() + p := parser.NewParser(false) rules, err := p.Parse([]byte(content)) if err != nil { return nil, err diff --git a/internal/checks/template_test.go b/internal/checks/template_test.go index c03d2fc4..e7d05b68 100644 --- a/internal/checks/template_test.go +++ b/internal/checks/template_test.go @@ -105,7 +105,7 @@ func TestTemplatedRegexpExpand(t *testing.T) { } func newMustRule(content string) parser.Rule { - p := parser.NewParser() + p := parser.NewParser(false) rules, err := p.Parse([]byte(content)) if err != nil { panic(err) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 36e5e687..08c46b55 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -133,7 +133,7 @@ func TestSetDisabledChecks(t *testing.T) { } func newRule(t *testing.T, content string) parser.Rule { - p := parser.NewParser() + p := parser.NewParser(false) rules, err := p.Parse([]byte(content)) if err != nil { t.Error(err) diff --git a/internal/discovery/discovery.go b/internal/discovery/discovery.go index be5fa005..152cefa3 100644 --- a/internal/discovery/discovery.go +++ b/internal/discovery/discovery.go @@ -6,10 +6,8 @@ import ( "fmt" "io" "log/slog" - "strings" "time" - "github.com/prometheus/prometheus/model/rulefmt" "golang.org/x/exp/slices" "github.com/cloudflare/pint/internal/comments" @@ -23,17 +21,6 @@ const ( RuleOwnerComment = "rule/owner" ) -var ignoredErrors = []string{ - "one of 'record' or 'alert' must be set", - "field 'expr' must be set in rule", - "could not parse expression: ", - "cannot unmarshal !!seq into rulefmt.ruleGroups", - ": template: __", - "invalid label name: ", - "invalid annotation name: ", - "invalid recording rule name: ", -} - type FileIgnoreError struct { Err error Line int @@ -43,16 +30,6 @@ func (fe FileIgnoreError) Error() string { return fe.Err.Error() } -func isStrictIgnored(err error) bool { - s := err.Error() - for _, ign := range ignoredErrors { - if strings.Contains(s, ign) { - return true - } - } - return false -} - type ChangeType uint8 func (c ChangeType) String() string { @@ -113,8 +90,6 @@ type Entry struct { } func readRules(reportedPath, sourcePath string, r io.Reader, isStrict bool) (entries []Entry, err error) { - p := parser.NewParser() - content, fileComments, err := parser.ReadContent(r) if err != nil { return nil, err @@ -182,36 +157,10 @@ func readRules(reportedPath, sourcePath string, r io.Reader, isStrict bool) (ent return entries, nil } - if isStrict { - if _, errs := rulefmt.Parse(content.Body); len(errs) > 0 { - seen := map[string]struct{}{} - for _, err := range errs { - if isStrictIgnored(err) { - continue - } - if _, ok := seen[err.Error()]; ok { - continue - } - seen[err.Error()] = struct{}{} - entries = append(entries, Entry{ - Path: Path{ - Name: sourcePath, - SymlinkTarget: reportedPath, - }, - PathError: err, - Owner: fileOwner, - ModifiedLines: contentLines.Expand(), - }) - } - if len(entries) > 0 { - return entries, nil - } - } - } - + p := parser.NewParser(isStrict) rules, err := p.Parse(content.Body) if err != nil { - slog.Error( + slog.Warn( "Failed to parse file content", slog.Any("err", err), slog.String("path", sourcePath), diff --git a/internal/discovery/discovery_test.go b/internal/discovery/discovery_test.go index c245a6a3..cedc17aa 100644 --- a/internal/discovery/discovery_test.go +++ b/internal/discovery/discovery_test.go @@ -24,7 +24,7 @@ func (r failingReader) Read(_ []byte) (int, error) { func TestReadRules(t *testing.T) { mustParse := func(offset int, s string) parser.Rule { - p := parser.NewParser() + p := parser.NewParser(false) 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)) diff --git a/internal/discovery/git_branch_test.go b/internal/discovery/git_branch_test.go index 7b0f848e..c1105437 100644 --- a/internal/discovery/git_branch_test.go +++ b/internal/discovery/git_branch_test.go @@ -2,6 +2,7 @@ package discovery_test import ( "encoding/json" + "errors" "fmt" "os" "regexp" @@ -9,7 +10,6 @@ import ( "testing" "github.com/google/go-cmp/cmp" - "github.com/prometheus/prometheus/model/rulefmt" "github.com/stretchr/testify/require" "github.com/cloudflare/pint/internal/discovery" @@ -38,7 +38,7 @@ func TestGitBranchFinder(t *testing.T) { includeAll := []*regexp.Regexp{regexp.MustCompile(".*")} mustParse := func(offset int, s string) parser.Rule { - p := parser.NewParser() + p := parser.NewParser(false) 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)) @@ -49,14 +49,6 @@ func TestGitBranchFinder(t *testing.T) { return r[0] } - mustErr := func(s string) error { - _, errs := rulefmt.Parse([]byte(s)) - if len(errs) == 0 { - panic(s) - } - return errs[0] - } - type setupFn func(t *testing.T) type testCaseT struct { @@ -726,15 +718,14 @@ groups: Name: "rules.yml", SymlinkTarget: "rules.yml", }, - ModifiedLines: []int{3}, - PathError: mustErr(` -groups: -- name: v2 - rules: - - record: up:count - expr: count(up) - expr: sum(up) -`), + ModifiedLines: []int{5, 6, 7}, + Rule: parser.Rule{ + Lines: parser.LineRange{First: 5, Last: 7}, + Error: parser.ParseError{ + Line: 7, + Err: errors.New("duplicated expr key"), + }, + }, }, }, }, diff --git a/internal/discovery/glob_test.go b/internal/discovery/glob_test.go index bba0ff3a..9802b43a 100644 --- a/internal/discovery/glob_test.go +++ b/internal/discovery/glob_test.go @@ -10,7 +10,6 @@ import ( "strings" "testing" - "github.com/prometheus/prometheus/model/rulefmt" "github.com/stretchr/testify/require" "github.com/cloudflare/pint/internal/discovery" @@ -27,19 +26,11 @@ func TestGlobPathFinder(t *testing.T) { entries []discovery.Entry } - p := parser.NewParser() + p := parser.NewParser(false) testRuleBody := "# pint file/owner bob\n\n- record: foo\n expr: sum(foo)\n" testRules, err := p.Parse([]byte(testRuleBody)) require.NoError(t, err) - parseErr := func(input string) error { - _, err := rulefmt.Parse([]byte(input)) - if err == nil { - panic(input) - } - return err[0] - } - testCases := []testCaseT{ { files: map[string]string{}, @@ -108,7 +99,10 @@ func TestGlobPathFinder(t *testing.T) { Name: "bar.yml", SymlinkTarget: "bar.yml", }, - PathError: parseErr(testRuleBody), + PathError: parser.ParseError{ + Err: errors.New("YAML list is not allowed here, expected a YAML mapping"), + Line: 3, + }, ModifiedLines: []int{1, 2, 3, 4}, Owner: "bob", }, @@ -124,7 +118,10 @@ func TestGlobPathFinder(t *testing.T) { Name: "bar.yml", SymlinkTarget: "bar.yml", }, - PathError: errors.New("yaml: line 2: mapping values are not allowed in this context"), + PathError: parser.ParseError{ + Err: errors.New("mapping values are not allowed in this context"), + Line: 2, + }, ModifiedLines: []int{1, 2, 3, 4}, Owner: "bob", }, @@ -141,7 +138,10 @@ func TestGlobPathFinder(t *testing.T) { Name: "bar.yml", SymlinkTarget: "bar.yml", }, - PathError: parseErr(testRuleBody), + PathError: parser.ParseError{ + Err: errors.New("YAML list is not allowed here, expected a YAML mapping"), + Line: 3, + }, ModifiedLines: []int{1, 2, 3, 4}, Owner: "bob", }, @@ -151,7 +151,10 @@ func TestGlobPathFinder(t *testing.T) { Name: "link.yml", SymlinkTarget: "bar.yml", }, - PathError: parseErr(testRuleBody), + PathError: parser.ParseError{ + Err: errors.New("YAML list is not allowed here, expected a YAML mapping"), + Line: 3, + }, ModifiedLines: []int{1, 2, 3, 4}, Owner: "bob", }, @@ -171,7 +174,10 @@ func TestGlobPathFinder(t *testing.T) { Name: "a/bar.yml", SymlinkTarget: "a/bar.yml", }, - PathError: parseErr(testRuleBody), + PathError: parser.ParseError{ + Err: errors.New("YAML list is not allowed here, expected a YAML mapping"), + Line: 3, + }, ModifiedLines: []int{1, 2, 3, 4}, Owner: "bob", }, @@ -181,7 +187,10 @@ func TestGlobPathFinder(t *testing.T) { Name: "b/c/link.yml", SymlinkTarget: "a/bar.yml", }, - PathError: parseErr(testRuleBody), + PathError: parser.ParseError{ + Err: errors.New("YAML list is not allowed here, expected a YAML mapping"), + Line: 3, + }, ModifiedLines: []int{1, 2, 3, 4}, Owner: "bob", }, @@ -191,7 +200,10 @@ func TestGlobPathFinder(t *testing.T) { Name: "b/link.yml", SymlinkTarget: "a/bar.yml", }, - PathError: parseErr(testRuleBody), + PathError: parser.ParseError{ + Err: errors.New("YAML list is not allowed here, expected a YAML mapping"), + Line: 3, + }, ModifiedLines: []int{1, 2, 3, 4}, Owner: "bob", }, @@ -226,7 +238,10 @@ func TestGlobPathFinder(t *testing.T) { Name: "a/bar.yml", SymlinkTarget: "a/bar.yml", }, - PathError: parseErr("xxx:\nyyy:\n"), + PathError: parser.ParseError{ + Err: errors.New("YAML list is not allowed here, expected a YAML mapping"), + Line: 1, + }, ModifiedLines: []int{1, 2}, Owner: "", }, @@ -236,7 +251,10 @@ func TestGlobPathFinder(t *testing.T) { Name: "b/c/link.yml", SymlinkTarget: "a/bar.yml", }, - PathError: parseErr("xxx:\nyyy:\n"), + PathError: parser.ParseError{ + Err: errors.New("YAML list is not allowed here, expected a YAML mapping"), + Line: 1, + }, ModifiedLines: []int{1, 2}, Owner: "", }, @@ -269,7 +287,10 @@ func TestGlobPathFinder(t *testing.T) { Name: "a/bar.yml", SymlinkTarget: "a/bar.yml", }, - PathError: parseErr(testRuleBody), + PathError: parser.ParseError{ + Err: errors.New("YAML list is not allowed here, expected a YAML mapping"), + Line: 3, + }, ModifiedLines: []int{1, 2, 3, 4}, Owner: "bob", }, @@ -279,7 +300,10 @@ func TestGlobPathFinder(t *testing.T) { Name: "b/c/link.yml", SymlinkTarget: "a/bar.yml", }, - PathError: parseErr(testRuleBody), + PathError: parser.ParseError{ + Err: errors.New("YAML list is not allowed here, expected a YAML mapping"), + Line: 3, + }, ModifiedLines: []int{1, 2, 3, 4}, Owner: "bob", }, diff --git a/internal/parser/fuzz_test.go b/internal/parser/fuzz_test.go index 971d0261..b5a7ec3c 100644 --- a/internal/parser/fuzz_test.go +++ b/internal/parser/fuzz_test.go @@ -276,7 +276,7 @@ labels: for _, tc := range testcases { f.Add(tc) } - p := parser.NewParser() + p := parser.NewParser(false) f.Fuzz(func(t *testing.T, s string) { t.Logf("Parsing: [%s]\n", s) _, _ = p.Parse([]byte(s)) diff --git a/internal/parser/models.go b/internal/parser/models.go index 97e1ef48..123b9e04 100644 --- a/internal/parser/models.go +++ b/internal/parser/models.go @@ -238,10 +238,15 @@ func (rr *RecordingRule) IsIdentical(b *RecordingRule) bool { return true } +// Use insread of StrictError. type ParseError struct { - Err error - Fragment string - Line int + Err error + Details string + Line int +} + +func (pe ParseError) Error() string { + return fmt.Sprintf("error at line %d: %s", pe.Line, pe.Err) } type LineRange struct { diff --git a/internal/parser/models_test.go b/internal/parser/models_test.go index 4f11defc..95bf5a0a 100644 --- a/internal/parser/models_test.go +++ b/internal/parser/models_test.go @@ -10,7 +10,7 @@ import ( ) func newMustRule(content string) parser.Rule { - p := parser.NewParser() + p := parser.NewParser(false) rules, err := p.Parse([]byte(content)) if err != nil { panic(err) diff --git a/internal/parser/parser.go b/internal/parser/parser.go index 0174fcc4..8e4466a9 100644 --- a/internal/parser/parser.go +++ b/internal/parser/parser.go @@ -5,6 +5,8 @@ import ( "errors" "fmt" "io" + "regexp" + "strconv" "strings" "gopkg.in/yaml.v3" @@ -26,11 +28,15 @@ const ( var ErrRuleCommentOnFile = errors.New("this comment is only valid when attached to a rule") -func NewParser() Parser { - return Parser{} +func NewParser(isStrict bool) Parser { + return Parser{ + isStrict: isStrict, + } } -type Parser struct{} +type Parser struct { + isStrict bool +} func (p Parser) Parse(content []byte) (rules []Rule, err error) { if len(content) == 0 { @@ -43,8 +49,8 @@ func (p Parser) Parse(content []byte) (rules []Rule, err error) { } }() - var documents []yaml.Node dec := yaml.NewDecoder(bytes.NewReader(content)) + var index int for { var doc yaml.Node decodeErr := dec.Decode(&doc) @@ -52,14 +58,31 @@ func (p Parser) Parse(content []byte) (rules []Rule, err error) { break } if decodeErr != nil { - return nil, decodeErr + return nil, tryDecodingYamlError(decodeErr) + } + index++ + if p.isStrict { + r, err := parseGroups(content, &doc) + if err.Err != nil { + return rules, err + } + rules = append(rules, r...) + } else { + rules = append(rules, parseNode(content, &doc, 0)...) + } + if index > 1 && p.isStrict { + rules = append(rules, Rule{ + Lines: LineRange{First: doc.Line, Last: doc.Line}, + Error: ParseError{ + Err: errors.New("multi-document YAML files are not allowed"), + Details: `This is a multi-document YAML file. Prometheus will only parse the first document and silently ignore the rest. +To allow for multi-document YAML files set parser->relaxed option in pint config file.`, + Line: doc.Line, + }, + }) } - documents = append(documents, doc) } - for _, doc := range documents { - rules = append(rules, parseNode(content, &doc, 0)...) - } return rules, err } @@ -121,8 +144,8 @@ func parseRule(content []byte, node *yaml.Node, offset int) (rule Rule, _ bool) var keepFiringForNode *yaml.Node var labelsNode *yaml.Node var annotationsNode *yaml.Node - labelsNodes := map[*yaml.Node]*yaml.Node{} - annotationsNodes := map[*yaml.Node]*yaml.Node{} + labelsNodes := []yamlMap{} + annotationsNodes := []yamlMap{} var key *yaml.Node unknownKeys := []*yaml.Node{} @@ -308,14 +331,25 @@ func parseRule(content []byte, node *yaml.Node, offset int) (rule Rule, _ bool) } } - for section, parts := range map[string]map[*yaml.Node]*yaml.Node{ + for section, parts := range map[string][]yamlMap{ labelsKey: labelsNodes, annotationsKey: annotationsNodes, } { - for key, value := range parts { - if !isTag(value.ShortTag(), "!!str") { - return invalidValueError(lines, value.Line+offset, fmt.Sprintf("%s %s", section, nodeValue(key)), "string", describeTag(value.ShortTag())) + names := map[string]struct{}{} + for _, entry := range parts { + if !isTag(entry.val.ShortTag(), "!!str") { + return invalidValueError(lines, entry.val.Line+offset, fmt.Sprintf("%s %s", section, nodeValue(entry.key)), "string", describeTag(entry.val.ShortTag())) } + if _, ok := names[entry.key.Value]; ok { + return Rule{ + Lines: rangeFromYamlMaps(parts), + Error: ParseError{ + Line: entry.key.Line, + Err: fmt.Errorf("duplicated %s key %s", section, entry.key.Value), + }, + }, false + } + names[entry.key.Value] = struct{}{} } } @@ -572,12 +606,17 @@ func describeTag(tag string) string { } } -func mappingNodes(node *yaml.Node) map[*yaml.Node]*yaml.Node { - m := make(map[*yaml.Node]*yaml.Node, len(node.Content)) +type yamlMap struct { + key *yaml.Node + val *yaml.Node +} + +func mappingNodes(node *yaml.Node) []yamlMap { + m := make([]yamlMap, 0, len(node.Content)/2) var key *yaml.Node for _, child := range node.Content { if key != nil { - m[key] = child + m = append(m, yamlMap{key: key, val: child}) key = nil } else { key = child @@ -585,3 +624,35 @@ func mappingNodes(node *yaml.Node) map[*yaml.Node]*yaml.Node { } return m } + +func rangeFromYamlMaps(m []yamlMap) (lr LineRange) { + for _, entry := range m { + if lr.First == 0 { + lr.First = entry.key.Line + lr.Last = entry.val.Line + } + lr.First = min(lr.First, entry.key.Line, entry.val.Line) + lr.Last = max(lr.Last, entry.key.Line, entry.val.Line) + } + return lr +} + +var ( + yamlErrRe = regexp.MustCompile("^yaml: line (.+): (.+)") + yamlUnmarshalErrRe = regexp.MustCompile("^yaml: unmarshal errors:\n line (.+): (.+)") +) + +func tryDecodingYamlError(err error) ParseError { + for _, re := range []*regexp.Regexp{yamlErrRe, yamlUnmarshalErrRe} { + parts := re.FindStringSubmatch(err.Error()) + if len(parts) > 2 { + if line, err2 := strconv.Atoi(parts[1]); line > 0 && err2 == nil { + return ParseError{ + Line: line, + Err: errors.New(parts[2]), + } + } + } + } + return ParseError{Line: 1, Err: err} +} diff --git a/internal/parser/parser_test.go b/internal/parser/parser_test.go index 38cf6326..3f1688bf 100644 --- a/internal/parser/parser_test.go +++ b/internal/parser/parser_test.go @@ -18,6 +18,7 @@ func TestParse(t *testing.T) { err string content []byte output []parser.Rule + strict bool } testCases := []testCaseT{ @@ -32,7 +33,7 @@ func TestParse(t *testing.T) { { content: []byte(string("! !00 \xf6")), output: nil, - err: "yaml: incomplete UTF-8 octet sequence", + err: "error at line 1: yaml: incomplete UTF-8 octet sequence", }, { content: []byte("- 0: 0\n 00000000: 000000\n 000000:00000000000: 00000000\n 00000000000:000000: 0000000000000000000000000000000000\n 000000: 0000000\n expr: |"), @@ -52,6 +53,30 @@ func TestParse(t *testing.T) { }, }, }, + { + content: []byte("---\n- expr: foo\n record: foo\n---\n- expr: bar\n"), + output: []parser.Rule{ + { + RecordingRule: &parser.RecordingRule{ + Record: parser.YamlNode{ + Lines: parser.LineRange{First: 3, Last: 3}, + Value: "foo", + }, + Expr: parser.PromQLExpr{ + Value: &parser.YamlNode{ + Lines: parser.LineRange{First: 2, Last: 2}, + Value: "foo", + }, + }, + }, + Lines: parser.LineRange{First: 2, Last: 3}, + }, + { + Lines: parser.LineRange{First: 5, Last: 5}, + Error: parser.ParseError{Err: errors.New("incomplete rule, no alert or record key"), Line: 5}, + }, + }, + }, { content: []byte("- expr: foo\n"), output: []parser.Rule{ @@ -90,15 +115,15 @@ func TestParse(t *testing.T) { }, { content: []byte("- record: - foo\n"), - err: "yaml: block sequence entries are not allowed in this context", + err: "error at line 1: yaml: block sequence entries are not allowed in this context", }, { content: []byte("- record: foo expr: sum(\n"), - err: "yaml: mapping values are not allowed in this context", + err: "error at line 1: yaml: mapping values are not allowed in this context", }, { content: []byte("- record\n\texpr: foo\n"), - err: "yaml: line 2: found a tab character that violates indentation", + err: "error at line 2: found a tab character that violates indentation", }, { content: []byte(` @@ -621,7 +646,7 @@ apiVersion: v1 metadata: name: example-app-alerts labels: - app: example-app + app: example-app data: alerts: | groups: @@ -1371,7 +1396,7 @@ data: foo: ` + string("\xed\xbf\xbf")), // Label values are invalid only if they aren't valid UTF-8 strings // which also makes them unparsable by YAML. - err: "yaml: invalid Unicode character", + err: "error at line 1: yaml: invalid Unicode character", }, { content: []byte(` @@ -1653,7 +1678,10 @@ data: output: []parser.Rule{ { Lines: parser.LineRange{First: 2, Last: 4}, - Error: parser.ParseError{Err: errors.New("labels value must be a YAML mapping, got binary data instead"), Line: 4}, + Error: parser.ParseError{ + Err: errors.New("labels value must be a YAML mapping, got binary data instead"), + Line: 4, + }, }, }, }, @@ -1666,7 +1694,10 @@ data: output: []parser.Rule{ { Lines: parser.LineRange{First: 2, Last: 4}, - Error: parser.ParseError{Err: errors.New("for value must be a YAML string, got float instead"), Line: 4}, + Error: parser.ParseError{ + Err: errors.New("for value must be a YAML string, got float instead"), + Line: 4, + }, }, }, }, @@ -1689,7 +1720,7 @@ data: expr: bar labels: !! "SGVsbG8sIFdvcmxkIQ==" `), - err: "yaml: line 4: did not find expected tag URI", + err: "error at line 4: did not find expected tag URI", }, { content: []byte(` @@ -1700,7 +1731,10 @@ data: output: []parser.Rule{ { Lines: parser.LineRange{First: 2, Last: 4}, - Error: parser.ParseError{Err: errors.New("labels value must be a YAML mapping, got string instead"), Line: 4}, + Error: parser.ParseError{ + Err: errors.New("labels value must be a YAML mapping, got string instead"), + Line: 4, + }, }, }, }, @@ -1730,7 +1764,10 @@ data: }, { Lines: parser.LineRange{First: 5, Last: 5}, - Error: parser.ParseError{Err: errors.New("incomplete rule, no alert or record key"), Line: 5}, + Error: parser.ParseError{ + Err: errors.New("incomplete rule, no alert or record key"), + Line: 5, + }, }, }, }, @@ -1806,6 +1843,697 @@ data: }, }, }, + { + content: []byte(`--- +groups: +- name: v1 + rules: + - record: up:count + expr: count(up) + labels: + foo: + bar: foo +`), + output: []parser.Rule{ + { + Lines: parser.LineRange{First: 5, Last: 9}, + Error: parser.ParseError{ + Err: errors.New("labels foo value must be a YAML string, got mapping instead"), + Line: 9, + }, + }, + }, + }, + { + content: []byte(` +groups: +- name: v1 + rules: + - record: up:count + expr: count(up) +`), + strict: true, + output: []parser.Rule{ + { + Lines: parser.LineRange{First: 5, Last: 6}, + RecordingRule: &parser.RecordingRule{ + Record: parser.YamlNode{ + Lines: parser.LineRange{First: 5, Last: 5}, + Value: "up:count", + }, + Expr: parser.PromQLExpr{ + Value: &parser.YamlNode{ + Lines: parser.LineRange{First: 6, Last: 6}, + Value: "count(up)", + }, + }, + }, + }, + }, + }, + { + content: []byte(` +groups: +- name: v1 + rules: + - record: up:count +`), + strict: true, + output: []parser.Rule{ + { + Lines: parser.LineRange{First: 5, Last: 5}, + Error: parser.ParseError{ + Err: errors.New("missing expr key"), + Line: 5, + }, + }, + }, + }, + { + content: []byte(` +- record: up:count + expr: count(up) +`), + strict: true, + err: "error at line 2: top level field must be a groups key, got YAML list", + }, + { + content: []byte(` +rules: + - record: up:count + expr: count(up) +`), + strict: true, + err: "error at line 2: unexpected key rules", + }, + { + content: []byte(` +groups: + - record: up:count + expr: count(up) +`), + strict: true, + err: "error at line 3: invalid group key record", + }, + { + content: []byte(` +groups: +- rules: + - record: up:count + expr: count(up) +`), + strict: true, + err: "error at line 3: incomplete group definition, name is required and must be set", + }, + { + content: []byte(` +groups: +- name: foo +`), + strict: true, + err: "error at line 3: incomplete group definition, rules is required and must be set", + }, + { + content: []byte(` +groups: {} +`), + strict: true, + err: "error at line 2: groups value must be a YAML list, got YAML mapping", + }, + { + content: []byte(` +groups: +- name: [] +`), + strict: true, + err: "error at line 3: group name must be a string, got list", + }, + { + content: []byte(` +groups: +- name: foo + name: bar + name: bob +`), + strict: true, + err: "error at line 4: duplicated key name", + }, + { + content: []byte(` +groups: +- name: v1 + rules: + rules: + - record: up:count + expr: count(up) +`), + strict: true, + err: "error at line 4: rules must be a YAML list, got YAML mapping", + }, + { + content: []byte(` +groups: +- name: v1 + rules: + - rules: + - record: up:count + expr: count(up) +`), + strict: true, + err: "error at line 5: invalid rule key rules", + }, + { + content: []byte(` +groups: +- name: v1 + rules: + - rules: + - record: up:count + expr: count(up) +`), + strict: true, + err: "error at line 6: found a tab character that violates indentation", + }, + { + content: []byte(` +--- +groups: +- name: v1 + rules: + - record: up:count + expr: count(up) +--- +groups: +- name: v1 + rules: + - rules: + - record: up:count + expr: count(up) +`), + strict: true, + output: []parser.Rule{ + { + Lines: parser.LineRange{First: 6, Last: 7}, + RecordingRule: &parser.RecordingRule{ + Record: parser.YamlNode{ + Lines: parser.LineRange{First: 6, Last: 6}, + Value: "up:count", + }, + Expr: parser.PromQLExpr{ + Value: &parser.YamlNode{ + Lines: parser.LineRange{First: 7, Last: 7}, + Value: "count(up)", + }, + }, + }, + }, + }, + err: "error at line 12: invalid rule key rules", + }, + { + content: []byte(` +--- +groups: [] +--- +groups: +- name: foo + rules: + - labels: !!binary "SGVsbG8sIFdvcmxkIQ==" +`), + strict: true, + output: []parser.Rule{ + { + Lines: parser.LineRange{First: 8, Last: 8}, + Error: parser.ParseError{ + Line: 8, + Err: errors.New("labels value must be a YAML mapping, got binary data instead"), + }, + }, + { + Lines: parser.LineRange{First: 4, Last: 4}, + Error: parser.ParseError{ + Line: 4, + Err: errors.New("multi-document YAML files are not allowed"), + }, + }, + }, + }, + { + content: []byte("[]"), + strict: true, + err: "error at line 1: top level field must be a groups key, got YAML list", + }, + { + content: []byte("\n\n[]"), + strict: true, + err: "error at line 3: top level field must be a groups key, got YAML list", + }, + { + content: []byte("groups: {}"), + strict: true, + err: "error at line 1: groups value must be a YAML list, got YAML mapping", + }, + { + content: []byte("groups: []"), + strict: true, + }, + { + content: []byte("xgroups: {}"), + strict: true, + err: "error at line 1: unexpected key xgroups", + }, + { + content: []byte("\nbob\n"), + strict: true, + err: "error at line 2: top level field must be a groups key, got YAML scalar value", + }, + { + content: []byte(`groups: [] + +rules: [] +`), + strict: true, + err: "error at line 3: unexpected key rules", + }, + { + content: []byte(` +groups: +- name: foo + rules: [] +`), + strict: true, + }, + { + content: []byte(` +groups: +- name: + rules: [] +`), + strict: true, + err: "error at line 3: group name must be a string, got null", + }, + { + content: []byte(` +groups: +- name: foo + rules: + - record: foo + expr: sum(up) + labels: + job: foo +`), + strict: true, + output: []parser.Rule{ + { + Lines: parser.LineRange{First: 5, Last: 8}, + RecordingRule: &parser.RecordingRule{ + Record: parser.YamlNode{ + Lines: parser.LineRange{First: 5, Last: 5}, + Value: "foo", + }, + Expr: parser.PromQLExpr{ + Value: &parser.YamlNode{ + Lines: parser.LineRange{First: 6, Last: 6}, + Value: "sum(up)", + }, + }, + Labels: &parser.YamlMap{ + Lines: parser.LineRange{First: 7, Last: 8}, + Key: &parser.YamlNode{ + Lines: parser.LineRange{First: 7, Last: 7}, + Value: "labels", + }, + Items: []*parser.YamlKeyValue{ + { + Key: &parser.YamlNode{ + Lines: parser.LineRange{First: 8, Last: 8}, + Value: "job", + }, + Value: &parser.YamlNode{ + Lines: parser.LineRange{First: 8, Last: 8}, + Value: "foo", + }, + }, + }, + }, + }, + }, + }, + }, + { + content: []byte(` +groups: +- name: foo + rules: + - record: foo + expr: sum(up) + xxx: 1 + labels: + job: foo +`), + strict: true, + err: "error at line 7: invalid rule key xxx", + }, + { + content: []byte(` +groups: +- name: foo + rules: + record: foo + expr: sum(up) + xxx: 1 + labels: + job: foo +`), + strict: true, + err: "error at line 4: rules must be a YAML list, got YAML mapping", + }, + { + content: []byte(` +groups: +- name: foo + rules: + - record: foo + expr: sum(up) + labels: + job: + foo: bar +`), + strict: true, + output: []parser.Rule{ + { + Lines: parser.LineRange{First: 5, Last: 9}, + Error: parser.ParseError{ + Line: 9, + Err: errors.New("labels job value must be a YAML string, got mapping instead"), + }, + }, + }, + }, + { + content: []byte(` +groups: +- name: foo + rules: + - record: foo + expr: + sum: sum(up) +`), + strict: true, + output: []parser.Rule{ + { + Lines: parser.LineRange{First: 5, Last: 7}, + Error: parser.ParseError{ + Line: 7, + Err: errors.New("expr value must be a YAML string, got mapping instead"), + }, + }, + }, + }, + { + content: []byte(` +groups: +- name: foo + rules: [] +- name: foo + rules: [] +`), + strict: true, + err: "error at line 5: duplicated group name", + }, + { + content: []byte(` +groups: +- name: foo + rules: + - record: foo + expr: sum(up) + labels: + foo: bob + foo: bar +`), + strict: true, + output: []parser.Rule{ + { + Lines: parser.LineRange{First: 8, Last: 9}, + Error: parser.ParseError{ + Line: 9, + Err: errors.New("duplicated labels key foo"), + }, + }, + }, + }, + { + content: []byte(` +groups: +- name: v2 + rules: + - record: up:count + expr: count(up) + expr: sum(up)`), + strict: true, + output: []parser.Rule{ + { + Lines: parser.LineRange{First: 5, Last: 7}, + Error: parser.ParseError{ + Line: 7, + Err: errors.New("duplicated expr key"), + }, + }, + }, + }, + { + content: []byte(` +groups: +- name: v2 + rules: + - record: up:count + expr: count(up) +bogus: 1 +`), + strict: true, + err: "error at line 7: unexpected key bogus", + }, + { + content: []byte(` +groups: +- name: v2 + rules: + - record: up:count + expr: count(up) + bogus: 1 +`), + strict: true, + err: "error at line 7: invalid rule key bogus", + }, + { + content: []byte(` +groups: + +- name: CloudflareKafkaZookeeperExporter + + rules: +`), + strict: true, + err: "error at line 6: rules must be a YAML list, got YAML scalar value", + }, + { + content: []byte(` +groups: +- name: foo + rules: + expr: 1 +`), + strict: true, + err: "error at line 4: rules must be a YAML list, got YAML mapping", + }, + { + content: []byte(` +groups: +- name: foo + rules: + - expr: 1 +`), + strict: true, + output: []parser.Rule{ + { + Lines: parser.LineRange{First: 5, Last: 5}, + Error: parser.ParseError{ + Line: 5, + Err: errors.New("incomplete rule, no alert or record key"), + }, + }, + }, + }, + { + content: []byte(` +groups: +- name: foo + rules: + - expr: null +`), + strict: true, + output: []parser.Rule{ + { + Lines: parser.LineRange{First: 5, Last: 5}, + Error: parser.ParseError{ + Line: 5, + Err: errors.New("incomplete rule, no alert or record key"), + }, + }, + }, + }, + { + content: []byte(` +groups: +- name: foo + rules: + - 1: null +`), + strict: true, + err: "error at line 5: invalid rule key 1", + }, + { + content: []byte(` +groups: +- name: foo + rules: + - true: !!binary "SGVsbG8sIFdvcmxkIQ==" +`), + strict: true, + err: "error at line 5: invalid rule key true", + }, + { + content: []byte(` +groups: +- name: foo + rules: + - expr: !!binary "SGVsbG8sIFdvcmxkIQ==" +`), + strict: true, + output: []parser.Rule{ + { + Lines: parser.LineRange{First: 5, Last: 5}, + Error: parser.ParseError{ + Line: 5, + Err: errors.New("incomplete rule, no alert or record key"), + }, + }, + }, + }, + { + content: []byte(` +groups: +- name: foo + rules: + - expr: !!binary "SGVsbG8sIFdvcmxkIQ==" + record: foo +`), + strict: true, + output: []parser.Rule{ + { + Lines: parser.LineRange{First: 5, Last: 6}, + Error: parser.ParseError{ + Line: 5, + Err: errors.New("expr value must be a YAML string, got binary data instead"), + }, + }, + }, + }, + { + content: []byte(` +groups: +- name: foo + rules: + - labels: !!binary "SGVsbG8sIFdvcmxkIQ==" +`), + strict: true, + output: []parser.Rule{ + { + Lines: parser.LineRange{First: 5, Last: 5}, + Error: parser.ParseError{ + Line: 5, + Err: errors.New("labels value must be a YAML mapping, got binary data instead"), + }, + }, + }, + }, + { + content: []byte(` +--- +groups: +- name: foo + rules: + - record: foo + expr: bar +--- +groups: +- name: foo + rules: + - record: foo + expr: bar +`), + strict: true, + output: []parser.Rule{ + { + Lines: parser.LineRange{First: 6, Last: 7}, + RecordingRule: &parser.RecordingRule{ + Record: parser.YamlNode{ + Lines: parser.LineRange{First: 6, Last: 6}, + Value: "foo", + }, + Expr: parser.PromQLExpr{ + Value: &parser.YamlNode{ + Lines: parser.LineRange{First: 7, Last: 7}, + Value: "bar", + }, + }, + }, + }, + { + Lines: parser.LineRange{First: 12, Last: 13}, + RecordingRule: &parser.RecordingRule{ + Record: parser.YamlNode{ + Lines: parser.LineRange{First: 12, Last: 12}, + Value: "foo", + }, + Expr: parser.PromQLExpr{ + Value: &parser.YamlNode{ + Lines: parser.LineRange{First: 13, Last: 13}, + Value: "bar", + }, + }, + }, + }, + { + Lines: parser.LineRange{First: 8, Last: 8}, + Error: parser.ParseError{ + Line: 8, + Err: errors.New("multi-document YAML files are not allowed"), + }, + }, + }, + }, + { + content: []byte(` +groups: +- name: foo + rules: + - record: foo + expr: foo + expr: foo +`), + strict: true, + output: []parser.Rule{ + { + Lines: parser.LineRange{First: 5, Last: 7}, + Error: parser.ParseError{ + Line: 7, + Err: errors.New("duplicated expr key"), + }, + }, + }, + }, } alwaysEqual := cmp.Comparer(func(_, _ interface{}) bool { return true }) @@ -1828,9 +2556,9 @@ data: for i, tc := range testCases { t.Run(strconv.Itoa(i+1), func(t *testing.T) { - t.Logf("--- Content ---%s--- END ---", tc.content) + t.Logf("\n--- Content ---%s--- END ---", tc.content) - p := parser.NewParser() + p := parser.NewParser(tc.strict) output, err := p.Parse(tc.content) if tc.err != "" { diff --git a/internal/parser/strict.go b/internal/parser/strict.go new file mode 100644 index 00000000..1cd34f48 --- /dev/null +++ b/internal/parser/strict.go @@ -0,0 +1,232 @@ +package parser + +import ( + "errors" + "fmt" + "strings" + + "github.com/prometheus/common/model" + "gopkg.in/yaml.v3" +) + +// https://github.com/go-yaml/yaml/blob/v3.0.1/resolve.go#L70-L81 +const ( + nullTag = "!!null" + boolTag = "!!bool" + strTag = "!!str" + intTag = "!!int" + floatTag = "!!float" + timestampTag = "!!timestamp" + seqTag = "!!seq" + mapTag = "!!map" + binaryTag = "!!binary" + mergeTag = "!!merge" +) + +func descirbeTag(tag string) string { + switch tag { + case "!!str": + return "string" + case "!!int": + return "integer" + case "!!seq": + return "list" + case "!!map": + return "mapping" + case "!!binary": + return "binary data" + default: + return strings.TrimLeft(tag, "!") + } +} + +func describeKind(kind yaml.Kind) string { + switch kind { + case yaml.DocumentNode: + return "YAML document" + case yaml.SequenceNode: + return "YAML list" + case yaml.MappingNode: + return "YAML mapping" + case yaml.ScalarNode: + return "YAML scalar value" + case yaml.AliasNode: + return "YAML alias" + } + return "unknown node" +} + +func parseGroups(content []byte, doc *yaml.Node) (rules []Rule, err ParseError) { + names := map[string]struct{}{} + + for _, node := range unpackNodes(doc) { + if node.Kind != yaml.MappingNode { + return nil, ParseError{ + Line: node.Line, + Err: fmt.Errorf("top level field must be a groups key, got %s", describeKind(node.Kind)), + } + } + + for _, entry := range mappingNodes(node) { + if entry.key.Kind != yaml.ScalarNode { + return nil, ParseError{ + Line: entry.key.Line, + Err: fmt.Errorf("top level field must be a groups key, got %s", describeKind(entry.key.Kind)), + } + } + if entry.key.ShortTag() != strTag { + return nil, ParseError{ + Line: entry.key.Line, + Err: fmt.Errorf("groups key must be a %s, got a %s", descirbeTag(strTag), descirbeTag(entry.key.ShortTag())), + } + } + if entry.key.Value != "groups" { + return nil, ParseError{ + Line: entry.key.Line, + Err: fmt.Errorf("unexpected key %s", entry.key.Value), + } + } + if entry.val.Kind != yaml.SequenceNode { + return nil, ParseError{ + Line: entry.key.Line, + Err: fmt.Errorf("groups value must be a %s, got %s", describeKind(yaml.SequenceNode), describeKind(node.Kind)), + } + } + for _, group := range unpackNodes(entry.val) { + name, r, err := parseGroup(content, group) + if err.Err != nil { + return rules, err + } + if _, ok := names[name]; ok { + return nil, ParseError{ + Line: group.Line, + Err: errors.New("duplicated group name"), + } + } + names[name] = struct{}{} + rules = append(rules, r...) + } + } + } + return rules, ParseError{} +} + +func parseGroup(content []byte, group *yaml.Node) (name string, rules []Rule, err ParseError) { + if group.Kind != yaml.MappingNode { + return "", nil, ParseError{ + Line: group.Line, + Err: fmt.Errorf("group must be a %s, got %s", describeKind(yaml.MappingNode), describeKind(group.Kind)), + } + } + + setKeys := make(map[string]struct{}, len(group.Content)) + + for _, entry := range mappingNodes(group) { + switch entry.key.Value { + case "name": + if entry.val.Kind != yaml.ScalarNode || entry.val.ShortTag() != strTag { + return "", nil, ParseError{ + Line: entry.key.Line, + Err: fmt.Errorf("group name must be a %s, got %s", descirbeTag(strTag), descirbeTag(entry.val.ShortTag())), + } + } + if entry.val.Value == "" { + return "", nil, ParseError{ + Line: entry.key.Line, + Err: errors.New("group name cannot be empty"), + } + } + name = entry.val.Value + case "interval", "query_offset": + if entry.val.Kind != yaml.ScalarNode || entry.val.ShortTag() != strTag { + return "", nil, ParseError{ + Line: entry.key.Line, + Err: fmt.Errorf("group %s must be a %s, got %s", entry.key.Value, descirbeTag(strTag), descirbeTag(entry.val.ShortTag())), + } + } + if _, err := model.ParseDuration(entry.val.Value); err != nil { + return "", nil, ParseError{ + Line: entry.key.Line, + Err: fmt.Errorf("invalid %s value: %w", entry.key.Value, err), + } + } + case "limit": + if entry.val.Kind != yaml.ScalarNode || entry.val.ShortTag() != intTag { + return "", nil, ParseError{ + Line: entry.key.Line, + Err: fmt.Errorf("group limit must be a %s, got %s", descirbeTag(intTag), descirbeTag(entry.val.ShortTag())), + } + } + case "rules": + if entry.val.Kind != yaml.SequenceNode { + return "", nil, ParseError{ + Line: entry.key.Line, + Err: fmt.Errorf("rules must be a %s, got %s", describeKind(yaml.SequenceNode), describeKind(entry.val.Kind)), + } + } + for _, rule := range unpackNodes(entry.val) { + r, err := parseRuleStrict(content, rule) + if err.Err != nil { + return "", nil, err + } + rules = append(rules, r) + } + default: + return "", nil, ParseError{ + Line: entry.key.Line, + Err: fmt.Errorf("invalid group key %s", entry.key.Value), + } + } + + if _, ok := setKeys[entry.key.Value]; ok { + return "", nil, ParseError{ + Line: entry.key.Line, + Err: fmt.Errorf("duplicated key %s", entry.key.Value), + } + } + setKeys[entry.key.Value] = struct{}{} + } + + for _, name := range []string{"name", "rules"} { + if _, ok := setKeys[name]; !ok { + return "", nil, ParseError{ + Line: group.Line, + Err: fmt.Errorf("incomplete group definition, %s is required and must be set", name), + } + } + } + + return name, rules, ParseError{} +} + +func parseRuleStrict(content []byte, rule *yaml.Node) (Rule, ParseError) { + if rule.Kind != yaml.MappingNode { + return Rule{}, ParseError{ + Line: rule.Line, + Err: fmt.Errorf("rule definion must be a %s, got %s", describeKind(yaml.MappingNode), describeKind(rule.Kind)), + } + } + + for i, node := range unpackNodes(rule) { + if i%2 != 0 { + continue + } + switch node.Value { + case "record": + case "alert": + case "expr": + case "for": + case "keep_firing_for": + case "labels": + case "annotations": + default: + return Rule{}, ParseError{ + Line: node.Line, + Err: fmt.Errorf("invalid rule key %s", node.Value), + } + } + } + + r, _ := parseRule(content, rule, 0) + return r, ParseError{} +} diff --git a/internal/reporter/bitbucket_test.go b/internal/reporter/bitbucket_test.go index 096e79d0..c7a9b579 100644 --- a/internal/reporter/bitbucket_test.go +++ b/internal/reporter/bitbucket_test.go @@ -41,7 +41,7 @@ func TestBitBucketReporter(t *testing.T) { pullRequestActivities reporter.BitBucketPullRequestActivities } - p := parser.NewParser() + p := parser.NewParser(false) mockRules, _ := p.Parse([]byte(` - record: target is down expr: up == 0 diff --git a/internal/reporter/comments_test.go b/internal/reporter/comments_test.go index 99f16e1d..48e34f9a 100644 --- a/internal/reporter/comments_test.go +++ b/internal/reporter/comments_test.go @@ -66,7 +66,7 @@ func (tc testCommenter) IsEqual(e ExistingCommentV2, p PendingCommentV2) bool { } func TestCommenter(t *testing.T) { - p := parser.NewParser() + p := parser.NewParser(false) mockRules, _ := p.Parse([]byte(` - record: target is down expr: up == 0 diff --git a/internal/reporter/github_test.go b/internal/reporter/github_test.go index 1d9d636e..eb7ce0e8 100644 --- a/internal/reporter/github_test.go +++ b/internal/reporter/github_test.go @@ -37,7 +37,7 @@ func TestGithubReporter(t *testing.T) { timeout time.Duration } - p := parser.NewParser() + p := parser.NewParser(false) mockRules, _ := p.Parse([]byte(` - record: target is down expr: up == 0 diff --git a/internal/reporter/gitlab_test.go b/internal/reporter/gitlab_test.go index 82053ebf..55ebffb6 100644 --- a/internal/reporter/gitlab_test.go +++ b/internal/reporter/gitlab_test.go @@ -49,7 +49,7 @@ func TestGitLabReporter(t *testing.T) { maxComments int } - p := parser.NewParser() + p := parser.NewParser(false) mockRules, _ := p.Parse([]byte(` - record: target is down expr: up == 0 diff --git a/internal/reporter/teamcity_test.go b/internal/reporter/teamcity_test.go index 33ad2360..2f4aa7ae 100644 --- a/internal/reporter/teamcity_test.go +++ b/internal/reporter/teamcity_test.go @@ -22,7 +22,7 @@ func TestTeamCityReporter(t *testing.T) { summary reporter.Summary } - p := parser.NewParser() + p := parser.NewParser(false) mockRules, _ := p.Parse([]byte(` - record: target is down expr: up == 0