From 3360d5b0e6a23fd80d4c7a46349992a41554ea09 Mon Sep 17 00:00:00 2001 From: Lukasz Mierzwa Date: Fri, 28 Jun 2024 13:46:09 +0100 Subject: [PATCH] Get rid of rulefmt.Parse --- .github/spellcheck/wordlist.txt | 1 + cmd/pint/scan.go | 60 ++-- cmd/pint/tests/0004_fail_invalid_yaml.txt | 4 +- cmd/pint/tests/0010_syntax_check.txt | 4 +- cmd/pint/tests/0067_relaxed.txt | 3 +- cmd/pint/tests/0071_ci_owner.txt | 1 + cmd/pint/tests/0074_strict_error.txt | 3 +- cmd/pint/tests/0076_ci_group_errors.txt | 1 + cmd/pint/tests/0078_repeated_group.txt | 3 +- .../tests/0086_rulefmt_ignored_errors.txt | 17 +- cmd/pint/tests/0123_ci_owner_allowed.txt | 1 + cmd/pint/tests/0141_empty_keys.txt | 20 +- cmd/pint/tests/0160_ci_comment_edit.txt | 1 + cmd/pint/tests/0161_ci_move_files.txt | 1 + cmd/pint/tests/0162_ci_deleted_dependency.txt | 1 + .../tests/0167_rule_duplicate_symlink.txt | 1 + cmd/pint/tests/0173_rule_duplicate_move.txt | 1 + docs/changelog.md | 4 + internal/checks/base_test.go | 2 +- internal/checks/template_test.go | 2 +- internal/config/config_test.go | 2 +- internal/discovery/discovery.go | 55 +-- internal/discovery/discovery_test.go | 2 +- internal/discovery/git_branch_test.go | 24 +- internal/discovery/glob_test.go | 66 ++-- internal/parser/fuzz_test.go | 2 +- internal/parser/models_test.go | 2 +- internal/parser/parser.go | 57 +++- internal/parser/parser_test.go | 234 ++++++++++++- internal/parser/strict.go | 272 +++++++++++++++ internal/parser/strict_test.go | 323 ++++++++++++++++++ internal/reporter/bitbucket_test.go | 2 +- internal/reporter/comments_test.go | 2 +- internal/reporter/github_test.go | 2 +- internal/reporter/gitlab_test.go | 2 +- internal/reporter/teamcity_test.go | 2 +- 36 files changed, 988 insertions(+), 192 deletions(-) create mode 100644 internal/parser/strict.go create mode 100644 internal/parser/strict_test.go 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..c56b3350 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,42 +19,12 @@ 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 -} - 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 { slog.Info("Offline mode, skipping Prometheus discovery") @@ -186,7 +153,27 @@ func scanWorker(ctx context.Context, jobs <-chan scanJob, results chan<- reporte default: var commentErr comments.CommentError var ignoreErr discovery.FileIgnoreError + var strictErr parser.StrictError switch { + case errors.As(job.entry.PathError, &strictErr): + results <- reporter.Report{ + Path: discovery.Path{ + Name: job.entry.Path.Name, + SymlinkTarget: job.entry.Path.SymlinkTarget, + }, + ModifiedLines: job.entry.ModifiedLines, + Problem: checks.Problem{ + Lines: parser.LineRange{ + First: strictErr.Line, + Last: strictErr.Line, + }, + Reporter: yamlParseReporter, + Text: strictErr.Err.Error(), + Details: strictErr.Details, + Severity: checks.Fatal, + }, + Owner: job.entry.Owner, + } case errors.As(job.entry.PathError, &ignoreErr): results <- reporter.Report{ Path: discovery.Path{ @@ -224,7 +211,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 +219,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). diff --git a/cmd/pint/tests/0004_fail_invalid_yaml.txt b/cmd/pint/tests/0004_fail_invalid_yaml.txt index e10d8088..28b40874 100644 --- a/cmd/pint/tests/0004_fail_invalid_yaml.txt +++ b/cmd/pint/tests/0004_fail_invalid_yaml.txt @@ -5,8 +5,8 @@ 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) +level=WARN msg="Failed to parse file content" err="YAML syntax error at line 4: did not find expected key" path=rules/bad.yaml lines=1-7 +rules/bad.yaml:4 Fatal: did not find expected key (yaml/parse) 4 | rules/ok.yml:5 Fatal: Prometheus failed to parse the query with this PromQL error: unclosed left bracket. (promql/syntax) diff --git a/cmd/pint/tests/0010_syntax_check.txt b/cmd/pint/tests/0010_syntax_check.txt index 65344f3c..b13cf69f 100644 --- a/cmd/pint/tests/0010_syntax_check.txt +++ b/cmd/pint/tests/0010_syntax_check.txt @@ -5,8 +5,8 @@ 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) +level=WARN msg="Failed to parse file content" err="YAML syntax error at line 6: did not find expected '-' indicator" path=rules/1.yaml lines=1-12 +rules/1.yaml:6 Fatal: did not find expected '-' indicator (yaml/parse) 6 | level=INFO msg="Problems found" Fatal=1 diff --git a/cmd/pint/tests/0067_relaxed.txt b/cmd/pint/tests/0067_relaxed.txt index 85d4613f..1a07f073 100644 --- a/cmd/pint/tests/0067_relaxed.txt +++ b/cmd/pint/tests/0067_relaxed.txt @@ -5,7 +5,8 @@ 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) +level=WARN msg="Failed to parse file content" err="YAML syntax error at line 2: YAML list is not allowed here, expected a YAML mapping" path=rules/strict.yml lines=1-4 +rules/strict.yml:2 Fatal: YAML list is not allowed here, expected a YAML mapping (yaml/parse) 2 | - alert: No Owner level=INFO msg="Problems found" Fatal=1 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..f6605a78 100644 --- a/cmd/pint/tests/0074_strict_error.txt +++ b/cmd/pint/tests/0074_strict_error.txt @@ -4,7 +4,8 @@ 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) +level=WARN msg="Failed to parse file content" err="YAML syntax error at line 2: unexpected key `alert`" path=rules/strict.yml lines=1-9 +rules/strict.yml:2 Fatal: unexpected key `alert` (yaml/parse) 2 | - alert: Conntrack_Table_Almost_Full level=INFO msg="Problems found" Fatal=1 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..c9c67b7c 100644 --- a/cmd/pint/tests/0078_repeated_group.txt +++ b/cmd/pint/tests/0078_repeated_group.txt @@ -4,7 +4,8 @@ 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) +level=WARN msg="Failed to parse file content" err="YAML syntax error at line 4: duplicated group name `name`" path=rules/strict.yml lines=1-5 +rules/strict.yml:4 Fatal: duplicated group name `name` (yaml/parse) 4 | - name: foo level=INFO msg="Problems found" Fatal=1 diff --git a/cmd/pint/tests/0086_rulefmt_ignored_errors.txt b/cmd/pint/tests/0086_rulefmt_ignored_errors.txt index 4a1d2efd..566677ac 100644 --- a/cmd/pint/tests/0086_rulefmt_ignored_errors.txt +++ b/cmd/pint/tests/0086_rulefmt_ignored_errors.txt @@ -4,22 +4,11 @@ cmp stderr stderr.txt -- stderr.txt -- level=INFO msg="Finding all rules to check" paths=["rules"] -rules/strict.yml:4 Fatal: This rule is not a valid Prometheus rule: `incomplete rule, no alert or record key`. (yaml/parse) - 4 | - expr: MissingAlertOrRecord - -rules/strict.yml:7 Fatal: This rule is not a valid Prometheus rule: `expr value cannot be empty`. (yaml/parse) +level=WARN msg="Failed to parse file content" err="YAML syntax error at line 7: expected a YAML string here, got null instead" path=rules/strict.yml lines=1-20 +rules/strict.yml:7 Fatal: expected a YAML string here, got null instead (yaml/parse) 7 | expr: -rules/strict.yml:10 Fatal: Prometheus failed to parse the query with this PromQL error: unknown function with name "sumz". (promql/syntax) - 10 | expr: sumz(0) - -rules/strict.yml:15 Fatal: Template failed to parse with this error: `function "bogus" not defined`. (alerts/template) - 15 | dashboard: '{{ bogus }}' - -rules/strict.yml:20 Fatal: Template failed to parse with this error: `function "bogus" not defined`. (alerts/template) - 20 | dashboard: '{{ bogus }}' - -level=INFO msg="Problems found" Fatal=5 +level=INFO msg="Problems found" Fatal=1 level=ERROR msg="Fatal error" err="found 1 problem(s) with severity Bug or higher" -- rules/strict.yml -- groups: 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/0141_empty_keys.txt b/cmd/pint/tests/0141_empty_keys.txt index fdeed374..3da69e11 100644 --- a/cmd/pint/tests/0141_empty_keys.txt +++ b/cmd/pint/tests/0141_empty_keys.txt @@ -4,25 +4,11 @@ cmp stderr stderr.txt -- stderr.txt -- level=INFO msg="Finding all rules to check" paths=["rules.yml"] -rules.yml:4 Fatal: This rule is not a valid Prometheus rule: `record value cannot be empty`. (yaml/parse) +level=WARN msg="Failed to parse file content" err="YAML syntax error at line 4: expected a YAML string here, got null instead" path=rules.yml lines=1-15 +rules.yml:4 Fatal: expected a YAML string here, got null instead (yaml/parse) 4 | - record: -rules.yml:6 Fatal: This rule is not a valid Prometheus rule: `record value cannot be empty`. (yaml/parse) - 6 | - record: - -rules.yml:9 Fatal: This rule is not a valid Prometheus rule: `expr value cannot be empty`. (yaml/parse) - 9 | expr: - -rules.yml:10 Fatal: This rule is not a valid Prometheus rule: `alert value cannot be empty`. (yaml/parse) - 10 | - alert: - -rules.yml:12 Fatal: This rule is not a valid Prometheus rule: `alert value cannot be empty`. (yaml/parse) - 12 | - alert: - -rules.yml:15 Fatal: This rule is not a valid Prometheus rule: `expr value cannot be empty`. (yaml/parse) - 15 | expr: - -level=INFO msg="Problems found" Fatal=6 +level=INFO msg="Problems found" Fatal=1 level=ERROR msg="Fatal error" err="found 1 problem(s) with severity Bug or higher" -- rules.yml -- groups: 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..1f15f5ee 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="YAML syntax error at line 1: YAML scalar value is not allowed here, expected a YAML mapping" 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..d408fd9d 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="YAML syntax error at line 1: YAML scalar value is not allowed here, expected a YAML mapping" 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..954de60f 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 { @@ -727,14 +719,10 @@ groups: SymlinkTarget: "rules.yml", }, ModifiedLines: []int{3}, - PathError: mustErr(` -groups: -- name: v2 - rules: - - record: up:count - expr: count(up) - expr: sum(up) -`), + PathError: parser.StrictError{ + Err: errors.New(`mapping key "expr" already defined at line 6`), + Line: 7, + }, }, }, }, diff --git a/internal/discovery/glob_test.go b/internal/discovery/glob_test.go index bba0ff3a..a5593b13 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.StrictError{ + 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.StrictError{ + 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.StrictError{ + 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.StrictError{ + 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.StrictError{ + 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.StrictError{ + 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.StrictError{ + 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.StrictError{ + 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.StrictError{ + 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.StrictError{ + 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.StrictError{ + 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_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..e316f760 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,25 @@ 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 { + if err = validateRuleFile(&doc); err != nil { + return nil, err + } } - documents = append(documents, doc) - } - - for _, doc := range documents { rules = append(rules, parseNode(content, &doc, 0)...) + if index > 1 && p.isStrict { + return nil, StrictError{ + 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, + } + } } + return rules, err } @@ -585,3 +602,25 @@ func mappingNodes(node *yaml.Node) map[*yaml.Node]*yaml.Node { } return m } + +var ( + yamlErrRe = regexp.MustCompile("^yaml: line (.+): (.+)") + yamlUnmarshalErrRe = regexp.MustCompile("^yaml: unmarshal errors:\n line (.+): (.+)") +) + +func tryDecodingYamlError(err error) error { + for _, re := range []*regexp.Regexp{yamlErrRe, yamlUnmarshalErrRe} { + parts := re.FindStringSubmatch(err.Error()) + if len(parts) > 2 { + line, err2 := strconv.Atoi(parts[1]) + if err2 != nil || line <= 0 { + return err + } + return StrictError{ + Line: line, + Err: errors.New(parts[2]), + } + } + } + return err +} diff --git a/internal/parser/parser_test.go b/internal/parser/parser_test.go index 38cf6326..f11b30b3 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{ @@ -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{ @@ -98,7 +123,7 @@ func TestParse(t *testing.T) { }, { content: []byte("- record\n\texpr: foo\n"), - err: "yaml: line 2: found a tab character that violates indentation", + err: "YAML syntax 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: @@ -1689,7 +1714,7 @@ data: expr: bar labels: !! "SGVsbG8sIFdvcmxkIQ==" `), - err: "yaml: line 4: did not find expected tag URI", + err: "YAML syntax error at line 4: did not find expected tag URI", }, { content: []byte(` @@ -1806,6 +1831,205 @@ 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: "YAML syntax error at line 2: YAML list is not allowed here, expected a YAML mapping", + }, + { + content: []byte(` +rules: + - record: up:count + expr: count(up) +`), + strict: true, + err: "YAML syntax error at line 2: unexpected key `rules`", + }, + { + content: []byte(` +groups: + - record: up:count + expr: count(up) +`), + strict: true, + err: "YAML syntax error at line 3: unexpected key `record`", + }, + { + content: []byte(` +groups: +- rules: + - record: up:count + expr: count(up) +`), + strict: true, + err: "YAML syntax error at line 3: `name` key is required and must be set", + }, + { + content: []byte(` +groups: +- name: foo +`), + strict: true, + err: "YAML syntax error at line 3: `rules` key is required and must be set", + }, + { + content: []byte(` +groups: {} +`), + strict: true, + err: `YAML syntax error at line 2: YAML mapping is not allowed here, expected a YAML list`, + }, + { + content: []byte(` +groups: +- name: [] +`), + strict: true, + err: `YAML syntax error at line 3: YAML list is not allowed here, expected a YAML scalar value`, + }, + { + content: []byte(` +groups: +- name: foo + name: bar +`), + strict: true, + err: "YAML syntax error at line 4: duplicated key `name`", + }, + { + content: []byte(` +groups: +- name: v1 + rules: + rules: + - record: up:count + expr: count(up) +`), + strict: true, + err: `YAML syntax error at line 5: YAML mapping is not allowed here, expected a YAML list`, + }, + { + content: []byte(` +groups: +- name: v1 + rules: + - rules: + - record: up:count + expr: count(up) +`), + strict: true, + err: "YAML syntax error at line 5: unexpected key `rules`", + }, + { + content: []byte(` +groups: +- name: v1 + rules: + - rules: + - record: up:count + expr: count(up) +`), + strict: true, + err: "YAML syntax 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, + err: "YAML syntax error at line 12: unexpected key `rules`", + }, + { + content: []byte(` +--- +groups: [] +--- +groups: +- name: foo + rules: + - labels: !!binary "SGVsbG8sIFdvcmxkIQ==" +`), + strict: true, + err: "YAML syntax error at line 8: YAML scalar value is not allowed here, expected a YAML mapping", + }, } alwaysEqual := cmp.Comparer(func(_, _ interface{}) bool { return true }) @@ -1828,9 +2052,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..e7235970 --- /dev/null +++ b/internal/parser/strict.go @@ -0,0 +1,272 @@ +package parser + +import ( + "fmt" + "strings" + + "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" +) + +type StrictError struct { + Err error + Details string + Line int +} + +func (se StrictError) Error() string { + return fmt.Sprintf("YAML syntax error at line %d: %s", se.Line, se.Err.Error()) +} + +type validator interface { + validate(*yaml.Node) error +} + +func mustKind(node *yaml.Node, kind yaml.Kind) error { + if node == nil { + return fmt.Errorf("%s is required here but got nil", describeKind(kind)) + } + if node.Kind != kind { + return fmt.Errorf("%s is not allowed here, expected a %s", describeKind(node.Kind), describeKind(kind)) + } + return nil +} + +func mustKindTag(node *yaml.Node, kind yaml.Kind, tag string) (err error) { + if err = mustKind(node, kind); err != nil { + return err + } + if tag != "" && node.ShortTag() != tag { + return fmt.Errorf("expected a YAML %s here, got %s instead", descirbeTag(tag), descirbeTag(node.ShortTag())) + } + return nil +} + +type requireDocument struct { + v validator +} + +func (rd requireDocument) validate(node *yaml.Node) (err error) { + if err = mustKind(node, yaml.DocumentNode); err != nil { + return StrictError{Err: err, Line: node.Line} + } + for _, child := range node.Content { + if err = rd.v.validate(child); err != nil { + return err + } + } + return nil +} + +type requireScalar struct { + tag string +} + +func (rs requireScalar) validate(node *yaml.Node) (err error) { + if err = mustKindTag(node, yaml.ScalarNode, rs.tag); err != nil { + return StrictError{Err: err, Line: node.Line} + } + return nil +} + +type requireList struct { + v validator +} + +func (rl requireList) validate(node *yaml.Node) (err error) { + if err = mustKind(node, yaml.SequenceNode); err != nil { + return StrictError{Err: err, Line: node.Line} + } + for _, n := range unpackNodes(node) { + if err = rl.v.validate(n); err != nil { + return err + } + } + return nil +} + +type requireMap struct { + v validator +} + +func (rm requireMap) validate(node *yaml.Node) (err error) { + if err = mustKind(node, yaml.MappingNode); err != nil { + return StrictError{Err: err, Line: node.Line} + } + + setKeys := map[string]struct{}{} + + var ok bool + for _, n := range unpackNodes(node) { + if err = rm.v.validate(n); err != nil { + return err + } + if _, ok = setKeys[n.Value]; ok { + return StrictError{ + Err: fmt.Errorf("duplicated key `%s`", n.Value), + Line: n.Line, + } + } + setKeys[n.Value] = struct{}{} + } + + return nil +} + +type requireExactMap struct { + nameCallback map[string]func(string, int) + keys map[string]validator + required []string +} + +func (rm requireExactMap) validate(node *yaml.Node) (err error) { + if err = mustKind(node, yaml.MappingNode); err != nil { + return StrictError{Err: err, Line: node.Line} + } + + setKeys := make(map[string]struct{}, len(rm.required)) + + var v validator + var ok bool + for i, n := range unpackNodes(node) { + if i%2 == 0 { + if err = mustKindTag(n, yaml.ScalarNode, strTag); err != nil { + return StrictError{Err: err, Line: n.Line} + } + v, ok = rm.keys[n.Value] + if !ok { + return StrictError{Err: fmt.Errorf("unexpected key `%s`", n.Value), Line: n.Line} + } + if _, ok = setKeys[n.Value]; ok { + return StrictError{ + Err: fmt.Errorf("duplicated key `%s`", n.Value), + Line: n.Line, + } + } + setKeys[n.Value] = struct{}{} + + for name, fn := range rm.nameCallback { + if name == n.Value { + fn(n.Value, n.Line) + } + } + } else { + if err = v.validate(n); err != nil { + return err + } + } + } + + for _, key := range rm.required { + if _, ok = setKeys[key]; !ok { + return StrictError{ + Err: fmt.Errorf("`%s` key is required and must be set", key), + Line: node.Line, + } + } + } + + return nil +} + +func validateRuleFile(node *yaml.Node) (err error) { + groupNames := map[string][]int{} + nameCallback := func(s string, l int) { + groupNames[s] = append(groupNames[s], l) + } + + v := requireDocument{ + v: requireExactMap{ + keys: map[string]validator{ + "groups": requireList{ + v: requireExactMap{ + keys: map[string]validator{ + "name": requireScalar{tag: strTag}, + "interval": requireScalar{tag: strTag}, + "limit": requireScalar{tag: intTag}, + "query_offset": requireScalar{tag: strTag}, + "rules": requireList{ + v: requireExactMap{ + keys: map[string]validator{ + "record": requireScalar{tag: strTag}, + "alert": requireScalar{tag: strTag}, + "expr": requireScalar{tag: strTag}, + "for": requireScalar{tag: strTag}, + "keep_firing_for": requireScalar{tag: strTag}, + "labels": requireMap{v: requireScalar{tag: strTag}}, + "annotations": requireMap{v: requireScalar{tag: strTag}}, + }, + }, + }, + }, + required: []string{"name", "rules"}, + nameCallback: map[string]func(string, int){ + "name": nameCallback, + }, + }, + }, + }, + }, + } + + if err = v.validate(node); err != nil { + return err + } + + for name, lines := range groupNames { + if len(lines) > 1 { + return StrictError{ + Err: fmt.Errorf("duplicated group name `%s`", name), + Line: lines[len(lines)-1], + } + } + } + + return nil +} + +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" +} diff --git a/internal/parser/strict_test.go b/internal/parser/strict_test.go new file mode 100644 index 00000000..31f93b8c --- /dev/null +++ b/internal/parser/strict_test.go @@ -0,0 +1,323 @@ +package parser + +import ( + "io" + "strconv" + "strings" + "testing" + + "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" +) + +func TestValidateRuleFile(t *testing.T) { + type testCaseT struct { + content string + err string + line int + } + + testCases := []testCaseT{ + { + content: "[]", + err: "YAML list is not allowed here, expected a YAML mapping", + line: 1, + }, + { + content: "\n\n[]", + err: "YAML list is not allowed here, expected a YAML mapping", + line: 3, + }, + { + content: "groups: {}", + err: "YAML mapping is not allowed here, expected a YAML list", + line: 1, + }, + { + content: "groups: []", + }, + { + content: "xgroups: {}", + err: "unexpected key `xgroups`", + line: 1, + }, + { + content: "\nbob\n", + err: "YAML scalar value is not allowed here, expected a YAML mapping", + line: 2, + }, + { + content: `groups: [] + +rules: [] +`, + err: "unexpected key `rules`", + line: 3, + }, + { + content: ` +groups: +- name: foo + rules: [] +`, + }, + { + content: ` +groups: +- name: + rules: [] +`, + err: "expected a YAML string here, got null instead", + line: 3, + }, + { + content: ` +groups: +- name: foo + rules: + - record: foo + expr: sum(up) + labels: + job: foo +`, + }, + { + content: ` +groups: +- name: foo + rules: + - record: foo + expr: sum(up) + xxx: 1 + labels: + job: foo +`, + err: "unexpected key `xxx`", + line: 7, + }, + { + content: ` +groups: +- name: foo + rules: + record: foo + expr: sum(up) + xxx: 1 + labels: + job: foo +`, + err: "YAML mapping is not allowed here, expected a YAML list", + line: 5, + }, + { + content: ` +groups: +- name: foo + rules: + - record: foo + expr: sum(up) + labels: + job: + foo: bar +`, + err: "YAML mapping is not allowed here, expected a YAML scalar value", + line: 9, + }, + { + content: ` +groups: +- name: foo + rules: + - record: foo + expr: + sum: sum(up) +`, + err: "YAML mapping is not allowed here, expected a YAML scalar value", + line: 7, + }, + { + content: ` +groups: +- name: foo + rules: [] +- name: foo + rules: [] +`, + err: "duplicated group name `name`", + line: 5, + }, + { + content: ` +groups: +- name: foo + rules: + - record: foo + expr: sum(up) + labels: + foo: bob + foo: bar +`, + err: "duplicated key `foo`", + line: 9, + }, + { + content: ` +groups: +- name: v2 + rules: + - record: up:count + expr: count(up) + expr: sum(up)`, + err: "duplicated key `expr`", + line: 7, + }, + { + content: ` +groups: +- name: v2 + rules: + - record: up:count + expr: count(up) +bogus: 1 +`, + err: "unexpected key `bogus`", + line: 7, + }, + { + content: ` +groups: +- name: v2 + rules: + - record: up:count + expr: count(up) + bogus: 1 +`, + err: "unexpected key `bogus`", + line: 7, + }, + { + content: ` +groups: + +- name: CloudflareKafkaZookeeperExporter + + rules: +`, + err: "YAML scalar value is not allowed here, expected a YAML list", + line: 6, + }, + { + content: ` +groups: +- name: foo + rules: + expr: 1 +`, + err: "YAML mapping is not allowed here, expected a YAML list", + line: 5, + }, + { + content: ` +groups: +- name: foo + rules: + - expr: 1 +`, + err: "expected a YAML string here, got integer instead", + line: 5, + }, + { + content: ` +groups: +- name: foo + rules: + - expr: null +`, + err: "expected a YAML string here, got null instead", + line: 5, + }, + { + content: ` +groups: +- name: foo + rules: + - 1: null +`, + err: "expected a YAML string here, got integer instead", + line: 5, + }, + { + content: ` +groups: +- name: foo + rules: + - true: !!binary "SGVsbG8sIFdvcmxkIQ==" +`, + err: "expected a YAML string here, got bool instead", + line: 5, + }, + { + content: ` +groups: +- name: foo + rules: + - expr: !!binary "SGVsbG8sIFdvcmxkIQ==" +`, + err: "expected a YAML string here, got binary data instead", + line: 5, + }, + { + content: ` +groups: +- name: foo + rules: + - labels: !!binary "SGVsbG8sIFdvcmxkIQ==" +`, + err: "YAML scalar value is not allowed here, expected a YAML mapping", + line: 5, + }, + { + content: ` +--- +groups: +- name: foo + rules: + - record: foo + expr: bar +--- +groups: +- name: foo + rules: + - record: foo + expr: bar +`, + }, + } + + for i, tc := range testCases { + t.Run(strconv.Itoa(i+1), func(t *testing.T) { + t.Logf("\n--- Content ---%s--- END ---", tc.content) + + dec := yaml.NewDecoder(strings.NewReader(tc.content)) + for { + var doc yaml.Node + decodeErr := dec.Decode(&doc) + if decodeErr != nil { + require.ErrorIs(t, decodeErr, io.EOF) + break + } + + err := validateRuleFile(&doc) + if tc.err == "" { + require.NoError(t, err) + } else { + require.Error(t, err) + var se StrictError + require.ErrorAs(t, err, &se) + require.EqualError(t, se.Err, tc.err) + require.Equal(t, tc.line, se.Line) + } + } + }) + } +} 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