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

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

Merged
merged 3 commits into from
Nov 24, 2023

Conversation

nfx
Copy link
Contributor

@nfx nfx commented Nov 18, 2023

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:

Tests

@nfx nfx requested review from mgyucht and tanmay-db November 18, 2023 17:24
@codecov-commenter
Copy link

codecov-commenter commented Nov 20, 2023

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.

Copy link

@nkuln nkuln left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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 Show resolved Hide resolved
httpclient/api_client.go Show resolved Hide resolved
Copy link
Contributor

@tanmay-db tanmay-db left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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
Copy link
Contributor

@mgyucht mgyucht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

httpclient/api_client.go Outdated Show resolved Hide resolved
httpclient/api_client.go Show resolved Hide resolved
httpclient/api_client.go Show resolved Hide resolved
httpclient/api_client.go Show resolved Hide resolved
httpclient/api_client.go Show resolved Hide resolved
httpclient/body_logger.go Show resolved Hide resolved
httpclient/request.go Outdated Show resolved Hide resolved
@nfx
Copy link
Contributor Author

nfx commented Nov 24, 2023

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

@nfx nfx added this pull request to the merge queue Nov 24, 2023
Merged via the queue into main with commit b0f3c83 Nov 24, 2023
4 checks passed
@nfx nfx deleted the httpclient branch November 24, 2023 16:45
github-merge-queue bot pushed a commit that referenced this pull request Nov 28, 2023
…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
<!-- 
How is this tested? Please see the checklist below and also describe any
other relevant tests
-->

- [x] `make test` passing
- [x] `make fmt` applied
- [ ] relevant integration tests applied
mgyucht added a commit that referenced this pull request Nov 29, 2023
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)).
@mgyucht mgyucht mentioned this pull request Nov 29, 2023
github-merge-queue bot pushed a commit that referenced this pull request Nov 29, 2023
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)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants