Rule: with-outside-test-context (#555)

This also introduces a new `performance` category.

Fixes #548

Signed-off-by: Anders Eknert <[email protected]>

bundle/regal/config/provided/data.yaml

@@ -81,6 +81,9 @@ rules:
       level: error
     use-rego-v1:
       level: error
+  performance:
+    with-outside-test-context:
+      level: error
   style:
     avoid-get-and-list-prefix:
       level: error

bundle/regal/main.rego

@@ -116,6 +116,7 @@ aggregate_report contains violation if {
 		["aggregates_internal"],
 	)
 
+	# regal ignore:with-outside-test-context
 	some violation in data.regal.rules[category][title].aggregate_report with input as input_for_rule
 
 	not ignored(violation, ignore_directives)
@@ -137,6 +138,7 @@ aggregate_report contains violation if {
 		["aggregates_internal"],
 	)
 
+	# regal ignore:with-outside-test-context
 	some violation in data.custom.regal.rules[category][title].aggregate_report with input as input_for_rule
 
 	not ignored(violation, ignore_directives)

bundle/regal/rules/performance/with_outside_test_context.rego

@@ -0,0 +1,18 @@
+# METADATA
+# description: '`with` used outside test context'
+package regal.rules.performance["with-outside-test-context"]
+
+import rego.v1
+
+import data.regal.ast
+import data.regal.result
+
+report contains violation if {
+	some rule in input.rules
+	some expr in rule.body
+
+	expr["with"]
+	not strings.any_prefix_match(ast.name(rule), {"test_", "todo_test"})
+
+	violation := result.fail(rego.metadata.chain(), result.location(expr["with"][0]))
+}

bundle/regal/rules/performance/with_outside_test_context_test.rego

@@ -0,0 +1,51 @@
+package regal.rules.performance["with-outside-test-context_test"]
+
+import rego.v1
+
+import data.regal.ast
+import data.regal.config
+
+import data.regal.rules.performance["with-outside-test-context"] as rule
+
+test_fail_with_used_outside_test if {
+	module := ast.with_rego_v1(`
+	allow if {
+		not foo.deny with input as {}
+	}
+	`)
+
+	r := rule.report with input as module
+	r == {{
+		"category": "performance",
+		"description": "`with` used outside test context",
+		"level": "error",
+		"location": {"col": 16, "file": "policy.rego", "row": 7, "text": "\t\tnot foo.deny with input as {}"},
+		"related_resources": [{
+			"description": "documentation",
+			"ref": config.docs.resolve_url("$baseUrl/$category/with-outside-test-context", "performance"),
+		}],
+		"title": "with-outside-test-context",
+	}}
+}
+
+test_success_with_used_in_test if {
+	module := ast.with_rego_v1(`
+	test_foo_deny if {
+		not foo.deny with input as {}
+	}
+	`)
+
+	r := rule.report with input as module
+	r == set()
+}
+
+test_success_with_used_in_todo_test if {
+	module := ast.with_rego_v1(`
+	todo_test_foo_deny if {
+		not foo.deny with input as {}
+	}
+	`)
+
+	r := rule.report with input as module
+	r == set()
+}

docs/rules/performance/with-outside-test-context.md

@@ -0,0 +1,98 @@
+# with-outside-test-context
+
+**Summary**: `with` used outside of test context
+
+**Category**: Performance
+
+**Avoid**
+```rego
+package policy
+
+import rego.v1
+
+allow if {
+    some user in data.users
+
+    # mock input to pass data to `allowed_user` rule
+    allowed_user with input as {"user": user}
+}
+
+verified := io.jwt.verify_rs256(input.token, data.keys.verification_key)
+
+allowed_user := input.user if {
+    # this expensive rule will be evaluated for each user!
+    verified
+    "admin" in input.user.roles
+}
+```
+
+**Prefer**
+```rego
+package policy
+
+import rego.v1
+
+allow if {
+    some user in data.users
+
+    allowed_user({"user": user})
+}
+
+verified := io.jwt.verify_rs256(input.token, data.keys.verification_key)
+
+allowed_user(user) := user if {
+    # this expensive rule will be evaluated only once
+    verified
+    "admin" in input.user.roles
+}
+```
+
+## Rationale
+
+The `with` keyword exists primarily as a way to easily mock `input` or `data` in unit tests. While it's not forbidden to
+use `with` in other contexts, and it's occasionally useful to do so, `with` is not optimized for performance and can
+easily result in increased evaluation time if not used with care.
+
+One optimization that OPA does all the time is to cache the result of rule evaluation. If OPA needs to evaluate the same
+rule more than once as part of evaluating a query, the result of the first evaluation is memorized and the cost of
+subsequent evaluations is essentially zero. Caching however assumes that the conditions that produced the result of the
+first evaluation won't _change_ — and changing the conditions (i.e. `input` or `data`) for evaluation is the very
+purpose of `with`! This means that rules evaluated in the context of `with` won't be cached, and an expensive operation,
+like the `io.jwt.verify_rs256` built-in function called in the examples above would be evaluated for each `user` in
+`data.users`, even if the `with` clause in this case doesn't change any value that the JWT verification function depends
+on.
+
+## Exceptions
+
+The obvious exception is stated already in the title of this rule: unit tests! Use `with` as much as want here, as that
+is what `with` is for.
+
+Using `with` outside the context of unit tests is most commonly seen in policies using
+[dynamic policy composition](https://www.styra.com/blog/dynamic-policy-composition-for-opa/), which typically involves
+a "main" policy dispatching to a number of other policies and aggregating the result of evaluating each one. In this
+scenario it's quite common to need to alter either `input` or `data` before evaluating a policy or rule, and `with` is
+commonly used for this purpose. If you need to use `with` outside of tests, make sure that rules evaluated frequently
+are done so outside of the scope of `with` to avoid performance issues.
+
+## Configuration Options
+
+This linter rule provides the following configuration options:
+
+```yaml
+rules:
+  performance:
+    with-outside-test-context:
+      # one of "error", "warning", "ignore"
+      level: error
+```
+
+## Related Resources
+
+- OPA Docs: [With Keyword](https://www.openpolicyagent.org/docs/latest/policy-language/#with-keyword)
+- Styra Blog: [Dynamic Policy Composition for OPA](https://www.styra.com/blog/dynamic-policy-composition-for-opa/)
+
+## Community
+
+If you think you've found a problem with this rule or its documentation, would like to suggest improvements, new rules,
+or just talk about Regal in general, please join us in the `#regal` channel in the Styra Community
+[Slack](https://communityinviter.com/apps/styracommunity/signup)!

e2e/testdata/violations/most_violations.rego

@@ -230,3 +230,9 @@ fine if not not_fine
 
 # rule name repeats package name
 all_violations := true
+
+### Performance
+
+with_outside_test if {
+	foo with input as {}
+}