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: built-in function count is not covered when count on a rule generating sets #1421

Closed
t83714 opened this issue May 9, 2019 · 2 comments

Comments

@t83714
Copy link
Contributor

t83714 commented May 9, 2019

Expected Behavior

Given the following rules:

package test

sites = [{"name": "prod"}, {"name": "smoke1"}, {"name": "dev"}]
q[name] { name := sites[i].name }

default allow = false

allow {
    count(q) > 1
    input.x.a = "xxxxx"
}

We expect the following when run:

OPA 0.10.7 (commit 0f39c27e, built at 2019-04-09T00:29:06Z)

Run 'help' to see a list of commands.

> unknown input
> data.test.allow
+-----------+--------------------------+
| Query 1   | data.partial.test.allow  |
+-----------+--------------------------+
| Support 1 | package partial.test     |
|           |                          |
|           | allow {                  |
|           |   input.x.a = "xxxxx"    |
|           | }                        |
|           |                          |
|           | default allow = false    |
+-----------+--------------------------+

Actual Behavior

We actually got:

OPA 0.10.7 (commit 0f39c27e, built at 2019-04-09T00:29:06Z)

Run 'help' to see a list of commands.

> unknown input
> data.test.allow
+-----------+--------------------------+
| Query 1   | data.partial.test.allow  |
+-----------+--------------------------+
| Support 1 | package partial.test     |
|           |                          |
|           | allow {                  |
|           |   count(data.test.q) > 1 |
|           |   input.x.a = "xxxxx"    |
|           | }                        |
|           |                          |
|           | default allow = false    |
+-----------+--------------------------+

Additional Info

After more tests, I find that if change rules to:

package test

sites = [{"name": "prod"}, {"name": "smoke1"}, {"name": "dev"}]

default allow = false

allow {
    count(sites) > 1
    input.x.a = "xxxxx"
}

The count is removed from the support:

OPA 0.10.7 (commit 0f39c27e, built at 2019-04-09T00:29:06Z)

Run 'help' to see a list of commands.

> sites = [{"name": "prod"}, {"name": "smoke1"}, {"name": "dev"}]
Rule 'sites' defined in package repl. Type 'show' to see rules.
> default allow = false
Rule 'allow' defined in package repl. Type 'show' to see rules.
> allow {
|     count(sites) > 1
|     input.x.a = "xxxxx"
| }
| 
Rule 'allow' defined in package repl. Type 'show' to see rules.
> unknown input
> allow
+-----------+-------------------------+
| Query 1   | data.partial.repl.allow |
+-----------+-------------------------+
| Support 1 | package partial.repl    |
|           |                         |
|           | allow {                 |
|           |   input.x.a = "xxxxx"   |
|           | }                       |
|           |                         |
|           | default allow = false   |
+-----------+-------------------------+
@tsandall
Copy link
Member

@t83714 sorry for the delay! KubeCon EU stole a lot of cycles over the last two weeks.

count(data.test.q) > 1 is saved because data.test.q refers to an incremental rule. In this case, it's a single rule but in the general case, there could be multiple definitions of:

q[1] { input[_] = 1 }`
q[2] { input[_] = 2 }`
q[3] { input[_] = 3 }`
# etc.

To inline these rules we'd have to generate 2^n combinations (where n is the number of rules that depend on unknowns.) If n is really small, that might be OK, but in the general case, it'll be too expensive (which is why we save the expression).

In some cases, it might be fine to partially evaluate the reference. A good example would be if the ref does not depend on unknowns. In this case, we can just inline the result.

@t83714
Copy link
Contributor Author

t83714 commented May 29, 2019

Thanks a lot for your comment 👍
I have now understood more on this issue 😄
I think it's fair to save the expression for an incremental rule that depends on unknowns.
Though, as you suggest, it would be good to inline the result and partially evaluate the count when an incremental rule that does not depend on unknowns --- personally I feel count is a useful concise shortcut 😄

@tsandall tsandall added this to To do in Open Policy Agent via automation Jun 20, 2019
tsandall added a commit to tsandall/opa that referenced this issue Jan 27, 2020
Certain portions of Rego cannot be easily inlined when they depend on
unknowns (e.g., comprehensions). In the past, if PE encountered
statements that could not be inlined, PE would simply save the
original statement regardless of whether that statement (or any of
it's dependencies) depended on unknowns. This was the safest way to
introduce PE initially and worked in many cases as long as authors
were aware of the fragment of Rego that could be PE-ed. Of course,
this places a burden on authors and also fails when constructs like
comprehensions are required.

This commit enhances the evaluator so that PE will evaluate all terms
(e.g., comprehensions, references to full extent of partial sets/objects,
etc.) as long as they do not depend on unknowns. This commit also
enhances the evaluato to support PE on expressions that include with
statements.

Support for with statements is handled by disabling inlining on any
references that are contained in the expression modified by the with
statement. This approach was chosen over open-policy-agent#1956 because while it
generates support rules it avoids the need to re-apply with statements
to inlined expressions.

This commit also addresses open-policy-agent#1417 because negated expressions that can
be completely evaluated will be now.

As part of this change, the evaluator has been refactored to maintain
the inlining controls as a stack (this makes with statements easier to
handle.)

name                       old time/op    new time/op    delta
InliningFullScan/1000-8      5.89ms ± 0%    5.92ms ± 3%    ~     (p=0.548 n=5+5)
InliningFullScan/10000-8     65.8ms ± 2%    65.1ms ± 0%    ~     (p=0.095 n=5+5)
InliningFullScan/300000-8     2.06s ± 2%     2.07s ± 5%    ~     (p=0.841 n=5+5)

name                       old alloc/op   new alloc/op   delta
InliningFullScan/1000-8      2.68MB ± 0%    2.68MB ± 0%  +0.01%  (p=0.008 n=5+5)
InliningFullScan/10000-8     27.4MB ± 0%    27.4MB ± 0%  +0.00%  (p=0.008 n=5+5)
InliningFullScan/300000-8     825MB ± 0%     825MB ± 0%    ~     (p=0.087 n=5+5)

name                       old allocs/op  new allocs/op  delta
InliningFullScan/1000-8       81.0k ± 0%     81.0k ± 0%  +0.01%  (p=0.008 n=5+5)
InliningFullScan/10000-8       810k ± 0%      810k ± 0%  +0.00%  (p=0.008 n=5+5)
InliningFullScan/300000-8     24.3M ± 0%     24.3M ± 0%  +0.00%  (p=0.008 n=5+5)

Fixes open-policy-agent#1069
Fixes open-policy-agent#653
Fixes open-policy-agent#1421
Fixes open-policy-agent#1417

Signed-off-by: Torin Sandall <torinsandall@gmail.com>
Open Policy Agent automation moved this from To do to Done Jan 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

2 participants