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: ignores password_reset_required change to support changes in AWS provider 4.x #24

Merged
merged 4 commits into from
Jul 20, 2022

Conversation

gberenice
Copy link
Contributor

what

  • Ignores password_reset_required change.

why

references

@gberenice gberenice requested review from a team as code owners July 19, 2022 20:15
@gberenice gberenice requested review from korenyoni and florian0410 and removed request for a team July 19, 2022 20:15
Copy link
Member

@Gowiem Gowiem left a comment

Choose a reason for hiding this comment

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

@gberenice Looks simple and solid, but one question on the change shared below! 💯 🚀 👋

@@ -16,6 +16,10 @@ resource "aws_iam_user_login_profile" "default" {
password_length = var.password_length
password_reset_required = var.password_reset_required
depends_on = [aws_iam_user.default]

lifecycle {
Copy link
Member

Choose a reason for hiding this comment

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

Hm... Does adding the lifecycle block force a recreate of this resource? What does that do for existing IAM users that you're managing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After upgrade of AWS provider to 4.x (in our case - to 4.21, without any change to root module configuration or terraform vars) the resource was forced to recreate:

  # module.users["veronika_gnilitska"].aws_iam_user_login_profile.default[0] must be replaced
-/+ resource "aws_iam_user_login_profile" "default" {
      ~ encrypted_password      = "wcBMAwEF3O2PIAyrAQgAvoQ6klRbbQ705f0Pj6l9uf4C/9GOdVfiEqmBkendM3dKPRkbbcJVXRP80xaTkElae17OiS69QJVji3/sS3ru2TLnFyZcNtSMayu9sodi2VDbNOhN6FRywuv9OBkBcgDu7686pOZLG27H2zT7V18sBwYR6bDaUepnLtDLq" -> (known after apply)
      ~ id                      = "veronika.gnilitska" -> (known after apply)
      ~ key_fingerprint         = "92f19546ec0f7d7ca8f9d" -> (known after apply)
      + password                = (known after apply)
      ~ password_reset_required = false -> true # forces replacement
        # (3 unchanged attributes hidden)
    }

If you apply this plan you won't be able to login with the credentials used before the apply. To avoid this behaviour for existing users - lifecycle was used.

Copy link
Member

Choose a reason for hiding this comment

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

👍 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Gowiem thank you! 💯

@Gowiem Gowiem added the patch A minor, backward compatible change label Jul 20, 2022
@Gowiem
Copy link
Member

Gowiem commented Jul 20, 2022

/test all

Gowiem
Gowiem previously approved these changes Jul 20, 2022
@gberenice
Copy link
Contributor Author

@Gowiem oh, I see that some check were failed, but I doubt that it's related to introduced changes:

Terraform  is required for master...
+ [[ '' =~ ^1\. ]]
+ PATH_TO_TERRAFORM=/usr/local/terraform//bin
+ '[' -x /usr/local/terraform//bin/terraform ']'
+ echo 'Unable to locate executable for terraform '
Unable to locate executable for terraform 

Could you please advise how it can be fixed?

@Gowiem
Copy link
Member

Gowiem commented Jul 20, 2022

@gberenice the issue is due to a missing versions.tf in examples/complete. Can you copy the root versions.tf to examples/complete and we'll give that a shot?

@Gowiem
Copy link
Member

Gowiem commented Jul 20, 2022

/test all

@gberenice
Copy link
Contributor Author

@Gowiem awesome, that worked! 😃
Is it possible to merge it now, or should I wait something?

@Gowiem Gowiem merged commit 343506e into cloudposse:master Jul 20, 2022
@Gowiem
Copy link
Member

Gowiem commented Jul 20, 2022

@gberenice Solid work -- Thanks for the fix! Released as https://github.com/cloudposse/terraform-aws-iam-user/releases/tag/0.8.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch A minor, backward compatible change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants