Skip to content

Commit

Permalink
fix: Minor fixes (#3231)
Browse files Browse the repository at this point in the history 
## Changes
- Add usage tracking to storage integration
- Adjust the missing link in the documentation for grants
- Address SNOW-1825671 issue
  • Loading branch information
@sfc-gh-jcieslak
sfc-gh-jcieslak authored Dec 5, 2024
1 parent 40de9ae commit 1863bf6
Show file tree
Hide file tree
Showing 6 changed files with 64 additions and 78 deletions.
2 changes: 1 addition & 1 deletion docs/technical-documentation/grants_redesign_design_decisions.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ Here’s a list of resources and data sources we introduced during the grant red
- [snowflake_grant_privileges_to_account_role](https://registry.terraform.io/providers/Snowflake-Labs/snowflake/latest/docs/resources/grant_privileges_to_account_role)
- [snowflake_grant_account_role](https://registry.terraform.io/providers/Snowflake-Labs/snowflake/latest/docs/resources/grant_account_role)
- [snowflake_grant_database_role](https://registry.terraform.io/providers/Snowflake-Labs/snowflake/latest/docs/resources/grant_database_role)
- snowflake_grant_application_role (coming soon)
- [snowflake_grant_application_role](https://registry.terraform.io/providers/Snowflake-Labs/snowflake/latest/docs/resources/grant_application_role)
- [snowflake_grant_privileges_to_share](https://registry.terraform.io/providers/Snowflake-Labs/snowflake/latest/docs/resources/grant_privileges_to_share)
- [snowflake_grant_ownership](https://registry.terraform.io/providers/Snowflake-Labs/snowflake/latest/docs/resources/grant_ownership)

Expand Down
12 changes: 7 additions & 5 deletions pkg/resources/grant_privileges_to_database_role_acceptance_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ func TestAcc_GrantPrivilegesToDatabaseRole_OnDatabase(t *testing.T) {
configVariables := config.Variables{
"name": config.StringVariable(databaseRoleId.Name()),
"privileges": config.ListVariable(
config.StringVariable(string(sdk.AccountObjectPrivilegeApplyBudget)),
config.StringVariable(string(sdk.AccountObjectPrivilegeCreateSchema)),
config.StringVariable(string(sdk.AccountObjectPrivilegeModify)),
config.StringVariable(string(sdk.AccountObjectPrivilegeUsage)),
Expand All @@ -53,13 +54,14 @@ func TestAcc_GrantPrivilegesToDatabaseRole_OnDatabase(t *testing.T) {
ConfigVariables: configVariables,
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(resourceName, "database_role_name", databaseRoleId.FullyQualifiedName()),
resource.TestCheckResourceAttr(resourceName, "privileges.#", "3"),
resource.TestCheckResourceAttr(resourceName, "privileges.0", string(sdk.AccountObjectPrivilegeCreateSchema)),
resource.TestCheckResourceAttr(resourceName, "privileges.1", string(sdk.AccountObjectPrivilegeModify)),
resource.TestCheckResourceAttr(resourceName, "privileges.2", string(sdk.AccountObjectPrivilegeUsage)),
resource.TestCheckResourceAttr(resourceName, "privileges.#", "4"),
resource.TestCheckResourceAttr(resourceName, "privileges.0", string(sdk.AccountObjectPrivilegeApplyBudget)),
resource.TestCheckResourceAttr(resourceName, "privileges.1", string(sdk.AccountObjectPrivilegeCreateSchema)),
resource.TestCheckResourceAttr(resourceName, "privileges.2", string(sdk.AccountObjectPrivilegeModify)),
resource.TestCheckResourceAttr(resourceName, "privileges.3", string(sdk.AccountObjectPrivilegeUsage)),
resource.TestCheckResourceAttr(resourceName, "on_database", acc.TestClient().Ids.DatabaseId().FullyQualifiedName()),
resource.TestCheckResourceAttr(resourceName, "with_grant_option", "true"),
resource.TestCheckResourceAttr(resourceName, "id", fmt.Sprintf("%s|true|false|CREATE SCHEMA,MODIFY,USAGE|OnDatabase|%s", databaseRoleId.FullyQualifiedName(), acc.TestClient().Ids.DatabaseId().FullyQualifiedName())),
resource.TestCheckResourceAttr(resourceName, "id", fmt.Sprintf("%s|true|false|APPLYBUDGET,CREATE SCHEMA,MODIFY,USAGE|OnDatabase|%s", databaseRoleId.FullyQualifiedName(), acc.TestClient().Ids.DatabaseId().FullyQualifiedName())),
),
},
{
Expand Down
12 changes: 12 additions & 0 deletions pkg/resources/object_parameter_acceptance_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,16 @@ import (
)

func TestAcc_ObjectParameter(t *testing.T) {
_ = testenvs.GetOrSkipTest(t, testenvs.EnableAcceptance)

// TODO(SNOW-1528546): Remove after parameter-setting resources are using UNSET in the delete operation.
t.Cleanup(func() {
acc.TestClient().Database.Alter(t, acc.TestClient().Ids.DatabaseId(), &sdk.AlterDatabaseOptions{
Unset: &sdk.DatabaseUnset{
UserTaskTimeoutMs: sdk.Bool(true),
},
})
})
resource.Test(t, resource.TestCase{
ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
TerraformVersionChecks: []tfversion.TerraformVersionCheck{
Expand All @@ -34,6 +44,8 @@ func TestAcc_ObjectParameter(t *testing.T) {
}

func TestAcc_ObjectParameterAccount(t *testing.T) {
_ = testenvs.GetOrSkipTest(t, testenvs.EnableAcceptance)

// TODO(SNOW-1528546): Remove after parameter-setting resources are using UNSET in the delete operation.
t.Cleanup(func() {
acc.TestClient().Parameter.UnsetAccountParameter(t, sdk.AccountParameterDataRetentionTimeInDays)
Expand Down
89 changes: 44 additions & 45 deletions pkg/resources/storage_integration.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ import (
"slices"
"strings"

"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/provider/resources"
"github.com/hashicorp/terraform-plugin-sdk/v2/diag"

"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/provider"
"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/schemas"

Expand Down Expand Up @@ -112,10 +115,10 @@ var storageIntegrationSchema = map[string]*schema.Schema{
// StorageIntegration returns a pointer to the resource representing a storage integration.
func StorageIntegration() *schema.Resource {
return &schema.Resource{
Create: CreateStorageIntegration,
Read: ReadStorageIntegration,
Update: UpdateStorageIntegration,
Delete: DeleteStorageIntegration,
CreateContext: TrackingCreateWrapper(resources.StorageIntegration, CreateStorageIntegration),
ReadContext: TrackingReadWrapper(resources.StorageIntegration, ReadStorageIntegration),
UpdateContext: TrackingUpdateWrapper(resources.StorageIntegration, UpdateStorageIntegration),
DeleteContext: TrackingDeleteWrapper(resources.StorageIntegration, DeleteStorageIntegration),

Schema: storageIntegrationSchema,
Importer: &schema.ResourceImporter{
Expand All @@ -124,9 +127,8 @@ func StorageIntegration() *schema.Resource {
}
}

func CreateStorageIntegration(d *schema.ResourceData, meta any) error {
func CreateStorageIntegration(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics {
client := meta.(*provider.Context).Client
ctx := context.Background()

name := sdk.NewAccountObjectIdentifierFromFullyQualifiedName(d.Get("name").(string))
enabled := d.Get("enabled").(bool)
Expand Down Expand Up @@ -161,12 +163,12 @@ func CreateStorageIntegration(d *schema.ResourceData, meta any) error {
case slices.Contains(sdk.AllS3Protocols, sdk.S3Protocol(storageProvider)):
s3Protocol, err := sdk.ToS3Protocol(storageProvider)
if err != nil {
return err
return diag.FromErr(err)
}

v, ok := d.GetOk("storage_aws_role_arn")
if !ok {
return fmt.Errorf("if you use the S3 storage provider you must specify a storage_aws_role_arn")
return diag.FromErr(fmt.Errorf("if you use the S3 storage provider you must specify a storage_aws_role_arn"))
}

s3Params := sdk.NewS3StorageParamsRequest(s3Protocol, v.(string))
Expand All @@ -177,29 +179,28 @@ func CreateStorageIntegration(d *schema.ResourceData, meta any) error {
case storageProvider == "AZURE":
v, ok := d.GetOk("azure_tenant_id")
if !ok {
return fmt.Errorf("if you use the Azure storage provider you must specify an azure_tenant_id")
return diag.FromErr(fmt.Errorf("if you use the Azure storage provider you must specify an azure_tenant_id"))
}
req.WithAzureStorageProviderParams(*sdk.NewAzureStorageParamsRequest(sdk.String(v.(string))))
case storageProvider == "GCS":
req.WithGCSStorageProviderParams(*sdk.NewGCSStorageParamsRequest())
default:
return fmt.Errorf("unexpected provider %v", storageProvider)
return diag.FromErr(fmt.Errorf("unexpected provider %v", storageProvider))
}

if err := client.StorageIntegrations.Create(ctx, req); err != nil {
return fmt.Errorf("error creating storage integration: %w", err)
return diag.FromErr(fmt.Errorf("error creating storage integration: %w", err))
}

d.SetId(helpers.EncodeSnowflakeID(name))
return ReadStorageIntegration(d, meta)
return ReadStorageIntegration(ctx, d, meta)
}

func ReadStorageIntegration(d *schema.ResourceData, meta any) error {
func ReadStorageIntegration(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics {
client := meta.(*provider.Context).Client
ctx := context.Background()
id, ok := helpers.DecodeSnowflakeID(d.Id()).(sdk.AccountObjectIdentifier)
if !ok {
return fmt.Errorf("storage integration read, error decoding id: %s as sdk.AccountObjectIdentifier, got: %T", d.Id(), id)
return diag.FromErr(fmt.Errorf("storage integration read, error decoding id: %s as sdk.AccountObjectIdentifier, got: %T", d.Id(), id))
}

s, err := client.StorageIntegrations.ShowByID(ctx, id)
Expand All @@ -209,91 +210,90 @@ func ReadStorageIntegration(d *schema.ResourceData, meta any) error {
return nil
}
if err := d.Set(FullyQualifiedNameAttributeName, id.FullyQualifiedName()); err != nil {
return err
return diag.FromErr(err)
}

if s.Category != "STORAGE" {
return fmt.Errorf("expected %v to be a STORAGE integration, got %v", d.Id(), s.Category)
return diag.FromErr(fmt.Errorf("expected %v to be a STORAGE integration, got %v", d.Id(), s.Category))
}
if err := d.Set("name", s.Name); err != nil {
return err
return diag.FromErr(err)
}
if err := d.Set("type", s.StorageType); err != nil {
return err
return diag.FromErr(err)
}
if err := d.Set("created_on", s.CreatedOn.String()); err != nil {
return err
return diag.FromErr(err)
}
if err := d.Set("enabled", s.Enabled); err != nil {
return err
return diag.FromErr(err)
}
if err := d.Set("comment", s.Comment); err != nil {
return err
return diag.FromErr(err)
}

storageIntegrationProps, err := client.StorageIntegrations.Describe(ctx, id)
if err != nil {
return fmt.Errorf("could not describe storage integration (%s), err = %w", d.Id(), err)
return diag.FromErr(fmt.Errorf("could not describe storage integration (%s), err = %w", d.Id(), err))
}

for _, prop := range storageIntegrationProps {
switch prop.Name {
case "STORAGE_PROVIDER":
if err := d.Set("storage_provider", prop.Value); err != nil {
return err
return diag.FromErr(err)
}
case "STORAGE_ALLOWED_LOCATIONS":
if err := d.Set("storage_allowed_locations", strings.Split(prop.Value, ",")); err != nil {
return err
return diag.FromErr(err)
}
case "STORAGE_BLOCKED_LOCATIONS":
if prop.Value != "" {
if err := d.Set("storage_blocked_locations", strings.Split(prop.Value, ",")); err != nil {
return err
return diag.FromErr(err)
}
}
case "STORAGE_AWS_IAM_USER_ARN":
if err := d.Set("storage_aws_iam_user_arn", prop.Value); err != nil {
return err
return diag.FromErr(err)
}
case "STORAGE_AWS_OBJECT_ACL":
if prop.Value != "" {
if err := d.Set("storage_aws_object_acl", prop.Value); err != nil {
return err
return diag.FromErr(err)
}
}
case "STORAGE_AWS_ROLE_ARN":
if err := d.Set("storage_aws_role_arn", prop.Value); err != nil {
return err
return diag.FromErr(err)
}
case "STORAGE_AWS_EXTERNAL_ID":
if err := d.Set("storage_aws_external_id", prop.Value); err != nil {
return err
return diag.FromErr(err)
}
case "STORAGE_GCP_SERVICE_ACCOUNT":
if err := d.Set("storage_gcp_service_account", prop.Value); err != nil {
return err
return diag.FromErr(err)
}
case "AZURE_CONSENT_URL":
if err := d.Set("azure_consent_url", prop.Value); err != nil {
return err
return diag.FromErr(err)
}
case "AZURE_MULTI_TENANT_APP_NAME":
if err := d.Set("azure_multi_tenant_app_name", prop.Value); err != nil {
return err
return diag.FromErr(err)
}
}
}

return err
return diag.FromErr(err)
}

func UpdateStorageIntegration(d *schema.ResourceData, meta any) error {
func UpdateStorageIntegration(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics {
client := meta.(*provider.Context).Client
ctx := context.Background()
id, ok := helpers.DecodeSnowflakeID(d.Id()).(sdk.AccountObjectIdentifier)
if !ok {
return fmt.Errorf("storage integration update, error decoding id: %s as sdk.AccountObjectIdentifier, got: %T", d.Id(), id)
return diag.FromErr(fmt.Errorf("storage integration update, error decoding id: %s as sdk.AccountObjectIdentifier, got: %T", d.Id(), id))
}

var runSetStatement bool
Expand Down Expand Up @@ -327,7 +327,7 @@ func UpdateStorageIntegration(d *schema.ResourceData, meta any) error {
if len(v) == 0 {
if err := client.StorageIntegrations.Alter(ctx, sdk.NewAlterStorageIntegrationRequest(id).
WithUnset(*sdk.NewStorageIntegrationUnsetRequest().WithStorageBlockedLocations(true))); err != nil {
return fmt.Errorf("error unsetting storage_blocked_locations, err = %w", err)
return diag.FromErr(fmt.Errorf("error unsetting storage_blocked_locations, err = %w", err))
}
} else {
runSetStatement = true
Expand All @@ -352,7 +352,7 @@ func UpdateStorageIntegration(d *schema.ResourceData, meta any) error {
} else {
if err := client.StorageIntegrations.Alter(ctx, sdk.NewAlterStorageIntegrationRequest(id).
WithUnset(*sdk.NewStorageIntegrationUnsetRequest().WithStorageAwsObjectAcl(true))); err != nil {
return fmt.Errorf("error unsetting storage_aws_object_acl, err = %w", err)
return diag.FromErr(fmt.Errorf("error unsetting storage_aws_object_acl, err = %w", err))
}
}
}
Expand All @@ -367,22 +367,21 @@ func UpdateStorageIntegration(d *schema.ResourceData, meta any) error {

if runSetStatement {
if err := client.StorageIntegrations.Alter(ctx, sdk.NewAlterStorageIntegrationRequest(id).WithSet(*setReq)); err != nil {
return fmt.Errorf("error updating storage integration, err = %w", err)
return diag.FromErr(fmt.Errorf("error updating storage integration, err = %w", err))
}
}

return ReadStorageIntegration(d, meta)
return ReadStorageIntegration(ctx, d, meta)
}

func DeleteStorageIntegration(d *schema.ResourceData, meta any) error {
func DeleteStorageIntegration(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics {
client := meta.(*provider.Context).Client
ctx := context.Background()
id, ok := helpers.DecodeSnowflakeID(d.Id()).(sdk.AccountObjectIdentifier)
if !ok {
return fmt.Errorf("storage integration delete, error decoding id: %s as sdk.AccountObjectIdentifier, got: %T", d.Id(), id)
return diag.FromErr(fmt.Errorf("storage integration delete, error decoding id: %s as sdk.AccountObjectIdentifier, got: %T", d.Id(), id))
}
if err := client.StorageIntegrations.Drop(ctx, sdk.NewDropStorageIntegrationRequest(id)); err != nil {
return fmt.Errorf("error dropping storage integration (%s), err = %w", d.Id(), err)
return diag.FromErr(fmt.Errorf("error dropping storage integration (%s), err = %w", d.Id(), err))
}

d.SetId("")
Expand Down
14 changes: 0 additions & 14 deletions pkg/sdk/grants_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
package sdk

import (
"errors"
"fmt"
"testing"
)

Expand Down Expand Up @@ -476,12 +474,6 @@ func TestGrants_GrantPrivilegesToDatabaseRole(t *testing.T) {
assertOptsInvalidJoinedErrors(t, opts, errExactlyOneOf("GrantOnSchemaObjectIn", "InDatabase", "InSchema"))
})

t.Run("validation: unsupported database privilege", func(t *testing.T) {
opts := defaultGrantsForDb()
opts.privileges.DatabasePrivileges = []AccountObjectPrivilege{AccountObjectPrivilegeCreateDatabaseRole}
assertOptsInvalidJoinedErrors(t, opts, fmt.Errorf("privilege CREATE DATABASE ROLE is not allowed"))
})

t.Run("on database", func(t *testing.T) {
opts := defaultGrantsForDb()
assertOptsValidAndSQLEquals(t, opts, `GRANT CREATE SCHEMA ON DATABASE %s TO DATABASE ROLE %s`, dbId.FullyQualifiedName(), databaseRoleId.FullyQualifiedName())
Expand Down Expand Up @@ -673,12 +665,6 @@ func TestGrants_RevokePrivilegesFromDatabaseRoleRole(t *testing.T) {
assertOptsInvalidJoinedErrors(t, opts, errExactlyOneOf("GrantOnSchemaObjectIn", "InDatabase", "InSchema"))
})

t.Run("validation: unsupported database privilege", func(t *testing.T) {
opts := defaultGrantsForDb()
opts.privileges.DatabasePrivileges = []AccountObjectPrivilege{AccountObjectPrivilegeCreateDatabaseRole}
assertOptsInvalidJoinedErrors(t, opts, errors.New("privilege CREATE DATABASE ROLE is not allowed"))
})

t.Run("on database", func(t *testing.T) {
opts := defaultGrantsForDb()
assertOptsValidAndSQLEquals(t, opts, `REVOKE CREATE SCHEMA ON DATABASE %s FROM DATABASE ROLE %s`, dbId.FullyQualifiedName(), databaseRoleId.FullyQualifiedName())
Expand Down
13 changes: 0 additions & 13 deletions pkg/sdk/grants_validations.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -293,19 +293,6 @@ func (v *DatabaseRoleGrantPrivileges) validate() error {
if !exactlyOneValueSet(v.DatabasePrivileges, v.SchemaPrivileges, v.SchemaObjectPrivileges, v.AllPrivileges) {
errs = append(errs, errExactlyOneOf("DatabaseRoleGrantPrivileges", "DatabasePrivileges", "SchemaPrivileges", "SchemaObjectPrivileges", "AllPrivileges"))
}
if valueSet(v.DatabasePrivileges) {
allowedPrivileges := []AccountObjectPrivilege{
AccountObjectPrivilegeCreateSchema,
AccountObjectPrivilegeModify,
AccountObjectPrivilegeMonitor,
AccountObjectPrivilegeUsage,
}
for _, p := range v.DatabasePrivileges {
if !slices.Contains(allowedPrivileges, p) {
errs = append(errs, fmt.Errorf("privilege %s is not allowed", p.String()))
}
}
}
return errors.Join(errs...)
}

Expand Down

0 comments on commit 1863bf6

Please sign in to comment.