Skip to content

Commit

Permalink
fix: Fix handling secure views read and import (#2655)
Browse files Browse the repository at this point in the history
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:
#2640 (comment)
  • Loading branch information
sfc-gh-asawicki authored Mar 27, 2024
1 parent d4f8bbc commit 3c3ede6
Show file tree
Hide file tree
Showing 4 changed files with 129 additions and 14 deletions.
2 changes: 1 addition & 1 deletion docs/resources/view.md
Original file line number Diff line number Diff line change
Expand Up @@ -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))

Expand Down
25 changes: 14 additions & 11 deletions pkg/resources/view.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
114 changes: 112 additions & 2 deletions pkg/resources/view_acceptance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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" {
Expand Down Expand Up @@ -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 {
Expand All @@ -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: &currentGrants})
require.NoError(t, err)
}
2 changes: 2 additions & 0 deletions pkg/snowflake/parser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down

0 comments on commit 3c3ede6

Please sign in to comment.