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.

Sign up for GitHub

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

Jump to bottom

planner: address ref head issue, don't optimize where it's incorrect #7439

Merged
merged 1 commit into from
Mar 12, 2025
Merged

planner: address ref head issue, don't optimize where it's incorrect #7439

merged 1 commit into from
Mar 12, 2025
+72 −0

Conversation

srenatus
Copy link
Contributor

@srenatus srenatus commented Mar 12, 2025 •
edited
Loading

When planning rules like these:

package authz

p.allow[action][resource] if { action := "list"; resource := "fruit" }

p.unrelated.eat.veggies if true

resp := p[input.rule][input.action][input.resource]

we ended up with a broken CallDynamic statement. Since the first ref rule is planned as g0.data.authz.allow and builds an object return value, and the second rule is planned as
g0.data.authz.allow.eat.veggies with a boolean return value, we cannot dynamically dispatch their calls.

% echo '{"rule": "allow", "action": "list", "resource": "fruit"}' | opa eval -I -fpretty  -d 7399.rego data.authz.resp
true
% echo '{"rule": "allow", "action": "list", "resource": "fruit"}' | opa eval -twasm -I -fpretty  -d 7399.rego data.authz.resp
undefined

With this change, the previously exising "unbalanced ruletrie" check now also hits before reaching the end of the ref. It'll catch this situation and avoid optimizing the dispatch. We'll end up with a longer, less efficient, but correct plan.

plans before and after

BEFORE:

compile.go:1271: minimum compatible version: 1.0.0
planner.go:2453: ref data.authz.resp: all nodes have 0 relevant children, break
planner.go:2488: no optimization of data.authz.resp: no vars seen before trie descend encountered no children
planner.go:2453: ../scratch/7399.rego:9: ref data.authz.p[__local3__][__local4__][__local5__]: all nodes have 0 relevant children, break
*ir.Policy Policy
| *ir.Static Static (7 strings, 1 files)
| | *ir.StringConst &{Value:result}
| | *ir.StringConst &{Value:rule}
| | *ir.StringConst &{Value:action}
| | *ir.StringConst &{Value:resource}
| | *ir.StringConst &{Value:g0}
| | *ir.StringConst &{Value:authz}
| | *ir.StringConst &{Value:p}
| | *ir.StringConst &{Value:../scratch/7399.rego}
| *ir.Plans &{Plans:[Plan authz/resp (1 blocks)]}
| | *ir.Plan Plan authz/resp (1 blocks)
| | | *ir.Block Block (5 statements)
| | | | *ir.CallStmt &{Func:g0.data.authz.resp Args:[{Value:Local<0>} {Value:Local<1>}] Result:Local<2> Location:{File:0 Col:0 Row:0 file: text:}}
| | | | *ir.AssignVarStmt &{Source:{Value:Local<2>} Target:Local<3> Location:{File:0 Col:0 Row:0 file: text:}}
| | | | *ir.MakeObjectStmt &{Target:Local<4> Location:{File:0 Col:0 Row:0 file: text:}}
| | | | *ir.ObjectInsertStmt &{Key:{Value:String<0>} Value:{Value:Local<3>} Object:Local<4> Location:{File:0 Col:0 Row:0 file: text:}}
| | | | *ir.ResultSetAddStmt &{Value:Local<4> Location:{File:0 Col:0 Row:0 file: text:}}
| *ir.Funcs Funcs (2 funcs)
| | *ir.Func g0.data.authz.p.unrelated.eat.veggies (2 params: [Local<0> Local<1>], 3 blocks, path: [g0 authz p unrelated eat veggies])
| | | *ir.Block Block (2 statements)
| | | | *ir.ResetLocalStmt &{Target:Local<3> Location:{File:0 Col:1 Row:7 file:../scratch/7399.rego text:p.unrelated.eat.veggies}}
| | | | *ir.AssignVarOnceStmt &{Source:{Value:Bool<true>} Target:Local<3> Location:{File:0 Col:1 Row:7 file:../scratch/7399.rego text:p.unrelated.eat.veggies}}
| | | *ir.Block Block (2 statements)
| | | | *ir.IsDefinedStmt &{Source:Local<3> Location:{File:0 Col:1 Row:7 file:../scratch/7399.rego text:p.unrelated.eat.veggies}}
| | | | *ir.AssignVarOnceStmt &{Source:{Value:Local<3>} Target:Local<2> Location:{File:0 Col:1 Row:7 file:../scratch/7399.rego text:p.unrelated.eat.veggies}}
| | | *ir.Block Block (1 statements)
| | | | *ir.ReturnLocalStmt &{Source:Local<2> Location:{File:0 Col:1 Row:7 file:../scratch/7399.rego text:p.unrelated.eat.veggies}}
| | *ir.Func g0.data.authz.resp (2 params: [Local<0> Local<1>], 3 blocks, path: [g0 authz resp])
| | | *ir.Block Block (8 statements)
| | | | *ir.ResetLocalStmt &{Target:Local<3> Location:{File:0 Col:1 Row:9 file:../scratch/7399.rego text:resp := p[input.rule][input.action][input.resource]}}
| | | | *ir.DotStmt &{Source:{Value:Local<0>} Key:{Value:String<1>} Target:Local<4> Location:{File:0 Col:11 Row:9 file:../scratch/7399.rego text:input.rule}}
| | | | *ir.AssignVarStmt &{Source:{Value:Local<4>} Target:Local<5> Location:{File:0 Col:11 Row:9 file:../scratch/7399.rego text:input.rule}}
| | | | *ir.DotStmt &{Source:{Value:Local<0>} Key:{Value:String<2>} Target:Local<6> Location:{File:0 Col:23 Row:9 file:../scratch/7399.rego text:input.action}}
| | | | *ir.AssignVarStmt &{Source:{Value:Local<6>} Target:Local<7> Location:{File:0 Col:23 Row:9 file:../scratch/7399.rego text:input.action}}
| | | | *ir.DotStmt &{Source:{Value:Local<0>} Key:{Value:String<3>} Target:Local<8> Location:{File:0 Col:37 Row:9 file:../scratch/7399.rego text:input.resource}}
| | | | *ir.AssignVarStmt &{Source:{Value:Local<8>} Target:Local<9> Location:{File:0 Col:37 Row:9 file:../scratch/7399.rego text:input.resource}}
| | | | *ir.BlockStmt BlockStmt (1 blocks) &{0 9 9 ../scratch/7399.rego p[input.rule][input.action][input.resource]}
| | | | | *ir.Block Block (3 statements)
| | | | | | *ir.BlockStmt BlockStmt (1 blocks) &{0 0 0  }
| | | | | | | *ir.Block Block (2 statements)
| | | | | | | | *ir.BlockStmt BlockStmt (2 blocks) &{0 0 0  }
| | | | | | | | | *ir.Block Block (2 statements)
| | | | | | | | | | *ir.CallDynamicStmt &{Args:[Local<0> Local<1>] Result:Local<10> Path:[{Value:String<4>} {Value:String<5>} {Value:String<6>} {Value:Local<5>} {Value:Local<7>} {Value:Local<9>}] Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | | | *ir.BreakStmt &{Index:1 Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | | *ir.Block Block (7 statements)
| | | | | | | | | | *ir.DotStmt &{Source:{Value:Local<1>} Key:{Value:String<5>} Target:Local<11> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | | | *ir.DotStmt &{Source:{Value:Local<11>} Key:{Value:String<6>} Target:Local<12> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | | | *ir.DotStmt &{Source:{Value:Local<12>} Key:{Value:Local<5>} Target:Local<13> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | | | *ir.DotStmt &{Source:{Value:Local<13>} Key:{Value:Local<7>} Target:Local<14> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | | | *ir.DotStmt &{Source:{Value:Local<14>} Key:{Value:Local<9>} Target:Local<15> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | | | *ir.AssignVarStmt &{Source:{Value:Local<15>} Target:Local<10> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | | | *ir.BreakStmt &{Index:1 Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | *ir.BreakStmt &{Index:2 Location:{File:0 Col:0 Row:0 file: text:}}
| | | | | | *ir.AssignVarStmt &{Source:{Value:Local<10>} Target:Local<16> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | *ir.AssignVarOnceStmt &{Source:{Value:Local<16>} Target:Local<3> Location:{File:0 Col:1 Row:9 file:../scratch/7399.rego text:resp := p[input.rule][input.action][input.resource]}}
| | | *ir.Block Block (2 statements)
| | | | *ir.IsDefinedStmt &{Source:Local<3> Location:{File:0 Col:1 Row:9 file:../scratch/7399.rego text:resp := p[input.rule][input.action][input.resource]}}
| | | | *ir.AssignVarOnceStmt &{Source:{Value:Local<3>} Target:Local<2> Location:{File:0 Col:1 Row:9 file:../scratch/7399.rego text:resp := p[input.rule][input.action][input.resource]}}
| | | *ir.Block Block (1 statements)
| | | | *ir.ReturnLocalStmt &{Source:Local<2> Location:{File:0 Col:1 Row:9 file:../scratch/7399.rego text:resp := p[input.rule][input.action][input.resource]}}

Note: Only two funcs are planned: "g0.data.authz.p.unrelated.eat.veggies" and "g0.data.authz.resp", the "data.authz.allow[..]" rule is missing

AFTER:

compile.go:1271: minimum compatible version: 1.0.0
planner.go:2463: ref data.authz.resp: all nodes have 0 relevant children, break
planner.go:2498: no optimization of data.authz.resp: no vars seen before trie descend encountered no children
planner.go:2420: ../scratch/7399.rego:9: no optimization of data.authz.p[__local3__][__local4__][__local5__]: node with rules ([p.allow[__local0__][__local1__]])
*ir.Policy Policy
| *ir.Static Static (12 strings, 1 files)
| | *ir.StringConst &{Value:result}
| | *ir.StringConst &{Value:rule}
| | *ir.StringConst &{Value:action}
| | *ir.StringConst &{Value:resource}
| | *ir.StringConst &{Value:allow}
| | *ir.StringConst &{Value:list}
| | *ir.StringConst &{Value:fruit}
| | *ir.StringConst &{Value:unrelated}
| | *ir.StringConst &{Value:eat}
| | *ir.StringConst &{Value:veggies}
| | *ir.StringConst &{Value:authz}
| | *ir.StringConst &{Value:p}
| | *ir.StringConst &{Value:../scratch/7399.rego}
| *ir.Plans &{Plans:[Plan authz/resp (1 blocks)]}
| | *ir.Plan Plan authz/resp (1 blocks)
| | | *ir.Block Block (5 statements)
| | | | *ir.CallStmt &{Func:g0.data.authz.resp Args:[{Value:Local<0>} {Value:Local<1>}] Result:Local<2> Location:{File:0 Col:0 Row:0 file: text:}}
| | | | *ir.AssignVarStmt &{Source:{Value:Local<2>} Target:Local<3> Location:{File:0 Col:0 Row:0 file: text:}}
| | | | *ir.MakeObjectStmt &{Target:Local<4> Location:{File:0 Col:0 Row:0 file: text:}}
| | | | *ir.ObjectInsertStmt &{Key:{Value:String<0>} Value:{Value:Local<3>} Object:Local<4> Location:{File:0 Col:0 Row:0 file: text:}}
| | | | *ir.ResultSetAddStmt &{Value:Local<4> Location:{File:0 Col:0 Row:0 file: text:}}
| *ir.Funcs Funcs (3 funcs)
| | *ir.Func g0.data.authz.p.allow (2 params: [Local<0> Local<1>], 3 blocks, path: [g0 authz p allow])
| | | *ir.Block Block (1 statements)
| | | | *ir.MakeObjectStmt &{Target:Local<2> Location:{File:0 Col:1 Row:3 file:../scratch/7399.rego text:p.allow[action][resource]}}
| | | *ir.Block Block (6 statements)
| | | | *ir.ResetLocalStmt &{Target:Local<3> Location:{File:0 Col:1 Row:3 file:../scratch/7399.rego text:p.allow[action][resource]}}
| | | | *ir.AssignVarStmt &{Source:{Value:String<5>} Target:Local<4> Location:{File:0 Col:32 Row:3 file:../scratch/7399.rego text:action := "list"}}
| | | | *ir.AssignVarStmt &{Source:{Value:String<6>} Target:Local<5> Location:{File:0 Col:50 Row:3 file:../scratch/7399.rego text:resource := "fruit"}}
| | | | *ir.BlockStmt BlockStmt (1 blocks) &{0 1 3 ../scratch/7399.rego p.allow[action][resource]}
| | | | | *ir.Block Block (2 statements)
| | | | | | *ir.BlockStmt BlockStmt (1 blocks) &{0 0 0  }
| | | | | | | *ir.Block Block (2 statements)
| | | | | | | | *ir.DotStmt &{Source:{Value:Local<2>} Key:{Value:Local<4>} Target:Local<6> Location:{File:0 Col:1 Row:3 file:../scratch/7399.rego text:p.allow[action][resource]}}
| | | | | | | | *ir.BreakStmt &{Index:1 Location:{File:0 Col:1 Row:3 file:../scratch/7399.rego text:p.allow[action][resource]}}
| | | | | | *ir.MakeObjectStmt &{Target:Local<6> Location:{File:0 Col:0 Row:0 file: text:}}
| | | | *ir.ObjectInsertOnceStmt &{Key:{Value:Local<5>} Value:{Value:Bool<true>} Object:Local<6> Location:{File:0 Col:1 Row:3 file:../scratch/7399.rego text:p.allow[action][resource]}}
| | | | *ir.ObjectInsertStmt &{Key:{Value:Local<4>} Value:{Value:Local<6>} Object:Local<2> Location:{File:0 Col:1 Row:3 file:../scratch/7399.rego text:p.allow[action][resource]}}
| | | *ir.Block Block (1 statements)
| | | | *ir.ReturnLocalStmt &{Source:Local<2> Location:{File:0 Col:1 Row:3 file:../scratch/7399.rego text:p.allow[action][resource]}}
| | *ir.Func g0.data.authz.p.unrelated.eat.veggies (2 params: [Local<0> Local<1>], 3 blocks, path: [g0 authz p unrelated eat veggies])
| | | *ir.Block Block (2 statements)
| | | | *ir.ResetLocalStmt &{Target:Local<3> Location:{File:0 Col:1 Row:7 file:../scratch/7399.rego text:p.unrelated.eat.veggies}}
| | | | *ir.AssignVarOnceStmt &{Source:{Value:Bool<true>} Target:Local<3> Location:{File:0 Col:1 Row:7 file:../scratch/7399.rego text:p.unrelated.eat.veggies}}
| | | *ir.Block Block (2 statements)
| | | | *ir.IsDefinedStmt &{Source:Local<3> Location:{File:0 Col:1 Row:7 file:../scratch/7399.rego text:p.unrelated.eat.veggies}}
| | | | *ir.AssignVarOnceStmt &{Source:{Value:Local<3>} Target:Local<2> Location:{File:0 Col:1 Row:7 file:../scratch/7399.rego text:p.unrelated.eat.veggies}}
| | | *ir.Block Block (1 statements)
| | | | *ir.ReturnLocalStmt &{Source:Local<2> Location:{File:0 Col:1 Row:7 file:../scratch/7399.rego text:p.unrelated.eat.veggies}}
| | *ir.Func g0.data.authz.resp (2 params: [Local<0> Local<1>], 3 blocks, path: [g0 authz resp])
| | | *ir.Block Block (14 statements)
| | | | *ir.ResetLocalStmt &{Target:Local<3> Location:{File:0 Col:1 Row:9 file:../scratch/7399.rego text:resp := p[input.rule][input.action][input.resource]}}
| | | | *ir.DotStmt &{Source:{Value:Local<0>} Key:{Value:String<1>} Target:Local<4> Location:{File:0 Col:11 Row:9 file:../scratch/7399.rego text:input.rule}}
| | | | *ir.AssignVarStmt &{Source:{Value:Local<4>} Target:Local<5> Location:{File:0 Col:11 Row:9 file:../scratch/7399.rego text:input.rule}}
| | | | *ir.DotStmt &{Source:{Value:Local<0>} Key:{Value:String<2>} Target:Local<6> Location:{File:0 Col:23 Row:9 file:../scratch/7399.rego text:input.action}}
| | | | *ir.AssignVarStmt &{Source:{Value:Local<6>} Target:Local<7> Location:{File:0 Col:23 Row:9 file:../scratch/7399.rego text:input.action}}
| | | | *ir.DotStmt &{Source:{Value:Local<0>} Key:{Value:String<3>} Target:Local<8> Location:{File:0 Col:37 Row:9 file:../scratch/7399.rego text:input.resource}}
| | | | *ir.AssignVarStmt &{Source:{Value:Local<8>} Target:Local<9> Location:{File:0 Col:37 Row:9 file:../scratch/7399.rego text:input.resource}}
| | | | *ir.BlockStmt BlockStmt (2 blocks) &{0 9 9 ../scratch/7399.rego p[input.rule][input.action][input.resource]}
| | | | | *ir.Block Block (6 statements)
| | | | | | *ir.EqualStmt &{A:{Value:String<4>} B:{Value:Local<5>} Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | *ir.CallStmt &{Func:g0.data.authz.p.allow Args:[{Value:Local<0>} {Value:Local<1>}] Result:Local<10> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | *ir.DotStmt &{Source:{Value:Local<10>} Key:{Value:Local<7>} Target:Local<11> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | *ir.DotStmt &{Source:{Value:Local<11>} Key:{Value:Local<9>} Target:Local<12> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | *ir.AssignVarStmt &{Source:{Value:Local<12>} Target:Local<13> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | *ir.AssignVarOnceStmt &{Source:{Value:Local<13>} Target:Local<3> Location:{File:0 Col:1 Row:9 file:../scratch/7399.rego text:resp := p[input.rule][input.action][input.resource]}}
| | | | | *ir.Block Block (8 statements)
| | | | | | *ir.EqualStmt &{A:{Value:String<7>} B:{Value:Local<5>} Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | *ir.BlockStmt BlockStmt (1 blocks) &{0 9 9 ../scratch/7399.rego p[input.rule][input.action][input.resource]}
| | | | | | | *ir.Block Block (9 statements)
| | | | | | | | *ir.EqualStmt &{A:{Value:String<8>} B:{Value:Local<7>} Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | *ir.BlockStmt BlockStmt (1 blocks) &{0 9 9 ../scratch/7399.rego p[input.rule][input.action][input.resource]}
| | | | | | | | | *ir.Block Block (4 statements)
| | | | | | | | | | *ir.EqualStmt &{A:{Value:String<9>} B:{Value:Local<9>} Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | | | *ir.CallStmt &{Func:g0.data.authz.p.unrelated.eat.veggies Args:[{Value:Local<0>} {Value:Local<1>}] Result:Local<14> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | | | *ir.AssignVarStmt &{Source:{Value:Local<14>} Target:Local<15> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | | | *ir.AssignVarOnceStmt &{Source:{Value:Local<15>} Target:Local<3> Location:{File:0 Col:1 Row:9 file:../scratch/7399.rego text:resp := p[input.rule][input.action][input.resource]}}
| | | | | | | | *ir.MakeSetStmt &{Target:Local<16> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | *ir.SetAddStmt &{Value:{Value:String<9>} Set:Local<16> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | *ir.DotStmt &{Source:{Value:Local<1>} Key:{Value:String<10>} Target:Local<17> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | *ir.DotStmt &{Source:{Value:Local<17>} Key:{Value:String<11>} Target:Local<18> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | *ir.DotStmt &{Source:{Value:Local<18>} Key:{Value:String<7>} Target:Local<19> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | *ir.DotStmt &{Source:{Value:Local<19>} Key:{Value:String<8>} Target:Local<20> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | *ir.ScanStmt &{Source:Local<20> Key:Local<21> Value:Local<22> Block:Block (5 statements) Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | | *ir.Block Block (5 statements)
| | | | | | | | | | *ir.EqualStmt &{A:{Value:Local<21>} B:{Value:Local<9>} Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | | | *ir.NotStmt &{Block:Block (1 statements) Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | | | | *ir.Block Block (1 statements)
| | | | | | | | | | | | *ir.DotStmt &{Source:{Value:Local<16>} Key:{Value:Local<21>} Target:Local<23> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | | | *ir.BlockStmt BlockStmt (1 blocks) &{0 9 9 ../scratch/7399.rego p[input.rule][input.action][input.resource]}
| | | | | | | | | | | *ir.Block Block (2 statements)
| | | | | | | | | | | | *ir.BlockStmt BlockStmt (1 blocks) &{0 9 9 ../scratch/7399.rego p[input.rule][input.action][input.resource]}
| | | | | | | | | | | | | *ir.Block Block (1 statements)
| | | | | | | | | | | | | | *ir.BreakStmt &{Index:1 Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | | | | | *ir.BreakStmt &{Index:1 Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | | | *ir.AssignVarStmt &{Source:{Value:Local<22>} Target:Local<26> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | | | *ir.AssignVarOnceStmt &{Source:{Value:Local<26>} Target:Local<3> Location:{File:0 Col:1 Row:9 file:../scratch/7399.rego text:resp := p[input.rule][input.action][input.resource]}}
| | | | | | *ir.MakeSetStmt &{Target:Local<27> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | *ir.SetAddStmt &{Value:{Value:String<8>} Set:Local<27> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | *ir.DotStmt &{Source:{Value:Local<1>} Key:{Value:String<10>} Target:Local<28> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | *ir.DotStmt &{Source:{Value:Local<28>} Key:{Value:String<11>} Target:Local<29> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | *ir.DotStmt &{Source:{Value:Local<29>} Key:{Value:String<7>} Target:Local<30> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | *ir.ScanStmt &{Source:Local<30> Key:Local<31> Value:Local<32> Block:Block (6 statements) Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | *ir.Block Block (6 statements)
| | | | | | | | *ir.EqualStmt &{A:{Value:Local<31>} B:{Value:Local<7>} Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | *ir.NotStmt &{Block:Block (1 statements) Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | | *ir.Block Block (1 statements)
| | | | | | | | | | *ir.DotStmt &{Source:{Value:Local<27>} Key:{Value:Local<31>} Target:Local<33> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | *ir.DotStmt &{Source:{Value:Local<32>} Key:{Value:Local<9>} Target:Local<34> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | *ir.BlockStmt BlockStmt (1 blocks) &{0 9 9 ../scratch/7399.rego p[input.rule][input.action][input.resource]}
| | | | | | | | | *ir.Block Block (2 statements)
| | | | | | | | | | *ir.BlockStmt BlockStmt (1 blocks) &{0 9 9 ../scratch/7399.rego p[input.rule][input.action][input.resource]}
| | | | | | | | | | | *ir.Block Block (1 statements)
| | | | | | | | | | | | *ir.BreakStmt &{Index:1 Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | | | *ir.BreakStmt &{Index:1 Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | *ir.AssignVarStmt &{Source:{Value:Local<34>} Target:Local<37> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | *ir.AssignVarOnceStmt &{Source:{Value:Local<37>} Target:Local<3> Location:{File:0 Col:1 Row:9 file:../scratch/7399.rego text:resp := p[input.rule][input.action][input.resource]}}
| | | | *ir.MakeSetStmt &{Target:Local<38> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | *ir.SetAddStmt &{Value:{Value:String<4>} Set:Local<38> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | *ir.SetAddStmt &{Value:{Value:String<7>} Set:Local<38> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | *ir.DotStmt &{Source:{Value:Local<1>} Key:{Value:String<10>} Target:Local<39> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | *ir.DotStmt &{Source:{Value:Local<39>} Key:{Value:String<11>} Target:Local<40> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | *ir.ScanStmt &{Source:Local<40> Key:Local<41> Value:Local<42> Block:Block (7 statements) Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | *ir.Block Block (7 statements)
| | | | | | *ir.EqualStmt &{A:{Value:Local<41>} B:{Value:Local<5>} Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | *ir.NotStmt &{Block:Block (1 statements) Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | *ir.Block Block (1 statements)
| | | | | | | | *ir.DotStmt &{Source:{Value:Local<38>} Key:{Value:Local<41>} Target:Local<43> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | *ir.DotStmt &{Source:{Value:Local<42>} Key:{Value:Local<7>} Target:Local<44> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | *ir.DotStmt &{Source:{Value:Local<44>} Key:{Value:Local<9>} Target:Local<45> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | *ir.BlockStmt BlockStmt (1 blocks) &{0 9 9 ../scratch/7399.rego p[input.rule][input.action][input.resource]}
| | | | | | | *ir.Block Block (2 statements)
| | | | | | | | *ir.BlockStmt BlockStmt (1 blocks) &{0 9 9 ../scratch/7399.rego p[input.rule][input.action][input.resource]}
| | | | | | | | | *ir.Block Block (1 statements)
| | | | | | | | | | *ir.BreakStmt &{Index:1 Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | | | *ir.BreakStmt &{Index:1 Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | *ir.AssignVarStmt &{Source:{Value:Local<45>} Target:Local<48> Location:{File:0 Col:9 Row:9 file:../scratch/7399.rego text:p[input.rule][input.action][input.resource]}}
| | | | | | *ir.AssignVarOnceStmt &{Source:{Value:Local<48>} Target:Local<3> Location:{File:0 Col:1 Row:9 file:../scratch/7399.rego text:resp := p[input.rule][input.action][input.resource]}}
| | | *ir.Block Block (2 statements)
| | | | *ir.IsDefinedStmt &{Source:Local<3> Location:{File:0 Col:1 Row:9 file:../scratch/7399.rego text:resp := p[input.rule][input.action][input.resource]}}
| | | | *ir.AssignVarOnceStmt &{Source:{Value:Local<3>} Target:Local<2> Location:{File:0 Col:1 Row:9 file:../scratch/7399.rego text:resp := p[input.rule][input.action][input.resource]}}
| | | *ir.Block Block (1 statements)
| | | | *ir.ReturnLocalStmt &{Source:Local<2> Location:{File:0 Col:1 Row:9 file:../scratch/7399.rego text:resp := p[input.rule][input.action][input.resource]}}

Note:

  1. a third function is planned, "g0.data.authz.p.allow"
  2. "g0.data.authz.resp" no longer uses CallDynamic, but the non-optimized IR constructions for calling into every possibly relevant function. Inefficient, but correct.

@srenatus srenatus force-pushed the push-sspwnvsmoywt branch from 6a92d32 to 45bea49 Compare March 12, 2025 11:04
srenatus
srenatus commented Mar 12, 2025
View reviewed changes
internal/planner/planner.go Outdated
refs := refsOfRules(node.Rules())
p.debugf("no optimization of %s: node with rules (%v)", ref, refs)
return dont()
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're moving a part of the "unbalanced rule trie" check up into the loop. Previously, we hadn't checked node.Rules() at all, so the rule of p.allow[action][resource] was dropped when we passed "action" -- the rest of the rule isn't part of the ruletrie, it's part of the rule body.

Now, we'll check when combing through the rule trie if some such ref rule is present, or any shorter-than expected rule.

Technically, even a rule like

p.allow := x if {
  x := { "list": { "fruit": true }}
}

cannot be neglected here.

@srenatus srenatus force-pushed the push-sspwnvsmoywt branch 3 times, most recently from ff15c2a to b8e03c1 Compare March 12, 2025 12:29
@srenatus srenatus marked this pull request as ready for review March 12, 2025 12:42
johanfylling
johanfylling approved these changes Mar 12, 2025
View reviewed changes
Copy link
Contributor

@johanfylling johanfylling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Makes sense to abort when rules are found along the ruletrie.

@srenatus srenatus force-pushed the push-sspwnvsmoywt branch from b8e03c1 to a909dbb Compare March 12, 2025 19:04
@srenatus
planner: address ref head issue
0dfb926 
When planning rules like these:

```
package authz

allow[action][resource] if { action := "list"; "resource": "fruit"}

allow.eat.veggies if true
```

and referencing them in a ref like

```
resp := allow[input.action][input.resource]
```

we ended up with a broken CallDynamic statement. Since the first ref
rule is planned as `g0.data.authz.allow` and builds an object return
value, and the second rule is planned as
`g0.data.authz.allow.eat.veggies` with a boolean return value, we cannot
dynamically dispatch their calls.

With this change, the previously exising "unbalanced ruletrie" check now
also hits before reaching the end of the ref. It'll catch this situation
and avoid optimizing the dispatch. We'll end up with a longer, less
efficient, but correct plan.

Signed-off-by: Stephan Renatus <[email protected]>
@srenatus srenatus force-pushed the push-sspwnvsmoywt branch from a909dbb to 0dfb926 Compare March 12, 2025 19:06
@srenatus srenatus enabled auto-merge (squash) March 12, 2025 19:09
@srenatus srenatus merged commit 7049966 into open-policy-agent:main Mar 12, 2025
28 checks passed
@srenatus srenatus deleted the push-sspwnvsmoywt branch March 12, 2025 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johanfylling johanfylling johanfylling approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@srenatus @johanfylling