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 -t wasm' with functions defined on entrypoint fails with "undefined ref" #4411

Closed
tristanls opened this issue Mar 4, 2022 · 15 comments

Comments

@tristanls
Copy link

tristanls commented Mar 4, 2022

Short description

Everything builds and tests pass. When attempting to build for WASM:

opa build -e "reproduction" -t wasm reproduction.rego
error: 1 error occurred: reproduction.rego:1: rego_type_error: undefined ref: data.reproduction.func_decide
        data.reproduction.func_decide
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        have: (any) => object<bool: boolean>

Problem observed on 0.38.0, 0.37.2

Steps To Reproduce

reproduction.rego

package reproduction

func_not(y) = {"bool":false} {
    y.bool
} else = {"bool":true}

func_decide(something) = func_not(something)

reproduction_test.rego

package reproduction

test_repro {
    func_decide({"bool":false}).bool
}

Expected behavior

Successful WASM build or test build failing or tests not passing.

Additional context

@tristanls tristanls added the bug label Mar 4, 2022
@srenatus
Copy link
Contributor

srenatus commented Mar 4, 2022

Tests don't use the wasm eval path iirc... I suppose that's worth fixing. And the overlying problem, too, of course. Thanks for reporting

@srenatus
Copy link
Contributor

srenatus commented Mar 4, 2022

Sent too early. That fail is at build time, before anything is evaluated.

@srenatus srenatus self-assigned this Mar 9, 2022
@srenatus
Copy link
Contributor

srenatus commented Mar 9, 2022

So it only happens with -t wasm.

@srenatus
Copy link
Contributor

srenatus commented Mar 9, 2022

The example can be boiled down to

package reproduction

g(_) = true
f(x) = g(x)

However, it's unclear to me what the desired outcome of that Wasm bundle build is: If we only have functions in a package, evaluating the package's extent (i.e. using entrypoint -e reproduction) will be {}.

If we have a rule in the that package, using that as entrypoint would seem like a decent workaround, since it's got the same result, and would not trigger this limitation.

In the example above, if we had

package reproduction

q = f(1) # new
g(_) = true
f(x) = g(x)

and used -e reproduction/q, the build (and subsequent eval of the wasm bundle) should work fine.

I wonder when this would not be an option 🤔 I suppose if you have more than one rule,

package reproduction

q = f(1)
r = f(2) # new
g(_) = true
f(x) = g(x)

it would be desirable to have a -e reproduction entrypoint... but the workaround would be to pass two -e, -e reproduction/q -e reproduction/r.

Bottom line, there is a bug in the compiler code triggered here; but for prioritizing, it would help to know more about your use -- i.e., is the mentioned workaround viable for you?

@tristanls
Copy link
Author

tristanls commented Mar 9, 2022

Thank you for confirming.

As you discovered, the reproduction (and your further reduction) is trivial and not interesting. This comes up when composing multiple functions into complex rules. I attempted a simplest example of the problem in this issue, but here's the shape where this comes up:

query_filter_has_x(query, filter) = {"bool": true, "reason": "filter has x"} {
    query[filter].x
} else = {"bool": false, "reason": "filter does not have x"}

query_filter_has_y(query, filter) = {"bool": true, "reason": "filter has y"} {
    query[filter].y
} else = {"bool": false, "reason": "filter does not have y"}

query_filter_blah(query, filter) = {"bool": true, "reason": "filter blah"} {
    query[filter].blah
} else = {"bool": false, "reason": "filter does not blah"}

# with the above shape, you can start composing things which will propagate reasons to the output
# imagine utils.bool_and, utils.bool_or, and utils.bool_not do what you think they should
# and return {"bool": bool, "reason": string} shapes

query_filter_something_more_complex(query, filter) = utils.bool_and(
  [
    query_filter_has_x(query, filter),
    utils.bool_not(query_filter_has_y(query, filter)),
    utils.bool_or(
     [
       query_filter_has_y(query, filter),
       query_filter_blah(query, filter)
     ],
     {
       "true_reason": "y or blah", 
       "false_reason": "not y or not blah"
     }
  ],
  {
    "true_reason": "something more complex",
    "false_reason": "something not more complex"
  }
)

# then we use these constructs for actual rules
allow = {
  query_filter_something_more_complex(my_query, my_filter).bool
}
reason["allow_something_more_complex"] = query_filter_something_more_complex(my_query, my_filter).reason

The f(x) = g(x) is something more useful like:

query_filter_something_more_complex(query, filter) = utils.bool_and(...)

When attempting function composition like this, things will fail to compile. That's the real use case, function composition. The real use case uses allow and reason rules to get policy decisions, but those are composed of simpler constructs.

Regarding prioritization, while the pattern I shared above will not compile in a single package, we did manage to find a workaround and noticed that if we put the functions being composed in a separate package, then we can compose them. This has worked so far, and may work for now. My concern is with artificially breaking things out into packages instead of keeping code that goes together together.

@srenatus srenatus changed the title Tests pass, wasm build fails. 'opa build -t wasm' with functions defined on entrypoint fails with "undefined ref" Mar 10, 2022
@srenatus srenatus added this to Backlog in Open Policy Agent via automation Mar 10, 2022
@srenatus
Copy link
Contributor

Regarding prioritization, while the pattern I shared above will not compile in a single package, we did manage to find a workaround and noticed that if we put the functions being composed in a separate package, then we can compose them. This has worked so far, and may work for now. My concern is with artificially breaking things out into packages instead of keeping code that goes together together.

I'm glad you've got a workaround; this still is something we should fix. I'd have to dig in a bit to understand the rationale behind the current way things are done 🔍

May I ask about your use case? I'm always curious about people using wasm 😅

@tristanls
Copy link
Author

tristanls commented Mar 12, 2022

May I ask about your use case?

Oh, nothing exciting, run in Node.js :)

@srenatus
Copy link
Contributor

Using npm-opa-wasm?

@tristanls
Copy link
Author

🤔 ...

const {loadPolicy} = require("@open-policy-agent/opa-wasm")

Yes. Very helpful.

@anderseknert
Copy link
Member

Thanks for the pointer @srenatus :) This came up in the context of Jarl, where there are 70-ish tests currently not included, as we use the package as entrypoint in those tests, and any tests including references to functions break the compilation.

@srenatus
Copy link
Contributor

@tristanls Whats your use case? Would passing --prune-unused to opa build help you, too? (See #5035)

@tristanls
Copy link
Author

tristanls commented Sep 8, 2022

@srenatus I am having difficulty understanding what --prune-unused does.
In the example you gave:

package reproduction

q = f(1)
r = f(2) # new
g(_) = true
f(x) = g(x)

What is pruned when --prune-unused is used?

When I tried to use latest opa release:

$ opa build -e "reproduction" --prune-unused -t wasm reproduction.rego

I got Error: unknown flag: --prune-unused

opa version
Version: 0.43.1
Build Commit: 196c92d
Build Timestamp: 2022-09-07T18:03:44Z
Build Hostname: f8ae390c74c4
Go Version: go1.18.6
Platform: linux/amd64
WebAssembly: unavailable

@srenatus
Copy link
Contributor

srenatus commented Sep 8, 2022

Please try again with 0.44.0.

@anderseknert
Copy link
Member

@tristanls if you find the time, I'd be curious to know if this helped solve your issue 🙂

@srenatus
Copy link
Contributor

Let's reopen if this still is a problem! 🧹

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

3 participants