fix: loosen identifier field validation for account object identifiers (

#2564) Fixes: #2548 Fails because identifier validation doesn't work well for account object identifiers (that may contain dots). ## Test Plan * [x] Acceptance test recreating role name with dots + unit tests for validation function
Snowflake-Labs · Feb 28, 2024 · a5ed8cd · a5ed8cd
1 parent 7aba384
commit a5ed8cd
Show file tree

Hide file tree

Showing 6 changed files with 138 additions and 46 deletions.
diff --git a/pkg/resources/grant_privileges_to_account_role_acceptance_test.go b/pkg/resources/grant_privileges_to_account_role_acceptance_test.go
@@ -929,6 +929,41 @@ func TestAcc_GrantPrivilegesToAccountRole_ImportedPrivileges(t *testing.T) {
 	})
 }
 
+func TestAcc_GrantPrivilegesToAccountRole_MultiplePartsInRoleName(t *testing.T) {
+	nameBytes := []byte(strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)))
+	nameBytes[3] = '.'
+	nameBytes[6] = '.'
+	name := string(nameBytes)
+	configVariables := config.Variables{
+		"name": config.StringVariable(name),
+		"privileges": config.ListVariable(
+			config.StringVariable(string(sdk.GlobalPrivilegeCreateDatabase)),
+			config.StringVariable(string(sdk.GlobalPrivilegeCreateRole)),
+		),
+		"with_grant_option": config.BoolVariable(true),
+	}
+	resourceName := "snowflake_grant_privileges_to_account_role.test"
+
+	resource.Test(t, resource.TestCase{
+		ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
+		PreCheck:                 func() { acc.TestAccPreCheck(t) },
+		TerraformVersionChecks: []tfversion.TerraformVersionCheck{
+			tfversion.RequireAbove(tfversion.Version1_5_0),
+		},
+		CheckDestroy: testAccCheckAccountRolePrivilegesRevoked(name),
+		Steps: []resource.TestStep{
+			{
+				PreConfig:       func() { createAccountRoleOutsideTerraform(t, name) },
+				ConfigDirectory: acc.ConfigurationDirectory("TestAcc_GrantPrivilegesToAccountRole/OnAccount"),
+				ConfigVariables: configVariables,
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr(resourceName, "account_role_name", name),
+				),
+			},
+		},
+	})
+}
+
 func getSecondaryAccountName(t *testing.T) (string, error) {
 	t.Helper()
 	config, err := sdk.ProfileConfig(testprofiles.Secondary)

diff --git a/pkg/resources/grant_privileges_to_role_acceptance_test.go b/pkg/resources/grant_privileges_to_role_acceptance_test.go
@@ -1090,3 +1090,38 @@ func TestAcc_GrantPrivilegesToRole_ImportedPrivileges(t *testing.T) {
 		},
 	})
 }
+
+func TestAcc_GrantPrivilegesToRole_MultiplePartsInRoleName(t *testing.T) {
+	nameBytes := []byte(strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)))
+	nameBytes[3] = '.'
+	nameBytes[6] = '.'
+	name := string(nameBytes)
+	configVariables := config.Variables{
+		"name": config.StringVariable(name),
+		"privileges": config.ListVariable(
+			config.StringVariable(string(sdk.GlobalPrivilegeCreateDatabase)),
+			config.StringVariable(string(sdk.GlobalPrivilegeCreateRole)),
+		),
+		"with_grant_option": config.BoolVariable(true),
+	}
+	resourceName := "snowflake_grant_privileges_to_role.test"
+
+	resource.Test(t, resource.TestCase{
+		ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
+		PreCheck:                 func() { acc.TestAccPreCheck(t) },
+		TerraformVersionChecks: []tfversion.TerraformVersionCheck{
+			tfversion.RequireAbove(tfversion.Version1_5_0),
+		},
+		CheckDestroy: testAccCheckAccountRolePrivilegesRevoked(name),
+		Steps: []resource.TestStep{
+			{
+				PreConfig:       func() { createAccountRoleOutsideTerraform(t, name) },
+				ConfigDirectory: acc.ConfigurationDirectory("TestAcc_GrantPrivilegesToRole/OnAccount"),
+				ConfigVariables: configVariables,
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr(resourceName, "role_name", name),
+				),
+			},
+		},
+	})
+}
diff --git a/pkg/resources/testdata/TestAcc_GrantPrivilegesToRole/OnAccount/test.tf b/pkg/resources/testdata/TestAcc_GrantPrivilegesToRole/OnAccount/test.tf
@@ -0,0 +1,6 @@
+resource "snowflake_grant_privileges_to_role" "test" {
+  role_name = var.name
+  privileges        = var.privileges
+  on_account        = true
+  with_grant_option = var.with_grant_option
+}
diff --git a/pkg/resources/testdata/TestAcc_GrantPrivilegesToRole/OnAccount/variables.tf b/pkg/resources/testdata/TestAcc_GrantPrivilegesToRole/OnAccount/variables.tf
@@ -0,0 +1,11 @@
+variable "name" {
+  type = string
+}
+
+variable "privileges" {
+  type = list(string)
+}
+
+variable "with_grant_option" {
+  type = bool
+}
diff --git a/pkg/resources/validators.go b/pkg/resources/validators.go
@@ -40,51 +40,58 @@ func IsDataType() schema.SchemaValidateFunc { //nolint:staticcheck
 // To use this function, pass it as a validation function on identifier field with generic parameter set to the desired sdk.ObjectIdentifier implementation.
 func IsValidIdentifier[T sdk.AccountObjectIdentifier | sdk.DatabaseObjectIdentifier | sdk.SchemaObjectIdentifier | sdk.TableColumnIdentifier]() schema.SchemaValidateDiagFunc {
 	return func(value any, path cty.Path) diag.Diagnostics {
-		var diags diag.Diagnostics
-
 		if _, ok := value.(string); !ok {
-			diags = append(diags, diag.Diagnostic{
-				Severity:      diag.Error,
-				Summary:       "Invalid schema identifier type",
-				Detail:        fmt.Sprintf("Expected schema string type, but got: %T. This is a provider error please file a report: https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/new/choose", value),
-				AttributePath: path,
-			})
-			return diags
+			return diag.Diagnostics{
+				diag.Diagnostic{
+					Severity:      diag.Error,
+					Summary:       "Invalid schema identifier type",
+					Detail:        fmt.Sprintf("Expected schema string type, but got: %T. This is a provider error please file a report: https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/new/choose", value),
+					AttributePath: path,
+				},
+			}
+		}
+
+		// TODO(SNOW-1163071): Right now we have to skip validation for AccountObjectIdentifier to handle a case where identifier contains dots
+		if _, ok := any(sdk.AccountObjectIdentifier{}).(T); ok {
+			return nil
 		}
 
 		stringValue := value.(string)
 		id, err := helpers.DecodeSnowflakeParameterID(stringValue)
 		if err != nil {
-			diags = append(diags, diag.Diagnostic{
-				Severity: diag.Error,
-				Summary:  "Unable to parse the identifier",
-				Detail: fmt.Sprintf(
-					"Unable to parse the identifier: %s. Make sure you are using the correct form of the fully qualified name for this field: %s.\nOriginal Error: %s",
-					stringValue,
-					getExpectedIdentifierRepresentationFromGeneric[T](),
-					err.Error(),
-				),
-				AttributePath: path,
-			})
-			return diags
+			return diag.Diagnostics{
+				diag.Diagnostic{
+					Severity: diag.Error,
+					Summary:  "Unable to parse the identifier",
+					Detail: fmt.Sprintf(
+						"Unable to parse the identifier: %s. Make sure you are using the correct form of the fully qualified name for this field: %s.\nOriginal Error: %s",
+						stringValue,
+						getExpectedIdentifierRepresentationFromGeneric[T](),
+						err.Error(),
+					),
+					AttributePath: path,
+				},
+			}
 		}
 
 		if _, ok := id.(T); !ok {
-			diags = append(diags, diag.Diagnostic{
-				Severity: diag.Error,
-				Summary:  "Invalid identifier type",
-				Detail: fmt.Sprintf(
-					"Expected %s identifier type, but got: %T. The correct form of the fully qualified name for this field is: %s, but was %s",
-					reflect.TypeOf(new(T)).Elem().Name(),
-					id,
-					getExpectedIdentifierRepresentationFromGeneric[T](),
-					getExpectedIdentifierRepresentationFromParam(id),
-				),
-				AttributePath: path,
-			})
+			return diag.Diagnostics{
+				diag.Diagnostic{
+					Severity: diag.Error,
+					Summary:  "Invalid identifier type",
+					Detail: fmt.Sprintf(
+						"Expected %s identifier type, but got: %T. The correct form of the fully qualified name for this field is: %s, but was %s",
+						reflect.TypeOf(new(T)).Elem().Name(),
+						id,
+						getExpectedIdentifierRepresentationFromGeneric[T](),
+						getExpectedIdentifierRepresentationFromParam(id),
+					),
+					AttributePath: path,
+				},
+			}
 		}
 
-		return diags
+		return nil
 	}
 }
 

diff --git a/pkg/resources/validators_test.go b/pkg/resources/validators_test.go
@@ -69,18 +69,6 @@ func TestIsValidIdentifier(t *testing.T) {
 			Error:      "Expected schema string type, but got: int",
 			CheckingFn: accountObjectIdentifierCheck,
 		},
-		{
-			Name:       "validation: invalid identifier representation",
-			Value:      "",
-			Error:      "Unable to parse the identifier: ",
-			CheckingFn: accountObjectIdentifierCheck,
-		},
-		{
-			Name:       "validation: incorrect form for account object identifier",
-			Value:      "a.b",
-			Error:      "<name>, but was <database_name>.<name>",
-			CheckingFn: accountObjectIdentifierCheck,
-		},
 		{
 			Name:       "validation: incorrect form for database object identifier",
 			Value:      "a.b.c",
@@ -104,6 +92,16 @@ func TestIsValidIdentifier(t *testing.T) {
 			Value:      "a",
 			CheckingFn: accountObjectIdentifierCheck,
 		},
+		{
+			Name:       "correct form for account object identifier - multiple parts",
+			Value:      "a.b",
+			CheckingFn: accountObjectIdentifierCheck,
+		},
+		{
+			Name:       "correct form for account object identifier - quoted",
+			Value:      "\"a.b\"",
+			CheckingFn: accountObjectIdentifierCheck,
+		},
 		{
 			Name:       "correct form for database object identifier",
 			Value:      "a.b",