From 3c3ede6b1a0e1c752c8435457c92db6200f0e7b6 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Wed, 27 Mar 2024 10:14:29 +0100 Subject: [PATCH] fix: Fix handling secure views read and import (#2655) To import the secure view the role owning the view is needed. As https://docs.snowflake.com/en/sql-reference/sql/create-view#usage-notes state: > By design, the [SHOW VIEWS](https://docs.snowflake.com/en/sql-reference/sql/show-views) command does not provide information about secure views. To view information about a secure view, you must use the [VIEWS](https://docs.snowflake.com/en/sql-reference/info-schema/views) view in the Information Schema and you must use the role that owns the view. This change: - tests and proves #2640 - checks the parser for view using union statement (that was the first idea for the source of error) - handles the empty `text` from `SHOW VIEWS` command gracefully (no panic like before) - adds a proper description with documentation link to the `snowflake_view` documentation References: https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2640#issuecomment-2020739114 --- docs/resources/view.md | 2 +- pkg/resources/view.go | 25 +++--- pkg/resources/view_acceptance_test.go | 114 +++++++++++++++++++++++++- pkg/snowflake/parser_test.go | 2 + 4 files changed, 129 insertions(+), 14 deletions(-) diff --git a/docs/resources/view.md b/docs/resources/view.md index 012eaab0c4..04d41b8c03 100644 --- a/docs/resources/view.md +++ b/docs/resources/view.md @@ -41,7 +41,7 @@ SQL - `comment` (String) Specifies a comment for the view. - `copy_grants` (Boolean) Retains the access permissions from the original view when a new view is created using the OR REPLACE clause. OR REPLACE must be set when COPY GRANTS is set. -- `is_secure` (Boolean) Specifies that the view is secure. +- `is_secure` (Boolean) Specifies that the view is secure. By design, the Snowflake's `SHOW VIEWS` command does not provide information about secure views (consult [view usage notes](https://docs.snowflake.com/en/sql-reference/sql/create-view#usage-notes)) which is essential to manage/import view with Terraform. Use the role owning the view while managing secure views. - `or_replace` (Boolean) Overwrites the View if it exists. - `tag` (Block List, Deprecated) Definitions of a tag to associate with the resource. (see [below for nested schema](#nestedblock--tag)) diff --git a/pkg/resources/view.go b/pkg/resources/view.go index 5563b0a9ff..30564b8e8d 100644 --- a/pkg/resources/view.go +++ b/pkg/resources/view.go @@ -55,7 +55,7 @@ var viewSchema = map[string]*schema.Schema{ Type: schema.TypeBool, Optional: true, Default: false, - Description: "Specifies that the view is secure.", + Description: "Specifies that the view is secure. By design, the Snowflake's `SHOW VIEWS` command does not provide information about secure views (consult [view usage notes](https://docs.snowflake.com/en/sql-reference/sql/create-view#usage-notes)) which is essential to manage/import view with Terraform. Use the role owning the view while managing secure views.", }, "comment": { Type: schema.TypeString, @@ -174,16 +174,19 @@ func ReadView(d *schema.ResourceData, meta interface{}) error { return err } - // TODO [SNOW-867235]: what do we do with these extractors (added as discussion topic)? - // Want to only capture the SELECT part of the query because before that is the CREATE part of the view. - extractor := snowflake.NewViewSelectStatementExtractor(view.Text) - substringOfQuery, err := extractor.Extract() - if err != nil { - return err - } - - if err = d.Set("statement", substringOfQuery); err != nil { - return err + if view.Text != "" { + // TODO [SNOW-867235]: what do we do with these extractors (added as discussion topic)? + // Want to only capture the SELECT part of the query because before that is the CREATE part of the view. + extractor := snowflake.NewViewSelectStatementExtractor(view.Text) + substringOfQuery, err := extractor.Extract() + if err != nil { + return err + } + if err = d.Set("statement", substringOfQuery); err != nil { + return err + } + } else { + return fmt.Errorf("error reading view %v, err = %w, `text` is missing; if the view is secure then the role used by the provider must own the view (consult https://docs.snowflake.com/en/sql-reference/sql/create-view#usage-notes)", d.Id(), err) } return nil diff --git a/pkg/resources/view_acceptance_test.go b/pkg/resources/view_acceptance_test.go index 9e03c9071b..13e1f8597d 100644 --- a/pkg/resources/view_acceptance_test.go +++ b/pkg/resources/view_acceptance_test.go @@ -7,16 +7,16 @@ import ( "strings" "testing" - "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/provider" - acc "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/provider" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk" "github.com/hashicorp/terraform-plugin-testing/config" "github.com/hashicorp/terraform-plugin-testing/helper/acctest" "github.com/hashicorp/terraform-plugin-testing/helper/resource" "github.com/hashicorp/terraform-plugin-testing/terraform" "github.com/hashicorp/terraform-plugin-testing/tfversion" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -415,6 +415,54 @@ func TestAcc_View_copyGrants(t *testing.T) { }) } +func TestAcc_View_Issue2640(t *testing.T) { + viewName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)) + part1 := "SELECT ROLE_NAME, ROLE_OWNER FROM INFORMATION_SCHEMA.APPLICABLE_ROLES" + part2 := "SELECT ROLE_OWNER, ROLE_NAME FROM INFORMATION_SCHEMA.APPLICABLE_ROLES" + roleName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)) + + resource.Test(t, resource.TestCase{ + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + PreCheck: func() { acc.TestAccPreCheck(t) }, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + CheckDestroy: testAccCheckViewDestroy, + Steps: []resource.TestStep{ + { + Config: viewConfigWithMultilineUnionStatement(acc.TestDatabaseName, acc.TestSchemaName, viewName, part1, part2), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_view.test", "name", viewName), + resource.TestCheckResourceAttr("snowflake_view.test", "statement", fmt.Sprintf("%s\n\tunion\n%s\n", part1, part2)), + resource.TestCheckResourceAttr("snowflake_view.test", "database", acc.TestDatabaseName), + resource.TestCheckResourceAttr("snowflake_view.test", "schema", acc.TestSchemaName), + ), + }, + // try to import secure view without being its owner (proves https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2640) + { + PreConfig: func() { + createAccountRoleOutsideTerraform(t, roleName) + registerAccountRoleCleanup(t, roleName) + alterViewOwnershipExternally(t, viewName, roleName) + }, + ResourceName: "snowflake_view.test", + ImportState: true, + ExpectError: regexp.MustCompile("`text` is missing; if the view is secure then the role used by the provider must own the view"), + }, + // import with the proper role + { + PreConfig: func() { + alterViewOwnershipExternally(t, viewName, "ACCOUNTADMIN") + }, + ResourceName: "snowflake_view.test", + ImportState: true, + ImportStateVerify: true, + ImportStateVerifyIgnore: []string{"or_replace"}, + }, + }, + }) +} + func viewConfigWithGrants(databaseName string, schemaName string, tableName string, viewName string, selectStatement string) string { return fmt.Sprintf(` resource "snowflake_table" "table" { @@ -499,6 +547,22 @@ resource "snowflake_view" "test" { `, databaseName, schemaName, name, selectStatement, orReplace) } +func viewConfigWithMultilineUnionStatement(databaseName string, schemaName string, name string, part1 string, part2 string) string { + return fmt.Sprintf(` +resource "snowflake_view" "test" { + name = "%[3]s" + database = "%[1]s" + schema = "%[2]s" + statement = <<-SQL +%[4]s + union +%[5]s +SQL + is_secure = true +} + `, databaseName, schemaName, name, part1, part2) +} + func testAccCheckViewDestroy(s *terraform.State) error { client := acc.TestAccProvider.Meta().(*provider.Context).Client for _, rs := range s.RootModule().Resources { @@ -525,3 +589,49 @@ func alterViewQueryExternally(t *testing.T, id sdk.SchemaObjectIdentifier, query err = client.Views.Create(ctx, sdk.NewCreateViewRequest(id, query).WithOrReplace(sdk.Bool(true))) require.NoError(t, err) } + +func registerAccountRoleCleanup(t *testing.T, roleName string) { + t.Helper() + + roleId := sdk.NewAccountObjectIdentifier(roleName) + + client, err := sdk.NewDefaultClient() + require.NoError(t, err) + ctx := context.Background() + + t.Cleanup(func() { + t.Logf("dropping account role (%s)", roleName) + // We remove the role, so the ownership will be changed back. The view will be deleted with db cleanup. + err = client.Roles.Drop(ctx, sdk.NewDropRoleRequest(roleId).WithIfExists(true)) + if err != nil { + t.Logf("failed to drop account role (%s), err = %s\n", roleName, err.Error()) + } + assert.Nil(t, err) + }) +} + +func alterViewOwnershipExternally(t *testing.T, viewName string, roleName string) { + t.Helper() + + viewId := sdk.NewSchemaObjectIdentifier(acc.TestDatabaseName, acc.TestSchemaName, viewName) + roleId := sdk.NewAccountObjectIdentifier(roleName) + + client, err := sdk.NewDefaultClient() + require.NoError(t, err) + ctx := context.Background() + + on := sdk.OwnershipGrantOn{ + Object: &sdk.Object{ + ObjectType: sdk.ObjectTypeView, + Name: viewId, + }, + } + to := sdk.OwnershipGrantTo{ + AccountRoleName: &roleId, + } + currentGrants := sdk.OwnershipCurrentGrants{ + OutboundPrivileges: sdk.Revoke, + } + err = client.Grants.GrantOwnership(ctx, on, to, &sdk.GrantOwnershipOptions{CurrentGrants: ¤tGrants}) + require.NoError(t, err) +} diff --git a/pkg/snowflake/parser_test.go b/pkg/snowflake/parser_test.go index 6f733f6d6e..14fa61fe4d 100644 --- a/pkg/snowflake/parser_test.go +++ b/pkg/snowflake/parser_test.go @@ -31,6 +31,7 @@ from bar;` identifier := `create view "foo"."bar"."bam" comment='asdf\'s are fun' as select * from bar;` full := `CREATE SECURE VIEW "rgdxfmnfhh"."PUBLIC"."rgdxfmnfhh" COMMENT = 'Terraform test resource' AS SELECT ROLE_NAME, ROLE_OWNER FROM INFORMATION_SCHEMA.APPLICABLE_ROLES` + issue2640 := `CREATE OR REPLACE SECURE VIEW "CLASSIFICATION" comment = 'Classification View of the union of classification tables' AS select * from AB1_SUBSCRIPTION.CLASSIFICATION.CLASSIFICATION union select * from AB2_SUBSCRIPTION.CLASSIFICATION.CLASSIFICATION` type args struct { input string @@ -55,6 +56,7 @@ from bar;` {"commentEscape", args{commentEscape}, "select * from bar;", false}, {"identifier", args{identifier}, "select * from bar;", false}, {"full", args{full}, "SELECT ROLE_NAME, ROLE_OWNER FROM INFORMATION_SCHEMA.APPLICABLE_ROLES", false}, + {"issue2640", args{issue2640}, "select * from AB1_SUBSCRIPTION.CLASSIFICATION.CLASSIFICATION union select * from AB2_SUBSCRIPTION.CLASSIFICATION.CLASSIFICATION", false}, } for _, tt := range tests { tt := tt