open-policy-agent / opa

Open Policy Agent (OPA) is an open source, general-purpose policy engine.
https://www.openpolicyagent.org
Apache License 2.0
9.52k stars 1.32k forks source link

Improve reporting of `rego_unsafe_var_error` #6393

Open anderseknert opened 10 months ago

anderseknert commented 10 months ago

The way we currently report rego_unsafe_var_errors is really confusing, and reasonably more so for people new to Rego. Given a policy like this:

package play

rule {
    my_string := x

    startswith(my_string, "foo")

    substr := substring(my_string, 0, 1)

    substr == "f"
}

Rego Playground

There's really one unsafe reference here, which is that to x. The way we report this however is cascading — i.e. all references following the first failure are included as well:

3 errors occurred:
policy.rego:4: rego_unsafe_var_error: var my_string is unsafe
policy.rego:4: rego_unsafe_var_error: var x is unsafe
policy.rego:8: rego_unsafe_var_error: var substr is unsafe

That my_string and substr are unsafe is simply a consequence of x being so, and reporting that does not help with debugging — it rather makes it harder. We who have worked with Rego for a long time have just come to mentally filter these errors, but there's no reason we should. For anyone else, this likely contributes to a bad debugging experience.

(That the list of errors reported isn't ordered — and changes randomly at re-evaluation does nothing to help with this, but that's an issue that would fix itself by not reporting multiple unsafe vars in the first place.)

ashutosh-narkar commented 10 months ago

Agreed from a debugging perspective especially for new users improving this would be helpful.

tymik commented 1 month ago

I would add to this that I've just got the following:

rego_unsafe_var_error: var _ is unsafe

and just to a minute ago I was pretty sure that _ is defined as a part of language.

anderseknert commented 1 month ago

@tymik it's mostly just an unnamed variable really, but yeah, it could easily be made unsafe as part of this "chaining" of errors reported here.