From 34bbbc18b61ce1224c4f8607a12a9f6dfd95c958 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Fri, 8 Nov 2024 11:33:55 +0100 Subject: [PATCH] fix: Fix user resource import (#3181) Handle user import correctly. Password is empty after the `snowflake_user` import; we can't read it from the config or from Snowflake. During the next terraform plan+apply it's updated to the "same" value. It results in an error on Snowflake side: `New password rejected by current password policy. Reason: 'PRIOR_USE'.` The error will be ignored on the provider side (after all, it means that the password in state is the same as on Snowflake side). Still, plan+apply is needed after importing user. --- MIGRATION_GUIDE.md | 12 ++++ docs/resources/legacy_service_user.md | 2 + docs/resources/user.md | 2 + .../resourceassert/user_resource_ext.go | 5 ++ pkg/resources/user.go | 50 ++++++++++++--- pkg/resources/user_acceptance_test.go | 62 +++++++++++++++++++ .../resources/legacy_service_user.md.tmpl | 2 + templates/resources/user.md.tmpl | 2 + 8 files changed, 130 insertions(+), 7 deletions(-) diff --git a/MIGRATION_GUIDE.md b/MIGRATION_GUIDE.md index 2d39ac4327..0b71ba3b3b 100644 --- a/MIGRATION_GUIDE.md +++ b/MIGRATION_GUIDE.md @@ -167,6 +167,18 @@ Note: Because [bcr 2024_07](https://docs.snowflake.com/en/release-notes/bcr-bund Connected issues: [#3125](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/3125) +### *(bugfix)* Handle user import correctly + +#### Context before the change + +Password is empty after the `snowflake_user` import; we can't read it from the config or from Snowflake. +During the next terraform plan+apply it's updated to the "same" value. +It results in an error on Snowflake side: `New password rejected by current password policy. Reason: 'PRIOR_USE'.` + +#### After the change + +The error will be ignored on the provider side (after all, it means that the password in state is the same as on Snowflake side). Still, plan+apply is needed after importing user. + ## v0.96.0 ➞ v0.97.0 ### *(new feature)* snowflake_stream_on_table, snowflake_stream_on_external_table resource diff --git a/docs/resources/legacy_service_user.md b/docs/resources/legacy_service_user.md index bd5865064b..820ef8957a 100644 --- a/docs/resources/legacy_service_user.md +++ b/docs/resources/legacy_service_user.md @@ -1011,3 +1011,5 @@ Import is supported using the following syntax: ```shell terraform import snowflake_legacy_service_user.example '""' ``` + +Note: terraform plan+apply may be needed after successful import to fill out all the missing fields (like `password`) in state. diff --git a/docs/resources/user.md b/docs/resources/user.md index 48e3f1e7f8..688dd75359 100644 --- a/docs/resources/user.md +++ b/docs/resources/user.md @@ -1021,3 +1021,5 @@ Import is supported using the following syntax: ```shell terraform import snowflake_user.example '""' ``` + +Note: terraform plan+apply may be needed after successful import to fill out all the missing fields (like `password`) in state. diff --git a/pkg/acceptance/bettertestspoc/assert/resourceassert/user_resource_ext.go b/pkg/acceptance/bettertestspoc/assert/resourceassert/user_resource_ext.go index 5fd51745bb..80d6dd7841 100644 --- a/pkg/acceptance/bettertestspoc/assert/resourceassert/user_resource_ext.go +++ b/pkg/acceptance/bettertestspoc/assert/resourceassert/user_resource_ext.go @@ -19,6 +19,11 @@ func (u *UserResourceAssert) HasEmptyPassword() *UserResourceAssert { return u } +func (u *UserResourceAssert) HasNotEmptyPassword() *UserResourceAssert { + u.AddAssertion(assert.ValuePresent("password")) + return u +} + func (u *UserResourceAssert) HasMustChangePassword(expected bool) *UserResourceAssert { u.AddAssertion(assert.ValueSet("must_change_password", strconv.FormatBool(expected))) return u diff --git a/pkg/resources/user.go b/pkg/resources/user.go index 2d391232ee..95ff6aeeea 100644 --- a/pkg/resources/user.go +++ b/pkg/resources/user.go @@ -512,17 +512,17 @@ func GetReadUserFunc(userType sdk.UserType, withExternalChangesMarking bool) sch // can't read disable_mfa d.Set("user_type", u.Type), - func() error { + func(rd *schema.ResourceData, ud *sdk.UserDetails) error { var errs error if userType == sdk.UserTypePerson { errs = errors.Join( - setFromStringPropertyIfNotEmpty(d, "first_name", userDetails.FirstName), - setFromStringPropertyIfNotEmpty(d, "middle_name", userDetails.MiddleName), - setFromStringPropertyIfNotEmpty(d, "last_name", userDetails.LastName), + setFromStringPropertyIfNotEmpty(rd, "first_name", ud.FirstName), + setFromStringPropertyIfNotEmpty(rd, "middle_name", ud.MiddleName), + setFromStringPropertyIfNotEmpty(rd, "last_name", ud.LastName), ) } return errs - }(), + }(d, userDetails), d.Set(FullyQualifiedNameAttributeName, id.FullyQualifiedName()), handleUserParameterRead(d, userParameters), @@ -607,7 +607,6 @@ func GetUpdateUserFunc(userType sdk.UserType) func(ctx context.Context, d *schem switch userType { case sdk.UserTypePerson: userTypeSpecificFieldsErrs = errors.Join( - stringAttributeUpdate(d, "password", &setObjectProperties.Password, &unsetObjectProperties.Password), stringAttributeUpdate(d, "first_name", &setObjectProperties.FirstName, &unsetObjectProperties.FirstName), stringAttributeUpdate(d, "middle_name", &setObjectProperties.MiddleName, &unsetObjectProperties.MiddleName), stringAttributeUpdate(d, "last_name", &setObjectProperties.LastName, &unsetObjectProperties.LastName), @@ -617,7 +616,6 @@ func GetUpdateUserFunc(userType sdk.UserType) func(ctx context.Context, d *schem ) case sdk.UserTypeLegacyService: userTypeSpecificFieldsErrs = errors.Join( - stringAttributeUpdate(d, "password", &setObjectProperties.Password, &unsetObjectProperties.Password), booleanStringAttributeUpdate(d, "must_change_password", &setObjectProperties.MustChangePassword, &unsetObjectProperties.MustChangePassword), ) } @@ -640,6 +638,10 @@ func GetUpdateUserFunc(userType sdk.UserType) func(ctx context.Context, d *schem } } + if err := handlePasswordUpdate(ctx, id, userType, d, client); err != nil { + return diag.FromErr(err) + } + set := &sdk.UserSet{ SessionParameters: &sdk.SessionParameters{}, ObjectParameters: &sdk.UserObjectParameters{}, @@ -677,6 +679,40 @@ func GetUpdateUserFunc(userType sdk.UserType) func(ctx context.Context, d *schem } } +// handlePasswordUpdate is a current workaround to handle user's password after import. +// Password is empty after the import, we can't read it from the config or from Snowflake. +// During the next terraform plan+apply it's updated to the "same" value. +// It results in an error on Snowflake side: New password rejected by current password policy. Reason: 'PRIOR_USE'. +// Current workaround is to ignore such an error. We will revisit it after migration to plugin framework. +func handlePasswordUpdate(ctx context.Context, id sdk.AccountObjectIdentifier, userType sdk.UserType, d *schema.ResourceData, client *sdk.Client) error { + if userType == sdk.UserTypePerson || userType == sdk.UserTypeLegacyService { + setPassword := sdk.UserAlterObjectProperties{} + unsetPassword := sdk.UserObjectPropertiesUnset{} + if err := stringAttributeUpdate(d, "password", &setPassword.Password, &unsetPassword.Password); err != nil { + return err + } + if (setPassword != sdk.UserAlterObjectProperties{}) { + err := client.Users.Alter(ctx, id, &sdk.AlterUserOptions{Set: &sdk.UserSet{ObjectProperties: &setPassword}}) + if err != nil { + if strings.Contains(err.Error(), "Error: 003002 (28P01)") || strings.Contains(err.Error(), "Reason: 'PRIOR_USE'") { + logging.DebugLogger.Printf("[DEBUG] Update to the same password is prohibited but it means we have a valid password in the current state. Continue.") + } else { + d.Partial(true) + return err + } + } + } + if (unsetPassword != sdk.UserObjectPropertiesUnset{}) { + err := client.Users.Alter(ctx, id, &sdk.AlterUserOptions{Unset: &sdk.UserUnset{ObjectProperties: &unsetPassword}}) + if err != nil { + d.Partial(true) + return err + } + } + } + return nil +} + func DeleteUser(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { client := meta.(*provider.Context).Client id := helpers.DecodeSnowflakeID(d.Id()).(sdk.AccountObjectIdentifier) diff --git a/pkg/resources/user_acceptance_test.go b/pkg/resources/user_acceptance_test.go index 2b9386535b..17e5ef747d 100644 --- a/pkg/resources/user_acceptance_test.go +++ b/pkg/resources/user_acceptance_test.go @@ -1753,3 +1753,65 @@ func TestAcc_User_handleChangesToShowUsers_bcr202408_migration_bcr202407_disable }, }) } + +func TestAcc_User_importPassword(t *testing.T) { + _ = testenvs.GetOrSkipTest(t, testenvs.EnableAcceptance) + acc.TestAccPreCheck(t) + + userId := acc.TestClient().Ids.RandomAccountObjectIdentifier() + pass := random.Password() + firstName := random.AlphaN(6) + + _, userCleanup := acc.TestClient().User.CreateUserWithOptions(t, userId, &sdk.CreateUserOptions{ObjectProperties: &sdk.UserObjectProperties{ + Password: sdk.String(pass), + FirstName: sdk.String(firstName), + }}) + t.Cleanup(userCleanup) + + userModel := model.User("w", userId.Name()).WithPassword(pass).WithFirstName(firstName) + + resource.Test(t, resource.TestCase{ + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + PreCheck: func() { acc.TestAccPreCheck(t) }, + CheckDestroy: acc.CheckDestroy(t, resources.User), + Steps: []resource.TestStep{ + // IMPORT + { + Config: config.FromModel(t, userModel), + ResourceName: userModel.ResourceReference(), + ImportState: true, + ImportStateId: userId.Name(), + ImportStateCheck: assert.AssertThatImport(t, + resourceassert.ImportedUserResource(t, userId.Name()). + HasNoPassword(). + HasFirstNameString(firstName), + ), + ImportStatePersist: true, + }, + { + Config: config.FromModel(t, userModel), + Check: assert.AssertThat(t, + resourceassert.UserResource(t, userModel.ResourceReference()). + HasNotEmptyPassword(). + HasFirstNameString(firstName), + ), + }, + { + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectEmptyPlan(), + }, + }, + Config: config.FromModel(t, userModel), + Check: assert.AssertThat(t, + resourceassert.UserResource(t, userModel.ResourceReference()). + HasNotEmptyPassword(). + HasFirstNameString(firstName), + ), + }, + }, + }) +} diff --git a/templates/resources/legacy_service_user.md.tmpl b/templates/resources/legacy_service_user.md.tmpl index c07fef3dc5..e9e1ac462f 100644 --- a/templates/resources/legacy_service_user.md.tmpl +++ b/templates/resources/legacy_service_user.md.tmpl @@ -41,3 +41,5 @@ Import is supported using the following syntax: {{ codefile "shell" (printf "examples/resources/%s/import.sh" .Name)}} {{- end }} + +Note: terraform plan+apply may be needed after successful import to fill out all the missing fields (like `password`) in state. diff --git a/templates/resources/user.md.tmpl b/templates/resources/user.md.tmpl index a15bbb722b..eeaa0b36ff 100644 --- a/templates/resources/user.md.tmpl +++ b/templates/resources/user.md.tmpl @@ -41,3 +41,5 @@ Import is supported using the following syntax: {{ codefile "shell" (printf "examples/resources/%s/import.sh" .Name)}} {{- end }} + +Note: terraform plan+apply may be needed after successful import to fill out all the missing fields (like `password`) in state.