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

PE with shallow inlining producing incorrect result for some function calls #3681

Closed
tsandall opened this issue Jul 27, 2021 · 0 comments · Fixed by #3683
Closed

PE with shallow inlining producing incorrect result for some function calls #3681

tsandall opened this issue Jul 27, 2021 · 0 comments · Fixed by #3683
Assignees
Labels

Comments

@tsandall
Copy link
Member

Given the following policy:

package x

p {
  y = f(1)
  count(y)
}

f(x) = [] {
    _ = input  # anything dependent on an unknown will do
}

PE w/ shallow inlining produces the following:

$ docker run -it --rm -v  $PWD:/src -w /src openpolicyagent/opa:0.30.2 eval -f pretty -d ./x.rego 'data.x.p' -p  -u input  --explain=full --shallow-inlining
query:1        Enter data.x.p
query:1        | Eval data.x.p
query:1        | Index data.x.p (matched 1 rule)
./x.rego:3     | Enter data.x.p
./x.rego:4     | | Eval data.x.f(1, __local1__)
./x.rego:4     | | Index data.x.f (matched 1 rule)
./x.rego:8     | | Enter data.x.f
./x.rego:9     | | | Eval _ = input
./x.rego:9     | | | Save _ = input
./x.rego:8     | | | Exit data.x.f
./x.rego:8     | | Redo data.x.f
./x.rego:9     | | | Redo _ = input
./x.rego:4     | | Save data.partial.x.f(1, __local1__)
./x.rego:4     | | Eval y = __local1__
./x.rego:5     | | Eval count(y)
./x.rego:5     | | Fail count(y)
./x.rego:4     | | Redo y = __local1__
./x.rego:4     | | Redo data.x.f(1, __local1__)
query:1        | Save data.partial.x.p = _
query:1        | Save _
query:1        | Exit data.x.p
query:1        Redo data.x.p
query:1        | Fail data.x.p
+-----------+------------------------------+
| Query 1   | data.partial.x.p = _term_0_0 |
|           | _term_0_0                    |
+-----------+------------------------------+
| Support 1 | package partial.x            |
|           |                              |
|           | f(__local0__2) = [] {        |
|           |   _ = input                  |
|           | }                            |
+-----------+------------------------------+

Running PE with --strict-builtin-errors reveals the following:

1 error occurred: ./x.rego:5: eval_type_error: count: operand 1 must be one of {array, object, set} but got var

The problem with the trace above is here:

./x.rego:4     | | Eval y = __local1__
./x.rego:5     | | Eval count(y)
./x.rego:5     | | Fail count(y)

In this case, the Fail on L5 is actually due to the type error above. However, count() should either be invoked with [] or it should not be invoked at all and it should be saved.

Note, evaluating without shallow inlining produces the expected answer (_ = input); PE also returns the correct answer when the output contains unknowns if shallow inlining is off (count([input])).

@tsandall tsandall added the bug label 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 srenatus self-assigned this Jul 30, 2021
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants