Fixed #656 - Make application_id optional for non-Azure deployments

Author: nfx

CHANGELOG.md

@@ -7,6 +7,7 @@
 * Added the `databricks_user` data source ([#648](https://github.com/databrickslabs/terraform-provider-databricks/pull/648))
 * Fixed support for `spot_instance_policy` in SQLA Endpoints ([#665](https://github.com/databrickslabs/terraform-provider-databricks/issues/665))
 * Added documentation for `databricks_pipeline` resource ([#673](https://github.com/databrickslabs/terraform-provider-databricks/pull/673))
+* Fixed mapping for `databricks_service_principal` on AWS ([#656](https://github.com/databrickslabs/terraform-provider-databricks/issues/656))
 * Made preview environment tests to run on a release basis
 
 Updated dependency versions:

docs/resources/service_principal.md

@@ -3,7 +3,7 @@ subcategory: "Security"
 ---
 # databricks_service_principal Resource
 
-Directly creates service principal, that could be added to [databricks_group](group.md) within workspace.
+Directly creates a service principal that could be added to [databricks_group](group.md) within workspace.
 
 ## Example Usage
 
@@ -19,7 +19,7 @@ Creating service principal with administrative permissions - referencing special
 
 ```hcl
 data "databricks_group" "admins" {
-    display_name = "admins"
+  display_name = "admins"
 }
 
 resource "databricks_service_principal" "sp" {
@@ -44,12 +44,14 @@ resource "databricks_service_principal" "sp" {
 
 ## Argument Reference
 
+-> `application_id` is required on Azure Databricks and is not allowed on other clouds. `display_name` is required on all clouds except Azure.
+
 The following arguments are available:
 
-* `application_id` - (Required) This is the application id of the given service principal and will be their form of access and identity.
-* `display_name` - (Optional) This is an alias for the service principal can be the full name of the service principal.
-* `allow_cluster_create` -  (Optional) Allow the service principal to have [cluster](cluster.md) create priviliges. Defaults to false. More fine grained permissions could be assigned with [databricks_permissions](permissions.md#Cluster-usage) and `cluster_id` argument. Everyone without `allow_cluster_create` arugment set, but with [permission to use](permissions.md#Cluster-Policy-usage) Cluster Policy would be able to create clusters, but within boundaries of that specific policy.
-* `allow_instance_pool_create` -  (Optional) Allow the service principal to have [instance pool](instance_pool.md) create priviliges. Defaults to false. More fine grained permissions could be assigned with [databricks_permissions](permissions.md#Instance-Pool-usage) and [instance_pool_id](permissions.md#instance_pool_id) argument.
+* `application_id` - This is the application id of the given service principal and will be their form of access and identity. On other clouds than Azure this value is auto-generated.
+* `display_name` - (Required) This is an alias for the service principal and can be the full name of the service principal.
+* `allow_cluster_create` -  (Optional) Allow the service principal to have [cluster](cluster.md) create privileges. Defaults to false. More fine grained permissions could be assigned with [databricks_permissions](permissions.md#Cluster-usage) and `cluster_id` argument. Everyone without `allow_cluster_create` argument set, but with [permission to use](permissions.md#Cluster-Policy-usage) Cluster Policy would be able to create clusters, but within the boundaries of that specific policy.
+* `allow_instance_pool_create` -  (Optional) Allow the service principal to have [instance pool](instance_pool.md) create privileges. Defaults to false. More fine grained permissions could be assigned with [databricks_permissions](permissions.md#Instance-Pool-usage) and [instance_pool_id](permissions.md#instance_pool_id) argument.
 * `active` - (Optional) Either service principal is active or not. True by default, but can be set to false in case of service principal deactivation with preserving service principal assets.
 
 ## Attribute Reference

identity/acceptance/resource_service_principal_test.go

@@ -1,49 +1,28 @@
 package acceptance
 
 import (
-	"os"
 	"testing"
 
 	"github.com/databrickslabs/terraform-provider-databricks/internal/acceptance"
-	"github.com/databrickslabs/terraform-provider-databricks/qa"
-	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
 )
 
 func TestAzureAccServicePrincipalResource(t *testing.T) {
-	if _, ok := os.LookupEnv("CLOUD_ENV"); !ok {
-		t.Skip("Acceptance tests skipped unless env 'CLOUD_ENV' is set")
-	}
-	config := qa.EnvironmentTemplate(t, `
-		data "databricks_group" "admins" {
-			display_name = "admins"
-		}
-		resource "databricks_service_principal" "sp_first" {
-			application_id = "00000000-1234-5678-0000-000000000001"
-			display_name = "Eerste {var.RANDOM}"
-		}
-		resource "databricks_service_principal" "sp_second" {
-			application_id = "00000000-1234-5678-0000-000000000002"
-			display_name = "Tweede {var.RANDOM}"
-			allow_cluster_create = true
-		}
-		resource "databricks_service_principal" "sp_third" {
-			application_id = "00000000-1234-5678-0000-000000000003"
-			allow_instance_pool_create = true
-		}`)
-	acceptance.AccTest(t, resource.TestCase{
-		Steps: []resource.TestStep{
-			{
-				Config: config,
-				Check: resource.ComposeTestCheckFunc(
-					resource.TestCheckResourceAttr("databricks_service_principal.sp_first", "allow_cluster_create", "false"),
-					resource.TestCheckResourceAttr("databricks_service_principal.sp_first", "allow_instance_pool_create", "false"),
-					resource.TestCheckResourceAttr("databricks_service_principal.sp_second", "allow_cluster_create", "true"),
-					resource.TestCheckResourceAttr("databricks_service_principal.sp_second", "allow_instance_pool_create", "false"),
-					resource.TestCheckResourceAttr("databricks_service_principal.sp_third", "allow_cluster_create", "false"),
-					resource.TestCheckResourceAttr("databricks_service_principal.sp_third", "allow_instance_pool_create", "true"),
-				),
-				Destroy: false,
-			},
+	acceptance.Test(t, []acceptance.Step{
+		{
+			Template: `resource "databricks_service_principal" "this" {
+				application_id = "00000000-1234-5678-0000-000000000001"
+				display_name = "SPN {var.RANDOM}"
+			}`,
+		},
+	})
+}
+
+func TestAwsAccServicePrincipalResource(t *testing.T) {
+	acceptance.Test(t, []acceptance.Step{
+		{
+			Template: `resource "databricks_service_principal" "this" {
+				display_name = "SPN {var.RANDOM}"
+			}`,
 		},
 	})
 }

identity/resource_service_principal.go

@@ -22,7 +22,7 @@ type ServicePrincipalsAPI struct {
 
 // ServicePrincipalEntity entity from which resource schema is made
 type ServicePrincipalEntity struct {
-	ApplicationID           string `json:"application_id"`
+	ApplicationID           string `json:"application_id,omitempty" tf:"computed"`
 	DisplayName             string `json:"display_name,omitempty" tf:"computed"`
 	Active                  bool   `json:"active,omitempty"`
 	AllowClusterCreate      bool   `json:"allow_cluster_create,omitempty"`
@@ -116,6 +116,15 @@ func ResourceServicePrincipal() *schema.Resource {
 			if err := common.DataToStructPointer(d, servicePrincipalSchema, &sp); err != nil {
 				return err
 			}
+			if c.IsAzure() && sp.ApplicationID == "" {
+				return fmt.Errorf("application_id is required for service principals in Azure Databricks")
+			}
+			if c.IsAws() && sp.ApplicationID != "" {
+				return fmt.Errorf("application_id is not allowed for service principals in Databricks on AWS")
+			}
+			if c.IsAws() && sp.DisplayName == "" {
+				return fmt.Errorf("display_name is required for service principals in Databricks on AWS")
+			}
 			servicePrincipal, err := NewServicePrincipalsAPI(ctx, c).CreateR(sp)
 			if err != nil {
 				return err

identity/resource_service_principal_test.go

@@ -13,7 +13,7 @@ import (
 )
 
 func TestAzureAccSP(t *testing.T) {
-	if _, ok := os.LookupEnv("CLOUD_ENV"); !ok {
+	if cloud, ok := os.LookupEnv("CLOUD_ENV"); !ok || cloud != "azure" {
 		t.Skip("Acceptance tests skipped unless env 'CLOUD_ENV' is set")
 	}
 
@@ -51,9 +51,8 @@ func TestResourceServicePrincipalRead(t *testing.T) {
 				Method:   "GET",
 				Resource: "/api/2.0/preview/scim/v2/ServicePrincipals/abc",
 				Response: ScimUser{
-					ID:            "abc",
-					DisplayName:   "Example Service Principal",
-					ApplicationID: "00000000-0000-0000-0000-000000000000",
+					ID:          "abc",
+					DisplayName: "Example Service Principal",
 					Groups: []GroupsListItem{
 						{
 							Display: "admins",
@@ -74,7 +73,6 @@ func TestResourceServicePrincipalRead(t *testing.T) {
 	}.Apply(t)
 	require.NoError(t, err, err)
 	assert.Equal(t, "abc", d.Id(), "Id should not be empty")
-	assert.Equal(t, "00000000-0000-0000-0000-000000000000", d.Get("application_id"))
 	assert.Equal(t, "Example Service Principal", d.Get("display_name"))
 	assert.Equal(t, false, d.Get("allow_cluster_create"))
 }
@@ -132,8 +130,7 @@ func TestResourceServicePrincipalCreate(t *testing.T) {
 							Value: "allow-cluster-create",
 						},
 					},
-					ApplicationID: "00000000-0000-0000-0000-000000000000",
-					Schemas:       []URN{ServicePrincipalSchema},
+					Schemas: []URN{ServicePrincipalSchema},
 				},
 				Response: ScimUser{
 					ID: "abc",
@@ -143,10 +140,9 @@ func TestResourceServicePrincipalCreate(t *testing.T) {
 				Method:   "GET",
 				Resource: "/api/2.0/preview/scim/v2/ServicePrincipals/abc",
 				Response: ScimUser{
-					DisplayName:   "Example Service Principal",
-					Active:        true,
-					ApplicationID: "00000000-0000-0000-0000-000000000000",
-					ID:            "abc",
+					DisplayName: "Example Service Principal",
+					Active:      true,
+					ID:          "abc",
 					Entitlements: []entitlementsListItem{
 						{
 							Value: AllowClusterCreateEntitlement,
@@ -168,14 +164,12 @@ func TestResourceServicePrincipalCreate(t *testing.T) {
 		Resource: ResourceServicePrincipal(),
 		Create:   true,
 		HCL: `
-		application_id    = "00000000-0000-0000-0000-000000000000"
 		display_name = "Example Service Principal"
 		allow_cluster_create = true
 		`,
 	}.Apply(t)
 	require.NoError(t, err, err)
 	assert.Equal(t, "abc", d.Id(), "Id should not be empty")
-	assert.Equal(t, "00000000-0000-0000-0000-000000000000", d.Get("application_id"))
 	assert.Equal(t, "Example Service Principal", d.Get("display_name"))
 	assert.Equal(t, true, d.Get("allow_cluster_create"))
 }
@@ -192,7 +186,6 @@ func TestResourceServicePrincipalCreate_Error(t *testing.T) {
 		Resource: ResourceServicePrincipal(),
 		Create:   true,
 		HCL: `
-		application_id    = "00000000-0000-0000-0000-000000000000"
 		display_name = "Example Service Principal"
 		allow_cluster_create = true
 		`,
@@ -202,10 +195,9 @@ func TestResourceServicePrincipalCreate_Error(t *testing.T) {
 
 func TestResourceServicePrincipalUpdate(t *testing.T) {
 	newServicePrincipal := ScimUser{
-		Schemas:       []URN{ServicePrincipalSchema},
-		DisplayName:   "Changed Name",
-		ApplicationID: "00000000-0000-0000-0000-000000000000",
-		Active:        true,
+		Schemas:     []URN{ServicePrincipalSchema},
+		DisplayName: "Changed Name",
+		Active:      true,
 		Entitlements: []entitlementsListItem{
 			{
 				Value: AllowInstancePoolCreateEntitlement,
@@ -228,10 +220,9 @@ func TestResourceServicePrincipalUpdate(t *testing.T) {
 				Method:   "GET",
 				Resource: "/api/2.0/preview/scim/v2/ServicePrincipals/abc",
 				Response: ScimUser{
-					DisplayName:   "Example Service Principal",
-					Active:        true,
-					ApplicationID: "00000000-0000-0000-0000-000000000000",
-					ID:            "abc",
+					DisplayName: "Example Service Principal",
+					Active:      true,
+					ID:          "abc",
 					Entitlements: []entitlementsListItem{
 						{
 							Value: AllowClusterCreateEntitlement,
@@ -264,15 +255,13 @@ func TestResourceServicePrincipalUpdate(t *testing.T) {
 		Update:   true,
 		ID:       "abc",
 		HCL: `
-		application_id    = "00000000-0000-0000-0000-000000000000"
 		display_name = "Changed Name"
 		allow_cluster_create = false
 		allow_instance_pool_create = true
 		`,
 	}.Apply(t)
 	require.NoError(t, err, err)
 	assert.Equal(t, "abc", d.Id(), "Id should not be empty")
-	assert.Equal(t, "00000000-0000-0000-0000-000000000000", d.Get("application_id"))
 	assert.Equal(t, "Changed Name", d.Get("display_name"))
 	assert.Equal(t, false, d.Get("allow_cluster_create"))
 	assert.Equal(t, true, d.Get("allow_instance_pool_create"))
@@ -291,7 +280,6 @@ func TestResourceServicePrincipalUpdate_Error(t *testing.T) {
 		Update:   true,
 		ID:       "abc",
 		HCL: `
-		application_id    = "00000000-0000-0000-0000-000000000000"
 		display_name = "Changed Name"
 		allow_cluster_create = false
 		allow_instance_pool_create = true
@@ -307,10 +295,9 @@ func TestResourceServicePrincipalUpdate_ErrorPut(t *testing.T) {
 				Method:   "GET",
 				Resource: "/api/2.0/preview/scim/v2/ServicePrincipals/abc",
 				Response: ScimUser{
-					DisplayName:   "Example Service Principal",
-					Active:        true,
-					ApplicationID: "00000000-0000-0000-0000-000000000000",
-					ID:            "abc",
+					DisplayName: "Example Service Principal",
+					Active:      true,
+					ID:          "abc",
 					Entitlements: []entitlementsListItem{
 						{
 							Value: AllowClusterCreateEntitlement,
@@ -338,7 +325,6 @@ func TestResourceServicePrincipalUpdate_ErrorPut(t *testing.T) {
 		Update:   true,
 		ID:       "abc",
 		HCL: `
-		application_id    = "00000000-0000-0000-0000-000000000000"
 		display_name = "Changed Name"
 		allow_cluster_create = false
 		allow_instance_pool_create = true