Skip to content

Commit 1c84f9f

Browse files
Use filter option with service principal API when lookup variable used (#4913)
## Changes Use filter option with service principal API when lookup variable used ## Why Previously we were listing all SPs in the workspace which can be very slow. Using filter option in API allows to filter out the result quicker/ ## Tests Updated unit test <!-- If your PR needs to be included in the release notes for next release, add a separate entry in NEXT_CHANGELOG.md as part of your PR. --> --------- Co-authored-by: simon <4305831+simonfaltum@users.noreply.github.com>
1 parent 5019667 commit 1c84f9f

File tree

9 files changed

+93
-18
lines changed

9 files changed

+93
-18
lines changed

acceptance/bundle/variables/issue_3039_lookup_with_ref/output.txt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
Error: failed to resolve service-principal: TIDALDBServAccount - usdev, err: ServicePrincipal named 'TIDALDBServAccount - usdev' does not exist
1+
Error: failed to resolve service-principal: TIDALDBServAccount - usdev, err: service principal named "TIDALDBServAccount - usdev" does not exist
22

33
Name: issue-3039
44
Target: personal
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
bundle:
2+
name: test-bundle
3+
4+
variables:
5+
sp:
6+
lookup:
7+
service_principal: "deco-test-spn"
8+
9+
result:
10+
default: ${var.sp}

acceptance/bundle/variables/lookup/out.test.toml

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
2+
>>> [CLI] bundle validate -o json
3+
{
4+
"result": {
5+
"default": "[UUID]",
6+
"value": "[UUID]"
7+
},
8+
"sp": {
9+
"lookup": {
10+
"service_principal": "deco-test-spn"
11+
},
12+
"value": "[UUID]"
13+
}
14+
}
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
trace $CLI bundle validate -o json | jq '.variables'
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
Local = true
2+
Cloud = true
3+
4+
[[Server]]
5+
Pattern = "GET /api/2.0/preview/scim/v2/ServicePrincipals"
6+
Response.Body = '''{
7+
"Resources": [
8+
{
9+
"displayName": "deco-test-spn",
10+
"applicationId": "123e4567-e89b-12d3-a456-426614174000"
11+
}
12+
]
13+
}'''

bundle/config/mutator/resolve_lookup_variables_test.go

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package mutator
22

33
import (
4+
"fmt"
45
"testing"
56

67
"github.com/databricks/cli/bundle"
@@ -11,6 +12,7 @@ import (
1112
"github.com/stretchr/testify/require"
1213

1314
"github.com/databricks/databricks-sdk-go/experimental/mocks"
15+
"github.com/databricks/databricks-sdk-go/listing"
1416
"github.com/databricks/databricks-sdk-go/service/compute"
1517
"github.com/databricks/databricks-sdk-go/service/iam"
1618
)
@@ -131,11 +133,18 @@ func TestResolveServicePrincipal(t *testing.T) {
131133

132134
m := mocks.NewMockWorkspaceClient(t)
133135
b.SetWorkpaceClient(m.WorkspaceClient)
134-
spApi := m.GetMockServicePrincipalsAPI()
135-
spApi.EXPECT().GetByDisplayName(mock.Anything, spName).Return(&iam.ServicePrincipal{
136-
Id: "1234",
137-
ApplicationId: "app-1234",
138-
}, nil)
136+
137+
api := m.GetMockServicePrincipalsV2API()
138+
iterator := listing.SliceIterator[iam.ServicePrincipal]([]iam.ServicePrincipal{
139+
{
140+
ApplicationId: "app-1234",
141+
},
142+
})
143+
api.EXPECT().
144+
List(mock.Anything, iam.ListServicePrincipalsRequest{
145+
Filter: fmt.Sprintf("displayName eq '%s'", spName),
146+
}).
147+
Return(&iterator)
139148

140149
diags := bundle.Apply(t.Context(), b, ResolveLookupVariables())
141150
require.NoError(t, diags.Error())

bundle/config/variable/resolve_service_principal.go

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,11 @@ package variable
22

33
import (
44
"context"
5+
"fmt"
56

67
"github.com/databricks/databricks-sdk-go"
8+
"github.com/databricks/databricks-sdk-go/listing"
9+
"github.com/databricks/databricks-sdk-go/service/iam"
710
)
811

912
type resolveServicePrincipal struct {
@@ -12,11 +15,23 @@ type resolveServicePrincipal struct {
1215

1316
func (l resolveServicePrincipal) Resolve(ctx context.Context, w *databricks.WorkspaceClient) (string, error) {
1417
//nolint:staticcheck // this API is deprecated but we still need use it as there is no replacement yet.
15-
entity, err := w.ServicePrincipals.GetByDisplayName(ctx, l.name)
18+
it := w.ServicePrincipalsV2.List(ctx, iam.ListServicePrincipalsRequest{
19+
Filter: fmt.Sprintf("displayName eq '%s'", l.name),
20+
})
21+
22+
servicePrincipals, err := listing.ToSliceN(ctx, it, 1)
1623
if err != nil {
1724
return "", err
1825
}
19-
return entity.ApplicationId, nil
26+
if len(servicePrincipals) == 0 {
27+
return "", fmt.Errorf("service principal named %q does not exist", l.name)
28+
}
29+
30+
if len(servicePrincipals) > 1 {
31+
return "", fmt.Errorf("multiple service principals found with display name %q", l.name)
32+
}
33+
34+
return servicePrincipals[0].ApplicationId, nil
2035
}
2136

2237
func (l resolveServicePrincipal) String() string {

bundle/config/variable/resolve_service_principal_test.go

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@ package variable
33
import (
44
"testing"
55

6-
"github.com/databricks/databricks-sdk-go/apierr"
76
"github.com/databricks/databricks-sdk-go/experimental/mocks"
7+
"github.com/databricks/databricks-sdk-go/listing"
88
"github.com/databricks/databricks-sdk-go/service/iam"
99
"github.com/stretchr/testify/assert"
1010
"github.com/stretchr/testify/mock"
@@ -14,12 +14,17 @@ import (
1414
func TestResolveServicePrincipal_ResolveSuccess(t *testing.T) {
1515
m := mocks.NewMockWorkspaceClient(t)
1616

17-
api := m.GetMockServicePrincipalsAPI()
18-
api.EXPECT().
19-
GetByDisplayName(mock.Anything, "service-principal").
20-
Return(&iam.ServicePrincipal{
17+
api := m.GetMockServicePrincipalsV2API()
18+
iterator := listing.SliceIterator[iam.ServicePrincipal]([]iam.ServicePrincipal{
19+
{
2120
ApplicationId: "5678",
22-
}, nil)
21+
},
22+
})
23+
api.EXPECT().
24+
List(mock.Anything, iam.ListServicePrincipalsRequest{
25+
Filter: "displayName eq 'service-principal'",
26+
}).
27+
Return(&iterator)
2328

2429
ctx := t.Context()
2530
l := resolveServicePrincipal{name: "service-principal"}
@@ -31,15 +36,18 @@ func TestResolveServicePrincipal_ResolveSuccess(t *testing.T) {
3136
func TestResolveServicePrincipal_ResolveNotFound(t *testing.T) {
3237
m := mocks.NewMockWorkspaceClient(t)
3338

34-
api := m.GetMockServicePrincipalsAPI()
39+
api := m.GetMockServicePrincipalsV2API()
40+
iterator := listing.SliceIterator[iam.ServicePrincipal]([]iam.ServicePrincipal{})
3541
api.EXPECT().
36-
GetByDisplayName(mock.Anything, "service-principal").
37-
Return(nil, &apierr.APIError{StatusCode: 404})
42+
List(mock.Anything, iam.ListServicePrincipalsRequest{
43+
Filter: "displayName eq 'service-principal'",
44+
}).
45+
Return(&iterator)
3846

3947
ctx := t.Context()
4048
l := resolveServicePrincipal{name: "service-principal"}
4149
_, err := l.Resolve(ctx, m.WorkspaceClient)
42-
require.ErrorIs(t, err, apierr.ErrNotFound)
50+
require.ErrorContains(t, err, "service principal named \"service-principal\" does not exist")
4351
}
4452

4553
func TestResolveServicePrincipal_String(t *testing.T) {

0 commit comments

Comments
 (0)