Skip to content

Commit bc37c9b

Browse files
nfxnfx
authored
Fix updating databricks_service_principal on Azure (#1322)
* propagate `application_id` on SCIM PUT call for SPN on Azure * behavior should not affect AWS Fix #1319
1 parent 94a6474 commit bc37c9b

File tree

4 files changed

+143
-106
lines changed

4 files changed

+143
-106
lines changed

‎common/version.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ package common
33
import "context"
44

55
var (
6-
version = "0.5.7"
6+
version = "0.5.8"
77
// ResourceName is resource name without databricks_ prefix
88
ResourceName contextKey = 1
99
// Provider is the current instance of provider

‎scim/resource_service_principal.go‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -125,11 +125,16 @@ func ResourceServicePrincipal() *schema.Resource {
125125
return sp.Entitlements.readIntoData(d)
126126
},
127127
Update: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
128+
var applicationId string
129+
if c.IsAzure() {
130+
applicationId = d.Get("application_id").(string)
131+
}
128132
return NewServicePrincipalsAPI(ctx, c).Update(d.Id(), User{
129-
DisplayName: d.Get("display_name").(string),
130-
Active: d.Get("active").(bool),
131-
Entitlements: readEntitlementsFromData(d),
132-
ExternalID: d.Get("external_id").(string),
133+
DisplayName: d.Get("display_name").(string),
134+
Active: d.Get("active").(bool),
135+
Entitlements: readEntitlementsFromData(d),
136+
ExternalID: d.Get("external_id").(string),
137+
ApplicationID: applicationId,
133138
})
134139
},
135140
Delete: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {

‎scim/resource_service_principal_test.go‎

Lines changed: 132 additions & 100 deletions
Original file line numberDiff line numberDiff line change
@@ -52,22 +52,19 @@ func TestAccServicePrincipalOnAzure(t *testing.T) {
5252
}
5353

5454
func TestResourceServicePrincipalRead(t *testing.T) {
55-
d, err := qa.ResourceFixture{
55+
qa.ResourceFixture{
5656
Fixtures: []qa.HTTPFixture{
5757
{
5858
Method: "GET",
5959
Resource: "/api/2.0/preview/scim/v2/ServicePrincipals/abc",
6060
Response: User{
61-
ID: "abc",
62-
DisplayName: "Example Service Principal",
63-
Groups: []ComplexValue{
64-
{
65-
Display: "admins",
66-
Value: "4567",
67-
},
61+
ID: "abc",
62+
ApplicationID: "bcd",
63+
DisplayName: "Example Service Principal",
64+
Active: true,
65+
Entitlements: []ComplexValue{
6866
{
69-
Display: "ds",
70-
Value: "9877",
67+
Value: "allow-cluster-create",
7168
},
7269
},
7370
},
@@ -78,11 +75,11 @@ func TestResourceServicePrincipalRead(t *testing.T) {
7875
New: true,
7976
Read: true,
8077
ID: "abc",
81-
}.Apply(t)
82-
require.NoError(t, err, err)
83-
assert.Equal(t, "abc", d.Id(), "Id should not be empty")
84-
assert.Equal(t, "Example Service Principal", d.Get("display_name"))
85-
assert.Equal(t, false, d.Get("allow_cluster_create"))
78+
}.ApplyAndExpectData(t, map[string]interface{}{
79+
"display_name": "Example Service Principal",
80+
"application_id": "bcd",
81+
"allow_cluster_create": true,
82+
})
8683
}
8784

8885
func TestResourceServicePrincipalRead_NotFound(t *testing.T) {
@@ -125,7 +122,7 @@ func TestResourceServicePrincipalRead_Error(t *testing.T) {
125122
}
126123

127124
func TestResourceServicePrincipalCreate(t *testing.T) {
128-
d, err := qa.ResourceFixture{
125+
qa.ResourceFixture{
129126
Fixtures: []qa.HTTPFixture{
130127
{
131128
Method: "POST",
@@ -175,59 +172,31 @@ func TestResourceServicePrincipalCreate(t *testing.T) {
175172
display_name = "Example Service Principal"
176173
allow_cluster_create = true
177174
`,
178-
}.Apply(t)
179-
require.NoError(t, err, err)
180-
assert.Equal(t, "abc", d.Id(), "Id should not be empty")
181-
assert.Equal(t, "Example Service Principal", d.Get("display_name"))
182-
assert.Equal(t, true, d.Get("allow_cluster_create"))
175+
}.ApplyNoError(t)
183176
}
184177

185178
func TestResourceServicePrincipalCreate_Error(t *testing.T) {
186-
_, err := qa.ResourceFixture{
187-
Fixtures: []qa.HTTPFixture{
188-
{
189-
Method: "POST",
190-
Resource: "/api/2.0/preview/scim/v2/ServicePrincipals",
191-
Status: 400,
192-
},
193-
},
179+
qa.ResourceFixture{
180+
Fixtures: qa.HTTPFailures,
194181
Resource: ResourceServicePrincipal(),
195182
Create: true,
196183
HCL: `
197184
display_name = "Example Service Principal"
198185
allow_cluster_create = true
199186
`,
200-
}.Apply(t)
201-
require.Error(t, err, err)
187+
}.ExpectError(t, "I'm a teapot")
202188
}
203189

204-
func TestResourceServicePrincipalUpdate(t *testing.T) {
205-
newServicePrincipal := User{
206-
Schemas: []URN{ServicePrincipalSchema},
207-
DisplayName: "Changed Name",
208-
Active: true,
209-
Entitlements: entitlements{
210-
{
211-
Value: "allow-instance-pool-create",
212-
},
213-
},
214-
Groups: []ComplexValue{
215-
{
216-
Display: "admins",
217-
Value: "4567",
218-
},
219-
{
220-
Display: "ds",
221-
Value: "9877",
222-
},
223-
},
224-
}
225-
d, err := qa.ResourceFixture{
190+
func TestResourceServicePrincipalUpdateOnAWS(t *testing.T) {
191+
qa.ResourceFixture{
226192
Fixtures: []qa.HTTPFixture{
227193
{
228194
Method: "GET",
229195
Resource: "/api/2.0/preview/scim/v2/ServicePrincipals/abc",
230196
Response: User{
197+
// application ID is created by platform on AWS
198+
ApplicationID: "existing-application-id",
199+
231200
DisplayName: "Example Service Principal",
232201
Active: true,
233202
ID: "abc",
@@ -249,14 +218,55 @@ func TestResourceServicePrincipalUpdate(t *testing.T) {
249218
},
250219
},
251220
{
252-
Method: "PUT",
253-
Resource: "/api/2.0/preview/scim/v2/ServicePrincipals/abc",
254-
ExpectedRequest: newServicePrincipal,
221+
Method: "PUT",
222+
Resource: "/api/2.0/preview/scim/v2/ServicePrincipals/abc",
223+
ExpectedRequest: User{
224+
// application ID is not allowed to be modified by client side on AWS
225+
226+
Schemas: []URN{ServicePrincipalSchema},
227+
DisplayName: "Changed Name",
228+
Active: true,
229+
Entitlements: entitlements{
230+
{
231+
Value: "allow-instance-pool-create",
232+
},
233+
},
234+
Groups: []ComplexValue{
235+
{
236+
Display: "admins",
237+
Value: "4567",
238+
},
239+
{
240+
Display: "ds",
241+
Value: "9877",
242+
},
243+
},
244+
},
255245
},
256246
{
257247
Method: "GET",
258248
Resource: "/api/2.0/preview/scim/v2/ServicePrincipals/abc",
259-
Response: newServicePrincipal,
249+
Response: User{
250+
Schemas: []URN{ServicePrincipalSchema},
251+
ApplicationID: "existing-application-id",
252+
DisplayName: "Changed Name",
253+
Active: true,
254+
Entitlements: entitlements{
255+
{
256+
Value: "allow-instance-pool-create",
257+
},
258+
},
259+
Groups: []ComplexValue{
260+
{
261+
Display: "admins",
262+
Value: "4567",
263+
},
264+
{
265+
Display: "ds",
266+
Value: "9877",
267+
},
268+
},
269+
},
260270
},
261271
},
262272
Resource: ResourceServicePrincipal(),
@@ -267,37 +277,83 @@ func TestResourceServicePrincipalUpdate(t *testing.T) {
267277
allow_cluster_create = false
268278
allow_instance_pool_create = true
269279
`,
270-
}.Apply(t)
271-
require.NoError(t, err, err)
272-
assert.Equal(t, "abc", d.Id(), "Id should not be empty")
273-
assert.Equal(t, "Changed Name", d.Get("display_name"))
274-
assert.Equal(t, false, d.Get("allow_cluster_create"))
275-
assert.Equal(t, true, d.Get("allow_instance_pool_create"))
280+
}.ApplyAndExpectData(t, map[string]interface{}{
281+
"display_name": "Changed Name",
282+
"allow_cluster_create": false,
283+
"allow_instance_pool_create": true,
284+
})
276285
}
277286

278-
func TestResourceServicePrincipalUpdate_Error(t *testing.T) {
279-
_, err := qa.ResourceFixture{
287+
// https://github.com/databrickslabs/terraform-provider-databricks/issues/1319
288+
func TestResourceServicePrincipalUpdateOnAzure(t *testing.T) {
289+
qa.ResourceFixture{
290+
Azure: true,
280291
Fixtures: []qa.HTTPFixture{
281292
{
282293
Method: "GET",
283294
Resource: "/api/2.0/preview/scim/v2/ServicePrincipals/abc",
284-
Status: 400,
295+
Response: User{
296+
// application id is specified by user on Azure
297+
ApplicationID: "existing-application-id",
298+
299+
DisplayName: "Example Service Principal",
300+
Active: true,
301+
ID: "abc",
302+
},
303+
},
304+
{
305+
Method: "PUT",
306+
Resource: "/api/2.0/preview/scim/v2/ServicePrincipals/abc",
307+
ExpectedRequest: User{
308+
// application id is specified by user on Azure and also must be part of modification
309+
ApplicationID: "existing-application-id",
310+
311+
Schemas: []URN{ServicePrincipalSchema},
312+
DisplayName: "Changed Name",
313+
Active: true,
314+
},
315+
},
316+
{
317+
Method: "GET",
318+
Resource: "/api/2.0/preview/scim/v2/ServicePrincipals/abc",
319+
Response: User{
320+
Schemas: []URN{ServicePrincipalSchema},
321+
ApplicationID: "existing-application-id",
322+
DisplayName: "Changed Name",
323+
Active: true,
324+
},
285325
},
286326
},
287327
Resource: ResourceServicePrincipal(),
288328
Update: true,
289329
ID: "abc",
330+
InstanceState: map[string]string{
331+
"application_id": "existing-application-id",
332+
"display_name": "Example Service Principal",
333+
},
334+
HCL: `
335+
application_id = "existing-application-id"
336+
display_name = "Changed Name"
337+
`,
338+
}.ApplyNoError(t)
339+
}
340+
341+
func TestResourceServicePrincipalUpdate_Error(t *testing.T) {
342+
qa.ResourceFixture{
343+
Fixtures: qa.HTTPFailures,
344+
Resource: ResourceServicePrincipal(),
345+
Update: true,
346+
ID: "abc",
290347
HCL: `
291348
display_name = "Changed Name"
292349
allow_cluster_create = false
293350
allow_instance_pool_create = true
294351
`,
295-
}.Apply(t)
296-
require.Error(t, err, err)
352+
}.ExpectError(t, "I'm a teapot")
297353
}
298354

299355
func TestResourceServicePrincipalUpdate_ErrorPut(t *testing.T) {
300-
_, err := qa.ResourceFixture{
356+
qa.ResourceFixture{
301357
Fixtures: []qa.HTTPFixture{
302358
{
303359
Method: "GET",
@@ -311,23 +367,9 @@ func TestResourceServicePrincipalUpdate_ErrorPut(t *testing.T) {
311367
Value: "allow-cluster-create",
312368
},
313369
},
314-
Groups: []ComplexValue{
315-
{
316-
Display: "admins",
317-
Value: "4567",
318-
},
319-
{
320-
Display: "ds",
321-
Value: "9877",
322-
},
323-
},
324370
},
325371
},
326-
{
327-
Method: "PUT",
328-
Resource: "/api/2.0/preview/scim/v2/ServicePrincipals/abc",
329-
Status: 400,
330-
},
372+
qa.HTTPFailures[0],
331373
},
332374
Resource: ResourceServicePrincipal(),
333375
Update: true,
@@ -337,12 +379,11 @@ func TestResourceServicePrincipalUpdate_ErrorPut(t *testing.T) {
337379
allow_cluster_create = false
338380
allow_instance_pool_create = true
339381
`,
340-
}.Apply(t)
341-
require.Error(t, err, err)
382+
}.ExpectError(t, "I'm a teapot")
342383
}
343384

344385
func TestResourceServicePrincipalDelete(t *testing.T) {
345-
d, err := qa.ResourceFixture{
386+
qa.ResourceFixture{
346387
Fixtures: []qa.HTTPFixture{
347388
{
348389
Method: "DELETE",
@@ -353,25 +394,16 @@ func TestResourceServicePrincipalDelete(t *testing.T) {
353394
HCL: `display_name = "Squanchy"`,
354395
Delete: true,
355396
ID: "abc",
356-
}.Apply(t)
357-
require.NoError(t, err, err)
358-
assert.Equal(t, "abc", d.Id(), "Id should not be empty")
397+
}.ApplyNoError(t)
359398
}
360399

361400
func TestResourceServicePrincipalDelete_Error(t *testing.T) {
362-
_, err := qa.ResourceFixture{
363-
Fixtures: []qa.HTTPFixture{
364-
{
365-
Method: "DELETE",
366-
Resource: "/api/2.0/preview/scim/v2/ServicePrincipals/abc",
367-
Status: 400,
368-
},
369-
},
401+
qa.ResourceFixture{
402+
Fixtures: qa.HTTPFailures,
370403
Resource: ResourceServicePrincipal(),
371404
Delete: true,
372405
ID: "abc",
373-
}.Apply(t)
374-
require.Error(t, err, err)
406+
}.ExpectError(t, "I'm a teapot")
375407
}
376408

377409
func TestCreateForceOverridesManuallyAddedServicePrincipalErrorNotMatched(t *testing.T) {

‎scripts/nightly/main.tf‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ output "tags" {
1515
}
1616

1717
data "external" "env" {
18-
program = ["python", "-c", "import sys,os,json;json.dump(dict(os.environ), sys.stdout)"]
18+
program = ["python3", "-c", "import sys,os,json;json.dump(dict(os.environ), sys.stdout)"]
1919
}
2020

2121
data "external" "me" {

0 commit comments

Comments
 (0)