From 77e86f08bb398a7bd7bfb15311e64486af124dbb Mon Sep 17 00:00:00 2001 From: Torin Sandall Date: Sat, 7 May 2016 15:13:45 -0700 Subject: [PATCH] Fix indexed evaluation bug Term evaluation was not plugging the expression before checking if the indexer could be used. As a result, the index was being built on the unplugged version of the expression which resulted in false positives being returned by the indexer. Because the evaluation expects the indexer to provide exact matches, the query evaluation was yielding false positives. Also, made a slight tweak to the docs to make the example slightly clearer. --- docs/CONCEPTS.md | 9 +++--- eval/topdown.go | 60 ++++++++++++++++++++++---------------- eval/topdown_test.go | 69 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 108 insertions(+), 30 deletions(-) diff --git a/docs/CONCEPTS.md b/docs/CONCEPTS.md index a86e57650c..efbc24a128 100644 --- a/docs/CONCEPTS.md +++ b/docs/CONCEPTS.md @@ -216,7 +216,7 @@ Content-Type: application/json { "servers": [ - {"id": "s1", "name": "app", "protocols": ["http", "https", "ssh"], "ports": ["p1", "p2", "p3"]}, + {"id": "s1", "name": "app", "protocols": ["https", "ssh"], "ports": ["p1", "p2", "p3"]}, {"id": "s2", "name": "db", "protocols": ["mysql"], "ports": ["p3"]}, {"id": "s3", "name": "cache", "protocols": ["memcache", "http"], "ports": ["p3"]}, {"id": "s4", "name": "dev", "protocols": ["http"], "ports": ["p1", "p2"]} @@ -237,7 +237,7 @@ Content-Type: application/json We can write a rule which enumerates servers that expose HTTP (but not HTTPS) and are connected to public networks. These represent violations of policy. ```rego -package opa.examples # this policy belongs the opa.examples package +package opa.example # this policy belongs the opa.example package import data.servers # import the data.servers document to refer to it as "servers" instead of "data.servers" import data.networks # same but for data.networks @@ -261,9 +261,9 @@ The key aspects of Rego are illustrated by this example: - Rules consist of assertions against data stored in OPA. In this case the assertions are expressions which test for equality and membership of `servers`, `networks`, and `ports` documents. -- Expressions can referenced nested documents, e.g., `ports[i].networks[]` references the network IDs stored in an array on each port document. +- Expressions can referenced nested documents, e.g., `ports[i].networks[_]` references the network IDs stored in an array on each port document. -- Expressions can reference elements in a collection using the `[]` and `[]` syntax. When this is done, OPA knows to iterate over elements of the collection when processing queries. +- Expressions can reference elements in a collection using the `[_]` and `[]` syntax. When this is done, OPA knows to iterate over elements of the collection when processing queries. If we query for the document produced by this rule, we receive an array of servers that expose an HTTP server and are connected to a public network: @@ -278,7 +278,6 @@ HTTP/1.1 200 OK Content-Type: application/json [ - {"id": "s1", "name": "app-server", "protocols": ["http", "https", "ssh"], "ports": ["p1", "p2", "p3"]}, {"id": "s4", "name": "dev", "protocols": ["http"], "ports": ["p2"]} ] ``` diff --git a/eval/topdown.go b/eval/topdown.go index 8adb3374bf..0f39b3dc99 100644 --- a/eval/topdown.go +++ b/eval/topdown.go @@ -956,23 +956,18 @@ func evalRefRuleResult(ctx *TopDownContext, ref ast.Ref, suffix ast.Ref, result // variables used in references involves iterating collections in storage or // evaluating rules identified by the references. In either case, this function // will invoke the iterator with each set of bindings that should be evaluated. -// -// TODO(tsandall): extend to support indexing. func evalTerms(ctx *TopDownContext, iter TopDownIterator) error { expr := ctx.Current() - var ts []*ast.Term - switch t := expr.Terms.(type) { - case []*ast.Term: - ts = t - case *ast.Term: - ts = append(ts, t) - default: - panic(fmt.Sprintf("illegal argument: %v", t)) - } - if indexAvailable(ctx, ts) { + // Check if indexing is available for this expression. Need to + // perform check on the plugged version of the expression, otherwise + // the index will return false positives. + plugged := plugExpr(expr, ctx.Bindings) + if indexAvailable(ctx, plugged) { + + ts := plugged.Terms.([]*ast.Term) ref, isRef := ts[1].Value.(ast.Ref) if isRef { @@ -998,6 +993,16 @@ func evalTerms(ctx *TopDownContext, iter TopDownIterator) error { } } + var ts []*ast.Term + switch t := expr.Terms.(type) { + case []*ast.Term: + ts = t + case *ast.Term: + ts = append(ts, t) + default: + panic(fmt.Sprintf("illegal argument: %v", t)) + } + return evalTermsRec(ctx, iter, ts) } @@ -1127,25 +1132,30 @@ func evalTermsRecObject(ctx *TopDownContext, obj ast.Object, idx int, iter TopDo // Indexing is used on equality expressions where both sides are non-ground refs (to base docs) or one // side is a non-ground ref (to a base doc) and the other side is any ground term. In the future, indexing // may be used on references embedded inside array/object values. -func indexAvailable(ctx *TopDownContext, terms []*ast.Term) bool { +func indexAvailable(ctx *TopDownContext, expr *ast.Expr) bool { - // Indexing can only be used when evaluating equality expressions. - if !terms[0].Value.Equal(equalityBuiltin) { + ts, ok := expr.Terms.([]*ast.Term) + if !ok { return false } - pluggedA := plugTerm(terms[1], ctx.Bindings) - pluggedB := plugTerm(terms[2], ctx.Bindings) - - _, isRefA := pluggedA.Value.(ast.Ref) - _, isRefB := pluggedB.Value.(ast.Ref) - - if isRefA && !pluggedA.IsGround() { - return pluggedB.IsGround() || isRefB + // Indexing can only be used when evaluating equality expressions. + if !ts[0].Value.Equal(equalityBuiltin) { + return false } - if isRefB && !pluggedB.IsGround() { - return pluggedA.IsGround() || isRefA + a := ts[1].Value + b := ts[2].Value + + _, isRefA := a.(ast.Ref) + _, isRefB := b.(ast.Ref) + + if isRefA && !a.IsGround() { + return b.IsGround() || isRefB + } + + if isRefB && !b.IsGround() { + return a.IsGround() || isRefA } return false diff --git a/eval/topdown_test.go b/eval/topdown_test.go index 9c2e4fad9a..c55ba414d2 100644 --- a/eval/topdown_test.go +++ b/eval/topdown_test.go @@ -533,6 +533,75 @@ func TestTopDownEmbeddedVirtualDoc(t *testing.T) { assertTopDown(t, store, 0, "deep embedded vdoc", []string{"b", "c", "d", "p"}, "[1, 2, 4]") } +func TestExample(t *testing.T) { + + bd := ` + { + "servers": [ + {"id": "s1", "name": "app", "protocols": ["https", "ssh"], "ports": ["p1", "p2", "p3"]}, + {"id": "s2", "name": "db", "protocols": ["mysql"], "ports": ["p3"]}, + {"id": "s3", "name": "cache", "protocols": ["memcache", "http"], "ports": ["p3"]}, + {"id": "s4", "name": "dev", "protocols": ["http"], "ports": ["p1", "p2"]} + ], + "networks": [ + {"id": "n1", "public": false}, + {"id": "n2", "public": false}, + {"id": "n3", "public": true} + ], + "ports": [ + {"id": "p1", "networks": ["n1"]}, + {"id": "p2", "networks": ["n3"]}, + {"id": "p3", "networks": ["n2"]} + ] + } + ` + + vd := ` + package opa.example + + import data.servers + import data.networks + import data.ports + + public_servers[server] :- + server = servers[i], + server.ports[j] = ports[k].id, + ports[k].networks[l] = networks[m].id, + networks[m].public = true + + violations[server] :- + server = servers[i], + server.protocols[j] = "http", + public_servers[server] + ` + + var doc map[string]interface{} + + if err := json.Unmarshal([]byte(bd), &doc); err != nil { + panic(err) + } + + mods := compileModules([]string{vd}) + + store, err := NewStorage([]map[string]interface{}{doc}, mods) + if err != nil { + panic(err) + } + + assertTopDown(t, store, 0, "public servers", []string{"opa", "example", "public_servers"}, ` + [ + {"id": "s1", "name": "app", "protocols": ["https", "ssh"], "ports": ["p1", "p2", "p3"]}, + {"id": "s4", "name": "dev", "protocols": ["http"], "ports": ["p1", "p2"]} + ] + `) + + assertTopDown(t, store, 0, "violations", []string{"opa", "example", "violations"}, ` + [ + {"id": "s4", "name": "dev", "protocols": ["http"], "ports": ["p1", "p2"]} + ] + `) +} + func compileModules(input []string) []*ast.Module { mods := []*ast.Module{}