From fd6adae92f5e202974562a87899678dd6ec52c30 Mon Sep 17 00:00:00 2001 From: Anders Eknert Date: Wed, 13 Nov 2024 23:05:08 +0100 Subject: [PATCH] Fix custom rules that report on the absence of aggregates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While custom aggregate rules would largely work before, those that report on the absence of data (like a rule named "allow") did not work. This as the aggregator sent nothing back in those caes — and contrary to the built-in rules, Regal had then no way of knowing in the next step that the aggregate report rules should be called even with no data. I'm not super happy about the way we communicate this back to the Go side, but there's a lot I'm not happy about with regards to how aggregate data is shuffled back and forth. Some effort to fix that will however have to wait until later. Fixes #1259 Signed-off-by: Anders Eknert --- bundle/regal/main/main.rego | 20 ++++++++++++++-- e2e/cli_test.go | 24 +++++++++++++++++++ .../empty_aggregate/empty_aggregate.rego | 21 ++++++++++++++++ pkg/linter/linter.go | 15 +++++++++++- 4 files changed, 77 insertions(+), 3 deletions(-) create mode 100644 e2e/testdata/aggregates/custom/regal/rules/testcase/empty_aggregate/empty_aggregate.rego diff --git a/bundle/regal/main/main.rego b/bundle/regal/main/main.rego index 961de0c4..2337e1cc 100644 --- a/bundle/regal/main/main.rego +++ b/bundle/regal/main/main.rego @@ -145,11 +145,23 @@ aggregate[category_title] contains entry if { config.for_rule(category, title).level != "ignore" not config.excluded_file(category, title, input.regal.file.name) - some entry in data.custom.regal.rules[category][title].aggregate + entries := _mark_if_empty(data.custom.regal.rules[category][title].aggregate) category_title := concat("/", [category, title]) + + some entry in entries } +# a custom aggregate rule may not come back with entries, but we still need +# to register the fact that it was called so that we know to call the +# aggregate_report for the same rule later +# +# for these cases we just return an empty map, and let the aggregator on the Go +# side handle this case +_mark_if_empty(entries) := {{}} if { + count(entries) == 0 +} else := entries + # METADATA # description: Check bundled rules using aggregated data # schemas: @@ -187,7 +199,7 @@ aggregate_report contains violation if { not config.excluded_file(category, title, input.regal.file.name) input_for_rule := object.remove( - object.union(input, {"aggregate": input.aggregates_internal[key]}), + object.union(input, {"aggregate": _null_to_empty(input.aggregates_internal[key])}), ["aggregates_internal"], ) @@ -211,3 +223,7 @@ _ignored(violation, directives) if { ignored_rules := directives[util.to_location_object(violation.location).row + 1] violation.title in ignored_rules } + +_null_to_empty(x) := [] if { + x == null +} else := x diff --git a/e2e/cli_test.go b/e2e/cli_test.go index 027ffba1..112c2fb1 100644 --- a/e2e/cli_test.go +++ b/e2e/cli_test.go @@ -429,6 +429,30 @@ func TestAggregatesAreCollectedAndUsed(t *testing.T) { t.Errorf("expected 1 violation, got %d", rep.Summary.NumViolations) } }) + + t.Run("custom policy where nothing aggregate is a violation", func(t *testing.T) { + stdout, stderr := bytes.Buffer{}, bytes.Buffer{} + + err := regal(&stdout, &stderr)("lint", "--format", "json", "--rules", + basedir+filepath.FromSlash("/custom/regal/rules/testcase/empty_aggregate/"), + basedir+filepath.FromSlash("/two_policies")) + + expectExitCode(t, err, 3, &stdout, &stderr) + + if exp, act := "", stderr.String(); exp != act { + t.Errorf("expected stderr %q, got %q", exp, act) + } + + var rep report.Report + + if err = json.Unmarshal(stdout.Bytes(), &rep); err != nil { + t.Fatalf("expected JSON response, got %v", stdout.String()) + } + + if rep.Summary.NumViolations != 1 { + t.Errorf("expected 1 violation, got %d", rep.Summary.NumViolations) + } + }) } func TestLintAggregateIgnoreDirective(t *testing.T) { diff --git a/e2e/testdata/aggregates/custom/regal/rules/testcase/empty_aggregate/empty_aggregate.rego b/e2e/testdata/aggregates/custom/regal/rules/testcase/empty_aggregate/empty_aggregate.rego new file mode 100644 index 00000000..3c280318 --- /dev/null +++ b/e2e/testdata/aggregates/custom/regal/rules/testcase/empty_aggregate/empty_aggregate.rego @@ -0,0 +1,21 @@ +# METADATA +# description: | +# Test to ensure a custom rule that aggregated no data is still reported +# related_resources: +# - description: issue +# ref: https://github.com/StyraInc/regal/issues/1259 +package custom.regal.rules.testcase.empty_aggregates + +import rego.v1 + +import data.regal.result + +aggregate contains result.aggregate(rego.metadata.chain(), {}) if { + input.nope +} + +aggregate_report contains violation if { + count(input.aggregate) == 0 + + violation := result.fail(rego.metadata.chain(), {}) +} diff --git a/pkg/linter/linter.go b/pkg/linter/linter.go index 9491c562..133ebd0c 100644 --- a/pkg/linter/linter.go +++ b/pkg/linter/linter.go @@ -914,7 +914,20 @@ func (l Linter) lintWithRegoRules( regoReport.Notices = append(regoReport.Notices, result.Notices...) for k := range result.Aggregates { - regoReport.Aggregates[k] = append(regoReport.Aggregates[k], result.Aggregates[k]...) + // Custom aggregate rules that have been invoked but not returned any data + // will return an empty map to signal that they have been called, and that + // the aggregate report for this rule should be invoked even when no data + // was aggregated. This because the absence of data is exactly what some rules + // will want to report on. + for _, agg := range result.Aggregates[k] { + if len(agg) == 0 { + if _, ok := regoReport.Aggregates[k]; !ok { + regoReport.Aggregates[k] = make([]report.Aggregate, 0) + } + } else { + regoReport.Aggregates[k] = append(regoReport.Aggregates[k], agg) + } + } } for k := range result.IgnoreDirectives {