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

Non-deterministic builtin functions can break policies run through partial evaluation #5171

Closed
philipaconrad opened this issue Sep 23, 2022 · 0 comments · Fixed by #5172
Closed
Assignees
Labels

Comments

@philipaconrad
Copy link
Contributor

philipaconrad commented Sep 23, 2022

Short description

When building a bundle with:

  • An optimization level > 0 and
  • opa.runtime() (or other builtin functions) used in the policy

The resulting optimized policy can wind up missing critical rules entirely. This occurs because the optimizer attempts partial evaluation at bundle build time, which can have nonsensical results for some non-deterministic Rego builtins that query the environment. (http.send, opa.runtime, etc.)

Short example

An easy example of this is the following policy:
test.rego:

package test

allow {
	opa.runtime().env.MY_APP_ID == input.details.id
}

Steps to reproduce

  1. opa build ./test/ -b -O=2 -e test/allow
  2. Check bundle, and see the following:

bundle.tar.gz/optimized/test.rego:

package test

default allow = false

Expected behavior

The optimizer should not be attempting to partially evaluate non-deterministic builtin functions like opa.runtime. Adding that one builtin to the "unsafe for partial eval" list would be enough on its own for solving some immediate issues, but it's probably best just to remove all of them from partial eval for safety.

Workaround techniques

You can currently force the optimizer to leave the opa.runtime calls alone by wrapping them in constructs that are opaque to it, like aggregate comprehensions, and else rules. (Thanks @tsandall for figuring this out!)

Example of else rule wrapping:

package test

allow {
	my_app_id == input.details.id
}

# HACK: The else rule here allows us to hide the opa.runtime call from partial eval in this policy.
my_app_id := x {
  x := opa.runtime().env.MY_APP_ID
} else := ""

Additional context

@philipaconrad philipaconrad self-assigned this Sep 23, 2022
philipaconrad added a commit to philipaconrad/opa that referenced this issue Sep 23, 2022
This commit removes the `IgnoreDuringPartialEval` list in
`ast/builtins`, and instead changes the partial evaluator to simply
ignore all non-deterministic builtins. This keeps true to the intent of
the original list, and reduces the potential for human error in
maintenance in the future.

Fixes: open-policy-agent#5171

Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
anderseknert pushed a commit that referenced this issue Sep 23, 2022
This commit removes the `IgnoreDuringPartialEval` list in
`ast/builtins`, and instead changes the partial evaluator to simply
ignore all non-deterministic builtins. This keeps true to the intent of
the original list, and reduces the potential for human error in
maintenance in the future.

Fixes: #5171

Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant