-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Adding support for password_policy.temporary_password_validity_days #10890
Conversation
ce6af14
to
7d00f44
Compare
@bflad @gdavison @aeschright any chance that this might get your attention, looks like more and more people are affected. |
Hi @michalschott -- thanks for getting a start on this! I think it makes sense to mark |
7d00f44
to
97fa671
Compare
97fa671
to
a95846d
Compare
62bdce5
to
f8e23ac
Compare
@aeschright thanks for the link, I'm facing very similar problems with failing tests without updating them with new parameter. Because of AWS sets
Without that we're not even able to update Updated the code to not fully drop support for the old parameter, but continuing to update user pools without updating your code will fail with in certain scenarios - reason why I had to add |
@michalschott is there a work around one can use while we wait for this patch to get merged? disregard - i somehow missed this comment #8827 (comment) |
Code used for testing:
Testing steps:
Any thought how can this be bypassed, I was thinking about forcing resource to be recreated but that might lead to various scenarios where this might not fit (once support for deprecated field drops). Shall we create new resource cognito_userpool_v2 to keep backwards compatibility? |
Since the old attribute is being deprecated, can we remove it from the provider, and push this forward? Seems the grace period is over, or perhaps I'm missing something? |
@aeschright have you internally agreed on final decision how this should be handled, as mentioned above adding support for new field partially drops support for (already legacy field) |
I've now run into this blocker like everyone else. Cannot update anything in the pools. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a couple of small things I'd like you to have a look at, and then we'll ship it 🚀
@@ -948,6 +955,11 @@ func resourceAwsCognitoUserPoolUpdate(d *schema.ResourceData, meta interface{}) | |||
log.Printf("[DEBUG] Received %s, retrying UpdateUserPool", err) | |||
return resource.RetryableError(err) | |||
} | |||
if isAWSErr(err, cognitoidentityprovider.ErrCodeInvalidParameterException, "Please use TemporaryPasswordValidityDays in PasswordPolicy instead of UnusedAccountValidityDays") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if isAWSErr(err, cognitoidentityprovider.ErrCodeInvalidParameterException, "Please use TemporaryPasswordValidityDays in PasswordPolicy instead of UnusedAccountValidityDays") { | |
if isAWSErr(err, cognitoidentityprovider.ErrCodeInvalidParameterException, "UnusedAccountValidityDays has been deprecated, set TemporaryPasswordValidityDays in PasswordPolicy instead.") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have time, I think it would be a good idea to add a test that checks the error condition, but I know this is a blocker for folks who've been following the PR, so I don't want to delay it any longer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested change is not working as this is the exact error message from AWS API I need to act on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think the test I've added should do the work. Since we're retrying on error without deprecated field being send, the plan will still show that a change will be required - reason why I have used nonemptyplan.
d8dfdab
to
6148590
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all your work on this! It will be included in next week's release.
--- PASS: TestAccAWSCognitoUserPoolClient_basic (17.77s)
--- PASS: TestAccAWSCognitoUserPoolDomain_basic (20.42s)
--- PASS: TestAccAWSCognitoUserPoolClient_allFields (20.49s)
--- PASS: TestAccAWSCognitoUserPool_basic (20.88s)
--- PASS: TestAccAWSCognitoUserPool_withDeviceConfiguration (27.58s)
--- PASS: TestAccAWSCognitoUserPoolClient_Name (27.68s)
--- PASS: TestAccAWSCognitoUserPool_withVerificationMessageTemplate (27.68s)
--- PASS: TestAccAWSCognitoUserPoolClient_allFieldsUpdatingOneField (30.56s)
--- PASS: TestAccAWSCognitoUserPool_withPasswordPolicy (31.04s)
--- PASS: TestAccAWSCognitoUserPool_withSmsVerificationMessage (31.19s)
--- PASS: TestAccAWSCognitoUserPool_withSmsConfiguration (32.60s)
--- PASS: TestAccAWSCognitoUserPool_withSchemaAttributes (34.14s)
--- PASS: TestAccAWSCognitoUserPool_withEmailVerificationMessage (34.22s)
--- PASS: TestAccAWSCognitoUserPool_withAliasAttributes (34.82s)
--- PASS: TestAccAWSCognitoUserPool_withAdvancedSecurityMode (38.25s)
--- PASS: TestAccAWSCognitoUserPool_withTags (39.37s)
--- PASS: TestAccAWSCognitoUserPool_withAdminCreateUserConfiguration (39.42s)
--- PASS: TestAccAWSCognitoUserPool_withSmsConfigurationUpdated (41.02s)
--- PASS: TestAccAWSCognitoUserPoolClient_RefreshTokenValidity (25.85s)
--- PASS: TestAccAWSCognitoUserPool_withLambdaConfig (58.46s)
--- PASS: TestAccAWSCognitoUserPool_update (62.47s)
This has been released in version 2.47.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks! |
…uration block unused_account_validity_days to be omitted Reference: #11858 Reference: #10890 There was previously no test configuration covering both admin_create_user_config and password_policy being defined. The upstream API has deprecated a field in the former, however if the configuration block was defined, the attribute would errantly show a difference on the deprecated field. Previous output from acceptance testing (before code fix): ``` --- FAIL: TestAccAWSCognitoUserPool_withAdminCreateUserConfigurationAndPasswordPolicy (13.28s) testing.go:640: Step 0 error: After applying this step, the plan was not empty: DIFF: UPDATE: aws_cognito_user_pool.test admin_create_user_config.#: "1" => "1" admin_create_user_config.0.allow_admin_create_user_only: "true" => "true" admin_create_user_config.0.invite_message_template.#: "0" => "0" admin_create_user_config.0.unused_account_validity_days: "7" => "" ... omitted for clarity ... ``` Output from acceptance testing: ``` --- PASS: TestAccAWSCognitoUserPool_withAdminCreateUserConfigurationAndPasswordPolicy (18.41s) --- PASS: TestAccAWSCognitoUserPool_basic (18.46s) --- PASS: TestAccAWSCognitoUserPool_withAliasAttributes (27.92s) --- PASS: TestAccAWSCognitoUserPool_withPasswordPolicy (29.64s) --- PASS: TestAccAWSCognitoUserPool_withVerificationMessageTemplate (29.68s) --- PASS: TestAccAWSCognitoUserPool_withDeviceConfiguration (30.52s) --- PASS: TestAccAWSCognitoUserPool_withEmailVerificationMessage (31.38s) --- PASS: TestAccAWSCognitoUserPool_withSmsVerificationMessage (32.67s) --- PASS: TestAccAWSCognitoUserPool_withSchemaAttributes (33.39s) --- PASS: TestAccAWSCognitoUserPool_withAdminCreateUserConfiguration (37.94s) --- PASS: TestAccAWSCognitoUserPool_withAdvancedSecurityMode (39.71s) --- PASS: TestAccAWSCognitoUserPool_withTags (44.33s) --- PASS: TestAccAWSCognitoUserPool_withSmsConfiguration (50.12s) --- PASS: TestAccAWSCognitoUserPool_withSmsConfigurationUpdated (51.37s) --- PASS: TestAccAWSCognitoUserPool_update (66.06s) --- PASS: TestAccAWSCognitoUserPool_withLambdaConfig (75.15s) ```
…uration block unused_account_validity_days to be omitted (#12001) Reference: #11858 Reference: #10890 There was previously no test configuration covering both admin_create_user_config and password_policy being defined. The upstream API has deprecated a field in the former, however if the configuration block was defined, the attribute would errantly show a difference on the deprecated field. Previous output from acceptance testing (before code fix): ``` --- FAIL: TestAccAWSCognitoUserPool_withAdminCreateUserConfigurationAndPasswordPolicy (13.28s) testing.go:640: Step 0 error: After applying this step, the plan was not empty: DIFF: UPDATE: aws_cognito_user_pool.test admin_create_user_config.#: "1" => "1" admin_create_user_config.0.allow_admin_create_user_only: "true" => "true" admin_create_user_config.0.invite_message_template.#: "0" => "0" admin_create_user_config.0.unused_account_validity_days: "7" => "" ... omitted for clarity ... ``` Output from acceptance testing: ``` --- PASS: TestAccAWSCognitoUserPool_withAdminCreateUserConfigurationAndPasswordPolicy (18.41s) --- PASS: TestAccAWSCognitoUserPool_basic (18.46s) --- PASS: TestAccAWSCognitoUserPool_withAliasAttributes (27.92s) --- PASS: TestAccAWSCognitoUserPool_withPasswordPolicy (29.64s) --- PASS: TestAccAWSCognitoUserPool_withVerificationMessageTemplate (29.68s) --- PASS: TestAccAWSCognitoUserPool_withDeviceConfiguration (30.52s) --- PASS: TestAccAWSCognitoUserPool_withEmailVerificationMessage (31.38s) --- PASS: TestAccAWSCognitoUserPool_withSmsVerificationMessage (32.67s) --- PASS: TestAccAWSCognitoUserPool_withSchemaAttributes (33.39s) --- PASS: TestAccAWSCognitoUserPool_withAdminCreateUserConfiguration (37.94s) --- PASS: TestAccAWSCognitoUserPool_withAdvancedSecurityMode (39.71s) --- PASS: TestAccAWSCognitoUserPool_withTags (44.33s) --- PASS: TestAccAWSCognitoUserPool_withSmsConfiguration (50.12s) --- PASS: TestAccAWSCognitoUserPool_withSmsConfigurationUpdated (51.37s) --- PASS: TestAccAWSCognitoUserPool_update (66.06s) --- PASS: TestAccAWSCognitoUserPool_withLambdaConfig (75.15s) ```
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
Community Note
Closes #8827
Release note for CHANGELOG:
Output from acceptance testing: