Check both IsNull and IsUnknown in plan (#154)

CHANGELOG.md

@@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Fixed
+
+- Fixed an issue where changing `num_virtual_cpus` on a `cockroach_cluster` resource would fail to scale the cluster
+  and would result in an inconsistent state error.
+
 ## [0.7.0] - 2023-07-13
 
 ### Added

examples/workflows/cockroach_dedicated_cluster/main.tf

@@ -49,10 +49,10 @@ variable "storage_gib" {
   default  = 15
 }
 
-variable "machine_type" {
-  type     = string
+variable "num_virtual_cpus" {
+  type     = number
   nullable = false
-  default  = "n1-standard-2"
+  default  = 2
 }
 
 variable "allow_list_name" {
@@ -101,8 +101,8 @@ resource "cockroach_cluster" "example" {
   cloud_provider    = var.cloud_provider
   cockroach_version = var.cockroach_version
   dedicated = {
-    storage_gib  = var.storage_gib
-    machine_type = var.machine_type
+    storage_gib      = var.storage_gib
+    num_virtual_cpus = var.num_virtual_cpus
   }
   regions = [
     for r in var.cloud_provider_regions : {

internal/provider/allowlist_resource.go

@@ -142,7 +142,7 @@ func (r *allowListResource) Create(
 		Sql:      entry.Sql.ValueBool(),
 	}
 
-	if !entry.Name.IsNull() {
+	if IsKnown(entry.Name) {
 		name := entry.Name.ValueString()
 		allowList.Name = &name
 	}

internal/provider/client_ca_cert_resource.go

@@ -177,8 +177,8 @@ func (r *clientCACertResource) Update(ctx context.Context, req resource.UpdateRe
 
 	// Only update cert if non-null.
 	// Setting it to null will essentially cause TF to forget the field.
-	if plan.X509PemCert.IsNull() {
-		if !state.X509PemCert.IsNull() {
+	if !IsKnown(plan.X509PemCert) {
+		if IsKnown(state.X509PemCert) {
 			resp.Diagnostics.AddWarning(
 				"Client CA Cert will not be changed",
 				"Setting the cert field to null will not clear the cert, it will only remove it from Terraform state. Delete the resource to unset a cert.",

internal/provider/cluster_resource.go

@@ -327,7 +327,7 @@ func (r *clusterResource) Create(
 				usageLimits.RequestUnitLimit.ValueInt64(), usageLimits.StorageMibLimit.ValueInt64())
 		} else {
 			spendLimit := plan.ServerlessConfig.SpendLimit
-			if !spendLimit.IsNull() {
+			if IsKnown(spendLimit) {
 				val := int32(spendLimit.ValueInt64())
 				serverless.SpendLimit = &val
 			}
@@ -336,7 +336,7 @@ func (r *clusterResource) Create(
 		clusterSpec.SetServerless(*serverless)
 	} else if plan.DedicatedConfig != nil {
 		dedicated := client.DedicatedClusterCreateSpecification{}
-		if !plan.CockroachVersion.IsNull() {
+		if IsKnown(plan.CockroachVersion) {
 			version := plan.CockroachVersion.ValueString()
 			dedicated.CockroachVersion = &version
 		}
@@ -350,18 +350,18 @@ func (r *clusterResource) Create(
 		if cfg := plan.DedicatedConfig; cfg != nil {
 			hardware := client.DedicatedHardwareCreateSpecification{}
 			machineSpec := client.DedicatedMachineTypeSpecification{}
-			if !cfg.NumVirtualCpus.IsNull() {
+			if IsKnown(cfg.NumVirtualCpus) {
 				cpus := int32(cfg.NumVirtualCpus.ValueInt64())
 				machineSpec.NumVirtualCpus = &cpus
-			} else if !cfg.MachineType.IsNull() {
+			} else if IsKnown(cfg.MachineType) {
 				machineType := cfg.MachineType.ValueString()
 				machineSpec.MachineType = &machineType
 			}
 			hardware.MachineSpec = machineSpec
-			if !cfg.StorageGib.IsNull() {
+			if IsKnown(cfg.StorageGib) {
 				hardware.StorageGib = int32(cfg.StorageGib.ValueInt64())
 			}
-			if !cfg.DiskIops.IsNull() {
+			if IsKnown(cfg.DiskIops) {
 				diskiops := int32(cfg.DiskIops.ValueInt64())
 				hardware.DiskIops = &diskiops
 			}
@@ -423,7 +423,7 @@ func (r *clusterResource) Read(
 		return
 	}
 
-	if cluster.ID.IsNull() {
+	if !IsKnown(cluster.ID) {
 		return
 	}
 	clusterID := cluster.ID.ValueString()
@@ -559,7 +559,7 @@ func (r *clusterResource) Update(
 	}
 
 	// CRDB Versions
-	if !plan.CockroachVersion.IsNull() && plan.CockroachVersion != state.CockroachVersion {
+	if IsKnown(plan.CockroachVersion) && plan.CockroachVersion != state.CockroachVersion {
 		// Validate that the target version is valid.
 		planVersion := plan.CockroachVersion.ValueString()
 		stateVersion := state.CockroachVersion.ValueString()
@@ -669,19 +669,19 @@ func (r *clusterResource) Update(
 			}
 		}
 		dedicated.Hardware = client.NewDedicatedHardwareUpdateSpecification()
-		if !plan.DedicatedConfig.StorageGib.IsNull() {
+		if IsKnown(plan.DedicatedConfig.StorageGib) {
 			storage := int32(plan.DedicatedConfig.StorageGib.ValueInt64())
 			dedicated.Hardware.StorageGib = &storage
 		}
-		if !plan.DedicatedConfig.DiskIops.IsNull() {
+		if IsKnown(plan.DedicatedConfig.DiskIops) {
 			diskiops := int32(plan.DedicatedConfig.DiskIops.ValueInt64())
 			dedicated.Hardware.DiskIops = &diskiops
 		}
 		machineSpec := client.DedicatedMachineTypeSpecification{}
-		if !plan.DedicatedConfig.MachineType.IsNull() {
+		if IsKnown(plan.DedicatedConfig.MachineType) {
 			machineType := plan.DedicatedConfig.MachineType.ValueString()
 			machineSpec.MachineType = &machineType
-		} else if !plan.DedicatedConfig.NumVirtualCpus.IsNull() {
+		} else if IsKnown(plan.DedicatedConfig.NumVirtualCpus) {
 			cpus := int32(plan.DedicatedConfig.NumVirtualCpus.ValueInt64())
 			machineSpec.NumVirtualCpus = &cpus
 		}
@@ -734,7 +734,7 @@ func (r *clusterResource) Delete(
 	}
 
 	// Get cluster ID from state
-	if state.ID.IsNull() {
+	if !IsKnown(state.ID) {
 		return
 	}
 	clusterID := state.ID.ValueString()
@@ -850,7 +850,7 @@ func loadClusterToTerraformState(
 				RequestUnitLimit: types.Int64Value(usageLimits.RequestUnitLimit),
 				StorageMibLimit:  types.Int64Value(usageLimits.StorageMibLimit),
 			}
-		} else if clusterObj.Config.Serverless.SpendLimit != nil && planConfig != nil && !planConfig.SpendLimit.IsNull() {
+		} else if clusterObj.Config.Serverless.SpendLimit != nil && planConfig != nil && IsKnown(planConfig.SpendLimit) {
 			serverlessConfig.SpendLimit = types.Int64Value(int64(clusterObj.Config.Serverless.GetSpendLimit()))
 		}
 		state.ServerlessConfig = serverlessConfig

internal/provider/cluster_resource_test.go

@@ -618,13 +618,19 @@ func TestIntegrationDedicatedClusterResource(t *testing.T) {
 	finalizedCluster := upgradingCluster
 	finalizedCluster.UpgradeStatus = client.CLUSTERUPGRADESTATUSTYPE_FINALIZED
 
+	scaledCluster := finalizedCluster
+	scaledCluster.Config.Dedicated = &client.DedicatedHardwareConfig{}
+	*scaledCluster.Config.Dedicated = *finalizedCluster.Config.Dedicated
+	scaledCluster.Config.Dedicated.NumVirtualCpus = 4
+
+	httpOk := &http.Response{Status: http.StatusText(http.StatusOK)}
+
 	// Creation
 
 	s.EXPECT().CreateCluster(gomock.Any(), gomock.Any()).
 		Return(&cluster, nil, nil)
 	s.EXPECT().GetCluster(gomock.Any(), clusterID).
-		Return(&cluster, &http.Response{Status: http.StatusText(http.StatusOK)}, nil).
-		Times(8)
+		Return(&cluster, httpOk, nil).AnyTimes()
 
 	// Upgrade
 
@@ -639,66 +645,93 @@ func TestIntegrationDedicatedClusterResource(t *testing.T) {
 		},
 	}, nil, nil)
 	s.EXPECT().UpdateCluster(gomock.Any(), clusterID, &client.UpdateClusterSpecification{UpgradeStatus: &upgradingCluster.UpgradeStatus}).
-		Return(&upgradingCluster, &http.Response{Status: http.StatusText(http.StatusOK)}, nil)
-	s.EXPECT().GetCluster(gomock.Any(), clusterID).
-		Return(&pendingCluster, &http.Response{Status: http.StatusText(http.StatusOK)}, nil)
+		DoAndReturn(
+			func(context.Context, string, *client.UpdateClusterSpecification,
+			) (*client.Cluster, *http.Response, error) {
+				cluster = upgradingCluster
+				return &cluster, httpOk, nil
+			},
+		)
+
 	// Scale (no-op)
+
 	s.EXPECT().UpdateCluster(gomock.Any(), clusterID, gomock.Any()).
-		Return(&pendingCluster, &http.Response{Status: http.StatusText(http.StatusOK)}, nil)
-	s.EXPECT().GetCluster(gomock.Any(), clusterID).
-		Return(&pendingCluster, &http.Response{Status: http.StatusText(http.StatusOK)}, nil).
-		Times(3)
+		DoAndReturn(func(context.Context, string, *client.UpdateClusterSpecification,
+		) (*client.Cluster, *http.Response, error) {
+			cluster = pendingCluster
+			return &cluster, httpOk, nil
+		})
+
 	// Finalize
+
 	s.EXPECT().UpdateCluster(gomock.Any(), clusterID, &client.UpdateClusterSpecification{UpgradeStatus: &finalizedCluster.UpgradeStatus}).
-		Return(&finalizedCluster, &http.Response{Status: http.StatusText(http.StatusOK)}, nil)
+		DoAndReturn(func(context.Context, string, *client.UpdateClusterSpecification,
+		) (*client.Cluster, *http.Response, error) {
+			cluster = finalizedCluster
+			return &cluster, httpOk, nil
+		})
 
+	// Scale
+
+	s.EXPECT().UpdateCluster(gomock.Any(), clusterID, gomock.Any()).
+		DoAndReturn(func(context.Context, string, *client.UpdateClusterSpecification,
+		) (*client.Cluster, *http.Response, error) {
+			cluster = scaledCluster
+			return &cluster, httpOk, nil
+		})
 	// Deletion
 
-	s.EXPECT().GetCluster(gomock.Any(), clusterID).
-		Return(&finalizedCluster, &http.Response{Status: http.StatusText(http.StatusOK)}, nil).Times(6)
 	s.EXPECT().DeleteCluster(gomock.Any(), clusterID)
 
-	testDedicatedClusterResource(t, clusterName, true)
+	scaleStep := resource.TestStep{
+		Config: getTestDedicatedClusterResourceConfig(clusterName, latestClusterMajorVersion, false, 4),
+		Check:  resource.TestCheckResourceAttr("cockroach_cluster.dedicated", "dedicated.num_virtual_cpus", "4"),
+	}
+
+	testDedicatedClusterResource(t, clusterName, true, scaleStep)
 }
 
-func testDedicatedClusterResource(t *testing.T, clusterName string, useMock bool) {
+func testDedicatedClusterResource(t *testing.T, clusterName string, useMock bool, additionalSteps ...resource.TestStep) {
 	const (
 		resourceName   = "cockroach_cluster.dedicated"
 		dataSourceName = "data.cockroach_cluster.test"
 	)
 
+	testSteps := []resource.TestStep{
+		{
+			Config: getTestDedicatedClusterResourceConfig(clusterName, minSupportedClusterMajorVersion, false, 2),
+			Check: resource.ComposeTestCheckFunc(
+				testCheckCockroachClusterExists(resourceName),
+				resource.TestCheckResourceAttr(resourceName, "name", clusterName),
+				resource.TestCheckResourceAttrSet(resourceName, "cloud_provider"),
+				resource.TestCheckResourceAttrSet(resourceName, "cockroach_version"),
+				resource.TestCheckResourceAttr(resourceName, "plan", "DEDICATED"),
+				resource.TestCheckResourceAttr(dataSourceName, "name", clusterName),
+				resource.TestCheckResourceAttrSet(dataSourceName, "cloud_provider"),
+				resource.TestCheckResourceAttrSet(dataSourceName, "cockroach_version"),
+				resource.TestCheckResourceAttr(dataSourceName, "plan", "DEDICATED"),
+			),
+		},
+		{
+			Config: getTestDedicatedClusterResourceConfig(clusterName, latestClusterMajorVersion, true, 2),
+			Check: resource.ComposeTestCheckFunc(
+				resource.TestCheckResourceAttr(resourceName, "cockroach_version", latestClusterMajorVersion),
+				resource.TestCheckResourceAttr(dataSourceName, "cockroach_version", latestClusterMajorVersion),
+			),
+		},
+		{
+			ResourceName:      resourceName,
+			ImportState:       true,
+			ImportStateVerify: true,
+		},
+	}
+	testSteps = append(testSteps, additionalSteps...)
+
 	resource.Test(t, resource.TestCase{
 		IsUnitTest:               useMock,
 		PreCheck:                 func() { testAccPreCheck(t) },
 		ProtoV6ProviderFactories: testAccProtoV6ProviderFactories,
-		Steps: []resource.TestStep{
-			{
-				Config: getTestDedicatedClusterResourceConfig(clusterName, minSupportedClusterMajorVersion, false),
-				Check: resource.ComposeTestCheckFunc(
-					testCheckCockroachClusterExists(resourceName),
-					resource.TestCheckResourceAttr(resourceName, "name", clusterName),
-					resource.TestCheckResourceAttrSet(resourceName, "cloud_provider"),
-					resource.TestCheckResourceAttrSet(resourceName, "cockroach_version"),
-					resource.TestCheckResourceAttr(resourceName, "plan", "DEDICATED"),
-					resource.TestCheckResourceAttr(dataSourceName, "name", clusterName),
-					resource.TestCheckResourceAttrSet(dataSourceName, "cloud_provider"),
-					resource.TestCheckResourceAttrSet(dataSourceName, "cockroach_version"),
-					resource.TestCheckResourceAttr(dataSourceName, "plan", "DEDICATED"),
-				),
-			},
-			{
-				Config: getTestDedicatedClusterResourceConfig(clusterName, latestClusterMajorVersion, true),
-				Check: resource.ComposeTestCheckFunc(
-					resource.TestCheckResourceAttr(resourceName, "cockroach_version", latestClusterMajorVersion),
-					resource.TestCheckResourceAttr(dataSourceName, "cockroach_version", latestClusterMajorVersion),
-				),
-			},
-			{
-				ResourceName:      resourceName,
-				ImportState:       true,
-				ImportStateVerify: true,
-			},
-		},
+		Steps:                    testSteps,
 	})
 }
 
@@ -726,15 +759,15 @@ func testCheckCockroachClusterExists(resourceName string) resource.TestCheckFunc
 	}
 }
 
-func getTestDedicatedClusterResourceConfig(name, version string, finalize bool) string {
+func getTestDedicatedClusterResourceConfig(name, version string, finalize bool, vcpus int) string {
 	config := fmt.Sprintf(`
 resource "cockroach_cluster" "dedicated" {
     name           = "%s"
     cloud_provider = "GCP"
     cockroach_version = "%s"
     dedicated = {
 	  storage_gib = 15
-	  num_virtual_cpus = 2
+	  num_virtual_cpus = %d
     }
 	regions = [{
 		name: "us-central1"
@@ -745,7 +778,7 @@ resource "cockroach_cluster" "dedicated" {
 data "cockroach_cluster" "test" {
     id = cockroach_cluster.dedicated.id
 }
-`, name, version)
+`, name, version, vcpus)
 
 	if finalize {
 		config += fmt.Sprintf(`

internal/provider/cmek_resource.go

@@ -203,7 +203,7 @@ func (r *cmekResource) Read(
 	var cmek ClusterCMEK
 	diags := req.State.Get(ctx, &cmek)
 	resp.Diagnostics.Append(diags...)
-	if resp.Diagnostics.HasError() || cmek.ID.IsNull() {
+	if resp.Diagnostics.HasError() || !IsKnown(cmek.ID) {
 		return
 	}
 

internal/provider/connection_string_data_source.go

@@ -120,12 +120,12 @@ func (d *connectionStringDataSource) Read(
 		database = config.Database.ValueString()
 	}
 	var sqlUser *string
-	if !config.SqlUser.IsNull() {
+	if IsKnown(config.SqlUser) {
 		sqlUser = new(string)
 		*sqlUser = config.SqlUser.ValueString()
 	}
 	var os string
-	if !config.OS.IsNull() {
+	if IsKnown(config.OS) {
 		os = config.OS.ValueString()
 	} else {
 		switch runtime.GOOS {
@@ -154,7 +154,7 @@ func (d *connectionStringDataSource) Read(
 	connectionString := apiResp.GetConnectionString()
 	connectionParams := apiResp.GetParams()
 
-	if !config.Password.IsNull() {
+	if IsKnown(config.Password) {
 		connectionParams["Password"] = config.Password.ValueString()
 		if connectionURL, err := url.Parse(connectionString); err != nil {
 			resp.Diagnostics.AddWarning("Couldn't parse connection URL to inject password", err.Error())