From 0bb670680b57f9dc2b93937571abb2bace81315e Mon Sep 17 00:00:00 2001 From: Serge Smertin Date: Thu, 10 Sep 2020 18:12:50 +0200 Subject: [PATCH] Fix #294 - Reliable indication of Azure * DatabricksClient.IsAzure is checking for both resource id and workspace hostname * Added more unit test coverage for ClustersAPI * FIxed version linking --- .goreleaser.yml | 2 +- Makefile | 10 +- common/client.go | 6 +- compute/clusters.go | 8 +- compute/clusters_test.go | 515 +++++++++++++++++++++++++ compute/common_instances.go | 2 +- compute/model.go | 5 + compute/resource_instance_pool_test.go | 4 +- internal/qa/testing.go | 17 +- provider/provider.go | 4 +- provider/provider_test.go | 118 +++--- 11 files changed, 623 insertions(+), 68 deletions(-) diff --git a/.goreleaser.yml b/.goreleaser.yml index 86ef57506b..c341a65e5a 100644 --- a/.goreleaser.yml +++ b/.goreleaser.yml @@ -10,7 +10,7 @@ builds: binary: terraform-provider-databricks_{{ .Tag }} ldflags: # sets client version, not the default main.version - - '-X "common.version={{ replace .Version "v" "" }}"' + - '-X github.com/databrickslabs/databricks-terraform/common.version={{ replace .Version "v" "" }}' goos: - windows - linux diff --git a/Makefile b/Makefile index 7f7b14643f..4abfb03b89 100644 --- a/Makefile +++ b/Makefile @@ -18,23 +18,25 @@ coverage: test @echo "✓ Opening coverage for unit tests..." @go tool cover -html=coverage.txt +VERSION = $(shell git describe --long --always | sed 's/v//') + build: @echo "✓ Building source code with go build..." - @go build -mod vendor -v -ldflags="-X 'common.version=$(git describe --long --always | sed 's/v//')'" -o terraform-provider-databricks + @go build -mod vendor -v -ldflags="-X github.com/databrickslabs/databricks-terraform/common.version=${VERSION}" -o terraform-provider-databricks install: build @echo "✓ Installing provider into ~/.terraform.d/plugins ..." @test -d $(HOME)/.terraform.d/plugins && rm $(HOME)/.terraform.d/plugins/terraform-provider-databricks* || mkdir -p $(HOME)/.terraform.d/plugins @cp terraform-provider-databricks $(HOME)/.terraform.d/plugins - @mkdir -p '$(HOME)/.terraform.d/plugins/registry.terraform.io/databrickslabs/databricks/$(shell git describe --long --always | sed 's/v//')/$(shell go version | awk '{print $$4}' | sed 's#/#_#')' - @cp terraform-provider-databricks '$(HOME)/.terraform.d/plugins/registry.terraform.io/databrickslabs/databricks/$(shell git describe --long --always | sed 's/v//')/$(shell go version | awk '{print $$4}' | sed 's#/#_#')' + @mkdir -p '$(HOME)/.terraform.d/plugins/registry.terraform.io/databrickslabs/databricks/${VERSION}/$(shell go version | awk '{print $$4}' | sed 's#/#_#')' + @cp terraform-provider-databricks '$(HOME)/.terraform.d/plugins/registry.terraform.io/databrickslabs/databricks/${VERSION}/$(shell go version | awk '{print $$4}' | sed 's#/#_#')' @echo "✓ Use the following configuration to enable the version you've built" @echo @echo "terraform {" @echo " required_providers {" @echo " databricks = {" @echo ' source = "databrickslabs/databricks"' - @echo ' version = "$(shell git describe --long --always | sed 's/v//')"' + @echo ' version = "${VERSION}"' @echo " }" @echo " }" @echo "}" diff --git a/common/client.go b/common/client.go index 96e0df5af2..0b5ae7130d 100644 --- a/common/client.go +++ b/common/client.go @@ -210,7 +210,7 @@ func (c *DatabricksClient) configureHTTPCLient() { } } -// IsUsingAzureAuth returns true if Azure authentication is configured, this is not a valid check to determine if we are running on Azure -func (c *DatabricksClient) IsUsingAzureAuth() bool { - return c.AzureAuth.resourceID() != "" +// IsAzure returns true if client is configured for Azure Databricks - either by using AAD auth or with host+token combination +func (c *DatabricksClient) IsAzure() bool { + return c.AzureAuth.resourceID() != "" || strings.Contains(c.Host, "azuredatabricks.net") } diff --git a/compute/clusters.go b/compute/clusters.go index 01787162c6..a7e6e3f755 100644 --- a/compute/clusters.go +++ b/compute/clusters.go @@ -230,9 +230,7 @@ func (a ClustersAPI) Unpin(clusterID string) error { // up to 70 of the most recently terminated interactive clusters in the past 30 days, // and up to 30 of the most recently terminated job clusters in the past 30 days func (a ClustersAPI) List() ([]ClusterInfo, error) { - var clusterList = struct { - Clusters []ClusterInfo `json:"clusters,omitempty" url:"clusters,omitempty"` - }{} + var clusterList ClusterList err := a.client.Get("/clusters/list", nil, &clusterList) return clusterList.Clusters, err } @@ -281,7 +279,7 @@ func (a ClustersAPI) GetOrCreateRunningCluster(name string, custom ...Cluster) ( NodeTypeID: smallestNodeType, AutoterminationMinutes: 10, } - if !a.client.IsUsingAzureAuth() { + if !a.client.IsAzure() { r.AwsAttributes = &AwsAttributes{ Availability: "SPOT", } @@ -296,7 +294,7 @@ func (a ClustersAPI) GetOrCreateRunningCluster(name string, custom ...Cluster) ( func (a ClustersAPI) GetSmallestNodeTypeWithStorage() string { nodeTypes, err := a.ListNodeTypes() if err != nil || len(nodeTypes) == 0 { - if a.client.IsUsingAzureAuth() { + if a.client.IsAzure() { return "Standard_D3_v2" } return "i3.xlarge" diff --git a/compute/clusters_test.go b/compute/clusters_test.go index 4c7f188537..051cc4e290 100644 --- a/compute/clusters_test.go +++ b/compute/clusters_test.go @@ -9,8 +9,523 @@ import ( "github.com/databrickslabs/databricks-terraform/common" "github.com/databrickslabs/databricks-terraform/internal/qa" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +func TestGetOrCreateRunningCluster_AzureAuth(t *testing.T) { + client, server, err := qa.HttpFixtureClient(t, []qa.HTTPFixture{ + { + Method: "GET", + Resource: "/api/2.0/clusters/list", + Response: map[string]interface{}{}, + }, + { + Method: "GET", + ReuseRequest: true, + Resource: "/api/2.0/clusters/list-node-types", + Response: NodeTypeList{ + []NodeType{ + { + NodeTypeID: "Standard_F4s", + InstanceTypeID: "Standard_F4s", + MemoryMB: 8192, + NumCores: 4, + NodeInstanceType: &NodeInstanceType{ + LocalDisks: 1, + InstanceTypeID: "Standard_F4s", + LocalDiskSizeGB: 16, + LocalNVMeDisks: 0, + }, + }, + { + NodeTypeID: "Standard_L80s_v2", + InstanceTypeID: "Standard_L80s_v2", + MemoryMB: 655360, + NumCores: 80, + NodeInstanceType: &NodeInstanceType{ + LocalDisks: 2, + InstanceTypeID: "Standard_L80s_v2", + LocalDiskSizeGB: 160, + LocalNVMeDisks: 1, + }, + }, + }, + }, + }, + { + Method: "POST", + Resource: "/api/2.0/clusters/create", + ExpectedRequest: Cluster{ + AutoterminationMinutes: 10, + ClusterName: "mount", + NodeTypeID: "Standard_F4s", + NumWorkers: 1, + SparkVersion: CommonRuntimeVersion(), + }, + Response: ClusterID{ + ClusterID: "bcd", + }, + }, + { + Method: "GET", + Resource: "/api/2.0/clusters/get?cluster_id=bcd", + Response: ClusterInfo{ + State: "RUNNING", + }, + }, + }) + defer server.Close() + require.NoError(t, err) + + client.AzureAuth.ResourceID = "/a/b/c" + + clusterInfo, err := NewClustersAPI(client).GetOrCreateRunningCluster("mount") + require.NoError(t, err) + + assert.NotNil(t, clusterInfo) +} + +func TestGetOrCreateRunningCluster_Existing_AzureAuth(t *testing.T) { + client, server, err := qa.HttpFixtureClient(t, []qa.HTTPFixture{ + { + Method: "GET", + Resource: "/api/2.0/clusters/list", + Response: ClusterList{ + Clusters: []ClusterInfo{ + { + ClusterID: "abc", + State: "TERMINATED", + AutoterminationMinutes: 10, + ClusterName: "mount", + NodeTypeID: "Standard_F4s", + NumWorkers: 1, + SparkVersion: CommonRuntimeVersion(), + }, + }, + }, + }, + { + Method: "GET", + Resource: "/api/2.0/clusters/get?cluster_id=abc", + Response: ClusterInfo{ + State: "TERMINATED", + }, + }, + { + Method: "POST", + Resource: "/api/2.0/clusters/start", + ExpectedRequest: ClusterID{ + ClusterID: "abc", + }, + }, + { + Method: "GET", + Resource: "/api/2.0/clusters/get?cluster_id=abc", + Response: ClusterInfo{ + State: "RUNNING", + }, + }, + }) + defer server.Close() + require.NoError(t, err) + + client.AzureAuth.ResourceID = "/a/b/c" + + clusterInfo, err := NewClustersAPI(client).GetOrCreateRunningCluster("mount") + require.NoError(t, err) + + assert.NotNil(t, clusterInfo) +} + +func TestWaitForClusterStatus_RetryOnNotFound(t *testing.T) { + client, server, err := qa.HttpFixtureClient(t, []qa.HTTPFixture{ + { + Method: "GET", + Resource: "/api/2.0/clusters/get?cluster_id=abc", + Response: common.APIErrorBody{ + Message: "Nope", + }, + Status: 404, + }, + { + Method: "GET", + Resource: "/api/2.0/clusters/get?cluster_id=abc", + Response: ClusterInfo{ + State: "RUNNING", + }, + }, + }) + defer server.Close() + require.NoError(t, err) + + client.AzureAuth.ResourceID = "/a/b/c" + + clusterInfo, err := NewClustersAPI(client).waitForClusterStatus("abc", ClusterStateRunning) + require.NoError(t, err) + + assert.NotNil(t, clusterInfo) +} + +func TestWaitForClusterStatus_StopRetryingEarly(t *testing.T) { + client, server, err := qa.HttpFixtureClient(t, []qa.HTTPFixture{ + { + Method: "GET", + Resource: "/api/2.0/clusters/get?cluster_id=abc", + Response: common.APIErrorBody{ + Message: "I am a teapot", + }, + Status: 418, + }, + }) + defer server.Close() + require.NoError(t, err) + + _, err = NewClustersAPI(client).waitForClusterStatus("abc", ClusterStateRunning) + require.Error(t, err) + require.Contains(t, err.Error(), "I am a teapot") +} + +func TestWaitForClusterStatus_NotReachable(t *testing.T) { + client, server, err := qa.HttpFixtureClient(t, []qa.HTTPFixture{ + { + Method: "GET", + Resource: "/api/2.0/clusters/get?cluster_id=abc", + Response: ClusterInfo{ + State: ClusterStateUnknown, + StateMessage: "Something strange is going on", + }, + }, + }) + defer server.Close() + require.NoError(t, err) + + client.AzureAuth.ResourceID = "/a/b/c" + + _, err = NewClustersAPI(client).waitForClusterStatus("abc", ClusterStateRunning) + require.Error(t, err) + assert.Contains(t, err.Error(), "abc is not able to transition from UNKNOWN to RUNNING: Something strange is going on.") +} + +func TestWaitForClusterStatus_NormalRetry(t *testing.T) { + client, server, err := qa.HttpFixtureClient(t, []qa.HTTPFixture{ + { + Method: "GET", + Resource: "/api/2.0/clusters/get?cluster_id=abc", + Response: ClusterInfo{ + State: ClusterStatePending, + }, + }, + { + Method: "GET", + Resource: "/api/2.0/clusters/get?cluster_id=abc", + Response: ClusterInfo{ + State: ClusterStateRunning, + }, + }, + }) + defer server.Close() + require.NoError(t, err) + + clusterInfo, err := NewClustersAPI(client).waitForClusterStatus("abc", ClusterStateRunning) + require.NoError(t, err) + assert.Equal(t, ClusterStateRunning, string(clusterInfo.State)) +} + +func TestEditCluster_Pending(t *testing.T) { + client, server, err := qa.HttpFixtureClient(t, []qa.HTTPFixture{ + { + Method: "GET", + Resource: "/api/2.0/clusters/get?cluster_id=abc", + Response: ClusterInfo{ + State: ClusterStatePending, + ClusterID: "abc", + }, + }, + { + Method: "GET", + Resource: "/api/2.0/clusters/get?cluster_id=abc", + Response: ClusterInfo{ + State: ClusterStateRunning, + ClusterID: "abc", + }, + }, + { + Method: "POST", + Resource: "/api/2.0/clusters/edit", + Response: Cluster{ + ClusterID: "abc", + ClusterName: "Morty", + }, + }, + { + Method: "GET", + Resource: "/api/2.0/clusters/get?cluster_id=abc", + Response: ClusterInfo{ + State: ClusterStateRunning, + }, + }, + }) + defer server.Close() + require.NoError(t, err) + + clusterInfo, err := NewClustersAPI(client).Edit(Cluster{ + ClusterID: "abc", + ClusterName: "Morty", + }) + require.NoError(t, err) + assert.Equal(t, ClusterStateRunning, string(clusterInfo.State)) +} + +func TestEditCluster_Terminating(t *testing.T) { + client, server, err := qa.HttpFixtureClient(t, []qa.HTTPFixture{ + { + Method: "GET", + Resource: "/api/2.0/clusters/get?cluster_id=abc", + Response: ClusterInfo{ + State: ClusterStateTerminating, + ClusterID: "abc", + }, + }, + { + Method: "GET", + Resource: "/api/2.0/clusters/get?cluster_id=abc", + Response: ClusterInfo{ + State: ClusterStateTerminated, + ClusterID: "abc", + }, + }, + { + Method: "POST", + Resource: "/api/2.0/clusters/edit", + Response: Cluster{ + ClusterID: "abc", + ClusterName: "Morty", + }, + }, + { + Method: "GET", + Resource: "/api/2.0/clusters/get?cluster_id=abc", + Response: ClusterInfo{ + State: ClusterStateRunning, + }, + }, + }) + defer server.Close() + require.NoError(t, err) + + clusterInfo, err := NewClustersAPI(client).Edit(Cluster{ + ClusterID: "abc", + ClusterName: "Morty", + }) + require.NoError(t, err) + assert.Equal(t, ClusterStateTerminated, string(clusterInfo.State)) +} + +func TestEditCluster_Error(t *testing.T) { + client, server, err := qa.HttpFixtureClient(t, []qa.HTTPFixture{ + { + Method: "GET", + Resource: "/api/2.0/clusters/get?cluster_id=abc", + Response: ClusterInfo{ + State: ClusterStateError, + ClusterID: "abc", + StateMessage: "I am a teapot", + }, + }, + }) + defer server.Close() + require.NoError(t, err) + + _, err = NewClustersAPI(client).Edit(Cluster{ + ClusterID: "abc", + ClusterName: "Morty", + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "I am a teapot") +} + +func TestStartAndGetInfo_Pending(t *testing.T) { + client, server, err := qa.HttpFixtureClient(t, []qa.HTTPFixture{ + { + Method: "GET", + Resource: "/api/2.0/clusters/get?cluster_id=abc", + Response: ClusterInfo{ + State: ClusterStatePending, + ClusterID: "abc", + }, + }, + { + Method: "GET", + Resource: "/api/2.0/clusters/get?cluster_id=abc", + Response: ClusterInfo{ + State: ClusterStateRunning, + ClusterID: "abc", + }, + }, + }) + defer server.Close() + require.NoError(t, err) + + clusterInfo, err := NewClustersAPI(client).StartAndGetInfo("abc") + require.NoError(t, err) + assert.Equal(t, ClusterStateRunning, string(clusterInfo.State)) +} + +func TestStartAndGetInfo_Terminating(t *testing.T) { + client, server, err := qa.HttpFixtureClient(t, []qa.HTTPFixture{ + { + Method: "GET", + Resource: "/api/2.0/clusters/get?cluster_id=abc", + Response: ClusterInfo{ + State: ClusterStateTerminating, + ClusterID: "abc", + }, + }, + { + Method: "GET", + Resource: "/api/2.0/clusters/get?cluster_id=abc", + Response: ClusterInfo{ + State: ClusterStateTerminated, + ClusterID: "abc", + }, + }, + { + Method: "POST", + Resource: "/api/2.0/clusters/start", + ExpectedRequest: ClusterID{ + ClusterID: "abc", + }, + }, + { + Method: "GET", + Resource: "/api/2.0/clusters/get?cluster_id=abc", + Response: ClusterInfo{ + State: ClusterStateRunning, + ClusterID: "abc", + }, + }, + }) + defer server.Close() + require.NoError(t, err) + + clusterInfo, err := NewClustersAPI(client).StartAndGetInfo("abc") + require.NoError(t, err) + assert.Equal(t, ClusterStateRunning, string(clusterInfo.State)) +} + +func TestStartAndGetInfo_Error(t *testing.T) { + client, server, err := qa.HttpFixtureClient(t, []qa.HTTPFixture{ + { + Method: "GET", + Resource: "/api/2.0/clusters/get?cluster_id=abc", + Response: ClusterInfo{ + State: ClusterStateError, + StateMessage: "I am a teapot", + }, + }, + { + Method: "POST", + Resource: "/api/2.0/clusters/start", + ExpectedRequest: ClusterID{ + ClusterID: "abc", + }, + }, + { + Method: "GET", + Resource: "/api/2.0/clusters/get?cluster_id=abc", + Response: ClusterInfo{ + State: ClusterStateRunning, + ClusterID: "abc", + }, + }, + }) + defer server.Close() + require.NoError(t, err) + + clusterInfo, err := NewClustersAPI(client).StartAndGetInfo("abc") + require.NoError(t, err) + assert.Equal(t, ClusterStateRunning, string(clusterInfo.State)) +} + +func TestStartAndGetInfo_StartingError(t *testing.T) { + client, server, err := qa.HttpFixtureClient(t, []qa.HTTPFixture{ + { + Method: "GET", + Resource: "/api/2.0/clusters/get?cluster_id=abc", + Response: ClusterInfo{ + State: ClusterStateError, + StateMessage: "I am a teapot", + }, + }, + { + Method: "POST", + Resource: "/api/2.0/clusters/start", + ExpectedRequest: ClusterID{ + ClusterID: "abc", + }, + Response: common.APIErrorBody{ + Message: "I am a teapot!", + }, + Status: 418, + }, + }) + defer server.Close() + require.NoError(t, err) + + _, err = NewClustersAPI(client).StartAndGetInfo("abc") + require.Error(t, err) + assert.Contains(t, err.Error(), "I am a teapot") +} + +func TestPermanentDelete_Pinned(t *testing.T) { + client, server, err := qa.HttpFixtureClient(t, []qa.HTTPFixture{ + { + Method: "POST", + Resource: "/api/2.0/clusters/delete", + ExpectedRequest: ClusterID{ + ClusterID: "abc", + }, + }, + { + Method: "GET", + Resource: "/api/2.0/clusters/get?cluster_id=abc", + Response: ClusterInfo{ + State: ClusterStateTerminated, + }, + }, + { + Method: "POST", + Resource: "/api/2.0/clusters/permanent-delete", + ExpectedRequest: ClusterID{ + ClusterID: "abc", + }, + Response: common.APIErrorBody{ + Message: "unpin the cluster first", + }, + Status: 400, + }, + { + Method: "POST", + Resource: "/api/2.0/clusters/unpin", + ExpectedRequest: ClusterID{ + ClusterID: "abc", + }, + }, + { + Method: "POST", + Resource: "/api/2.0/clusters/permanent-delete", + ExpectedRequest: ClusterID{ + ClusterID: "abc", + }, + }, + }) + defer server.Close() + require.NoError(t, err) + + err = NewClustersAPI(client).PermanentDelete("abc") + require.NoError(t, err) +} + func TestClustersAPI_List(t *testing.T) { tests := []struct { name string diff --git a/compute/common_instances.go b/compute/common_instances.go index e634b478c6..91273a2a95 100644 --- a/compute/common_instances.go +++ b/compute/common_instances.go @@ -64,7 +64,7 @@ func CommonInstancePoolID() string { MaxCapacity: 10, IdleInstanceAutoTerminationMinutes: 15, } - if !client.IsUsingAzureAuth() { + if !client.IsAzure() { instancePool.AwsAttributes = &InstancePoolAwsAttributes{ Availability: AwsAvailabilitySpot, } diff --git a/compute/model.go b/compute/model.go index 2fd53cd74d..100fbbd216 100644 --- a/compute/model.go +++ b/compute/model.go @@ -247,6 +247,11 @@ type Cluster struct { IdempotencyToken string `json:"idempotency_token,omitempty"` } +// ClusterList shows existing clusters +type ClusterList struct { + Clusters []ClusterInfo `json:"clusters,omitempty"` +} + // ClusterInfo contains the information when getting cluster info from the get request. type ClusterInfo struct { NumWorkers int32 `json:"num_workers,omitempty"` diff --git a/compute/resource_instance_pool_test.go b/compute/resource_instance_pool_test.go index 9418949c66..f3e8b985d0 100644 --- a/compute/resource_instance_pool_test.go +++ b/compute/resource_instance_pool_test.go @@ -249,7 +249,7 @@ func TestAccInstancePools(t *testing.T) { "7.1.x-scala2.12", }, } - if !client.IsUsingAzureAuth() { + if !client.IsAzure() { pool.DiskSpec = &InstancePoolDiskSpec{ DiskType: &InstancePoolDiskType{ EbsVolumeType: EbsVolumeTypeGeneralPurposeSsd, @@ -289,7 +289,7 @@ func TestAccInstancePools(t *testing.T) { "7.1.x-scala2.12", }, } - if !client.IsUsingAzureAuth() { + if !client.IsAzure() { u.DiskSpec = &InstancePoolDiskSpec{ DiskType: &InstancePoolDiskType{ EbsVolumeType: EbsVolumeTypeGeneralPurposeSsd, diff --git a/internal/qa/testing.go b/internal/qa/testing.go index e867c570c7..fba00a52af 100644 --- a/internal/qa/testing.go +++ b/internal/qa/testing.go @@ -192,13 +192,18 @@ func HttpFixtureClient(t *testing.T, fixtures []HTTPFixture) (client *common.Dat assert.JSONEq(t, string(jsonStr), buf.String(), "json strings do not match") } if fixture.Response != nil { - responseBytes, err := json.Marshal(fixture.Response) - if err != nil { + if alreadyJSON, ok := fixture.Response.(string); ok { + _, err = rw.Write([]byte(alreadyJSON)) + assert.NoError(t, err, err) + } else { + responseBytes, err := json.Marshal(fixture.Response) + if err != nil { + assert.NoError(t, err, err) + t.FailNow() + } + _, err = rw.Write(responseBytes) assert.NoError(t, err, err) - t.FailNow() } - _, err = rw.Write(responseBytes) - assert.NoError(t, err, err) } found = true // Reset the request if it is already used @@ -371,7 +376,7 @@ func compare(t *testing.T, a interface{}, b interface{}) { // GetCloudInstanceType gives common minimal instance type, depending on a cloud func GetCloudInstanceType(c *common.DatabricksClient) string { - if c.IsUsingAzureAuth() { + if c.IsAzure() { return "Standard_DS3_v2" } // TODO: create a method on ClustersAPI to give diff --git a/provider/provider.go b/provider/provider.go index 1aa68cc68b..f5ff13d43e 100644 --- a/provider/provider.go +++ b/provider/provider.go @@ -220,7 +220,9 @@ func DatabricksProvider() *schema.Provider { Description: "Create ephemeral PAT tokens also for AZ CLI authenticated requests", }, "azure_auth": { - Type: schema.TypeMap, + // TODO: tf13 - azure_auth: TypeMap with Elem *Resource not supported,use TypeList/TypeSet + Type: schema.TypeList, + MaxItems: 1, Optional: true, Deprecated: "azure_auth {} block is deprecated in favor of azure_* properties with more previctable behavior. " + "This configuration attribute will be removed in 0.3.", diff --git a/provider/provider_test.go b/provider/provider_test.go index e9c253ef5e..e2d6ebdf61 100644 --- a/provider/provider_test.go +++ b/provider/provider_test.go @@ -29,9 +29,10 @@ type providerConfigTest struct { azureSubscriptionID string azureWorkspaceResourceID string env map[string]string - errPrefix string - hasToken string - hasHost string + assertError string + assertToken string + assertHost string + assertAzure bool } func (tt providerConfigTest) rawConfig() map[string]interface{} { @@ -81,44 +82,44 @@ func (tt providerConfigTest) rawConfig() map[string]interface{} { func TestProviderConfigurationOptions(t *testing.T) { tests := []providerConfigTest{ { - errPrefix: "Authentication is not configured for provider", + assertError: "Authentication is not configured for provider", }, { env: map[string]string{ "DATABRICKS_HOST": "x", }, - errPrefix: "Authentication is not configured for provider", + assertError: "Authentication is not configured for provider", }, { env: map[string]string{ "DATABRICKS_TOKEN": "x", }, - errPrefix: "Host is empty, but is required by token", + assertError: "Host is empty, but is required by token", }, { env: map[string]string{ "DATABRICKS_HOST": "x", "DATABRICKS_TOKEN": "x", }, - hasToken: "x", - hasHost: "https://x", + assertToken: "x", + assertHost: "https://x", }, { host: "https://x", env: map[string]string{ "DATABRICKS_TOKEN": "x", }, - hasToken: "x", - hasHost: "https://x", + assertToken: "x", + assertHost: "https://x", }, { env: map[string]string{ "DATABRICKS_USERNAME": "x", "DATABRICKS_PASSWORD": "x", }, - errPrefix: "Host is empty, but is required by basic_auth", - hasToken: "x", - hasHost: "https://x", + assertError: "Host is empty, but is required by basic_auth", + assertToken: "x", + assertHost: "https://x", }, { env: map[string]string{ @@ -126,8 +127,8 @@ func TestProviderConfigurationOptions(t *testing.T) { "DATABRICKS_USERNAME": "x", "DATABRICKS_PASSWORD": "x", }, - hasToken: "eDp4", - hasHost: "https://x", + assertToken: "eDp4", + assertHost: "https://x", }, { host: "y", @@ -136,8 +137,8 @@ func TestProviderConfigurationOptions(t *testing.T) { "DATABRICKS_USERNAME": "x", "DATABRICKS_PASSWORD": "x", }, - hasToken: "eDp4", - hasHost: "https://y", + assertToken: "eDp4", + assertHost: "https://y", }, { host: "y", @@ -145,15 +146,23 @@ func TestProviderConfigurationOptions(t *testing.T) { env: map[string]string{ "DATABRICKS_PASSWORD": "x", }, - hasToken: "eDp4", - hasHost: "https://y", + assertToken: "eDp4", + assertHost: "https://y", }, { - host: "y", - username: "x", - password: "x", - hasToken: "eDp4", - hasHost: "https://y", + host: "y", + username: "x", + password: "x", + assertToken: "eDp4", + assertHost: "https://y", + }, + { + // Azure hostnames can support host+token auth, as usual + host: "https://adb-xxx.y.azuredatabricks.net/", + token: "y", + assertAzure: true, + assertHost: "https://adb-xxx.y.azuredatabricks.net/", + assertToken: "y", }, { env: map[string]string{ @@ -162,21 +171,21 @@ func TestProviderConfigurationOptions(t *testing.T) { "DATABRICKS_USERNAME": "x", "DATABRICKS_PASSWORD": "x", }, - errPrefix: "More than one authorization method configured: password and token", + assertError: "More than one authorization method configured: password and token", }, { env: map[string]string{ "CONFIG_FILE": "x", }, - errPrefix: "Authentication is not configured for provider", + assertError: "Authentication is not configured for provider", }, { // loading with DEFAULT profile in databrickscfs env: map[string]string{ "HOME": "../common/testdata", }, - hasHost: "https://dbc-XXXXXXXX-YYYY.cloud.databricks.com/", - hasToken: "PT0+IC9kZXYvdXJhbmRvbSA8PT0KYFZ", + assertHost: "https://dbc-XXXXXXXX-YYYY.cloud.databricks.com/", + assertToken: "PT0+IC9kZXYvdXJhbmRvbSA8PT0KYFZ", }, { // loading with nohost profile in databrickscfs @@ -184,7 +193,7 @@ func TestProviderConfigurationOptions(t *testing.T) { "HOME": "../common/testdata", "DATABRICKS_CONFIG_PROFILE": "nohost", }, - errPrefix: "config file ../common/testdata/.databrickscfg is corrupt: cannot find host in nohost profile", + assertError: "config file ../common/testdata/.databrickscfg is corrupt: cannot find host in nohost profile", }, { env: map[string]string{ @@ -192,7 +201,7 @@ func TestProviderConfigurationOptions(t *testing.T) { "DATABRICKS_CONFIG_PROFILE": "nohost", "HOME": "../common/testdata", }, - errPrefix: "More than one authorization method configured: config profile and token", + assertError: "More than one authorization method configured: config profile and token", }, { env: map[string]string{ @@ -200,7 +209,7 @@ func TestProviderConfigurationOptions(t *testing.T) { "DATABRICKS_CONFIG_PROFILE": "nohost", "HOME": "../common/testdata", }, - errPrefix: "More than one authorization method configured: config profile and password", + assertError: "More than one authorization method configured: config profile and password", }, { // this test will skip ensureWorkspaceUrl @@ -211,8 +220,9 @@ func TestProviderConfigurationOptions(t *testing.T) { "PATH": "../common/testdata:/bin", "HOME": "../common/testdata", }, - hasHost: "https://x", - hasToken: "", + assertAzure: true, + assertHost: "https://x", + assertToken: "", }, { azureWorkspaceResourceID: "/a/b/c", @@ -222,7 +232,7 @@ func TestProviderConfigurationOptions(t *testing.T) { "FAIL": "yes", "HOME": "../common/testdata", }, - errPrefix: "Invoking Azure CLI failed with the following error: This is just a failing script.", + assertError: "Invoking Azure CLI failed with the following error: This is just a failing script.", }, { // `az` not installed, which is expected for deployers on other clouds... @@ -231,7 +241,7 @@ func TestProviderConfigurationOptions(t *testing.T) { "PATH": "/bin", "HOME": "../common/testdata", }, - errPrefix: "Most likely Azure CLI is not installed.", + assertError: "Most likely Azure CLI is not installed.", }, { azureWorkspaceResourceID: "/a/b/c", @@ -241,7 +251,7 @@ func TestProviderConfigurationOptions(t *testing.T) { "PATH": "../common/testdata:/bin", "HOME": "../common/testdata", }, - errPrefix: "More than one authorization method configured: azure and token", + assertError: "More than one authorization method configured: azure and token", }, { // omit request to management endpoint to get workspace properties @@ -252,7 +262,8 @@ func TestProviderConfigurationOptions(t *testing.T) { "PATH": "../common/testdata:/bin", "HOME": "../common/testdata", }, - hasHost: "https://x", + assertAzure: true, + assertHost: "https://x", }, { host: "x", @@ -263,7 +274,7 @@ func TestProviderConfigurationOptions(t *testing.T) { "HOME": "../common/testdata", "DATABRICKS_USERNAME": "x", }, - errPrefix: "More than one authorization method configured: azure and password", + assertError: "More than one authorization method configured: azure and password", }, { azureWorkspaceResourceID: "/a/b/c", @@ -273,24 +284,41 @@ func TestProviderConfigurationOptions(t *testing.T) { env: map[string]string{ "HOME": "../common/testdata", }, - hasHost: "", - hasToken: "", + assertAzure: true, + assertHost: "", + assertToken: "", + }, + { + // https://github.com/databrickslabs/terraform-provider-databricks/issues/294 + azureResourceGroup: "b", + azureWorkspaceName: "c", + env: map[string]string{ + "ARM_CLIENT_ID": "x", + "ARM_CLIENT_SECRET": "y", + "ARM_SUBSCRIPTION_ID": "q", + "ARM_TENANT_ID": "z", + "HOME": "../common/testdata", + }, + assertAzure: true, + assertHost: "", + assertToken: "", }, } for _, tt := range tests { t.Run(fmt.Sprintf("config:%v env:%v", tt.rawConfig(), tt.env), func(t *testing.T) { c, err := configureProviderAndReturnClient(t, tt) - if tt.errPrefix != "" { - require.NotNilf(t, err, "Expected to have %s error", tt.errPrefix) - require.True(t, strings.HasPrefix(err.Error(), tt.errPrefix), err) + if tt.assertError != "" { + require.NotNilf(t, err, "Expected to have %s error", tt.assertError) + require.True(t, strings.HasPrefix(err.Error(), tt.assertError), err) return } if err != nil { require.NoError(t, err) return } - assert.Equal(t, tt.hasToken, c.Token) - assert.Equal(t, tt.hasHost, c.Host) + assert.Equal(t, tt.assertAzure, c.IsAzure()) + assert.Equal(t, tt.assertToken, c.Token) + assert.Equal(t, tt.assertHost, c.Host) }) } }