fix: Fix tests and relax warehouse validations (#2959)

- Unskip cortex acceptance test - Fix application package tests after recent SF release - Relax warehouse parameter validations upper bounds - Fix user tests after validation change for one of the params - Fix cortex grant tests (they started to work) References: #2948
Snowflake-Labs · Jul 26, 2024 · dd01ce9 · dd01ce9
1 parent 6781133
commit dd01ce9
Show file tree

Hide file tree

Showing 12 changed files with 114 additions and 32 deletions.
diff --git a/MIGRATION_GUIDE.md b/MIGRATION_GUIDE.md
@@ -83,6 +83,10 @@ Added a new datasource enabling querying and filtering network policies. Notes:
   The additional parameters call **DESC NETWORK POLICY** (with `with_describe` turned on) **per network policy** returned by **SHOW NETWORK POLICIES**.
   It's important to limit the records and calls to Snowflake to the minimum. That's why we recommend assessing which information you need from the data source and then providing strong filters and turning off additional fields for better plan performance.
 
+### *(fix)* snowflake_warehouse resource
+
+Because of the issue [#2948](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2948), we are relaxing the validations for the Snowflake parameter values. Read more in [CHANGES_BEFORE_V1.md](v1-preparations/CHANGES_BEFORE_V1.md#validations).
+
 ## v0.92.0 ➞ v0.93.0
 
 ### general changes

diff --git a/pkg/resources/cortex_search_service_acceptance_test.go b/pkg/resources/cortex_search_service_acceptance_test.go
@@ -14,27 +14,27 @@ import (
 )
 
 func TestAcc_CortexSearchService_basic(t *testing.T) {
-	t.Skipf("Skipped for now because of the <err: 090105 (22000): Cannot perform operation. This session does not have a current database. Call 'USE DATABASE', or use a qualified name.> problem.")
 	resourceName := "snowflake_cortex_search_service.css"
 	id := acc.TestClient().Ids.RandomSchemaObjectIdentifier()
 	tableId := acc.TestClient().Ids.RandomSchemaObjectIdentifier()
 	newWarehouseName := acc.TestClient().Ids.Alpha()
 	m := func() map[string]config.Variable {
 		return map[string]config.Variable{
 			"name":       config.StringVariable(id.Name()),
-			"on":         config.StringVariable("id"),
+			"on":         config.StringVariable("SOME_TEXT"),
 			"database":   config.StringVariable(acc.TestDatabaseName),
 			"schema":     config.StringVariable(acc.TestSchemaName),
 			"warehouse":  config.StringVariable(acc.TestWarehouseName),
-			"query":      config.StringVariable(fmt.Sprintf(`select "id" from %s"`, tableId.FullyQualifiedName())),
+			"query":      config.StringVariable(fmt.Sprintf("select SOME_TEXT from %s", tableId.FullyQualifiedName())),
 			"comment":    config.StringVariable("Terraform acceptance test"),
 			"table_name": config.StringVariable(tableId.Name()),
 		}
 	}
 	variableSet2 := m()
-	variableSet2["attributes"] = config.SetVariable(config.StringVariable("type"))
+	variableSet2["attributes"] = config.SetVariable(config.StringVariable("SOME_OTHER_TEXT"))
 	variableSet2["warehouse"] = config.StringVariable(newWarehouseName)
 	variableSet2["comment"] = config.StringVariable("Terraform acceptance test - updated")
+	variableSet2["query"] = config.StringVariable(fmt.Sprintf("select SOME_TEXT, SOME_OTHER_TEXT from %s", tableId.FullyQualifiedName()))
 
 	resource.Test(t, resource.TestCase{
 		ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
@@ -56,16 +56,15 @@ func TestAcc_CortexSearchService_basic(t *testing.T) {
 					resource.TestCheckResourceAttr(resourceName, "name", id.Name()),
 					resource.TestCheckResourceAttr(resourceName, "database", acc.TestDatabaseName),
 					resource.TestCheckResourceAttr(resourceName, "schema", acc.TestSchemaName),
-					resource.TestCheckResourceAttr(resourceName, "on", "id"),
+					resource.TestCheckResourceAttr(resourceName, "on", "SOME_TEXT"),
 					resource.TestCheckNoResourceAttr(resourceName, "attributes"),
 					resource.TestCheckResourceAttr(resourceName, "warehouse", acc.TestWarehouseName),
 					resource.TestCheckResourceAttr(resourceName, "target_lag", "2 minutes"),
 					resource.TestCheckResourceAttr(resourceName, "comment", "Terraform acceptance test"),
-					resource.TestCheckResourceAttr(resourceName, "query", fmt.Sprintf("select \"id\" from %s", tableId.FullyQualifiedName())),
+					resource.TestCheckResourceAttr(resourceName, "query", fmt.Sprintf("select SOME_TEXT from %s", tableId.FullyQualifiedName())),
 					resource.TestCheckResourceAttrSet(resourceName, "created_on"),
 				),
 			},
-
 			{
 				ConfigDirectory: config.TestStepDirectory(),
 				ConfigVariables: variableSet2,
@@ -78,12 +77,13 @@ func TestAcc_CortexSearchService_basic(t *testing.T) {
 					resource.TestCheckResourceAttr(resourceName, "name", id.Name()),
 					resource.TestCheckResourceAttr(resourceName, "database", acc.TestDatabaseName),
 					resource.TestCheckResourceAttr(resourceName, "schema", acc.TestSchemaName),
-					resource.TestCheckResourceAttr(resourceName, "on", "id"),
-					resource.TestCheckResourceAttr(resourceName, "attributes", "type"),
+					resource.TestCheckResourceAttr(resourceName, "on", "SOME_TEXT"),
+					resource.TestCheckResourceAttr(resourceName, "attributes.#", "1"),
+					resource.TestCheckResourceAttr(resourceName, "attributes.0", "SOME_OTHER_TEXT"),
 					resource.TestCheckResourceAttr(resourceName, "warehouse", newWarehouseName),
 					resource.TestCheckResourceAttr(resourceName, "target_lag", "2 minutes"),
 					resource.TestCheckResourceAttr(resourceName, "comment", "Terraform acceptance test - updated"),
-					resource.TestCheckResourceAttr(resourceName, "query", fmt.Sprintf("select \"id\" from %s", tableId.FullyQualifiedName())),
+					resource.TestCheckResourceAttr(resourceName, "query", fmt.Sprintf("select SOME_TEXT, SOME_OTHER_TEXT from %s", tableId.FullyQualifiedName())),
 					resource.TestCheckResourceAttrSet(resourceName, "created_on"),
 				),
 			},
@@ -94,6 +94,8 @@ func TestAcc_CortexSearchService_basic(t *testing.T) {
 				ResourceName:      resourceName,
 				ImportState:       true,
 				ImportStateVerify: true,
+				// currently not set in read because the early implementation on Snowflake side did not return these values on SHOW/DESCRIBE
+				ImportStateVerifyIgnore: []string{"attributes", "on", "query", "target_lag", "warehouse"},
 			},
 		},
 	})

diff --git a/pkg/resources/testdata/TestAcc_CortexSearchService_basic/1/test.tf b/pkg/resources/testdata/TestAcc_CortexSearchService_basic/1/test.tf
@@ -3,10 +3,16 @@ resource "snowflake_table" "t" {
   schema          = var.schema
   name            = var.table_name
   change_tracking = true
+
   column {
-    name = "id"
+    name = "ID"
     type = "NUMBER(38,0)"
   }
+
+  column {
+    name = "SOME_TEXT"
+    type = "VARCHAR"
+  }
 }
 
 resource "snowflake_cortex_search_service" "css" {

diff --git a/pkg/resources/testdata/TestAcc_CortexSearchService_basic/2/test.tf b/pkg/resources/testdata/TestAcc_CortexSearchService_basic/2/test.tf
@@ -8,12 +8,19 @@ resource "snowflake_table" "t" {
   schema          = var.schema
   name            = var.table_name
   change_tracking = true
+
   column {
-    name = "id"
+    name = "ID"
     type = "NUMBER(38,0)"
   }
+
+  column {
+    name = "SOME_TEXT"
+    type = "VARCHAR"
+  }
+
   column {
-    name = "type"
+    name = "SOME_OTHER_TEXT"
     type = "VARCHAR(32)"
   }
 }

diff --git a/pkg/resources/testdata/TestAcc_CortexSearchService_basic/3/test.tf b/pkg/resources/testdata/TestAcc_CortexSearchService_basic/3/test.tf
@@ -1,20 +1,35 @@
-
+resource "snowflake_warehouse" "t" {
+  name           = var.warehouse
+  warehouse_size = "XSMALL"
+}
 
 resource "snowflake_table" "t" {
   database        = var.database
   schema          = var.schema
   name            = var.table_name
   change_tracking = true
+
   column {
-    name = "id"
+    name = "ID"
     type = "NUMBER(38,0)"
   }
+
+  column {
+    name = "SOME_TEXT"
+    type = "VARCHAR"
+  }
+
+  column {
+    name = "SOME_OTHER_TEXT"
+    type = "VARCHAR(32)"
+  }
 }
 
 resource "snowflake_cortex_search_service" "css" {
-  depends_on = [snowflake_table.t]
-  name       = var.name
+  depends_on = [snowflake_table.t, snowflake_warehouse.t]
   on         = var.on
+  attributes = var.attributes
+  name       = var.name
   database   = var.database
   schema     = var.schema
   target_lag = "2 minutes"

diff --git a/pkg/resources/testdata/TestAcc_CortexSearchService_basic/3/variables.tf b/pkg/resources/testdata/TestAcc_CortexSearchService_basic/3/variables.tf
@@ -8,6 +8,10 @@ variable "on" {
   type = string
 }
 
+variable "attributes" {
+  type = set(string)
+}
+
 variable "database" {
   type = string
 }

diff --git a/pkg/resources/warehouse.go b/pkg/resources/warehouse.go
@@ -42,14 +42,14 @@ var warehouseSchema = map[string]*schema.Schema{
 	"max_cluster_count": {
 		Type:             schema.TypeInt,
 		Optional:         true,
-		ValidateDiagFunc: validation.ToDiagFunc(validation.IntBetween(1, 10)),
+		ValidateDiagFunc: validation.ToDiagFunc(validation.IntAtLeast(1)),
 		DiffSuppressFunc: IgnoreChangeToCurrentSnowflakeValueInShow("max_cluster_count"),
 		Description:      "Specifies the maximum number of server clusters for the warehouse.",
 	},
 	"min_cluster_count": {
 		Type:             schema.TypeInt,
 		Optional:         true,
-		ValidateDiagFunc: validation.ToDiagFunc(validation.IntBetween(1, 10)),
+		ValidateDiagFunc: validation.ToDiagFunc(validation.IntAtLeast(1)),
 		DiffSuppressFunc: IgnoreChangeToCurrentSnowflakeValueInShow("min_cluster_count"),
 		Description:      "Specifies the minimum number of server clusters for the warehouse (only applies to multi-cluster warehouses).",
 	},

diff --git a/pkg/resources/warehouse_acceptance_test.go b/pkg/resources/warehouse_acceptance_test.go
@@ -665,12 +665,12 @@ func TestAcc_Warehouse_Validations(t *testing.T) {
 				ExpectError: regexp.MustCompile("invalid warehouse size: SMALLa"),
 			},
 			{
-				Config:      warehouseConfigWithMaxClusterCount(id.Name(), 100),
-				ExpectError: regexp.MustCompile(`expected max_cluster_count to be in the range \(1 - 10\), got 100`),
+				Config:      warehouseConfigWithMaxClusterCount(id.Name(), 0),
+				ExpectError: regexp.MustCompile(`expected max_cluster_count to be at least \(1\), got 0`),
 			},
 			{
-				Config:      warehouseConfigWithMinClusterCount(id.Name(), 100),
-				ExpectError: regexp.MustCompile(`expected min_cluster_count to be in the range \(1 - 10\), got 100`),
+				Config:      warehouseConfigWithMinClusterCount(id.Name(), 0),
+				ExpectError: regexp.MustCompile(`expected min_cluster_count to be at least \(1\), got 0`),
 			},
 			{
 				Config:      warehouseConfigWithScalingPolicy(id.Name(), "unknown"),

diff --git a/pkg/sdk/testint/application_packages_integration_test.go b/pkg/sdk/testint/application_packages_integration_test.go
@@ -213,8 +213,8 @@ func TestInt_ApplicationPackagesVersionAndReleaseDirective(t *testing.T) {
 		e := createApplicationPackageHandle(t)
 		stage, stageCleanup := testClientHelper().Stage.CreateStage(t)
 		t.Cleanup(stageCleanup)
-		testClientHelper().Stage.PutOnStageWithContent(t, stage.ID(), "manifest.yml", "")
-		testClientHelper().Stage.PutOnStageWithContent(t, stage.ID(), "setup.sql", "")
+		testClientHelper().Stage.PutOnStage(t, stage.ID(), "manifest.yml")
+		testClientHelper().Stage.PutOnStage(t, stage.ID(), "setup.sql")
 
 		version := "V001"
 		using := "@" + stage.ID().FullyQualifiedName()
@@ -253,8 +253,8 @@ func TestInt_ApplicationPackagesVersionAndReleaseDirective(t *testing.T) {
 		e := createApplicationPackageHandle(t)
 		stage, stageCleanup := testClientHelper().Stage.CreateStage(t)
 		t.Cleanup(stageCleanup)
-		testClientHelper().Stage.PutOnStageWithContent(t, stage.ID(), "manifest.yml", "")
-		testClientHelper().Stage.PutOnStageWithContent(t, stage.ID(), "setup.sql", "")
+		testClientHelper().Stage.PutOnStage(t, stage.ID(), "manifest.yml")
+		testClientHelper().Stage.PutOnStage(t, stage.ID(), "setup.sql")
 
 		version := "V001"
 		using := "@" + stage.ID().FullyQualifiedName()

diff --git a/pkg/sdk/testint/grants_integration_test.go b/pkg/sdk/testint/grants_integration_test.go
@@ -241,6 +241,10 @@ func TestInt_GrantAndRevokePrivilegesToAccountRole(t *testing.T) {
 	t.Run("on all: cortex search service", func(t *testing.T) {
 		roleTest, roleCleanup := testClientHelper().Role.CreateRole(t)
 		t.Cleanup(roleCleanup)
+		table, tableTestCleanup := testClientHelper().Table.CreateTableWithPredefinedColumns(t)
+		t.Cleanup(tableTestCleanup)
+		cortex, cortexCleanup := testClientHelper().CortexSearchService.CreateCortexSearchService(t, table.ID())
+		t.Cleanup(cortexCleanup)
 
 		privileges := &sdk.AccountRoleGrantPrivileges{
 			SchemaObjectPrivileges: []sdk.SchemaObjectPrivilege{sdk.SchemaObjectPrivilegeUsage},
@@ -254,7 +258,31 @@ func TestInt_GrantAndRevokePrivilegesToAccountRole(t *testing.T) {
 			},
 		}
 		err := client.Grants.GrantPrivilegesToAccountRole(ctx, privileges, on, roleTest.ID(), nil)
-		require.ErrorContains(t, err, "unexpected 'SEARCH'")
+		require.NoError(t, err)
+
+		grants, err := client.Grants.Show(ctx, &sdk.ShowGrantOptions{
+			To: &sdk.ShowGrantsTo{
+				Role: roleTest.ID(),
+			},
+		})
+		require.NoError(t, err)
+		usagePrivilege, err := collections.FindOne[sdk.Grant](grants, func(g sdk.Grant) bool {
+			return g.Privilege == sdk.SchemaObjectPrivilegeUsage.String()
+		})
+		require.NoError(t, err)
+		assert.Equal(t, cortex.ID().FullyQualifiedName(), usagePrivilege.Name.FullyQualifiedName())
+		assert.Equal(t, sdk.ObjectTypeCortexSearchService, usagePrivilege.GrantedOn)
+
+		// now revoke and verify that the grant(s) are gone
+		err = client.Grants.RevokePrivilegesFromAccountRole(ctx, privileges, on, roleTest.ID(), nil)
+		require.NoError(t, err)
+		grants, err = client.Grants.Show(ctx, &sdk.ShowGrantOptions{
+			To: &sdk.ShowGrantsTo{
+				Role: roleTest.ID(),
+			},
+		})
+		require.NoError(t, err)
+		assert.Equal(t, 0, len(grants))
 	})
 
 	t.Run("on future schema object", func(t *testing.T) {
@@ -1117,6 +1145,10 @@ func TestInt_GrantOwnership(t *testing.T) {
 		return ownershipGrantOnObject(sdk.ObjectTypePipe, pipe.ID())
 	}
 
+	ownershipGrantOnCortexSearchService := func(cortexSearchService *sdk.CortexSearchService) sdk.OwnershipGrantOn {
+		return ownershipGrantOnObject(sdk.ObjectTypeCortexSearchService, cortexSearchService.ID())
+	}
+
 	ownershipGrantOnTask := func(task *sdk.Task) sdk.OwnershipGrantOn {
 		return ownershipGrantOnObject(sdk.ObjectTypeTask, task.ID())
 	}
@@ -1281,7 +1313,13 @@ func TestInt_GrantOwnership(t *testing.T) {
 			},
 			new(sdk.GrantOwnershipOptions),
 		)
-		require.ErrorContains(t, err, "Invalid object type 'CORTEX_SEARCH_SERVICE' for privilege 'OWNERSHIP'")
+		require.NoError(t, err)
+		checkOwnershipOnObjectToRole(t, ownershipGrantOnCortexSearchService(cortex), role.ID())
+
+		currentRole := testClientHelper().Context.CurrentRole(t)
+
+		grantOwnershipToRole(t, currentRole, ownershipGrantOnCortexSearchService(cortex), nil)
+		checkOwnershipOnObjectToRole(t, ownershipGrantOnCortexSearchService(cortex), currentRole)
 	})
 
 	t.Run("on pipe - with operate and monitor privileges granted", func(t *testing.T) {

diff --git a/pkg/sdk/testint/users_integration_test.go b/pkg/sdk/testint/users_integration_test.go
@@ -85,7 +85,7 @@ func TestInt_Users(t *testing.T) {
 			HasQueryTag("some_tag").
 			HasQuotedIdentifiersIgnoreCase(true).
 			HasRowsPerResultset(2).
-			HasS3StageVpceDnsName("vpce-some_dns-vpce.amazonaws.com").
+			HasS3StageVpceDnsName("vpce-id.s3.region.vpce.amazonaws.com").
 			HasSearchPath("$public, $current").
 			HasSimulatedDataSharingConsumer("some_consumer").
 			HasStatementQueuedTimeoutInSeconds(10).
@@ -485,7 +485,7 @@ func TestInt_Users(t *testing.T) {
 				QueryTag:                                 sdk.String("some_tag"),
 				QuotedIdentifiersIgnoreCase:              sdk.Bool(true),
 				RowsPerResultset:                         sdk.Int(2),
-				S3StageVpceDnsName:                       sdk.String("vpce-some_dns-vpce.amazonaws.com"),
+				S3StageVpceDnsName:                       sdk.String("vpce-id.s3.region.vpce.amazonaws.com"),
 				SearchPath:                               sdk.String("$public, $current"),
 				SimulatedDataSharingConsumer:             sdk.String("some_consumer"),
 				StatementQueuedTimeoutInSeconds:          sdk.Int(10),
@@ -700,7 +700,7 @@ func TestInt_Users(t *testing.T) {
 					QueryTag:                                 sdk.String("some_tag"),
 					QuotedIdentifiersIgnoreCase:              sdk.Bool(true),
 					RowsPerResultset:                         sdk.Int(2),
-					S3StageVpceDnsName:                       sdk.String("vpce-some_dns-vpce.amazonaws.com"),
+					S3StageVpceDnsName:                       sdk.String("vpce-id.s3.region.vpce.amazonaws.com"),
 					SearchPath:                               sdk.String("$public, $current"),
 					SimulatedDataSharingConsumer:             sdk.String("some_consumer"),
 					StatementQueuedTimeoutInSeconds:          sdk.Int(10),

diff --git a/v1-preparations/CHANGES_BEFORE_V1.md b/v1-preparations/CHANGES_BEFORE_V1.md
@@ -15,6 +15,12 @@ create a resource with slightly different configuration in Snowflake (depending
 current account configuration, and most-likely other factors). That is why we recommend setting optional fields where
 you want to ensure that the specified value has been set on the Snowflake side.
 
+## Validations
+
+This point connects with the one on about the [default values](#default-values). First of all, we want to reduce the coupling between Snowflake and the provider. Secondly, some of the value limits are soft (consult issues [#2948](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2948) and [#1919](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/1919)) which makes it difficult to align provider validations with the custom setups. Lastly, some values depend on the Snowflake edition used.
+
+Because of all that, we plan to reduce the number of validations (mostly numeric) on the provider side. We won't get rid of them entirely, so that successful plans but apply failures can be limited, but please be aware that you may encounter them. 
+
 ## "Empty" values
 The [Terraform SDK v2](https://github.com/hashicorp/terraform-plugin-sdk) that is currently used in our provider detects the presence of the attribute based on its non-zero Golang value. This means, that it is not possible to distinguish the removal of the value inside a config from setting it explicitely to a zero value, e.g. `0` for the numeric value (check [this thread](https://discuss.hashicorp.com/t/is-it-possible-to-differentiate-between-a-zero-value-and-a-removed-property-in-the-terraform-provider-sdk/43131)). Before we migrate to the new recommended [Terraform Plugin Framework](https://github.com/hashicorp/terraform-plugin-framework) we want to handle such cases the same way inside the provider. It means that:
 - boolean attributes will be migrated to the string attributes with two values: `"true"` and `"false"` settable in the config and the special third value `"default"` that will mean, that the given attribute is not set inside the config.