Skip to content

Commit

Permalink
Decouple HTTP retries and error mapping mechanics from `DatabricksCli…
Browse files Browse the repository at this point in the history
…ent` into `httpclient.ApiClient` (#699)

## Changes

This PR decouples resiliency of HTTP client (rate limiting, retries,
timeouts, debug logging) from `client.DatabricksClient` and moves it to
a dedicated class, allowing to later use the same robust mechanism to
refresh AAD tokens.

What's changed in `httpclient.ApiClient` compared from
`client.DatabricksClient` (originated in Databricks Terraform Provider
back in the early 2020):
- added native integration with `golang.org/x/oauth2` package for both
using token sources in the request as well as injecting a custom
`*http.Client` the context for token refreshes.
- error mapper is now a pluggable function, default implementation
available
- error retry checking is now pluggable function, default implementation
checks for HTTP 429 and 504
- transient error messages are now configurable per client instance
- HTTP request visitors are now configurable per client instance
- Explicit and coupled `Do(ctx context.Context, method, path string,
headers map[string]string, request, response any, visitors
...func(*http.Request) error) error` became concise and flexible `func
(c *ApiClient) Do(ctx context.Context, method, path string, opts
...DoOption) error`. Headers can now be specified with
`httpclient.WithHeaders(headers)`, JSON deserialisation can now be
specified with `httpclient.WithUnmarshal(&entity)`.
- added `httpclient.WithCaptureHeader(key, &value)` to assign headers to
a string pointer. This is required to get a proper integration with
Azure Container Registry Managed Identity passwordless setup.
- body can be specified as `httpclient.WithBody(any)`, that internally
sets a special data field. Technically, this could have been a request
visitor, but we have special logic for "debug bytes" logging, which is
out of scope of this PR change.
- removed flawed trailing double-forward-slash trimming (`//`), as it
was breaking requests containing full URLs.
- Once we merge the
#697, we can
properly unit-test the implementations of Azure Client Secret and MSI
flows

This is required for downstream tasks like:
- #240
- #566
- #495
- proper solution to
#321 instead of
hack in
d5e57f7
- 
## Tests

- [x] Only one test was failing in TF provider, fixed in
databricks/terraform-provider-databricks#2957
  • Loading branch information
nfx authored Nov 24, 2023
1 parent fe6b768 commit b0f3c83
Show file tree
Hide file tree
Showing 18 changed files with 1,646 additions and 1,113 deletions.
2 changes: 1 addition & 1 deletion apierr/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ func GenericIOError(ue *url.Error) *APIError {
}

// GetAPIError inspects HTTP errors from the Databricks API for known transient errors.
func GetAPIError(ctx context.Context, resp *http.Response, body io.ReadCloser) *APIError {
func GetAPIError(ctx context.Context, resp *http.Response, body io.ReadCloser) error {
if resp.StatusCode == 429 {
return TooManyRequests()
}
Expand Down
Loading

0 comments on commit b0f3c83

Please sign in to comment.