Skip to content

Commit

Permalink
Fix updating databricks_service_principal on Azure (databricks#1322)
Browse files Browse the repository at this point in the history 
* propagate `application_id` on SCIM PUT call for SPN on Azure
* behavior should not affect AWS

Fix databricks#1319
  • Loading branch information
@nfx
nfx authored May 20, 2022
1 parent eb6fca7 commit 3980839
Show file tree
Hide file tree
Showing 3 changed files with 142 additions and 105 deletions.
2 changes: 1 addition & 1 deletion common/version.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package common
import "context"

var (
version = "0.5.7"
version = "0.5.8"
// ResourceName is resource name without databricks_ prefix
ResourceName contextKey = 1
// Provider is the current instance of provider
Expand Down
13 changes: 9 additions & 4 deletions scim/resource_service_principal.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,11 +125,16 @@ func ResourceServicePrincipal() *schema.Resource {
return sp.Entitlements.readIntoData(d)
},
Update: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
var applicationId string
if c.IsAzure() {
applicationId = d.Get("application_id").(string)
}
return NewServicePrincipalsAPI(ctx, c).Update(d.Id(), User{
DisplayName: d.Get("display_name").(string),
Active: d.Get("active").(bool),
Entitlements: readEntitlementsFromData(d),
ExternalID: d.Get("external_id").(string),
DisplayName: d.Get("display_name").(string),
Active: d.Get("active").(bool),
Entitlements: readEntitlementsFromData(d),
ExternalID: d.Get("external_id").(string),
ApplicationID: applicationId,
})
},
Delete: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
Expand Down
232 changes: 132 additions & 100 deletions scim/resource_service_principal_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,22 +52,19 @@ func TestAccServicePrincipalOnAzure(t *testing.T) {
}

func TestResourceServicePrincipalRead(t *testing.T) {
d, err := qa.ResourceFixture{
qa.ResourceFixture{
Fixtures: []qa.HTTPFixture{
{
Method: "GET",
Resource: "/api/2.0/preview/scim/v2/ServicePrincipals/abc",
Response: User{
ID: "abc",
DisplayName: "Example Service Principal",
Groups: []ComplexValue{
{
Display: "admins",
Value: "4567",
},
ID: "abc",
ApplicationID: "bcd",
DisplayName: "Example Service Principal",
Active: true,
Entitlements: []ComplexValue{
{
Display: "ds",
Value: "9877",
Value: "allow-cluster-create",
},
},
},
Expand All @@ -78,11 +75,11 @@ func TestResourceServicePrincipalRead(t *testing.T) {
New: true,
Read: true,
ID: "abc",
}.Apply(t)
require.NoError(t, err, err)
assert.Equal(t, "abc", d.Id(), "Id should not be empty")
assert.Equal(t, "Example Service Principal", d.Get("display_name"))
assert.Equal(t, false, d.Get("allow_cluster_create"))
}.ApplyAndExpectData(t, map[string]interface{}{
"display_name": "Example Service Principal",
"application_id": "bcd",
"allow_cluster_create": true,
})
}

func TestResourceServicePrincipalRead_NotFound(t *testing.T) {
Expand Down Expand Up @@ -125,7 +122,7 @@ func TestResourceServicePrincipalRead_Error(t *testing.T) {
}

func TestResourceServicePrincipalCreate(t *testing.T) {
d, err := qa.ResourceFixture{
qa.ResourceFixture{
Fixtures: []qa.HTTPFixture{
{
Method: "POST",
Expand Down Expand Up @@ -175,59 +172,31 @@ func TestResourceServicePrincipalCreate(t *testing.T) {
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, "Example Service Principal", d.Get("display_name"))
assert.Equal(t, true, d.Get("allow_cluster_create"))
}.ApplyNoError(t)
}

func TestResourceServicePrincipalCreate_Error(t *testing.T) {
_, err := qa.ResourceFixture{
Fixtures: []qa.HTTPFixture{
{
Method: "POST",
Resource: "/api/2.0/preview/scim/v2/ServicePrincipals",
Status: 400,
},
},
qa.ResourceFixture{
Fixtures: qa.HTTPFailures,
Resource: ResourceServicePrincipal(),
Create: true,
HCL: `
display_name = "Example Service Principal"
allow_cluster_create = true
`,
}.Apply(t)
require.Error(t, err, err)
}.ExpectError(t, "I'm a teapot")
}

func TestResourceServicePrincipalUpdate(t *testing.T) {
newServicePrincipal := User{
Schemas: []URN{ServicePrincipalSchema},
DisplayName: "Changed Name",
Active: true,
Entitlements: entitlements{
{
Value: "allow-instance-pool-create",
},
},
Groups: []ComplexValue{
{
Display: "admins",
Value: "4567",
},
{
Display: "ds",
Value: "9877",
},
},
}
d, err := qa.ResourceFixture{
func TestResourceServicePrincipalUpdateOnAWS(t *testing.T) {
qa.ResourceFixture{
Fixtures: []qa.HTTPFixture{
{
Method: "GET",
Resource: "/api/2.0/preview/scim/v2/ServicePrincipals/abc",
Response: User{
// application ID is created by platform on AWS
ApplicationID: "existing-application-id",

DisplayName: "Example Service Principal",
Active: true,
ID: "abc",
Expand All @@ -249,14 +218,55 @@ func TestResourceServicePrincipalUpdate(t *testing.T) {
},
},
{
Method: "PUT",
Resource: "/api/2.0/preview/scim/v2/ServicePrincipals/abc",
ExpectedRequest: newServicePrincipal,
Method: "PUT",
Resource: "/api/2.0/preview/scim/v2/ServicePrincipals/abc",
ExpectedRequest: User{
// application ID is not allowed to be modified by client side on AWS

Schemas: []URN{ServicePrincipalSchema},
DisplayName: "Changed Name",
Active: true,
Entitlements: entitlements{
{
Value: "allow-instance-pool-create",
},
},
Groups: []ComplexValue{
{
Display: "admins",
Value: "4567",
},
{
Display: "ds",
Value: "9877",
},
},
},
},
{
Method: "GET",
Resource: "/api/2.0/preview/scim/v2/ServicePrincipals/abc",
Response: newServicePrincipal,
Response: User{
Schemas: []URN{ServicePrincipalSchema},
ApplicationID: "existing-application-id",
DisplayName: "Changed Name",
Active: true,
Entitlements: entitlements{
{
Value: "allow-instance-pool-create",
},
},
Groups: []ComplexValue{
{
Display: "admins",
Value: "4567",
},
{
Display: "ds",
Value: "9877",
},
},
},
},
},
Resource: ResourceServicePrincipal(),
Expand All @@ -267,37 +277,83 @@ func TestResourceServicePrincipalUpdate(t *testing.T) {
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, "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"))
}.ApplyAndExpectData(t, map[string]interface{}{
"display_name": "Changed Name",
"allow_cluster_create": false,
"allow_instance_pool_create": true,
})
}

func TestResourceServicePrincipalUpdate_Error(t *testing.T) {
_, err := qa.ResourceFixture{
// https://github.com/databrickslabs/terraform-provider-databricks/issues/1319
func TestResourceServicePrincipalUpdateOnAzure(t *testing.T) {
qa.ResourceFixture{
Azure: true,
Fixtures: []qa.HTTPFixture{
{
Method: "GET",
Resource: "/api/2.0/preview/scim/v2/ServicePrincipals/abc",
Status: 400,
Response: User{
// application id is specified by user on Azure
ApplicationID: "existing-application-id",

DisplayName: "Example Service Principal",
Active: true,
ID: "abc",
},
},
{
Method: "PUT",
Resource: "/api/2.0/preview/scim/v2/ServicePrincipals/abc",
ExpectedRequest: User{
// application id is specified by user on Azure and also must be part of modification
ApplicationID: "existing-application-id",

Schemas: []URN{ServicePrincipalSchema},
DisplayName: "Changed Name",
Active: true,
},
},
{
Method: "GET",
Resource: "/api/2.0/preview/scim/v2/ServicePrincipals/abc",
Response: User{
Schemas: []URN{ServicePrincipalSchema},
ApplicationID: "existing-application-id",
DisplayName: "Changed Name",
Active: true,
},
},
},
Resource: ResourceServicePrincipal(),
Update: true,
ID: "abc",
InstanceState: map[string]string{
"application_id": "existing-application-id",
"display_name": "Example Service Principal",
},
HCL: `
application_id = "existing-application-id"
display_name = "Changed Name"
`,
}.ApplyNoError(t)
}

func TestResourceServicePrincipalUpdate_Error(t *testing.T) {
qa.ResourceFixture{
Fixtures: qa.HTTPFailures,
Resource: ResourceServicePrincipal(),
Update: true,
ID: "abc",
HCL: `
display_name = "Changed Name"
allow_cluster_create = false
allow_instance_pool_create = true
`,
}.Apply(t)
require.Error(t, err, err)
}.ExpectError(t, "I'm a teapot")
}

func TestResourceServicePrincipalUpdate_ErrorPut(t *testing.T) {
_, err := qa.ResourceFixture{
qa.ResourceFixture{
Fixtures: []qa.HTTPFixture{
{
Method: "GET",
Expand All @@ -311,23 +367,9 @@ func TestResourceServicePrincipalUpdate_ErrorPut(t *testing.T) {
Value: "allow-cluster-create",
},
},
Groups: []ComplexValue{
{
Display: "admins",
Value: "4567",
},
{
Display: "ds",
Value: "9877",
},
},
},
},
{
Method: "PUT",
Resource: "/api/2.0/preview/scim/v2/ServicePrincipals/abc",
Status: 400,
},
qa.HTTPFailures[0],
},
Resource: ResourceServicePrincipal(),
Update: true,
Expand All @@ -337,12 +379,11 @@ func TestResourceServicePrincipalUpdate_ErrorPut(t *testing.T) {
allow_cluster_create = false
allow_instance_pool_create = true
`,
}.Apply(t)
require.Error(t, err, err)
}.ExpectError(t, "I'm a teapot")
}

func TestResourceServicePrincipalDelete(t *testing.T) {
d, err := qa.ResourceFixture{
qa.ResourceFixture{
Fixtures: []qa.HTTPFixture{
{
Method: "DELETE",
Expand All @@ -353,25 +394,16 @@ func TestResourceServicePrincipalDelete(t *testing.T) {
HCL: `display_name = "Squanchy"`,
Delete: true,
ID: "abc",
}.Apply(t)
require.NoError(t, err, err)
assert.Equal(t, "abc", d.Id(), "Id should not be empty")
}.ApplyNoError(t)
}

func TestResourceServicePrincipalDelete_Error(t *testing.T) {
_, err := qa.ResourceFixture{
Fixtures: []qa.HTTPFixture{
{
Method: "DELETE",
Resource: "/api/2.0/preview/scim/v2/ServicePrincipals/abc",
Status: 400,
},
},
qa.ResourceFixture{
Fixtures: qa.HTTPFailures,
Resource: ResourceServicePrincipal(),
Delete: true,
ID: "abc",
}.Apply(t)
require.Error(t, err, err)
}.ExpectError(t, "I'm a teapot")
}

func TestCreateForceOverridesManuallyAddedServicePrincipalErrorNotMatched(t *testing.T) {
Expand Down

0 comments on commit 3980839

Please sign in to comment.