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

(*Rego).Partial() with default values #812

Closed

Conversation

srenatus
Copy link
Contributor

@srenatus srenatus commented Jul 2, 2018

This result seems a bit odd to me. First pushing the commit to show that it works now, and then the change to trigger the odd behaviour.

Concretely, when adding

default allow = false

to the test module, the output that was

Query #1: "GET" = input.method; input.path = ["reviews", _]; neq(input.is_admin, false)
Query #2: "GET" = input.method; input.path = ["reviews", user3]; user3 = input.user

becomes

Query #1: equal(data.partial.test.allow, true)
Support #1: package partial.test

allow = true { "GET" = input.method; input.path = ["reviews", _]; input.is_admin = _; neq(_, false) }
allow = true { "GET" = input.method; input.path = ["reviews", user]; input.user = user }
default allow = false

So, it seems to be adding a layer of indirection.

To illustrate why this seems odd, I've added an example that is closer to our real-world usage, and its output is

Query #1: neq(data.partial.test.allow, false)
Support #1: package partial.test

allow = true { true }
default allow = false

it seems like this could be further reduced -- am I wrong? I'm not sure if this really is a variant of #709 .

srenatus added 3 commits July 2, 2018 13:26
These are not currently part of the results, so this "example test"
should still pass just fine.

Signed-off-by: Stephan Renatus <srenatus@chef.io>
Signed-off-by: Stephan Renatus <srenatus@chef.io>
Signed-off-by: Stephan Renatus <srenatus@chef.io>
@@ -538,6 +538,8 @@ func ExampleRego_Partial() {
// Define a simple policy for example purposes.
module := `package test

default allow = false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default keyword implies ordering. I.e., the default rule is only defined if none of the other rules with the same name are defined. In the general case, the partial evaluation has to generate support rules to handle this.

For the initial Compile API (and rego.Rego#Partial) use cases where we're returning the partial evaluation result to the client, I think it's better if we do not have to deal with support rules.

In the short term, I think we can introduce a special case into partial evaluation where expressions like data.example.allow = true do not require a support rule when data.example.allow refers to a rule with a default value.

input["path"] = []string{"reviews", "alice"}

r := rego.New(
rego.Query("data.test.allow"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, OPA is going to have to generate support rules because this query is asking for the value of data.test.allow which implicitly says data.test.allow = x--so we can't omit the default value like I described above.

Signed-off-by: Stephan Renatus <srenatus@chef.io>
input["path"] = []string{"reviews", "alice"}

r := rego.New(
rego.Query("data.test.allow == true"),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, OPA is going to have to generate support rules because this query is asking for the value of data.test.allow which implicitly says data.test.allow = x--so we can't omit the default value like I described above.

Thanks, I wasn't aware of that distinction for a moment. However, having changed it, the output goes from

Query #1: neq(data.partial.test.allow, false)
Support #1: package partial.test

allow = true { true }
default allow = false

to

Query #1: equal(data.partial.test.allow, true)
Support #1: package partial.test

allow = true { true }
default allow = false

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the current implementation, that's what I would expect in this case--since allow has a default rule, OPA blindly generates support rules. I'll file an issue to improve the behaviour for this relatively common case. I think we can improve this for the next release (v0.9) the Compile API.

For now, I would rather leave ExampleRego_Partial as is.

@tsandall
Copy link
Member

tsandall commented Jul 3, 2018

Filed #820 to track the improvements. Closing this for now, if you think we should revisit, let me know.

@ovidius72
Copy link

Hi, sorry to bother on an old issue. I've searched around but I'm still a bit confused about this issue. Is it possible to add default allow = true in a policy that will be evaluated with the compile API ?
While testing I get the name of the policy = true if I provide the default value, otherwise it works as expected. Can you please tell me if this is the default behaviour of the partial evaluation ?
Thanks

@tsandall
Copy link
Member

@ovidius72 it's possible but you will receive "support" rules from the Compile API depending on how you reference allow. If you reference allow like this: data.path.to.allow = false then the support rule can be omitted. OTOH, if you reference allow in any of these ways, it can't:

  • data.path.to.allow
  • data.path.to.allow = x
  • data.path.to.allow = true

The Compile API has to generate a support rule if the reference to allow could match the default rule. In the case of data.path.to.allow = false it's known that the default value will never match so it can be omitted.

@ovidius72
Copy link

ovidius72 commented Jan 25, 2020

Thanks @tsandall
It seems that I have to remove the default allow = true from the rego policy and reference it in the data, e.g in the "query" field when using the Rest APIs.
In this case I'm not receiving anymore the "support" field from opa.
However it works with both data.path.to.allow = true and data.path.to.allow = false, in the sense that the support field is not received.
I'm probably asking for silly questions but I'm still trying to learn. When I reference allow = false, should I expect the result to be false when no rule matches or it will always be an empty object:
{ "result": {} }
Thanks

@tsandall
Copy link
Member

tsandall commented Feb 5, 2020

The result structure is described in this section of the docs: https://www.openpolicyagent.org/docs/latest/rest-api/#unconditional-results-from-partial-evaluation

The result you posted ({"result": {}}) indicates query was unconditionally false/undefined, i.e., allow = false was not even conditionally true.

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

Successfully merging this pull request may close these issues.

3 participants