topdown/http: handle http.send errors in-band

[ashutosh-narkar]

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

[tsandall]

I think the implementation could be a improved a bit.

I'm wondering about the format of the error in the result...as a user, looking at the docs and even the test cases, it was unclear how I'm supposed to consume the error--it's just a string and the content could be pretty much anything. I wonder if we should make the error a bit more structured and indicate the different kinds of failures, e.g., a bad parameter is essentially an internal error whereas a network error could be distinguished. Something like:

"error": {
   "code": "eval_http_send_internal_error",
   "message": "..."
}

"error": {
   "code": "eval_http_send_network_error",
   "message": "..."
}

@patrick-east thoughts?

[tsandall]

Nit: the description is a bit ambiguous; presumably the status_code is only zero if there was some unexpected error, right?

[ashutosh-narkar]

Yes. Updated the description.

[tsandall]

I think this code can be simplified quite a bit: https://gist.github.com/tsandall/deeaff535f9eaae5a55559067611473e

There are a couple red flags:

• the httpSendErrType variable is a bit brittle. E.g., if we add another error path in the future, we need to make sure to set the variable accordingly; if we forget, the built-in won't behave correctly.

• the defer block is re-executing the iterator; this means the search after this expression will be re-evaluated unnecessarily

• generally, appending "helper" to a function name is an indicator that the function is doing too much--in this case, the solution is to split the iterator invocation from the http request execution--that avoids the need for the defer and the boolean.

[ashutosh-narkar]

Refactored the code as per your comment.

[ashutosh-narkar]

I think the implementation could be a improved a bit.

I'm wondering about the format of the error in the result...as a user, looking at the docs and even the test cases, it was unclear how I'm supposed to consume the error--it's just a string and the content could be pretty much anything. I wonder if we should make the error a bit more structured and indicate the different kinds of failures, e.g., a bad parameter is essentially an internal error whereas a network error could be distinguished. Something like:

"error": {
   "code": "eval_http_send_internal_error",
   "message": "..."
}

"error": {
   "code": "eval_http_send_network_error",
   "message": "..."
}

Updated the error in the response to be an object containing the error code and message.

[tsandall]

LGTM