Decouple HTTP retries and error mapping mechanics from `DatabricksClient` into `httpclient.ApiClient` by nfx · Pull Request #699 · databricks/databricks-sdk-go

nfx · 2023-11-18T17:24:12Z

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 Allow injection of HTTP transport to enable HTTP replayer pattern #697, we can properly unit-test the implementations of Azure Client Secret and MSI flows

This is required for downstream tasks like:

Better handling of AAD errors #240
[FEATURE] Support for Azure AD Workload Identities #566
Add ARM_USE_OIDC auth type #495
proper solution to Retry token refresh for Azure SPN in case of Temporarily throttled, too many requests #321 instead of hack in d5e57f7

Tests

Only one test was failing in TF provider, fixed in Fixed TestWorkspace_WaitForResolve_Failure by checking the unwrapped *net.DNSError terraform-provider-databricks#2957

codecov-commenter · 2023-11-20T14:16:51Z

Codecov Report

Attention: 60 lines in your changes are missing coverage. Please review.

Comparison is base (fe6b768) 15.77% compared to head (c3188f4) 16.33%.

Files	Patch %	Lines
httpclient/api_client.go	89.71%	12 Missing and 6 partials ⚠️
httpclient/response.go	73.80%	7 Missing and 4 partials ⚠️
httpclient/request.go	89.15%	7 Missing and 2 partials ⚠️
client/client.go	82.05%	4 Missing and 3 partials ⚠️
httpclient/errors.go	65.00%	5 Missing and 2 partials ⚠️
config/azure.go	0.00%	6 Missing ⚠️
config/auth_azure_client_secret.go	0.00%	1 Missing ⚠️
config/auth_azure_msi.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #699      +/-   ##
==========================================
+ Coverage   15.77%   16.33%   +0.55%     
==========================================
  Files          94       98       +4     
  Lines       13880    13995     +115     
==========================================
+ Hits         2190     2286      +96     
- Misses      11506    11518      +12     
- Partials      184      191       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nkuln

(drive-by) Just did a quick skim through. Although looks like most of the corner case handlings (that I have questions about) are moved from a different package that is time-tested in TF provider so that should be good.

httpclient/api_client.go

tanmay-db

LGTM

config/azure.go

httpclient/api_client.go

httpclient/api_client_test.go

httpclient/api_client.go

mgyucht

Overall LGTM, thanks for doing this! Couple nits and suggestions.

httpclient/api_client.go

httpclient/body_logger.go

httpclient/request.go

nfx · 2023-11-24T12:53:46Z

testing TF provider with c3188f4:

 go get github.com/databricks/databricks-sdk-go@c3188f42c1913f1f1fc30d8bac82fc9944418809
go: upgraded github.com/databricks/databricks-sdk-go v0.25.0 => v0.25.1-0.20231124124904-c3188f42c191
go: upgraded google.golang.org/api v0.150.0 => v0.151.0
➜  terraform-provider-databricks git:(test/go-sdk-http-client) ✗ go mod tidy
go: downloading github.com/jhump/protoreflect v1.15.1
go: downloading github.com/bufbuild/protocompile v0.4.0
➜  terraform-provider-databricks git:(test/go-sdk-http-client) ✗ make test                                                                              
✓ Filling vendor folder with library code ...
✓ Linting source code with https://staticcheck.io/ ...

...

=== RUN   TestWorkspace_WaitForResolve_Failure
    resource_mws_workspaces_test.go:1035: 
                Error Trace:    /Users/serge.smertin/git/databricks/terraform-provider-databricks/mws/resource_mws_workspaces_test.go:1035
                                                        /Users/serge.smertin/git/databricks/terraform-provider-databricks/qa/testing.go:534
                                                        /Users/serge.smertin/git/databricks/terraform-provider-databricks/mws/resource_mws_workspaces_test.go:1029
                Error:          Expected value not to be nil.
                Test:           TestWorkspace_WaitForResolve_Failure

Only TestWorkspace_WaitForResolve_Failure was failing, which is fixed by databricks/terraform-provider-databricks#2957

…ere IMDS doesn't give host environment information (#700) ## Changes This PR allows determining Azure Environment from a Databricks account or workspace hostname, removing the need for a separate configuration/environment variable and complexities related to Azure MSI from within ACR. Similar functionality in Python SDK: - databricks/databricks-sdk-py#390 Stacked on top of: - #699 ## Tests  - [x] `make test` passing - [x] `make fmt` applied - [ ] relevant integration tests applied

Major changes: * There has been a major overhaul of error handling. Users can now compare errors in API responses to the well-known error responses defined in the `apierr` package and reexported in the `databricks` package. Users can check whether a specific error was returned, for example `errors.Is(err, databricks.ErrResourceAlreadyExists)`, rather than converting the error to `*APIError` to check the status code and error code. This change is backwards-compatible; users do not need to modify existing error-handling code when upgrading the SDK. See [#682](#682) and [#703](#703) for the changes and https://github.com/databricks/databricks-sdk-go/blob/main/error_alias.go for the full set of errors. Bug fixes: * Handle "no configuration file found at" error during databricks-cli authentication ([#707](#707)). * Introduce `DatabricksEnvironment` and fix Azure MSI auth from ACR, where IMDS doesn't give host environment information ([#700](#700)). * Fix SCIM Pagination default parameters in the Go SDK ([#717](#717)). Other changes: * Update `slog` example with the correct interface ([#694](#694)). * Fixed typo in error message for unknown azure environment ([#701](#701)). * Allow injection of HTTP transport to enable HTTP replayer pattern ([#697](#697)). * Decouple HTTP retries and error mapping mechanics from `DatabricksClient` into `httpclient.ApiClient` ([#699](#699), [#702](#702), [#712](#712)). * Port `qa.HTTPFixtures` to faster transport-level stubs ([#708](#708)). API Changes: * Removed `EnableOptimization` method for [w.Metastores](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#MetastoresAPI) workspace-level service. * Added `PipelineId` field for [catalog.TableInfo](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#TableInfo). * Added `EnablePredictiveOptimization` field for [catalog.UpdateCatalog](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#UpdateCatalog) and [catalog.UpdateSchema](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#UpdateSchema). * Removed [catalog.UpdatePredictiveOptimization](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#UpdatePredictiveOptimization) and [catalog.UpdatePredictiveOptimizationResponse](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#UpdatePredictiveOptimizationResponse). * Added `Description` field for [jobs.CreateJob](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/jobs#CreateJob) and [jobs.JobSettings](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/jobs#JobSettings). * Added `ListNetworkConnectivityConfigurations` and `ListPrivateEndpointRules` method for [a.NetworkConnectivity](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/settings#NetworkConnectivityAPI) account-level service. * Added [settings.ListNccAzurePrivateEndpointRulesResponse](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/settings#ListNccAzurePrivateEndpointRulesResponse), [settings.ListNetworkConnectivityConfigurationsRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/settings#ListNetworkConnectivityConfigurationsRequest), [settings.ListNetworkConnectivityConfigurationsResponse](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/settings#ListNetworkConnectivityConfigurationsResponse), and [settings.ListPrivateEndpointRulesRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/settings#ListPrivateEndpointRulesRequest). * Added `StringSharedAs` field for [sharing.SharedDataObject](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/sharing#SharedDataObject). Internal changes: * Added `contains` method in OpenAPI Generator ([#690](#690)). * Skip recipients tests in Azure ([#692](#692)). * Allow Files API tests to run in UC environments ([#695](#695)). * More cleanup in Unity Catalog integration test ([#719](#719)). OpenAPI SHA: 22f09783eb8a84d52026f856be3b2068f9498db3, Date: 2023-11-23 Dependency updates: * Bump golang.org/x/oauth2 from 0.13.0 to 0.14.0 ([#689](#689)). * Bump google.golang.org/api from 0.150.0 to 0.151.0 ([#698](#698)). * Bump the OpenAPI Spec ([#706](#706)). * Bump golang.org/x/oauth2 from 0.14.0 to 0.15.0 ([#715](#715)). * Bump golang.org/x/time from 0.4.0 to 0.5.0 ([#714](#714)). * Bump google.golang.org/api from 0.151.0 to 0.152.0 ([#716](#716)).

nfx requested review from mgyucht and tanmay-db November 18, 2023 17:24

nkuln approved these changes Nov 20, 2023

View reviewed changes

httpclient/api_client.go Show resolved Hide resolved

httpclient/api_client.go Show resolved Hide resolved

nfx mentioned this pull request Nov 21, 2023

Introduce DatabricksEnvironment and fix Azure MSI auth from ACR, where IMDS doesn't give host environment information #700

Merged

3 tasks

tanmay-db approved these changes Nov 21, 2023

View reviewed changes

config/azure.go Show resolved Hide resolved

config/azure.go Show resolved Hide resolved

httpclient/api_client.go Show resolved Hide resolved

httpclient/api_client_test.go Show resolved Hide resolved

httpclient/api_client.go Show resolved Hide resolved

mgyucht approved these changes Nov 22, 2023

View reviewed changes

Refactor HTTP client

0b52c35

nfx force-pushed the httpclient branch from 15ad5bc to 0b52c35 Compare November 24, 2023 11:39

nfx added 2 commits November 24, 2023 13:01

refactor tests

fa7c2fb

cleanup

c3188f4

This was referenced Nov 24, 2023

Port qa.HTTPFixtures to faster transport-level stubs #708

Merged

Fixed TestWorkspace_WaitForResolve_Failure by checking the unwrapped *net.DNSError databricks/terraform-provider-databricks#2957

Closed

nfx added this pull request to the merge queue Nov 24, 2023

Merged via the queue into main with commit b0f3c83 Nov 24, 2023

nfx deleted the httpclient branch November 24, 2023 16:45

mgyucht mentioned this pull request Nov 29, 2023

Release v0.26.0 #720

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple HTTP retries and error mapping mechanics from `DatabricksClient` into `httpclient.ApiClient`#699

Decouple HTTP retries and error mapping mechanics from `DatabricksClient` into `httpclient.ApiClient`#699
nfx merged 3 commits intomainfrom
httpclient

nfx commented Nov 18, 2023 •

edited

Loading

Uh oh!

codecov-commenter commented Nov 20, 2023 •

edited

Loading

Uh oh!

nkuln left a comment

Uh oh!

Uh oh!

Uh oh!

tanmay-db left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mgyucht left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nfx commented Nov 24, 2023 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

nfx commented Nov 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Tests

Uh oh!

codecov-commenter commented Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nkuln left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tanmay-db left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mgyucht left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nfx commented Nov 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nfx commented Nov 18, 2023 •

edited

Loading

codecov-commenter commented Nov 20, 2023 •

edited

Loading

nfx commented Nov 24, 2023 •

edited

Loading