From b15409fc06f41c1b049e5981acf8d2a17f910754 Mon Sep 17 00:00:00 2001 From: Christopher Fitzner Date: Tue, 29 Oct 2024 16:23:07 -0700 Subject: [PATCH] [CC-30315] rely on cluster api for validating parent_id Previously, the cluster create and update operations would validate the parent_id manually via a separate API call. This validation is also done within the create and update api calls so these additional calls were unnecessary. We remove them here and add tests around setting this value which were previously missing. --- CHANGELOG.md | 3 + internal/provider/cluster_resource.go | 20 -- internal/provider/cluster_resource_test.go | 242 ++++++++++++++++++++- 3 files changed, 241 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 67a14bdb..7df400ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Changed +- Remove unnecessary api call to Folders endpoint to manually validate + `cockroach_cluster.parent_id`. + - Allow removal (deletion) of locked clusters. The api now supports deletion of locked clusters so we remove the wait prior to following through with the deletion. diff --git a/internal/provider/cluster_resource.go b/internal/provider/cluster_resource.go index 7238f9e9..885225a6 100644 --- a/internal/provider/cluster_resource.go +++ b/internal/provider/cluster_resource.go @@ -482,16 +482,6 @@ func (r *clusterResource) Create( if IsKnown(plan.ParentId) { parentID := plan.ParentId.ValueString() - if parentID != "root" { - traceAPICall("GetFolder") - _, _, err := r.provider.service.GetFolder(ctx, parentID) - if err != nil { - resp.Diagnostics.AddError( - "Error getting the parent folder", - fmt.Sprintf("Could not get the parent folder: %s", formatAPIErrorMessage(err))) - return - } - } clusterSpec.SetParentId(parentID) } @@ -956,16 +946,6 @@ func (r *clusterResource) Update( // Parent Id if IsKnown(plan.ParentId) { parentID := plan.ParentId.ValueString() - if parentID != "root" { - traceAPICall("GetFolder") - _, _, err := r.provider.service.GetFolder(ctx, parentID) - if err != nil { - resp.Diagnostics.AddError( - "Error getting the parent folder", - fmt.Sprintf("Could not get the parent folder: %s", formatAPIErrorMessage(err))) - return - } - } clusterReq.SetParentId(parentID) } diff --git a/internal/provider/cluster_resource_test.go b/internal/provider/cluster_resource_test.go index fcba0c0e..abb4e008 100644 --- a/internal/provider/cluster_resource_test.go +++ b/internal/provider/cluster_resource_test.go @@ -184,16 +184,16 @@ var secondUpdatedBackupConfig = &client.BackupConfiguration{ FrequencyMinutes: 1440, } -// TestAccBackupConfig is an acceptance test focused only on testing the -// backup_config parameter. It will be skipped if TF_ACC isn't set. +// TestAccClusterWithBackupConfig is an acceptance test focused only on testing +// the backup_config parameter. It will be skipped if TF_ACC isn't set. func TestAccClusterWithBackupConfig(t *testing.T) { t.Parallel() clusterName := fmt.Sprintf("%s-serverless-%s", tfTestPrefix, GenerateRandomString(2)) testClusterWithBackupConfig(t, clusterName, false /* useMock */) } -// TestAccBackupConfig is an acceptance test focused only on testing the -// backup_config parameter. It will be skipped if TF_ACC isn't set. +// TestIntegrationClusterWithBackupConfig is an integration test focused only on +// testing the backup_config parameter. func TestIntegrationClusterWithBackupConfig(t *testing.T) { clusterName := fmt.Sprintf("%s-serverless-%s", tfTestPrefix, GenerateRandomString(2)) clusterID := uuid.Nil.String() @@ -470,6 +470,198 @@ func checkBackupConfig(clusterResourceName string, expected *client.BackupConfig } } +var notFoundResourceId = "00000000-0000-0000-0000-000000000002" + +// TestAccClusterWithParentID is an acceptance test focused only on testing the +// parent_id parameter. It will be skipped if TF_ACC isn't set. +func TestAccClusterWithParentID(t *testing.T) { + t.Parallel() + clusterName := fmt.Sprintf("%s-parent-c-%s", tfTestPrefix, GenerateRandomString(2)) + folderName := fmt.Sprintf("%s-parent-f-%s", tfTestPrefix, GenerateRandomString(2)) + testClusterWithParentID(t, clusterName, folderName, false /* useMock */) +} + +// TestIntegrationClusterWithParentID is an integration test focused only on +// testing the parent_id parameter. +func TestIntegrationClusterWithParentID(t *testing.T) { + clusterName := fmt.Sprintf("%s-serverless-%s", tfTestPrefix, GenerateRandomString(2)) + clusterID := uuid.Nil.String() + if os.Getenv(CockroachAPIKey) == "" { + os.Setenv(CockroachAPIKey, "fake") + } + + ctrl := gomock.NewController(t) + s := mock_client.NewMockService(ctrl) + defer HookGlobal(&NewService, func(c *client.Client) client.Service { + return s + })() + + cluster := client.Cluster{ + Id: uuid.Nil.String(), + Name: clusterName, + CockroachVersion: latestClusterPatchVersion, + CloudProvider: "GCP", + State: "CREATED", + Plan: "STANDARD", + Config: client.ClusterConfig{ + Serverless: &client.ServerlessClusterConfig{ + UpgradeType: client.UPGRADETYPETYPE_AUTOMATIC, + UsageLimits: &client.UsageLimits{ + ProvisionedVirtualCpus: ptr(int64(2)), + }, + RoutingId: "routing-id", + }, + }, + Regions: []client.Region{ + { + Name: "us-central1", + }, + }, + ParentId: ptr("root"), + } + + folder := client.FolderResource{ + Name: "folder123", + ResourceId: "00000000-0000-0000-0000-000000000001", + ParentId: "root", + ResourceType: client.FOLDERRESOURCETYPETYPE_FOLDER, + } + + updatedCluster := cluster + updatedCluster.ParentId = ptr(folder.ResourceId) + + // Step: a cluster without a backup config block still has a default config + s.EXPECT().CreateCluster(gomock.Any(), gomock.Any()).Return(&cluster, nil, nil) + s.EXPECT().GetBackupConfiguration(gomock.Any(), clusterID).Return(initialBackupConfig, httpOk, nil).AnyTimes() + s.EXPECT().GetCluster(gomock.Any(), clusterID).Return(&cluster, httpOk, nil).Times(3) + + // Step: updating to explicit root is a no op + s.EXPECT().GetCluster(gomock.Any(), clusterID).Return(&cluster, httpOk, nil).Times(3) + + // Step: error case: values besides root and uuid are not allowed + // No API calls expected + + // Step: error case: valid uuid but not found results in reasonable error + s.EXPECT().GetCluster(gomock.Any(), clusterID).Return(&cluster, httpOk, nil) + s.EXPECT().UpdateCluster(gomock.Any(), clusterID, gomock.Any()).Return(nil, httpFail, fmt.Errorf("invalid argument: the parent must exist")) + + // Step: move a cluster under a non-root folder + s.EXPECT().GetCluster(gomock.Any(), clusterID).Return(&cluster, httpOk, nil) + s.EXPECT().CreateFolder(gomock.Any(), gomock.Any()).Return(&folder, httpOk, nil) + s.EXPECT().UpdateCluster(gomock.Any(), clusterID, gomock.Any()).Return(&updatedCluster, httpOk, nil) + s.EXPECT().GetCluster(gomock.Any(), clusterID).Return(&updatedCluster, httpOk, nil).Times(2) + s.EXPECT().ListFolders(gomock.Any(), gomock.Any()).Return(&client.ListFoldersResponse{ + Folders: []client.FolderResource{folder}, + }, httpOk, nil) + s.EXPECT().GetFolder(gomock.Any(), folder.ResourceId).Return(&folder, httpOk, nil) + + // Delete phase + s.EXPECT().GetCluster(gomock.Any(), clusterID).Return(&updatedCluster, httpOk, nil) + s.EXPECT().DeleteCluster(gomock.Any(), clusterID) + s.EXPECT().DeleteFolder(gomock.Any(), folder.ResourceId) + + testClusterWithParentID(t, clusterName, folder.Name, true /* useMock */) +} + +func testClusterWithParentID(t *testing.T, clusterName, folderName string, useMock bool) { + resource.Test(t, resource.TestCase{ + IsUnitTest: useMock, + PreCheck: func() { testAccPreCheck(t) }, + ProtoV6ProviderFactories: testAccProtoV6ProviderFactories, + Steps: []resource.TestStep{ + { + PreConfig: func() { + traceMessageStep("a cluster without a parent_id is allowed and gets the default of root") + }, + Config: getTestClusterWithParentFolder(clusterName, parentIDTestConfig{}), + Check: checkParentID(serverlessResourceName, "root"), + }, + { + PreConfig: func() { + traceMessageStep("updating to explicit root is a no op") + }, + Config: getTestClusterWithParentFolder(clusterName, parentIDTestConfig{folderID: ptr("root")}), + Check: checkParentID(serverlessResourceName, "root"), + }, + { + // Don't run error cases as the last step, or else it leave's the config in a bad state. + PreConfig: func() { + traceMessageStep("error case: values besides root and uuid are not allowed") + }, + Config: getTestClusterWithParentFolder(clusterName, parentIDTestConfig{folderID: ptr("123")}), + Check: checkParentID(serverlessResourceName, "root"), + ExpectError: regexp.MustCompile(`Attribute parent_id value must be a UUID or the string "root", got:`), + }, + { + // Don't run error cases as the last step, or else it leave's the config in a bad state. + PreConfig: func() { + traceMessageStep("error case: valid uuid but not found results in reasonable error") + }, + Config: getTestClusterWithParentFolder(clusterName, parentIDTestConfig{folderID: ¬FoundResourceId}), + Check: checkParentID(serverlessResourceName, "root"), + ExpectError: regexp.MustCompile(`invalid argument: the parent must exist`), + }, + { + PreConfig: func() { + traceMessageStep("move a cluster under a non-root folder") + }, + Config: getTestClusterWithParentFolder(clusterName, parentIDTestConfig{folderName: &folderName}), + Check: resource.ComposeTestCheckFunc( + checkParentID(serverlessResourceName, folderName), + traceEndOfPlan(), + ), + }, + }, + }) +} + +func checkParentID(clusterResourceName, parentFolderName string) resource.TestCheckFunc { + return func(s *terraform.State) error { + p := testAccProvider.(*provider) + p.service = NewService(cl) + + clusterRs, ok := s.RootModule().Resources[clusterResourceName] + if !ok { + return fmt.Errorf("not found: %s", clusterResourceName) + } + + clusterID := clusterRs.Primary.Attributes["id"] + + traceAPICall("GetCluster") + cluster, _, err := p.service.GetCluster(context.TODO(), clusterID) + if err != nil { + return fmt.Errorf("unexpected error during cluster lookup: %w", err) + } + clusterParentID := *cluster.ParentId + + if parentFolderName == "root" { + if clusterParentID != "root" { + return fmt.Errorf("expect parentID to be root but was: %s", clusterParentID) + } + return nil + } + + folderPath := fmt.Sprintf("/%s", parentFolderName) + traceAPICall("ListFolders") + foldersResp, _, err := p.service.ListFolders(context.TODO(), &client.ListFoldersOptions{ + Path: &folderPath, + }) + if err != nil { + return fmt.Errorf("unexpected error during folder lookup: %w", err) + } + if len(foldersResp.Folders) > 0 { + folder := foldersResp.Folders[0] + if folder.ResourceId != clusterParentID { + return fmt.Errorf("expect parentID to be %s but was: %s", folder.ResourceId, clusterParentID) + } + } else { + return fmt.Errorf("folder with path %s not found", folderPath) + } + + return nil + } +} + // TestAccMultiRegionServerlessClusterResource attempts to create, update, check, // and destroy a real multi-region serverless cluster. It will be skipped if // TF_ACC isn't set. @@ -1741,6 +1933,48 @@ func getTestClusterWithBackupConfig(clusterName string, config backupCreateConfi `, clusterName, backupConfigString) } +type parentIDTestConfig struct { + folderID *string + folderName *string +} + +func getTestClusterWithParentFolder(clusterName string, config parentIDTestConfig) string { + + parentConfigString := "" + parentIDString := "" + if config.folderID != nil { + parentIDString = fmt.Sprintf(` + parent_id = "%s" +`, *config.folderID) + } else if config.folderName != nil { + parentConfigString = fmt.Sprintf(` + resource "cockroach_folder" "parent" { + name = "%s" + parent_id = "root" + } +`, *config.folderName) + parentIDString = ` + parent_id = cockroach_folder.parent.id +` + } + + return fmt.Sprintf(`%s + resource "cockroach_cluster" "test" { + name = "%s" + cloud_provider = "GCP" + plan = "STANDARD" + serverless = { + usage_limits = { + provisioned_virtual_cpus = 2 + } + } + regions = [{ + name = "us-central1" + }] %s + } + `, parentConfigString, clusterName, parentIDString) +} + func TestSortRegionsByPlan(t *testing.T) { t.Run("Plan matches cluster", func(t *testing.T) { regions := []client.Region{