From 90e24b103d1083b72b9862bc4605eb12c9a2e3a9 Mon Sep 17 00:00:00 2001 From: Stephan Renatus Date: Fri, 8 Oct 2021 10:45:34 +0200 Subject: [PATCH 1/3] rego/PrepareForEval: don't compile wasm if there's no engine Signed-off-by: Stephan Renatus --- rego/rego.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/rego/rego.go b/rego/rego.go index 6601b42fcd..35d77e6726 100644 --- a/rego/rego.go +++ b/rego/rego.go @@ -1445,22 +1445,22 @@ func (r *Rego) PrepareForEval(ctx context.Context, opts ...PrepareOption) (Prepa queries := []ast.Body{r.compiledQueries[evalQueryType].query} - // nolint: staticcheck // SA4006 false positive - cr, err := r.compileWasm(modules, queries, evalQueryType) + e, err := opa.LookupEngine(targetWasm) if err != nil { - _ = txnClose(ctx, err) // Ignore error return PreparedEvalQuery{}, err } // nolint: staticcheck // SA4006 false positive - data, err := r.store.Read(ctx, r.txn, storage.Path{}) + cr, err := r.compileWasm(modules, queries, evalQueryType) if err != nil { _ = txnClose(ctx, err) // Ignore error return PreparedEvalQuery{}, err } - e, err := opa.LookupEngine(targetWasm) + // nolint: staticcheck // SA4006 false positive + data, err := r.store.Read(ctx, r.txn, storage.Path{}) if err != nil { + _ = txnClose(ctx, err) // Ignore error return PreparedEvalQuery{}, err } From a312faebc99f4a9450e18c2e1e513d11178304c5 Mon Sep 17 00:00:00 2001 From: Stephan Renatus Date: Fri, 8 Oct 2021 12:02:12 +0200 Subject: [PATCH 2/3] ast/compile: include hidden nodes in recursion check Before, this package would slip through: package system.main allow { data[input] } and wreck havoc either in evaluation (topdown) or the wasm compilation. Now, it'll be caught: open-policy-agent/opa % opa eval -fpretty -d r.rego 'data.system.main' 1 error occurred: r.rego:3: rego_recursion_error: rule allow is recursive: allow -> allow Signed-off-by: Stephan Renatus --- ast/compile.go | 14 +++----------- ast/compile_test.go | 41 +++++++++++++++++++++++++++-------------- 2 files changed, 30 insertions(+), 25 deletions(-) diff --git a/ast/compile.go b/ast/compile.go index 18b3b5b2bc..8e616b86bb 100644 --- a/ast/compile.go +++ b/ast/compile.go @@ -571,7 +571,7 @@ func (c *Compiler) GetRulesDynamic(ref Ref) (rules []*Rule) { // under this "prefix". node.DepthFirst(func(descendant *TreeNode) bool { insertRules(set, descendant.Values) - return descendant.Hide + return false }) } else if i == 0 || IsConstant(ref[i].Value) { // The head of the ref is always grounded. In case another part of the @@ -591,9 +591,6 @@ func (c *Compiler) GetRulesDynamic(ref Ref) (rules []*Rule) { // This part of the ref is a dynamic term. We can't know what it refers // to and will just need to try all of the children. for _, child := range node.Children { - if child.Hide { - continue - } insertRules(set, child.Values) walk(child, i+1) } @@ -707,7 +704,7 @@ func (c *Compiler) checkRecursion() { func (c *Compiler) checkSelfPath(loc *Location, eq func(a, b util.T) bool, a, b util.T) { tr := NewGraphTraversal(c.Graph) if p := util.DFSPath(tr, eq, a, b); len(p) > 0 { - n := []string{} + n := make([]string, 0, len(p)) for _, x := range p { n = append(n, astNodeToString(x)) } @@ -716,12 +713,7 @@ func (c *Compiler) checkSelfPath(loc *Location, eq func(a, b util.T) bool, a, b } func astNodeToString(x interface{}) string { - switch x := x.(type) { - case *Rule: - return string(x.Head.Name) - default: - panic("not reached") - } + return string(x.(*Rule).Head.Name) } // checkRuleConflicts ensures that rules definitions are not in conflict. diff --git a/ast/compile_test.go b/ast/compile_test.go index 54231e192b..83adfa907e 100644 --- a/ast/compile_test.go +++ b/ast/compile_test.go @@ -3142,24 +3142,34 @@ func TestCompilerCheckDynamicRecursion(t *testing.T) { // This test tries to circumvent the recursion check by using dynamic // references. For more background info, see // . - c := NewCompiler() - c.Modules = map[string]*Module{ - "recursion": MustParseModule(`package recursion + for note, mod := range map[string]*Module{ + "recursion": MustParseModule(` +package recursion pkg = "recursion" - foo[x] { - data[pkg]["foo"][x] -}`), - } - - compileStages(c, c.checkRecursion) + data[pkg]["foo"][x] +} +`), + "system.main": MustParseModule(` +package system.main +foo { + data[input] +} +`), + } { + t.Run(note, func(t *testing.T) { + c := NewCompiler() + c.Modules = map[string]*Module{note: mod} + compileStages(c, c.checkRecursion) - result := compilerErrsToStringSlice(c.Errors) - expected := "rego_recursion_error: rule foo is recursive: foo -> foo" + result := compilerErrsToStringSlice(c.Errors) + expected := "rego_recursion_error: rule foo is recursive: foo -> foo" - if len(result) != 1 || result[0] != expected { - t.Errorf("Expected %v but got: %v", expected, result) + if len(result) != 1 || result[0] != expected { + t.Errorf("Expected %v but got: %v", expected, result) + } + }) } } @@ -3412,6 +3422,8 @@ r1 = 1`, r2 = 2`, "mod3": `package a.b r3 = 3`, + "hidden": `package system.hidden +r4 = 4`, }) compileStages(compiler, nil) @@ -3419,6 +3431,7 @@ r3 = 3`, rule1 := compiler.Modules["mod1"].Rules[0] rule2 := compiler.Modules["mod2"].Rules[0] rule3 := compiler.Modules["mod3"].Rules[0] + rule4 := compiler.Modules["hidden"].Rules[0] tests := []struct { input string @@ -3429,7 +3442,7 @@ r3 = 3`, {"data.a.b[x].d", []*Rule{rule1, rule3}}, {"data.a.b.c", []*Rule{rule1, rule2}}, {"data.a.b.d", nil}, - {"data[x]", []*Rule{rule1, rule2, rule3}}, + {"data[x]", []*Rule{rule1, rule2, rule3, rule4}}, {"data[data.complex_computation].b[y]", []*Rule{rule1, rule2, rule3}}, {"data[x][y].c.e", []*Rule{rule2}}, {"data[x][y].r3", []*Rule{rule3}}, From bfbb7e531d2d3ad0758e34069a5fdffe0e818dbf Mon Sep 17 00:00:00 2001 From: Stephan Renatus Date: Wed, 13 Oct 2021 10:09:45 +0200 Subject: [PATCH 3/3] ast: make changes backwards-compatible Signed-off-by: Stephan Renatus --- ast/compile.go | 62 +++++++++++++++++++++++++++++++++++++++------ ast/compile_test.go | 29 ++++++++++++--------- topdown/save.go | 2 +- 3 files changed, 72 insertions(+), 21 deletions(-) diff --git a/ast/compile.go b/ast/compile.go index 8e616b86bb..e407e38fff 100644 --- a/ast/compile.go +++ b/ast/compile.go @@ -120,6 +120,14 @@ type CompilerStageDefinition struct { Stage CompilerStage } +// RulesOptions defines the options for retrieving rules by Ref from the +// compiler. +type RulesOptions struct { + // IncludeHiddenModules determines if the result contains hidden modules, + // currently only the "system" namespace, i.e. "data.system.*". + IncludeHiddenModules bool +} + // QueryContext contains contextual information for running an ad-hoc query. // // Ad-hoc queries can be run in the context of a package and imports may be @@ -539,6 +547,14 @@ func (c *Compiler) GetRules(ref Ref) (rules []*Rule) { } // GetRulesDynamic returns a slice of rules that could be referred to by a ref. +// +// Deprecated: use GetRulesDynamicWithOpts +func (c *Compiler) GetRulesDynamic(ref Ref) []*Rule { + return c.GetRulesDynamicWithOpts(ref, RulesOptions{}) +} + +// GetRulesDynamicWithOpts returns a slice of rules that could be referred to by +// a ref. // When parts of the ref are statically known, we use that information to narrow // down which rules the ref could refer to, but in the most general case this // will be an over-approximation. @@ -557,10 +573,25 @@ func (c *Compiler) GetRules(ref Ref) (rules []*Rule) { // // The following calls yield the rules on the right. // -// GetRulesDynamic("data.a[x].c[y]") => [rule1, rule2] -// GetRulesDynamic("data.a[x].c.r2") => [rule2] -// GetRulesDynamic("data.a.b[x][y]") => [rule1] -func (c *Compiler) GetRulesDynamic(ref Ref) (rules []*Rule) { +// GetRulesDynamicWithOpts("data.a[x].c[y]", opts) => [rule1, rule2] +// GetRulesDynamicWithOpts("data.a[x].c.r2", opts) => [rule2] +// GetRulesDynamicWithOpts("data.a.b[x][y]", opts) => [rule1] +// +// Using the RulesOptions parameter, the inclusion of hidden modules can be +// controlled: +// +// With +// +// package system.main +// +// r3 = 3 # rule3 +// +// We'd get this result: +// +// GetRulesDynamicWithOpts("data[x]", RulesOptions{IncludeHiddenModules: true}) => [rule1, rule2, rule3] +// +// Without the options, it would be excluded. +func (c *Compiler) GetRulesDynamicWithOpts(ref Ref, opts RulesOptions) []*Rule { node := c.RuleTree set := map[*Rule]struct{}{} @@ -571,7 +602,10 @@ func (c *Compiler) GetRulesDynamic(ref Ref) (rules []*Rule) { // under this "prefix". node.DepthFirst(func(descendant *TreeNode) bool { insertRules(set, descendant.Values) - return false + if opts.IncludeHiddenModules { + return false + } + return descendant.Hide }) } else if i == 0 || IsConstant(ref[i].Value) { // The head of the ref is always grounded. In case another part of the @@ -591,6 +625,9 @@ func (c *Compiler) GetRulesDynamic(ref Ref) (rules []*Rule) { // This part of the ref is a dynamic term. We can't know what it refers // to and will just need to try all of the children. for _, child := range node.Children { + if child.Hide && !opts.IncludeHiddenModules { + continue + } insertRules(set, child.Values) walk(child, i+1) } @@ -598,6 +635,7 @@ func (c *Compiler) GetRulesDynamic(ref Ref) (rules []*Rule) { } walk(node, 0) + rules := make([]*Rule, 0, len(set)) for rule := range set { rules = append(rules, rule) } @@ -704,7 +742,7 @@ func (c *Compiler) checkRecursion() { func (c *Compiler) checkSelfPath(loc *Location, eq func(a, b util.T) bool, a, b util.T) { tr := NewGraphTraversal(c.Graph) if p := util.DFSPath(tr, eq, a, b); len(p) > 0 { - n := make([]string, 0, len(p)) + n := []string{} for _, x := range p { n = append(n, astNodeToString(x)) } @@ -713,7 +751,12 @@ func (c *Compiler) checkSelfPath(loc *Location, eq func(a, b util.T) bool, a, b } func astNodeToString(x interface{}) string { - return string(x.(*Rule).Head.Name) + switch x := x.(type) { + case *Rule: + return string(x.Head.Name) + default: + panic("not reached") + } } // checkRuleConflicts ensures that rules definitions are not in conflict. @@ -1558,7 +1601,10 @@ func (c *Compiler) setRuleTree() { } func (c *Compiler) setGraph() { - c.Graph = NewGraph(c.Modules, c.GetRulesDynamic) + list := func(r Ref) []*Rule { + return c.GetRulesDynamicWithOpts(r, RulesOptions{IncludeHiddenModules: true}) + } + c.Graph = NewGraph(c.Modules, list) } type queryCompiler struct { diff --git a/ast/compile_test.go b/ast/compile_test.go index 83adfa907e..7a09370240 100644 --- a/ast/compile_test.go +++ b/ast/compile_test.go @@ -3434,23 +3434,28 @@ r4 = 4`, rule4 := compiler.Modules["hidden"].Rules[0] tests := []struct { - input string - expected []*Rule + input string + expected []*Rule + excludeHidden bool }{ - {"data.a.b.c.d.r1", []*Rule{rule1}}, - {"data.a.b[x]", []*Rule{rule1, rule2, rule3}}, - {"data.a.b[x].d", []*Rule{rule1, rule3}}, - {"data.a.b.c", []*Rule{rule1, rule2}}, - {"data.a.b.d", nil}, - {"data[x]", []*Rule{rule1, rule2, rule3, rule4}}, - {"data[data.complex_computation].b[y]", []*Rule{rule1, rule2, rule3}}, - {"data[x][y].c.e", []*Rule{rule2}}, - {"data[x][y].r3", []*Rule{rule3}}, + {input: "data.a.b.c.d.r1", expected: []*Rule{rule1}}, + {input: "data.a.b[x]", expected: []*Rule{rule1, rule2, rule3}}, + {input: "data.a.b[x].d", expected: []*Rule{rule1, rule3}}, + {input: "data.a.b.c", expected: []*Rule{rule1, rule2}}, + {input: "data.a.b.d"}, + {input: "data[x]", expected: []*Rule{rule1, rule2, rule3, rule4}}, + {input: "data[data.complex_computation].b[y]", expected: []*Rule{rule1, rule2, rule3}}, + {input: "data[x][y].c.e", expected: []*Rule{rule2}}, + {input: "data[x][y].r3", expected: []*Rule{rule3}}, + {input: "data[x][y]", expected: []*Rule{rule1, rule2, rule3}, excludeHidden: true}, // old behaviour of GetRulesDynamic } for _, tc := range tests { t.Run(tc.input, func(t *testing.T) { - result := compiler.GetRulesDynamic(MustParseRef(tc.input)) + result := compiler.GetRulesDynamicWithOpts( + MustParseRef(tc.input), + RulesOptions{IncludeHiddenModules: !tc.excludeHidden}, + ) if len(result) != len(tc.expected) { t.Fatalf("Expected %v but got: %v", tc.expected, result) diff --git a/topdown/save.go b/topdown/save.go index 4e898e239f..4ea644f0b8 100644 --- a/topdown/save.go +++ b/topdown/save.go @@ -347,7 +347,7 @@ func saveRequired(c *ast.Compiler, ic *inliningControl, icIgnoreInternal bool, s } else if ic.Disabled(v.ConstantPrefix(), icIgnoreInternal) { found = true } else { - for _, rule := range c.GetRulesDynamic(v) { + for _, rule := range c.GetRulesDynamicWithOpts(v, ast.RulesOptions{IncludeHiddenModules: false}) { if saveRequired(c, ic, icIgnoreInternal, ss, b, rule, true) { found = true break