From cdfdd218b1c05a5b9f0780ba6d8d4b814085bff1 Mon Sep 17 00:00:00 2001 From: Tomas Farkas Date: Mon, 2 Nov 2020 19:29:53 +0100 Subject: [PATCH] [fix] future priv (#281) ## Test Plan * [ ] acceptance tests * [x ] Test cases modified `table_grant_test.go` and `view_grant_test.go` ## References * Fixes https://github.com/chanzuckerberg/terraform-provider-snowflake/issues/279 * Some notes about the fix: as I am not really experienced with the provider I did not find any elegant solution how to exactly test that the `readGenericGrant` is executed on the `ReadTableGrant` or `ReadViewGrant` function call. So I had to reflect the builder interface but that was not enough, as the builder in both cases is FutureGrant. So I made a workaround by using a `validPrivileges` set, in VIEWS it contains only one item, the `SELECT` privilege. --- pkg/resources/grant_helpers.go | 28 ++++++- pkg/resources/schema_grant_acceptance_test.go | 79 ++++++++++++++++++- pkg/resources/table_grant_test.go | 9 +++ pkg/resources/view_grant_test.go | 10 +++ 4 files changed, 120 insertions(+), 6 deletions(-) diff --git a/pkg/resources/grant_helpers.go b/pkg/resources/grant_helpers.go index 91f22cea78..7d4e969a50 100644 --- a/pkg/resources/grant_helpers.go +++ b/pkg/resources/grant_helpers.go @@ -5,6 +5,7 @@ import ( "database/sql" "encoding/csv" "fmt" + "reflect" "strings" "time" @@ -213,6 +214,21 @@ func readGenericGrant(data *schema.ResourceData, meta interface{}, builder snowf priv := data.Get("privilege").(string) grantOption := data.Get("with_grant_option").(bool) + // This is the only way how I can test that this function is reading VIEW grants or TABLE grants + // is checking what kind of builder we have. If it is future grant, then I doubple check if the + // privilegeSet has only one member - SELECT - then it is a VIEW, if it has 6 members and contains + // Truncate then it must be Table + futureGrantOnViews := false + futureGrantOnTables := false + if reflect.TypeOf(builder) == reflect.TypeOf(&snowflake.FutureGrantBuilder{}) { + if _, ok := validPrivileges[privilegeSelect]; ok && len(validPrivileges) == 1 { + futureGrantOnViews = true + } + if _, ok := validPrivileges[privilegeTruncate]; ok && len(validPrivileges) == 6 { + futureGrantOnTables = true + } + } + // We re-aggregate grants that would be equivalent to the "ALL" grant grants = filterALLGrants(grants, validPrivileges) @@ -231,8 +247,16 @@ func readGenericGrant(data *schema.ResourceData, meta interface{}, builder snowf // If not there, create an empty set privileges = privilegeSet{} } - // Add privilege to the set - privileges.addString(grant.Privilege) + // Add privilege to the set but consider valid privileges only + // for VIEW in ReadViewGrant + // and for non-VIEW in ReadTableGrant + if futureGrantOnViews || futureGrantOnTables { + if (futureGrantOnViews && grant.GrantType == "VIEW") || (futureGrantOnTables && grant.GrantType == "TABLE") { + privileges.addString(grant.Privilege) + } + } else { // Other grants + privileges.addString(grant.Privilege) + } // Reassign set back rolePrivileges[roleName] = privileges case "SHARE": diff --git a/pkg/resources/schema_grant_acceptance_test.go b/pkg/resources/schema_grant_acceptance_test.go index 78b24dbb2c..5066b1852e 100644 --- a/pkg/resources/schema_grant_acceptance_test.go +++ b/pkg/resources/schema_grant_acceptance_test.go @@ -5,6 +5,8 @@ import ( "os" "testing" + "strings" + "github.com/hashicorp/terraform-plugin-sdk/helper/acctest" "github.com/hashicorp/terraform-plugin-sdk/helper/resource" ) @@ -14,9 +16,9 @@ func TestAccSchemaGrant(t *testing.T) { t.Skip("Skipping TestAccSchemaGrant") } - sName := acctest.RandStringFromCharSet(10, acctest.CharSetAlpha) - roleName := acctest.RandStringFromCharSet(10, acctest.CharSetAlpha) - shareName := acctest.RandStringFromCharSet(10, acctest.CharSetAlpha) + sName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)) + roleName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)) + shareName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)) resource.Test(t, resource.TestCase{ Providers: providers(), @@ -29,7 +31,7 @@ func TestAccSchemaGrant(t *testing.T) { resource.TestCheckResourceAttr("snowflake_schema_grant.test", "privilege", "USAGE"), ), }, - // FUTURE + // FUTURE SHARES { Config: schemaGrantConfig(sName, roleName, shareName, true), Check: resource.ComposeTestCheckFunc( @@ -48,6 +50,75 @@ func TestAccSchemaGrant(t *testing.T) { }) } +func TestAccSchemaFutureGrants(t *testing.T) { + + sName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)) + roleNameTable := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)) + roleNameView := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)) + + resource.Test(t, resource.TestCase{ + Providers: providers(), + Steps: []resource.TestStep{ + // TABLE AND VIEW FUTURE GRANTS + { + Config: futureTableAndViewGrantConfig(sName, roleNameTable, roleNameView), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_view_grant.select_on_future_views", "roles.#", "1"), + resource.TestCheckResourceAttr("snowflake_table_grant.select_on_future_tables", "roles.#", "1"), + resource.TestCheckResourceAttr("snowflake_view_grant.select_on_future_views", "privilege", "SELECT"), + ), + }, + // IMPORT + { + ResourceName: "snowflake_view_grant.select_on_future_views", + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + +func futureTableAndViewGrantConfig(n, role_table, role_view string) string { + return fmt.Sprintf(` +resource "snowflake_database" "test" { + name = "%v" +} + +resource "snowflake_schema" "test" { + name = "%v" + database = snowflake_database.test.name + comment = "Terraform acceptance test" +} + +resource "snowflake_role" "table_reader" { + name = "%v" +} + +resource "snowflake_role" "view_reader" { + name = "%v" +} + +resource "snowflake_table_grant" "select_on_future_tables" { + database_name = snowflake_database.test.name + schema_name = snowflake_schema.test.name + privilege = "SELECT" + on_future = true + roles = [snowflake_role.table_reader.name] + depends_on = [snowflake_schema.test, snowflake_role.table_reader] +} + +resource "snowflake_view_grant" "select_on_future_views" { + database_name = snowflake_database.test.name + schema_name = snowflake_schema.test.name + privilege = "SELECT" + on_future = true + roles = [snowflake_role.view_reader.name] + depends_on = [snowflake_schema.test, snowflake_role.view_reader] +} + +`, n, n, role_table, role_view) +} + func schemaGrantConfig(n, role, share string, future bool) string { schema_name_config := `schema_name = snowflake_schema.test.name shares = [snowflake_share.test.name]` diff --git a/pkg/resources/table_grant_test.go b/pkg/resources/table_grant_test.go index eae0e9f948..cc955b3883 100644 --- a/pkg/resources/table_grant_test.go +++ b/pkg/resources/table_grant_test.go @@ -116,6 +116,13 @@ func TestFutureTableGrantCreate(t *testing.T) { ).WillReturnResult(sqlmock.NewResult(1, 1)) expectReadFutureTableGrant(mock) err := resources.CreateTableGrant(d, db) + roles := d.Get("roles").(*schema.Set) + // After the CreateTableGrant has been created a ReadTableGrant reads the current grants + // and this read should ignore test-role-3 what is returned by SHOW FUTURE GRANTS ON SCHEMA PUBLIC because + // test-role-3 has been granted to a SELECT on future VIEW and not on future TABLE + r.True(roles.Contains("test-role-1")) + r.True(roles.Contains("test-role-2")) + r.False(roles.Contains("test-role-3")) r.NoError(err) }) @@ -151,6 +158,8 @@ func expectReadFutureTableGrant(mock sqlmock.Sqlmock) { time.Date(2000, 1, 1, 0, 0, 0, 0, time.UTC), "SELECT", "TABLE", "test-db.PUBLIC.", "ROLE", "test-role-1", false, ).AddRow( time.Date(2000, 1, 1, 0, 0, 0, 0, time.UTC), "SELECT", "TABLE", "test-db.PUBLIC.
", "ROLE", "test-role-2", false, + ).AddRow( + time.Date(2000, 1, 1, 0, 0, 0, 0, time.UTC), "SELECT", "VIEW", "test-db.PUBLIC.", "ROLE", "test-role-3", false, ) mock.ExpectQuery(`^SHOW FUTURE GRANTS IN SCHEMA "test-db"."PUBLIC"$`).WillReturnRows(rows) } diff --git a/pkg/resources/view_grant_test.go b/pkg/resources/view_grant_test.go index 534c492fcc..5c31e8af58 100644 --- a/pkg/resources/view_grant_test.go +++ b/pkg/resources/view_grant_test.go @@ -116,6 +116,14 @@ func TestFutureViewGrantCreate(t *testing.T) { expectReadFutureViewGrant(mock) err := resources.CreateViewGrant(d, db) r.NoError(err) + + roles := d.Get("roles").(*schema.Set) + // After the CreateFutureViewGrant has been created a ReadViewGrant reads the current grants + // and this read should ignore test-role-3 what is returned by SHOW FUTURE GRANTS ON SCHEMA PUBLIC because + // test-role-3 has been granted to a SELECT on future TABLE and not on future VIEW + r.True(roles.Contains("test-role-1")) + r.True(roles.Contains("test-role-2")) + r.False(roles.Contains("test-role-3")) }) b := require.New(t) @@ -150,6 +158,8 @@ func expectReadFutureViewGrant(mock sqlmock.Sqlmock) { time.Date(2000, 1, 1, 0, 0, 0, 0, time.UTC), "SELECT", "VIEW", "test-db.PUBLIC.", "ROLE", "test-role-1", false, ).AddRow( time.Date(2000, 1, 1, 0, 0, 0, 0, time.UTC), "SELECT", "VIEW", "test-db.PUBLIC.", "ROLE", "test-role-2", false, + ).AddRow( + time.Date(2000, 1, 1, 0, 0, 0, 0, time.UTC), "SELECT", "TABLE", "test-db.PUBLIC.
", "ROLE", "test-role-3", false, ) mock.ExpectQuery(`^SHOW FUTURE GRANTS IN SCHEMA "test-db"."PUBLIC"$`).WillReturnRows(rows) }