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

fix: Fix user resource import #3181

Merged
merged 3 commits into from
Nov 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 12 additions & 0 deletions MIGRATION_GUIDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions docs/resources/legacy_service_user.md
Original file line number Diff line number Diff line change
Expand Up @@ -1011,3 +1011,5 @@ Import is supported using the following syntax:
```shell
terraform import snowflake_legacy_service_user.example '"<user_name>"'
```

Note: terraform plan+apply may be needed after successful import to fill out all the missing fields (like `password`) in state.
2 changes: 2 additions & 0 deletions docs/resources/user.md
Original file line number Diff line number Diff line change
Expand Up @@ -1021,3 +1021,5 @@ Import is supported using the following syntax:
```shell
terraform import snowflake_user.example '"<user_name>"'
```

Note: terraform plan+apply may be needed after successful import to fill out all the missing fields (like `password`) in state.
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
50 changes: 43 additions & 7 deletions pkg/resources/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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),
Expand All @@ -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),
)
}
Expand All @@ -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{},
Expand Down Expand Up @@ -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'") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be handled on SDK level, and we could return a custom error from there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I can migrate it with the next PR

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)
Expand Down
62 changes: 62 additions & 0 deletions pkg/resources/user_acceptance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
),
},
},
})
}
2 changes: 2 additions & 0 deletions templates/resources/legacy_service_user.md.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -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.
2 changes: 2 additions & 0 deletions templates/resources/user.md.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Loading