[Fix] Only allow authorized_paths to be updated in the options field of databricks_catalog (#4517)

## Changes
There was a misunderstanding with the Unity Catalog API in connection
with
#4476.
Apparently, only `authorized_paths` is eligible for update in the UC
Catalog API. Otherwise, any other changes to `options` must trigger
resource recreation.

This PR customizes the logic of `databricks_catalog` to do this. To be
specific:
* `CustomizeDiff` marks the `options` field as `ForceNew` if there is a
key other than `authorized_paths` which was updated.
* The `Update` command removes any options that aren't
`authorized_paths`. This might be the case in the future if other
options are added for HMS catalogs that can be set at create time but
not updated.
* The `Create` command removes any options from the `UpdateCatalog` RPC.

## Tests
Added two integration tests to verify the behavior of
`databricks_catalog` update.
* Updated `TestUcAccCatalogUpdate` to verify that adding options with a
key other than `authorized_paths` to a catalog defined without any
options causes the catalog to be recreated.
* Added `TestUcAccCatalogHmsConnectionUpdate` which tests the lifecycle
of HMS catalogs, including updating the `authorized_paths` option,
enforcing that the catalog resource is never deleted during the
lifecycle of the test.

---------

Co-authored-by: Alex Ott <[email protected]>
Co-authored-by: vuong-nguyen <[email protected]>

Author: 3 people

NEXT_CHANGELOG.md

@@ -9,6 +9,7 @@
 
 ### Bug Fixes
 
+ * Only allow `authorized_paths` to be updated in the `options` field of `databricks_catalog` ([#4517](https://github.com/databricks/terraform-provider-databricks/pull/4517)).
  * Mark `default_catalog_name` attribute in `databricks_metastore_assignment` as deprecated ([#4522](https://github.com/databricks/terraform-provider-databricks/pull/4522))
  * Delete `databricks_sql_endpoint` that failed to start ([#4520](https://github.com/databricks/terraform-provider-databricks/pull/4520))
 

catalog/catalog_test.go

@@ -1,10 +1,17 @@
 package catalog_test
 
 import (
+	"context"
 	"fmt"
+	"regexp"
+	"strings"
 	"testing"
 
 	"github.com/databricks/terraform-provider-databricks/internal/acceptance"
+	"github.com/databricks/terraform-provider-databricks/qa"
+	tfjson "github.com/hashicorp/terraform-json"
+	"github.com/hashicorp/terraform-plugin-testing/helper/resource"
+	"github.com/hashicorp/terraform-plugin-testing/plancheck"
 )
 
 func TestUcAccCatalog(t *testing.T) {
@@ -55,6 +62,35 @@ func TestUcAccCatalogIsolated(t *testing.T) {
 	})
 }
 
+type checkResourceRecreate struct {
+	address string
+}
+
+func (c checkResourceRecreate) CheckPlan(ctx context.Context, req plancheck.CheckPlanRequest, resp *plancheck.CheckPlanResponse) {
+	var change *tfjson.ResourceChange
+	for _, resourceChange := range req.Plan.ResourceChanges {
+		if resourceChange.Address == c.address {
+			change = resourceChange
+			break
+		}
+	}
+	if change == nil {
+		addressesWithPlannedChanges := make([]string, 0, len(req.Plan.ResourceChanges))
+		for _, change := range req.Plan.ResourceChanges {
+			addressesWithPlannedChanges = append(addressesWithPlannedChanges, change.Address)
+		}
+		resp.Error = fmt.Errorf("address %s not found in resource changes; only planned changes for addresses %s", c.address, strings.Join(addressesWithPlannedChanges, ", "))
+		return
+	}
+	if change.Change.Actions[0] != tfjson.ActionDelete {
+		plannedActions := make([]string, 0, len(change.Change.Actions))
+		for _, action := range change.Change.Actions {
+			plannedActions = append(plannedActions, string(action))
+		}
+		resp.Error = fmt.Errorf("no delete is planned for %s; planned actions are: %s", c.address, strings.Join(plannedActions, ", "))
+	}
+}
+
 func TestUcAccCatalogUpdate(t *testing.T) {
 	acceptance.LoadUcwsEnv(t)
 	acceptance.UnityWorkspaceLevel(t, acceptance.Step{
@@ -100,5 +136,124 @@ func TestUcAccCatalogUpdate(t *testing.T) {
 			%s
 			owner = "{env.TEST_METASTORE_ADMIN_GROUP_NAME}"
 		}`, getPredictiveOptimizationSetting(t, false)),
+	}, acceptance.Step{
+		// Adding options should cause the catalog to be recreated.
+		Template: fmt.Sprintf(`
+		resource "databricks_catalog" "sandbox" {
+			name           = "sandbox{var.STICKY_RANDOM}"
+			comment        = "this catalog is managed by terraform - updated comment"
+			properties     = {
+				purpose = "testing"
+			}
+			options = {
+				user = "miles"
+			}
+			%s
+			owner = "{env.TEST_METASTORE_ADMIN_GROUP_NAME}"
+		}`, getPredictiveOptimizationSetting(t, false)),
+		ConfigPlanChecks: resource.ConfigPlanChecks{
+			PreApply: []plancheck.PlanCheck{
+				checkResourceRecreate{address: "databricks_catalog.sandbox"},
+			},
+		},
+	})
+}
+
+// Create a connection to an HMS catalog and update authorized_paths.
+func TestUcAccCatalogHmsConnectionUpdate(t *testing.T) {
+	authorizedPath := fmt.Sprintf("s3://%s/path/to/authorized", qa.RandomName("hms-bucket-"))
+	otherAuthorizedPath := fmt.Sprintf("s3://%s/path/to/authorized", qa.RandomName("hms-other-bucket-"))
+	otherInfra := fmt.Sprintf(`
+		resource "databricks_connection" "sandbox" {
+			name = "hms_connection{var.STICKY_RANDOM}"
+			connection_type = "HIVE_METASTORE"
+			comment         = "created in TestUcAccCatalogHmsConnectionUpdate"
+			options = {
+				host     = "test.mysql.database.azure.com"
+				port     = "3306"
+				user     = "user"
+				password = "password"
+				database = "metastore"
+				db_type  = "MYSQL"
+				version  = "2.3"
+			}
+			properties = {
+				purpose = "testing"
+			}
+		}
+		resource "databricks_storage_credential" "external" {
+			name = "cred-{var.STICKY_RANDOM}"
+			aws_iam_role {
+				role_arn = "{env.TEST_METASTORE_DATA_ACCESS_ARN}"
+			}
+			comment = "created in TestUcAccCatalogHmsConnectionUpdate"
+		}
+		resource "databricks_external_location" "sandbox" {
+		    name = "sandbox{var.STICKY_RANDOM}"
+			comment = "created in TestUcAccCatalogHmsConnectionUpdate"
+			url = "%s"
+			credential_name = "${databricks_storage_credential.external.name}"
+			skip_validation = true
+		}
+		resource "databricks_external_location" "sandbox-other" {
+		    name = "sandbox-other{var.STICKY_RANDOM}"
+			comment = "created in TestUcAccCatalogHmsConnectionUpdate"
+			url = "%s"
+			credential_name = "${databricks_storage_credential.external.name}"
+			skip_validation = true
+		}
+	`, authorizedPath, otherAuthorizedPath)
+	acceptance.UnityWorkspaceLevel(t, acceptance.Step{
+		Template: otherInfra + fmt.Sprintf(`
+		resource "databricks_catalog" "sandbox" {
+			name           = "sandbox{var.STICKY_RANDOM}"
+			comment        = "created in TestUcAccCatalogHmsConnectionUpdate"
+			connection_name = "${databricks_connection.sandbox.name}"
+			options = {
+				authorized_paths = "%s"
+			}
+			lifecycle {
+			    prevent_destroy = true
+			}
+			depends_on = [databricks_external_location.sandbox, databricks_external_location.sandbox-other]
+		}`, authorizedPath),
+	}, acceptance.Step{
+		Template: otherInfra + fmt.Sprintf(`
+		resource "databricks_catalog" "sandbox" {
+			name           = "sandbox{var.STICKY_RANDOM}"
+			comment        = "created in TestUcAccCatalogHmsConnectionUpdate"
+			connection_name = "${databricks_connection.sandbox.name}"
+			options = {
+				authorized_paths = "%s,%s"
+			}
+			lifecycle {
+			    prevent_destroy = true
+			}
+		}`, authorizedPath, otherAuthorizedPath),
+	}, acceptance.Step{
+		Template: otherInfra + fmt.Sprintf(`
+		resource "databricks_catalog" "sandbox" {
+			name           = "sandbox{var.STICKY_RANDOM}"
+			comment        = "created in TestUcAccCatalogHmsConnectionUpdate"
+			connection_name = "${databricks_connection.sandbox.name}"
+			options = {
+				authorized_paths = "%s,%s"
+				other_option = "value"
+			}
+			lifecycle {
+			    prevent_destroy = true
+			}
+		}`, authorizedPath, otherAuthorizedPath),
+		ExpectError: regexp.MustCompile("Instance cannot be destroyed"),
+	}, acceptance.Step{
+		Template: otherInfra + fmt.Sprintf(`
+		resource "databricks_catalog" "sandbox" {
+			name           = "sandbox{var.STICKY_RANDOM}"
+			comment        = "created in TestUcAccCatalogHmsConnectionUpdate"
+			connection_name = "${databricks_connection.sandbox.name}"
+			options = {
+				authorized_paths = "%s,%s"
+			}
+		}`, authorizedPath, otherAuthorizedPath),
 	})
 }

catalog/resource_catalog.go

@@ -94,6 +94,8 @@ func ResourceCatalog() common.Resource {
 			var updateCatalogRequest catalog.UpdateCatalog
 			common.DataToStructPointer(d, catalogSchema, &updateCatalogRequest)
 			updateCatalogRequest.Name = d.Id()
+			// Options must be set in the create request only (aside from HMS-backed catalogs).
+			updateCatalogRequest.Options = nil
 			_, err = w.Catalogs.Update(ctx, updateCatalogRequest)
 			if err != nil {
 				return err
@@ -151,6 +153,14 @@ func ResourceCatalog() common.Resource {
 			}
 
 			updateCatalogRequest.Owner = ""
+			// The only option allowed in update is "authorized_paths". All other options must be removed.
+			if opts := updateCatalogRequest.Options; opts != nil {
+				if v, ok := opts["authorized_paths"]; ok {
+					updateCatalogRequest.Options = map[string]string{"authorized_paths": v}
+				} else {
+					updateCatalogRequest.Options = nil
+				}
+			}
 			ci, err := w.Catalogs.Update(ctx, updateCatalogRequest)
 
 			if err != nil {
@@ -204,5 +214,33 @@ func ResourceCatalog() common.Resource {
 			}
 			return w.Catalogs.Delete(ctx, catalog.DeleteCatalogRequest{Force: force, Name: d.Id()})
 		},
+		CustomizeDiff: func(ctx context.Context, d *schema.ResourceDiff) error {
+			// The only scenario in which we can update options is for the `authorized_paths` key. Any
+			// other changes to the options field will result in an error.
+			if d.HasChange("options") {
+				old, new := d.GetChange("options")
+				oldMap := old.(map[string]interface{})
+				newMap := new.(map[string]interface{})
+				delete(oldMap, "authorized_paths")
+				delete(newMap, "authorized_paths")
+				// If any attribute other than `authorized_paths` is removed, the resource should be recreated.
+				for k := range oldMap {
+					if _, ok := newMap[k]; !ok {
+						if err := d.ForceNew("options"); err != nil {
+							return err
+						}
+					}
+				}
+				// If any attribute other than `authorized_paths` is added or changed, the resource should be recreated.
+				for k, v := range newMap {
+					if oldV, ok := oldMap[k]; !ok || oldV != v {
+						if err := d.ForceNew("options"); err != nil {
+							return err
+						}
+					}
+				}
+			}
+			return nil
+		},
 	}
 }

internal/acceptance/init.go

@@ -92,6 +92,7 @@ type Step struct {
 	Destroy                   bool
 	ExpectNonEmptyPlan        bool
 	ExpectError               *regexp.Regexp
+	ConfigPlanChecks          resource.ConfigPlanChecks
 	PlanOnly                  bool
 	PreventDiskCleanup        bool
 	PreventPostDestroyRefresh bool
@@ -219,6 +220,7 @@ func run(t *testing.T, steps []Step) {
 			Config:                               stepConfig,
 			Destroy:                              s.Destroy,
 			ExpectNonEmptyPlan:                   s.ExpectNonEmptyPlan,
+			ConfigPlanChecks:                     s.ConfigPlanChecks,
 			PlanOnly:                             s.PlanOnly,
 			PreventDiskCleanup:                   s.PreventDiskCleanup,
 			PreventPostDestroyRefresh:            s.PreventPostDestroyRefresh,