open-policy-agent / opa

Open Policy Agent (OPA) is an open source, general-purpose policy engine.
https://www.openpolicyagent.org
Apache License 2.0
9.75k stars 1.35k forks source link

Add ability to wrap errors returned by custom built-in functions #5890

Closed ajith-sub closed 1 year ago

ajith-sub commented 1 year ago

What is the underlying problem you're trying to solve?

I'd like to be able to retrieve a specific error object that's bubbled up from the return value of a custom built-in function during evaluation. This would help in cases where relying on the Message field of topdown.Error doesn't give enough context about how to handle the error returned from policy evaluation originating from a custom built-in function.

Currently, errors returned by built-ins have their messages formatted in the topdown.Error.Message field in rego.finishFunction. If the built-in function returns a a custom error type that contains more contextual information about the error in its fields, this information is either lost or only available through the string representation in the Message field.

Describe the ideal solution

An example of what I would like to be able to do:

result, err := opa.Decision(ctx, sdk.DecisionOptions{Input: input, Path: path})
var customErr mybuiltins.BuiltinError
if err != nil && errors.As(err, &customErr) {
    // Get additional error information from customErr
}

This could be possible if an Unwrap method and an additional error field is added to topdown.Error so that the wrapped error can be retrieved. However, I'm not sure how feasible or far-reaching that change would be to other API/SDK methods that rely on topdown.Error.

ashutosh-narkar commented 1 year ago

As you mentioned you could encode all the necessary context from your custom error in it's Error method and the caller should get all the additional info for further processing. Is that not sufficient for your use case?

ajith-sub commented 1 year ago

Retrieving that information from the Message might work, but requires parsing that information from a string that's formatted using a specific pattern (e.g. "builtin: output from custom Error() implementation goes here").

An example use case is whenever the custom error information needs to be serialized, e.g. as part of a response from an HTTP server. It may be possible to recover this data from the error string, but it would involve serializing the custom error information in Error(), relying on how the message has been formatted in the topdown.Error.Message that consumes the built-in error, and unmarshaling the serialized JSON in the message.

It would be convenient to be able to retrieve the error data using Go's error wrapping idiom if it's feasible to extend topdown.Error.

ashutosh-narkar commented 1 year ago

It seems to me the caller should be able to handle the roundtrip given the Error method is defined by the plugin author. It would still be interesting to know what wrapping idiom you are referring to here.

ajith-sub commented 1 year ago

The caller can retrieve that information from the returned error string, but the string needs to be parsed according to how it's formatted internally by OPA. Can we rely on this format not changing? Specifically, I'm talking about the following method that finalizes built-in method calls: https://github.com/open-policy-agent/opa/blob/main/rego/rego.go#L2576

The idiom I'm referring to is the wrapping functionality introduced in Go 1.13 that allows nested errors to be retrieved: https://go.dev/doc/go1.13#error_wrapping

ashutosh-narkar commented 1 year ago

Can we rely on this format not changing?

topdown.Error is part of the public API, so I would not expect it to change in a backward incompatible manner.

srenatus commented 1 year ago

I suppose it would be possible to have unwrap functionality added to the topdown error without breaking existing code. It might be easiest to discuss a PR, at least as a draft?

ajith-sub commented 1 year ago

Thanks for the feedback.

I've submitted a draft PR with a first pass of changes required to support this: https://github.com/open-policy-agent/opa/pull/5909

stale[bot] commented 1 year ago

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days. Although currently inactive, the issue could still be considered and actively worked on in the future. More details about the use-case this issue attempts to address, the value provided by completing it or possible solutions to resolve it would help to prioritize the issue.