Fix error dropping when making requests

[bigkevmcd]

This removes the custom ErrorHandler configuration.

Currently, if there's an error trying to connect to the upstream API, the actual error message is dropped, and it falls back to the underlying Go client which warns that the Transport returned no error and no response.

After it's exhausted the retries, if the ErrorHandler is configured, it simply returns whatever the ErrorHandler returns https://github.com/hashicorp/go-retryablehttp/blob/main/client.go#L749-L751

If there's no response (perhaps because it can't connect) at all, RetryableErrorHandler was returning nil, nil which the Go client https://github.com/golang/go/blob/2e1003e2f7e42efc5771812b9ee6ed264803796c/src/net/http/client.go#L276 will complain about, and, the error message is dropped, so no way to diagnose the issue.

[manicminer]

Hi @bigkevmcd, thanks for suggesting this. This function was added in order to preserve the response in the event that an error occurred making the request. By the time the ErrorHandler is called, the retry logic has already been exhausted and retryablehttp.Do() is preparing to return.

There are several points of failure-handling here:

• Our handling in the base client inline CheckRetry function
• Any per-operation handling in individual API clients, which is passed into the request as a ConsistencyFailureFunc
• Any generic handling by inspecting the headers in go-retryablehttp
• Any actual errors raised by net/http and/or the RoundTripper provided by go-retryablehttp

If we don't provide an ErrorHandler function (as proposed here), we lose the response body due to this block in go-retryablehttp.Do()`, and thus are unable to parse the response to look for eventual consistency related errors.

If our ErrorHandler func returns the response and the error as-is (just like the provided PassthroughErrorHandler in go-retryablehttp), we get to keep the response body, however if err is not nil, due to this block in the send() function from net/http, then we still lose the response body.

However, I believe we can improve on this by modifying the RetryableErrorHandler() function so that it does return the error, if the response is nil.

// RetryableErrorHandler ensures that the response is returned after exhausting retries for a request
// If the response is not nil, we can't return an error here, or net/http will not return the response
func RetryableErrorHandler(resp *http.Response, err error, numTries int) (*http.Response, error) {
	if resp == nil {
		return nil, err
	}
	return resp, nil
}

[bigkevmcd]

@manicminer thanks for the response, not long after I opened this I wondered about that case.

I've added an additional test which validates both paths, and restored the RetryableErrorHandler to handle the other case.

[manicminer]

Thanks @bigkevmcd, this looks good, and thanks for adding the tests!

[manicminer]

Suggested change

	func TestClient_Get_response(t *testing.T) {
	func TestClient_GetWithResponse(t *testing.T) {

[bigkevmcd]

I'm pretty sure this isn't connected to my change 😭

=== RUN   TestConnectedOrganizationClient
2024/03/28 16:59:07 [DEBUG] POST https://login.microsoftonline.com/6df54acb-f3cd-4734-85e3-7511ade57a02/oauth2/v2.0/token
    connectedorganization_test.go:141: ConnectedOrganizationClient.AddExternalSponsorGroup(): AddExternalSponsorUser returned status code 400: unexpected status 400 with OData error: ArgumentException: The URI is invalid. It either doesn't contain a key or has an unsupported resource.
--- FAIL: TestConnectedOrganizationClient (2.27s)

[manicminer]

@bigkevmcd Don't worry, that's an unrelated flapping test I need to look at