Skip to content

Commit 7049966

Browse files
authored
planner: address ref head issue, don't optimize if impossible (#7439)
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.p.allow` and builds an object return value, and the second rule is planned as `g0.data.authz.p.unrelated.eat.veggies` with a boolean return value, we cannot dynamically dispatch their calls. With this change, the previously existing "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]>
1 parent fb97231 commit 7049966

File tree

3 files changed

+72
-0
lines changed

3 files changed

+72
-0
lines changed

internal/planner/planner.go

+16
Original file line numberDiff line numberDiff line change
@@ -2410,6 +2410,10 @@ outer:
24102410
opt = true
24112411
// take all children, they might match
24122412
for _, node := range nodes {
2413+
if nr := node.Rules(); len(nr) > 0 {
2414+
p.debugf("no optimization of %s: node with rules (%v)", ref, refsOfRules(nr))
2415+
return dont()
2416+
}
24132417
for _, child := range node.Children() {
24142418
if node := node.Get(child); node != nil {
24152419
nextNodes = append(nextNodes, node)
@@ -2419,6 +2423,10 @@ outer:
24192423
case ast.String:
24202424
// take all children that either match or have a var key // TODO(sr): Where's the code for the second part, having a var key?
24212425
for _, node := range nodes {
2426+
if nr := node.Rules(); len(nr) > 0 {
2427+
p.debugf("no optimization of %s: node with rules (%v)", ref, refsOfRules(nr))
2428+
return dont()
2429+
}
24222430
if node := node.Get(r); node != nil {
24232431
nextNodes = append(nextNodes, node)
24242432
}
@@ -2559,3 +2567,11 @@ func (p *Planner) isFunction(r ast.Ref) bool {
25592567
func op(v ir.Val) ir.Operand {
25602568
return ir.Operand{Value: v}
25612569
}
2570+
2571+
func refsOfRules(rs []*ast.Rule) []string {
2572+
refs := make([]string, len(rs))
2573+
for i := range rs {
2574+
refs[i] = rs[i].Head.Ref().String()
2575+
}
2576+
return refs
2577+
}

internal/planner/planner_test.go

+22
Original file line numberDiff line numberDiff line change
@@ -1101,6 +1101,28 @@ func TestOptimizeLookup(t *testing.T) {
11011101
t.Fatalf("expected %d rules in ruleset[0], got %d\n", exp, act)
11021102
}
11031103
})
1104+
1105+
t.Run("ref heads, mixed-length rules in ruletrie", func(t *testing.T) {
1106+
r0 := ast.MustParseRule("allow[x].something { x := \"show\" }")
1107+
r1 := ast.MustParseRule("allow.see.something_else { true }")
1108+
r := newRuletrie()
1109+
val := r.LookupOrInsert(ref("primary.allow"))
1110+
val.rules = append(val.rules, r0)
1111+
val = r.LookupOrInsert(ref("secondary.allow.see.something_else"))
1112+
val.rules = append(val.rules, r1)
1113+
1114+
if testing.Verbose() {
1115+
t.Logf("rules: %v", r)
1116+
}
1117+
1118+
p := planner()
1119+
p.vars.Put(ast.Var("x"), p.newLocal())
1120+
_, _, _, opt := p.optimizeLookup(r, ast.MustParseRef("data[x].allow.see.something_else"))
1121+
1122+
if exp, act := false, opt; exp != act {
1123+
t.Errorf("expected 'optimize' %v, got %v\n", exp, act)
1124+
}
1125+
})
11041126
}
11051127

11061128
func TestPlannerCallDynamic(t *testing.T) {

v1/test/cases/testdata/v1/planner-ir/test-call-dynamic.yaml

+34
Original file line numberDiff line numberDiff line change
@@ -132,3 +132,37 @@ cases:
132132
allow.other.stuff if true
133133
want_result:
134134
- x: true
135+
- note: ir/call-dynamic not used with ref heads and mixed-length rules (issue 7399)
136+
query: data.test.q = x
137+
modules:
138+
- |
139+
package test
140+
141+
p.allow[action][resource] if { action := "list"; resource := "fruit" }
142+
143+
p.unrelated.eat.veggies if true
144+
145+
q := p[input.rule][input.action][input.resource]
146+
input:
147+
rule: allow
148+
action: list
149+
resource: fruit
150+
want_result:
151+
- x: true
152+
- note: ir/call-dynamic not used with ref heads and mixed-length rules (issue 7399), second rule
153+
query: data.test.q = x
154+
modules:
155+
- |
156+
package test
157+
158+
p.allow[action][resource] if { action := "list"; resource := "fruit" }
159+
160+
p.unrelated.eat.veggies if true
161+
162+
q := p[input.rule][input.action][input.resource]
163+
input:
164+
rule: unrelated
165+
action: eat
166+
resource: veggies
167+
want_result:
168+
- x: true

0 commit comments

Comments
 (0)