Skip to content

Commit

Permalink
Fix custom rules that report on the absence of aggregates (#1260)
Browse files Browse the repository at this point in the history
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 <[email protected]>
  • Loading branch information
anderseknert authored Nov 14, 2024
1 parent 0c6e8ed commit 743a65b
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 3 deletions.
20 changes: 18 additions & 2 deletions bundle/regal/main/main.rego
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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"],
)

Expand All @@ -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
24 changes: 24 additions & 0 deletions e2e/cli_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
@@ -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(), {})
}
15 changes: 14 additions & 1 deletion pkg/linter/linter.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit 743a65b

Please sign in to comment.