From d7780aed31dcdfbe4dc252b06fb74eb5d9483c57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Cie=C5=9Blak?= Date: Fri, 23 Aug 2024 08:24:17 +0200 Subject: [PATCH] chore: apply identifier conventions to grants (#3008) ## Changes - Applied identifier conventions to grant resources - Used `helpers.EncodeResourceIdentifier` where possible - Used `sdk.ParseXIdentifier` where possible - Applied quote ignore on identifier fields - Added tests to check the migration between the versions > Note: Some of the changes like `resource_identifier.go`, `custom_diff.go`, `diff_suppressions.go` were taken from #2996 --- pkg/datasources/grants.go | 84 ++++++++-- pkg/helpers/resource_identifier_test.go | 2 +- pkg/resources/account_role.go | 3 +- .../api_authentication_integration_common.go | 2 +- pkg/resources/common.go | 50 +++++- pkg/resources/database.go | 2 +- pkg/resources/database_old.go | 2 +- pkg/resources/database_role.go | 61 ++++--- .../database_role_acceptance_test.go | 90 ++++++++++ .../grant_account_role_acceptance_test.go | 105 ++++++++++++ pkg/resources/grant_application_role.go | 86 +++++++--- .../grant_application_role_acceptance_test.go | 107 ++++++++++++ pkg/resources/grant_database_role.go | 91 +++++++--- .../grant_database_role_acceptance_test.go | 136 +++++++++++++++ pkg/resources/grant_ownership.go | 71 ++++++-- .../grant_ownership_acceptance_test.go | 157 ++++++++++++++++++ pkg/resources/grant_ownership_identifier.go | 24 ++- .../grant_ownership_identifier_test.go | 59 ++++--- pkg/resources/grant_ownership_test.go | 30 +++- .../grant_privileges_identifier_commons.go | 16 +- .../grant_privileges_to_account_role.go | 68 ++++++-- ...vileges_to_account_role_acceptance_test.go | 124 ++++++++++++-- ...t_privileges_to_account_role_identifier.go | 46 +++-- ...vileges_to_account_role_identifier_test.go | 32 ++-- .../grant_privileges_to_database_role.go | 85 ++++++++-- ...ileges_to_database_role_acceptance_test.go | 111 +++++++++++++ ..._privileges_to_database_role_identifier.go | 52 +++--- ...ileges_to_database_role_identifier_test.go | 2 +- pkg/resources/grant_privileges_to_share.go | 102 +++++++++--- ...ant_privileges_to_share_acceptance_test.go | 104 ++++++++++++ .../grant_privileges_to_share_identifier.go | 11 +- ...ant_privileges_to_share_identifier_test.go | 19 ++- pkg/resources/network_policy.go | 2 +- pkg/resources/secondary_database.go | 2 +- pkg/resources/share.go | 14 +- pkg/resources/shared_database.go | 2 +- pkg/resources/streamlit.go | 3 +- pkg/resources/table.go | 7 +- pkg/sdk/identifier_parsers_test.go | 8 +- 39 files changed, 1688 insertions(+), 284 deletions(-) diff --git a/pkg/datasources/grants.go b/pkg/datasources/grants.go index ad2e465e6c..80ac1568aa 100644 --- a/pkg/datasources/grants.go +++ b/pkg/datasources/grants.go @@ -396,35 +396,59 @@ func buildOptsForGrantsTo(grantsTo map[string]any) (*sdk.ShowGrantOptions, error opts := new(sdk.ShowGrantOptions) if application := grantsTo["application"].(string); application != "" { + applicationId, err := sdk.ParseAccountObjectIdentifier(application) + if err != nil { + return nil, err + } opts.To = &sdk.ShowGrantsTo{ - Application: sdk.NewAccountObjectIdentifier(application), + Application: applicationId, } } if applicationRole := grantsTo["application_role"].(string); applicationRole != "" { + applicationRoleId, err := sdk.ParseDatabaseObjectIdentifier(applicationRole) + if err != nil { + return nil, err + } opts.To = &sdk.ShowGrantsTo{ - ApplicationRole: sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(applicationRole), + ApplicationRole: applicationRoleId, } } if accountRole := grantsTo["account_role"].(string); accountRole != "" { + accountRoleId, err := sdk.ParseAccountObjectIdentifier(accountRole) + if err != nil { + return nil, err + } opts.To = &sdk.ShowGrantsTo{ - Role: sdk.NewAccountObjectIdentifier(accountRole), + Role: accountRoleId, } } if databaseRole := grantsTo["database_role"].(string); databaseRole != "" { + databaseRoleId, err := sdk.ParseDatabaseObjectIdentifier(databaseRole) + if err != nil { + return nil, err + } opts.To = &sdk.ShowGrantsTo{ - DatabaseRole: sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(databaseRole), + DatabaseRole: databaseRoleId, } } if user := grantsTo["user"].(string); user != "" { + userId, err := sdk.ParseAccountObjectIdentifier(user) + if err != nil { + return nil, err + } opts.To = &sdk.ShowGrantsTo{ - User: sdk.NewAccountObjectIdentifier(user), + User: userId, } } if share := grantsTo["share"]; share != nil && len(share.([]any)) > 0 { shareMap := share.([]any)[0].(map[string]any) + sharedId, err := sdk.ParseAccountObjectIdentifier(shareMap["share_name"].(string)) + if err != nil { + return nil, err + } opts.To = &sdk.ShowGrantsTo{ Share: &sdk.ShowGrantsToShare{ - Name: sdk.NewAccountObjectIdentifier(shareMap["share_name"].(string)), + Name: sharedId, }, } // TODO [SNOW-1284382]: Uncomment after SHOW GRANTS TO SHARE IN APPLICATION PACKAGE syntax starts working. @@ -439,23 +463,39 @@ func buildOptsForGrantsOf(grantsOf map[string]any) (*sdk.ShowGrantOptions, error opts := new(sdk.ShowGrantOptions) if accountRole := grantsOf["account_role"].(string); accountRole != "" { + accountRoleId, err := sdk.ParseAccountObjectIdentifier(accountRole) + if err != nil { + return nil, err + } opts.Of = &sdk.ShowGrantsOf{ - Role: sdk.NewAccountObjectIdentifier(accountRole), + Role: accountRoleId, } } if databaseRole := grantsOf["database_role"].(string); databaseRole != "" { + databaseRoleId, err := sdk.ParseDatabaseObjectIdentifier(databaseRole) + if err != nil { + return nil, err + } opts.Of = &sdk.ShowGrantsOf{ - DatabaseRole: sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(databaseRole), + DatabaseRole: databaseRoleId, } } if applicationRole := grantsOf["application_role"].(string); applicationRole != "" { + applicationRoleId, err := sdk.ParseDatabaseObjectIdentifier(applicationRole) + if err != nil { + return nil, err + } opts.Of = &sdk.ShowGrantsOf{ - ApplicationRole: sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(applicationRole), + ApplicationRole: applicationRoleId, } } if share := grantsOf["share"].(string); share != "" { + shareId, err := sdk.ParseAccountObjectIdentifier(share) + if err != nil { + return nil, err + } opts.Of = &sdk.ShowGrantsOf{ - Share: sdk.NewAccountObjectIdentifier(share), + Share: shareId, } } return opts, nil @@ -466,13 +506,21 @@ func buildOptsForFutureGrantsIn(futureGrantsIn map[string]any) (*sdk.ShowGrantOp opts.Future = sdk.Bool(true) if db := futureGrantsIn["database"].(string); db != "" { + databaseId, err := sdk.ParseAccountObjectIdentifier(db) + if err != nil { + return nil, err + } opts.In = &sdk.ShowGrantsIn{ - Database: sdk.Pointer(sdk.NewAccountObjectIdentifier(db)), + Database: sdk.Pointer(databaseId), } } if sc := futureGrantsIn["schema"].(string); sc != "" { + schemaId, err := sdk.ParseDatabaseObjectIdentifier(sc) + if err != nil { + return nil, err + } opts.In = &sdk.ShowGrantsIn{ - Schema: sdk.Pointer(sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(sc)), + Schema: sdk.Pointer(schemaId), } } return opts, nil @@ -483,13 +531,21 @@ func buildOptsForFutureGrantsTo(futureGrantsTo map[string]any) (*sdk.ShowGrantOp opts.Future = sdk.Bool(true) if accountRole := futureGrantsTo["account_role"].(string); accountRole != "" { + accountRoleId, err := sdk.ParseAccountObjectIdentifier(accountRole) + if err != nil { + return nil, err + } opts.To = &sdk.ShowGrantsTo{ - Role: sdk.NewAccountObjectIdentifier(accountRole), + Role: accountRoleId, } } if databaseRole := futureGrantsTo["database_role"].(string); databaseRole != "" { + databaseRoleId, err := sdk.ParseDatabaseObjectIdentifier(databaseRole) + if err != nil { + return nil, err + } opts.To = &sdk.ShowGrantsTo{ - DatabaseRole: sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(databaseRole), + DatabaseRole: databaseRoleId, } } return opts, nil diff --git a/pkg/helpers/resource_identifier_test.go b/pkg/helpers/resource_identifier_test.go index 95cc62a2b5..3fcd1f78eb 100644 --- a/pkg/helpers/resource_identifier_test.go +++ b/pkg/helpers/resource_identifier_test.go @@ -56,7 +56,7 @@ func Test_Encoding_And_Parsing_Of_ResourceIdentifier_Identifier(t *testing.T) { for _, testCase := range testCases { t.Run(fmt.Sprintf(`Encoding and parsing %s resource identifier`, testCase.Input), func(t *testing.T) { - switch typedInput := any(testCase.Input).(type) { + switch typedInput := testCase.Input.(type) { case []sdk.AccountObjectIdentifier: encodedIdentifier := EncodeResourceIdentifier(typedInput...) assert.Equal(t, testCase.Expected, encodedIdentifier) diff --git a/pkg/resources/account_role.go b/pkg/resources/account_role.go index 1f760f31a8..a656c294f1 100644 --- a/pkg/resources/account_role.go +++ b/pkg/resources/account_role.go @@ -55,7 +55,7 @@ func AccountRole() *schema.Resource { ), Importer: &schema.ResourceImporter{ - StateContext: ImportName, + StateContext: ImportName[sdk.AccountObjectIdentifier], }, } } @@ -67,7 +67,6 @@ func CreateAccountRole(ctx context.Context, d *schema.ResourceData, meta any) di if err != nil { return diag.FromErr(err) } - req := sdk.NewCreateRoleRequest(id) if v, ok := d.GetOk("comment"); ok { diff --git a/pkg/resources/api_authentication_integration_common.go b/pkg/resources/api_authentication_integration_common.go index 5fc6f7c323..d21b3c943b 100644 --- a/pkg/resources/api_authentication_integration_common.go +++ b/pkg/resources/api_authentication_integration_common.go @@ -212,7 +212,7 @@ func handleApiAuthCreate(d *schema.ResourceData) (commonApiAuthCreate, error) { func handleApiAuthImport(d *schema.ResourceData, integration *sdk.SecurityIntegration, properties []sdk.SecurityIntegrationProperty, ) error { - if _, err := ImportName(context.Background(), d, nil); err != nil { + if _, err := ImportName[sdk.AccountObjectIdentifier](context.Background(), d, nil); err != nil { return err } diff --git a/pkg/resources/common.go b/pkg/resources/common.go index af537eca40..05579effba 100644 --- a/pkg/resources/common.go +++ b/pkg/resources/common.go @@ -2,9 +2,10 @@ package resources import ( "context" - "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk" "strings" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk" + "github.com/hashicorp/go-cty/cty" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" ) @@ -48,14 +49,47 @@ func ctyValToSliceString(valueElems []cty.Value) []string { return elems } -func ImportName(ctx context.Context, d *schema.ResourceData, meta any) ([]*schema.ResourceData, error) { - id, err := sdk.ParseAccountObjectIdentifier(d.Id()) - if err != nil { - return nil, err - } +func ImportName[T sdk.AccountObjectIdentifier | sdk.DatabaseObjectIdentifier | sdk.SchemaObjectIdentifier](ctx context.Context, d *schema.ResourceData, meta any) ([]*schema.ResourceData, error) { + switch any(new(T)).(type) { + case *sdk.AccountObjectIdentifier: + id, err := sdk.ParseAccountObjectIdentifier(d.Id()) + if err != nil { + return nil, err + } + + if err := d.Set("name", id.Name()); err != nil { + return nil, err + } + case *sdk.DatabaseObjectIdentifier: + id, err := sdk.ParseDatabaseObjectIdentifier(d.Id()) + if err != nil { + return nil, err + } + + if err := d.Set("name", id.Name()); err != nil { + return nil, err + } + + if err := d.Set("database", id.DatabaseName()); err != nil { + return nil, err + } + case *sdk.SchemaObjectIdentifier: + id, err := sdk.ParseSchemaObjectIdentifier(d.Id()) + if err != nil { + return nil, err + } + + if err := d.Set("name", id.Name()); err != nil { + return nil, err + } + + if err := d.Set("database", id.DatabaseName()); err != nil { + return nil, err + } - if err := d.Set("name", id.Name()); err != nil { - return nil, err + if err := d.Set("schema", id.SchemaName()); err != nil { + return nil, err + } } return []*schema.ResourceData{d}, nil diff --git a/pkg/resources/database.go b/pkg/resources/database.go index 80a0c9a508..cc06ee6e60 100644 --- a/pkg/resources/database.go +++ b/pkg/resources/database.go @@ -97,7 +97,7 @@ func Database() *schema.Resource { Schema: helpers.MergeMaps(databaseSchema, databaseParametersSchema), Importer: &schema.ResourceImporter{ - StateContext: ImportName, + StateContext: ImportName[sdk.AccountObjectIdentifier], }, CustomizeDiff: customdiff.All( diff --git a/pkg/resources/database_old.go b/pkg/resources/database_old.go index c7086f5728..ede9949eef 100644 --- a/pkg/resources/database_old.go +++ b/pkg/resources/database_old.go @@ -96,7 +96,7 @@ func DatabaseOld() *schema.Resource { Schema: databaseOldSchema, Importer: &schema.ResourceImporter{ - StateContext: ImportName, + StateContext: ImportName[sdk.AccountObjectIdentifier], }, } } diff --git a/pkg/resources/database_role.go b/pkg/resources/database_role.go index d44d40a469..2745514e8c 100644 --- a/pkg/resources/database_role.go +++ b/pkg/resources/database_role.go @@ -5,26 +5,30 @@ import ( "fmt" "log" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/helpers" + "github.com/hashicorp/go-cty/cty" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/provider" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/schemas" - "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/helpers" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" ) var databaseRoleSchema = map[string]*schema.Schema{ "name": { - Type: schema.TypeString, - Required: true, - Description: "Specifies the identifier for the database role.", - ForceNew: true, + Type: schema.TypeString, + Required: true, + Description: "Specifies the identifier for the database role.", + ForceNew: true, + DiffSuppressFunc: suppressIdentifierQuoting, }, "database": { - Type: schema.TypeString, - Required: true, - Description: "The database in which to create the database role.", - ForceNew: true, + Type: schema.TypeString, + Required: true, + Description: "The database in which to create the database role.", + ForceNew: true, + DiffSuppressFunc: suppressIdentifierQuoting, }, "comment": { Type: schema.TypeString, @@ -44,7 +48,17 @@ func DatabaseRole() *schema.Resource { Schema: databaseRoleSchema, Importer: &schema.ResourceImporter{ - StateContext: schema.ImportStatePassthroughContext, + StateContext: ImportName[sdk.DatabaseObjectIdentifier], + }, + + SchemaVersion: 1, + StateUpgraders: []schema.StateUpgrader{ + { + Version: 0, + // setting type to cty.EmptyObject is a bit hacky here but following https://developer.hashicorp.com/terraform/plugin/framework/migrating/resources/state-upgrade#sdkv2-1 would require lots of repetitive code; this should work with cty.EmptyObject + Type: cty.EmptyObject, + Upgrade: migratePipeSeparatedObjectIdentifierResourceIdToFullyQualifiedName, + }, }, } } @@ -53,7 +67,10 @@ func DatabaseRole() *schema.Resource { func ReadDatabaseRole(d *schema.ResourceData, meta interface{}) error { client := meta.(*provider.Context).Client - id := helpers.DecodeSnowflakeID(d.Id()).(sdk.DatabaseObjectIdentifier) + id, err := sdk.ParseDatabaseObjectIdentifier(d.Id()) + if err != nil { + return err + } ctx := context.Background() databaseRole, err := client.DatabaseRoles.ShowByID(ctx, id) @@ -68,14 +85,6 @@ func ReadDatabaseRole(d *schema.ResourceData, meta interface{}) error { return err } - if err := d.Set("name", databaseRole.Name); err != nil { - return err - } - - if err := d.Set("database", id.DatabaseName()); err != nil { - return err - } - if err := d.Set("comment", databaseRole.Comment); err != nil { return err } @@ -102,7 +111,7 @@ func CreateDatabaseRole(d *schema.ResourceData, meta interface{}) error { return err } - d.SetId(helpers.EncodeSnowflakeID(objectIdentifier)) + d.SetId(helpers.EncodeResourceIdentifier(objectIdentifier)) return ReadDatabaseRole(d, meta) } @@ -111,7 +120,10 @@ func CreateDatabaseRole(d *schema.ResourceData, meta interface{}) error { func UpdateDatabaseRole(d *schema.ResourceData, meta interface{}) error { client := meta.(*provider.Context).Client - objectIdentifier := helpers.DecodeSnowflakeID(d.Id()).(sdk.DatabaseObjectIdentifier) + objectIdentifier, err := sdk.ParseDatabaseObjectIdentifier(d.Id()) + if err != nil { + return err + } if d.HasChange("comment") { _, newVal := d.GetChange("comment") @@ -131,11 +143,14 @@ func UpdateDatabaseRole(d *schema.ResourceData, meta interface{}) error { func DeleteDatabaseRole(d *schema.ResourceData, meta interface{}) error { client := meta.(*provider.Context).Client - objectIdentifier := helpers.DecodeSnowflakeID(d.Id()).(sdk.DatabaseObjectIdentifier) + objectIdentifier, err := sdk.ParseDatabaseObjectIdentifier(d.Id()) + if err != nil { + return err + } ctx := context.Background() dropRequest := sdk.NewDropDatabaseRoleRequest(objectIdentifier) - err := client.DatabaseRoles.Drop(ctx, dropRequest) + err = client.DatabaseRoles.Drop(ctx, dropRequest) if err != nil { return err } diff --git a/pkg/resources/database_role_acceptance_test.go b/pkg/resources/database_role_acceptance_test.go index 92738f7de7..b7d2fda990 100644 --- a/pkg/resources/database_role_acceptance_test.go +++ b/pkg/resources/database_role_acceptance_test.go @@ -4,6 +4,8 @@ import ( "fmt" "testing" + "github.com/hashicorp/terraform-plugin-testing/plancheck" + acc "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/helpers/random" @@ -58,3 +60,91 @@ resource "snowflake_database_role" "test_db_role" { ` return fmt.Sprintf(s, dbRoleName, databaseName, comment) } + +func TestAcc_DatabaseRole_migrateFromV0941_ensureSmoothUpgradeWithNewResourceId(t *testing.T) { + id := acc.TestClient().Ids.RandomDatabaseObjectIdentifier() + comment := random.Comment() + + resource.Test(t, resource.TestCase{ + PreCheck: func() { acc.TestAccPreCheck(t) }, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{ + "snowflake": { + VersionConstraint: "=0.94.1", + Source: "Snowflake-Labs/snowflake", + }, + }, + Config: databaseRoleConfig(id.Name(), id.DatabaseName(), comment), + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_database_role.test_db_role", "id", fmt.Sprintf(`%s|%s`, id.DatabaseName(), id.Name())), + ), + }, + { + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + Config: databaseRoleConfig(id.Name(), id.DatabaseName(), comment), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_database_role.test_db_role", plancheck.ResourceActionNoop), + }, + PostApplyPostRefresh: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_database_role.test_db_role", plancheck.ResourceActionNoop), + }, + }, + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_database_role.test_db_role", "id", id.FullyQualifiedName()), + ), + }, + }, + }) +} + +func TestAcc_DatabaseRole_IdentifierQuotingDiffSuppression(t *testing.T) { + id := acc.TestClient().Ids.RandomDatabaseObjectIdentifier() + quotedDatabaseRoleId := fmt.Sprintf(`\"%s\"`, id.Name()) + comment := random.Comment() + + resource.Test(t, resource.TestCase{ + PreCheck: func() { acc.TestAccPreCheck(t) }, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{ + "snowflake": { + VersionConstraint: "=0.94.1", + Source: "Snowflake-Labs/snowflake", + }, + }, + ExpectNonEmptyPlan: true, + Config: databaseRoleConfig(quotedDatabaseRoleId, id.DatabaseName(), comment), + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_database_role.test_db_role", "database", id.DatabaseName()), + resource.TestCheckResourceAttr("snowflake_database_role.test_db_role", "name", id.Name()), + resource.TestCheckResourceAttr("snowflake_database_role.test_db_role", "id", fmt.Sprintf(`%s|%s`, id.DatabaseName(), id.Name())), + ), + }, + { + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + Config: databaseRoleConfig(quotedDatabaseRoleId, id.DatabaseName(), comment), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_database_role.test_db_role", plancheck.ResourceActionNoop), + }, + PostApplyPostRefresh: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_database_role.test_db_role", plancheck.ResourceActionNoop), + }, + }, + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_database_role.test_db_role", "database", id.DatabaseName()), + resource.TestCheckResourceAttr("snowflake_database_role.test_db_role", "name", id.Name()), + resource.TestCheckResourceAttr("snowflake_database_role.test_db_role", "id", id.FullyQualifiedName()), + ), + }, + }, + }) +} diff --git a/pkg/resources/grant_account_role_acceptance_test.go b/pkg/resources/grant_account_role_acceptance_test.go index 9d76d23841..3a05e90595 100644 --- a/pkg/resources/grant_account_role_acceptance_test.go +++ b/pkg/resources/grant_account_role_acceptance_test.go @@ -2,8 +2,11 @@ package resources_test import ( "fmt" + "regexp" "testing" + "github.com/hashicorp/terraform-plugin-testing/plancheck" + acc "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance" "github.com/hashicorp/terraform-plugin-testing/config" @@ -88,3 +91,105 @@ func TestAcc_GrantAccountRole_user(t *testing.T) { }, }) } + +func TestAcc_GrantAccountRole_migrateFromV0941_ensureSmoothUpgradeWithNewResourceId(t *testing.T) { + roleName := acc.TestClient().Ids.RandomAccountObjectIdentifier() + parentRoleName := acc.TestClient().Ids.RandomAccountObjectIdentifier() + + resource.Test(t, resource.TestCase{ + PreCheck: func() { acc.TestAccPreCheck(t) }, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{ + "snowflake": { + VersionConstraint: "=0.94.1", + Source: "Snowflake-Labs/snowflake", + }, + }, + Config: grantAccountRoleBasicConfig(roleName.Name(), parentRoleName.Name()), + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_grant_account_role.test", "id", fmt.Sprintf(`%v|ROLE|%v`, roleName.FullyQualifiedName(), parentRoleName.FullyQualifiedName())), + ), + }, + { + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + Config: grantAccountRoleBasicConfig(roleName.Name(), parentRoleName.Name()), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_grant_account_role.test", plancheck.ResourceActionNoop), + }, + PostApplyPostRefresh: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_grant_account_role.test", plancheck.ResourceActionNoop), + }, + }, + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_grant_account_role.test", "id", fmt.Sprintf(`%v|ROLE|%v`, roleName.FullyQualifiedName(), parentRoleName.FullyQualifiedName())), + ), + }, + }, + }) +} + +func grantAccountRoleBasicConfig(roleName string, parentRoleName string) string { + return fmt.Sprintf(` +resource "snowflake_account_role" "role" { + name = "%s" +} + +resource "snowflake_account_role" "parent_role" { + name = "%s" +} + +resource "snowflake_grant_account_role" "test" { + role_name = snowflake_account_role.role.name + parent_role_name = snowflake_account_role.parent_role.name +} +`, roleName, parentRoleName) +} + +func TestAcc_GrantAccountRole_IdentifierQuotingDiffSuppression(t *testing.T) { + roleId := acc.TestClient().Ids.RandomAccountObjectIdentifier() + quotedRoleId := fmt.Sprintf(`\"%s\"`, roleId.Name()) + + parentRoleId := acc.TestClient().Ids.RandomAccountObjectIdentifier() + quotedParentRoleId := fmt.Sprintf(`\"%s\"`, parentRoleId.Name()) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { acc.TestAccPreCheck(t) }, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{ + "snowflake": { + VersionConstraint: "=0.94.1", + Source: "Snowflake-Labs/snowflake", + }, + }, + ExpectError: regexp.MustCompile("Error: Provider produced inconsistent final plan"), + Config: grantAccountRoleBasicConfig(quotedRoleId, quotedParentRoleId), + }, + { + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + Config: grantAccountRoleBasicConfig(quotedRoleId, quotedParentRoleId), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_grant_account_role.test", plancheck.ResourceActionCreate), + }, + PostApplyPostRefresh: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_grant_account_role.test", plancheck.ResourceActionNoop), + }, + }, + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_grant_account_role.test", "role_name", roleId.Name()), + resource.TestCheckResourceAttr("snowflake_grant_account_role.test", "parent_role_name", parentRoleId.Name()), + resource.TestCheckResourceAttr("snowflake_grant_account_role.test", "id", fmt.Sprintf(`%v|ROLE|%v`, roleId.FullyQualifiedName(), parentRoleId.FullyQualifiedName())), + ), + }, + }, + }) +} diff --git a/pkg/resources/grant_application_role.go b/pkg/resources/grant_application_role.go index 98f0668de1..9d0c27ae38 100644 --- a/pkg/resources/grant_application_role.go +++ b/pkg/resources/grant_application_role.go @@ -21,6 +21,7 @@ var grantApplicationRoleSchema = map[string]*schema.Schema{ Description: "Specifies the identifier for the application role to grant.", ForceNew: true, ValidateDiagFunc: IsValidIdentifier[sdk.DatabaseObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, }, "parent_account_role_name": { Type: schema.TypeString, @@ -28,6 +29,7 @@ var grantApplicationRoleSchema = map[string]*schema.Schema{ Description: "The fully qualified name of the account role on which application role will be granted.", ForceNew: true, ValidateDiagFunc: IsValidIdentifier[sdk.AccountObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, ExactlyOneOf: []string{ "parent_account_role_name", "application_name", @@ -39,6 +41,7 @@ var grantApplicationRoleSchema = map[string]*schema.Schema{ Description: "The fully qualified name of the application on which application role will be granted.", ForceNew: true, ValidateDiagFunc: IsValidIdentifier[sdk.AccountObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, ExactlyOneOf: []string{ "parent_account_role_name", "application_name", @@ -54,7 +57,7 @@ func GrantApplicationRole() *schema.Resource { Schema: grantApplicationRoleSchema, Importer: &schema.ResourceImporter{ StateContext: func(ctx context.Context, d *schema.ResourceData, m interface{}) ([]*schema.ResourceData, error) { - parts := strings.Split(d.Id(), helpers.IDDelimiter) + parts := helpers.ParseResourceIdentifier(d.Id()) if len(parts) != 3 { return nil, fmt.Errorf("invalid ID specified: %v, expected ||", d.Id()) } @@ -63,11 +66,19 @@ func GrantApplicationRole() *schema.Resource { } switch parts[1] { case "ACCOUNT_ROLE": - if err := d.Set("parent_account_role_name", sdk.NewAccountObjectIdentifier(parts[2]).FullyQualifiedName()); err != nil { + accountRoleId, err := sdk.ParseAccountObjectIdentifier(parts[2]) + if err != nil { + return nil, err + } + if err := d.Set("parent_account_role_name", accountRoleId.FullyQualifiedName()); err != nil { return nil, err } case "APPLICATION": - if err := d.Set("application_name", sdk.NewAccountObjectIdentifier(parts[2]).FullyQualifiedName()); err != nil { + applicationId, err := sdk.ParseAccountObjectIdentifier(parts[2]) + if err != nil { + return nil, err + } + if err := d.Set("application_name", applicationId.FullyQualifiedName()); err != nil { return nil, err } default: @@ -83,19 +94,28 @@ func GrantApplicationRole() *schema.Resource { func CreateContextGrantApplicationRole(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { client := meta.(*provider.Context).Client name := d.Get("application_role_name").(string) - applicationRoleIdentifier := sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(name) + applicationRoleIdentifier, err := sdk.ParseDatabaseObjectIdentifier(name) + if err != nil { + return diag.FromErr(err) + } // format of snowflakeResourceID is || var snowflakeResourceID string if parentRoleName, ok := d.GetOk("parent_account_role_name"); ok && parentRoleName.(string) != "" { - parentRoleIdentifier := sdk.NewAccountObjectIdentifierFromFullyQualifiedName(parentRoleName.(string)) - snowflakeResourceID = strings.Join([]string{applicationRoleIdentifier.FullyQualifiedName(), "ACCOUNT_ROLE", parentRoleIdentifier.FullyQualifiedName()}, helpers.IDDelimiter) + parentRoleIdentifier, err := sdk.ParseAccountObjectIdentifier(parentRoleName.(string)) + if err != nil { + return diag.FromErr(err) + } + snowflakeResourceID = helpers.EncodeResourceIdentifier(applicationRoleIdentifier.FullyQualifiedName(), "ACCOUNT_ROLE", parentRoleIdentifier.FullyQualifiedName()) req := sdk.NewGrantApplicationRoleRequest(applicationRoleIdentifier).WithTo(*sdk.NewKindOfRoleRequest().WithRoleName(&parentRoleIdentifier)) if err := client.ApplicationRoles.Grant(ctx, req); err != nil { return diag.FromErr(err) } } else if applicationName, ok := d.GetOk("application_name"); ok && applicationName.(string) != "" { - applicationIdentifier := sdk.NewAccountObjectIdentifierFromFullyQualifiedName(applicationName.(string)) - snowflakeResourceID = strings.Join([]string{applicationRoleIdentifier.FullyQualifiedName(), sdk.ObjectTypeApplication.String(), applicationIdentifier.FullyQualifiedName()}, helpers.IDDelimiter) + applicationIdentifier, err := sdk.ParseAccountObjectIdentifier(applicationName.(string)) + if err != nil { + return diag.FromErr(err) + } + snowflakeResourceID = helpers.EncodeResourceIdentifier(applicationRoleIdentifier.FullyQualifiedName(), sdk.ObjectTypeApplication.String(), applicationIdentifier.FullyQualifiedName()) req := sdk.NewGrantApplicationRoleRequest(applicationRoleIdentifier).WithTo(*sdk.NewKindOfRoleRequest().WithApplicationName(&applicationIdentifier)) if err := client.ApplicationRoles.Grant(ctx, req); err != nil { return diag.FromErr(err) @@ -109,7 +129,10 @@ func ReadContextGrantApplicationRole(ctx context.Context, d *schema.ResourceData client := meta.(*provider.Context).Client parts := strings.Split(d.Id(), helpers.IDDelimiter) applicationRoleName := parts[0] - applicationRoleIdentifier := sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(applicationRoleName) + applicationRoleIdentifier, err := sdk.ParseDatabaseObjectIdentifier(applicationRoleName) + if err != nil { + return diag.FromErr(err) + } objectTypeString := parts[1] if objectTypeString == "ACCOUNT_ROLE" { objectTypeString = "ROLE" @@ -120,20 +143,26 @@ func ReadContextGrantApplicationRole(ctx context.Context, d *schema.ResourceData objectType := sdk.ObjectType(objectTypeString) switch objectType { case sdk.ObjectTypeRole: - { - if _, err := client.Roles.ShowByID(ctx, sdk.NewAccountObjectIdentifierFromFullyQualifiedName(targetIdentifier)); err != nil && errors.Is(err, sdk.ErrObjectNotFound) { - d.SetId("") - return diag.Diagnostics{ - diag.Diagnostic{ - Severity: diag.Warning, - Summary: "Failed to retrieve account role. Marking the resource as removed.", - Detail: fmt.Sprintf("Id: %s", d.Id()), - }, - } + roleId, err := sdk.ParseAccountObjectIdentifier(targetIdentifier) + if err != nil { + return diag.FromErr(err) + } + if _, err := client.Roles.ShowByID(ctx, roleId); err != nil && errors.Is(err, sdk.ErrObjectNotFound) { + d.SetId("") + return diag.Diagnostics{ + diag.Diagnostic{ + Severity: diag.Warning, + Summary: "Failed to retrieve account role. Marking the resource as removed.", + Detail: fmt.Sprintf("Id: %s", d.Id()), + }, } } case sdk.ObjectTypeApplication: - if _, err := client.Applications.ShowByID(ctx, sdk.NewAccountObjectIdentifierFromFullyQualifiedName(targetIdentifier)); err != nil && errors.Is(err, sdk.ErrObjectNotFound) { + applicationId, err := sdk.ParseAccountObjectIdentifier(targetIdentifier) + if err != nil { + return diag.FromErr(err) + } + if _, err := client.Applications.ShowByID(ctx, applicationId); err != nil && errors.Is(err, sdk.ErrObjectNotFound) { d.SetId("") return diag.Diagnostics{ diag.Diagnostic{ @@ -225,20 +254,29 @@ func ReadContextGrantApplicationRole(ctx context.Context, d *schema.ResourceData func DeleteContextGrantApplicationRole(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { client := meta.(*provider.Context).Client - parts := strings.Split(d.Id(), "|") - id := sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(parts[0]) + parts := helpers.ParseResourceIdentifier(d.Id()) + id, err := sdk.ParseDatabaseObjectIdentifier(parts[0]) + if err != nil { + return diag.FromErr(err) + } objectType := parts[1] granteeName := parts[2] switch objectType { case "ACCOUNT_ROLE": - applicationRoleName := sdk.NewAccountObjectIdentifierFromFullyQualifiedName(granteeName) + applicationRoleName, err := sdk.ParseAccountObjectIdentifier(granteeName) + if err != nil { + return diag.FromErr(err) + } if err := client.ApplicationRoles.Revoke(ctx, sdk.NewRevokeApplicationRoleRequest(id).WithFrom(*sdk.NewKindOfRoleRequest().WithRoleName(&applicationRoleName))); err != nil { if errors.Is(err, sdk.ErrObjectNotExistOrAuthorized) { return diag.FromErr(err) } } case "APPLICATION": - applicationName := sdk.NewAccountObjectIdentifierFromFullyQualifiedName(granteeName) + applicationName, err := sdk.ParseAccountObjectIdentifier(granteeName) + if err != nil { + return diag.FromErr(err) + } if err := client.ApplicationRoles.Revoke(ctx, sdk.NewRevokeApplicationRoleRequest(id).WithFrom(*sdk.NewKindOfRoleRequest().WithApplicationName(&applicationName))); err != nil { if errors.Is(err, sdk.ErrObjectNotExistOrAuthorized) { return diag.FromErr(err) diff --git a/pkg/resources/grant_application_role_acceptance_test.go b/pkg/resources/grant_application_role_acceptance_test.go index b5f48a6675..fb70f56a2f 100644 --- a/pkg/resources/grant_application_role_acceptance_test.go +++ b/pkg/resources/grant_application_role_acceptance_test.go @@ -2,8 +2,11 @@ package resources_test import ( "fmt" + "regexp" "testing" + "github.com/hashicorp/terraform-plugin-testing/plancheck" + acc "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/testvars" @@ -122,3 +125,107 @@ func TestAcc_GrantApplicationRole_application(t *testing.T) { }, }) } + +func TestAcc_GrantApplicationRole_migrateFromV0941_ensureSmoothUpgradeWithNewResourceId(t *testing.T) { + parentRoleId := acc.TestClient().Ids.RandomAccountObjectIdentifier() + + acc.TestAccPreCheck(t) + app := createApp(t) + applicationRoleName := testvars.ApplicationRole1 + appRoleId := sdk.NewDatabaseObjectIdentifier(app.ID().Name(), applicationRoleName) + + resource.Test(t, resource.TestCase{ + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{ + "snowflake": { + VersionConstraint: "=0.94.1", + Source: "Snowflake-Labs/snowflake", + }, + }, + Config: grantApplicationRoleBasicConfig(fmt.Sprintf(`\"%s\".\"%s\"`, appRoleId.DatabaseName(), appRoleId.Name()), parentRoleId.Name()), + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_grant_application_role.test", "id", fmt.Sprintf(`%s|ACCOUNT_ROLE|%s`, appRoleId.FullyQualifiedName(), parentRoleId.FullyQualifiedName())), + ), + }, + { + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + Config: grantApplicationRoleBasicConfig(fmt.Sprintf(`\"%s\".\"%s\"`, appRoleId.DatabaseName(), appRoleId.Name()), parentRoleId.Name()), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_grant_application_role.test", plancheck.ResourceActionNoop), + }, + PostApplyPostRefresh: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_grant_application_role.test", plancheck.ResourceActionNoop), + }, + }, + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_grant_application_role.test", "id", fmt.Sprintf(`%s|ACCOUNT_ROLE|%s`, appRoleId.FullyQualifiedName(), parentRoleId.FullyQualifiedName())), + ), + }, + }, + }) +} + +func grantApplicationRoleBasicConfig(applicationRoleName string, parentRoleName string) string { + return fmt.Sprintf(` +resource "snowflake_account_role" "test" { + name = "%s" +} + +resource "snowflake_grant_application_role" "test" { + application_role_name = "%s" + parent_account_role_name = snowflake_account_role.test.name +} +`, parentRoleName, applicationRoleName) +} + +func TestAcc_GrantApplicationRole_IdentifierQuotingDiffSuppression(t *testing.T) { + parentRoleId := acc.TestClient().Ids.RandomAccountObjectIdentifier() + + acc.TestAccPreCheck(t) + app := createApp(t) + applicationRoleName := testvars.ApplicationRole1 + appRoleId := sdk.NewDatabaseObjectIdentifier(app.ID().Name(), applicationRoleName) + + unquotedApplicationRoleId := fmt.Sprintf(`%s.%s`, appRoleId.DatabaseName(), appRoleId.Name()) + quotedParentRoleId := fmt.Sprintf(`\"%s\"`, parentRoleId.Name()) + + resource.Test(t, resource.TestCase{ + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{ + "snowflake": { + VersionConstraint: "=0.94.1", + Source: "Snowflake-Labs/snowflake", + }, + }, + ExpectError: regexp.MustCompile("Error: Provider produced inconsistent final plan"), + Config: grantApplicationRoleBasicConfig(unquotedApplicationRoleId, quotedParentRoleId), + }, + { + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + Config: grantApplicationRoleBasicConfig(unquotedApplicationRoleId, quotedParentRoleId), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_grant_application_role.test", plancheck.ResourceActionCreate), + }, + PostApplyPostRefresh: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_grant_application_role.test", plancheck.ResourceActionNoop), + }, + }, + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_grant_application_role.test", "application_role_name", unquotedApplicationRoleId), + resource.TestCheckResourceAttr("snowflake_grant_application_role.test", "parent_account_role_name", parentRoleId.Name()), + resource.TestCheckResourceAttr("snowflake_grant_application_role.test", "id", fmt.Sprintf(`%s|ACCOUNT_ROLE|%s`, appRoleId.FullyQualifiedName(), parentRoleId.FullyQualifiedName())), + ), + }, + }, + }) +} diff --git a/pkg/resources/grant_database_role.go b/pkg/resources/grant_database_role.go index 1855de87ec..132712fd48 100644 --- a/pkg/resources/grant_database_role.go +++ b/pkg/resources/grant_database_role.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "log" - "strings" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/provider" @@ -20,6 +19,7 @@ var grantDatabaseRoleSchema = map[string]*schema.Schema{ Description: "The fully qualified name of the database role which will be granted to share or parent role.", ForceNew: true, ValidateDiagFunc: IsValidIdentifier[sdk.DatabaseObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, }, "parent_role_name": { Type: schema.TypeString, @@ -27,6 +27,7 @@ var grantDatabaseRoleSchema = map[string]*schema.Schema{ Description: "The fully qualified name of the parent account role which will create a parent-child relationship between the roles.", ForceNew: true, ValidateDiagFunc: IsValidIdentifier[sdk.AccountObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, ExactlyOneOf: []string{ "parent_role_name", "parent_database_role_name", @@ -39,6 +40,7 @@ var grantDatabaseRoleSchema = map[string]*schema.Schema{ Description: "The fully qualified name of the parent database role which will create a parent-child relationship between the roles.", ForceNew: true, ValidateDiagFunc: IsValidIdentifier[sdk.DatabaseObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, ExactlyOneOf: []string{ "parent_role_name", "parent_database_role_name", @@ -51,6 +53,7 @@ var grantDatabaseRoleSchema = map[string]*schema.Schema{ Description: "The fully qualified name of the share on which privileges will be granted.", ForceNew: true, ValidateDiagFunc: IsValidIdentifier[sdk.AccountObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, ExactlyOneOf: []string{ "parent_role_name", "parent_database_role_name", @@ -67,24 +70,42 @@ func GrantDatabaseRole() *schema.Resource { Schema: grantDatabaseRoleSchema, Importer: &schema.ResourceImporter{ StateContext: func(ctx context.Context, d *schema.ResourceData, m interface{}) ([]*schema.ResourceData, error) { - parts := strings.Split(d.Id(), helpers.IDDelimiter) + parts := helpers.ParseResourceIdentifier(d.Id()) if len(parts) != 3 { return nil, fmt.Errorf("invalid ID specified: %v, expected ||", d.Id()) } - if err := d.Set("database_role_name", parts[0]); err != nil { + + databaseRoleId, err := sdk.ParseDatabaseObjectIdentifier(parts[0]) + if err != nil { + return nil, err + } + if err := d.Set("database_role_name", databaseRoleId.FullyQualifiedName()); err != nil { return nil, err } + switch parts[1] { case "ROLE": - if err := d.Set("parent_role_name", strings.Trim(parts[2], "\"")); err != nil { + accountRoleId, err := sdk.ParseAccountObjectIdentifier(parts[2]) + if err != nil { + return nil, err + } + if err := d.Set("parent_role_name", accountRoleId.Name()); err != nil { return nil, err } case "DATABASE ROLE": - if err := d.Set("parent_database_role_name", parts[2]); err != nil { + parentDatabaseId, err := sdk.ParseDatabaseObjectIdentifier(parts[2]) + if err != nil { + return nil, err + } + if err := d.Set("parent_database_role_name", parentDatabaseId.FullyQualifiedName()); err != nil { return nil, err } case "SHARE": - if err := d.Set("share_name", strings.Trim(parts[2], "\"")); err != nil { + shareId, err := sdk.ParseAccountObjectIdentifier(parts[2]) + if err != nil { + return nil, err + } + if err := d.Set("share_name", shareId.Name()); err != nil { return nil, err } default: @@ -102,26 +123,38 @@ func CreateGrantDatabaseRole(d *schema.ResourceData, meta interface{}) error { client := meta.(*provider.Context).Client ctx := context.Background() databaseRoleName := d.Get("database_role_name").(string) - databaseRoleIdentifier := sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(databaseRoleName) + databaseRoleIdentifier, err := sdk.ParseDatabaseObjectIdentifier(databaseRoleName) + if err != nil { + return err + } // format of snowflakeResourceID is || var snowflakeResourceID string if parentRoleName, ok := d.GetOk("parent_role_name"); ok && parentRoleName.(string) != "" { - parentRoleIdentifier := sdk.NewAccountObjectIdentifierFromFullyQualifiedName(parentRoleName.(string)) - snowflakeResourceID = helpers.EncodeSnowflakeID(databaseRoleIdentifier.FullyQualifiedName(), sdk.ObjectTypeRole.String(), parentRoleIdentifier.FullyQualifiedName()) + parentRoleIdentifier, err := sdk.ParseAccountObjectIdentifier(parentRoleName.(string)) + if err != nil { + return err + } + snowflakeResourceID = helpers.EncodeResourceIdentifier(databaseRoleIdentifier.FullyQualifiedName(), sdk.ObjectTypeRole.String(), parentRoleIdentifier.FullyQualifiedName()) req := sdk.NewGrantDatabaseRoleRequest(databaseRoleIdentifier).WithAccountRole(parentRoleIdentifier) if err := client.DatabaseRoles.Grant(ctx, req); err != nil { return err } } else if parentDatabaseRoleName, ok := d.GetOk("parent_database_role_name"); ok && parentDatabaseRoleName.(string) != "" { - parentRoleIdentifier := sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(parentDatabaseRoleName.(string)) - snowflakeResourceID = helpers.EncodeSnowflakeID(databaseRoleIdentifier.FullyQualifiedName(), sdk.ObjectTypeDatabaseRole.String(), parentRoleIdentifier.FullyQualifiedName()) + parentRoleIdentifier, err := sdk.ParseDatabaseObjectIdentifier(parentDatabaseRoleName.(string)) + if err != nil { + return err + } + snowflakeResourceID = helpers.EncodeResourceIdentifier(databaseRoleIdentifier.FullyQualifiedName(), sdk.ObjectTypeDatabaseRole.String(), parentRoleIdentifier.FullyQualifiedName()) req := sdk.NewGrantDatabaseRoleRequest(databaseRoleIdentifier).WithDatabaseRole(parentRoleIdentifier) if err := client.DatabaseRoles.Grant(ctx, req); err != nil { return err } } else if shareName, ok := d.GetOk("share_name"); ok && shareName.(string) != "" { - shareIdentifier := sdk.NewAccountObjectIdentifierFromFullyQualifiedName(shareName.(string)) - snowflakeResourceID = helpers.EncodeSnowflakeID(databaseRoleIdentifier.FullyQualifiedName(), sdk.ObjectTypeShare.String(), shareIdentifier.FullyQualifiedName()) + shareIdentifier, err := sdk.ParseAccountObjectIdentifier(shareName.(string)) + if err != nil { + return err + } + snowflakeResourceID = helpers.EncodeResourceIdentifier(databaseRoleIdentifier.FullyQualifiedName(), sdk.ObjectTypeShare.String(), shareIdentifier.FullyQualifiedName()) req := sdk.NewGrantDatabaseRoleToShareRequest(databaseRoleIdentifier, shareIdentifier) if err := client.DatabaseRoles.GrantToShare(ctx, req); err != nil { return err @@ -134,9 +167,12 @@ func CreateGrantDatabaseRole(d *schema.ResourceData, meta interface{}) error { // ReadGrantDatabaseRole implements schema.ReadFunc. func ReadGrantDatabaseRole(d *schema.ResourceData, meta interface{}) error { client := meta.(*provider.Context).Client - parts := strings.Split(d.Id(), helpers.IDDelimiter) + parts := helpers.ParseResourceIdentifier(d.Id()) databaseRoleName := parts[0] - databaseRoleIdentifier := sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(databaseRoleName) + databaseRoleIdentifier, err := sdk.ParseDatabaseObjectIdentifier(databaseRoleName) + if err != nil { + return err + } objectType := parts[1] targetIdentifier := parts[2] ctx := context.Background() @@ -171,22 +207,37 @@ func ReadGrantDatabaseRole(d *schema.ResourceData, meta interface{}) error { func DeleteGrantDatabaseRole(d *schema.ResourceData, meta interface{}) error { client := meta.(*provider.Context).Client - parts := strings.Split(d.Id(), "|") - id := sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(parts[0]) + parts := helpers.ParseResourceIdentifier(d.Id()) + id, err := sdk.ParseDatabaseObjectIdentifier(parts[0]) + if err != nil { + return err + } objectType := parts[1] granteeName := parts[2] ctx := context.Background() switch objectType { case "ROLE": - if err := client.DatabaseRoles.Revoke(ctx, sdk.NewRevokeDatabaseRoleRequest(id).WithAccountRole(sdk.NewAccountObjectIdentifierFromFullyQualifiedName(granteeName))); err != nil { + accountRoleId, err := sdk.ParseAccountObjectIdentifier(granteeName) + if err != nil { + return err + } + if err := client.DatabaseRoles.Revoke(ctx, sdk.NewRevokeDatabaseRoleRequest(id).WithAccountRole(accountRoleId)); err != nil { return err } case "DATABASE ROLE": - if err := client.DatabaseRoles.Revoke(ctx, sdk.NewRevokeDatabaseRoleRequest(id).WithDatabaseRole(sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(granteeName))); err != nil { + databaseRoleId, err := sdk.ParseDatabaseObjectIdentifier(granteeName) + if err != nil { + return err + } + if err := client.DatabaseRoles.Revoke(ctx, sdk.NewRevokeDatabaseRoleRequest(id).WithDatabaseRole(databaseRoleId)); err != nil { return err } case "SHARE": - if err := client.DatabaseRoles.RevokeFromShare(ctx, sdk.NewRevokeDatabaseRoleFromShareRequest(id, sdk.NewAccountObjectIdentifierFromFullyQualifiedName(granteeName))); err != nil { + sharedId, err := sdk.ParseAccountObjectIdentifier(granteeName) + if err != nil { + return err + } + if err := client.DatabaseRoles.RevokeFromShare(ctx, sdk.NewRevokeDatabaseRoleFromShareRequest(id, sharedId)); err != nil { return err } } diff --git a/pkg/resources/grant_database_role_acceptance_test.go b/pkg/resources/grant_database_role_acceptance_test.go index e5a39be6b2..48120de920 100644 --- a/pkg/resources/grant_database_role_acceptance_test.go +++ b/pkg/resources/grant_database_role_acceptance_test.go @@ -5,6 +5,8 @@ import ( "strings" "testing" + "github.com/hashicorp/terraform-plugin-testing/plancheck" + acc "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance" "github.com/hashicorp/terraform-plugin-testing/config" @@ -250,3 +252,137 @@ func TestAcc_GrantDatabaseRole_shareWithDots(t *testing.T) { }, }) } + +func TestAcc_GrantDatabaseRole_migrateFromV0941_ensureSmoothUpgradeWithNewResourceId(t *testing.T) { + databaseId := acc.TestClient().Ids.RandomAccountObjectIdentifier() + roleId := acc.TestClient().Ids.RandomDatabaseObjectIdentifierInDatabase(databaseId) + parentRoleId := acc.TestClient().Ids.RandomDatabaseObjectIdentifierInDatabase(databaseId) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { acc.TestAccPreCheck(t) }, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{ + "snowflake": { + VersionConstraint: "=0.94.1", + Source: "Snowflake-Labs/snowflake", + }, + }, + Config: grantDatabaseRoleBasicConfigQuoted(databaseId.Name(), roleId.Name(), parentRoleId.Name()), + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_grant_database_role.test", "id", fmt.Sprintf(`%s|DATABASE ROLE|%s`, roleId.FullyQualifiedName(), parentRoleId.FullyQualifiedName())), + ), + }, + { + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + Config: grantDatabaseRoleBasicConfigQuoted(databaseId.Name(), roleId.Name(), parentRoleId.Name()), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_grant_database_role.test", plancheck.ResourceActionNoop), + }, + PostApplyPostRefresh: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_grant_database_role.test", plancheck.ResourceActionNoop), + }, + }, + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_grant_database_role.test", "id", fmt.Sprintf(`%s|DATABASE ROLE|%s`, roleId.FullyQualifiedName(), parentRoleId.FullyQualifiedName())), + ), + }, + }, + }) +} + +func grantDatabaseRoleBasicConfigQuoted(databaseName string, roleName string, parentRoleName string) string { + return fmt.Sprintf(` +resource "snowflake_database" "test" { + name = "%[1]s" +} + +resource "snowflake_database_role" "role" { + database = snowflake_database.test.name + name = "%[2]s" +} + +resource "snowflake_database_role" "parent_role" { + database = snowflake_database.test.name + name = "%[3]s" +} + +resource "snowflake_grant_database_role" "test" { + database_role_name = "\"${snowflake_database.test.name}\".\"${snowflake_database_role.role.name}\"" + parent_database_role_name = "\"${snowflake_database.test.name}\".\"${snowflake_database_role.parent_role.name}\"" +} +`, databaseName, roleName, parentRoleName) +} + +func grantDatabaseRoleBasicConfigUnquoted(databaseName string, roleName string, parentRoleName string) string { + return fmt.Sprintf(` +resource "snowflake_database" "test" { + name = "%[1]s" +} + +resource "snowflake_database_role" "role" { + database = snowflake_database.test.name + name = "%[2]s" +} + +resource "snowflake_database_role" "parent_role" { + database = snowflake_database.test.name + name = "%[3]s" +} + +resource "snowflake_grant_database_role" "test" { + database_role_name = "${snowflake_database.test.name}.${snowflake_database_role.role.name}" + parent_database_role_name = "${snowflake_database.test.name}.${snowflake_database_role.parent_role.name}" +} +`, databaseName, roleName, parentRoleName) +} + +func TestAcc_GrantDatabaseRole_IdentifierQuotingDiffSuppression(t *testing.T) { + databaseId := acc.TestClient().Ids.RandomAccountObjectIdentifier() + roleId := acc.TestClient().Ids.RandomDatabaseObjectIdentifierInDatabase(databaseId) + parentRoleId := acc.TestClient().Ids.RandomDatabaseObjectIdentifierInDatabase(databaseId) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { acc.TestAccPreCheck(t) }, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{ + "snowflake": { + VersionConstraint: "=0.94.1", + Source: "Snowflake-Labs/snowflake", + }, + }, + Config: grantDatabaseRoleBasicConfigUnquoted(databaseId.Name(), roleId.Name(), parentRoleId.Name()), + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_grant_database_role.test", "database_role_name", fmt.Sprintf("%s.%s", roleId.DatabaseName(), roleId.Name())), + resource.TestCheckResourceAttr("snowflake_grant_database_role.test", "parent_database_role_name", fmt.Sprintf("%s.%s", roleId.DatabaseName(), parentRoleId.Name())), + resource.TestCheckResourceAttr("snowflake_grant_database_role.test", "id", fmt.Sprintf(`%s|DATABASE ROLE|%s`, roleId.FullyQualifiedName(), parentRoleId.FullyQualifiedName())), + ), + }, + { + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + Config: grantDatabaseRoleBasicConfigUnquoted(databaseId.Name(), roleId.Name(), parentRoleId.Name()), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_grant_database_role.test", plancheck.ResourceActionNoop), + }, + PostApplyPostRefresh: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_grant_database_role.test", plancheck.ResourceActionNoop), + }, + }, + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_grant_database_role.test", "database_role_name", fmt.Sprintf("%s.%s", roleId.DatabaseName(), roleId.Name())), + resource.TestCheckResourceAttr("snowflake_grant_database_role.test", "parent_database_role_name", fmt.Sprintf("%s.%s", roleId.DatabaseName(), parentRoleId.Name())), + resource.TestCheckResourceAttr("snowflake_grant_database_role.test", "id", fmt.Sprintf(`%s|DATABASE ROLE|%s`, roleId.FullyQualifiedName(), parentRoleId.FullyQualifiedName())), + ), + }, + }, + }) +} diff --git a/pkg/resources/grant_ownership.go b/pkg/resources/grant_ownership.go index 37aea7768b..a79e9679ae 100644 --- a/pkg/resources/grant_ownership.go +++ b/pkg/resources/grant_ownership.go @@ -21,6 +21,7 @@ var grantOwnershipSchema = map[string]*schema.Schema{ ForceNew: true, Description: "The fully qualified name of the account role to which privileges will be granted.", ValidateDiagFunc: IsValidIdentifier[sdk.AccountObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, ExactlyOneOf: []string{ "account_role_name", "database_role_name", @@ -32,6 +33,7 @@ var grantOwnershipSchema = map[string]*schema.Schema{ ForceNew: true, Description: "The fully qualified name of the database role to which privileges will be granted.", ValidateDiagFunc: IsValidIdentifier[sdk.DatabaseObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, ExactlyOneOf: []string{ "account_role_name", "database_role_name", @@ -66,10 +68,11 @@ var grantOwnershipSchema = map[string]*schema.Schema{ ValidateFunc: validation.StringInSlice(sdk.ValidGrantOwnershipObjectTypesString, true), }, "object_name": { - Type: schema.TypeString, - Optional: true, - ForceNew: true, - Description: "Specifies the identifier for the object on which you are transferring ownership.", + Type: schema.TypeString, + Optional: true, + ForceNew: true, + Description: "Specifies the identifier for the object on which you are transferring ownership.", + DiffSuppressFunc: suppressIdentifierQuoting, RequiredWith: []string{ "on.0.object_type", }, @@ -129,6 +132,7 @@ func grantOwnershipBulkOperationSchema(branchName string) map[string]*schema.Sch ForceNew: true, Description: "The fully qualified name of the database.", ValidateDiagFunc: IsValidIdentifier[sdk.AccountObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, ExactlyOneOf: []string{ fmt.Sprintf("on.0.%s.0.in_database", branchName), fmt.Sprintf("on.0.%s.0.in_schema", branchName), @@ -140,6 +144,7 @@ func grantOwnershipBulkOperationSchema(branchName string) map[string]*schema.Sch ForceNew: true, Description: "The fully qualified name of the schema.", ValidateDiagFunc: IsValidIdentifier[sdk.DatabaseObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, ExactlyOneOf: []string{ fmt.Sprintf("on.0.%s.0.in_database", branchName), fmt.Sprintf("on.0.%s.0.in_schema", branchName), @@ -246,10 +251,14 @@ func CreateGrantOwnership(ctx context.Context, d *schema.ResourceData, meta any) return diag.FromErr(err) } + grantTo, err := getOwnershipGrantTo(d) + if err != nil { + return diag.FromErr(err) + } err = client.Grants.GrantOwnership( ctx, *grantOn, - getOwnershipGrantTo(d), + grantTo, getOwnershipGrantOpts(id), ) if err != nil { @@ -473,26 +482,42 @@ func getOwnershipGrantOn(d *schema.ResourceData) (*sdk.OwnershipGrantOn, error) Name: objectName, } case len(onAll) > 0: - ownershipGrantOn.All = getGrantOnSchemaObjectIn(onAll[0].(map[string]any)) + grantOnSchemaObjectIn, err := getGrantOnSchemaObjectIn(onAll[0].(map[string]any)) + if err != nil { + return nil, err + } + ownershipGrantOn.All = grantOnSchemaObjectIn case len(onFuture) > 0: - ownershipGrantOn.Future = getGrantOnSchemaObjectIn(onFuture[0].(map[string]any)) + grantOnSchemaObjectIn, err := getGrantOnSchemaObjectIn(onFuture[0].(map[string]any)) + if err != nil { + return nil, err + } + ownershipGrantOn.Future = grantOnSchemaObjectIn } return ownershipGrantOn, nil } -func getOwnershipGrantTo(d *schema.ResourceData) sdk.OwnershipGrantTo { +func getOwnershipGrantTo(d *schema.ResourceData) (sdk.OwnershipGrantTo, error) { var ownershipGrantTo sdk.OwnershipGrantTo if accountRoleName, ok := d.GetOk("account_role_name"); ok { - ownershipGrantTo.AccountRoleName = sdk.Pointer(sdk.NewAccountObjectIdentifierFromFullyQualifiedName(accountRoleName.(string))) + accountRoleId, err := sdk.ParseAccountObjectIdentifier(accountRoleName.(string)) + if err != nil { + return ownershipGrantTo, err + } + ownershipGrantTo.AccountRoleName = &accountRoleId } if databaseRoleName, ok := d.GetOk("database_role_name"); ok { - ownershipGrantTo.DatabaseRoleName = sdk.Pointer(sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(databaseRoleName.(string))) + databaseRoleId, err := sdk.ParseDatabaseObjectIdentifier(databaseRoleName.(string)) + if err != nil { + return ownershipGrantTo, err + } + ownershipGrantTo.DatabaseRoleName = sdk.Pointer(databaseRoleId) } - return ownershipGrantTo + return ownershipGrantTo, nil } func getOwnershipGrantOpts(id *GrantOwnershipId) *sdk.GrantOwnershipOptions { @@ -565,10 +590,18 @@ func createGrantOwnershipIdFromSchema(d *schema.ResourceData) (*GrantOwnershipId switch { case accountRoleNameOk: id.GrantOwnershipTargetRoleKind = ToAccountGrantOwnershipTargetRoleKind - id.AccountRoleName = sdk.NewAccountObjectIdentifierFromFullyQualifiedName(accountRoleName.(string)) + accountRoleId, err := sdk.ParseAccountObjectIdentifier(accountRoleName.(string)) + if err != nil { + return nil, err + } + id.AccountRoleName = accountRoleId case databaseRoleNameOk: id.GrantOwnershipTargetRoleKind = ToDatabaseGrantOwnershipTargetRoleKind - id.DatabaseRoleName = sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(databaseRoleName.(string)) + databaseRoleId, err := sdk.ParseDatabaseObjectIdentifier(databaseRoleName.(string)) + if err != nil { + return nil, err + } + id.DatabaseRoleName = databaseRoleId } outboundPrivileges, outboundPrivilegesOk := d.GetOk("outbound_privileges") @@ -601,10 +634,18 @@ func createGrantOwnershipIdFromSchema(d *schema.ResourceData) (*GrantOwnershipId } case len(all) > 0: id.Kind = OnAllGrantOwnershipKind - id.Data = getBulkOperationGrantData(getGrantOnSchemaObjectIn(all[0].(map[string]any))) + grantOn, err := getGrantOnSchemaObjectIn(all[0].(map[string]any)) + if err != nil { + return nil, err + } + id.Data = getBulkOperationGrantData(grantOn) case len(future) > 0: id.Kind = OnFutureGrantOwnershipKind - id.Data = getBulkOperationGrantData(getGrantOnSchemaObjectIn(future[0].(map[string]any))) + grantOn, err := getGrantOnSchemaObjectIn(future[0].(map[string]any)) + if err != nil { + return nil, err + } + id.Data = getBulkOperationGrantData(grantOn) } return id, nil diff --git a/pkg/resources/grant_ownership_acceptance_test.go b/pkg/resources/grant_ownership_acceptance_test.go index 1d2357b00d..63212e069f 100644 --- a/pkg/resources/grant_ownership_acceptance_test.go +++ b/pkg/resources/grant_ownership_acceptance_test.go @@ -1367,3 +1367,160 @@ func checkResourceOwnershipIsGranted(opts *sdk.ShowGrantOptions, grantOn sdk.Obj return nil } } + +func TestAcc_GrantOwnership_migrateFromV0941_ensureSmoothUpgradeWithNewResourceId(t *testing.T) { + tableId := acc.TestClient().Ids.RandomSchemaObjectIdentifier() + accountRoleId := acc.TestClient().Ids.RandomAccountObjectIdentifier() + escapedFullyQualifiedName := fmt.Sprintf(`\"%s\".\"%s\".\"%s\"`, tableId.DatabaseName(), tableId.SchemaName(), tableId.Name()) + + acc.TestAccPreCheck(t) + resourceName := "snowflake_grant_ownership.test" + + resource.Test(t, resource.TestCase{ + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{ + "snowflake": { + VersionConstraint: "=0.94.1", + Source: "Snowflake-Labs/snowflake", + }, + }, + Config: grantOwnershipOnTableBasicConfig(acc.TestDatabaseName, acc.TestSchemaName, tableId.Name(), accountRoleId.Name(), escapedFullyQualifiedName), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(resourceName, "id", fmt.Sprintf("ToAccountRole|%s||OnObject|TABLE|%s", accountRoleId.FullyQualifiedName(), tableId.FullyQualifiedName())), + ), + }, + { + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + Config: grantOwnershipOnTableBasicConfig(acc.TestDatabaseName, acc.TestSchemaName, tableId.Name(), accountRoleId.Name(), escapedFullyQualifiedName), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction(resourceName, plancheck.ResourceActionNoop), + }, + PostApplyPostRefresh: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction(resourceName, plancheck.ResourceActionNoop), + }, + }, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(resourceName, "id", fmt.Sprintf("ToAccountRole|%s||OnObject|TABLE|%s", accountRoleId.FullyQualifiedName(), tableId.FullyQualifiedName())), + ), + }, + }, + }) +} + +func grantOwnershipOnTableBasicConfig(databaseName string, schemaName string, tableName string, roleName string, fullTableName string) string { + return fmt.Sprintf(` +resource "snowflake_account_role" "test" { + name = "%[4]s" +} + +resource "snowflake_table" "test" { + name = "%[3]s" + database = "%[1]s" + schema = "%[2]s" + + column { + name = "id" + type = "NUMBER(38,0)" + } +} + +resource "snowflake_grant_ownership" "test" { + depends_on = [snowflake_table.test] + account_role_name = snowflake_account_role.test.name + on { + object_type = "TABLE" + object_name = "%[5]s" + } +} +`, databaseName, schemaName, tableName, roleName, fullTableName) +} + +func TestAcc_GrantOwnership_IdentifierQuotingDiffSuppression(t *testing.T) { + databaseId := acc.TestClient().Ids.RandomAccountObjectIdentifier() + schemaId := acc.TestClient().Ids.RandomDatabaseObjectIdentifierInDatabase(databaseId) + tableId := acc.TestClient().Ids.RandomSchemaObjectIdentifierInSchema(schemaId) + accountRoleId := acc.TestClient().Ids.RandomAccountObjectIdentifier() + unescapedFullyQualifiedName := fmt.Sprintf(`%s.%s.%s`, tableId.DatabaseName(), tableId.SchemaName(), tableId.Name()) + + acc.TestAccPreCheck(t) + resourceName := "snowflake_grant_ownership.test" + + resource.Test(t, resource.TestCase{ + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{ + "snowflake": { + VersionConstraint: "=0.94.1", + Source: "Snowflake-Labs/snowflake", + }, + }, + Config: grantOwnershipOnTableBasicConfigWithManagedDatabaseAndSchema(databaseId.Name(), schemaId.Name(), tableId.Name(), accountRoleId.Name(), unescapedFullyQualifiedName), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(resourceName, "on.0.object_name", unescapedFullyQualifiedName), + resource.TestCheckResourceAttr(resourceName, "id", fmt.Sprintf("ToAccountRole|%s||OnObject|TABLE|%s", accountRoleId.FullyQualifiedName(), tableId.FullyQualifiedName())), + ), + }, + { + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + Config: grantOwnershipOnTableBasicConfigWithManagedDatabaseAndSchema(databaseId.Name(), schemaId.Name(), tableId.Name(), accountRoleId.Name(), unescapedFullyQualifiedName), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction(resourceName, plancheck.ResourceActionNoop), + }, + PostApplyPostRefresh: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction(resourceName, plancheck.ResourceActionNoop), + }, + }, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(resourceName, "on.0.object_name", unescapedFullyQualifiedName), + resource.TestCheckResourceAttr(resourceName, "id", fmt.Sprintf("ToAccountRole|%s||OnObject|TABLE|%s", accountRoleId.FullyQualifiedName(), tableId.FullyQualifiedName())), + ), + }, + }, + }) +} + +func grantOwnershipOnTableBasicConfigWithManagedDatabaseAndSchema(databaseName string, schemaName string, tableName string, roleName string, fullTableName string) string { + return fmt.Sprintf(` +resource "snowflake_account_role" "test" { + name = "%[4]s" +} + +resource "snowflake_database" "test" { + name = "%[1]s" +} + +resource "snowflake_schema" "test" { + database = snowflake_database.test.name + name = "%[2]s" +} + +resource "snowflake_table" "test" { + name = "%[3]s" + database = snowflake_database.test.name + schema = snowflake_schema.test.name + + column { + name = "id" + type = "NUMBER(38,0)" + } +} + +resource "snowflake_grant_ownership" "test" { + depends_on = [snowflake_table.test] + account_role_name = snowflake_account_role.test.name + on { + object_type = "TABLE" + object_name = "%[5]s" + } +} +`, databaseName, schemaName, tableName, roleName, fullTableName) +} diff --git a/pkg/resources/grant_ownership_identifier.go b/pkg/resources/grant_ownership_identifier.go index 2b897f262c..2b233d6932 100644 --- a/pkg/resources/grant_ownership_identifier.go +++ b/pkg/resources/grant_ownership_identifier.go @@ -94,9 +94,17 @@ func ParseGrantOwnershipId(id string) (*GrantOwnershipId, error) { grantOwnershipId.GrantOwnershipTargetRoleKind = GrantOwnershipTargetRoleKind(parts[0]) switch grantOwnershipId.GrantOwnershipTargetRoleKind { case ToAccountGrantOwnershipTargetRoleKind: - grantOwnershipId.AccountRoleName = sdk.NewAccountObjectIdentifierFromFullyQualifiedName(parts[1]) + accountRoleId, err := sdk.ParseAccountObjectIdentifier(parts[1]) + if err != nil { + return nil, err + } + grantOwnershipId.AccountRoleName = accountRoleId case ToDatabaseGrantOwnershipTargetRoleKind: - grantOwnershipId.DatabaseRoleName = sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(parts[1]) + databaseRoleId, err := sdk.ParseDatabaseObjectIdentifier(parts[1]) + if err != nil { + return nil, err + } + grantOwnershipId.DatabaseRoleName = databaseRoleId default: return grantOwnershipId, sdk.NewError(fmt.Sprintf("unknown GrantOwnershipTargetRoleKind: %v, valid options are %v | %v", grantOwnershipId.GrantOwnershipTargetRoleKind, ToAccountGrantOwnershipTargetRoleKind, ToDatabaseGrantOwnershipTargetRoleKind)) } @@ -135,9 +143,17 @@ func ParseGrantOwnershipId(id string) (*GrantOwnershipId, error) { bulkOperationGrantData.Kind = BulkOperationGrantKind(parts[5]) switch bulkOperationGrantData.Kind { case InDatabaseBulkOperationGrantKind: - bulkOperationGrantData.Database = sdk.Pointer(sdk.NewAccountObjectIdentifierFromFullyQualifiedName(parts[6])) + databaseId, err := sdk.ParseAccountObjectIdentifier(parts[6]) + if err != nil { + return nil, err + } + bulkOperationGrantData.Database = sdk.Pointer(databaseId) case InSchemaBulkOperationGrantKind: - bulkOperationGrantData.Schema = sdk.Pointer(sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(parts[6])) + schemaId, err := sdk.ParseDatabaseObjectIdentifier(parts[6]) + if err != nil { + return nil, err + } + bulkOperationGrantData.Schema = sdk.Pointer(schemaId) default: return grantOwnershipId, sdk.NewError(fmt.Sprintf("invalid BulkOperationGrantKind: %s, valid options are %v | %v", bulkOperationGrantData.Kind, InDatabaseBulkOperationGrantKind, InSchemaBulkOperationGrantKind)) } diff --git a/pkg/resources/grant_ownership_identifier_test.go b/pkg/resources/grant_ownership_identifier_test.go index 54ea170adc..61d755b2a4 100644 --- a/pkg/resources/grant_ownership_identifier_test.go +++ b/pkg/resources/grant_ownership_identifier_test.go @@ -86,6 +86,20 @@ func TestParseGrantOwnershipId(t *testing.T) { }, }, }, + { + Name: "grant ownership on all tables in database to account role - empty database id", + Identifier: `ToAccountRole|"account-role"||OnAll|TABLES|InDatabase|`, + Expected: GrantOwnershipId{ + GrantOwnershipTargetRoleKind: ToAccountGrantOwnershipTargetRoleKind, + AccountRoleName: sdk.NewAccountObjectIdentifier("account-role"), + Kind: OnAllGrantOwnershipKind, + Data: &BulkOperationGrantData{ + ObjectNamePlural: sdk.PluralObjectTypeTables, + Kind: InDatabaseBulkOperationGrantKind, + Database: sdk.Pointer(sdk.NewAccountObjectIdentifier("")), + }, + }, + }, { Name: "grant ownership on all tables in schema to account role", Identifier: `ToAccountRole|"account-role"||OnAll|TABLES|InSchema|"database-name"."schema-name"`, @@ -115,6 +129,21 @@ func TestParseGrantOwnershipId(t *testing.T) { }, }, }, + { + Name: "grant ownership on future tables in database to account role - empty database", + Identifier: `ToAccountRole|"account-role"|COPY|OnFuture|TABLES|InDatabase|`, + Expected: GrantOwnershipId{ + GrantOwnershipTargetRoleKind: ToAccountGrantOwnershipTargetRoleKind, + AccountRoleName: sdk.NewAccountObjectIdentifier("account-role"), + OutboundPrivilegesBehavior: sdk.Pointer(CopyOutboundPrivilegesBehavior), + Kind: OnFutureGrantOwnershipKind, + Data: &BulkOperationGrantData{ + ObjectNamePlural: sdk.PluralObjectTypeTables, + Kind: InDatabaseBulkOperationGrantKind, + Database: sdk.Pointer(sdk.NewAccountObjectIdentifier("")), + }, + }, + }, { Name: "grant ownership on future tables in schema to account role", Identifier: `ToAccountRole|"account-role"|COPY|OnFuture|TABLES|InSchema|"database-name"."schema-name"`, @@ -188,16 +217,11 @@ func TestParseGrantOwnershipId(t *testing.T) { Identifier: `ToAccountRole|"account-role"|COPY|OnAll|TABLES|InvalidOption|"some-identifier"`, Error: "invalid BulkOperationGrantKind: InvalidOption, valid options are InDatabase | InSchema", }, - // { - // Name: "TODO(SNOW-999049 - no error because of bad identifiers): validation: OnAll in database - missing database identifier", - // Identifier: `ToAccountRole|"account-role"|COPY|OnAll|InvalidTarget|InDatabase|`, - // Error: "TODO", - // }, - // { - // Name: "TODO(SNOW-999049 - panic because of bad identifiers): validation: OnAll in database - missing schema identifier", - // Identifier: `ToAccountRole|"account-role"|COPY|OnAll|InvalidTarget|InSchema|`, - // Error: "TODO", - // }, + { + Name: "validation: OnAll in database - missing schema identifier", + Identifier: `ToAccountRole|"account-role"|COPY|OnAll|InvalidTarget|InSchema|`, + Error: "incompatible identifier: ", + }, { Name: "validation: not enough parts for OnFuture kind", Identifier: `ToAccountRole|"account-role"|COPY|OnFuture|TABLES`, @@ -208,16 +232,11 @@ func TestParseGrantOwnershipId(t *testing.T) { Identifier: `ToAccountRole|"account-role"|COPY|OnFuture|TABLES|InvalidOption|"some-identifier"`, Error: "invalid BulkOperationGrantKind: InvalidOption, valid options are InDatabase | InSchema", }, - // { - // Name: "TODO(SNOW-999049 - no error because of bad identifiers): validation: OnFuture in database - missing database identifier", - // Identifier: `ToAccountRole|"account-role"|COPY|OnFuture|InvalidTarget|InDatabase|`, - // Error: "TODO", - // }, - // { - // Name: "TODO(SNOW-999049 - panic because of bad identifiers): validation: OnFuture in database - missing schema identifier", - // Identifier: `ToAccountRole|"account-role"|COPY|OnFuture|InvalidTarget|InSchema|`, - // Error: "TODO", - // }, + { + Name: "validation: OnFuture in database - missing schema identifier", + Identifier: `ToAccountRole|"account-role"|COPY|OnFuture|InvalidTarget|InSchema|`, + Error: "incompatible identifier: ", + }, } for _, tt := range testCases { diff --git a/pkg/resources/grant_ownership_test.go b/pkg/resources/grant_ownership_test.go index 90938a0a14..b1b091a82a 100644 --- a/pkg/resources/grant_ownership_test.go +++ b/pkg/resources/grant_ownership_test.go @@ -3,6 +3,8 @@ package resources import ( "testing" + "github.com/stretchr/testify/require" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/stretchr/testify/assert" @@ -62,7 +64,7 @@ func TestGetOnObjectIdentifier(t *testing.T) { Name: "validation - valid identifier", ObjectType: sdk.ObjectTypeSchema, ObjectName: "to.many.parts.in.this.identifier", - Error: "unexpected number of parts 6 in identifier to.many.parts.in.this.identifier, expected 2 in a form of \".\"", + Expected: sdk.NewAccountObjectIdentifier("to.many.parts.in.this.identifier"), }, { Name: "validation - unsupported type", @@ -399,6 +401,7 @@ func TestValidAccountRoleNameGetOwnershipGrantTo(t *testing.T) { Name string AccountRole *string Expected sdk.OwnershipGrantTo + Error string }{ { Name: "account role name", @@ -421,16 +424,27 @@ func TestValidAccountRoleNameGetOwnershipGrantTo(t *testing.T) { AccountRoleName: sdk.Pointer(sdk.NewAccountObjectIdentifier("account.role.with.dots")), }, }, + { + Name: "account role name - with dots quoted", + AccountRole: sdk.String("\"account.role.with.dots\""), + Expected: sdk.OwnershipGrantTo{ + AccountRoleName: sdk.Pointer(sdk.NewAccountObjectIdentifier("account.role.with.dots")), + }, + }, } for _, tt := range testCases { tt := tt t.Run(tt.Name, func(t *testing.T) { - grantTo := getOwnershipGrantTo(schema.TestResourceDataRaw(t, grantOwnershipSchema, map[string]any{ + grantTo, err := getOwnershipGrantTo(schema.TestResourceDataRaw(t, grantOwnershipSchema, map[string]any{ "account_role_name": *tt.AccountRole, })) - - assert.Equal(t, *tt.Expected.AccountRoleName, *grantTo.AccountRoleName) + if tt.Error != "" { + require.ErrorContains(t, err, tt.Error) + } else { + require.NoError(t, err) + assert.Equal(t, *tt.Expected.AccountRoleName, *grantTo.AccountRoleName) + } }) } } @@ -460,9 +474,10 @@ func TestValidDatabaseRoleNameGetOwnershipGrantTo(t *testing.T) { for _, tt := range testCases { tt := tt t.Run(tt.Name, func(t *testing.T) { - grantTo := getOwnershipGrantTo(schema.TestResourceDataRaw(t, grantOwnershipSchema, map[string]any{ + grantTo, err := getOwnershipGrantTo(schema.TestResourceDataRaw(t, grantOwnershipSchema, map[string]any{ "database_role_name": *tt.DatabaseRole, })) + require.NoError(t, err) assert.Equal(t, *tt.Expected.DatabaseRoleName, *grantTo.DatabaseRoleName) }) @@ -474,9 +489,8 @@ func TestInvalidDatabaseRoleGetOwnershipGrantTo(t *testing.T) { "database_role_name": "account_role_name", }) - assert.Panics(t, func() { - _ = getOwnershipGrantTo(d) - }) + _, err := getOwnershipGrantTo(d) + require.Error(t, err) } func TestGetOwnershipGrantOpts(t *testing.T) { diff --git a/pkg/resources/grant_privileges_identifier_commons.go b/pkg/resources/grant_privileges_identifier_commons.go index 81779c08d9..cd203e2222 100644 --- a/pkg/resources/grant_privileges_identifier_commons.go +++ b/pkg/resources/grant_privileges_identifier_commons.go @@ -105,18 +105,26 @@ func getBulkOperationGrantData(in *sdk.GrantOnSchemaObjectIn) *BulkOperationGran return bulkOperationGrantData } -func getGrantOnSchemaObjectIn(allOrFuture map[string]any) *sdk.GrantOnSchemaObjectIn { +func getGrantOnSchemaObjectIn(allOrFuture map[string]any) (*sdk.GrantOnSchemaObjectIn, error) { grantOnSchemaObjectIn := &sdk.GrantOnSchemaObjectIn{ PluralObjectType: sdk.PluralObjectType(strings.ToUpper(allOrFuture["object_type_plural"].(string))), } if inDatabase, ok := allOrFuture["in_database"].(string); ok && len(inDatabase) > 0 { - grantOnSchemaObjectIn.InDatabase = sdk.Pointer(sdk.NewAccountObjectIdentifierFromFullyQualifiedName(inDatabase)) + databaseId, err := sdk.ParseAccountObjectIdentifier(inDatabase) + if err != nil { + return nil, err + } + grantOnSchemaObjectIn.InDatabase = sdk.Pointer(databaseId) } if inSchema, ok := allOrFuture["in_schema"].(string); ok && len(inSchema) > 0 { - grantOnSchemaObjectIn.InSchema = sdk.Pointer(sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(inSchema)) + schemaId, err := sdk.ParseDatabaseObjectIdentifier(inSchema) + if err != nil { + return nil, err + } + grantOnSchemaObjectIn.InSchema = sdk.Pointer(schemaId) } - return grantOnSchemaObjectIn + return grantOnSchemaObjectIn, nil } diff --git a/pkg/resources/grant_privileges_to_account_role.go b/pkg/resources/grant_privileges_to_account_role.go index d08207c9fb..e314e067b0 100644 --- a/pkg/resources/grant_privileges_to_account_role.go +++ b/pkg/resources/grant_privileges_to_account_role.go @@ -25,6 +25,7 @@ var grantPrivilegesToAccountRoleSchema = map[string]*schema.Schema{ ForceNew: true, Description: "The fully qualified name of the account role to which privileges will be granted.", ValidateDiagFunc: IsValidIdentifier[sdk.AccountObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, }, // According to docs https://docs.snowflake.com/en/user-guide/data-exchange-marketplace-privileges#usage-notes IMPORTED PRIVILEGES // will be returned as USAGE in SHOW GRANTS command. In addition, USAGE itself is a valid privilege, but both cannot be set at the @@ -122,6 +123,7 @@ var grantPrivilegesToAccountRoleSchema = map[string]*schema.Schema{ ForceNew: true, Description: "The fully qualified name of the object on which privileges will be granted.", ValidateDiagFunc: IsValidIdentifier[sdk.AccountObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, }, }, }, @@ -146,6 +148,7 @@ var grantPrivilegesToAccountRoleSchema = map[string]*schema.Schema{ ForceNew: true, Description: "The fully qualified name of the schema.", ValidateDiagFunc: IsValidIdentifier[sdk.DatabaseObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, ExactlyOneOf: []string{ "on_schema.0.schema_name", "on_schema.0.all_schemas_in_database", @@ -158,6 +161,7 @@ var grantPrivilegesToAccountRoleSchema = map[string]*schema.Schema{ ForceNew: true, Description: "The fully qualified name of the database.", ValidateDiagFunc: IsValidIdentifier[sdk.AccountObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, ExactlyOneOf: []string{ "on_schema.0.schema_name", "on_schema.0.all_schemas_in_database", @@ -170,6 +174,7 @@ var grantPrivilegesToAccountRoleSchema = map[string]*schema.Schema{ ForceNew: true, Description: "The fully qualified name of the database.", ValidateDiagFunc: IsValidIdentifier[sdk.AccountObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, ExactlyOneOf: []string{ "on_schema.0.schema_name", "on_schema.0.all_schemas_in_database", @@ -220,6 +225,7 @@ var grantPrivilegesToAccountRoleSchema = map[string]*schema.Schema{ "on_schema_object.0.all", "on_schema_object.0.future", }, + DiffSuppressFunc: suppressIdentifierQuoting, }, "all": { Type: schema.TypeList, @@ -276,12 +282,14 @@ func getGrantPrivilegesOnAccountRoleBulkOperationSchema(validGrantToObjectTypes Optional: true, ForceNew: true, ValidateDiagFunc: IsValidIdentifier[sdk.AccountObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, }, "in_schema": { Type: schema.TypeString, Optional: true, ForceNew: true, ValidateDiagFunc: IsValidIdentifier[sdk.DatabaseObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, }, } } @@ -414,6 +422,7 @@ func CreateGrantPrivilegesToAccountRole(ctx context.Context, d *schema.ResourceD if err != nil { return diag.FromErr(err) } + err = client.Grants.GrantPrivilegesToAccountRole( ctx, getAccountRolePrivilegesFromSchema(d), @@ -462,13 +471,13 @@ func UpdateGrantPrivilegesToAccountRole(ctx context.Context, d *schema.ResourceD // handle all_privileges -> privileges change (revoke all privileges) if d.HasChange("all_privileges") { _, allPrivileges := d.GetChange("all_privileges") - grantOn, err := getAccountRoleGrantOn(d) - if err != nil { - return diag.FromErr(err) - } if !allPrivileges.(bool) { logging.DebugLogger.Printf("[DEBUG] Revoking all privileges") + grantOn, err := getAccountRoleGrantOn(d) + if err != nil { + return diag.FromErr(err) + } err = client.Grants.RevokePrivilegesFromAccountRole(ctx, &sdk.AccountRoleGrantPrivileges{ AllPrivileges: sdk.Bool(true), }, @@ -1007,7 +1016,10 @@ func getAccountRoleGrantOn(d *schema.ResourceData) (*sdk.AccountRoleGrantOn, err objectType := onAccountObject["object_type"].(string) objectName := onAccountObject["object_name"].(string) - objectIdentifier := sdk.NewAccountObjectIdentifierFromFullyQualifiedName(objectName) + objectIdentifier, err := sdk.ParseAccountObjectIdentifier(objectName) + if err != nil { + return nil, err + } switch sdk.ObjectType(objectType) { case sdk.ObjectTypeDatabase: @@ -1047,11 +1059,23 @@ func getAccountRoleGrantOn(d *schema.ResourceData) (*sdk.AccountRoleGrantOn, err switch { case schemaNameOk: - grantOnSchema.Schema = sdk.Pointer(sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(schemaName)) + schemaId, err := sdk.ParseDatabaseObjectIdentifier(schemaName) + if err != nil { + return nil, err + } + grantOnSchema.Schema = sdk.Pointer(schemaId) case allSchemasInDatabaseOk: - grantOnSchema.AllSchemasInDatabase = sdk.Pointer(sdk.NewAccountObjectIdentifierFromFullyQualifiedName(allSchemasInDatabase)) + databaseId, err := sdk.ParseAccountObjectIdentifier(allSchemasInDatabase) + if err != nil { + return nil, err + } + grantOnSchema.AllSchemasInDatabase = sdk.Pointer(databaseId) case futureSchemasInDatabaseOk: - grantOnSchema.FutureSchemasInDatabase = sdk.Pointer(sdk.NewAccountObjectIdentifierFromFullyQualifiedName(futureSchemasInDatabase)) + databaseId, err := sdk.ParseAccountObjectIdentifier(futureSchemasInDatabase) + if err != nil { + return nil, err + } + grantOnSchema.FutureSchemasInDatabase = sdk.Pointer(databaseId) } on.Schema = grantOnSchema @@ -1076,24 +1100,35 @@ func getAccountRoleGrantOn(d *schema.ResourceData) (*sdk.AccountRoleGrantOn, err case objectTypeOk && objectNameOk: objectType := sdk.ObjectType(objectType) var id sdk.ObjectIdentifier + var err error // TODO(SNOW-1569535): use a mapper from object type to parsing function if objectType.IsWithArguments() { - var err error id, err = sdk.ParseSchemaObjectIdentifierWithArguments(objectName) if err != nil { return nil, err } } else { - id = sdk.NewSchemaObjectIdentifierFromFullyQualifiedName(objectName) + id, err = sdk.ParseSchemaObjectIdentifier(objectName) + if err != nil { + return nil, err + } } grantOnSchemaObject.SchemaObject = &sdk.Object{ ObjectType: objectType, Name: id, } case allOk: - grantOnSchemaObject.All = getGrantOnSchemaObjectIn(all[0].(map[string]any)) + grantOnSchemaObjectIn, err := getGrantOnSchemaObjectIn(all[0].(map[string]any)) + if err != nil { + return nil, err + } + grantOnSchemaObject.All = grantOnSchemaObjectIn case futureOk: - grantOnSchemaObject.Future = getGrantOnSchemaObjectIn(future[0].(map[string]any)) + grantOnSchemaObjectIn, err := getGrantOnSchemaObjectIn(future[0].(map[string]any)) + if err != nil { + return nil, err + } + grantOnSchemaObject.Future = grantOnSchemaObjectIn } on.SchemaObject = grantOnSchemaObject @@ -1102,9 +1137,12 @@ func getAccountRoleGrantOn(d *schema.ResourceData) (*sdk.AccountRoleGrantOn, err return on, nil } -func createGrantPrivilegesToAccountRoleIdFromSchema(d *schema.ResourceData) (*GrantPrivilegesToAccountRoleId, error) { - id := new(GrantPrivilegesToAccountRoleId) - id.RoleName = sdk.NewAccountObjectIdentifierFromFullyQualifiedName(d.Get("account_role_name").(string)) +func createGrantPrivilegesToAccountRoleIdFromSchema(d *schema.ResourceData) (id *GrantPrivilegesToAccountRoleId, err error) { + id = new(GrantPrivilegesToAccountRoleId) + id.RoleName, err = sdk.ParseAccountObjectIdentifier(d.Get("account_role_name").(string)) + if err != nil { + return nil, err + } id.AllPrivileges = d.Get("all_privileges").(bool) if p, ok := d.GetOk("privileges"); ok { id.Privileges = expandStringList(p.(*schema.Set).List()) diff --git a/pkg/resources/grant_privileges_to_account_role_acceptance_test.go b/pkg/resources/grant_privileges_to_account_role_acceptance_test.go index 6386860639..eeb05eb77e 100644 --- a/pkg/resources/grant_privileges_to_account_role_acceptance_test.go +++ b/pkg/resources/grant_privileges_to_account_role_acceptance_test.go @@ -155,19 +155,15 @@ func TestAcc_GrantPrivilegesToAccountRole_OnAccountObject(t *testing.T) { resource.TestCheckResourceAttr(resourceName, "on_account_object.#", "1"), resource.TestCheckResourceAttr(resourceName, "on_account_object.0.object_type", "DATABASE"), resource.TestCheckResourceAttr(resourceName, "on_account_object.0.object_name", databaseName), - // TODO (SNOW-999049): Even if the identifier is passed as a non-escaped value it will be escaped in the identifier and later on in the CRUD operations (right now, it's "only" read, which can cause behavior similar to always_apply) - resource.TestCheckResourceAttr(resourceName, "id", fmt.Sprintf("%s|true|false|CREATE DATABASE ROLE,CREATE SCHEMA|OnAccountObject|DATABASE|\"%s\"", roleFullyQualifiedName, acc.TestDatabaseName)), + resource.TestCheckResourceAttr(resourceName, "id", fmt.Sprintf("%s|true|false|CREATE DATABASE ROLE,CREATE SCHEMA|OnAccountObject|DATABASE|%s", roleFullyQualifiedName, acc.TestClient().Ids.DatabaseId().FullyQualifiedName())), ), }, { - // TODO (SNOW-999049): this fails, because after import object_name identifier is escaped (was unescaped) - // Make grant_privileges_to_account_role and grant_privileges_to_account_role identifiers accept - // quoted and unquoted identifiers. - // ConfigPlanChecks: resource.ConfigPlanChecks{ - // PostApplyPostRefresh: []plancheck.PlanCheck{ - // plancheck.ExpectEmptyPlan(), - // }, - // }, + ConfigPlanChecks: resource.ConfigPlanChecks{ + PostApplyPostRefresh: []plancheck.PlanCheck{ + plancheck.ExpectEmptyPlan(), + }, + }, ConfigDirectory: acc.ConfigurationDirectory("TestAcc_GrantPrivilegesToAccountRole/OnAccountObject"), ConfigVariables: configVariables, ResourceName: resourceName, @@ -1716,3 +1712,111 @@ func createExternalVolume(t *testing.T, externalVolumeName string) func() { require.NoError(t, err) } } + +func TestAcc_GrantPrivilegesToAccountRole_migrateFromV0941_ensureSmoothUpgradeWithNewResourceId(t *testing.T) { + accountRoleId := acc.TestClient().Ids.RandomAccountObjectIdentifier() + schemaId := acc.TestClient().Ids.SchemaId() + quotedSchemaId := fmt.Sprintf(`\"%s\".\"%s\"`, schemaId.DatabaseName(), schemaId.Name()) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { acc.TestAccPreCheck(t) }, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{ + "snowflake": { + VersionConstraint: "=0.94.1", + Source: "Snowflake-Labs/snowflake", + }, + }, + Config: grantPrivilegesToAccountRoleBasicConfig(accountRoleId.Name(), accountRoleId.Name(), quotedSchemaId), + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_grant_privileges_to_account_role.test", "id", fmt.Sprintf("%s|false|false|USAGE|OnSchema|OnSchema|%s", accountRoleId.FullyQualifiedName(), schemaId.FullyQualifiedName())), + ), + }, + { + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + Config: grantPrivilegesToAccountRoleBasicConfig(accountRoleId.Name(), accountRoleId.Name(), quotedSchemaId), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_grant_privileges_to_account_role.test", plancheck.ResourceActionNoop), + }, + PostApplyPostRefresh: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_grant_privileges_to_account_role.test", plancheck.ResourceActionNoop), + }, + }, + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_grant_privileges_to_account_role.test", "id", fmt.Sprintf("%s|false|false|USAGE|OnSchema|OnSchema|%s", accountRoleId.FullyQualifiedName(), schemaId.FullyQualifiedName())), + ), + }, + }, + }) +} + +func grantPrivilegesToAccountRoleBasicConfig(accountRoleName string, grantedAccountRoleName string, fullyQualifiedSchemaName string) string { + return fmt.Sprintf(` +resource "snowflake_account_role" "test" { + name = "%[1]s" +} + +resource "snowflake_grant_privileges_to_account_role" "test" { + depends_on = [ snowflake_account_role.test ] + account_role_name = "%[2]s" + privileges = ["USAGE"] + + on_schema { + schema_name = "%[3]s" + } +} +`, accountRoleName, grantedAccountRoleName, fullyQualifiedSchemaName) +} + +func TestAcc_GrantPrivilegesToAccountRole_IdentifierQuotingDiffSuppression(t *testing.T) { + accountRoleId := acc.TestClient().Ids.RandomAccountObjectIdentifier() + unquotedAccountRoleId := accountRoleId.Name() + + schemaId := acc.TestClient().Ids.SchemaId() + unquotedSchemaId := fmt.Sprintf(`%s.%s`, schemaId.DatabaseName(), schemaId.Name()) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { acc.TestAccPreCheck(t) }, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{ + "snowflake": { + VersionConstraint: "=0.94.1", + Source: "Snowflake-Labs/snowflake", + }, + }, + Config: grantPrivilegesToAccountRoleBasicConfig(accountRoleId.Name(), unquotedAccountRoleId, unquotedSchemaId), + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_grant_privileges_to_account_role.test", "account_role_name", unquotedAccountRoleId), + resource.TestCheckResourceAttr("snowflake_grant_privileges_to_account_role.test", "on_schema.0.schema_name", unquotedSchemaId), + resource.TestCheckResourceAttr("snowflake_grant_privileges_to_account_role.test", "id", fmt.Sprintf("%s|false|false|USAGE|OnSchema|OnSchema|%s", accountRoleId.FullyQualifiedName(), schemaId.FullyQualifiedName())), + ), + }, + { + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + Config: grantPrivilegesToAccountRoleBasicConfig(accountRoleId.Name(), unquotedAccountRoleId, unquotedSchemaId), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_grant_privileges_to_account_role.test", plancheck.ResourceActionNoop), + }, + PostApplyPostRefresh: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_grant_privileges_to_account_role.test", plancheck.ResourceActionNoop), + }, + }, + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_grant_privileges_to_account_role.test", "account_role_name", unquotedAccountRoleId), + resource.TestCheckResourceAttr("snowflake_grant_privileges_to_account_role.test", "on_schema.0.schema_name", unquotedSchemaId), + resource.TestCheckResourceAttr("snowflake_grant_privileges_to_account_role.test", "id", fmt.Sprintf("%s|false|false|USAGE|OnSchema|OnSchema|%s", accountRoleId.FullyQualifiedName(), schemaId.FullyQualifiedName())), + ), + }, + }, + }) +} diff --git a/pkg/resources/grant_privileges_to_account_role_identifier.go b/pkg/resources/grant_privileges_to_account_role_identifier.go index 4f6f45ce4b..2eb12831a3 100644 --- a/pkg/resources/grant_privileges_to_account_role_identifier.go +++ b/pkg/resources/grant_privileges_to_account_role_identifier.go @@ -69,13 +69,11 @@ func ParseGrantPrivilegesToAccountRoleId(id string) (GrantPrivilegesToAccountRol return accountRoleId, sdk.NewError(`account role identifier should hold at least 5 parts "||||"`) } - // TODO: Identifier parsing should be replaced with better version introduced in SNOW-999049. - // Right now, it's same as sdk.NewAccountObjectIdentifierFromFullyQualifiedName, but with error handling. - name := strings.Trim(parts[0], `"`) - if len(name) == 0 { - return accountRoleId, sdk.NewError(fmt.Sprintf(`invalid (empty) AccountRoleName value: %s, should be a fully qualified name of account object `, parts[0])) + roleId, err := sdk.ParseAccountObjectIdentifier(parts[0]) + if err != nil { + return accountRoleId, err } - accountRoleId.RoleName = sdk.NewAccountObjectIdentifier(name) + accountRoleId.RoleName = roleId if parts[1] != "false" && parts[1] != "true" { return accountRoleId, sdk.NewError(fmt.Sprintf(`invalid WithGrantOption value: %s, should be either "true" or "false"`, parts[1])) @@ -105,9 +103,13 @@ func ParseGrantPrivilegesToAccountRoleId(id string) (GrantPrivilegesToAccountRol if len(parts) != 7 { return accountRoleId, sdk.NewError(`account role identifier should hold at least 7 parts "||||OnAccountObject||"`) } + objectId, err := sdk.ParseAccountObjectIdentifier(parts[6]) + if err != nil { + return accountRoleId, err + } accountRoleId.Data = &OnAccountObjectGrantData{ ObjectType: sdk.ObjectType(parts[5]), - ObjectName: sdk.NewAccountObjectIdentifierFromFullyQualifiedName(parts[6]), + ObjectName: objectId, } case OnSchemaAccountRoleGrantKind: if len(parts) < 7 { @@ -118,9 +120,17 @@ func ParseGrantPrivilegesToAccountRoleId(id string) (GrantPrivilegesToAccountRol } switch onSchemaGrantData.Kind { case OnSchemaSchemaGrantKind: - onSchemaGrantData.SchemaName = sdk.Pointer(sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(parts[6])) + schemaId, err := sdk.ParseDatabaseObjectIdentifier(parts[6]) + if err != nil { + return accountRoleId, err + } + onSchemaGrantData.SchemaName = sdk.Pointer(schemaId) case OnAllSchemasInDatabaseSchemaGrantKind, OnFutureSchemasInDatabaseSchemaGrantKind: - onSchemaGrantData.DatabaseName = sdk.Pointer(sdk.NewAccountObjectIdentifier(parts[6])) + databaseId, err := sdk.ParseAccountObjectIdentifier(parts[6]) + if err != nil { + return accountRoleId, err + } + onSchemaGrantData.DatabaseName = sdk.Pointer(databaseId) default: return accountRoleId, sdk.NewError(fmt.Sprintf("invalid OnSchemaGrantKind: %s", onSchemaGrantData.Kind)) } @@ -141,13 +151,15 @@ func ParseGrantPrivilegesToAccountRoleId(id string) (GrantPrivilegesToAccountRol var id sdk.ObjectIdentifier // TODO(SNOW-1569535): use a mapper from object type to parsing function if objectType.IsWithArguments() { - var err error id, err = sdk.ParseSchemaObjectIdentifierWithArguments(parts[7]) if err != nil { return accountRoleId, err } } else { - id = sdk.NewSchemaObjectIdentifierFromFullyQualifiedName(parts[7]) + id, err = sdk.ParseSchemaObjectIdentifier(parts[7]) + if err != nil { + return accountRoleId, err + } } onSchemaObjectGrantData.Object = &sdk.Object{ ObjectType: objectType, @@ -164,9 +176,17 @@ func ParseGrantPrivilegesToAccountRoleId(id string) (GrantPrivilegesToAccountRol bulkOperationGrantData.Kind = BulkOperationGrantKind(parts[7]) switch bulkOperationGrantData.Kind { case InDatabaseBulkOperationGrantKind: - bulkOperationGrantData.Database = sdk.Pointer(sdk.NewAccountObjectIdentifierFromFullyQualifiedName(parts[8])) + databaseId, err := sdk.ParseAccountObjectIdentifier(parts[8]) + if err != nil { + return accountRoleId, err + } + bulkOperationGrantData.Database = sdk.Pointer(databaseId) case InSchemaBulkOperationGrantKind: - bulkOperationGrantData.Schema = sdk.Pointer(sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(parts[8])) + schemaId, err := sdk.ParseDatabaseObjectIdentifier(parts[8]) + if err != nil { + return accountRoleId, err + } + bulkOperationGrantData.Schema = sdk.Pointer(schemaId) default: return accountRoleId, sdk.NewError(fmt.Sprintf("invalid BulkOperationGrantKind: %s", bulkOperationGrantData.Kind)) } diff --git a/pkg/resources/grant_privileges_to_account_role_identifier_test.go b/pkg/resources/grant_privileges_to_account_role_identifier_test.go index 8a047021d6..3f2c059096 100644 --- a/pkg/resources/grant_privileges_to_account_role_identifier_test.go +++ b/pkg/resources/grant_privileges_to_account_role_identifier_test.go @@ -25,6 +25,17 @@ func TestParseGrantPrivilegesToAccountRoleId(t *testing.T) { Data: new(OnAccountGrantData), }, }, + { + Name: "grant account role on account - empty role name", + Identifier: `|false|false|CREATE DATABASE,CREATE USER|OnAccount`, + Expected: GrantPrivilegesToAccountRoleId{ + RoleName: sdk.NewAccountObjectIdentifier(""), + WithGrantOption: false, + Privileges: []string{"CREATE DATABASE", "CREATE USER"}, + Kind: OnAccountAccountRoleGrantKind, + Data: new(OnAccountGrantData), + }, + }, { Name: "grant account role on account - always apply with grant option", Identifier: `"account-role"|true|true|CREATE DATABASE,CREATE USER|OnAccount`, @@ -267,42 +278,42 @@ func TestParseGrantPrivilegesToAccountRoleId(t *testing.T) { }, { Name: "validation: grant account role not enough parts for OnAccountObject kind", - Identifier: `"database-name"."role-name"|false|false|CREATE SCHEMA,USAGE,MONITOR|OnAccountObject`, + Identifier: `"role-name"|false|false|CREATE SCHEMA,USAGE,MONITOR|OnAccountObject`, Error: `account role identifier should hold at least 7 parts "||||OnAccountObject||"`, }, { Name: "validation: grant account role not enough parts for OnSchema kind", - Identifier: `"database-name"."role-name"|false|false|CREATE SCHEMA,USAGE,MONITOR|OnSchema|OnAllSchemasInDatabase`, + Identifier: `"role-name"|false|false|CREATE SCHEMA,USAGE,MONITOR|OnSchema|OnAllSchemasInDatabase`, Error: "account role identifier should hold at least 7 parts", }, { Name: "validation: grant account role not enough parts for OnSchemaObject kind", - Identifier: `"database-name"."role-name"|false|false|CREATE SCHEMA,USAGE,MONITOR|OnSchemaObject|OnObject`, + Identifier: `"role-name"|false|false|CREATE SCHEMA,USAGE,MONITOR|OnSchemaObject|OnObject`, Error: "account role identifier should hold at least 7 parts", }, { Name: "validation: grant account role not enough parts for OnSchemaObject kind", - Identifier: `"database-name"."role-name"|false|false|CREATE SCHEMA,USAGE,MONITOR|OnSchemaObject|OnObject|TABLE`, + Identifier: `"role-name"|false|false|CREATE SCHEMA,USAGE,MONITOR|OnSchemaObject|OnObject|TABLE`, Error: "account role identifier should hold 8 parts", }, { Name: "validation: grant account role not enough parts for OnSchemaObject.InDatabase kind", - Identifier: `"database-name"."role-name"|false|false|CREATE SCHEMA,USAGE,MONITOR|OnSchemaObject|OnAll|TABLES|InDatabase`, + Identifier: `"role-name"|false|false|CREATE SCHEMA,USAGE,MONITOR|OnSchemaObject|OnAll|TABLES|InDatabase`, Error: "account role identifier should hold 9 parts", }, { Name: "validation: grant account role invalid AccountRoleGrantKind kind", - Identifier: `"database-name"."role-name"|false|false|CREATE SCHEMA,USAGE,MONITOR|some-kind|some-data`, + Identifier: `"role-name"|false|false|CREATE SCHEMA,USAGE,MONITOR|some-kind|some-data`, Error: "invalid AccountRoleGrantKind: some-kind", }, { Name: "validation: grant account role invalid OnSchemaGrantKind kind", - Identifier: `"database-name"."role-name"|false|false|CREATE SCHEMA,USAGE,MONITOR|OnSchema|some-kind|some-data`, + Identifier: `"role-name"|false|false|CREATE SCHEMA,USAGE,MONITOR|OnSchema|some-kind|some-data`, Error: "invalid OnSchemaGrantKind: some-kind", }, { Name: "validation: grant account role invalid OnSchemaObjectGrantKind kind", - Identifier: `"database-name"."role-name"|false|false|CREATE SCHEMA,USAGE,MONITOR|OnSchemaObject|some-kind|some-data`, + Identifier: `"role-name"|false|false|CREATE SCHEMA,USAGE,MONITOR|OnSchemaObject|some-kind|some-data`, Error: "invalid OnSchemaObjectGrantKind: some-kind", }, { @@ -320,11 +331,6 @@ func TestParseGrantPrivilegesToAccountRoleId(t *testing.T) { Identifier: `"account-role"|false||ALL PRIVILEGES|OnAccount`, Error: `invalid AlwaysApply value: , should be either "true" or "false"`, }, - { - Name: "validation: grant account role empty role name", - Identifier: `|false|false|ALL PRIVILEGES|OnAccount`, - Error: "invalid (empty) AccountRoleName value: , should be a fully qualified name of account object ", - }, { Name: "validation: account role empty type", Identifier: `"account-role"|false|false|ALL PRIVILEGES||"on-database-name"`, diff --git a/pkg/resources/grant_privileges_to_database_role.go b/pkg/resources/grant_privileges_to_database_role.go index 411f127e3f..b69b089a97 100644 --- a/pkg/resources/grant_privileges_to_database_role.go +++ b/pkg/resources/grant_privileges_to_database_role.go @@ -25,6 +25,7 @@ var grantPrivilegesToDatabaseRoleSchema = map[string]*schema.Schema{ ForceNew: true, Description: "The fully qualified name of the database role to which privileges will be granted.", ValidateDiagFunc: IsValidIdentifier[sdk.DatabaseObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, }, "privileges": { Type: schema.TypeSet, @@ -74,6 +75,7 @@ var grantPrivilegesToDatabaseRoleSchema = map[string]*schema.Schema{ ForceNew: true, Description: "The fully qualified name of the database on which privileges will be granted.", ValidateDiagFunc: IsValidIdentifier[sdk.AccountObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, ExactlyOneOf: []string{ "on_database", "on_schema", @@ -99,6 +101,7 @@ var grantPrivilegesToDatabaseRoleSchema = map[string]*schema.Schema{ ForceNew: true, Description: "The fully qualified name of the schema.", ValidateDiagFunc: IsValidIdentifier[sdk.DatabaseObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, ExactlyOneOf: []string{ "on_schema.0.schema_name", "on_schema.0.all_schemas_in_database", @@ -111,6 +114,7 @@ var grantPrivilegesToDatabaseRoleSchema = map[string]*schema.Schema{ ForceNew: true, Description: "The fully qualified name of the database.", ValidateDiagFunc: IsValidIdentifier[sdk.AccountObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, ExactlyOneOf: []string{ "on_schema.0.schema_name", "on_schema.0.all_schemas_in_database", @@ -123,6 +127,7 @@ var grantPrivilegesToDatabaseRoleSchema = map[string]*schema.Schema{ ForceNew: true, Description: "The fully qualified name of the database.", ValidateDiagFunc: IsValidIdentifier[sdk.AccountObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, ExactlyOneOf: []string{ "on_schema.0.schema_name", "on_schema.0.all_schemas_in_database", @@ -172,6 +177,7 @@ var grantPrivilegesToDatabaseRoleSchema = map[string]*schema.Schema{ "on_schema_object.0.all", "on_schema_object.0.future", }, + DiffSuppressFunc: suppressIdentifierQuoting, }, "all": { Type: schema.TypeList, @@ -229,6 +235,7 @@ func getGrantPrivilegesOnDatabaseRoleBulkOperationSchema(validGrantToObjectTypes ForceNew: true, Description: "The fully qualified name of the database.", ValidateDiagFunc: IsValidIdentifier[sdk.AccountObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, }, "in_schema": { Type: schema.TypeString, @@ -236,6 +243,7 @@ func getGrantPrivilegesOnDatabaseRoleBulkOperationSchema(validGrantToObjectTypes ForceNew: true, Description: "The fully qualified name of the schema.", ValidateDiagFunc: IsValidIdentifier[sdk.DatabaseObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, }, } } @@ -345,6 +353,7 @@ func CreateGrantPrivilegesToDatabaseRole(ctx context.Context, d *schema.Resource if err != nil { return diag.FromErr(err) } + grantOn, err := getDatabaseRoleGrantOn(d) if err != nil { return diag.FromErr(err) @@ -391,16 +400,15 @@ func UpdateGrantPrivilegesToDatabaseRole(ctx context.Context, d *schema.Resource id.WithGrantOption = d.Get("with_grant_option").(bool) } - grantOn, err := getDatabaseRoleGrantOn(d) - if err != nil { - return diag.FromErr(err) - } - // handle all_privileges -> privileges change (revoke all privileges) if d.HasChange("all_privileges") { _, allPrivileges := d.GetChange("all_privileges") if !allPrivileges.(bool) { + grantOn, err := getDatabaseRoleGrantOn(d) + if err != nil { + return diag.FromErr(err) + } err = client.Grants.RevokePrivilegesFromDatabaseRole(ctx, &sdk.DatabaseRoleGrantPrivileges{ AllPrivileges: sdk.Bool(true), }, @@ -452,6 +460,11 @@ func UpdateGrantPrivilegesToDatabaseRole(ctx context.Context, d *schema.Resource } } + grantOn, err := getDatabaseRoleGrantOn(d) + if err != nil { + return diag.FromErr(err) + } + if len(privilegesToAdd) > 0 { privilegesToGrant := getDatabaseRolePrivileges( false, @@ -521,6 +534,10 @@ func UpdateGrantPrivilegesToDatabaseRole(ctx context.Context, d *schema.Resource _, allPrivileges := d.GetChange("all_privileges") if allPrivileges.(bool) { + grantOn, err := getDatabaseRoleGrantOn(d) + if err != nil { + return diag.FromErr(err) + } err = client.Grants.GrantPrivilegesToDatabaseRole(ctx, &sdk.DatabaseRoleGrantPrivileges{ AllPrivileges: sdk.Bool(true), }, @@ -547,6 +564,10 @@ func UpdateGrantPrivilegesToDatabaseRole(ctx context.Context, d *schema.Resource } if id.AlwaysApply { + grantOn, err := getDatabaseRoleGrantOn(d) + if err != nil { + return diag.FromErr(err) + } err = client.Grants.GrantPrivilegesToDatabaseRole( ctx, getDatabaseRolePrivilegesFromSchema(d), @@ -584,6 +605,7 @@ func DeleteGrantPrivilegesToDatabaseRole(ctx context.Context, d *schema.Resource }, } } + grantOn, err := getDatabaseRoleGrantOn(d) if err != nil { return diag.FromErr(err) @@ -859,7 +881,11 @@ func getDatabaseRoleGrantOn(d *schema.ResourceData) (*sdk.DatabaseRoleGrantOn, e switch { case onDatabaseOk: - on.Database = sdk.Pointer(sdk.NewAccountObjectIdentifierFromFullyQualifiedName(onDatabase.(string))) + databaseId, err := sdk.ParseAccountObjectIdentifier(onDatabase.(string)) + if err != nil { + return nil, err + } + on.Database = sdk.Pointer(databaseId) case onSchemaOk: onSchema := onSchemaBlock.([]any)[0].(map[string]any) @@ -876,11 +902,23 @@ func getDatabaseRoleGrantOn(d *schema.ResourceData) (*sdk.DatabaseRoleGrantOn, e switch { case schemaNameOk: - grantOnSchema.Schema = sdk.Pointer(sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(schemaName)) + schemaId, err := sdk.ParseDatabaseObjectIdentifier(schemaName) + if err != nil { + return nil, err + } + grantOnSchema.Schema = sdk.Pointer(schemaId) case allSchemasInDatabaseOk: - grantOnSchema.AllSchemasInDatabase = sdk.Pointer(sdk.NewAccountObjectIdentifierFromFullyQualifiedName(allSchemasInDatabase)) + databaseId, err := sdk.ParseAccountObjectIdentifier(allSchemasInDatabase) + if err != nil { + return nil, err + } + grantOnSchema.AllSchemasInDatabase = sdk.Pointer(databaseId) case futureSchemasInDatabaseOk: - grantOnSchema.FutureSchemasInDatabase = sdk.Pointer(sdk.NewAccountObjectIdentifierFromFullyQualifiedName(futureSchemasInDatabase)) + databaseId, err := sdk.ParseAccountObjectIdentifier(futureSchemasInDatabase) + if err != nil { + return nil, err + } + grantOnSchema.FutureSchemasInDatabase = sdk.Pointer(databaseId) } on.Schema = grantOnSchema @@ -905,24 +943,35 @@ func getDatabaseRoleGrantOn(d *schema.ResourceData) (*sdk.DatabaseRoleGrantOn, e case objectTypeOk && objectNameOk: objectType := sdk.ObjectType(objectType) var id sdk.ObjectIdentifier + var err error // TODO(SNOW-1569535): use a mapper from object type to parsing function if objectType.IsWithArguments() { - var err error id, err = sdk.ParseSchemaObjectIdentifierWithArguments(objectName) if err != nil { return nil, err } } else { - id = sdk.NewSchemaObjectIdentifierFromFullyQualifiedName(objectName) + id, err = sdk.ParseSchemaObjectIdentifier(objectName) + if err != nil { + return nil, err + } } grantOnSchemaObject.SchemaObject = &sdk.Object{ ObjectType: objectType, Name: id, } case allOk: - grantOnSchemaObject.All = getGrantOnSchemaObjectIn(all[0].(map[string]any)) + grantOnSchemaObjectIn, err := getGrantOnSchemaObjectIn(all[0].(map[string]any)) + if err != nil { + return nil, err + } + grantOnSchemaObject.All = grantOnSchemaObjectIn case futureOk: - grantOnSchemaObject.Future = getGrantOnSchemaObjectIn(future[0].(map[string]any)) + grantOnSchemaObjectIn, err := getGrantOnSchemaObjectIn(future[0].(map[string]any)) + if err != nil { + return nil, err + } + grantOnSchemaObject.Future = grantOnSchemaObjectIn } on.SchemaObject = grantOnSchemaObject @@ -931,9 +980,13 @@ func getDatabaseRoleGrantOn(d *schema.ResourceData) (*sdk.DatabaseRoleGrantOn, e return on, nil } -func createGrantPrivilegesToDatabaseRoleIdFromSchema(d *schema.ResourceData) (*GrantPrivilegesToDatabaseRoleId, error) { - id := new(GrantPrivilegesToDatabaseRoleId) - id.DatabaseRoleName = sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(d.Get("database_role_name").(string)) +func createGrantPrivilegesToDatabaseRoleIdFromSchema(d *schema.ResourceData) (id *GrantPrivilegesToDatabaseRoleId, err error) { + id = new(GrantPrivilegesToDatabaseRoleId) + roleId, err := sdk.ParseDatabaseObjectIdentifier(d.Get("database_role_name").(string)) + if err != nil { + return nil, err + } + id.DatabaseRoleName = roleId id.AllPrivileges = d.Get("all_privileges").(bool) if p, ok := d.GetOk("privileges"); ok { id.Privileges = expandStringList(p.(*schema.Set).List()) diff --git a/pkg/resources/grant_privileges_to_database_role_acceptance_test.go b/pkg/resources/grant_privileges_to_database_role_acceptance_test.go index b98d23578a..a272447b7e 100644 --- a/pkg/resources/grant_privileges_to_database_role_acceptance_test.go +++ b/pkg/resources/grant_privileges_to_database_role_acceptance_test.go @@ -1398,3 +1398,114 @@ func revokeAndGrantPrivilegesOnDatabaseToDatabaseRole( t.Fatal(err) } } + +func TestAcc_GrantPrivilegesToDatabaseRole_migrateFromV0941_ensureSmoothUpgradeWithNewResourceId(t *testing.T) { + databaseRoleId := acc.TestClient().Ids.RandomDatabaseObjectIdentifier() + quotedDatabaseRoleId := fmt.Sprintf(`\"%s\".\"%s\"`, databaseRoleId.DatabaseName(), databaseRoleId.Name()) + + schemaId := acc.TestClient().Ids.SchemaId() + quotedSchemaId := fmt.Sprintf(`\"%s\".\"%s\"`, schemaId.DatabaseName(), schemaId.Name()) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { acc.TestAccPreCheck(t) }, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{ + "snowflake": { + VersionConstraint: "=0.94.1", + Source: "Snowflake-Labs/snowflake", + }, + }, + Config: grantPrivilegesToDatabaseRoleBasicConfig(acc.TestClient().Ids.DatabaseId().Name(), databaseRoleId.Name(), quotedDatabaseRoleId, quotedSchemaId), + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_grant_privileges_to_database_role.test", "id", fmt.Sprintf("%s|false|false|USAGE|OnSchema|OnSchema|%s", databaseRoleId.FullyQualifiedName(), schemaId.FullyQualifiedName())), + ), + }, + { + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + Config: grantPrivilegesToDatabaseRoleBasicConfig(acc.TestClient().Ids.DatabaseId().Name(), databaseRoleId.Name(), quotedDatabaseRoleId, quotedSchemaId), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_grant_privileges_to_database_role.test", plancheck.ResourceActionNoop), + }, + PostApplyPostRefresh: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_grant_privileges_to_database_role.test", plancheck.ResourceActionNoop), + }, + }, + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_grant_privileges_to_database_role.test", "id", fmt.Sprintf("%s|false|false|USAGE|OnSchema|OnSchema|%s", databaseRoleId.FullyQualifiedName(), schemaId.FullyQualifiedName())), + ), + }, + }, + }) +} + +func grantPrivilegesToDatabaseRoleBasicConfig(databaseName string, databaseRoleName string, fullyQualifiedDatabaseRoleName string, fullyQualifiedSchemaName string) string { + return fmt.Sprintf(` +resource "snowflake_database_role" "test" { + name = "%[2]s" + database = "%[1]s" +} + +resource "snowflake_grant_privileges_to_database_role" "test" { + depends_on = [ snowflake_database_role.test ] + database_role_name = "%[3]s" + privileges = ["USAGE"] + + on_schema { + schema_name = "%[4]s" + } +} +`, databaseName, databaseRoleName, fullyQualifiedDatabaseRoleName, fullyQualifiedSchemaName) +} + +func TestAcc_GrantPrivilegesToDatabaseRole_IdentifierQuotingDiffSuppression(t *testing.T) { + databaseRoleId := acc.TestClient().Ids.RandomDatabaseObjectIdentifier() + unquotedDatabaseRoleId := fmt.Sprintf(`%s.%s`, databaseRoleId.DatabaseName(), databaseRoleId.Name()) + + schemaId := acc.TestClient().Ids.SchemaId() + unquotedSchemaId := fmt.Sprintf(`%s.%s`, schemaId.DatabaseName(), schemaId.Name()) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { acc.TestAccPreCheck(t) }, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{ + "snowflake": { + VersionConstraint: "=0.94.1", + Source: "Snowflake-Labs/snowflake", + }, + }, + Config: grantPrivilegesToDatabaseRoleBasicConfig(acc.TestClient().Ids.DatabaseId().Name(), databaseRoleId.Name(), unquotedDatabaseRoleId, unquotedSchemaId), + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_grant_privileges_to_database_role.test", "database_role_name", unquotedDatabaseRoleId), + resource.TestCheckResourceAttr("snowflake_grant_privileges_to_database_role.test", "on_schema.0.schema_name", unquotedSchemaId), + resource.TestCheckResourceAttr("snowflake_grant_privileges_to_database_role.test", "id", fmt.Sprintf("%s|false|false|USAGE|OnSchema|OnSchema|%s", databaseRoleId.FullyQualifiedName(), schemaId.FullyQualifiedName())), + ), + }, + { + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + Config: grantPrivilegesToDatabaseRoleBasicConfig(acc.TestClient().Ids.DatabaseId().Name(), databaseRoleId.Name(), unquotedDatabaseRoleId, unquotedSchemaId), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_grant_privileges_to_database_role.test", plancheck.ResourceActionNoop), + }, + PostApplyPostRefresh: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_grant_privileges_to_database_role.test", plancheck.ResourceActionNoop), + }, + }, + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_grant_privileges_to_database_role.test", "database_role_name", unquotedDatabaseRoleId), + resource.TestCheckResourceAttr("snowflake_grant_privileges_to_database_role.test", "on_schema.0.schema_name", unquotedSchemaId), + resource.TestCheckResourceAttr("snowflake_grant_privileges_to_database_role.test", "id", fmt.Sprintf("%s|false|false|USAGE|OnSchema|OnSchema|%s", databaseRoleId.FullyQualifiedName(), schemaId.FullyQualifiedName())), + ), + }, + }, + }) +} diff --git a/pkg/resources/grant_privileges_to_database_role_identifier.go b/pkg/resources/grant_privileges_to_database_role_identifier.go index 45ad11a432..f67c27f049 100644 --- a/pkg/resources/grant_privileges_to_database_role_identifier.go +++ b/pkg/resources/grant_privileges_to_database_role_identifier.go @@ -58,19 +58,11 @@ func ParseGrantPrivilegesToDatabaseRoleId(id string) (GrantPrivilegesToDatabaseR return databaseRoleId, sdk.NewError(`database role identifier should hold at least 6 parts "|||||"`) } - // TODO: Identifier parsing should be replaced with better version introduced in SNOW-999049. - // Right now, it's same as sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName, but with error handling. - databaseRoleNameParts := strings.Split(parts[0], ".") - if len(databaseRoleNameParts) == 0 || - (len(databaseRoleNameParts) == 1 && strings.Trim(databaseRoleNameParts[0], `"`) == "") || - (len(databaseRoleNameParts) == 2 && strings.Trim(databaseRoleNameParts[1], `"`) == "") || - len(databaseRoleNameParts) > 2 { - return databaseRoleId, sdk.NewError(fmt.Sprintf(`invalid DatabaseRoleName value: %s, should be a fully qualified name of database object .`, parts[0])) + roleId, err := sdk.ParseDatabaseObjectIdentifier(parts[0]) + if err != nil { + return databaseRoleId, err } - databaseRoleId.DatabaseRoleName = sdk.NewDatabaseObjectIdentifier( - strings.Trim(databaseRoleNameParts[0], `"`), - strings.Trim(databaseRoleNameParts[1], `"`), - ) + databaseRoleId.DatabaseRoleName = roleId if parts[1] != "false" && parts[1] != "true" { return databaseRoleId, sdk.NewError(fmt.Sprintf(`invalid WithGrantOption value: %s, should be either "true" or "false"`, parts[1])) @@ -96,8 +88,12 @@ func ParseGrantPrivilegesToDatabaseRoleId(id string) (GrantPrivilegesToDatabaseR switch databaseRoleId.Kind { case OnDatabaseDatabaseRoleGrantKind: + databaseId, err := sdk.ParseAccountObjectIdentifier(parts[5]) + if err != nil { + return databaseRoleId, err + } databaseRoleId.Data = &OnDatabaseGrantData{ - DatabaseName: sdk.NewAccountObjectIdentifierFromFullyQualifiedName(parts[5]), + DatabaseName: databaseId, } case OnSchemaDatabaseRoleGrantKind: if len(parts) < 7 { @@ -108,9 +104,17 @@ func ParseGrantPrivilegesToDatabaseRoleId(id string) (GrantPrivilegesToDatabaseR } switch onSchemaGrantData.Kind { case OnSchemaSchemaGrantKind: - onSchemaGrantData.SchemaName = sdk.Pointer(sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(parts[6])) + schemaId, err := sdk.ParseDatabaseObjectIdentifier(parts[6]) + if err != nil { + return databaseRoleId, err + } + onSchemaGrantData.SchemaName = sdk.Pointer(schemaId) case OnAllSchemasInDatabaseSchemaGrantKind, OnFutureSchemasInDatabaseSchemaGrantKind: - onSchemaGrantData.DatabaseName = sdk.Pointer(sdk.NewAccountObjectIdentifier(parts[6])) + databaseId, err := sdk.ParseAccountObjectIdentifier(parts[6]) + if err != nil { + return databaseRoleId, err + } + onSchemaGrantData.DatabaseName = sdk.Pointer(databaseId) default: return databaseRoleId, sdk.NewError(fmt.Sprintf("invalid OnSchemaGrantKind: %s", onSchemaGrantData.Kind)) } @@ -131,13 +135,15 @@ func ParseGrantPrivilegesToDatabaseRoleId(id string) (GrantPrivilegesToDatabaseR var id sdk.ObjectIdentifier // TODO(SNOW-1569535): use a mapper from object type to parsing function if objectType.IsWithArguments() { - var err error id, err = sdk.ParseSchemaObjectIdentifierWithArguments(parts[7]) if err != nil { return databaseRoleId, err } } else { - id = sdk.NewSchemaObjectIdentifierFromFullyQualifiedName(parts[7]) + id, err = sdk.ParseSchemaObjectIdentifier(parts[7]) + if err != nil { + return databaseRoleId, err + } } onSchemaObjectGrantData.Object = &sdk.Object{ ObjectType: objectType, @@ -154,9 +160,17 @@ func ParseGrantPrivilegesToDatabaseRoleId(id string) (GrantPrivilegesToDatabaseR bulkOperationGrantData.Kind = BulkOperationGrantKind(parts[7]) switch bulkOperationGrantData.Kind { case InDatabaseBulkOperationGrantKind: - bulkOperationGrantData.Database = sdk.Pointer(sdk.NewAccountObjectIdentifierFromFullyQualifiedName(parts[8])) + databaseId, err := sdk.ParseAccountObjectIdentifier(parts[8]) + if err != nil { + return databaseRoleId, err + } + bulkOperationGrantData.Database = sdk.Pointer(databaseId) case InSchemaBulkOperationGrantKind: - bulkOperationGrantData.Schema = sdk.Pointer(sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(parts[8])) + schemaId, err := sdk.ParseDatabaseObjectIdentifier(parts[8]) + if err != nil { + return databaseRoleId, err + } + bulkOperationGrantData.Schema = sdk.Pointer(schemaId) default: return databaseRoleId, sdk.NewError(fmt.Sprintf("invalid BulkOperationGrantKind: %s", bulkOperationGrantData.Kind)) } diff --git a/pkg/resources/grant_privileges_to_database_role_identifier_test.go b/pkg/resources/grant_privileges_to_database_role_identifier_test.go index a9ed631605..1f7dac5776 100644 --- a/pkg/resources/grant_privileges_to_database_role_identifier_test.go +++ b/pkg/resources/grant_privileges_to_database_role_identifier_test.go @@ -315,7 +315,7 @@ func TestParseGrantPrivilegesToDatabaseRoleId(t *testing.T) { { Name: "validation: grant database role empty database role name", Identifier: `|false|false|ALL PRIVILEGES|OnDatabase|"on-database-name"`, - Error: "invalid DatabaseRoleName value: , should be a fully qualified name of database object .", + Error: "incompatible identifier: ", }, { Name: "validation: grant database role empty type", diff --git a/pkg/resources/grant_privileges_to_share.go b/pkg/resources/grant_privileges_to_share.go index 6d5e232d47..ee3ab1eaab 100644 --- a/pkg/resources/grant_privileges_to_share.go +++ b/pkg/resources/grant_privileges_to_share.go @@ -31,6 +31,7 @@ var grantPrivilegesToShareSchema = map[string]*schema.Schema{ ForceNew: true, Description: "The fully qualified name of the share on which privileges will be granted.", ValidateDiagFunc: IsValidIdentifier[sdk.AccountObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, }, "privileges": { Type: schema.TypeSet, @@ -44,6 +45,7 @@ var grantPrivilegesToShareSchema = map[string]*schema.Schema{ ForceNew: true, Description: "The fully qualified name of the database on which privileges will be granted.", ValidateDiagFunc: IsValidIdentifier[sdk.AccountObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, ExactlyOneOf: grantPrivilegesToShareGrantExactlyOneOfValidation, }, "on_schema": { @@ -52,6 +54,7 @@ var grantPrivilegesToShareSchema = map[string]*schema.Schema{ ForceNew: true, Description: "The fully qualified name of the schema on which privileges will be granted.", ValidateDiagFunc: IsValidIdentifier[sdk.DatabaseObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, ExactlyOneOf: grantPrivilegesToShareGrantExactlyOneOfValidation, }, "on_table": { @@ -60,6 +63,7 @@ var grantPrivilegesToShareSchema = map[string]*schema.Schema{ ForceNew: true, Description: "The fully qualified name of the table on which privileges will be granted.", ValidateDiagFunc: IsValidIdentifier[sdk.SchemaObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, ExactlyOneOf: grantPrivilegesToShareGrantExactlyOneOfValidation, }, "on_all_tables_in_schema": { @@ -68,6 +72,7 @@ var grantPrivilegesToShareSchema = map[string]*schema.Schema{ ForceNew: true, Description: "The fully qualified identifier for the schema for which the specified privilege will be granted for all tables.", ValidateDiagFunc: IsValidIdentifier[sdk.DatabaseObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, ExactlyOneOf: grantPrivilegesToShareGrantExactlyOneOfValidation, }, "on_tag": { @@ -76,6 +81,7 @@ var grantPrivilegesToShareSchema = map[string]*schema.Schema{ ForceNew: true, Description: "The fully qualified name of the tag on which privileges will be granted.", ValidateDiagFunc: IsValidIdentifier[sdk.SchemaObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, ExactlyOneOf: grantPrivilegesToShareGrantExactlyOneOfValidation, }, "on_view": { @@ -84,14 +90,16 @@ var grantPrivilegesToShareSchema = map[string]*schema.Schema{ ForceNew: true, Description: "The fully qualified name of the view on which privileges will be granted.", ValidateDiagFunc: IsValidIdentifier[sdk.SchemaObjectIdentifier](), + DiffSuppressFunc: suppressIdentifierQuoting, ExactlyOneOf: grantPrivilegesToShareGrantExactlyOneOfValidation, }, "on_function": { - Type: schema.TypeString, - Optional: true, - ForceNew: true, - Description: "The fully qualified name of the function on which privileges will be granted.", - ExactlyOneOf: grantPrivilegesToShareGrantExactlyOneOfValidation, + Type: schema.TypeString, + Optional: true, + ForceNew: true, + Description: "The fully qualified name of the function on which privileges will be granted.", + ExactlyOneOf: grantPrivilegesToShareGrantExactlyOneOfValidation, + DiffSuppressFunc: suppressIdentifierQuoting, }, } @@ -164,6 +172,7 @@ func CreateGrantPrivilegesToShare(ctx context.Context, d *schema.ResourceData, m return diag.FromErr(err) } log.Printf("[DEBUG] created identifier from schema: %s", id.String()) + grantOn, err := getShareGrantOn(d) if err != nil { return diag.FromErr(err) @@ -279,6 +288,7 @@ func DeleteGrantPrivilegesToShare(ctx context.Context, d *schema.ResourceData, m }, } } + grantOn, err := getShareGrantOn(d) if err != nil { return diag.FromErr(err) @@ -387,9 +397,13 @@ func ReadGrantPrivilegesToShare(ctx context.Context, d *schema.ResourceData, met return nil } -func createGrantPrivilegesToShareIdFromSchema(d *schema.ResourceData) (*GrantPrivilegesToShareId, error) { - id := new(GrantPrivilegesToShareId) - id.ShareName = sdk.NewAccountObjectIdentifier(d.Get("to_share").(string)) +func createGrantPrivilegesToShareIdFromSchema(d *schema.ResourceData) (id *GrantPrivilegesToShareId, err error) { + sharedId, err := sdk.ParseAccountObjectIdentifier(d.Get("to_share").(string)) + if err != nil { + return nil, err + } + id = new(GrantPrivilegesToShareId) + id.ShareName = sharedId id.Privileges = expandStringList(d.Get("privileges").(*schema.Set).List()) databaseName, databaseNameOk := d.GetOk("on_database") @@ -403,10 +417,18 @@ func createGrantPrivilegesToShareIdFromSchema(d *schema.ResourceData) (*GrantPri switch { case databaseNameOk: id.Kind = OnDatabaseShareGrantKind - id.Identifier = sdk.NewAccountObjectIdentifierFromFullyQualifiedName(databaseName.(string)) + databaseId, err := sdk.ParseAccountObjectIdentifier(databaseName.(string)) + if err != nil { + return nil, err + } + id.Identifier = databaseId case schemaNameOk: id.Kind = OnSchemaShareGrantKind - id.Identifier = sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(schemaName.(string)) + schemaId, err := sdk.ParseDatabaseObjectIdentifier(schemaName.(string)) + if err != nil { + return nil, err + } + id.Identifier = schemaId case functionNameOk: id.Kind = OnFunctionShareGrantKind parsed, err := sdk.ParseSchemaObjectIdentifierWithArguments(functionName.(string)) @@ -416,16 +438,32 @@ func createGrantPrivilegesToShareIdFromSchema(d *schema.ResourceData) (*GrantPri id.Identifier = parsed case tableNameOk: id.Kind = OnTableShareGrantKind - id.Identifier = sdk.NewSchemaObjectIdentifierFromFullyQualifiedName(tableName.(string)) + tableId, err := sdk.ParseSchemaObjectIdentifier(tableName.(string)) + if err != nil { + return nil, err + } + id.Identifier = tableId case allTablesInSchemaOk: id.Kind = OnAllTablesInSchemaShareGrantKind - id.Identifier = sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(allTablesInSchema.(string)) + schemaId, err := sdk.ParseDatabaseObjectIdentifier(allTablesInSchema.(string)) + if err != nil { + return nil, err + } + id.Identifier = schemaId case tagNameOk: id.Kind = OnTagShareGrantKind - id.Identifier = sdk.NewSchemaObjectIdentifierFromFullyQualifiedName(tagName.(string)) + tagId, err := sdk.ParseSchemaObjectIdentifier(tagName.(string)) + if err != nil { + return nil, err + } + id.Identifier = tagId case viewNameOk: id.Kind = OnViewShareGrantKind - id.Identifier = sdk.NewSchemaObjectIdentifierFromFullyQualifiedName(viewName.(string)) + viewId, err := sdk.ParseSchemaObjectIdentifier(viewName.(string)) + if err != nil { + return nil, err + } + id.Identifier = viewId } return id, nil @@ -453,9 +491,17 @@ func getShareGrantOn(d *schema.ResourceData) (*sdk.ShareGrantOn, error) { switch { case len(databaseName.(string)) > 0 && databaseNameOk: - grantOn.Database = sdk.NewAccountObjectIdentifierFromFullyQualifiedName(databaseName.(string)) + databaseId, err := sdk.ParseAccountObjectIdentifier(databaseName.(string)) + if err != nil { + return nil, err + } + grantOn.Database = databaseId case len(schemaName.(string)) > 0 && schemaNameOk: - grantOn.Schema = sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(schemaName.(string)) + schemaId, err := sdk.ParseDatabaseObjectIdentifier(schemaName.(string)) + if err != nil { + return nil, err + } + grantOn.Schema = schemaId case len(functionName.(string)) > 0 && functionNameOk: id, err := sdk.ParseSchemaObjectIdentifierWithArguments(functionName.(string)) if err != nil { @@ -463,17 +509,33 @@ func getShareGrantOn(d *schema.ResourceData) (*sdk.ShareGrantOn, error) { } grantOn.Function = id case len(tableName.(string)) > 0 && tableNameOk: + tableId, err := sdk.ParseSchemaObjectIdentifier(tableName.(string)) + if err != nil { + return nil, err + } grantOn.Table = &sdk.OnTable{ - Name: sdk.NewSchemaObjectIdentifierFromFullyQualifiedName(tableName.(string)), + Name: tableId, } case len(allTablesInSchema.(string)) > 0 && allTablesInSchemaOk: + schemaId, err := sdk.ParseDatabaseObjectIdentifier(allTablesInSchema.(string)) + if err != nil { + return nil, err + } grantOn.Table = &sdk.OnTable{ - AllInSchema: sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(allTablesInSchema.(string)), + AllInSchema: schemaId, } case len(tagName.(string)) > 0 && tagNameOk: - grantOn.Tag = sdk.NewSchemaObjectIdentifierFromFullyQualifiedName(tagName.(string)) + tagId, err := sdk.ParseSchemaObjectIdentifier(tagName.(string)) + if err != nil { + return nil, err + } + grantOn.Tag = tagId case len(viewName.(string)) > 0 && viewNameOk: - grantOn.View = sdk.NewSchemaObjectIdentifierFromFullyQualifiedName(viewName.(string)) + viewId, err := sdk.ParseSchemaObjectIdentifier(viewName.(string)) + if err != nil { + return nil, err + } + grantOn.View = viewId } return grantOn, nil diff --git a/pkg/resources/grant_privileges_to_share_acceptance_test.go b/pkg/resources/grant_privileges_to_share_acceptance_test.go index 3920ad3983..459ed7ae6d 100644 --- a/pkg/resources/grant_privileges_to_share_acceptance_test.go +++ b/pkg/resources/grant_privileges_to_share_acceptance_test.go @@ -5,6 +5,8 @@ import ( "regexp" "testing" + "github.com/hashicorp/terraform-plugin-testing/plancheck" + acc "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/testenvs" @@ -706,3 +708,105 @@ func TestAcc_GrantPrivilegesToShareWithNameContainingDots_OnTable(t *testing.T) }, }) } + +func TestAcc_GrantPrivilegesToShare_migrateFromV0941_ensureSmoothUpgradeWithNewResourceId(t *testing.T) { + databaseId := acc.TestClient().Ids.RandomAccountObjectIdentifier() + shareId := acc.TestClient().Ids.RandomAccountObjectIdentifier() + + resource.Test(t, resource.TestCase{ + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{ + "snowflake": { + VersionConstraint: "=0.94.1", + Source: "Snowflake-Labs/snowflake", + }, + }, + Config: grantPrivilegesToShareBasicConfig(databaseId.Name(), shareId.Name()), + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_grant_privileges_to_share.test", "id", fmt.Sprintf(`%s|USAGE|OnDatabase|%s`, shareId.FullyQualifiedName(), databaseId.FullyQualifiedName())), + ), + }, + { + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + Config: grantPrivilegesToShareBasicConfig(databaseId.Name(), shareId.Name()), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_grant_privileges_to_share.test", plancheck.ResourceActionNoop), + }, + PostApplyPostRefresh: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_grant_privileges_to_share.test", plancheck.ResourceActionNoop), + }, + }, + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_grant_privileges_to_share.test", "id", fmt.Sprintf(`%s|USAGE|OnDatabase|%s`, shareId.FullyQualifiedName(), databaseId.FullyQualifiedName())), + ), + }, + }, + }) +} + +func grantPrivilegesToShareBasicConfig(databaseName string, shareName string) string { + return fmt.Sprintf(` +resource "snowflake_database" "test" { + name = "%[1]s" +} + +resource "snowflake_share" "test" { + depends_on = [snowflake_database.test] + name = "%[2]s" +} + +resource "snowflake_grant_privileges_to_share" "test" { + to_share = snowflake_share.test.name + privileges = ["USAGE"] + on_database = snowflake_database.test.name +} +`, databaseName, shareName) +} + +func TestAcc_GrantPrivilegesToShare_IdentifierQuotingDiffSuppression(t *testing.T) { + databaseId := acc.TestClient().Ids.RandomAccountObjectIdentifier() + quotedDatabaseId := fmt.Sprintf(`\"%s\"`, databaseId.Name()) + + shareId := acc.TestClient().Ids.RandomAccountObjectIdentifier() + quotedShareId := fmt.Sprintf(`\"%s\"`, shareId.Name()) + + resource.Test(t, resource.TestCase{ + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{ + "snowflake": { + VersionConstraint: "=0.94.1", + Source: "Snowflake-Labs/snowflake", + }, + }, + ExpectError: regexp.MustCompile("Error: Provider produced inconsistent final plan"), + Config: grantPrivilegesToShareBasicConfig(quotedDatabaseId, quotedShareId), + }, + { + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + Config: grantPrivilegesToShareBasicConfig(quotedDatabaseId, quotedShareId), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_grant_privileges_to_share.test", plancheck.ResourceActionCreate), + }, + PostApplyPostRefresh: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("snowflake_grant_privileges_to_share.test", plancheck.ResourceActionNoop), + }, + }, + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_grant_privileges_to_share.test", "to_share", shareId.Name()), + resource.TestCheckResourceAttr("snowflake_grant_privileges_to_share.test", "on_database", databaseId.Name()), + resource.TestCheckResourceAttr("snowflake_grant_privileges_to_share.test", "id", fmt.Sprintf(`%s|USAGE|OnDatabase|%s`, shareId.FullyQualifiedName(), databaseId.FullyQualifiedName())), + ), + }, + }, + }) +} diff --git a/pkg/resources/grant_privileges_to_share_identifier.go b/pkg/resources/grant_privileges_to_share_identifier.go index 7397cf34b0..a996eb1257 100644 --- a/pkg/resources/grant_privileges_to_share_identifier.go +++ b/pkg/resources/grant_privileges_to_share_identifier.go @@ -36,15 +36,16 @@ func (id *GrantPrivilegesToShareId) String() string { ) } -func ParseGrantPrivilegesToShareId(idString string) (GrantPrivilegesToShareId, error) { - var grantPrivilegesToShareId GrantPrivilegesToShareId - +func ParseGrantPrivilegesToShareId(idString string) (grantPrivilegesToShareId GrantPrivilegesToShareId, err error) { parts := helpers.ParseResourceIdentifier(idString) if len(parts) != 4 { return grantPrivilegesToShareId, sdk.NewError(fmt.Sprintf(`snowflake_grant_privileges_to_share id is composed out of 4 parts "|||", but got %d parts: %v`, len(parts), parts)) } - - grantPrivilegesToShareId.ShareName = sdk.NewAccountObjectIdentifier(parts[0]) + shareId, err := sdk.ParseAccountObjectIdentifier(parts[0]) + if err != nil { + return grantPrivilegesToShareId, err + } + grantPrivilegesToShareId.ShareName = shareId privileges := strings.Split(parts[1], ",") if len(privileges) == 0 || (len(privileges) == 1 && privileges[0] == "") { return grantPrivilegesToShareId, sdk.NewError(fmt.Sprintf(`invalid Privileges value: %s, should be comma separated list of privileges`, privileges)) diff --git a/pkg/resources/grant_privileges_to_share_identifier_test.go b/pkg/resources/grant_privileges_to_share_identifier_test.go index ef38ddcb3a..70b77d4fc1 100644 --- a/pkg/resources/grant_privileges_to_share_identifier_test.go +++ b/pkg/resources/grant_privileges_to_share_identifier_test.go @@ -24,6 +24,16 @@ func TestParseGrantPrivilegesToShareId(t *testing.T) { Identifier: sdk.NewAccountObjectIdentifier("on-database-name"), }, }, + { + Name: "grant privileges on database to share - database identifier with dots", + Identifier: `"share-name"|SELECT|OnDatabase|"one.two.three.four.five.six.seven.eight.nine.ten"`, + Expected: GrantPrivilegesToShareId{ + ShareName: sdk.NewAccountObjectIdentifier("share-name"), + Privileges: []string{"SELECT"}, + Kind: OnDatabaseShareGrantKind, + Identifier: sdk.NewAccountObjectIdentifier("one.two.three.four.five.six.seven.eight.nine.ten"), + }, + }, { Name: "grant privileges on schema to share", Identifier: `"share-name"|USAGE|OnSchema|"on-database-name"."on-schema-name"`, @@ -109,15 +119,10 @@ func TestParseGrantPrivilegesToShareId(t *testing.T) { Identifier: `"share-name"|SELECT|OnSomething|"object-name"`, Error: `unexpected share grant kind: OnSomething`, }, - { - Name: "validation: invalid identifier", - Identifier: `"share-name"|SELECT|OnDatabase|"one.two.three.four.five"."six.seven.eight.nine.ten"`, - Error: `unexpected number of parts 2 in identifier "one.two.three.four.five"."six.seven.eight.nine.ten", expected 1 in a form of ""`, - }, { Name: "validation: invalid account object identifier", - Identifier: `"share-name"|SELECT|OnTable|one.two`, - Error: `unexpected number of parts 2 in identifier one.two, expected 3 in a form of ".."`, + Identifier: `"share-name"|SELECT|OnTable|"one"."two"`, + Error: `unexpected number of parts 2 in identifier "one"."two", expected 3 in a form of ".."`, }, { Name: "validation: invalid database object identifier", diff --git a/pkg/resources/network_policy.go b/pkg/resources/network_policy.go index ee5744d038..bd0bcd5950 100644 --- a/pkg/resources/network_policy.go +++ b/pkg/resources/network_policy.go @@ -117,7 +117,7 @@ func NetworkPolicy() *schema.Resource { ), Importer: &schema.ResourceImporter{ - StateContext: ImportName, + StateContext: ImportName[sdk.AccountObjectIdentifier], }, } } diff --git a/pkg/resources/secondary_database.go b/pkg/resources/secondary_database.go index 3e5a12f1db..15cdf7f703 100644 --- a/pkg/resources/secondary_database.go +++ b/pkg/resources/secondary_database.go @@ -57,7 +57,7 @@ func SecondaryDatabase() *schema.Resource { ), Schema: helpers.MergeMaps(secondaryDatabaseSchema, databaseParametersSchema), Importer: &schema.ResourceImporter{ - StateContext: ImportName, + StateContext: ImportName[sdk.AccountObjectIdentifier], }, } } diff --git a/pkg/resources/share.go b/pkg/resources/share.go index d14f63bd49..7ebaf4a25d 100644 --- a/pkg/resources/share.go +++ b/pkg/resources/share.go @@ -16,10 +16,11 @@ import ( var shareSchema = map[string]*schema.Schema{ "name": { - Type: schema.TypeString, - Required: true, - Description: "Specifies the identifier for the share; must be unique for the account in which the share is created.", - ForceNew: true, + Type: schema.TypeString, + Required: true, + Description: "Specifies the identifier for the share; must be unique for the account in which the share is created.", + ForceNew: true, + DiffSuppressFunc: suppressIdentifierQuoting, }, "comment": { Type: schema.TypeString, @@ -51,7 +52,7 @@ func Share() *schema.Resource { Schema: shareSchema, Importer: &schema.ResourceImporter{ - StateContext: schema.ImportStatePassthroughContext, + StateContext: ImportName[sdk.AccountObjectIdentifier], }, } } @@ -167,9 +168,6 @@ func ReadShare(d *schema.ResourceData, meta interface{}) error { if err := d.Set(FullyQualifiedNameAttributeName, id.FullyQualifiedName()); err != nil { return err } - if err := d.Set("name", share.Name.Name()); err != nil { - return err - } if err := d.Set("comment", share.Comment); err != nil { return err } diff --git a/pkg/resources/shared_database.go b/pkg/resources/shared_database.go index f3c176ff36..02675867a5 100644 --- a/pkg/resources/shared_database.go +++ b/pkg/resources/shared_database.go @@ -58,7 +58,7 @@ func SharedDatabase() *schema.Resource { Schema: helpers.MergeMaps(sharedDatabaseSchema, sharedDatabaseParametersSchema), Importer: &schema.ResourceImporter{ - StateContext: ImportName, + StateContext: ImportName[sdk.AccountObjectIdentifier], }, } } diff --git a/pkg/resources/streamlit.go b/pkg/resources/streamlit.go index 05cbc519ce..342b9e0c61 100644 --- a/pkg/resources/streamlit.go +++ b/pkg/resources/streamlit.go @@ -4,9 +4,10 @@ import ( "context" "errors" "fmt" - "github.com/hashicorp/go-cty/cty" "path" + "github.com/hashicorp/go-cty/cty" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/helpers" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/logging" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/provider" diff --git a/pkg/resources/table.go b/pkg/resources/table.go index 44c6070ebb..13280eb9fc 100644 --- a/pkg/resources/table.go +++ b/pkg/resources/table.go @@ -22,9 +22,10 @@ import ( // TODO [SNOW-1031688]: move data manipulation logic to the SDK - SQL generation or builders part (e.g. different default types/identity) var tableSchema = map[string]*schema.Schema{ "name": { - Type: schema.TypeString, - Required: true, - Description: "Specifies the identifier for the table; must be unique for the database and schema in which the table is created.", + Type: schema.TypeString, + Required: true, + Description: "Specifies the identifier for the table; must be unique for the database and schema in which the table is created.", + DiffSuppressFunc: suppressIdentifierQuoting, }, "schema": { Type: schema.TypeString, diff --git a/pkg/sdk/identifier_parsers_test.go b/pkg/sdk/identifier_parsers_test.go index 75b744b745..381a530a9a 100644 --- a/pkg/sdk/identifier_parsers_test.go +++ b/pkg/sdk/identifier_parsers_test.go @@ -136,12 +136,12 @@ func Test_IdentifierParsers(t *testing.T) { Expected ObjectIdentifier Error string }{ - {IdentifierType: "AccountObjectIdentifier", Input: `a"b`, Error: "unable to read identifier: \"a\"b\", err = parse error on line 1, column 3: extraneous or missing \" in quoted-field"}, + {IdentifierType: "AccountObjectIdentifier", Input: ``, Expected: NewAccountObjectIdentifier("")}, + {IdentifierType: "AccountObjectIdentifier", Input: "a\nb", Expected: NewAccountObjectIdentifier("a\nb")}, + {IdentifierType: "AccountObjectIdentifier", Input: `a"b`, Error: `unable to read identifier: "a"b", err = parse error on line 1, column 3: extraneous or missing " in quoted-field`}, + {IdentifierType: "AccountObjectIdentifier", Input: `abc.cde`, Expected: NewAccountObjectIdentifier("abc.cde")}, {IdentifierType: "AccountObjectIdentifier", Input: `""""`, Error: `unable to parse identifier: """", currently identifiers containing double quotes are not supported in the provider`}, {IdentifierType: "AccountObjectIdentifier", Input: `"a""bc"`, Error: `unable to parse identifier: "a""bc", currently identifiers containing double quotes are not supported in the provider`}, - {IdentifierType: "AccountObjectIdentifier", Input: ``, Expected: NewAccountObjectIdentifier(``)}, - {IdentifierType: "AccountObjectIdentifier", Input: "a\nb", Expected: NewAccountObjectIdentifier("a\nb")}, - {IdentifierType: "AccountObjectIdentifier", Input: `abc.cde`, Expected: NewAccountObjectIdentifier(`abc.cde`)}, {IdentifierType: "AccountObjectIdentifier", Input: `""`, Expected: NewAccountObjectIdentifier(``)}, {IdentifierType: "AccountObjectIdentifier", Input: `abc`, Expected: NewAccountObjectIdentifier(`abc`)}, {IdentifierType: "AccountObjectIdentifier", Input: `"abc"`, Expected: NewAccountObjectIdentifier(`abc`)},