glob.match function does not default delimiter like the docs describe

[FredrikAppelros]

Expected Behavior

Queries for a particular document should return the same result both with and without the partial parameter as detailed here.

Using the policy and request given below we would expect the following response:

{
    "result": true
}

Actual Behavior

We receive expected response when querying the document without the partial parameter. However when we add it we instead get the following response:

{
    "result": false
}

Steps to Reproduce the Problem

1. Start OPA with the policy file given in the gist below using the following command:
   opa run -s authz.rego
2. Verify that we get the expected response without the partial parameter:
   curl "http://localhost:8181/v1/data/authz/allow" -d '{"input": {"user": {"name": "bob", "groups": ["admins"]}, "resource": {"name": "foo"}, "operation": "foo.read"}}'
3. Send the same request but this time with the partial parameter
   curl "http://localhost:8181/v1/data/authz/allow?partial" -d '{"input": {"user": {"name": "bob", "groups": ["admins"]}, "resource": {"name": "foo"}, "operation": "foo.read"}}'

Additional Info

Use the following request body when calling the data API:

{
  "input": {
    "user": {
      "name": "bob",
      "groups": ["admins"]
    },
    "resource": {
    	"name": "foo"
    },
    "operation": "foo.read"
  }
}

You can find example code of the issue in this gist.

[FredrikAppelros]

It seems to be an issue with the globbing of operation. I can only reproduce the issue when operation has a . character in it.

[tsandall]

@FredrikAppelros thanks for filing this (and btw, this is an excellent bug report...)

Executing the query with the partial eval optimization enabled does return a different answer. With tracing enabled we can see what's different:

$ curl 'localhost:8181/v1/data/authz/allow?pretty&explain=full&partial' -d @input.json
{
  "explanation": [
    "Enter data.partial.__result__ = _",
    "| Eval data.partial.__result__ = _",
    "| Index data.partial.__result__ = _ (matched 1 rule)",
    "| Enter data.partial.__result__",
    "| | Eval data.partial.authz.allow = _",
    "| | Index data.partial.authz.allow = _ matched 0 rules)",
    "| | Enter data.partial.authz.allow",
    "| | | Eval true",
    "| | | Exit data.partial.authz.allow",
    "| | Exit data.partial.__result__",
    "| Exit data.partial.__result__ = _",
    "Redo data.partial.__result__ = _",
    "| Redo data.partial.__result__ = _",
    "| Redo data.partial.__result__",
    "| | Redo data.partial.authz.allow = _",
    "| | Redo data.partial.authz.allow",
    "| | | Redo true"
  ],
  "result": false
}

This line suggests the rule indexer is to blame:

EDIT: whoops, hit enter too soon.

    "| | Index data.partial.authz.allow = _ matched 0 rules)",

The rule indexer builds a trie/multi-dimensional index out of = and glob.match statements in policies. This line indicates the rule indexer returned no potential matches for the allow rule.

If we partially evaluate the policy we can see what's actually be evaluated when ?partial is used:

$ opa eval -d x.rego -f source -p 'data.authz.allow'
# Query 1
data.partial.authz.allow

# Module 1
package partial.authz

allow {
	"admins" = input.user.groups[_]
	glob.match("*", [], input.resource.name)
	glob.match("*", [], input.operation)
}

default allow = false

If the last expression in the allow rule is disabled, the answer is true as expected. When the last expression is enabled (or it's the only expression in the rule), the answer is false.

Now, the interesting thing is that I would NOT have expected glob.match("*", [], "foo.bar") to be true. When the second operand is empty it is supposed to default to ["."] (see the docs) and sure enough:

> glob.match("*", [], "foo")
true
> glob.match("*", ["."], "foo")
true
> glob.match("*", [], "foo.bar")
true
> glob.match("*", ["."], "foo.bar")
false

The reason the partially evaluated policy returns a different answer is that the expression has been simplified to the point where the rule indexer can understand it. The rule indexer implements the defaulting described in the docs:

opa/ast/index.go

Line 716 in 4ffd5ff

if len(arr) == 0 {

.

It looks like the built-in function in the evaluator has not implemented defaulting described in the docs: https://github.com/open-policy-agent/opa/blob/master/topdown/glob.go#L39.

The solution would appear to be a simple fix in the evaluator's built-in implementation that checks if the delimiter slice is empty and defaults to ["."] like the docs describe. I'm slightly concerned about making this change because it's going to be backwards incompatible, but that's probably the best thing to do.

cc @aeneasr do you have any thought son what the correct behaviour is for the built-in function? See the four REPL examples above (per the docs the built-in is supposed to default the delimiters to ["."] if empty but that doesn't seem to be happening.)

[FredrikAppelros]

Thank you for the detailed explanation and quick response!