From 743a65b30a872b9793e3d98c7aef7c773575fc3c Mon Sep 17 00:00:00 2001 From: Anders Eknert Date: Thu, 14 Nov 2024 10:58:46 +0100 Subject: [PATCH] Fix custom rules that report on the absence of aggregates (#1260) 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 {