Rule: unresolved-import (#658)

Requested since 2017! This aggregate rule will try to resolve any
import provided in policy, while allowing the user to configure
paths that should be resolved at runtime, like data refs.

Fixes #300

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

README.md

@@ -232,6 +232,7 @@ The following rules are currently available:
 | imports     | [prefer-package-imports](https://docs.styra.com/regal/rules/imports/prefer-package-imports)           | Prefer importing packages over rules                      |
 | imports     | [redundant-alias](https://docs.styra.com/regal/rules/imports/redundant-alias)                         | Redundant alias                                           |
 | imports     | [redundant-data-import](https://docs.styra.com/regal/rules/imports/redundant-data-import)             | Redundant import of data                                  |
+| imports     | [unresolved-import](https://docs.styra.com/regal/rules/imports/unresolved-import)                     | Unresolved import                                         |
 | imports     | [use-rego-v1](https://docs.styra.com/regal/rules/imports/use-rego-v1)                                 | Use `import rego.v1`                                      |
 | performance | [with-outside-test-context](https://docs.styra.com/regal/rules/performance/with-outside-test-context) | `with` used outside test context                          |
 | style       | [avoid-get-and-list-prefix](https://docs.styra.com/regal/rules/style/avoid-get-and-list-prefix)       | Avoid `get_` and `list_` prefix for rules and functions   |

bundle/regal/config/provided/data.yaml

@@ -81,6 +81,8 @@ rules:
       level: error
     redundant-data-import:
       level: error
+    unresolved-import:
+      level: error
     use-rego-v1:
       level: error
   performance:

bundle/regal/rules/imports/unresolved_import.rego

@@ -0,0 +1,142 @@
+# METADATA
+# description: Unresolved import
+package regal.rules.imports["unresolved-import"]
+
+import rego.v1
+
+import data.regal.config
+import data.regal.result
+import data.regal.util
+
+aggregate contains entry if {
+	package_path := [part.value | some i, part in input["package"].path; i > 0]
+
+	imports_with_location := [imp |
+		some _import in input.imports
+
+		_import.path.value[0].value == "data"
+		len := count(_import.path.value)
+		len > 1
+		path := [part.value | some part in array.slice(_import.path.value, 1, len)]
+
+		# Special case for custom rules, where we don't want to flag e.g. `import data.regal.ast`
+		# as unknown, even though it's not a package included in evaluation.
+		not custom_regal_package_and_import(package_path, path)
+
+		imp := object.union(result.location(_import), {"path": path})
+	]
+
+	exported_refs := {package_path} | {ref |
+		some rule in input.rules
+
+		# locations will only contribute to each item in the set being unique,
+		# which we don't want here — we only care for distinct ref paths
+		some ref in to_paths(package_path, rule.head.ref)
+	}
+
+	entry := result.aggregate(rego.metadata.chain(), {
+		"imports": imports_with_location,
+		"exported_refs": exported_refs,
+	})
+}
+
+# METADATA
+# schemas:
+#   - input: schema.regal.aggregate
+aggregate_report contains violation if {
+	all_known_refs := {path |
+		some entry in input.aggregate
+
+		some path in entry.aggregate_data.exported_refs
+	}
+
+	all_imports := {imp |
+		some entry in input.aggregate
+		some imp in entry.aggregate_data.imports
+	}
+
+	some imp in all_imports
+	not imp.path in (all_known_refs | except_imports)
+
+	# cheap operation failed — need to check wildcards here to account
+	# for map generating / general ref head rules
+	not wildcard_match(imp.path, all_known_refs, except_imports)
+
+	violation := result.fail(rego.metadata.chain(), result.location(imp))
+}
+
+custom_regal_package_and_import(pkg_path, path) if {
+	pkg_path[0] == "custom"
+	pkg_path[1] == "regal"
+	path[0] == "regal"
+}
+
+# the package part will always be included exported refs
+# but if we have a rule like foo.bar.baz
+# we'll want to include both foo.bar and foo.bar.baz
+to_paths(pkg_path, ref) := [to_path(pkg_path, ref)] if count(ref) < 3
+
+to_paths(pkg_path, ref) := paths if {
+	count(ref) > 2
+
+	paths := [path |
+		some p in util.all_paths(ref)
+		path := to_path(pkg_path, p)
+	]
+}
+
+to_path(pkg_path, ref) := array.concat(pkg_path, [str |
+	some i, part in ref
+	str := to_string(i, part)
+])
+
+to_string(0, part) := part.value
+
+to_string(i, part) := part.value if {
+	i > 0
+	part.type == "string"
+}
+
+to_string(i, part) := "**" if {
+	i > 0
+	part.type == "var"
+}
+
+all_paths(path) := [array.slice(path, 0, len) | some len in numbers.range(1, count(path))]
+
+except_imports contains exception if {
+	cfg := config.for_rule("imports", "unresolved-import")
+
+	some str in cfg["except-imports"]
+	exception := trim_data(split(str, "."))
+}
+
+trim_data(path) := array.slice(path, 1, count(path)) if path[0] == "data"
+
+trim_data(path) := path if path[0] != "data"
+
+wildcard_match(imp_path, all_known_refs, except_imports) if {
+	except_imports_wildcards := {path |
+		some except in except_imports
+		path := concat(".", except)
+		contains(path, "*")
+	}
+
+	all_known_refs_wildcards := {path |
+		some ref in all_known_refs
+		path := concat(".", ref)
+		contains(path, "*")
+	}
+
+	all_wildcard_paths := except_imports_wildcards | all_known_refs_wildcards
+
+	some path in all_wildcard_paths
+
+	# note that we are quite forgiving here, as we'll match the
+	# shortest path component containing a wildcard at the end..
+	# we may want to make this more strict later, but as this is
+	# a new rule with a potentially high impact, let's start like
+	# this and then decide if we want to be more strict later, and
+	# perhaps offer that as a "strict" option
+	glob.match(path, [], concat(".", imp_path))
+}

bundle/regal/rules/imports/unresolved_import_test.rego

@@ -0,0 +1,124 @@
+package regal.rules.imports["unresolved-import_test"]
+
+import rego.v1
+
+import data.regal.config
+
+import data.regal.rules.imports["unresolved-import"] as rule
+
+test_fail_identifies_unresolved_imports if {
+	agg1 := rule.aggregate with input as regal.parse_module("p1.rego", `package foo
+	import data.bar
+	import data.bar.x
+	import data.bar.nope
+	import data.nope
+
+	x := 1
+	`)
+	agg2 := rule.aggregate with input as regal.parse_module("p2.rego", `package bar
+	import data.foo
+	import data.foo.x
+
+	x := 1
+	`)
+
+	r := rule.aggregate_report with input as {"aggregate": (agg1 | agg2)}
+	r == {
+		with_location({"file": "p1.rego", "row": 5, "col": 2, "text": "\timport data.nope"}),
+		with_location({"file": "p1.rego", "row": 4, "col": 2, "text": "\timport data.bar.nope"}),
+	}
+}
+
+test_success_no_unresolved_imports if {
+	agg1 := rule.aggregate with input as regal.parse_module("p1.rego", `package foo
+	import data.bar.x
+
+	x := 1
+	`)
+	agg2 := rule.aggregate with input as regal.parse_module("p2.rego", `package bar
+	import data.foo.x
+
+	x := 1
+	`)
+
+	r := rule.aggregate_report with input as {"aggregate": (agg1 | agg2)}
+	r == set()
+}
+
+test_success_unresolved_imports_are_excepted if {
+	agg1 := rule.aggregate with input as regal.parse_module("p1.rego", `package foo
+	import data.bar.x
+	import data.bar.excepted
+
+	x := 1
+	`)
+	agg2 := rule.aggregate with input as regal.parse_module("p2.rego", `package bar
+
+	x := 1
+	`)
+
+	r := rule.aggregate_report with input as {"aggregate": (agg1 | agg2)}
+		with config.for_rule as {"level": "error", "except-imports": ["data.bar.excepted"]}
+	r == set()
+}
+
+test_success_resolved_import_in_middle_of_explicit_paths if {
+	agg1 := rule.aggregate with input as regal.parse_module("p1.rego", `package foo
+	import data.bar.x.y
+	`)
+	agg2 := rule.aggregate with input as regal.parse_module("p2.rego", `package bar
+
+	x.y.z := 1
+	`)
+
+	r := rule.aggregate_report with input as {"aggregate": (agg1 | agg2)}
+	r == set()
+}
+
+test_success_map_rule_may_resolve_so_allow if {
+	agg1 := rule.aggregate with input as regal.parse_module("p1.rego", `package foo
+	import data.bar.x.y
+	`)
+
+	agg2 := rule.aggregate with input as regal.parse_module("p2.rego", `package bar
+	import rego.v1
+
+	x[y] := z if {
+		some y in input.ys
+		z := {"foo": y + 1}
+	}
+	`)
+
+	r := rule.aggregate_report with input as {"aggregate": (agg1 | agg2)}
+	r == set()
+}
+
+test_success_general_ref_head_rule_may_resolve_so_allow if {
+	agg1 := rule.aggregate with input as regal.parse_module("p1.rego", `package foo
+	import data.bar.x.foo.z.bar
+	`)
+
+	agg2 := rule.aggregate with input as regal.parse_module("p2.rego", `package bar
+	import rego.v1
+
+	x[y].z[foo] := z if {
+		some y in input.ys
+		z := {"foo": y + 1}
+	}
+	`)
+
+	r := rule.aggregate_report with input as {"aggregate": (agg1 | agg2)}
+	r == set()
+}
+
+with_location(location) := {
+	"category": "imports",
+	"description": "Unresolved import",
+	"level": "error",
+	"location": location,
+	"related_resources": [{
+		"description": "documentation",
+		"ref": config.docs.resolve_url("$baseUrl/$category/unresolved-import", "imports"),
+	}],
+	"title": "unresolved-import",
+}

bundle/regal/rules/style/prefer_some_in_iteration.rego

@@ -7,6 +7,7 @@ import rego.v1
 import data.regal.ast
 import data.regal.config
 import data.regal.result
+import data.regal.util
 
 cfg := config.for_rule("style", "prefer-some-in-iteration")
 
@@ -59,11 +60,9 @@ filter_top_level_ref(rule) := rule.body if {
 	rule.head.value.type == "ref"
 } else := rule
 
-all_paths(path) := [array.slice(path, 0, len) | some len in numbers.range(1, count(path))]
-
 # don't recommend `some .. in` if iteration occurs inside of arrays, objects, or sets
 invalid_some_context(rule, path) if {
-	some p in all_paths(path)
+	some p in util.all_paths(path)
 
 	node := object.get(rule, p, [])
 
@@ -76,7 +75,7 @@ invalid_some_context(rule, path) if {
 # not _directly_ replaceable by `some .. in`, so we'll leave it
 # be here
 invalid_some_context(rule, path) if {
-	some p in all_paths(path)
+	some p in util.all_paths(path)
 
 	node := object.get(rule, p, [])
 

bundle/regal/util/util.rego

@@ -18,3 +18,9 @@ find_duplicates(arr) := {indices |
 
 	count(indices) > 1
 }
+
+# METADATA
+# description: |
+#   returns an array of arrays built from all parts of the provided path array,
+#   so e.g. [1, 2, 3] would return [[1], [1, 2], [1, 2, 3]]
+all_paths(path) := [array.slice(path, 0, len) | some len in numbers.range(1, count(path))]

docs/rules/imports/unresolved-import.md

@@ -0,0 +1,64 @@
+# unresolved-import
+
+**Summary**: Unresolved import
+
+**Category**: Imports
+
+**Avoid**
+
+Imports that can't be resolved.
+
+## Rationale
+
+OPA does no compile time checks to ensure that references in imports _resolve_ to anything, and unresolved references at
+runtime are simply **undefined**. This is not a bug in OPA, but a necessary feature to allow for dynamic loading of data
+and policy at runtime. The fact that it's not a bug does however not mean that it can't be
+[a problem](https://github.com/open-policy-agent/opa/issues/491)! A simple typo, a refactoring, or a mistake, could
+easily lead to an an import being unresolved, and as such undefined at runtime.
+
+This rule takes a stricter approach to imports, and will have Regal try to resolve them by scanning all the policies it
+is provided for **packages**, **rules** and **functions** that may resolve the import. Note that Regal does not scan any
+_data_ files. If no reference is found, the rule will flag it as unresolved.
+
+Since unresolved imports may be perfectly valid — for example when an import points to data — this rule provides an
+option in its configuration to except certain paths from being checked. These paths may even contain a wildcard suffix
+to indicate that any path past the wildcard (e.g. `data.users.*`) should be ignored. It is also possible to use a
+regular [ignore directive](https://docs.styra.com/regal#inline-ignore-directives):
+
+```rego
+package example
+
+# this is provided as data!
+# regal ignore:unresolved-import
+import data.users
+```
+
+## Configuration Options
+
+This linter rule provides the following configuration options:
+
+```yaml
+rules: 
+  imports:
+    unresolved-import:
+      # one of "error", "warning", "ignore"
+      level: error
+      # list of paths that should be ignored
+      # these may be paths to data, or rules that may
+      # not be present at the time of linting
+      except-imports:
+        - data.identity.users
+        - data.permissions.*
+```
+
+## Related Resources
+
+- OPA Docs: [Imports](https://www.openpolicyagent.org/docs/latest/policy-language/#imports)
+- OPA Docs: [Collaboration Using Import](https://www.openpolicyagent.org/docs/latest/faq/#collaboration-using-import)
+- OPA Issues: [Missing import should create error](https://github.com/open-policy-agent/opa/issues/491)
+
+## 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)!