Merge pull request #551 from cloudflare/owner-validate

Validate rule owners if configured

cmd/pint/ci.go

@@ -104,7 +104,7 @@ func actionCI(c *cli.Context) error {
 	summary := checkRules(ctx, meta.workers, meta.cfg, entries)
 
 	if c.Bool(requireOwnerFlag) {
-		summary.Report(verifyOwners(entries)...)
+		summary.Report(verifyOwners(entries, meta.cfg.Owners.CompileAllowed())...)
 	}
 
 	reps := []reporter.Reporter{

cmd/pint/lint.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"os"
+	"regexp"
 
 	"github.com/cloudflare/pint/internal/checks"
 	"github.com/cloudflare/pint/internal/config"
@@ -62,7 +63,7 @@ func actionLint(c *cli.Context) error {
 	summary := checkRules(ctx, meta.workers, meta.cfg, entries)
 
 	if c.Bool(requireOwnerFlag) {
-		summary.Report(verifyOwners(entries)...)
+		summary.Report(verifyOwners(entries, meta.cfg.Owners.CompileAllowed())...)
 	}
 
 	minSeverity, err := checks.ParseSeverity(c.String(minSeverityFlag))
@@ -101,16 +102,34 @@ func actionLint(c *cli.Context) error {
 	return nil
 }
 
-func verifyOwners(entries []discovery.Entry) (reports []reporter.Report) {
+func verifyOwners(entries []discovery.Entry, allowedOwners []*regexp.Regexp) (reports []reporter.Report) {
 	for _, entry := range entries {
 		if entry.State == discovery.Removed {
 			continue
 		}
 		if entry.PathError != nil {
 			continue
 		}
-		if entry.Owner != "" {
-			continue
+		if entry.Owner == "" {
+			reports = append(reports, reporter.Report{
+				ReportedPath:  entry.ReportedPath,
+				SourcePath:    entry.SourcePath,
+				ModifiedLines: entry.ModifiedLines,
+				Rule:          entry.Rule,
+				Problem: checks.Problem{
+					Lines:    entry.Rule.Lines(),
+					Reporter: discovery.RuleOwnerComment,
+					Text: fmt.Sprintf(`%s comments are required in all files, please add a "# pint %s $owner" somewhere in this file and/or "# pint %s $owner" on top of each rule`,
+						discovery.RuleOwnerComment, discovery.FileOwnerComment, discovery.RuleOwnerComment),
+					Severity: checks.Bug,
+				},
+			})
+			goto NEXT
+		}
+		for _, re := range allowedOwners {
+			if re.MatchString(entry.Owner) {
+				goto NEXT
+			}
 		}
 		reports = append(reports, reporter.Report{
 			ReportedPath:  entry.ReportedPath,
@@ -120,11 +139,11 @@ func verifyOwners(entries []discovery.Entry) (reports []reporter.Report) {
 			Problem: checks.Problem{
 				Lines:    entry.Rule.Lines(),
 				Reporter: discovery.RuleOwnerComment,
-				Text: fmt.Sprintf(`%s comments are required in all files, please add a "# pint %s $owner" somewhere in this file and/or "# pint %s $owner" on top of each rule`,
-					discovery.RuleOwnerComment, discovery.FileOwnerComment, discovery.RuleOwnerComment),
+				Text:     fmt.Sprintf("this rule is set as owned by %q but %q doesn't match any of the allowed owner values", entry.Owner, entry.Owner),
 				Severity: checks.Bug,
 			},
 		})
+	NEXT:
 	}
 	return reports
 }

cmd/pint/tests/0025_config.txt

@@ -174,7 +174,8 @@ level=info msg="Loading configuration file" path=.pint.hcl
       ],
       "cost": {}
     }
-  ]
+  ],
+  "owners": {}
 }
 -- .pint.hcl --
 prometheus "prom" {

cmd/pint/tests/0113_config_env_expand.txt

@@ -72,7 +72,8 @@ level=info msg="Loading configuration file" path=.pint.hcl
         }
       ]
     }
-  ]
+  ],
+  "owners": {}
 }
 -- .pint.hcl --
 parser {

cmd/pint/tests/0122_lint_owner_allowed.txt

@@ -0,0 +1,33 @@
+pint.error --no-color lint --require-owner rules
+! stdout .
+cmp stderr stderr.txt
+
+-- stderr.txt --
+level=info msg="Loading configuration file" path=.pint.hcl
+rules/1.yml:4-5 Bug: rule/owner comments are required in all files, please add a "# pint file/owner $owner" somewhere in this file and/or "# pint rule/owner $owner" on top of each rule (rule/owner)
+ 4 |   - alert: No Owner
+ 5 |     expr: up > 0
+
+rules/1.yml:7-8 Bug: this rule is set as owned by "bob" but "bob" doesn't match any of the allowed owner values (rule/owner)
+ 7 |   - alert: Invalid
+ 8 |     expr: up == 0
+
+level=info msg="Problems found" Bug=2
+level=fatal msg="Fatal error" error="problems found"
+-- rules/1.yml --
+groups:
+- name: foo
+  rules:
+  - alert: No Owner
+    expr: up > 0
+  # pint rule/owner bob
+  - alert: Invalid
+    expr: up == 0
+  # pint rule/owner alice
+  - alert: Owner Alice
+    expr: up > 0
+
+-- .pint.hcl --
+owners {
+  allowed = ["alice", "max", "not-bob"]
+}

cmd/pint/tests/0123_ci_owner_allowed.txt

@@ -0,0 +1,75 @@
+http response bitbucket / 200 OK
+http start bitbucket 127.0.0.1:6123
+
+mkdir testrepo
+cd testrepo
+exec git init --initial-branch=main .
+
+cp ../src/v1.yml rules.yml
+cp ../src/.pint.hcl .
+env GIT_AUTHOR_NAME=pint
+env [email protected]
+env GIT_COMMITTER_NAME=pint
+env [email protected]
+exec git add .
+exec git commit -am 'import rules and config'
+
+exec git checkout -b v2
+cp ../src/v2.yml rules.yml
+exec git commit -am 'v2'
+
+env BITBUCKET_AUTH_TOKEN="12345"
+pint.error -l error --no-color ci --require-owner
+! stdout .
+cd ..
+cmp stderr stderr.txt
+
+-- stderr.txt --
+rules.yml:4-5 Bug: rule/owner comments are required in all files, please add a "# pint file/owner $owner" somewhere in this file and/or "# pint rule/owner $owner" on top of each rule (rule/owner)
+ 4 |   - alert: No Owner
+ 5 |     expr: up > 0
+
+rules.yml:7-8 Bug: this rule is set as owned by "bob" but "bob" doesn't match any of the allowed owner values (rule/owner)
+ 7 |   - alert: Invalid
+ 8 |     expr: up == 0
+
+level=fatal msg="Fatal error" error="problems found"
+-- src/v1.yml --
+groups:
+- name: foo
+  rules:
+  - alert: Foo
+    expr: up > 0
+  - alert: Foo
+    expr: up == 0
+  - alert: Foo
+    expr: up > 0
+
+-- src/v2.yml --
+groups:
+- name: foo
+  rules:
+  - alert: No Owner
+    expr: up > 0
+  # pint rule/owner bob
+  - alert: Invalid
+    expr: up == 0
+  # pint rule/owner alice
+  - alert: Owner Alice
+    expr: up > 0
+
+-- src/.pint.hcl --
+owners {
+  allowed = ["alice", "max", "not-bob"]
+}
+ci {
+  baseBranch = "main"
+}
+repository {
+  bitbucket {
+    uri        = "http://127.0.0.1:6123"
+    timeout    = "10s"
+    project    = "prometheus"
+    repository = "rules"
+  }
+}

docs/changelog.md

@@ -17,6 +17,8 @@
     expr: rate(my:sum[5m])
   ```
 - Added [rule/for](checks/rule/for.md) check.
+- Added `owners` configuration block for setting the list of allowed rule owner values.
+  See [configuration](configuration.md) for details.
 
 ### Changed
 

docs/configuration.md

@@ -85,6 +85,28 @@ parser {
   This option takes a list of file patterns, all files matching those regexp rules
   will be parsed in relaxed mode.
 
+## Owners
+
+When `pint ci` or `pint lint` is run with `--require-owner` flag it will require
+all Prometheus rules to have an owner assigned via comment.
+See [rule/owner](checks/rule/owner.md) for details.
+
+Those checks can be further customised by setting a list of allowed owner names.
+
+Syntax:
+
+```js
+owners {
+  allowed = [ "(.*)", ... ]
+}
+```
+
+- `allowed` - list of allowed owner names, this option accepts regexp rules.
+  When set all owners set via comments must much at least one entry on this list.
+
+If there's no `owners:allowed` configuration block, or if it's empty, then any
+owner name is accepted.
+
 ## CI
 
 Configure continuous integration environments.