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

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

Closed
ajith-sub opened this issue May 2, 2023 · 8 comments
Closed

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

ajith-sub opened this issue May 2, 2023 · 8 comments

Comments

@ajith-sub
Copy link
Contributor

ajith-sub commented May 2, 2023

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor

srenatus commented May 5, 2023

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 added a commit to ajith-sub/opa that referenced this issue May 8, 2023
OPA currently doesn't support wrapping errors returned by builtin
functions. This change modifies topdown.Error to include a wrapped
error so that additional context about the builtin error is available.

Fixes open-policy-agent#5890

Signed-off-by: Ajith Subramanian <ajithsub@gmail.com>
ajith-sub added a commit to ajith-sub/opa that referenced this issue May 8, 2023
OPA currently doesn't support wrapping errors returned by builtin
functions. This change modifies topdown.Error to include a wrapped
error so that additional context about the builtin error is available.

Fixes open-policy-agent#5890

Signed-off-by: Ajith Subramanian <ajithsub@gmail.com>
ajith-sub added a commit to ajith-sub/opa that referenced this issue May 8, 2023
OPA currently doesn't support wrapping errors returned by builtin
functions. This change modifies topdown.Error to include a wrapped
error so that additional context about the builtin error is available.

Fixes open-policy-agent#5890

Signed-off-by: Ajith Subramanian <ajithsub@gmail.com>
ajith-sub added a commit to ajith-sub/opa that referenced this issue May 9, 2023
OPA currently doesn't support wrapping errors returned by builtin
functions. This change modifies topdown.Error to include a wrapped
error so that additional context about the builtin error is available.

Fixes open-policy-agent#5890

Signed-off-by: Ajith Subramanian <ajith_subramanian@trimble.com>
@ajith-sub
Copy link
Contributor Author

Thanks for the feedback.

I've submitted a draft PR with a first pass of changes required to support this: #5909

@stale
Copy link

stale bot commented Jun 8, 2023

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.

@stale stale bot added the inactive label Jun 8, 2023
ashutosh-narkar pushed a commit to ajith-sub/opa that referenced this issue Jun 8, 2023
OPA currently doesn't support wrapping errors returned by builtin
functions. This change modifies topdown.Error to include a wrapped
error so that additional context about the builtin error is available.

Fixes open-policy-agent#5890

Signed-off-by: Ajith Subramanian <ajith_subramanian@trimble.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants