From eb20051ee1d2b6f23e851fc7b9b87c3aac9ef589 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Cie=C5=9Blak?= Date: Thu, 8 Feb 2024 16:39:34 +0100 Subject: [PATCH] feat: Handle IMPORTED PRIVILEGES privileges in privilege-to-role granting resources (#2471) IMPORTED PRIVILEGES privilege is a special case where returned (IMPORTED PRIVILEGES) privilege from SHOW GRANTS shows up as a USAGE privilege. Thus, I had to create a logic that would swap those two in the Read operation. As IMPORTED PRIVILEGES is a privilege that can only be applied to the database created from share, I had to create a test with share and the database created on the second account. References - https://github.com/Snowflake-Labs/terraform-provider-snowflake/discussions/1890#discussioncomment-8376705 - https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/1998 --- .../grant_privileges_to_account_role.go | 32 ++++- ...vileges_to_account_role_acceptance_test.go | 126 +++++++++++++++++- pkg/resources/grant_privileges_to_role.go | 28 +++- ...rant_privileges_to_role_acceptance_test.go | 61 +++++++++ .../ImportedPrivileges/test.tf | 22 +++ .../ImportedPrivileges/variables.tf | 19 +++ .../ImportedPrivileges/test.tf | 22 +++ .../ImportedPrivileges/variables.tf | 19 +++ 8 files changed, 318 insertions(+), 11 deletions(-) create mode 100644 pkg/resources/testdata/TestAcc_GrantPrivilegesToAccountRole/ImportedPrivileges/test.tf create mode 100644 pkg/resources/testdata/TestAcc_GrantPrivilegesToAccountRole/ImportedPrivileges/variables.tf create mode 100644 pkg/resources/testdata/TestAcc_GrantPrivilegesToRole/ImportedPrivileges/test.tf create mode 100644 pkg/resources/testdata/TestAcc_GrantPrivilegesToRole/ImportedPrivileges/variables.tf diff --git a/pkg/resources/grant_privileges_to_account_role.go b/pkg/resources/grant_privileges_to_account_role.go index 77a464a01b..0176d0f95d 100644 --- a/pkg/resources/grant_privileges_to_account_role.go +++ b/pkg/resources/grant_privileges_to_account_role.go @@ -5,6 +5,7 @@ import ( "database/sql" "fmt" "slices" + "strings" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/logging" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk" @@ -22,6 +23,10 @@ var grantPrivilegesToAccountRoleSchema = map[string]*schema.Schema{ Description: "The fully qualified name of the account role to which privileges will be granted.", ValidateDiagFunc: IsValidIdentifier[sdk.AccountObjectIdentifier](), }, + // 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 + // same time (IMPORTED PRIVILEGES can only be granted to the database created from SHARE and USAGE in every other case). + // To handle both cases, additional logic was added in read operation where IMPORTED PRIVILEGES is replaced with USAGE. "privileges": { Type: schema.TypeSet, Optional: true, @@ -746,7 +751,15 @@ func ReadGrantPrivilegesToAccountRole(ctx context.Context, d *schema.ResourceDat } } - var privileges []string + actualPrivileges := make([]string, 0) + expectedPrivileges := make([]string, 0) + expectedPrivileges = append(expectedPrivileges, id.Privileges...) + + if slices.ContainsFunc(expectedPrivileges, func(s string) bool { + return strings.ToUpper(s) == sdk.AccountObjectPrivilegeImportedPrivileges.String() + }) { + expectedPrivileges = append(expectedPrivileges, sdk.AccountObjectPrivilegeUsage.String()) + } logging.DebugLogger.Printf("[DEBUG] Filtering grants to be set on account: count = %d", len(grants)) for _, grant := range grants { @@ -756,7 +769,7 @@ func ReadGrantPrivilegesToAccountRole(ctx context.Context, d *schema.ResourceDat } // Only consider privileges that are already present in the ID, so we // don't delete privileges managed by other resources. - if !slices.Contains(id.Privileges, grant.Privilege) { + if !slices.Contains(expectedPrivileges, grant.Privilege) { continue } if grant.GrantOption == id.WithGrantOption && grant.GranteeName.Name() == id.RoleName.Name() { @@ -768,18 +781,25 @@ func ReadGrantPrivilegesToAccountRole(ctx context.Context, d *schema.ResourceDat // grant_on is for future grants, granted_on is for current grants. // They function the same way though in a test for matching the object type if grantedOn == grant.GrantedOn || grantedOn == grant.GrantOn { - privileges = append(privileges, grant.Privilege) + actualPrivileges = append(actualPrivileges, grant.Privilege) } } } - logging.DebugLogger.Printf("[DEBUG] Setting privileges: %v", privileges) - if err := d.Set("privileges", privileges); err != nil { + usageIndex := slices.IndexFunc(actualPrivileges, func(s string) bool { return strings.ToUpper(s) == sdk.AccountObjectPrivilegeUsage.String() }) + if slices.ContainsFunc(expectedPrivileges, func(s string) bool { + return strings.ToUpper(s) == sdk.AccountObjectPrivilegeImportedPrivileges.String() + }) && usageIndex >= 0 { + actualPrivileges[usageIndex] = sdk.AccountObjectPrivilegeImportedPrivileges.String() + } + + logging.DebugLogger.Printf("[DEBUG] Setting privileges: %v", actualPrivileges) + if err := d.Set("privileges", actualPrivileges); err != nil { return diag.Diagnostics{ diag.Diagnostic{ Severity: diag.Error, Summary: "Error setting privileges for account role", - Detail: fmt.Sprintf("Id: %s\nPrivileges: %v\nError: %s", d.Id(), privileges, err.Error()), + Detail: fmt.Sprintf("Id: %s\nPrivileges: %v\nError: %s", d.Id(), actualPrivileges, err.Error()), }, } } 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 c2a89f1cf8..a4306ecfdb 100644 --- a/pkg/resources/grant_privileges_to_account_role_acceptance_test.go +++ b/pkg/resources/grant_privileges_to_account_role_acceptance_test.go @@ -3,12 +3,16 @@ package resources_test import ( "context" "database/sql" + "errors" "fmt" "log" "regexp" "strings" "testing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/hashicorp/terraform-plugin-testing/helper/acctest" acc "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance" @@ -821,6 +825,123 @@ func TestAcc_GrantPrivilegesToAccountRole_AlwaysApply(t *testing.T) { }) } +func TestAcc_GrantPrivilegesToAccountRole_ImportedPrivileges(t *testing.T) { + sharedDatabaseName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)) + shareName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)) + roleName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)) + secondaryAccountName, err := getSecondaryAccountName(t) + require.NoError(t, err) + configVariables := config.Variables{ + "role_name": config.StringVariable(roleName), + "shared_database_name": config.StringVariable(sharedDatabaseName), + "share_name": config.StringVariable(shareName), + "account_name": config.StringVariable(secondaryAccountName), + "privileges": config.ListVariable( + config.StringVariable(sdk.AccountObjectPrivilegeImportedPrivileges.String()), + ), + } + resourceName := "snowflake_grant_privileges_to_account_role.test" + + resource.Test(t, resource.TestCase{ + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + PreCheck: func() { acc.TestAccPreCheck(t) }, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + CheckDestroy: func(state *terraform.State) error { + return errors.Join( + testAccCheckAccountRolePrivilegesRevoked(roleName)(state), + dropSharedDatabaseOnSecondaryAccount(t, sharedDatabaseName, shareName), + ) + }, + Steps: []resource.TestStep{ + { + PreConfig: func() { assert.NoError(t, createSharedDatabaseOnSecondaryAccount(t, sharedDatabaseName, shareName)) }, + ConfigDirectory: acc.ConfigurationDirectory("TestAcc_GrantPrivilegesToAccountRole/ImportedPrivileges"), + ConfigVariables: configVariables, + ConfigPlanChecks: resource.ConfigPlanChecks{ + PostApplyPostRefresh: []plancheck.PlanCheck{ + plancheck.ExpectEmptyPlan(), + }, + }, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(resourceName, "privileges.#", "1"), + resource.TestCheckResourceAttr(resourceName, "privileges.0", sdk.AccountObjectPrivilegeImportedPrivileges.String()), + ), + }, + { + ConfigDirectory: acc.ConfigurationDirectory("TestAcc_GrantPrivilegesToAccountRole/ImportedPrivileges"), + ConfigVariables: configVariables, + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + +func getSecondaryAccountName(t *testing.T) (string, error) { + t.Helper() + config, err := sdk.ProfileConfig("secondary_test_account") + if err != nil { + t.Fatal(err) + } + client, err := sdk.NewClient(config) + if err != nil { + t.Fatal(err) + } + return client.ContextFunctions.CurrentAccount(context.Background()) +} + +func getAccountName(t *testing.T) (string, error) { + t.Helper() + client, err := sdk.NewDefaultClient() + if err != nil { + t.Fatal(err) + } + return client.ContextFunctions.CurrentAccount(context.Background()) +} + +func createSharedDatabaseOnSecondaryAccount(t *testing.T, databaseName string, shareName string) error { + t.Helper() + config, err := sdk.ProfileConfig("secondary_test_account") + if err != nil { + t.Fatal(err) + } + client, err := sdk.NewClient(config) + if err != nil { + t.Fatal(err) + } + ctx := context.Background() + accountName, err := getAccountName(t) + return errors.Join( + err, + client.Databases.Create(ctx, sdk.NewAccountObjectIdentifier(databaseName), &sdk.CreateDatabaseOptions{}), + client.Shares.Create(ctx, sdk.NewAccountObjectIdentifier(shareName), &sdk.CreateShareOptions{}), + client.Grants.GrantPrivilegeToShare(ctx, []sdk.ObjectPrivilege{sdk.ObjectPrivilegeReferenceUsage}, &sdk.ShareGrantOn{Database: sdk.NewAccountObjectIdentifier(databaseName)}, sdk.NewAccountObjectIdentifier(shareName)), + client.Shares.Alter(ctx, sdk.NewAccountObjectIdentifier(shareName), &sdk.AlterShareOptions{Set: &sdk.ShareSet{ + Accounts: []sdk.AccountIdentifier{sdk.NewAccountIdentifierFromAccountLocator(accountName)}, + }}), + ) +} + +func dropSharedDatabaseOnSecondaryAccount(t *testing.T, databaseName string, shareName string) error { + t.Helper() + config, err := sdk.ProfileConfig("secondary_test_account") + if err != nil { + t.Fatal(err) + } + client, err := sdk.NewClient(config) + if err != nil { + t.Fatal(err) + } + ctx := context.Background() + return errors.Join( + client.Shares.Drop(ctx, sdk.NewAccountObjectIdentifier(shareName)), + client.Databases.Drop(ctx, sdk.NewAccountObjectIdentifier(databaseName), &sdk.DropDatabaseOptions{}), + ) +} + func createAccountRoleOutsideTerraform(t *testing.T, name string) { t.Helper() client, err := sdk.NewDefaultClient() @@ -840,7 +961,7 @@ func testAccCheckAccountRolePrivilegesRevoked(name string) func(*terraform.State client := sdk.NewClientFromDB(db) defer func() { - err := client.Roles.Drop(context.Background(), sdk.NewDropRoleRequest(sdk.NewAccountObjectIdentifier(name))) + err := client.Roles.Drop(context.Background(), sdk.NewDropRoleRequest(sdk.NewAccountObjectIdentifier(name)).WithIfExists(true)) if err != nil { log.Printf("failed to drop account role (%s), err = %s\n", name, err.Error()) } @@ -859,6 +980,9 @@ func testAccCheckAccountRolePrivilegesRevoked(name string) func(*terraform.State }, }) if err != nil { + if errors.Is(err, sdk.ErrObjectNotExistOrAuthorized) { + continue + } return err } var grantedPrivileges []string diff --git a/pkg/resources/grant_privileges_to_role.go b/pkg/resources/grant_privileges_to_role.go index 2faf7c3f3c..302b567f04 100644 --- a/pkg/resources/grant_privileges_to_role.go +++ b/pkg/resources/grant_privileges_to_role.go @@ -16,6 +16,10 @@ import ( ) var grantPrivilegesToRoleSchema = map[string]*schema.Schema{ + // 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 + // same time (IMPORTED PRIVILEGES can only be granted to the database created from SHARE and USAGE in every other case). + // To handle both cases, additional logic was added in read operation where IMPORTED PRIVILEGES is replaced with USAGE. "privileges": { Type: schema.TypeSet, Optional: true, @@ -834,14 +838,23 @@ func readRoleGrantPrivileges(ctx context.Context, client *sdk.Client, grantedOn } withGrantOption := d.Get("with_grant_option").(bool) - privileges := []string{} roleName := d.Get("role_name").(string) + actualPrivileges := make([]string, 0) + expectedPrivileges := make([]string, 0) + expectedPrivileges = append(expectedPrivileges, id.Privileges...) + + if slices.ContainsFunc(expectedPrivileges, func(s string) bool { + return strings.ToUpper(s) == sdk.AccountObjectPrivilegeImportedPrivileges.String() + }) { + expectedPrivileges = append(expectedPrivileges, sdk.AccountObjectPrivilegeUsage.String()) + } + logging.DebugLogger.Printf("[DEBUG] Filtering grants to be set on account: count = %d", len(grants)) for _, grant := range grants { // Only consider privileges that are already present in the ID so we // don't delete privileges managed by other resources. - if !slices.Contains(id.Privileges, grant.Privilege) { + if !slices.Contains(expectedPrivileges, grant.Privilege) { continue } if grant.GrantOption == withGrantOption && grant.GranteeName.Name() == sdk.NewAccountObjectIdentifier(roleName).Name() { @@ -852,13 +865,20 @@ func readRoleGrantPrivileges(ctx context.Context, client *sdk.Client, grantedOn } // grant_on is for future grants, granted_on is for current grants. They function the same way though in a test for matching the object type if grantedOn == grant.GrantedOn || grantedOn == grant.GrantOn { - privileges = append(privileges, grant.Privilege) + actualPrivileges = append(actualPrivileges, grant.Privilege) } } } + usageIndex := slices.IndexFunc(actualPrivileges, func(s string) bool { return strings.ToUpper(s) == sdk.AccountObjectPrivilegeUsage.String() }) + if slices.ContainsFunc(expectedPrivileges, func(s string) bool { + return strings.ToUpper(s) == sdk.AccountObjectPrivilegeImportedPrivileges.String() + }) && usageIndex >= 0 { + actualPrivileges[usageIndex] = sdk.AccountObjectPrivilegeImportedPrivileges.String() + } + logging.DebugLogger.Printf("[DEBUG] Setting privileges on account") - if err := d.Set("privileges", privileges); err != nil { + if err := d.Set("privileges", actualPrivileges); err != nil { logging.DebugLogger.Printf("[DEBUG] Error setting privileges for account role: err = %v", err) return fmt.Errorf("error setting privileges for account role: %w", err) } diff --git a/pkg/resources/grant_privileges_to_role_acceptance_test.go b/pkg/resources/grant_privileges_to_role_acceptance_test.go index 6dc43b2ca5..bd76b12553 100644 --- a/pkg/resources/grant_privileges_to_role_acceptance_test.go +++ b/pkg/resources/grant_privileges_to_role_acceptance_test.go @@ -1,11 +1,17 @@ package resources_test import ( + "errors" "fmt" "regexp" "strings" "testing" + "github.com/hashicorp/terraform-plugin-testing/config" + "github.com/hashicorp/terraform-plugin-testing/terraform" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/hashicorp/terraform-plugin-testing/plancheck" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk" @@ -980,3 +986,58 @@ resource "snowflake_grant_privileges_to_role" "test_invalidation" { }, }) } + +func TestAcc_GrantPrivilegesToRole_ImportedPrivileges(t *testing.T) { + sharedDatabaseName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)) + shareName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)) + roleName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)) + secondaryAccountName, err := getSecondaryAccountName(t) + require.NoError(t, err) + configVariables := config.Variables{ + "role_name": config.StringVariable(roleName), + "shared_database_name": config.StringVariable(sharedDatabaseName), + "share_name": config.StringVariable(shareName), + "account_name": config.StringVariable(secondaryAccountName), + "privileges": config.ListVariable( + config.StringVariable(sdk.AccountObjectPrivilegeImportedPrivileges.String()), + ), + } + resourceName := "snowflake_grant_privileges_to_role.test" + + resource.Test(t, resource.TestCase{ + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + PreCheck: func() { acc.TestAccPreCheck(t) }, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + CheckDestroy: func(state *terraform.State) error { + return errors.Join( + testAccCheckAccountRolePrivilegesRevoked(roleName)(state), + dropSharedDatabaseOnSecondaryAccount(t, sharedDatabaseName, shareName), + ) + }, + Steps: []resource.TestStep{ + { + PreConfig: func() { assert.NoError(t, createSharedDatabaseOnSecondaryAccount(t, sharedDatabaseName, shareName)) }, + ConfigDirectory: acc.ConfigurationDirectory("TestAcc_GrantPrivilegesToRole/ImportedPrivileges"), + ConfigVariables: configVariables, + ConfigPlanChecks: resource.ConfigPlanChecks{ + PostApplyPostRefresh: []plancheck.PlanCheck{ + plancheck.ExpectEmptyPlan(), + }, + }, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(resourceName, "privileges.#", "1"), + resource.TestCheckResourceAttr(resourceName, "privileges.0", sdk.AccountObjectPrivilegeImportedPrivileges.String()), + ), + }, + { + ConfigDirectory: acc.ConfigurationDirectory("TestAcc_GrantPrivilegesToRole/ImportedPrivileges"), + ConfigVariables: configVariables, + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} diff --git a/pkg/resources/testdata/TestAcc_GrantPrivilegesToAccountRole/ImportedPrivileges/test.tf b/pkg/resources/testdata/TestAcc_GrantPrivilegesToAccountRole/ImportedPrivileges/test.tf new file mode 100644 index 0000000000..f0ab03829d --- /dev/null +++ b/pkg/resources/testdata/TestAcc_GrantPrivilegesToAccountRole/ImportedPrivileges/test.tf @@ -0,0 +1,22 @@ +resource "snowflake_database" "test" { + name = var.shared_database_name + data_retention_time_in_days = 0 + from_share = { + provider = var.account_name + share = var.share_name + } +} + +resource "snowflake_role" "test" { + name = var.role_name +} + +resource "snowflake_grant_privileges_to_account_role" "test" { + depends_on = [snowflake_database.test, snowflake_role.test] + account_role_name = "\"${var.role_name}\"" + privileges = var.privileges + on_account_object { + object_type = "DATABASE" + object_name = "\"${var.shared_database_name}\"" + } +} diff --git a/pkg/resources/testdata/TestAcc_GrantPrivilegesToAccountRole/ImportedPrivileges/variables.tf b/pkg/resources/testdata/TestAcc_GrantPrivilegesToAccountRole/ImportedPrivileges/variables.tf new file mode 100644 index 0000000000..9e948b415f --- /dev/null +++ b/pkg/resources/testdata/TestAcc_GrantPrivilegesToAccountRole/ImportedPrivileges/variables.tf @@ -0,0 +1,19 @@ +variable "privileges" { + type = list(string) +} + +variable "role_name" { + type = string +} + +variable "shared_database_name" { + type = string +} + +variable "share_name" { + type = string +} + +variable "account_name" { + type = string +} diff --git a/pkg/resources/testdata/TestAcc_GrantPrivilegesToRole/ImportedPrivileges/test.tf b/pkg/resources/testdata/TestAcc_GrantPrivilegesToRole/ImportedPrivileges/test.tf new file mode 100644 index 0000000000..593ba8b1e2 --- /dev/null +++ b/pkg/resources/testdata/TestAcc_GrantPrivilegesToRole/ImportedPrivileges/test.tf @@ -0,0 +1,22 @@ +resource "snowflake_database" "test" { + name = var.shared_database_name + data_retention_time_in_days = 0 + from_share = { + provider = var.account_name + share = var.share_name + } +} + +resource "snowflake_role" "test" { + name = var.role_name +} + +resource "snowflake_grant_privileges_to_role" "test" { + depends_on = [snowflake_database.test, snowflake_role.test] + role_name = "\"${var.role_name}\"" + privileges = var.privileges + on_account_object { + object_type = "DATABASE" + object_name = "\"${var.shared_database_name}\"" + } +} diff --git a/pkg/resources/testdata/TestAcc_GrantPrivilegesToRole/ImportedPrivileges/variables.tf b/pkg/resources/testdata/TestAcc_GrantPrivilegesToRole/ImportedPrivileges/variables.tf new file mode 100644 index 0000000000..9e948b415f --- /dev/null +++ b/pkg/resources/testdata/TestAcc_GrantPrivilegesToRole/ImportedPrivileges/variables.tf @@ -0,0 +1,19 @@ +variable "privileges" { + type = list(string) +} + +variable "role_name" { + type = string +} + +variable "shared_database_name" { + type = string +} + +variable "share_name" { + type = string +} + +variable "account_name" { + type = string +}