Unexpected response from partial evaluation call

[robhafner]

I'm seeing an unexpected response when making an opa.Partial(...) call in the test case linked below.

https://github.com/open-policy-agent/opa/compare/main...robhafner:opa:discussion1?expand=1

A similar call (in the same test) to opa.Decision(...) results in the value true being returned. However, the call to Partial(...) does not result in the partial Queries field being an empty array as a result of OPA being able to make a non-conditional decision. Which is what I would expect based upon the information found in the following link:

https://www.openpolicyagent.org/docs/latest/rest-api/#unconditional-results-from-partial-evaluation

Could this be a bug in the behavior of the partial decision evaluation? Or am I overlooking something?

[robhafner]

Just checking back to see if there are any thoughts on my previous post? If it helps I can create a PR on the OPA project for you to evaluate further.

[ashutosh-narkar]

@srenatus @johanfylling any thoughts on this?

[srenatus]

However, the call to Partial(...) does not result in the partial Queries field being an empty array as a result of OPA being able to make a non-conditional decision.

What does it return?

[robhafner]

When I remove the mapper from the partial decision test, it returns an array of size 1 for the queries with the single query containing 9 *ast.Expr values.

Here is a screen shot from my debugger.

[srenatus]

That's not helping us much, is it? 😅
I've changed the assertion to this

	if presult, err := opa.Partial(ctx, sdk.PartialOptions{Query: "data.sas.types.authz.allow = true", Input: input}); err != nil {
		t.Fatal(err)
	} else if !reflect.DeepEqual(presult.Result, exp) {
		t.Error(presult.Result.(*rego.PartialQueries).Queries[0])
	}

and that gives

internal.member_2(input.request.permission, ["read"])
input.request.uri = "/types/"
opa.runtime(__local5__4)
io.jwt.verify_rs256(input.token, __local5__4.config.env.SAS_OAUTH_TOKEN_VERIFY_KEY)
[__local0__4, _, __local2__4] = io.jwt.decode(input.token) 
_
opa.runtime(__local5__5)
io.jwt.verify_rs256(input.token, __local5__5.config.env.SAS_OAUTH_TOKEN_VERIFY_KEY)
[__local0__5, __local13__3, __local2__5] = io.jwt.decode(input.token)
gt(count(__local13__3), 0, true)

So, it looks like input being part of the unknowns is a default of some sort. Explicitly passing Unknowns: []string{} like this

	if presult, err := opa.Partial(ctx, sdk.PartialOptions{Unknowns: []string{}, Query: "data.sas.types.authz.allow = true", Input: input}); err != nil {

changes the results to

opa.runtime(__local5__4)
io.jwt.verify_rs256("eyJhbGciOiJSUzI1NiIsImprdSI6Imh0dHBzOi8vbG9jYWxob3N0L1NBU0xvZ29uL3Rva2VuX2tleXMiLCJraWQiOiJsZWdhY3ktdG9rZW4ta2V5IiwidHlwIjoiSldUIn0.eyJqdGkiOiI1NGEyNzczOTljMmY0NjQ1OGJjNWM1NmY5YzhlMzFiOCIsImF1dGhvcml0aWVzIjpbInVuaXhfciZkIl0sImV4dF9pZCI6ImNuPVdvcmtmbG93IFVzZXIgMSxvdT1HZW5lcmljIGFuZCBTaGFyZWQgQWNjb3VudHMsb3U9QWRtaW4sZGM9bmEsZGM9U0FTLGRjPWNvbSIsInN1YiI6Ijc2MmI2M2FkLTUwMDItNDA3My04MzE5LTg2OTMyNTg1ZDQyYyIsInNjb3BlIjpbIm9wZW5pZCIsInVhYS51c2VyIl0sImNsaWVudF9pZCI6InNhcy5lYyIsImNpZCI6InNhcy5lYyIsImF6cCI6InNhcy5lYyIsImdyYW50X3R5cGUiOiJwYXNzd29yZCIsInVzZXJfaWQiOiI3NjJiNjNhZC01MDAyLTQwNzMtODMxOS04NjkzMjU4NWQ0MmMiLCJvcmlnaW4iOiJsZGFwIiwidXNlcl9uYW1lIjoid2Z1c2VyMSIsImVtYWlsIjoid2Z1c2VyMUB1c2VyLmZyb20ubGRhcC5jZiIsImF1dGhfdGltZSI6MTcwMDE3OTk3NCwicmV2X3NpZyI6IjE2ZDg2ODU4IiwiaWF0IjoxNzAwMTc5OTc0LCJleHAiOjE3MDAxODM1NzQsImlzcyI6Imh0dHA6Ly9sb2NhbGhvc3QvU0FTTG9nb24vb2F1dGgvdG9rZW4iLCJ6aWQiOiJ1YWEiLCJhdWQiOlsidWFhIiwib3BlbmlkIiwic2FzLmVjIl19.ThPZLVINo5094XEiueiCD_Kt0tI6BDzZHdZ2Smz8KgAA07rIxG4mIdtehEiLMR5wJzjYh0eIL66DIBTMVBCqkdXGYNvVo5IGHDzFpjVGurnrpa-ANlY5Y4Epml03cF9edmILP4K1UpIfmGFL1Fxz1HfNkKXoq5KalJQTGAVi3Q_ArJ4pIkjz85NMdqE8NFoh6jBjI8bvlpgTthYftLA_jaDF6tpJPWuO_eTyWTL562hVkIErqSCyBp_BnBs-XxfWqvlqe5fTslumWDlBSkAN3Kbf_VBKWpzEJ25tOxwz_swODG6QJOdL1vH35-agKkNg9Q7qLzN_beaXMuSmKMZJxw", __local5__4.config.env.SAS_OAUTH_TOKEN_VERIFY_KEY)
opa.runtime(__local5__5)
io.jwt.verify_rs256("eyJhbGciOiJSUzI1NiIsImprdSI6Imh0dHBzOi8vbG9jYWxob3N0L1NBU0xvZ29uL3Rva2VuX2tleXMiLCJraWQiOiJsZWdhY3ktdG9rZW4ta2V5IiwidHlwIjoiSldUIn0.eyJqdGkiOiI1NGEyNzczOTljMmY0NjQ1OGJjNWM1NmY5YzhlMzFiOCIsImF1dGhvcml0aWVzIjpbInVuaXhfciZkIl0sImV4dF9pZCI6ImNuPVdvcmtmbG93IFVzZXIgMSxvdT1HZW5lcmljIGFuZCBTaGFyZWQgQWNjb3VudHMsb3U9QWRtaW4sZGM9bmEsZGM9U0FTLGRjPWNvbSIsInN1YiI6Ijc2MmI2M2FkLTUwMDItNDA3My04MzE5LTg2OTMyNTg1ZDQyYyIsInNjb3BlIjpbIm9wZW5pZCIsInVhYS51c2VyIl0sImNsaWVudF9pZCI6InNhcy5lYyIsImNpZCI6InNhcy5lYyIsImF6cCI6InNhcy5lYyIsImdyYW50X3R5cGUiOiJwYXNzd29yZCIsInVzZXJfaWQiOiI3NjJiNjNhZC01MDAyLTQwNzMtODMxOS04NjkzMjU4NWQ0MmMiLCJvcmlnaW4iOiJsZGFwIiwidXNlcl9uYW1lIjoid2Z1c2VyMSIsImVtYWlsIjoid2Z1c2VyMUB1c2VyLmZyb20ubGRhcC5jZiIsImF1dGhfdGltZSI6MTcwMDE3OTk3NCwicmV2X3NpZyI6IjE2ZDg2ODU4IiwiaWF0IjoxNzAwMTc5OTc0LCJleHAiOjE3MDAxODM1NzQsImlzcyI6Imh0dHA6Ly9sb2NhbGhvc3QvU0FTTG9nb24vb2F1dGgvdG9rZW4iLCJ6aWQiOiJ1YWEiLCJhdWQiOlsidWFhIiwib3BlbmlkIiwic2FzLmVjIl19.ThPZLVINo5094XEiueiCD_Kt0tI6BDzZHdZ2Smz8KgAA07rIxG4mIdtehEiLMR5wJzjYh0eIL66DIBTMVBCqkdXGYNvVo5IGHDzFpjVGurnrpa-ANlY5Y4Epml03cF9edmILP4K1UpIfmGFL1Fxz1HfNkKXoq5KalJQTGAVi3Q_ArJ4pIkjz85NMdqE8NFoh6jBjI8bvlpgTthYftLA_jaDF6tpJPWuO_eTyWTL562hVkIErqSCyBp_BnBs-XxfWqvlqe5fTslumWDlBSkAN3Kbf_VBKWpzEJ25tOxwz_swODG6QJOdL1vH35-agKkNg9Q7qLzN_beaXMuSmKMZJxw", __local5__5.config.env.SAS_OAUTH_TOKEN_VERIFY_KEY)

Which looks rather redundant. Also, I suppose opa.runtime() is taken to be opaque, or "unsafe" for PE -- it might not get evaluated during PE.

[robhafner]

Sorry about that. I wasn't sure what the easiest way was to attach the value of presult to this thread which is why I ended up taking a screenshot and attaching a link to the test case. Thanks for the tip on how to make it easier to share that information.

I noticed the same behavior with a nil value for the unknowns as well. I wasn't sure if that was the intended behavior or not. In our service code the unknowns value will always be non nil so that behavior isn't so important to our use case.

I definitely agree that this issue appears to be related to the use of opa.runtime() as we recently added a call to it when we were switching over to use a JWT in our policy.

Given your assessment would you classify this as a bug or missing functionality in partial evaluation? If so, should I open an issue for it?

[srenatus]

Given your assessment would you classify this as a bug or missing functionality in partial evaluation? If so, should I open an issue for it?

Is it on par with what the Compile API gives you? I think if it is, then the enhancement worth proposing is to let users control the currently-hardcoded set of builtins that will not be evaluated during PE.

For your concrete problem, if you want some env vars to have an effect, you could try injecting a synthetic module that is just

package variables

SAS_OAUTH_TOKEN_VERIFY_KEY := "eyJh...."

and use data.variables.SAS_OAUTH_TOKEN_VERIFY_KEY in your policies. That way, PE will have the values available and can do the JWT verification at PE-time.

[robhafner]

I have not tried the Compile API as we don't intend to use it in our use case.

Yes, I was planning to just define the key as a variable instead as a workaround I hadn't considered putting it in its own package. Not sure if there is any benefit of doing that over just defining the variable in the same package.

One of things that I feel I'm struggling a bit with is the Document model. I've read the following link, however I feel I don't have a full understanding of it. For example, with Dynamic Composition data.policies can reference the policies defined in the retrieved bundle. Based on the doc, I understand your example with variables but why the policies are there is a bit less clear. Is there a way to view the document that OPA builds to evaluate a decision. That might help me understand what properties are available.

[srenatus]

I hadn't considered putting it in its own package. Not sure if there is any benefit of doing that over just defining the variable in the same package.

Yeah that's the same thing. I've proposed an extra package because you don't have to mess with your existing ones then, but since you'll have to reference that extra package anyways, it doesn't make much of a difference 👍

Is there a way to view the document that OPA builds to evaluate a decision. That might help me understand what properties are available.

You can run all the things using opa eval -fpretty, including PE. To see the virtual doc that's built, you can query data, or data.some.thing, you're not restricted to only checking your allow rule itself. Does that help? Usually when dealing with PE, I'd start with opa eval -p, not go straight to golang library code. 😅

[robhafner]

Yes, that is helpful! I had tried the -pretty option before but had not changed the query string to be 'data'. Thanks for the tip!

I started using the opa eval command for dynamic composition which helped significantly with testing Dynamic Composition given I didn't see a way to define multiple packages in the Rego Playground.

When you mentioned the Compile API earlier I thought you were referring to the API mentioned at this link which is REST based. Thinking about it some more, I'm wondering if you might have been referring to the 'opa' binary (i.e opa eval command) as the "compile api". If so, I can give that a try and report back.

[srenatus]

I didn't see a way to define multiple packages in the Rego Playground.

There is no way. 😅

When you mentioned the Compile API earlier I thought you were referring to the API mentioned at this link which is REST based. Thinking about it some more, I'm wondering if you might have been referring to the 'opa' binary (i.e opa eval command) as the "compile api". If so, I can give that a try and report back.

I was referring to the Compile API of opa run --server, indeed. But in the end, the SDK's + the REST API's + opa eval's possibilities should be pretty much the same. So, if it behaves the same in opa eval --partial as it does in the SDK, I wouldn't call it bug.

Nonetheless, you're asking for something sensible, I think, so it's just matter of the right label to put on it: bug fix or feature request.

[robhafner]

Similar behavior occurs when using opa eval. For example:

Using this policy:
`package sas.types.authz

import future.keywords.if
import future.keywords.in

default allow := false

grant if {
input.request.permission in ["read"]
input.request.uri = "/types/"
opa.runtime().env.SAS_AUTHENTICATED_USER = "true"
#authenticatedUser = true
}`

And the following input:
{ "request" : { "uri" : "/types/", "permission" : "read" } }

And the following environment variable set:
SAS_AUTHENTICATED_USER="false"

Running the following command:
opa eval 'data.sas.types.authz.allow' -i input.json -b . -f pretty -u "data.resources" -p

produces the following output:

+-----------+---------------------------------------------------+ | Query 1 | data.partial.sas.types.authz.allow | +-----------+---------------------------------------------------+ | Support 1 | package partial.sas.types.authz | | | default allow = false | | | | | | allow { | | | opa.runtime(__local0__2) | | | __local0__2.env.SAS_AUTHENTICATED_USER = "true" | | | } | +-----------+---------------------------------------------------+

Where as running this command:

opa eval 'data.sas.types.authz.allow' -i input.json -b . -f pretty

Produces this output:
false

Does that information help you decide if this is a bug or a new feature request? I think I still lean toward this being a (missing functionality) bug as I have not found any documentation suggesting partial evaluation does not support the use of "opa.runtime()".

[johanfylling]

The output of opa.runtime() can change from run to run, so I would expect PE to treat its output as unknown, and always preserve such calls in the resulting query/module (if part of the hot path). Which seems to be what's happening here.

[robhafner]

Thanks Johan! In our use case the environment variable will always be available and will not change without a restart of the Kubernetes pod. For this use case, it would be helpful if OPA understood that and evaluated the conditions associated with the environment variable. Would it make sense for OPA to make a decision on whether it can evaluate the condition based on if the environment variable is available and if it isn't keep the same behavior?

[robhafner]

The company I work for is looking to support open source projects that we use by contributing back to the projects. Therefore we are open to contributing the requested functionality back to OPA if the project owners feel this is something that is within the grasp of a new user and the maintainers support our contribution effort. Please let me know if this is something you are open to.