Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: User improvements #3034

Merged
merged 43 commits into from
Sep 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
17fba7e
rework users datasource starts here
sfc-gh-asawicki Sep 2, 2024
a45ac4f
Add user describe schema and mapper; rework users datasource
sfc-gh-asawicki Sep 2, 2024
10a3e7b
Pass really basic test
sfc-gh-asawicki Sep 2, 2024
6f1361e
Add comment to handle sensitive in show output schemas
sfc-gh-asawicki Sep 2, 2024
9fef8f1
Add a way to use show_output assertions in data sources test
sfc-gh-asawicki Sep 2, 2024
db4b05a
Add assertions to show output in users data source acc test
sfc-gh-asawicki Sep 2, 2024
09ae8f6
Add assertions to parameters in users data source acc test
sfc-gh-asawicki Sep 2, 2024
71af1da
Check user details
sfc-gh-asawicki Sep 2, 2024
e477ebf
Finish users datasource acceptance tests
sfc-gh-asawicki Sep 2, 2024
fa7156e
Add users data source changes to the migration guide
sfc-gh-asawicki Sep 2, 2024
1787d5e
Update users data source documentation
sfc-gh-asawicki Sep 2, 2024
0643851
Fix failing test
sfc-gh-asawicki Sep 2, 2024
728350f
fix custom diffs for fields with diff supression starts here
sfc-gh-asawicki Sep 2, 2024
f0ea064
Prepare new custom diff function taking the diff suppress into consid…
sfc-gh-asawicki Sep 2, 2024
31b17e7
Use the improved computed if any attribute changed implementation
sfc-gh-asawicki Sep 2, 2024
c54ad5e
Unit test improved computed if any attribute changed
sfc-gh-asawicki Sep 2, 2024
720a04b
Add negative test
sfc-gh-asawicki Sep 2, 2024
f616276
Add description
sfc-gh-asawicki Sep 2, 2024
480305a
Add important TODO
sfc-gh-asawicki Sep 2, 2024
31b2544
Hack resource data
sfc-gh-asawicki Sep 2, 2024
a05e041
Fix unit test
sfc-gh-asawicki Sep 2, 2024
dd66718
Fix hack impl
sfc-gh-asawicki Sep 2, 2024
3e41121
Fix old and new values passed to diff suppress
sfc-gh-asawicki Sep 2, 2024
99fa603
fix custom diffs for fields with diff supression starts here
sfc-gh-asawicki Sep 2, 2024
0d0d598
Prepare new custom diff function taking the diff suppress into consid…
sfc-gh-asawicki Sep 2, 2024
e356370
Use the improved computed if any attribute changed implementation
sfc-gh-asawicki Sep 2, 2024
1ae8ab4
Unit test improved computed if any attribute changed
sfc-gh-asawicki Sep 2, 2024
cddde7d
Add negative test
sfc-gh-asawicki Sep 2, 2024
3c36974
Add description
sfc-gh-asawicki Sep 2, 2024
4b90b49
Add important TODO
sfc-gh-asawicki Sep 2, 2024
a7cb8cf
Hack resource data
sfc-gh-asawicki Sep 2, 2024
50706e5
Fix unit test
sfc-gh-asawicki Sep 2, 2024
a9831f7
Fix hack impl
sfc-gh-asawicki Sep 2, 2024
85d1e27
Fix old and new values passed to diff suppress
sfc-gh-asawicki Sep 2, 2024
82649d4
Fix saml2 tests after changes
sfc-gh-asawicki Sep 3, 2024
4b5dc10
Fix warehouse and user
sfc-gh-asawicki Sep 3, 2024
0d9c1b0
Fix tests
sfc-gh-asawicki Sep 3, 2024
5563a6a
Merge remote-tracking branch 'origin/fix-custom-diffs-for-fields-with…
sfc-gh-jcieslak Sep 3, 2024
24aaeeb
rework users datasource starts here
sfc-gh-asawicki Sep 2, 2024
b803f0b
fix custom diffs for fields with diff supression starts here
sfc-gh-asawicki Sep 2, 2024
da45481
Add user improvements
sfc-gh-jcieslak Sep 3, 2024
fc1e8ce
Add user improvements
sfc-gh-jcieslak Sep 3, 2024
74107d8
Add user improvements
sfc-gh-jcieslak Sep 3, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions MIGRATION_GUIDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,10 @@ Validation changes:
- `default_secondary_roles` - only 1-element lists with `"ALL"` element are now supported. Check [Snowflake docs](https://docs.snowflake.com/en/sql-reference/sql/create-user#optional-object-properties-objectproperties) for more details.

#### *(breaking change)* refactored snowflake_users datasource
> **IMPORTANT NOTE:** when querying users you don't have permissions to, the querying options are limited.
You won't get almost any field in `show_output` (only empty or default values), the DESCRIBE command cannot be called, so you have to set `with_describe = false`.
Only `parameters` output is not affected by the lack of privileges.

Changes:
- account checking logic was entirely removed
- `pattern` renamed to `like`
Expand Down
5 changes: 3 additions & 2 deletions docs/data-sources/users.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@
page_title: "snowflake_users Data Source - terraform-provider-snowflake"
subcategory: ""
description: |-
Datasource used to get details of filtered users. Filtering is aligned with the current possibilities for SHOW USERS https://docs.snowflake.com/en/sql-reference/sql/show-users query. The results of SHOW, DESCRIBE, and SHOW PARAMETERS IN are encapsulated in one output collection.
Datasource used to get details of filtered users. Filtering is aligned with the current possibilities for SHOW USERS https://docs.snowflake.com/en/sql-reference/sql/show-users query. The results of SHOW, DESCRIBE, and SHOW PARAMETERS IN are encapsulated in one output collection. Important note is that when querying users you don't have permissions to, the querying options are limited. You won't get almost any field in show_output (only empty or default values), the DESCRIBE command cannot be called, so you have to set with_describe = false. Only parameters output is not affected by the lack of privileges.
---

!> **V1 release candidate** This data source was reworked and is a release candidate for the V1. We do not expect significant changes in it before the V1. We will welcome any feedback and adjust the data source if needed. Any errors reported will be resolved with a higher priority. We encourage checking this data source out before the V1 release. Please follow the [migration guide](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/MIGRATION_GUIDE.md#v0920--v0930) to use it.

# snowflake_users (Data Source)

Datasource used to get details of filtered users. Filtering is aligned with the current possibilities for [SHOW USERS](https://docs.snowflake.com/en/sql-reference/sql/show-users) query. The results of SHOW, DESCRIBE, and SHOW PARAMETERS IN are encapsulated in one output collection.
Datasource used to get details of filtered users. Filtering is aligned with the current possibilities for [SHOW USERS](https://docs.snowflake.com/en/sql-reference/sql/show-users) query. The results of SHOW, DESCRIBE, and SHOW PARAMETERS IN are encapsulated in one output collection. Important note is that when querying users you don't have permissions to, the querying options are limited. You won't get almost any field in `show_output` (only empty or default values), the DESCRIBE command cannot be called, so you have to set `with_describe = false`. Only `parameters` output is not affected by the lack of privileges.

## Example Usage

Expand Down Expand Up @@ -145,6 +145,7 @@ Read-Only:
- `ext_authn_duo` (Boolean)
- `ext_authn_uid` (String)
- `first_name` (String)
- `has_mfa` (Boolean)
- `last_name` (String)
- `login_name` (String)
- `middle_name` (String)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,17 @@ func (w *UserAssert) HasCreatedOnNotEmpty() *UserAssert {
return w
}

func (w *UserAssert) HasOwnerNotEmpty() *UserAssert {
w.AddAssertion(func(t *testing.T, o *sdk.User) error {
t.Helper()
if o.Owner == "" {
return fmt.Errorf("expected owner not empty; got empty")
}
return nil
})
return w
}

func (w *UserAssert) HasLastSuccessLoginEmpty() *UserAssert {
w.AddAssertion(func(t *testing.T, o *sdk.User) error {
t.Helper()
Expand Down
2 changes: 1 addition & 1 deletion pkg/datasources/users.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ func Users() *schema.Resource {
return &schema.Resource{
ReadContext: ReadUsers,
Schema: usersSchema,
Description: "Datasource used to get details of filtered users. Filtering is aligned with the current possibilities for [SHOW USERS](https://docs.snowflake.com/en/sql-reference/sql/show-users) query. The results of SHOW, DESCRIBE, and SHOW PARAMETERS IN are encapsulated in one output collection.",
Description: "Datasource used to get details of filtered users. Filtering is aligned with the current possibilities for [SHOW USERS](https://docs.snowflake.com/en/sql-reference/sql/show-users) query. The results of SHOW, DESCRIBE, and SHOW PARAMETERS IN are encapsulated in one output collection. Important note is that when querying users you don't have permissions to, the querying options are limited. You won't get almost any field in `show_output` (only empty or default values), the DESCRIBE command cannot be called, so you have to set `with_describe = false`. Only `parameters` output is not affected by the lack of privileges.",
}
}

Expand Down
4 changes: 3 additions & 1 deletion pkg/datasources/users_acceptance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ func TestAcc_Users_Complete(t *testing.T) {
HasDefaultSecondaryRoles(`["ALL"]`).
HasMinsToBypassMfaNotEmpty().
HasHasRsaPublicKey(true).
HasComment(comment),
HasComment(comment).
HasHasMfa(false),
resourceparametersassert.UsersDatasourceParameters(t, "snowflake_users.test").
HasAllDefaults(),
assert.Check(resource.TestCheckResourceAttr("data.snowflake_users.test", "users.0.describe_output.0.name", id.Name())),
Expand Down Expand Up @@ -113,6 +114,7 @@ func TestAcc_Users_Complete(t *testing.T) {
assert.Check(resource.TestCheckResourceAttrSet("data.snowflake_users.test", "users.0.describe_output.0.password_last_set_time")),
assert.Check(resource.TestCheckResourceAttr("data.snowflake_users.test", "users.0.describe_output.0.custom_landing_page_url", "")),
assert.Check(resource.TestCheckResourceAttr("data.snowflake_users.test", "users.0.describe_output.0.custom_landing_page_url_flush_next_ui_load", "false")),
assert.Check(resource.TestCheckResourceAttr("data.snowflake_users.test", "users.0.describe_output.0.has_mfa", "false")),
),
},
{
Expand Down
36 changes: 36 additions & 0 deletions pkg/internal/provider/sdkv2enhancements/resource_data.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package sdkv2enhancements

import (
"reflect"
"unsafe"

"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
)

// CreateResourceDataFromResourceDiff allows to create schema.ResourceData out of schema.ResourceDiff.
// Unfortunately, schema.resourceDiffer is an unexported interface, and functions like schema.SchemaDiffSuppressFunc do not use the interface but a concrete implementation.
// One use case in which we needed to have schema.ResourceData from schema.ResourceDiff was to run schema.SchemaDiffSuppressFunc from inside schema.CustomizeDiffFunc.
// This implementation uses:
// - schema.InternalMap that exposes hidden schema.schemaMap (a wrapper over map[string]*schema.Schema)
// - (m schemaMap) Data method allowing to create schema.ResourceData from terraform.InstanceState and terraform.InstanceDiff
// - terraform.InstanceState and terraform.InstanceDiff are unexported in schema.ResourceDiff, so we get them using reflection
func CreateResourceDataFromResourceDiff(resourceSchema schema.InternalMap, diff *schema.ResourceDiff) (*schema.ResourceData, bool) {
unexportedState := reflect.ValueOf(diff).Elem().FieldByName("state")
stateFromResourceDiff := reflect.NewAt(unexportedState.Type(), unsafe.Pointer(unexportedState.UnsafeAddr())).Elem().Interface()

Check failure on line 20 in pkg/internal/provider/sdkv2enhancements/resource_data.go

View workflow job for this annotation

GitHub Actions / reviewdog

[golangci] reported by reviewdog 🐶 G115: integer overflow conversion uintptr -> unsafe.Pointer (gosec) Raw Output: pkg/internal/provider/sdkv2enhancements/resource_data.go:20:79: G115: integer overflow conversion uintptr -> unsafe.Pointer (gosec) stateFromResourceDiff := reflect.NewAt(unexportedState.Type(), unsafe.Pointer(unexportedState.UnsafeAddr())).Elem().Interface() ^
unexportedDiff := reflect.ValueOf(diff).Elem().FieldByName("diff")
diffFroResourceDif := reflect.NewAt(unexportedDiff.Type(), unsafe.Pointer(unexportedDiff.UnsafeAddr())).Elem().Interface()

Check failure on line 22 in pkg/internal/provider/sdkv2enhancements/resource_data.go

View workflow job for this annotation

GitHub Actions / reviewdog

[golangci] reported by reviewdog 🐶 G115: integer overflow conversion uintptr -> unsafe.Pointer (gosec) Raw Output: pkg/internal/provider/sdkv2enhancements/resource_data.go:22:75: G115: integer overflow conversion uintptr -> unsafe.Pointer (gosec) diffFroResourceDif := reflect.NewAt(unexportedDiff.Type(), unsafe.Pointer(unexportedDiff.UnsafeAddr())).Elem().Interface() ^
castState, ok := stateFromResourceDiff.(*terraform.InstanceState)
if !ok {
return nil, false
}
castDiff, ok := diffFroResourceDif.(*terraform.InstanceDiff)
if !ok {
return nil, false
}
resourceData, err := resourceSchema.Data(castState, castDiff)
if err != nil {
return nil, false
}
return resourceData, true
}
2 changes: 1 addition & 1 deletion pkg/resources/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ func Account() *schema.Resource {
Delete: DeleteAccount,

CustomizeDiff: customdiff.All(
ComputedIfAnyAttributeChanged(FullyQualifiedNameAttributeName, "name"),
ComputedIfAnyAttributeChanged(accountSchema, FullyQualifiedNameAttributeName, "name"),
),

Schema: accountSchema,
Expand Down
5 changes: 2 additions & 3 deletions pkg/resources/account_role.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,8 @@ func AccountRole() *schema.Resource {
Description: "The resource is used for role management, where roles can be assigned privileges and, in turn, granted to users and other roles. When granted to roles they can create hierarchies of privilege structures. For more details, refer to the [official documentation](https://docs.snowflake.com/en/user-guide/security-access-control-overview).",

CustomizeDiff: customdiff.All(
ComputedIfAnyAttributeChanged(ShowOutputAttributeName, "comment"),
ComputedIfAnyAttributeChangedWithSuppressDiff(ShowOutputAttributeName, suppressIdentifierQuoting, "name"),
ComputedIfAnyAttributeChangedWithSuppressDiff(FullyQualifiedNameAttributeName, suppressIdentifierQuoting, "name"),
ComputedIfAnyAttributeChanged(accountRoleSchema, ShowOutputAttributeName, "comment", "name"),
ComputedIfAnyAttributeChanged(accountRoleSchema, FullyQualifiedNameAttributeName, "name"),
),

Importer: &schema.ResourceImporter{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ func ApiAuthenticationIntegrationWithAuthorizationCodeGrant() *schema.Resource {
ForceNewIfChangeToEmptyString("oauth_token_endpoint"),
ForceNewIfChangeToEmptyString("oauth_authorization_endpoint"),
ForceNewIfChangeToEmptyString("oauth_client_auth_method"),
ComputedIfAnyAttributeChanged(ShowOutputAttributeName, "enabled", "comment"),
ComputedIfAnyAttributeChanged(DescribeOutputAttributeName, "enabled", "comment", "oauth_access_token_validity", "oauth_refresh_token_validity",
ComputedIfAnyAttributeChanged(apiAuthAuthorizationCodeGrantSchema, ShowOutputAttributeName, "enabled", "comment"),
ComputedIfAnyAttributeChanged(apiAuthAuthorizationCodeGrantSchema, DescribeOutputAttributeName, "enabled", "comment", "oauth_access_token_validity", "oauth_refresh_token_validity",
"oauth_client_id", "oauth_client_auth_method", "oauth_authorization_endpoint",
"oauth_token_endpoint", "oauth_allowed_scopes"),
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ func ApiAuthenticationIntegrationWithClientCredentials() *schema.Resource {
CustomizeDiff: customdiff.All(
ForceNewIfChangeToEmptyString("oauth_token_endpoint"),
ForceNewIfChangeToEmptyString("oauth_client_auth_method"),
ComputedIfAnyAttributeChanged(ShowOutputAttributeName, "enabled", "comment"),
ComputedIfAnyAttributeChanged(DescribeOutputAttributeName, "enabled", "comment", "oauth_access_token_validity", "oauth_refresh_token_validity",
ComputedIfAnyAttributeChanged(apiAuthClientCredentialsSchema, ShowOutputAttributeName, "enabled", "comment"),
ComputedIfAnyAttributeChanged(apiAuthClientCredentialsSchema, DescribeOutputAttributeName, "enabled", "comment", "oauth_access_token_validity", "oauth_refresh_token_validity",
"oauth_client_id", "oauth_client_auth_method", "oauth_token_endpoint", "oauth_allowed_scopes"),
),
Importer: &schema.ResourceImporter{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ func ApiAuthenticationIntegrationWithJwtBearer() *schema.Resource {
ForceNewIfChangeToEmptyString("oauth_token_endpoint"),
ForceNewIfChangeToEmptyString("oauth_authorization_endpoint"),
ForceNewIfChangeToEmptyString("oauth_client_auth_method"),
ComputedIfAnyAttributeChanged(ShowOutputAttributeName, "enabled", "comment"),
ComputedIfAnyAttributeChanged(DescribeOutputAttributeName, "enabled", "comment", "oauth_access_token_validity", "oauth_refresh_token_validity",
ComputedIfAnyAttributeChanged(apiAuthJwtBearerSchema, ShowOutputAttributeName, "enabled", "comment"),
ComputedIfAnyAttributeChanged(apiAuthJwtBearerSchema, DescribeOutputAttributeName, "enabled", "comment", "oauth_access_token_validity", "oauth_refresh_token_validity",
"oauth_client_id", "oauth_client_auth_method", "oauth_authorization_endpoint",
"oauth_token_endpoint", "oauth_assertion_issuer"),
),
Expand Down
46 changes: 26 additions & 20 deletions pkg/resources/custom_diffs.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@ package resources

import (
"context"
"fmt"
"log"
"strconv"
"strings"

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

"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/provider/sdkv2enhancements"
"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
Expand Down Expand Up @@ -83,33 +84,38 @@ func ForceNewIfChangeToEmptyString(key string) schema.CustomizeDiffFunc {
})
}

func ComputedIfAnyAttributeChanged(key string, changedAttributeKeys ...string) schema.CustomizeDiffFunc {
// ComputedIfAnyAttributeChanged marks the given fields as computed if any of the listed fields changes.
// It takes field-level diffSuppress into consideration based on the schema passed.
// If the field is not found in the given schema, it continues without error.
func ComputedIfAnyAttributeChanged(resourceSchema map[string]*schema.Schema, key string, changedAttributeKeys ...string) schema.CustomizeDiffFunc {
return customdiff.ComputedIf(key, func(ctx context.Context, diff *schema.ResourceDiff, meta interface{}) bool {
var result bool
for _, changedKey := range changedAttributeKeys {
if diff.HasChange(changedKey) {
old, new := diff.GetChange(changedKey)
log.Printf("[DEBUG] ComputedIfAnyAttributeChanged: changed key: %s old: %s new: %s\n", changedKey, old, new)
}
result = result || diff.HasChange(changedKey)
}
return result
})
}

// TODO(SNOW-1629468): Adjust the function to make it more flexible
func ComputedIfAnyAttributeChangedWithSuppressDiff(key string, suppressDiffFunc schema.SchemaDiffSuppressFunc, changedAttributeKeys ...string) schema.CustomizeDiffFunc {
return customdiff.ComputedIf(key, func(ctx context.Context, diff *schema.ResourceDiff, meta interface{}) bool {
for _, changedKey := range changedAttributeKeys {
if diff.HasChange(changedKey) {
oldValue, newValue := diff.GetChange(changedKey)
if !suppressDiffFunc(key, oldValue.(string), newValue.(string), nil) {
log.Printf("[DEBUG] ComputedIfAnyAttributeChangedWithSuppressDiff: changed key: %s", changedKey)
return true
log.Printf("[DEBUG] ComputedIfAnyAttributeChanged: changed key: %s old: %s new: %s\n", changedKey, oldValue, newValue)

if v, ok := resourceSchema[changedKey]; ok {
if diffSuppressFunc := v.DiffSuppressFunc; diffSuppressFunc != nil {
resourceData, resourceDataOk := sdkv2enhancements.CreateResourceDataFromResourceDiff(resourceSchema, diff)
if !resourceDataOk {
log.Printf("[DEBUG] ComputedIfAnyAttributeChanged: did not create resource data correctly, skipping\n")
continue
}
if !diffSuppressFunc(key, fmt.Sprintf("%v", oldValue), fmt.Sprintf("%v", newValue), resourceData) {
log.Printf("[DEBUG] ComputedIfAnyAttributeChanged: key %s was changed and the diff is not suppressed", changedKey)
result = true
} else {
log.Printf("[DEBUG] ComputedIfAnyAttributeChanged: key %s was changed but the diff is suppresed", changedKey)
}
} else {
log.Printf("[DEBUG] ComputedIfAnyAttributeChanged: key %s was changed and it does not have a diff suppressor", changedKey)
result = true
}
}
}
}
return false
return result
})
}

Expand Down
Loading
Loading