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

CallDynamic regression #5964

Closed
srenatus opened this issue Jun 2, 2023 · 1 comment · Fixed by #5965
Closed

CallDynamic regression #5964

srenatus opened this issue Jun 2, 2023 · 1 comment · Fixed by #5965

Comments

@srenatus
Copy link
Contributor

srenatus commented Jun 2, 2023

opa eval and opa eval -t wasm differ for

# x.rego
package x
import future.keywords.if

foo.bar.allow if f(7)
foo.baz.allow if f(8)
qux.corge.allow if f(-1)

f(_) if true

and

# y.rego
package y
import future.keywords.if

static if data.x.foo.bar.allow

dd1 if data.x[input.a].bar.allow

dd2 if data.x[input.a][input.b].allow

When evaluating data.y with input as {"a": "foo", "b": "bar"}, opa eval -d x.rego -d y.rego -i input.json -f pretty data.y yields

{
  "static": true,
  "dd1": true,
  "dd2": true
}

whereas adding -t wasm only yields

{
  "static": true
}

Probably related to #5839

@srenatus
Copy link
Contributor Author

srenatus commented Jun 2, 2023

The problem is that it's stopping to eval with

ref data.x[__local1__]: at least one rule has 1+ rules, break

when optimizing data.x[input.a].bar.allow (and same for the dd2 ref), where it really should not do that.

srenatus added a commit to srenatus/opa that referenced this issue Jun 2, 2023
So the check introduced before was too broad: it aborted optimizations
at the wrong spot -- the resulting plan didn't add up: path lengths used
in CallDynamicStmt didn't match path lengths that the planned funcs
had.

So, while this change looks like over-fitting (also to me), we're really
trying to make test previous fix more specific.

Generally looking at that section of the planner, it feels like the intro
of general refs would be a good moment to nuke and start over: the way
that refs-with-vars are put into the ruletrie seems like the root cause of
our trouble here.

Fixes open-policy-agent#5964.

Signed-off-by: Stephan Renatus <stephan@styra.com>
srenatus added a commit to srenatus/opa that referenced this issue Jun 2, 2023
So the check introduced before was too broad: it aborted optimizations
at the wrong spot -- the resulting plan didn't add up: path lengths used
in CallDynamicStmt didn't match path lengths that the planned funcs
had.

So, while this change looks like over-fitting (also to me), we're really
trying to make test previous fix more specific.

Generally looking at that section of the planner, it feels like the intro
of general refs would be a good moment to nuke and start over: the way
that refs-with-vars are put into the ruletrie seems like the root cause of
our trouble here.

Fixes open-policy-agent#5964.

Signed-off-by: Stephan Renatus <stephan@styra.com>
srenatus added a commit to srenatus/opa that referenced this issue Jun 3, 2023
So the check introduced before was too broad: it aborted optimizations
at the wrong spot -- the resulting plan didn't add up: path lengths used
in CallDynamicStmt didn't match path lengths that the planned funcs
had.

So, while this change looks like over-fitting (also to me), we're really
trying to make test previous fix more specific.

Generally looking at that section of the planner, it feels like the intro
of general refs would be a good moment to nuke and start over: the way
that refs-with-vars are put into the ruletrie seems like the root cause of
our trouble here.

Fixes open-policy-agent#5964.

Signed-off-by: Stephan Renatus <stephan@styra.com>
srenatus added a commit that referenced this issue Jun 3, 2023
So the check introduced before was too broad: it aborted optimizations
at the wrong spot -- the resulting plan didn't add up: path lengths used
in CallDynamicStmt didn't match path lengths that the planned funcs
had.

So, while this change looks like over-fitting (also to me), we're really
trying to make test previous fix more specific.

Generally looking at that section of the planner, it feels like the intro
of general refs would be a good moment to nuke and start over: the way
that refs-with-vars are put into the ruletrie seems like the root cause of
our trouble here.

Fixes #5964.

Signed-off-by: Stephan Renatus <stephan@styra.com>
ashutosh-narkar pushed a commit to ashutosh-narkar/opa that referenced this issue Jun 5, 2023
…cy-agent#5965)

So the check introduced before was too broad: it aborted optimizations
at the wrong spot -- the resulting plan didn't add up: path lengths used
in CallDynamicStmt didn't match path lengths that the planned funcs
had.

So, while this change looks like over-fitting (also to me), we're really
trying to make test previous fix more specific.

Generally looking at that section of the planner, it feels like the intro
of general refs would be a good moment to nuke and start over: the way
that refs-with-vars are put into the ruletrie seems like the root cause of
our trouble here.

Fixes open-policy-agent#5964.

Signed-off-by: Stephan Renatus <stephan@styra.com>
(cherry picked from commit a808056)
ashutosh-narkar pushed a commit that referenced this issue Jun 6, 2023
So the check introduced before was too broad: it aborted optimizations
at the wrong spot -- the resulting plan didn't add up: path lengths used
in CallDynamicStmt didn't match path lengths that the planned funcs
had.

So, while this change looks like over-fitting (also to me), we're really
trying to make test previous fix more specific.

Generally looking at that section of the planner, it feels like the intro
of general refs would be a good moment to nuke and start over: the way
that refs-with-vars are put into the ruletrie seems like the root cause of
our trouble here.

Fixes #5964.

Signed-off-by: Stephan Renatus <stephan@styra.com>
(cherry picked from commit a808056)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant