From dd01ce9f5cc4aedef9091a2238bc9f8c09905b84 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Fri, 26 Jul 2024 11:53:04 +0200 Subject: [PATCH] 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 --- MIGRATION_GUIDE.md | 4 ++ .../cortex_search_service_acceptance_test.go | 22 +++++----- .../1/test.tf | 8 +++- .../2/test.tf | 11 ++++- .../3/test.tf | 23 ++++++++-- .../3/variables.tf | 4 ++ pkg/resources/warehouse.go | 4 +- pkg/resources/warehouse_acceptance_test.go | 8 ++-- .../application_packages_integration_test.go | 8 ++-- pkg/sdk/testint/grants_integration_test.go | 42 ++++++++++++++++++- pkg/sdk/testint/users_integration_test.go | 6 +-- v1-preparations/CHANGES_BEFORE_V1.md | 6 +++ 12 files changed, 114 insertions(+), 32 deletions(-) diff --git a/MIGRATION_GUIDE.md b/MIGRATION_GUIDE.md index 4bfd708aaa..8ed26012f5 100644 --- 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 index 8fba481276..d4b831cbea 100644 --- a/pkg/resources/cortex_search_service_acceptance_test.go +++ b/pkg/resources/cortex_search_service_acceptance_test.go @@ -14,7 +14,6 @@ import ( ) func TestAcc_CortexSearchService_basic(t *testing.T) { - t.Skipf("Skipped for now because of the problem.") resourceName := "snowflake_cortex_search_service.css" id := acc.TestClient().Ids.RandomSchemaObjectIdentifier() tableId := acc.TestClient().Ids.RandomSchemaObjectIdentifier() @@ -22,19 +21,20 @@ func TestAcc_CortexSearchService_basic(t *testing.T) { 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 index 7455508400..870c24ad59 100644 --- 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 index ee41122c7c..e22209c7d0 100644 --- 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 index 354fe90d4f..e22209c7d0 100644 --- 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 index 100f499271..2287d89e7a 100644 --- 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 index 3641df2272..9ce8ec6a2e 100644 --- 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 index 1a3e58d601..1c337f0f54 100644 --- 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 index 94f3ff0773..ec4a005304 100644 --- 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 index cff2094f81..112ad3b7ec 100644 --- 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 index 0c99018e25..4dc97584ed 100644 --- 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 index ece0569893..d9cf10240f 100644 --- 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.