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

Handle http.send errors in-band #2187

Closed
jpeach opened this issue Mar 16, 2020 · 10 comments · Fixed by #2763
Closed

Handle http.send errors in-band #2187

jpeach opened this issue Mar 16, 2020 · 10 comments · Fixed by #2763
Assignees

Comments

@jpeach
Copy link
Contributor

jpeach commented Mar 16, 2020

Expected Behavior

Rego programs should be able to handle errors from the builtin http.send.

Actual Behavior

If the http.send builtin gets an error, this is returned from the (*Rego) Eval() and can't be handled within a Rego program.

In my context, I have a test harness that queries Rego rules. These rules might be expressed using http.send. If the Eval fails because of the HTTP client failure, then I will retry (up til the timeout), but the Eval might fail for some other reason that's permanently fatal (e.g syntax). The rules already handle expecting HTTP status, so I'd like to be able to handle the builtin failing in a consistent way.

@patrick-east
Copy link
Contributor

I think part of the issue is that most of the builtin functions today lack any sort of error handling mechanism. They'll either do the right thing, be undefined, or halt evaluation by raising an error.

We could maybe just modify the http.send builtin to have an option for like raising the error versus returning some error status for the request. That might be pretty low impact, especially since the signature of the function lends itself to this by having the return value be an object.

Longer term we may want to think about what could be done for other builtin functions that require (or would be nice to have) similar error handling treatment. Having a consistent pattern would be ideal.

@jpeach
Copy link
Contributor Author

jpeach commented Mar 16, 2020

Yep, I agree that propagating the error from most builtins makes sense. I think that http.send is an outlier in this case. I'll be investigating handling the BuiltinErr in my code and retrying, but wanted to make sure there was an issue for this :)

@jpeach
Copy link
Contributor Author

jpeach commented Apr 5, 2020

I found a case that I can't easily work around. I have a test suite where the test cases are expressed in Rego. One test case is that TLS is configured properly. If it is configured properly, the HTTPS request fails with a TLS alert. I can't handle this properly because from the test harness there is no way to know whether the Rego check expects the error or not. The harness also doesn't know that it is an error to not get the alert.

@jpeach
Copy link
Contributor Author

jpeach commented Apr 5, 2020

Proposal:

  • If the response fails, set the status_code field to 0
  • Capture the error message, either
    • capture the string in a new error field
    • capture the string in the raw_body field

@jpeach
Copy link
Contributor Author

jpeach commented Apr 15, 2020

@patrick-east Do you have any thoughts about the proposal above? If we do that, would we need extra flags to switch the behavior?

@patrick-east
Copy link
Contributor

Apologies for the delay! Unfortunately this fell off my radar 😞

I like the proposal (would lean towards the new error field) and yes, I think a new flag to switch behavior is probably the best way forward. In this case I would guess it would be too much of a backwards incompatible change if we do this for the default behavior.

@jpeach
Copy link
Contributor Author

jpeach commented Apr 17, 2020

Apologies for the delay! Unfortunately this fell off my radar 😞

I like the proposal (would lean towards the new error field) and yes, I think a new flag to switch behavior is probably the best way forward. In this case I would guess it would be too much of a backwards incompatible change if we do this for the default behavior.

Thinking out loud a bit ...

Rather than a per-request flag which seems burdensome, could we make a builtin API to specify HTTP client behaviors? Maybe it would be enough to expose the setting only in the Go API, and not force Rego police authors to toggle it ...

@tsandall
Copy link
Member

Adding another parameter on the request object doesn't feel like a huge deal to me. If policy authors find it annoying they could always wrap http.send with their own function in Rego.

That said, I'm not opposed to another version of http.send as long as we keep code duplication to a minimum, in fact, I could imagine extending http.send with the parameter anyway and then just implementing this new one as a wrapper.

@tsandall
Copy link
Member

Adding this into the backlog as something we need to work on. I'm wondering if we should just change the default behaviour to return network errors as data. I doubt anyone is relying on network errors halting policy evaluation.

@brainerazer
Copy link

I think that http.send is an outlier in this case

We have a separate case too: JWT parsing builtin. It fails the eval for us, but we would probably like to return a more meaningful error / go into a different auth flow - but right now it just crashes the eval and returns 500

ashutosh-narkar added a commit to ashutosh-narkar/opa that referenced this issue Oct 9, 2020
This change updates how errors from http.send are handled.
By default, an error returned by `http.send` halts the policy evaluation.
This commit allows users to return errors in the response object
returned by `http.send` instead of halting evaluation.

Fixes: open-policy-agent#2187

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
ashutosh-narkar added a commit that referenced this issue Oct 9, 2020
This change updates how errors from http.send are handled.
By default, an error returned by `http.send` halts the policy evaluation.
This commit allows users to return errors in the response object
returned by `http.send` instead of halting evaluation.

Fixes: #2187

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants