Fixed updating owners for UC resources (#3189)

tanmay-db · web-flow · commit 4213468d2c50 · 2024-02-02T14:01:24.000Z
* Fix updating owners

* fix tests

* -

* test

* integration test

* test

* Test

* Update connection resource

* restore connection

* restore connection

* update

* docs

* docs

* docs
diff --git a/catalog/resource_catalog.go b/catalog/resource_catalog.go
@@ -148,6 +148,10 @@ func ResourceCatalog() common.Resource {
 				}
 			}
 
+			if !d.HasChangeExcept("owner") {
+				return nil
+			}
+
 			updateCatalogRequest.Owner = ""
 			ci, err := w.Catalogs.Update(ctx, updateCatalogRequest)
 
diff --git a/catalog/resource_external_location.go b/catalog/resource_external_location.go
@@ -108,6 +108,10 @@ func ResourceExternalLocation() common.Resource {
 				}
 			}
 
+			if !d.HasChangeExcept("owner") {
+				return nil
+			}
+
 			updateExternalLocationRequest.Owner = ""
 			_, err = w.ExternalLocations.Update(ctx, updateExternalLocationRequest)
 			if err != nil {
diff --git a/catalog/resource_metastore.go b/catalog/resource_metastore.go
@@ -138,6 +138,11 @@ func ResourceMetastore() common.Resource {
 						return err
 					}
 				}
+
+				if !d.HasChangeExcept("owner") {
+					return nil
+				}
+
 				update.Owner = ""
 				_, err := acc.Metastores.Update(ctx, catalog.AccountsUpdateMetastore{
 					MetastoreId:   d.Id(),
@@ -171,6 +176,11 @@ func ResourceMetastore() common.Resource {
 						return err
 					}
 				}
+
+				if !d.HasChangeExcept("owner") {
+					return nil
+				}
+
 				update.Owner = ""
 				_, err := w.Metastores.Update(ctx, update)
 				if err != nil {
diff --git a/catalog/resource_metastore_test.go b/catalog/resource_metastore_test.go
@@ -172,14 +172,42 @@ func TestUpdateMetastore_NoChanges(t *testing.T) {
 				StorageRoot: "s3://b/abc",
 				Name:        "abc",
 			}, nil)
+		},
+		Resource:    ResourceMetastore(),
+		ID:          "abc",
+		Update:      true,
+		RequiresNew: true,
+		InstanceState: map[string]string{
+			"name":                "abc",
+			"storage_root":        "s3:/a",
+			"owner":               "admin",
+			"delta_sharing_scope": "INTERNAL_AND_EXTERNAL",
+			"delta_sharing_recipient_token_lifetime_in_seconds": "1002",
+		},
+		HCL: `
+		name = "abc"
+		storage_root = "s3:/a"
+		owner = "admin"
+		delta_sharing_scope = "INTERNAL_AND_EXTERNAL"
+		delta_sharing_recipient_token_lifetime_in_seconds = 1002
+		`,
+	}.ApplyNoError(t)
+}
+
+func TestUpdateMetastore_OnlyOwnerChanges(t *testing.T) {
+	qa.ResourceFixture{
+		MockWorkspaceClientFunc: func(w *mocks.MockWorkspaceClient) {
+			e := w.GetMockMetastoresAPI().EXPECT()
 			e.Update(mock.Anything, catalog.UpdateMetastore{
-				Id:                "abc",
-				Name:              "abc",
-				DeltaSharingScope: "INTERNAL_AND_EXTERNAL",
-				DeltaSharingRecipientTokenLifetimeInSeconds: 1002,
-				ForceSendFields: []string{"DeltaSharingRecipientTokenLifetimeInSeconds"},
+				Id:    "abc",
+				Owner: "updatedOwner",
 			}).Return(&catalog.MetastoreInfo{
-				Name:              "a",
+				Name:  "abc",
+				Owner: "updatedOwner",
+			}, nil)
+			e.GetById(mock.Anything, "abc").Return(&catalog.MetastoreInfo{
+				Name:              "abc",
+				Owner:             "updatedOwner",
 				DeltaSharingScope: "INTERNAL_AND_EXTERNAL",
 				DeltaSharingRecipientTokenLifetimeInSeconds: 1002,
 			}, nil)
@@ -198,14 +226,14 @@ func TestUpdateMetastore_NoChanges(t *testing.T) {
 		HCL: `
 		name = "abc"
 		storage_root = "s3:/a"
-		owner = "admin"
+		owner = "updatedOwner"
 		delta_sharing_scope = "INTERNAL_AND_EXTERNAL"
 		delta_sharing_recipient_token_lifetime_in_seconds = 1002
 		`,
 	}.ApplyNoError(t)
 }
 
-func TestUpdateMetastore_OwnerChanges(t *testing.T) {
+func TestUpdateMetastore_OwnerAndOtherChanges(t *testing.T) {
 	qa.ResourceFixture{
 		MockWorkspaceClientFunc: func(w *mocks.MockWorkspaceClient) {
 			e := w.GetMockMetastoresAPI().EXPECT()
@@ -220,19 +248,18 @@ func TestUpdateMetastore_OwnerChanges(t *testing.T) {
 				Id:                "abc",
 				Name:              "abc",
 				DeltaSharingScope: "INTERNAL_AND_EXTERNAL",
-				DeltaSharingRecipientTokenLifetimeInSeconds: 1002,
+				DeltaSharingRecipientTokenLifetimeInSeconds: 1004,
 				ForceSendFields: []string{"DeltaSharingRecipientTokenLifetimeInSeconds"},
 			}).Return(&catalog.MetastoreInfo{
-				Name:              "a",
 				Owner:             "updatedOwner",
 				DeltaSharingScope: "INTERNAL_AND_EXTERNAL",
-				DeltaSharingRecipientTokenLifetimeInSeconds: 1002,
+				DeltaSharingRecipientTokenLifetimeInSeconds: 1004,
 			}, nil)
 			e.GetById(mock.Anything, "abc").Return(&catalog.MetastoreInfo{
 				Name:              "abc",
 				Owner:             "updatedOwner",
 				DeltaSharingScope: "INTERNAL_AND_EXTERNAL",
-				DeltaSharingRecipientTokenLifetimeInSeconds: 1002,
+				DeltaSharingRecipientTokenLifetimeInSeconds: 1004,
 			}, nil)
 		},
 		Resource:    ResourceMetastore(),
@@ -251,7 +278,7 @@ func TestUpdateMetastore_OwnerChanges(t *testing.T) {
 		storage_root = "s3:/a"
 		owner = "updatedOwner"
 		delta_sharing_scope = "INTERNAL_AND_EXTERNAL"
-		delta_sharing_recipient_token_lifetime_in_seconds = 1002
+		delta_sharing_recipient_token_lifetime_in_seconds = 1004
 		`,
 	}.ApplyNoError(t)
 }
@@ -271,7 +298,7 @@ func TestUpdateMetastore_Rollback(t *testing.T) {
 				Id:                "abc",
 				Name:              "abc",
 				DeltaSharingScope: "INTERNAL_AND_EXTERNAL",
-				DeltaSharingRecipientTokenLifetimeInSeconds: 1002,
+				DeltaSharingRecipientTokenLifetimeInSeconds: 1004,
 				ForceSendFields: []string{"DeltaSharingRecipientTokenLifetimeInSeconds"},
 			}).Return(nil, errors.New("Something unexpected happened"))
 			e.Update(mock.Anything, catalog.UpdateMetastore{
@@ -298,7 +325,7 @@ func TestUpdateMetastore_Rollback(t *testing.T) {
 		storage_root = "s3:/a"
 		owner = "updatedOwner"
 		delta_sharing_scope = "INTERNAL_AND_EXTERNAL"
-		delta_sharing_recipient_token_lifetime_in_seconds = 1002
+		delta_sharing_recipient_token_lifetime_in_seconds = 1004
 		`,
 	}.Apply(t)
 	qa.AssertErrorStartsWith(t, err, "Something unexpected happened")
@@ -507,22 +534,6 @@ func TestUpdateAccountMetastore_NoChanges(t *testing.T) {
 	qa.ResourceFixture{
 		MockAccountClientFunc: func(a *mocks.MockAccountClient) {
 			e := a.GetMockAccountMetastoresAPI().EXPECT()
-			e.Update(mock.Anything, catalog.AccountsUpdateMetastore{
-				MetastoreId: "abc",
-				MetastoreInfo: &catalog.UpdateMetastore{
-					Id:                "abc",
-					Name:              "abc",
-					DeltaSharingScope: "INTERNAL_AND_EXTERNAL",
-					DeltaSharingRecipientTokenLifetimeInSeconds: 1002,
-					ForceSendFields: []string{"DeltaSharingRecipientTokenLifetimeInSeconds"},
-				},
-			}).Return(&catalog.AccountsMetastoreInfo{
-				MetastoreInfo: &catalog.MetastoreInfo{
-					Name:              "abc",
-					DeltaSharingScope: "INTERNAL_AND_EXTERNAL",
-					DeltaSharingRecipientTokenLifetimeInSeconds: 1002,
-				},
-			}, nil)
 			e.GetByMetastoreId(mock.Anything, "abc").Return(&catalog.AccountsMetastoreInfo{
 				MetastoreInfo: &catalog.MetastoreInfo{
 					StorageRoot: "s3://b/abc",
@@ -569,22 +580,6 @@ func TestUpdateAccountMetastore_OwnerChanges(t *testing.T) {
 					Owner: "updatedOwner",
 				},
 			}, nil)
-			e.Update(mock.Anything, catalog.AccountsUpdateMetastore{
-				MetastoreId: "abc",
-				MetastoreInfo: &catalog.UpdateMetastore{
-					Id:                "abc",
-					Name:              "abc",
-					DeltaSharingScope: "INTERNAL_AND_EXTERNAL",
-					DeltaSharingRecipientTokenLifetimeInSeconds: 1002,
-					ForceSendFields: []string{"DeltaSharingRecipientTokenLifetimeInSeconds"},
-				},
-			}).Return(&catalog.AccountsMetastoreInfo{
-				MetastoreInfo: &catalog.MetastoreInfo{
-					Name:              "abc",
-					DeltaSharingScope: "INTERNAL_AND_EXTERNAL",
-					DeltaSharingRecipientTokenLifetimeInSeconds: 1002,
-				},
-			}, nil)
 			e.GetByMetastoreId(mock.Anything, "abc").Return(&catalog.AccountsMetastoreInfo{
 				MetastoreInfo: &catalog.MetastoreInfo{
 					StorageRoot: "s3://b/abc",
@@ -637,7 +632,7 @@ func TestUpdateAccountMetastore_Rollback(t *testing.T) {
 					Id:                "abc",
 					Name:              "abc",
 					DeltaSharingScope: "INTERNAL_AND_EXTERNAL",
-					DeltaSharingRecipientTokenLifetimeInSeconds: 1002,
+					DeltaSharingRecipientTokenLifetimeInSeconds: 1004,
 					ForceSendFields: []string{"DeltaSharingRecipientTokenLifetimeInSeconds"},
 				},
 			}).Return(nil, errors.New("Something unexpected happened"))
@@ -671,7 +666,7 @@ func TestUpdateAccountMetastore_Rollback(t *testing.T) {
 		storage_root = "s3:/a"
 		owner = "updatedOwner"
 		delta_sharing_scope = "INTERNAL_AND_EXTERNAL"
-		delta_sharing_recipient_token_lifetime_in_seconds = 1002
+		delta_sharing_recipient_token_lifetime_in_seconds = 1004
 		`,
 	}.Apply(t)
 	qa.AssertErrorStartsWith(t, err, "Something unexpected happened")
diff --git a/catalog/resource_schema.go b/catalog/resource_schema.go
@@ -99,6 +99,10 @@ func ResourceSchema() common.Resource {
 				}
 			}
 
+			if !d.HasChangeExcept("owner") {
+				return nil
+			}
+
 			updateSchemaRequest.Owner = ""
 			schema, err := w.Schemas.Update(ctx, updateSchemaRequest)
 			if err != nil {
diff --git a/catalog/resource_share.go b/catalog/resource_share.go
@@ -237,6 +237,10 @@ func ResourceShare() common.Resource {
 				}
 			}
 
+			if !d.HasChangeExcept("owner") {
+				return nil
+			}
+
 			err = NewSharesAPI(ctx, c).update(d.Id(), ShareUpdates{
 				Updates: changes,
 			})
diff --git a/catalog/resource_storage_credential.go b/catalog/resource_storage_credential.go
@@ -152,6 +152,11 @@ func ResourceStorageCredential() common.Resource {
 						return err
 					}
 				}
+
+				if !d.HasChangeExcept("owner") {
+					return nil
+				}
+
 				update.Owner = ""
 				_, err := acc.StorageCredentials.Update(ctx, catalog.AccountsUpdateStorageCredential{
 					CredentialInfo:        &update,
@@ -191,6 +196,11 @@ func ResourceStorageCredential() common.Resource {
 						return err
 					}
 				}
+
+				if !d.HasChangeExcept("owner") {
+					return nil
+				}
+
 				update.Owner = ""
 				_, err = w.StorageCredentials.Update(ctx, update)
 				if err != nil {
diff --git a/catalog/resource_volume.go b/catalog/resource_volume.go
@@ -93,6 +93,10 @@ func ResourceVolume() common.Resource {
 				}
 			}
 
+			if !d.HasChangeExcept("owner") {
+				return nil
+			}
+
 			updateVolumeRequestContent.Owner = ""
 			v, err := w.Volumes.Update(ctx, updateVolumeRequestContent)
 			if err != nil {
diff --git a/docs/index.md b/docs/index.md
@@ -355,6 +355,10 @@ Except for metastore, metastore assignment and storage credential objects, Unity
 
 If you are configuring a new Databricks account for the first time, please create at least one workspace with an identity (user or service principal) that you intend to use for Unity Catalog rollout. You can then configure the provider using that identity and workspace to provision the required Unity Catalog resources.
 
+## Special considerations for Unity Catalog Resources
+
+When performing a single Terraform apply to update both the owner and other fields for Unity Catalog resources, the process first updates the owner, followed by the other fields using the new owner's permissions. If your principal is not the owner (specifically, the newly updated owner), you will not have the authority to modify those fields. In cases where you wish to change the owner to another individual and also update other fields, we recommend initially updating the fields using your principal, which should have owner permissions, and then updating the owner in a separate step.
+
 ## Miscellaneous configuration parameters
 
 !> **Warning** Combination of `debug_headers` and `debug_truncate_bytes` results in dumping of sensitive information to logs. Use it for troubleshooting purposes only.
diff --git a/internal/acceptance/catalog_test.go b/internal/acceptance/catalog_test.go
@@ -70,6 +70,16 @@ func TestUcAccCatalogUpdate(t *testing.T) {
 			}
 			owner = "account users"
 		}`,
+	}, step{
+		Template: `
+		resource "databricks_catalog" "sandbox" {
+			name           = "sandbox{var.STICKY_RANDOM}"
+			comment        = "this catalog is managed by terraform"
+			properties     = {
+				purpose = "testing"
+			}
+			owner = "{env.TEST_DATA_ENG_GROUP}"
+		}`,
 	}, step{
 		Template: `
 		resource "databricks_catalog" "sandbox" {
diff --git a/internal/acceptance/external_location_test.go b/internal/acceptance/external_location_test.go
@@ -91,6 +91,10 @@ func TestUcAccExternalLocationUpdate(t *testing.T) {
 		Template: storageCredentialTemplateWithOwner("Managed by TF -- Updated Comment", "account users") +
 			externalLocationTemplateWithOwner("Managed by TF -- Updated Comment", "account users") +
 			grantsTemplateForExternalLocation,
+	}, step{
+		Template: storageCredentialTemplateWithOwner("Managed by TF -- Updated Comment", "{env.TEST_DATA_ENG_GROUP}") +
+			externalLocationTemplateWithOwner("Managed by TF -- Updated Comment", "{env.TEST_DATA_ENG_GROUP}") +
+			grantsTemplateForExternalLocation,
 	}, step{
 		Template: storageCredentialTemplateWithOwner("Managed by TF -- Updated Comment 2", "{env.TEST_METASTORE_ADMIN_GROUP_NAME}") +
 			externalLocationTemplateWithOwner("Managed by TF -- Updated Comment 2", "{env.TEST_METASTORE_ADMIN_GROUP_NAME}") +
diff --git a/internal/acceptance/metastore_test.go b/internal/acceptance/metastore_test.go
@@ -110,6 +110,13 @@ func runMetastoreTestWithOwnerUpdates(t *testing.T, extraAttributes map[string]a
 			owner = "account users"
 			%s
 		}`, template),
+	}, step{
+		Template: fmt.Sprintf(`resource "databricks_metastore" "this" {
+			name = "{var.STICKY_RANDOM}"
+			force_destroy = true
+			owner = "{env.TEST_DATA_ENG_GROUP}"
+			%s
+		}`, template),
 	}, step{
 		Template: fmt.Sprintf(`resource "databricks_metastore" "this" {
 			name = "{var.STICKY_RANDOM}-updated"
diff --git a/internal/acceptance/recipient_test.go b/internal/acceptance/recipient_test.go
@@ -50,6 +50,8 @@ func TestUcAccUpdateRecipientDb2Open(t *testing.T) {
 		Template: recipientTemplateWithOwner("made by terraform", "account users"),
 	}, step{
 		Template: recipientTemplateWithOwner("made by terraform -- updated comment", "account users"),
+	}, step{
+		Template: recipientTemplateWithOwner("made by terraform -- updated comment", "{env.TEST_DATA_ENG_GROUP}"),
 	}, step{
 		Template: recipientTemplateWithOwner("made by terraform -- updated comment 2", "{env.TEST_METASTORE_ADMIN_GROUP_NAME}"),
 	})
diff --git a/internal/acceptance/schema_test.go b/internal/acceptance/schema_test.go
@@ -92,6 +92,8 @@ func TestUcAccSchemaUpdate(t *testing.T) {
 		Template: catalogTemplate + schemaTemplateWithOwner("this database is managed by terraform", "account users"),
 	}, step{
 		Template: catalogTemplate + schemaTemplateWithOwner("this database is managed by terraform -- updated comment", "account users"),
+	}, step{
+		Template: catalogTemplate + schemaTemplateWithOwner("this database is managed by terraform -- updated comment", "{env.TEST_DATA_ENG_GROUP}"),
 	}, step{
 		Template: catalogTemplate + schemaTemplateWithOwner("this database is managed by terraform -- updated comment 2", "{env.TEST_METASTORE_ADMIN_GROUP_NAME}"),
 	})
diff --git a/internal/acceptance/share_test.go b/internal/acceptance/share_test.go
@@ -129,6 +129,8 @@ func TestUcAccUpdateShare(t *testing.T) {
 		Template: preTestTemplate + preTestTemplateUpdate + shareTemplateWithOwner("c", "account users"),
 	}, step{
 		Template: preTestTemplate + preTestTemplateUpdate + shareTemplateWithOwner("e", "account users"),
+	}, step{
+		Template: preTestTemplate + preTestTemplateUpdate + shareTemplateWithOwner("e", "{env.TEST_DATA_ENG_GROUP}"),
 	}, step{
 		Template: preTestTemplate + preTestTemplateUpdate + shareTemplateWithOwner("f", "{env.TEST_METASTORE_ADMIN_GROUP_NAME}"),
 	})
diff --git a/internal/acceptance/volume_test.go b/internal/acceptance/volume_test.go
diff --git a/sharing/resource_recipient.go b/sharing/resource_recipient.go

Original file line number	Diff line number	Diff line change
`@@ -148,6 +148,10 @@ func ResourceCatalog() common.Resource {`
`148`	`148`	`}`
`149`	`149`	`}`
`150`	`150`
	`151`	`+ if !d.HasChangeExcept("owner") {`
	`152`	`+ return nil`
	`153`	`+ }`
	`154`	`+`
`151`	`155`	`updateCatalogRequest.Owner = ""`
`152`	`156`	`ci, err := w.Catalogs.Update(ctx, updateCatalogRequest)`
`153`	`157`
Original file line number	Diff line number	Diff line change
`@@ -108,6 +108,10 @@ func ResourceExternalLocation() common.Resource {`
`108`	`108`	`}`
`109`	`109`	`}`
`110`	`110`
	`111`	`+ if !d.HasChangeExcept("owner") {`
	`112`	`+ return nil`
	`113`	`+ }`
	`114`	`+`
`111`	`115`	`updateExternalLocationRequest.Owner = ""`
`112`	`116`	`_, err = w.ExternalLocations.Update(ctx, updateExternalLocationRequest)`
`113`	`117`	`if err != nil {`
Original file line number	Diff line number	Diff line change
`@@ -138,6 +138,11 @@ func ResourceMetastore() common.Resource {`
`138`	`138`	`return err`
`139`	`139`	`}`
`140`	`140`	`}`
	`141`	`+`
	`142`	`+ if !d.HasChangeExcept("owner") {`
	`143`	`+ return nil`
	`144`	`+ }`
	`145`	`+`
`141`	`146`	`update.Owner = ""`
`142`	`147`	`_, err := acc.Metastores.Update(ctx, catalog.AccountsUpdateMetastore{`
`143`	`148`	`MetastoreId: d.Id(),`
`@@ -171,6 +176,11 @@ func ResourceMetastore() common.Resource {`
`171`	`176`	`return err`
`172`	`177`	`}`
`173`	`178`	`}`
	`179`	`+`
	`180`	`+ if !d.HasChangeExcept("owner") {`
	`181`	`+ return nil`
	`182`	`+ }`
	`183`	`+`
`174`	`184`	`update.Owner = ""`
`175`	`185`	`_, err := w.Metastores.Update(ctx, update)`
`176`	`186`	`if err != nil {`
Original file line number	Diff line number	Diff line change
`@@ -99,6 +99,10 @@ func ResourceSchema() common.Resource {`
`99`	`99`	`}`
`100`	`100`	`}`
`101`	`101`
	`102`	`+ if !d.HasChangeExcept("owner") {`
	`103`	`+ return nil`
	`104`	`+ }`
	`105`	`+`
`102`	`106`	`updateSchemaRequest.Owner = ""`
`103`	`107`	`schema, err := w.Schemas.Update(ctx, updateSchemaRequest)`
`104`	`108`	`if err != nil {`
Original file line number	Diff line number	Diff line change
`@@ -237,6 +237,10 @@ func ResourceShare() common.Resource {`
`237`	`237`	`}`
`238`	`238`	`}`
`239`	`239`
	`240`	`+ if !d.HasChangeExcept("owner") {`
	`241`	`+ return nil`
	`242`	`+ }`
	`243`	`+`
`240`	`244`	`err = NewSharesAPI(ctx, c).update(d.Id(), ShareUpdates{`
`241`	`245`	`Updates: changes,`
`242`	`246`	`})`
Original file line number	Diff line number	Diff line change
`@@ -152,6 +152,11 @@ func ResourceStorageCredential() common.Resource {`
`152`	`152`	`return err`
`153`	`153`	`}`
`154`	`154`	`}`
	`155`	`+`
	`156`	`+ if !d.HasChangeExcept("owner") {`
	`157`	`+ return nil`
	`158`	`+ }`
	`159`	`+`
`155`	`160`	`update.Owner = ""`
`156`	`161`	`_, err := acc.StorageCredentials.Update(ctx, catalog.AccountsUpdateStorageCredential{`
`157`	`162`	`CredentialInfo: &update,`
`@@ -191,6 +196,11 @@ func ResourceStorageCredential() common.Resource {`
`191`	`196`	`return err`
`192`	`197`	`}`
`193`	`198`	`}`
	`199`	`+`
	`200`	`+ if !d.HasChangeExcept("owner") {`
	`201`	`+ return nil`
	`202`	`+ }`
	`203`	`+`
`194`	`204`	`update.Owner = ""`
`195`	`205`	`_, err = w.StorageCredentials.Update(ctx, update)`
`196`	`206`	`if err != nil {`
Original file line number	Diff line number	Diff line change
`@@ -93,6 +93,10 @@ func ResourceVolume() common.Resource {`
`93`	`93`	`}`
`94`	`94`	`}`
`95`	`95`
	`96`	`+ if !d.HasChangeExcept("owner") {`
	`97`	`+ return nil`
	`98`	`+ }`
	`99`	`+`
`96`	`100`	`updateVolumeRequestContent.Owner = ""`
`97`	`101`	`v, err := w.Volumes.Update(ctx, updateVolumeRequestContent)`
`98`	`102`	`if err != nil {`