From 1863bf697f05177f27c351c0687c4bee24fe2c1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Cie=C5=9Blak?= Date: Thu, 5 Dec 2024 08:08:23 +0100 Subject: [PATCH] fix: Minor fixes (#3231) ## Changes - Add usage tracking to storage integration - Adjust the missing link in the documentation for grants - Address SNOW-1825671 issue --- .../grants_redesign_design_decisions.md | 2 +- ...ileges_to_database_role_acceptance_test.go | 12 +-- .../object_parameter_acceptance_test.go | 12 +++ pkg/resources/storage_integration.go | 89 +++++++++---------- pkg/sdk/grants_test.go | 14 --- pkg/sdk/grants_validations.go | 13 --- 6 files changed, 64 insertions(+), 78 deletions(-) diff --git a/docs/technical-documentation/grants_redesign_design_decisions.md b/docs/technical-documentation/grants_redesign_design_decisions.md index b70af89be8..ced7616159 100644 --- a/docs/technical-documentation/grants_redesign_design_decisions.md +++ b/docs/technical-documentation/grants_redesign_design_decisions.md @@ -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) diff --git a/pkg/resources/grant_privileges_to_database_role_acceptance_test.go b/pkg/resources/grant_privileges_to_database_role_acceptance_test.go index c1da413547..8b701424e4 100644 --- a/pkg/resources/grant_privileges_to_database_role_acceptance_test.go +++ b/pkg/resources/grant_privileges_to_database_role_acceptance_test.go @@ -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)), @@ -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())), ), }, { diff --git a/pkg/resources/object_parameter_acceptance_test.go b/pkg/resources/object_parameter_acceptance_test.go index a1f3021ba4..b03ff6450d 100644 --- a/pkg/resources/object_parameter_acceptance_test.go +++ b/pkg/resources/object_parameter_acceptance_test.go @@ -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{ @@ -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) diff --git a/pkg/resources/storage_integration.go b/pkg/resources/storage_integration.go index 85a40729d3..cd15e0cc6f 100644 --- a/pkg/resources/storage_integration.go +++ b/pkg/resources/storage_integration.go @@ -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" @@ -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{ @@ -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) @@ -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)) @@ -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) @@ -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 @@ -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 @@ -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)) } } } @@ -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("") diff --git a/pkg/sdk/grants_test.go b/pkg/sdk/grants_test.go index 82dfc4fff4..9f0823b769 100644 --- a/pkg/sdk/grants_test.go +++ b/pkg/sdk/grants_test.go @@ -1,8 +1,6 @@ package sdk import ( - "errors" - "fmt" "testing" ) @@ -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()) @@ -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()) diff --git a/pkg/sdk/grants_validations.go b/pkg/sdk/grants_validations.go index 2d1727ee85..16f36413c5 100644 --- a/pkg/sdk/grants_validations.go +++ b/pkg/sdk/grants_validations.go @@ -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...) }