From 45c292e834f8838883cd1bd4bc65d31c8e3b463d Mon Sep 17 00:00:00 2001 From: Stephan Renatus Date: Wed, 13 Jan 2021 08:54:52 +0100 Subject: [PATCH] wasm: fix "else-conflict 2" (#3031) There are two kinds of else conflicts in the e2e tests, and one had been failing in WASM (i.e., it didn't fail when it should). With this change to the planner, the underlying issue is resolved: With multiple rule bodies for one rule like this, where r1 and r2 both fail, - r1 - else1 - r2 - else2 previously, else1 and else2 checked if the output variable used for both r1 and r2 was defined, and didn't run it was. Topdown, however, evaluates both else1 and else2, and checks that they have the same outcome. The output variable checking in WASM would prohibit else2 from being run when the output variable was set by else1. Now, each rule and its else branches use an output variable different from the overall output; and an extra block writing that output variable's value into the overall output variable of that rule. The extra layering allows each else block to check its rule body's output, and not the overall rule output. Also: * planner+wasm: add nop stmt Useful for debugging planner->ir->compiler->wasm action * wasm: reuse local variable for separate function bodies+elses * wasm-e2e: map multiple wasm errors to eval_conflict_error * planner: fix functon errors with multiple returns (single) For rule bodies and their else's, the AssignVarOnceStmt would be no different from the AssignVarStmt -- since it's zeroed by the starting ResetLocalStmt. For function bodies involving ScanStmt, it does make a difference, since it the AssignVarOnceStmt ends up in a its block: | | *ir.Func g0.data.p.p (3 params: [0 1 3], 3 blocks) | | | *ir.Block Block (3 statements) | | | | *ir.ResetLocalStmt &{Target:4 Location:{Index:0 Col:5 Row:3 file:module-0.rego text:p(a) = y}} | | | | *ir.AssignVarStmt &{Source:3 Target:5 Location:{Index:0 Col:5 Row:3 file:module-0.rego text:p(a) = y}} | | | | *ir.ScanStmt &{Source:5 Key:6 Value:7 Block:Block (3 statements) Location:{Index:0 Col:7 Row:4 file:module-0.rego text:y = a[_]}} | | | | | *ir.Block Block (3 statements) | | | | | | *ir.AssignVarStmt &{Source:6 Target:8 Location:{Index:0 Col:7 Row:4 file:module-0.rego text:y = a[_]}} | | | | | | *ir.AssignVarStmt &{Source:7 Target:9 Location:{Index:0 Col:7 Row:4 file:module-0.rego text:y = a[_]}} |H|E|R|E|>| *ir.AssignVarOnceStmt &{Target:4 Source:9 Location:{Index:0 Col:5 Row:3 file:module-0.rego text:p(a) = y}} | | | *ir.Block Block (2 statements) | | | | *ir.IsDefinedStmt &{Source:4 Location:{Index:0 Col:5 Row:3 file:module-0.rego text:p(a) = y}} | | | | *ir.AssignVarOnceStmt &{Target:2 Source:4 Location:{Index:0 Col:5 Row:3 file:module-0.rego text:p(a) = y}} | | | *ir.Block Block (1 statements) | | | | *ir.ReturnLocalStmt &{Source:2 Location:{Index:0 Col:5 Row:3 file:module-0.rego text:p(a) = y}} Signed-off-by: Stephan Renatus --- internal/compiler/wasm/wasm.go | 5 + internal/ir/ir.go | 12 ++ internal/planner/planner.go | 34 ++++- internal/planner/planner_test.go | 77 ++++++++++- internal/wasm/sdk/opa/opa_test.go | 33 +++++ internal/wasm/sdk/test/e2e/exceptions.yaml | 1 - internal/wasm/sdk/test/e2e/external_test.go | 33 +++-- .../test-functionerrors-1012.yaml | 120 +----------------- .../test-functionerrors-1013.yaml | 120 +----------------- .../test-functionerrors-1014.yaml | 120 +----------------- 10 files changed, 186 insertions(+), 369 deletions(-) diff --git a/internal/compiler/wasm/wasm.go b/internal/compiler/wasm/wasm.go index 83ae97b538..f5d6e81e30 100644 --- a/internal/compiler/wasm/wasm.go +++ b/internal/compiler/wasm/wasm.go @@ -664,6 +664,8 @@ func (c *Compiler) compileBlock(block *ir.Block) ([]instruction.Instruction, err if err := c.compileScan(stmt, &instrs); err != nil { return nil, err } + case *ir.NopStmt: + instrs = append(instrs, instruction.Nop{}) case *ir.NotStmt: if err := c.compileNot(stmt, &instrs); err != nil { return nil, err @@ -776,6 +778,9 @@ func (c *Compiler) compileBlock(block *ir.Block) ([]instruction.Instruction, err instrs = append(instrs, instruction.I32Const{Value: 0}) instrs = append(instrs, instruction.I32Ne{}) instrs = append(instrs, instruction.BrIf{Index: 0}) + case *ir.ResetLocalStmt: + instrs = append(instrs, instruction.I32Const{Value: 0}) + instrs = append(instrs, instruction.SetLocal{Index: c.local(stmt.Target)}) case *ir.IsDefinedStmt: instrs = append(instrs, instruction.GetLocal{Index: c.local(stmt.Source)}) instrs = append(instrs, instruction.I32Eqz{}) diff --git a/internal/ir/ir.go b/internal/ir/ir.go index 9a79be8a81..791a915425 100644 --- a/internal/ir/ir.go +++ b/internal/ir/ir.go @@ -267,6 +267,13 @@ type AssignVarOnceStmt struct { Location } +// ResetLocalStmt resets a local variable to 0. +type ResetLocalStmt struct { + Target Local + + Location +} + // MakeStringStmt constructs a local variable that refers to a string constant. type MakeStringStmt struct { Index int @@ -476,6 +483,11 @@ type WithStmt struct { Location } +// NopStmt adds a nop instruction. Useful during development and debugging only. +type NopStmt struct { + Location +} + // ResultSetAdd adds a value into the result set returned by the query plan. type ResultSetAdd struct { Value Local diff --git a/internal/planner/planner.go b/internal/planner/planner.go index d431d69140..0d583c8f2b 100644 --- a/internal/planner/planner.go +++ b/internal/planner/planner.go @@ -177,8 +177,8 @@ func (p *Planner) planRules(rules []*ast.Rule) (string, error) { params := fn.Params[2:] - // Initialize return value for partial set/object rules. Complete docs do - // not require their return value to be initialized. + // Initialize return value for partial set/object rules. Complete document + // rules assign directly to `fn.Return`. switch rules[0].Head.DocKind() { case ast.PartialObjectDoc: fn.Blocks = append(fn.Blocks, p.blockWithStmt(&ir.MakeObjectStmt{Target: fn.Return})) @@ -186,6 +186,12 @@ func (p *Planner) planRules(rules []*ast.Rule) (string, error) { fn.Blocks = append(fn.Blocks, p.blockWithStmt(&ir.MakeSetStmt{Target: fn.Return})) } + // For complete document rules, allocate one local variable for output + // of the rule body + else branches. + // It is used to let ordered rules (else blocks) check if the previous + // rule body returned a value. + lresult := p.newLocal() + // At this point the locals for the params and return value have been // allocated. This will be the first local that can be used in each block. lnext := p.lnext @@ -208,7 +214,7 @@ func (p *Planner) planRules(rules []*ast.Rule) (string, error) { } // Ordered rules are nested inside an additional block so that execution - // can short-circuit. For unordered rules blocks can be added directly + // can short-circuit. For unordered rules, blocks can be added directly // to the function. var blocks *[]*ir.Block @@ -243,11 +249,14 @@ func (p *Planner) planRules(rules []*ast.Rule) (string, error) { if prev != nil { // Ordered rules are handled by short circuiting execution. The // plan will jump out to the extra block that was planned above. - p.appendStmt(&ir.IsUndefinedStmt{Source: fn.Return}) + p.appendStmt(&ir.IsUndefinedStmt{Source: lresult}) + } else { + // The first rule body resets the local, so it can be reused. + p.appendStmt(&ir.ResetLocalStmt{Target: lresult}) } // Complete and partial rules are treated as special cases of - // functions. If there are args, the first step is a no-op. + // functions. If there are no args, the first step is a no-op. err := p.planFuncParams(params, rule.Head.Args, 0, func() error { // Run planner on the rule body. @@ -258,7 +267,7 @@ func (p *Planner) planRules(rules []*ast.Rule) (string, error) { case ast.CompleteDoc: return p.planTerm(rule.Head.Value, func() error { p.appendStmt(&ir.AssignVarOnceStmt{ - Target: fn.Return, + Target: lresult, Source: p.ltarget, }) return nil @@ -300,6 +309,19 @@ func (p *Planner) planRules(rules []*ast.Rule) (string, error) { return "", err } } + + // rule[i] and its else-rule(s), if present, are done + if rules[i].Head.DocKind() == ast.CompleteDoc { + end := &ir.Block{} + p.appendStmtToBlock(&ir.IsDefinedStmt{Source: lresult}, end) + p.appendStmtToBlock( + &ir.AssignVarOnceStmt{ + Target: fn.Return, + Source: lresult, + }, + end) + *blocks = append(*blocks, end) + } } // Default rules execute if the return is undefined. diff --git a/internal/planner/planner_test.go b/internal/planner/planner_test.go index 80534bf567..a0a02dc0f9 100644 --- a/internal/planner/planner_test.go +++ b/internal/planner/planner_test.go @@ -224,6 +224,77 @@ func TestPlannerHelloWorld(t *testing.T) { note: "relation unify", queries: []string{`walk(input, [["foo", y], x])`}, }, + { + note: "else conflict-1", + queries: []string{`data.p.q`}, + modules: []string{ + `package p + + q { + false + } + else = true { + true + } + q = false + `, + }, + }, + { + note: "else conflict-2", + queries: []string{`data.p.q`}, + modules: []string{ + `package p + + q { + false + } + else = false { + true + } + q { + false + } + else = true { + true + }`, + }, + }, + { + note: "multiple function outputs (single)", + queries: []string{`data.p.r`}, + modules: []string{ + `package p + + p(a) = y { + y = a[_] + } + + r = y { + data.p.p([1, 2, 3], y) + } + `, + }, + }, + { + note: "multiple function outputs (multiple)", + queries: []string{`data.p.r`}, + modules: []string{ + `package p + + p(1, a) = y { + y = a + } + p(x, y) = z { + z = x + } + + r = y { + data.p.p(1, 0, y) + } + `, + }, + }, } for _, tc := range tests { @@ -261,6 +332,7 @@ func TestPlannerHelloWorld(t *testing.T) { type cmpWalker struct { needle interface{} loc string + found bool // stop comparing after first found needle } func (*cmpWalker) Before(interface{}) {} @@ -276,7 +348,8 @@ func (*cmpWalker) After(interface{}) {} // returned. This trap can be avoided by starting with a failing test, // and proceeding with caution. ;) func (f *cmpWalker) Visit(x interface{}) (ir.Visitor, error) { - if reflect.TypeOf(f.needle) == reflect.TypeOf(x) { + if !f.found && reflect.TypeOf(f.needle) == reflect.TypeOf(x) { + f.found = true expLoc := f.loc actLoc := getLocation(x) if expLoc != actLoc { @@ -402,7 +475,7 @@ p = x { &ir.AssignVarOnceStmt{}: `module-0.rego:3:9: p = {"foo": "bar"}`, }, where: func(p *ir.Policy) interface{} { - return p.Funcs.Funcs[0].Blocks[1] // default rule block + return p.Funcs.Funcs[0].Blocks[2] // default rule block }, }, { diff --git a/internal/wasm/sdk/opa/opa_test.go b/internal/wasm/sdk/opa/opa_test.go index fddc0b2f30..c798a8fc55 100644 --- a/internal/wasm/sdk/opa/opa_test.go +++ b/internal/wasm/sdk/opa/opa_test.go @@ -121,6 +121,39 @@ a = "c" { input > 2 }`, }, WantErr: "module.rego:3:1: var assignment conflict: internal error", }, + { + Description: "Runtime error/else conflict-1", + Query: `data.p.q`, + Policy: ` + q { + false + } + else = true { + true + } + q = false`, + Evals: []Eval{{}}, + WantErr: "module.rego:9:5: var assignment conflict: internal error", + }, + { + Description: "Runtime error/else conflict-2", + Query: `data.p.q`, + Policy: ` + q { + false + } + else = false { + true + } + q { + false + } + else = true { + true + }`, + Evals: []Eval{{}}, + WantErr: "module.rego:12:5: var assignment conflict: internal error", + }, // NOTE(sr): The next two test cases were used to replicate issue // https://github.com/open-policy-agent/opa/issues/2962 -- their raison d'être // is thus questionable, but it might be good to keep them around a bit. diff --git a/internal/wasm/sdk/test/e2e/exceptions.yaml b/internal/wasm/sdk/test/e2e/exceptions.yaml index 0ad98c8167..f6925e5ac0 100644 --- a/internal/wasm/sdk/test/e2e/exceptions.yaml +++ b/internal/wasm/sdk/test/e2e/exceptions.yaml @@ -1,4 +1,3 @@ # Exception Format is : "jsonpatch/set": "unexpected panic or evaluation error - https://github.com/open-policy-agent/opa/issues/2949" "jsonpatch/json_patch_tests": "unexpected panic or evaluation error - https://github.com/open-policy-agent/opa/issues/2949" -"elsekeyword/conflict-2": "expected error missing - https://github.com/open-policy-agent/opa/issues/2954" diff --git a/internal/wasm/sdk/test/e2e/external_test.go b/internal/wasm/sdk/test/e2e/external_test.go index 1d37a54353..45ff65b41f 100644 --- a/internal/wasm/sdk/test/e2e/external_test.go +++ b/internal/wasm/sdk/test/e2e/external_test.go @@ -131,19 +131,13 @@ func assert(t *testing.T, tc cases.TestCase, result *opa.Result, err error) { return } if err == nil { + if result != nil { + t.Fatalf("expected error, got result %s", result.Result) + } t.Fatal("expected error") } - // TODO: implement more specific error checking, for now log results and skip the test - if tc.WantErrorCode != nil { - t.Logf("\nExpected Code: %s\nGot Err: %s\n", *tc.WantErrorCode, err) - } - - if tc.WantError != nil { - t.Logf("\nExpected Err: %s\nGot Err: %s\n", *tc.WantError, err) - } - - t.Skip("Skipping test case: Error validation not supported") + assertErrorCode(t, *tc.WantErrorCode, err) } } @@ -204,7 +198,26 @@ func assertResultSet(t *testing.T, want []map[string]interface{}, sortBindings b if !a.Equal(b) { t.Fatalf("expected %v but got %v", a, b) } +} +func assertErrorCode(t *testing.T, expected string, actual error) { + t.Helper() + switch expected { + case "eval_conflict_error": + exps := []string{"var assignment conflict", "object insert conflict"} + found := false + for _, exp := range exps { + if strings.Contains(actual.Error(), exp) { + found = true + break + } + } + if !found { + t.Errorf("expected %q to contain one of %v", actual, exps) + } + default: + t.Errorf("unmatched error: %v (expected %s)", actual, expected) + } } func toAST(a interface{}) *ast.Term { diff --git a/test/cases/testdata/functionerrors/test-functionerrors-1012.yaml b/test/cases/testdata/functionerrors/test-functionerrors-1012.yaml index c9fabfbc6b..e4f0533f54 100644 --- a/test/cases/testdata/functionerrors/test-functionerrors-1012.yaml +++ b/test/cases/testdata/functionerrors/test-functionerrors-1012.yaml @@ -1,132 +1,16 @@ cases: - data: - a: - - 1 - - 2 - - 3 - - 4 - b: - v1: hello - v2: goodbye - c: - - x: - - true - - false - - foo - "y": - - null - - 3.14159 - z: - p: true - q: false - d: - e: - - bar - - baz - f: - - xs: - - 1 - ys: - - 2 - - xs: - - 2 - ys: - - 3 - g: - a: - - 1 - - 0 - - 0 - - 0 - b: - - 0 - - 2 - - 0 - - 0 - c: - - 0 - - 0 - - 0 - - 4 - h: - - - 1 - - 2 - - 3 - - - 2 - - 3 - - 4 - l: - - a: bob - b: -1 - c: - - 1 - - 2 - - 3 - - 4 - - a: alice - b: 1 - c: - - 2 - - 3 - - 4 - - 5 - d: null - m: [] - numbers: - - "1" - - "2" - - "3" - - "4" - strings: - bar: 2 - baz: 3 - foo: 1 - three: 3 modules: - | package test1 - p(__local0__) = y { - y = __local0__[_] + p(a) = y { + y = a[_] } r = y { data.test1.p([1, 2, 3], y) } - - | - package test2 - - p(1, __local1__) = y { - y = __local1__ - } - - p(2, __local2__) = y { - __local7__ = __local2__ + 1 - y = __local7__ - } - - r = y { - data.test2.p(3, 0, y) - } - - | - package test3 - - p(1, __local3__) = y { - y = __local3__ - } - - p(2, __local4__) = y { - __local8__ = __local4__ + 1 - y = __local8__ - } - - p(__local5__, __local6__) = z { - z = __local5__ - } - - r = y { - data.test3.p(1, 0, y) - } note: functionerrors/function output conflict single query: data.test1.r = x want_error: functions must not produce multiple outputs for same inputs diff --git a/test/cases/testdata/functionerrors/test-functionerrors-1013.yaml b/test/cases/testdata/functionerrors/test-functionerrors-1013.yaml index ce79ac8311..84c56bcca8 100644 --- a/test/cases/testdata/functionerrors/test-functionerrors-1013.yaml +++ b/test/cases/testdata/functionerrors/test-functionerrors-1013.yaml @@ -1,127 +1,15 @@ cases: - data: - a: - - 1 - - 2 - - 3 - - 4 - b: - v1: hello - v2: goodbye - c: - - x: - - true - - false - - foo - "y": - - null - - 3.14159 - z: - p: true - q: false - d: - e: - - bar - - baz - f: - - xs: - - 1 - ys: - - 2 - - xs: - - 2 - ys: - - 3 - g: - a: - - 1 - - 0 - - 0 - - 0 - b: - - 0 - - 2 - - 0 - - 0 - c: - - 0 - - 0 - - 0 - - 4 - h: - - - 1 - - 2 - - 3 - - - 2 - - 3 - - 4 - l: - - a: bob - b: -1 - c: - - 1 - - 2 - - 3 - - 4 - - a: alice - b: 1 - c: - - 2 - - 3 - - 4 - - 5 - d: null - m: [] - numbers: - - "1" - - "2" - - "3" - - "4" - strings: - bar: 2 - baz: 3 - foo: 1 - three: 3 modules: - - | - package test3 - - p(1, __local3__) = y { - y = __local3__ - } - - p(2, __local4__) = y { - __local8__ = __local4__ + 1 - y = __local8__ - } - - p(__local5__, __local6__) = z { - z = __local5__ - } - - r = y { - data.test3.p(1, 0, y) - } - - | - package test1 - - p(__local0__) = y { - y = __local0__[_] - } - - r = y { - data.test1.p([1, 2, 3], y) - } - | package test2 - p(1, __local1__) = y { - y = __local1__ + p(1, a) = y { + y = a } - p(2, __local2__) = y { - __local7__ = __local2__ + 1 - y = __local7__ + p(2, b) = y { + y = b + 1 } r = y { diff --git a/test/cases/testdata/functionerrors/test-functionerrors-1014.yaml b/test/cases/testdata/functionerrors/test-functionerrors-1014.yaml index 1e464c2bed..14a4ed116e 100644 --- a/test/cases/testdata/functionerrors/test-functionerrors-1014.yaml +++ b/test/cases/testdata/functionerrors/test-functionerrors-1014.yaml @@ -1,132 +1,20 @@ cases: - data: - a: - - 1 - - 2 - - 3 - - 4 - b: - v1: hello - v2: goodbye - c: - - x: - - true - - false - - foo - "y": - - null - - 3.14159 - z: - p: true - q: false - d: - e: - - bar - - baz - f: - - xs: - - 1 - ys: - - 2 - - xs: - - 2 - ys: - - 3 - g: - a: - - 1 - - 0 - - 0 - - 0 - b: - - 0 - - 2 - - 0 - - 0 - c: - - 0 - - 0 - - 0 - - 4 - h: - - - 1 - - 2 - - 3 - - - 2 - - 3 - - 4 - l: - - a: bob - b: -1 - c: - - 1 - - 2 - - 3 - - 4 - - a: alice - b: 1 - c: - - 2 - - 3 - - 4 - - 5 - d: null - m: [] - numbers: - - "1" - - "2" - - "3" - - "4" - strings: - bar: 2 - baz: 3 - foo: 1 - three: 3 modules: - | package test3 - p(1, __local3__) = y { - y = __local3__ + p(1, a) = y { + y = a } - p(2, __local4__) = y { - __local8__ = __local4__ + 1 - y = __local8__ - } - - p(__local5__, __local6__) = z { - z = __local5__ + p(x, y) = z { + z = x } r = y { data.test3.p(1, 0, y) } - - | - package test1 - - p(__local0__) = y { - y = __local0__[_] - } - - r = y { - data.test1.p([1, 2, 3], y) - } - - | - package test2 - - p(1, __local1__) = y { - y = __local1__ - } - - p(2, __local2__) = y { - __local7__ = __local2__ + 1 - y = __local7__ - } - - r = y { - data.test2.p(3, 0, y) - } note: functionerrors/function output conflict multiple query: data.test3.r = x want_error: functions must not produce multiple outputs for same inputs