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

opa build generated WASM binary always returns empty set #2441

Closed
patrick-east opened this issue May 28, 2020 · 0 comments · Fixed by #2442
Closed

opa build generated WASM binary always returns empty set #2441

patrick-east opened this issue May 28, 2020 · 0 comments · Fixed by #2442
Assignees
Labels

Comments

@patrick-east
Copy link
Contributor

patrick-east commented May 28, 2020

Expected Behavior

When using the new OPA build command with an example policy like:

package example

default hello = false

hello {
    x := input.message
    x == data.world
}

And then building with a command like:

 build -t wasm -e example/hello ./example.rego

If I load and evaluate it, dumping all the results, I would expect that regardless of data or input it should at least return some value for the default rule, eg:

[
    { "x": false }
]

with older OPA versions where the query was something like x := data.example.hello

Actual Behavior

The result is consistently

[
  {}
]

Steps to Reproduce the Problem

Using the example over at https://github.com/open-policy-agent/npm-opa-wasm/tree/master/examples/nodejs-app

Building with:

opa build -t wasm -e 'example/hello' ./example.rego
tar -xzf ./bundle.tar.gz /policy.wasm

Then run with:

node app.js '{"message": "world"}'

This just dumps the resulting JSON document without any modifications.

Additional Info

You can see in the dumped plan output (not available with the build command? had to modify the source to dump to stdout) there is no "insert" for the result, and the $result variable is never defined in the static section with the other ones:

*ir.Policy Policy
| *ir.Static Static (2 strings)
| | *ir.StringConst &{Value:message}
| | *ir.StringConst &{Value:world}
| *ir.Plan Plan (1 blocks)
| | *ir.Block Block (4 statements)
| | | *ir.CallStmt &{Func:g0.data.example.hello Args:[0 1] Result:2}
| | | *ir.AssignVarStmt &{Source:2 Target:3}
| | | *ir.MakeObjectStmt &{Target:4}
| | | *ir.ResultSetAdd &{Value:4}
| *ir.Funcs Funcs (1 funcs)
| | *ir.Func g0.data.example.hello (2 params: [0 1], 3 blocks)
| | | *ir.Block Block (7 statements)

<snip>

This differs from the older compile if you use an "x" var, you get:

*ir.Policy Policy
| *ir.Static Static (3 strings)
| | *ir.StringConst &{Value:x}
| | *ir.StringConst &{Value:message}
| | *ir.StringConst &{Value:world}
| *ir.Plan Plan (2 blocks)
| | *ir.Block Block (1 statements)
| | | *ir.MakeStringStmt &{Index:0 Target:2}
| | *ir.Block Block (5 statements)
| | | *ir.CallStmt &{Func:g0.data.example.hello Args:[0 1] Result:3}
| | | *ir.AssignVarStmt &{Source:3 Target:4}
| | | *ir.MakeObjectStmt &{Target:5}
| | | *ir.ObjectInsertStmt &{Key:2 Value:4 Object:5}
| | | *ir.ResultSetAdd &{Value:5}
| *ir.Funcs Funcs (1 funcs)
| | *ir.Func g0.data.example.hello (2 params: [0 1], 3 blocks)
| | | *ir.Block Block (7 statements)

<snip>

(the snipped parts are identical)

Still need to debug more but it seems like maybe something being mishandled with the wildcard. If i edit the build flow to use __result__ rather than $result The plan looks more correct.

Update: So yea, we explicitly exclude wildcard vars that were in the query:

if !qv.IsGenerated() && !qv.IsWildcard() {

and
if !rw.IsGenerated() && !rw.IsWildcard() {

@patrick-east patrick-east self-assigned this May 28, 2020
patrick-east added a commit to patrick-east/opa that referenced this issue May 29, 2020
Previously it was being bound to `$result` but this was then stripped
out of the resultset by the planner. To avoid this issue we need the
variable to not be seen as a wildcard or generated var. The new one
is just `result`.

The documentation is also now updated to show this behavior. The
various client SDK's can strip it out as needed.

The idea is that this is going to just be a part of the built WASM
binary format. Anyone building with the lower level API's using
ad-hoc queries will not need to worry about anything changing.

Fixes: open-policy-agent#2441
Signed-off-by: Patrick East <east.patrick@gmail.com>
tsandall pushed a commit that referenced this issue Jun 1, 2020
Previously it was being bound to `$result` but this was then stripped
out of the resultset by the planner. To avoid this issue we need the
variable to not be seen as a wildcard or generated var. The new one
is just `result`.

The documentation is also now updated to show this behavior. The
various client SDK's can strip it out as needed.

The idea is that this is going to just be a part of the built WASM
binary format. Anyone building with the lower level API's using
ad-hoc queries will not need to worry about anything changing.

Fixes: #2441
Signed-off-by: Patrick East <east.patrick@gmail.com>
patrick-east added a commit to patrick-east/opa that referenced this issue Jun 1, 2020
Previously it was being bound to `$result` but this was then stripped
out of the resultset by the planner. To avoid this issue we need the
variable to not be seen as a wildcard or generated var. The new one
is just `result`.

The documentation is also now updated to show this behavior. The
various client SDK's can strip it out as needed.

The idea is that this is going to just be a part of the built WASM
binary format. Anyone building with the lower level API's using
ad-hoc queries will not need to worry about anything changing.

Fixes: open-policy-agent#2441
Signed-off-by: Patrick East <east.patrick@gmail.com>
(cherry picked from commit 729a853)
This was referenced Jun 1, 2020
patrick-east added a commit that referenced this issue Jun 1, 2020
Previously it was being bound to `$result` but this was then stripped
out of the resultset by the planner. To avoid this issue we need the
variable to not be seen as a wildcard or generated var. The new one
is just `result`.

The documentation is also now updated to show this behavior. The
various client SDK's can strip it out as needed.

The idea is that this is going to just be a part of the built WASM
binary format. Anyone building with the lower level API's using
ad-hoc queries will not need to worry about anything changing.

Fixes: #2441
Signed-off-by: Patrick East <east.patrick@gmail.com>
(cherry picked from commit 729a853)
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.

1 participant