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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Nov 28, 2023
Merged

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

merged 1 commit into from
Nov 28, 2023

Conversation

nfx
Copy link
Contributor

@nfx nfx commented Nov 20, 2023 •
edited
Loading

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:

Stacked on top of:

Tests

  • make test passing
  • make fmt applied
  • relevant integration tests applied

@nfx nfx requested review from mgyucht and tanmay-db November 20, 2023 15:22
@codecov-commenter
Copy link

codecov-commenter commented Nov 20, 2023 •
edited
Loading

Codecov Report

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

Comparison is base (b0f3c83) 16.33% compared to head (70fb589) 16.39%.

Files Patch % Lines
config/environments.go 52.17% 9 Missing and 2 partials ⚠️
config/auth_azure_client_secret.go 0.00% 7 Missing ⚠️
config/auth_azure_msi.go 0.00% 4 Missing ⚠️
config/azure.go 0.00% 4 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #700      +/-   ##
==========================================
+ Coverage   16.33%   16.39%   +0.06%     
==========================================
  Files          98       99       +1     
  Lines       13995    13974      -21     
==========================================
+ Hits         2286     2291       +5     
+ Misses      11518    11492      -26     
  Partials      191      191

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

@nfx nfx mentioned this pull request Nov 21, 2023
Merged
3 tasks
mgyucht
mgyucht reviewed Nov 22, 2023
View reviewed changes
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.

Couple small suggestions, but mostly looking good. Thanks!

config/environments.go Outdated Show resolved Hide resolved
config/environments.go Outdated Show resolved Hide resolved
config/environments.go Outdated Show resolved Hide resolved
config/environments.go Outdated Show resolved Hide resolved
config/config.go Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
Base automatically changed from httpclient to main November 24, 2023 16:45
@nfx nfx force-pushed the feat/envs branch 2 times, most recently from 9a93959 to 18f31cf Compare November 24, 2023 17:33
@nfx nfx removed the do-not-merge label Nov 24, 2023
@nfx nfx requested a review from mgyucht November 24, 2023 18:26
@nfx
Introduce DatabricksEnvironment and fix Azure MSI auth from ACR, wh…
70fb589 
…ere IMDS doesn't give host environment information
This was referenced Nov 24, 2023
Merged
Closed
mgyucht
mgyucht approved these changes Nov 28, 2023
View reviewed changes
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.

Thanks for doing this!

config/azure.go
@@ -20,73 +19,45 @@ type azureEnvironment struct {
// based on github.com/Azure/go-autorest/autorest/azure/azureEnvironments.go
var (
publicCloud = azureEnvironment{
Name: "AzurePublicCloud",
Name: "PUBLIC",
Copy link
Contributor

Choose a reason for hiding this comment

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

Just looking from https://github.com/hashicorp/terraform-provider-azurerm/blob/79b4740980de5c34902d592e44a055c2ec1dce7c/internal/provider/provider.go#L211, is this where you got these names from?

@mgyucht mgyucht added this pull request to the merge queue Nov 28, 2023
Merged via the queue into main with commit 832d554 Nov 28, 2023
4 checks passed
@mgyucht mgyucht deleted the feat/envs branch November 28, 2023 12:46
mgyucht added a commit that referenced this pull request Nov 29, 2023
@mgyucht
Release v0.26.0
37dcb6b 
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
Merged
github-merge-queue bot pushed a commit that referenced this pull request Nov 29, 2023
@mgyucht
Release v0.26.0 (#720)
1813fa6 
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)).
github-merge-queue bot pushed a commit that referenced this pull request Dec 1, 2023
@nfx
Migrate Azure MSI & Metadata Service token sources to httpclient an…
82d089a 
…d add 100% test coverage (#709)

This PR improves the stability for Azure MSI authentication by adopting
the httpclient transport.

Needs two PRs merged first:
- #708
- #700
@mgyucht mgyucht mentioned this pull request Feb 1, 2024
Merged
github-merge-queue bot pushed a commit to databricks/databricks-sdk-java that referenced this pull request Feb 2, 2024
@mgyucht
Make new workspace client from account client (#218)
dc07492 
## Changes
Ported to the Java SDK from
https://github.com/databricks/databricks-sdk-go/pull/792/files and
databricks/databricks-sdk-go#700.

## Tests
<!-- How is this tested? -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgyucht mgyucht mgyucht approved these changes

@tanmay-db tanmay-db Awaiting requested review from tanmay-db

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nfx @codecov-commenter @mgyucht