Skip to content

Commit

Permalink
chore: Replace parsing function for saving granted object names (#2813)
Browse files Browse the repository at this point in the history 
Add better parsing of identifiers returned by SHOW GRANTS. It should
resolve some of our issues with failing tests (they sometimes fail
because they depend on the generated name of a resource and whether they
will be quoted in the output provided by Snowflake).

New `ParseObjectIdentifier` function is based on
`DecodeSnowflakeParameterID` function used in resources. As a fallback
(because `ParseObjectIdentifier` can fail)
`NewObjectIdentifierFromFullyQualifiedName` will be used.
  • Loading branch information
@sfc-gh-jcieslak
sfc-gh-jcieslak authored May 16, 2024
1 parent 838d241 commit 175cfc7
Show file tree
Hide file tree
Showing 4 changed files with 104 additions and 29 deletions.
57 changes: 29 additions & 28 deletions pkg/resources/grant_ownership_acceptance_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func TestAcc_GrantOwnership_OnObject_Database_ToAccountRole(t *testing.T) {
To: &sdk.ShowGrantsTo{
Role: accountRoleId,
},
}, sdk.ObjectTypeDatabase, accountRoleName, databaseName),
}, sdk.ObjectTypeDatabase, accountRoleName, databaseFullyQualifiedName),
),
},
{
Expand Down Expand Up @@ -102,7 +102,7 @@ func TestAcc_GrantOwnership_OnObject_Database_IdentifiersWithDots(t *testing.T)
To: &sdk.ShowGrantsTo{
Role: accountRoleId,
},
}, sdk.ObjectTypeDatabase, accountRoleName, databaseName),
}, sdk.ObjectTypeDatabase, accountRoleName, databaseFullyQualifiedName),
),
},
{
Expand Down Expand Up @@ -153,7 +153,7 @@ func TestAcc_GrantOwnership_OnObject_Schema_ToAccountRole(t *testing.T) {
To: &sdk.ShowGrantsTo{
Role: accountRoleId,
},
}, sdk.ObjectTypeSchema, accountRoleName, fmt.Sprintf("%s.%s", databaseName, schemaName)),
}, sdk.ObjectTypeSchema, accountRoleName, schemaFullyQualifiedName),
),
},
{
Expand Down Expand Up @@ -201,7 +201,7 @@ func TestAcc_GrantOwnership_OnObject_Schema_ToDatabaseRole(t *testing.T) {
To: &sdk.ShowGrantsTo{
DatabaseRole: sdk.NewDatabaseObjectIdentifier(databaseName, databaseRoleName),
},
}, sdk.ObjectTypeSchema, databaseRoleName, fmt.Sprintf("%s.%s", databaseName, schemaName)),
}, sdk.ObjectTypeSchema, databaseRoleName, schemaFullyQualifiedName),
),
},
{
Expand Down Expand Up @@ -253,7 +253,7 @@ func TestAcc_GrantOwnership_OnObject_Table_ToAccountRole(t *testing.T) {
To: &sdk.ShowGrantsTo{
Role: accountRoleId,
},
}, sdk.ObjectTypeTable, accountRoleName, fmt.Sprintf("%s.%s.%s", databaseName, schemaName, tableName)),
}, sdk.ObjectTypeTable, accountRoleName, tableFullyQualifiedName),
),
},
{
Expand Down Expand Up @@ -303,7 +303,7 @@ func TestAcc_GrantOwnership_OnObject_Table_ToDatabaseRole(t *testing.T) {
To: &sdk.ShowGrantsTo{
DatabaseRole: sdk.NewDatabaseObjectIdentifier(databaseName, databaseRoleName),
},
}, sdk.ObjectTypeTable, databaseRoleName, fmt.Sprintf("%s.%s.%s", databaseName, schemaName, tableName)),
}, sdk.ObjectTypeTable, databaseRoleName, tableFullyQualifiedName),
),
},
{
Expand All @@ -329,6 +329,8 @@ func TestAcc_GrantOwnership_OnAll_InDatabase_ToAccountRole(t *testing.T) {
schemaName := acc.TestClient().Ids.Alpha()
tableName := acc.TestClient().Ids.Alpha()
secondTableName := acc.TestClient().Ids.Alpha()
tableFullyQualifiedName := sdk.NewSchemaObjectIdentifier(databaseName, schemaName, tableName).FullyQualifiedName()
secondTableFullyQualifiedName := sdk.NewSchemaObjectIdentifier(databaseName, schemaName, secondTableName).FullyQualifiedName()

configVariables := config.Variables{
"account_role_name": config.StringVariable(accountRoleName),
Expand Down Expand Up @@ -358,7 +360,7 @@ func TestAcc_GrantOwnership_OnAll_InDatabase_ToAccountRole(t *testing.T) {
To: &sdk.ShowGrantsTo{
Role: accountRoleId,
},
}, sdk.ObjectTypeTable, accountRoleName, fmt.Sprintf("%s.%s.%s", databaseName, schemaName, tableName), fmt.Sprintf("%s.%s.%s", databaseName, schemaName, secondTableName)),
}, sdk.ObjectTypeTable, accountRoleName, tableFullyQualifiedName, secondTableFullyQualifiedName),
),
},
{
Expand All @@ -379,6 +381,8 @@ func TestAcc_GrantOwnership_OnAll_InSchema_ToAccountRole(t *testing.T) {

tableName := acc.TestClient().Ids.Alpha()
secondTableName := acc.TestClient().Ids.Alpha()
tableFullyQualifiedName := sdk.NewSchemaObjectIdentifier(databaseName, schemaName, tableName).FullyQualifiedName()
secondTableFullyQualifiedName := sdk.NewSchemaObjectIdentifier(databaseName, schemaName, secondTableName).FullyQualifiedName()

accountRoleId := acc.TestClient().Ids.RandomAccountObjectIdentifier()
accountRoleName := accountRoleId.Name()
Expand Down Expand Up @@ -412,7 +416,7 @@ func TestAcc_GrantOwnership_OnAll_InSchema_ToAccountRole(t *testing.T) {
To: &sdk.ShowGrantsTo{
Role: accountRoleId,
},
}, sdk.ObjectTypeTable, accountRoleName, fmt.Sprintf("%s.%s.%s", databaseName, schemaName, tableName), fmt.Sprintf("%s.%s.%s", databaseName, schemaName, secondTableName)),
}, sdk.ObjectTypeTable, accountRoleName, tableFullyQualifiedName, secondTableFullyQualifiedName),
),
},
{
Expand Down Expand Up @@ -461,7 +465,7 @@ func TestAcc_GrantOwnership_OnFuture_InDatabase_ToAccountRole(t *testing.T) {
In: &sdk.ShowGrantsIn{
Database: sdk.Pointer(databaseId),
},
}, sdk.ObjectTypeTable, accountRoleName, fmt.Sprintf("%s.<TABLE>", databaseName)),
}, sdk.ObjectTypeTable, accountRoleName, fmt.Sprintf(`"%s"."<TABLE>"`, databaseName)),
),
},
{
Expand Down Expand Up @@ -511,7 +515,7 @@ func TestAcc_GrantOwnership_OnFuture_InSchema_ToAccountRole(t *testing.T) {
In: &sdk.ShowGrantsIn{
Schema: sdk.Pointer(sdk.NewDatabaseObjectIdentifier(databaseName, schemaName)),
},
}, sdk.ObjectTypeTable, accountRoleName, fmt.Sprintf("%s.%s.<TABLE>", databaseName, schemaName)),
}, sdk.ObjectTypeTable, accountRoleName, fmt.Sprintf(`"%s"."%s"."<TABLE>"`, databaseName, schemaName)),
),
},
{
Expand Down Expand Up @@ -608,7 +612,7 @@ func TestAcc_GrantOwnership_TargetObjectRemovedOutsideTerraform(t *testing.T) {
To: &sdk.ShowGrantsTo{
Role: accountRoleId,
},
}, sdk.ObjectTypeDatabase, accountRoleName, databaseName),
}, sdk.ObjectTypeDatabase, accountRoleName, databaseFullyQualifiedName),
),
},
{
Expand Down Expand Up @@ -667,7 +671,7 @@ func TestAcc_GrantOwnership_AccountRoleRemovedOutsideTerraform(t *testing.T) {
To: &sdk.ShowGrantsTo{
Role: accountRoleId,
},
}, sdk.ObjectTypeDatabase, accountRoleName, databaseName),
}, sdk.ObjectTypeDatabase, accountRoleName, databaseFullyQualifiedName),
),
},
{
Expand Down Expand Up @@ -723,7 +727,7 @@ func TestAcc_GrantOwnership_OnMaterializedView(t *testing.T) {
To: &sdk.ShowGrantsTo{
Role: accountRoleId,
},
}, sdk.ObjectTypeMaterializedView, accountRoleName, fmt.Sprintf("%s.%s.%s", databaseName, schemaName, materializedViewName)),
}, sdk.ObjectTypeMaterializedView, accountRoleName, materializedViewFullyQualifiedName),
),
},
{
Expand Down Expand Up @@ -870,7 +874,7 @@ func TestAcc_GrantOwnership_MoveOwnershipOutsideTerraform(t *testing.T) {
Name: databaseId,
},
},
}, sdk.ObjectTypeDatabase, accountRoleName, databaseName),
}, sdk.ObjectTypeDatabase, accountRoleName, databaseFullyQualifiedName),
),
},
},
Expand Down Expand Up @@ -964,8 +968,7 @@ func TestAcc_GrantOwnership_OnPipe(t *testing.T) {
Name: sdk.NewSchemaObjectIdentifierFromFullyQualifiedName(pipeFullyQualifiedName),
},
},
// TODO(SNOW-999049): Fix this identifier
}, sdk.ObjectTypePipe, accountRoleName, fmt.Sprintf("%s\".\"%s\".%s", acc.TestDatabaseName, acc.TestSchemaName, pipeName)),
}, sdk.ObjectTypePipe, accountRoleName, pipeFullyQualifiedName),
),
},
},
Expand All @@ -977,6 +980,8 @@ func TestAcc_GrantOwnership_OnAllPipes(t *testing.T) {
tableName := acc.TestClient().Ids.Alpha()
pipeName := acc.TestClient().Ids.Alpha()
secondPipeName := acc.TestClient().Ids.Alpha()
pipeFullyQualifiedName := sdk.NewSchemaObjectIdentifier(acc.TestDatabaseName, acc.TestSchemaName, pipeName).FullyQualifiedName()
secondPipeFullyQualifiedName := sdk.NewSchemaObjectIdentifier(acc.TestDatabaseName, acc.TestSchemaName, secondPipeName).FullyQualifiedName()

accountRoleId := acc.TestClient().Ids.RandomAccountObjectIdentifier()
accountRoleName := accountRoleId.Name()
Expand Down Expand Up @@ -1011,8 +1016,7 @@ func TestAcc_GrantOwnership_OnAllPipes(t *testing.T) {
To: &sdk.ShowGrantsTo{
Role: accountRoleId,
},
// TODO(SNOW-999049): Fix this identifier
}, sdk.ObjectTypePipe, accountRoleName, fmt.Sprintf("%s\".\"%s\".%s", acc.TestDatabaseName, acc.TestSchemaName, pipeName), fmt.Sprintf("%s\".\"%s\".%s", acc.TestDatabaseName, acc.TestSchemaName, secondPipeName)),
}, sdk.ObjectTypePipe, accountRoleName, pipeFullyQualifiedName, secondPipeFullyQualifiedName),
),
},
},
Expand Down Expand Up @@ -1058,8 +1062,7 @@ func TestAcc_GrantOwnership_OnTask(t *testing.T) {
Name: sdk.NewSchemaObjectIdentifierFromFullyQualifiedName(taskFullyQualifiedName),
},
},
// TODO(SNOW-999049): Fix this identifier
}, sdk.ObjectTypeTask, accountRoleName, fmt.Sprintf("%s\".\"%s\".%s", acc.TestDatabaseName, acc.TestSchemaName, taskName)),
}, sdk.ObjectTypeTask, accountRoleName, taskFullyQualifiedName),
),
},
},
Expand All @@ -1073,6 +1076,8 @@ func TestAcc_GrantOwnership_OnAllTasks(t *testing.T) {
accountRoleName := accountRoleId.Name()
accountRoleFullyQualifiedName := accountRoleId.FullyQualifiedName()
schemaFullyQualifiedName := acc.TestClient().Ids.SchemaId().FullyQualifiedName()
taskFullyQualifiedName := sdk.NewSchemaObjectIdentifier(acc.TestDatabaseName, acc.TestSchemaName, taskName).FullyQualifiedName()
secondTaskFullyQualifiedName := sdk.NewSchemaObjectIdentifier(acc.TestDatabaseName, acc.TestSchemaName, secondTaskName).FullyQualifiedName()

configVariables := config.Variables{
"account_role_name": config.StringVariable(accountRoleName),
Expand Down Expand Up @@ -1101,11 +1106,7 @@ func TestAcc_GrantOwnership_OnAllTasks(t *testing.T) {
Role: accountRoleId,
},
},
sdk.ObjectTypeTask, accountRoleName,
// TODO(SNOW-999049): Fix this identifier
fmt.Sprintf("%s\".\"%s\".%s", acc.TestDatabaseName, acc.TestSchemaName, taskName),
fmt.Sprintf("%s\".\"%s\".%s", acc.TestDatabaseName, acc.TestSchemaName, secondTaskName),
),
sdk.ObjectTypeTask, accountRoleName, taskFullyQualifiedName, secondTaskFullyQualifiedName),
),
},
},
Expand Down Expand Up @@ -1151,7 +1152,7 @@ func TestAcc_GrantOwnership_OnDatabaseRole(t *testing.T) {
Name: sdk.NewDatabaseObjectIdentifierFromFullyQualifiedName(databaseRoleFullyQualifiedName),
},
},
}, sdk.ObjectTypeRole, accountRoleName, fmt.Sprintf("%s.%s", databaseName, databaseRoleName)),
}, sdk.ObjectTypeRole, accountRoleName, databaseRoleFullyQualifiedName),
),
},
},
Expand Down Expand Up @@ -1194,8 +1195,8 @@ func checkResourceOwnershipIsGranted(opts *sdk.ShowGrantOptions, grantOn sdk.Obj
if grant.Privilege == "OWNERSHIP" &&
(grant.GrantedOn == grantOn || grant.GrantOn == grantOn) &&
grant.GranteeName.Name() == roleName &&
slices.Contains(objectNames, grant.Name.Name()) {
found = append(found, grant.Name.Name())
slices.Contains(objectNames, grant.Name.FullyQualifiedName()) {
found = append(found, grant.Name.FullyQualifiedName())
}
}

Expand Down
9 changes: 8 additions & 1 deletion pkg/sdk/grants.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -263,14 +263,21 @@ func (row grantRow) convert() *Grant {
grantOn = ObjectTypeExternalVolume
}

// TODO(SNOW-1058419): Change identifier parsing during identifiers rework
name, err := ParseObjectIdentifier(row.Name)
if err != nil {
log.Printf("Failed to parse identifier: %s", err)
name = NewObjectIdentifierFromFullyQualifiedName(row.Name)
}

return &Grant{
CreatedOn: row.CreatedOn,
Privilege: row.Privilege,
GrantedOn: grantedOn,
GrantOn: grantOn,
GrantedTo: grantedTo,
GrantTo: grantTo,
Name: NewAccountObjectIdentifier(strings.Trim(row.Name, "\"")),
Name: name,
GranteeName: granteeName,
GrantOption: row.GrantOption,
GrantedBy: NewAccountObjectIdentifier(row.GrantedBy),
Expand Down
28 changes: 28 additions & 0 deletions pkg/sdk/identifier_helpers.go
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package sdk

import (
"encoding/csv"
"fmt"
"strings"
)
Expand All @@ -14,6 +15,33 @@ type ObjectIdentifier interface {
FullyQualifiedName() string
}

// TODO(SNOW-999049): This function will be tested/improved/used more wiedely during the identifiers rework.
// Right now, the implementation is just a copy of DecodeSnowflakeParameterID used in resources.
func ParseObjectIdentifier(identifier string) (ObjectIdentifier, error) {
reader := csv.NewReader(strings.NewReader(identifier))
reader.Comma = '.'
lines, err := reader.ReadAll()
if err != nil {
return nil, fmt.Errorf("unable to read identifier: %s, err = %w", identifier, err)
}
if len(lines) != 1 {
return nil, fmt.Errorf("incompatible identifier: %s", identifier)
}
parts := lines[0]
switch len(parts) {
case 1:
return NewAccountObjectIdentifier(parts[0]), nil
case 2:
return NewDatabaseObjectIdentifier(parts[0], parts[1]), nil
case 3:
return NewSchemaObjectIdentifier(parts[0], parts[1], parts[2]), nil
case 4:
return NewTableColumnIdentifier(parts[0], parts[1], parts[2], parts[3]), nil
default:
return nil, fmt.Errorf("unable to classify identifier: %s", identifier)
}
}

func NewObjectIdentifierFromFullyQualifiedName(fullyQualifiedName string) ObjectIdentifier {
parts := strings.Split(fullyQualifiedName, ".")
switch len(parts) {
Expand Down
39 changes: 39 additions & 0 deletions pkg/sdk/testint/grants_integration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1752,8 +1752,10 @@ func TestInt_GrantOwnership(t *testing.T) {
func TestInt_ShowGrants(t *testing.T) {
client := testClient(t)
ctx := testContext(t)

shareTest, shareCleanup := testClientHelper().Share.CreateShare(t)
t.Cleanup(shareCleanup)

err := client.Grants.GrantPrivilegeToShare(ctx, []sdk.ObjectPrivilege{sdk.ObjectPrivilegeUsage}, &sdk.ShareGrantOn{
Database: testDb(t).ID(),
}, shareTest.ID())
Expand All @@ -1764,10 +1766,12 @@ func TestInt_ShowGrants(t *testing.T) {
}, shareTest.ID())
require.NoError(t, err)
})

t.Run("without options", func(t *testing.T) {
_, err := client.Grants.Show(ctx, nil)
require.NoError(t, err)
})

t.Run("with options", func(t *testing.T) {
grants, err := client.Grants.Show(ctx, &sdk.ShowGrantOptions{
On: &sdk.ShowGrantsOn{
Expand All @@ -1780,6 +1784,41 @@ func TestInt_ShowGrants(t *testing.T) {
require.NoError(t, err)
assert.LessOrEqual(t, 2, len(grants))
})

t.Run("handles unquoted granted object names", func(t *testing.T) {
columns := []sdk.TableColumnRequest{
*sdk.NewTableColumnRequest("id", sdk.DataTypeNumber),
}
// This name is returned as unquoted from Snowflake
name := "G6TM2"
table, tableCleanup := testClientHelper().Table.CreateTableWithColumns(t, testClientHelper().Ids.SchemaId(), name, columns)
t.Cleanup(tableCleanup)

role, roleCleanup := testClientHelper().Role.CreateRole(t)
t.Cleanup(roleCleanup)

privileges := &sdk.AccountRoleGrantPrivileges{
SchemaObjectPrivileges: []sdk.SchemaObjectPrivilege{sdk.SchemaObjectPrivilegeSelect},
}
on := &sdk.AccountRoleGrantOn{
SchemaObject: &sdk.GrantOnSchemaObject{
SchemaObject: &sdk.Object{
ObjectType: sdk.ObjectTypeTable,
Name: table.ID(),
},
},
}
err = client.Grants.GrantPrivilegesToAccountRole(ctx, privileges, on, role.ID(), nil)
require.NoError(t, err)

grants, err := client.Grants.Show(ctx, &sdk.ShowGrantOptions{
To: &sdk.ShowGrantsTo{
Role: role.ID(),
},
})
require.NoError(t, err)
assert.Equal(t, table.ID().FullyQualifiedName(), grants[0].Name.FullyQualifiedName())
})
}

func grantsToPrivileges(grants []sdk.Grant) []string {
Expand Down

0 comments on commit 175cfc7

Please sign in to comment.