Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

partial eval: Improve support for cases where default values are not required #820

Closed
tsandall opened this issue Jul 3, 2018 · 2 comments
Assignees

Comments

@tsandall
Copy link
Member

tsandall commented Jul 3, 2018

Currently, OPA generates support rules for all rule sets that include a default rule, regardless of whether:

  1. the default value is required in the partial evaluation output
  2. the rule set depends on unknown values

This issue focuses on the first problem.

For example:

Query (unknowns=[input]):

data.example.allow = true

Policy:

package example

default allow = false

allow {
  input.x = 1
}

allow {
  input.y = 2
}

In this case, the result of partial evaluation is:

Queries (1 total):

data.partial.example.allow = true

Support modules (1 total):

package partial.example

allow = true { input.x = 1 }
default allow = false

In this case, the default value is not actually required in the partial evaluation output (since true != false). OPA should avoid generating support rules in cases like this which will be common for Compile API integrations.

Related Issues: #812

@srenatus
Copy link
Contributor

srenatus commented Jul 5, 2018

Where would this go? I'd be happy to give this a try, but I'm a bit lost as to where to start

@tsandall tsandall changed the title partial eval: Improve support for default rules partial eval: Improve support for cases where default value is not required Jul 5, 2018
@tsandall tsandall changed the title partial eval: Improve support for cases where default value is not required partial eval: Improve support for cases where default values are not required Jul 5, 2018
@tsandall
Copy link
Member Author

tsandall commented Jul 5, 2018

hey @srenatus, I've updated this issue to scope it better. I've created another issue (#823) to capture the other improvement we could make. The other issue will require a bit more groundwork.

For this issue, the observation is that the default value is not required in the partial evaluation output, i.e., the query above will be undefined when all of the non-default rules are undefined, similarly, the query will be defined if any of the non-default rules are defined.

The evaluator currently has a special case for partial evaluation of rules with a default value: https://github.com/open-policy-agent/opa/blob/master/topdown/eval.go#L1445. When the evaluator checks if the rule set has a default value, it can determine if the default value is required. In these cases, the default value is NOT required if the other term in the expression (<ref> = <term>) does not equal the default value. The other term in the expression is part of the evalVirtualComplete struct state (it's the rterm field.)

I think what needs to happen is the evaluator should plug rterm and compare the result to the default value. Something like this:

if e.ir.Default != nil {
  plugged := e.rbindings.Plug(e.rterm)
  if !ast.IsConstant(plugged) || plugged.Equal(e.ir.Default.Value) {
     return // generate support rule like we do today
  }
}

return // partially evaluate rule normally

Plugging substitues variable bindings. E.g., given the query query x=1; x=y; y=z; z>0, plugging x and y in the last expression would result in 1>0.

@tsandall tsandall self-assigned this Jul 23, 2018
tsandall added a commit to tsandall/opa that referenced this issue Jul 24, 2018
Previously, partial eval would always generate a support rule when it
evaluated a rule with a default value. These changes tweak this
behaviour so that support rules are only generated if the default value
is required in the partial eval output. It's better to avoid generating
support rules as it simplifies the assumptions that the consumer of
partial eval can make.

Fixes open-policy-agent#820

Signed-off-by: Torin Sandall <torinsandall@gmail.com>
tsandall added a commit that referenced this issue Jul 24, 2018
Previously, partial eval would always generate a support rule when it
evaluated a rule with a default value. These changes tweak this
behaviour so that support rules are only generated if the default value
is required in the partial eval output. It's better to avoid generating
support rules as it simplifies the assumptions that the consumer of
partial eval can make.

Fixes #820

Signed-off-by: Torin Sandall <torinsandall@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants