Skip to content

Commit

Permalink
fix: with_grant_option diff drift (#2608)
Browse files Browse the repository at this point in the history 
Fixes: #2459

The issue was related to the fact that the Read operation is only
concerned about privileges (with_grant_option is also taken into
consideration, but it's never set). Given this configuration:
```terraform
resource "snowflake_grant_privileges_to_account_role" "test" {
  account_role_name = "TEST_ROLE"
  privileges = ["TRUNCATE"]
  on_schema_object {
    object_type = "TABLE"
    object_name = "TEST_DATABASE.TEST_SCHEMA.TEST_TABLE"
  }
  with_grant_option = true
}
```
after apply we run the following commands by hand
```sql
revoke truncate on table test_table from role test_role;
grant truncate on table test_table to role test_role; -- notice we don't add "with grant option" which our resource should detect
```
Now, when we run `plan` or `apply` our resource is seeing a drift (the
"TRUNCATE" privilege is not present, because `with_grant_option` is not
matching) and tries to run the Update operation (unsuccessfully; 1.
because of the "sdk.GrantPrivOptions" not set 2. because of the
incorrect logic). When there're already existing grants there are two
ways to update `with_grant_option` which depends on what is set in
Snowflake. It's better to show it with SQLs, so:

```sql
-- imagine this is ran by Snowflake Terraform Plugin (with_grant_option is set to true in the config)
grant truncate on table test_table to role test_role with grant option;

-- this is ran by hand in the worksheet
revoke truncate on table test_table from role test_role;
grant truncate on table test_table to role test_role; 

-- now update tries to run the following
grant truncate on table test_table to role test_role with grant option; -- this will successfully update with_grant_option to 'true'
```

```sql
-- imagine this is ran by Snowflake Terraform Plugin (with_grant_option is set to false in the config)
grant truncate on table test_table to role test_role;

-- this is ran by hand in the worksheet
revoke truncate on table test_table from role test_role;
grant truncate on table test_table to role test_role with grant option; 

-- now update tries to run the following
grant truncate on table test_table to role test_role; -- this won't update the with_grant_option to false because Snowflake is not updating the value when the option is already set to true (you have to revoke it first)
```

The fix I opted to is to:
- when with_grant_option is set to true in the config
- proceed as it was (but now set option struct correctly with
with_grant_option set to true)
- when with_grant_option is set to false in the config
- firstly revoke privileges we would like to add (just in case this
issue happens; it won't fail even if the grant does not exist)
  - then proceed as it was (grant privileges we would like to add) 

todo other grant privileges to database role

## Test Plan
<!-- detail ways in which this PR has been tested or needs to be tested
-->
* [x] Acceptance tests that prove the issue has been fixed for every
privilege-granting resource
  • Loading branch information
@sfc-gh-jcieslak
sfc-gh-jcieslak authored Apr 3, 2024
1 parent 918873d commit f0018c6
Show file tree
Hide file tree
Showing 4 changed files with 365 additions and 25 deletions.
38 changes: 25 additions & 13 deletions pkg/resources/grant_privileges_to_account_role.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,10 @@ func UpdateGrantPrivilegesToAccountRole(ctx context.Context, d *schema.ResourceD
}
logging.DebugLogger.Printf("[DEBUG] Parsed identifier to %s", id.String())

if d.HasChange("with_grant_option") {
id.WithGrantOption = d.Get("with_grant_option").(bool)
}

// handle all_privileges -> privileges change (revoke all privileges)
if d.HasChange("all_privileges") {
_, allPrivileges := d.GetChange("all_privileges")
Expand Down Expand Up @@ -512,20 +516,28 @@ func UpdateGrantPrivilegesToAccountRole(ctx context.Context, d *schema.ResourceD

if len(privilegesToAdd) > 0 {
logging.DebugLogger.Printf("[DEBUG] Granting privileges: %v", privilegesToAdd)
err = client.Grants.GrantPrivilegesToAccountRole(
ctx,
getAccountRolePrivileges(
false,
privilegesToAdd,
id.Kind == OnAccountAccountRoleGrantKind,
id.Kind == OnAccountObjectAccountRoleGrantKind,
id.Kind == OnSchemaAccountRoleGrantKind,
id.Kind == OnSchemaObjectAccountRoleGrantKind,
),
grantOn,
id.RoleName,
new(sdk.GrantPrivilegesToAccountRoleOptions),
privilegesToGrant := getAccountRolePrivileges(
false,
privilegesToAdd,
id.Kind == OnAccountAccountRoleGrantKind,
id.Kind == OnAccountObjectAccountRoleGrantKind,
id.Kind == OnSchemaAccountRoleGrantKind,
id.Kind == OnSchemaObjectAccountRoleGrantKind,
)

if !id.WithGrantOption {
if err = client.Grants.RevokePrivilegesFromAccountRole(ctx, privilegesToGrant, grantOn, id.RoleName, new(sdk.RevokePrivilegesFromAccountRoleOptions)); err != nil {
return diag.Diagnostics{
diag.Diagnostic{
Severity: diag.Error,
Summary: "Failed to revoke privileges to add",
Detail: fmt.Sprintf("Id: %s\nPrivileges to add: %v\nError: %s", d.Id(), privilegesToAdd, err.Error()),
},
}
}
}

err = client.Grants.GrantPrivilegesToAccountRole(ctx, privilegesToGrant, grantOn, id.RoleName, &sdk.GrantPrivilegesToAccountRoleOptions{WithGrantOption: sdk.Bool(id.WithGrantOption)})
if err != nil {
return diag.Diagnostics{
diag.Diagnostic{
Expand Down
168 changes: 168 additions & 0 deletions pkg/resources/grant_privileges_to_account_role_acceptance_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1190,6 +1190,174 @@ func TestAcc_GrantPrivilegesToAccountRole_MLPrivileges(t *testing.T) {
})
}

// proves https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2459 is fixed
func TestAcc_GrantPrivilegesToAccountRole_ChangeWithGrantOptionsOutsideOfTerraform_WithGrantOptions(t *testing.T) {
name := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha))
roleName := sdk.NewAccountObjectIdentifier(name).FullyQualifiedName()
tableName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha))

configVariables := config.Variables{
"name": config.StringVariable(roleName),
"table_name": config.StringVariable(tableName),
"privileges": config.ListVariable(
config.StringVariable(sdk.SchemaObjectPrivilegeTruncate.String()),
),
"database": config.StringVariable(acc.TestDatabaseName),
"schema": config.StringVariable(acc.TestSchemaName),
"with_grant_option": config.BoolVariable(true),
}

resource.Test(t, resource.TestCase{
ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
PreCheck: func() { acc.TestAccPreCheck(t) },
TerraformVersionChecks: []tfversion.TerraformVersionCheck{
tfversion.RequireAbove(tfversion.Version1_5_0),
},
CheckDestroy: testAccCheckAccountRolePrivilegesRevoked(name),
Steps: []resource.TestStep{
{
PreConfig: func() { createAccountRoleOutsideTerraform(t, name) },
ConfigPlanChecks: resource.ConfigPlanChecks{
PostApplyPostRefresh: []plancheck.PlanCheck{
plancheck.ExpectEmptyPlan(),
},
},
ConfigDirectory: acc.ConfigurationDirectory("TestAcc_GrantPrivilegesToAccountRole/OnSchemaObject_OnObject"),
ConfigVariables: configVariables,
},
{
PreConfig: func() {
revokeAndGrantPrivilegesOnTableToAccountRole(
t, name,
sdk.NewSchemaObjectIdentifier(acc.TestDatabaseName, acc.TestSchemaName, tableName),
[]sdk.SchemaObjectPrivilege{sdk.SchemaObjectPrivilegeTruncate},
false,
)
},
ConfigPlanChecks: resource.ConfigPlanChecks{
PostApplyPostRefresh: []plancheck.PlanCheck{
plancheck.ExpectEmptyPlan(),
},
},
ConfigDirectory: acc.ConfigurationDirectory("TestAcc_GrantPrivilegesToAccountRole/OnSchemaObject_OnObject"),
ConfigVariables: configVariables,
},
},
})
}

// proves https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2459 is fixed
func TestAcc_GrantPrivilegesToAccountRole_ChangeWithGrantOptionsOutsideOfTerraform_WithoutGrantOptions(t *testing.T) {
name := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha))
roleName := sdk.NewAccountObjectIdentifier(name).FullyQualifiedName()
tableName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha))

configVariables := config.Variables{
"name": config.StringVariable(roleName),
"table_name": config.StringVariable(tableName),
"privileges": config.ListVariable(
config.StringVariable(sdk.SchemaObjectPrivilegeTruncate.String()),
),
"database": config.StringVariable(acc.TestDatabaseName),
"schema": config.StringVariable(acc.TestSchemaName),
"with_grant_option": config.BoolVariable(false),
}

resource.Test(t, resource.TestCase{
ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
PreCheck: func() { acc.TestAccPreCheck(t) },
TerraformVersionChecks: []tfversion.TerraformVersionCheck{
tfversion.RequireAbove(tfversion.Version1_5_0),
},
CheckDestroy: testAccCheckAccountRolePrivilegesRevoked(name),
Steps: []resource.TestStep{
{
PreConfig: func() { createAccountRoleOutsideTerraform(t, name) },
ConfigPlanChecks: resource.ConfigPlanChecks{
PostApplyPostRefresh: []plancheck.PlanCheck{
plancheck.ExpectEmptyPlan(),
},
},
ConfigDirectory: acc.ConfigurationDirectory("TestAcc_GrantPrivilegesToAccountRole/OnSchemaObject_OnObject"),
ConfigVariables: configVariables,
},
{
PreConfig: func() {
revokeAndGrantPrivilegesOnTableToAccountRole(
t, name,
sdk.NewSchemaObjectIdentifier(acc.TestDatabaseName, acc.TestSchemaName, tableName),
[]sdk.SchemaObjectPrivilege{sdk.SchemaObjectPrivilegeTruncate},
true,
)
},
ConfigPlanChecks: resource.ConfigPlanChecks{
PostApplyPostRefresh: []plancheck.PlanCheck{
plancheck.ExpectEmptyPlan(),
},
},
ConfigDirectory: acc.ConfigurationDirectory("TestAcc_GrantPrivilegesToAccountRole/OnSchemaObject_OnObject"),
ConfigVariables: configVariables,
},
},
})
}

func revokeAndGrantPrivilegesOnTableToAccountRole(
t *testing.T,
accountRoleName string,
tableName sdk.SchemaObjectIdentifier,
privileges []sdk.SchemaObjectPrivilege,
withGrantOption bool,
) {
t.Helper()
client, err := sdk.NewDefaultClient()
if err != nil {
t.Fatal(err)
}
ctx := context.Background()
err = client.Grants.RevokePrivilegesFromAccountRole(
ctx,
&sdk.AccountRoleGrantPrivileges{
SchemaObjectPrivileges: privileges,
},
&sdk.AccountRoleGrantOn{
SchemaObject: &sdk.GrantOnSchemaObject{
SchemaObject: &sdk.Object{
ObjectType: sdk.ObjectTypeTable,
Name: tableName,
},
},
},
sdk.NewAccountObjectIdentifier(accountRoleName),
new(sdk.RevokePrivilegesFromAccountRoleOptions),
)
if err != nil {
t.Fatal(err)
}

err = client.Grants.GrantPrivilegesToAccountRole(
ctx,
&sdk.AccountRoleGrantPrivileges{
SchemaObjectPrivileges: privileges,
},
&sdk.AccountRoleGrantOn{
SchemaObject: &sdk.GrantOnSchemaObject{
SchemaObject: &sdk.Object{
ObjectType: sdk.ObjectTypeTable,
Name: tableName,
},
},
},
sdk.NewAccountObjectIdentifier(accountRoleName),
&sdk.GrantPrivilegesToAccountRoleOptions{
WithGrantOption: sdk.Bool(withGrantOption),
},
)
if err != nil {
t.Fatal(err)
}
}

func getSecondaryAccountName(t *testing.T) (string, error) {
t.Helper()
config, err := sdk.ProfileConfig(testprofiles.Secondary)
Expand Down
36 changes: 24 additions & 12 deletions pkg/resources/grant_privileges_to_database_role.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,10 @@ func UpdateGrantPrivilegesToDatabaseRole(ctx context.Context, d *schema.Resource
}
}

if d.HasChange("with_grant_option") {
id.WithGrantOption = d.Get("with_grant_option").(bool)
}

// handle all_privileges -> privileges change (revoke all privileges)
if d.HasChange("all_privileges") {
_, allPrivileges := d.GetChange("all_privileges")
Expand Down Expand Up @@ -439,19 +443,27 @@ func UpdateGrantPrivilegesToDatabaseRole(ctx context.Context, d *schema.Resource
grantOn := getDatabaseRoleGrantOn(d)

if len(privilegesToAdd) > 0 {
err = client.Grants.GrantPrivilegesToDatabaseRole(
ctx,
getDatabaseRolePrivileges(
false,
privilegesToAdd,
id.Kind == OnDatabaseDatabaseRoleGrantKind,
id.Kind == OnSchemaDatabaseRoleGrantKind,
id.Kind == OnSchemaObjectDatabaseRoleGrantKind,
),
grantOn,
id.DatabaseRoleName,
new(sdk.GrantPrivilegesToDatabaseRoleOptions),
privilegesToGrant := getDatabaseRolePrivileges(
false,
privilegesToAdd,
id.Kind == OnDatabaseDatabaseRoleGrantKind,
id.Kind == OnSchemaDatabaseRoleGrantKind,
id.Kind == OnSchemaObjectDatabaseRoleGrantKind,
)

if !id.WithGrantOption {
if err = client.Grants.RevokePrivilegesFromDatabaseRole(ctx, privilegesToGrant, grantOn, id.DatabaseRoleName, new(sdk.RevokePrivilegesFromDatabaseRoleOptions)); err != nil {
return diag.Diagnostics{
diag.Diagnostic{
Severity: diag.Error,
Summary: "Failed to revoke privileges to add",
Detail: fmt.Sprintf("Id: %s\nPrivileges to add: %v\nError: %s", d.Id(), privilegesToAdd, err.Error()),
},
}
}
}

err = client.Grants.GrantPrivilegesToDatabaseRole(ctx, privilegesToGrant, grantOn, id.DatabaseRoleName, &sdk.GrantPrivilegesToDatabaseRoleOptions{WithGrantOption: sdk.Bool(id.WithGrantOption)})
if err != nil {
return diag.Diagnostics{
diag.Diagnostic{
Expand Down
Loading

0 comments on commit f0018c6

Please sign in to comment.