`azurerm_databricks_access_connector` - add support for `UserManaged` identity. #21069

WodansSon · 2023-03-22T00:29:05Z

Upgrade the azurerm_databricks_access_connector API version from 2022-04-01-preview to 2022-10-01-preview which exposes UserManaged identities.

…d but not both can be...

WodansSon · 2023-03-24T10:34:08Z

WodansSon · 2023-03-24T23:25:04Z

…r-azurerm into e_databricks_user_assigned_identity

tombuildsstuff

hey @WodansSon

Thanks for this PR.

Taking a look through here whilst this is mostly looking good, we'll want to add a new type to the commonschema (and identity) packages here - we're intentionally no longer defining identity blocks within the Provider to ensure behavioural consistency across the provider. As such would you mind moving this logic there, introducing:

The SystemOrUserAssignedIdentity schema method in commonschema.
A typed model for SystemOrUserAssignedIdentity, as per the other types of Managed Identities in the identity package.
An Expand and Flatten function for SystemOrUserAssignedIdentity so that we can ensure these are handled consistently.

Since it's taken a lot of coordination to make the identity types consistent, we're being particularly cautious with these - as such we'll need these types to exist within the commonschema and identity types to move forward here, but once those are present we can switch over to using those and this should otherwise be good to go :)

Thanks!

tombuildsstuff · 2023-03-28T08:26:51Z

internal/services/databricks/databricks_access_connector_resource.go

 	}
 }

 func (r AccessConnectorResource) Update() sdk.ResourceFunc {
 	return sdk.ResourceFunc{
+		Timeout: 5 * time.Minute,


if we're rate limited we'll hit this, we should make this 30m

tombuildsstuff · 2023-03-28T08:26:58Z

internal/services/databricks/databricks_access_connector_resource.go

@@ -66,97 +107,125 @@ func (r AccessConnectorResource) IDValidationFunc() pluginsdk.SchemaValidateFunc

 func (r AccessConnectorResource) Create() sdk.ResourceFunc {
 	return sdk.ResourceFunc{
+		Timeout: 5 * time.Minute,


if we're rate limited we'll hit this, we should make this 30m

tombuildsstuff · 2023-03-28T08:27:11Z

internal/services/databricks/databricks_access_connector_resource.go

+		// UserAssigned Identity is supported for an Access Connector
+		// resource, not both together.') and only allows for a single 'identity_ids'
+		// to be passed...
+		"identity": {


rather than doing this, let's introduce a new commonschema type to support this, commonschema.SystemAssignedOrUserAssignedIdentity - identity is intentionally standardised across the provider now

I have opened PR #164 to address this comment...

tombuildsstuff · 2023-03-28T08:46:00Z

internal/services/databricks/databricks_access_connector_resource.go

+			if identityValue.Type == identity.TypeUserAssigned && len(identityValue.IdentityIds) == 0 {
+				return fmt.Errorf("`identity_ids` must be specified when `type` is set to %q", string(identity.TypeUserAssigned))
+			}


if we add an Expand and Flatten function to the identity package, then this logic can be made contained within the identity package, which'll ensure we keep this consistent - can we update this?

I have opened PR #164 to address this comment...

tombuildsstuff · 2023-03-28T09:14:23Z

internal/services/databricks/databricks_access_connector_resource_test.go

+func TestAccDatabricksAccessConnector_identityMissingIdentityIDsError(t *testing.T) {
+	data := acceptance.BuildTestData(t, "azurerm_databricks_access_connector", "test")
+	r := DatabricksAccessConnectorResource{}
+	data.ResourceTest(t, r, []acceptance.TestStep{
+		{
+			Config:      r.identityUserAssignedMissingIdentityIDs(data),
+			Check:       acceptance.ComposeTestCheckFunc(),
+			ExpectError: regexp.MustCompile(`must be specified when`),
+		},
+	})
+}
+
+func TestAccDatabricksAccessConnector_identityUserAssignedTooManyError(t *testing.T) {
+	data := acceptance.BuildTestData(t, "azurerm_databricks_access_connector", "test")
+	r := DatabricksAccessConnectorResource{}
+	data.ResourceTest(t, r, []acceptance.TestStep{
+		{
+			Config:      r.identityUserAssignedTooManyError(data),
+			Check:       acceptance.ComposeTestCheckFunc(),
+			ExpectError: regexp.MustCompile(`Too many list items`),
+		},
 	})


fwiw this logic should be handled by the identity package, so we should be able to remove these?

tombuildsstuff · 2023-03-28T09:15:13Z

website/docs/r/databricks_access_connector.html.markdown


 ## Attributes Reference

-The following attributes are exported:
+The following Attributes are exported in addition to the Arguments listed above:


(minor) looks like we use this phrasing in other resources, we should probably make this consistent:

Suggested change

The following Attributes are exported in addition to the Arguments listed above:

In addition to the Arguments listed above - the following Attributes are exported:

tombuildsstuff · 2023-03-30T14:59:38Z

hey @WodansSon

Thanks for pushing those changes.

Apologies for the late notice here, but I've just spotted that this is a duplicate of #21059 and as such I hope you don't mind but I'm going to close this in favour of #21059.

I've confirmed that whilst the Swagger defines SystemAssigned,UserAssigned as a value - that it neither supports this (or SystemAssigned, UserAssigned) as a value - as such I've sent this PR to correct the Swagger to account for this, since this use-case applies to several other APIs, which'll allow us to represent this correctly in the SDK too.

Whilst the Swagger does allow for MaxItems to be defined for the userAssignedIdentityIds field, unfortunately since the Swagger's don't generally define this value to allow for service expansion, whilst we can add a MaxItems: 1 to the identity_ids value here, we've intentionally avoided doing so at this time.

As such I hope you don't mind but since #21059 was opened before this I'm going to close this PR in favour of #21059, but once that's merged we should be good to go here 👍

Thanks!

github-actions · 2023-03-31T04:46:10Z

This functionality has been released in v3.50.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

github-actions · 2023-05-01T02:10:09Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

Add databricks/2022-10-01-preview/accessconnector

5fec0a3

WodansSon added dependencies service/databricks labels Mar 22, 2023

github-actions bot added the size/XL label Mar 22, 2023

WodansSon marked this pull request as draft March 22, 2023 00:29

Checking-in progress...

b80460c

github-actions bot added the documentation label Mar 22, 2023

WodansSon added the breaking-change label Mar 22, 2023

WodansSon changed the title ~~azurerm_databricks_access_connector - add support for user_managed_identity field~~ azurerm_databricks_access_connector - add support for UserManaged in type for identity code block Mar 22, 2023

WodansSon changed the title ~~azurerm_databricks_access_connector - add support for UserManaged in type for identity code block~~ azurerm_databricks_access_connector - add support for UserManaged in type field in the identity code block Mar 22, 2023

WodansSon changed the title ~~azurerm_databricks_access_connector - add support for UserManaged in type field in the identity code block~~ azurerm_databricks_access_connector - add support for UserManaged in the type field of the identity code block Mar 22, 2023

WodansSon added 2 commits March 22, 2023 02:23

Progress... I broke all the tests...

a401563

Go mod vendor...

b9500f7

github-actions bot added size/L and removed size/XL labels Mar 23, 2023

Check-in progress...

c8227a9

WodansSon removed the breaking-change label Mar 23, 2023

WodansSon changed the title ~~azurerm_databricks_access_connector - add support for UserManaged in the type field of the identity code block~~ azurerm_databricks_access_connector - add support for UserManaged and SystemManaged, UserManaged in identity code block. Mar 23, 2023

WodansSon added 4 commits March 23, 2023 02:28

Fix lint import error...

aa1b0ec

Merge branch 'main' into e_databricks_user_assigned_identity

9bf8b3d

Update AccessConnectorResourceModel...

f3cf481

Been learned tonight much has, either user assigned or system assigne…

067dbfd

…d but not both can be...

github-actions bot added size/XL and removed size/L labels Mar 24, 2023

Update test case name...

95e057f

WodansSon changed the title ~~azurerm_databricks_access_connector - add support for UserManaged and SystemManaged, UserManaged in identity code block.~~ azurerm_databricks_access_connector - add support for UserManaged identity. Mar 24, 2023

More learning there has been...

ade3541

WodansSon added this to the v3.50.0 milestone Mar 24, 2023

WodansSon marked this pull request as ready for review March 24, 2023 08:51

Add test cases and validation...

1d1562a

WodansSon added 2 commits March 25, 2023 00:44

Removing a superfluous space in docs...

f3002b6

Merge branch 'main' of https://github.com/hashicorp/terraform-provide…

1fc81e4

…r-azurerm into e_databricks_user_assigned_identity

github-actions bot added the waiting-response label Mar 28, 2023

tombuildsstuff suggested changes Mar 28, 2023

View reviewed changes

github-actions bot removed the waiting-response label Mar 28, 2023

WodansSon added 2 commits March 28, 2023 17:33

Address some of the PR comments...

e43bcd5

Add TODO comments per PR review...

7802cba

tombuildsstuff self-assigned this Mar 29, 2023

tombuildsstuff closed this Mar 30, 2023

github-actions bot locked as resolved and limited conversation to collaborators May 1, 2023

WodansSon deleted the e_databricks_user_assigned_identity branch October 26, 2023 04:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_databricks_access_connector` - add support for `UserManaged` identity. #21069

`azurerm_databricks_access_connector` - add support for `UserManaged` identity. #21069

WodansSon commented Mar 22, 2023 •

edited

Loading

WodansSon commented Mar 24, 2023

WodansSon commented Mar 24, 2023

tombuildsstuff left a comment

tombuildsstuff Mar 28, 2023

WodansSon Mar 28, 2023

tombuildsstuff Mar 28, 2023

WodansSon Mar 28, 2023

tombuildsstuff Mar 28, 2023

WodansSon Mar 29, 2023

tombuildsstuff Mar 28, 2023

WodansSon Mar 29, 2023

tombuildsstuff Mar 28, 2023

tombuildsstuff Mar 28, 2023

WodansSon Mar 28, 2023

tombuildsstuff commented Mar 30, 2023

github-actions bot commented Mar 31, 2023

github-actions bot commented May 1, 2023

	The following Attributes are exported in addition to the Arguments listed above:
	In addition to the Arguments listed above - the following Attributes are exported:

azurerm_databricks_access_connector - add support for UserManaged identity. #21069

azurerm_databricks_access_connector - add support for UserManaged identity. #21069

Conversation

WodansSon commented Mar 22, 2023 • edited Loading

WodansSon commented Mar 24, 2023

WodansSon commented Mar 24, 2023

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff commented Mar 30, 2023

github-actions bot commented Mar 31, 2023

github-actions bot commented May 1, 2023

`azurerm_databricks_access_connector` - add support for `UserManaged` identity. #21069

`azurerm_databricks_access_connector` - add support for `UserManaged` identity. #21069

WodansSon commented Mar 22, 2023 •

edited

Loading