Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Built-in function errors should not turn into undefined if terms requiring evaluation are passed (Var, Ref, etc.) #3680

Open
tsandall opened this issue Jul 26, 2021 · 1 comment

Comments

@tsandall
Copy link
Member

tsandall commented Jul 26, 2021

OPA should return an internal error instead of invoking the built-in function if the operand requires evaluation. The built-in functions assume that operands have been plugged and are grounded so if they receive a type they do not support they return a generic eval_type_error that gets turned into undefined unless strict builtin error checking is enabled. We should improve the evaluator to return an internal error if builtin functions would be run with terms that require evaluation.

Note, this is not really a bug but an improvement that will help better surface bugs in OPA itself.

@tsandall tsandall added the bug label Jul 26, 2021
@tsandall tsandall added enhancement and removed bug labels Jul 27, 2021
srenatus added a commit to srenatus/opa that referenced this issue Jul 30, 2021
Before, we had been passing `len(e.terms)` to `saveCall`, thereby
sidestepping the checks that would get the output variable into the
save set.

In the problematic policy,

    y = f(1)
    count(y)

with

    f(x) = [] { _ = input }

the call to `count(y)` would be mishandled: it would be evaluated,
since its arguments aren't in the saveset. Eval would fail because
its argument was a variable (see also
open-policy-agent#3680)

Fixes open-policy-agent#3681.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
srenatus added a commit that referenced this issue Aug 12, 2021
Before, we had been passing `len(e.terms)` to `saveCall`, thereby
sidestepping the checks that would get the output variable into the
save set.

In the problematic policy,

    y = f(1)
    count(y)

with

    f(x) = [] { _ = input }

the call to `count(y)` would be mishandled: it would be evaluated,
since its arguments aren't in the saveset. Eval would fail because
its argument was a variable (see also
#3680)

Fixes #3681.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
jgchn pushed a commit to jgchn/opa that referenced this issue Aug 20, 2021
…-agent#3683)

Before, we had been passing `len(e.terms)` to `saveCall`, thereby
sidestepping the checks that would get the output variable into the
save set.

In the problematic policy,

    y = f(1)
    count(y)

with

    f(x) = [] { _ = input }

the call to `count(y)` would be mishandled: it would be evaluated,
since its arguments aren't in the saveset. Eval would fail because
its argument was a variable (see also
open-policy-agent#3680)

Fixes open-policy-agent#3681.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
dolevf pushed a commit to dolevf/opa that referenced this issue Nov 4, 2021
…-agent#3683)

Before, we had been passing `len(e.terms)` to `saveCall`, thereby
sidestepping the checks that would get the output variable into the
save set.

In the problematic policy,

    y = f(1)
    count(y)

with

    f(x) = [] { _ = input }

the call to `count(y)` would be mishandled: it would be evaluated,
since its arguments aren't in the saveset. Eval would fail because
its argument was a variable (see also
open-policy-agent#3680)

Fixes open-policy-agent#3681.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
Signed-off-by: Dolev Farhi <farhi.dolev@gmail.com>
@tsandall tsandall added rego and removed enhancement labels Dec 3, 2021
@stale
Copy link

stale bot commented Jan 2, 2022

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

@stale stale bot added the inactive label Jan 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant