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

Add master_password output to return generated password. #45

Merged
merged 3 commits into from
May 23, 2023

Conversation

petur
Copy link
Contributor

@petur petur commented Apr 12, 2022

what

  • Add a master_password output for the generated password.

why

  • The generated password needs to be stored somewhere so that it's possible to use it to connect to the cluster.
  • This can for example be done by storing it in an aws_secretsmanager_secret_version as part of the terraform configuration that creates the cluster.
  • Exposing the password as a module output makes it available to other parts of the configuration so that the password can be passed to the resource that stores it.

references

@petur petur requested review from a team as code owners April 12, 2022 09:21
@petur petur requested review from r351574nc3 and milldr April 12, 2022 09:21
@erikhaq
Copy link

erikhaq commented Oct 26, 2022

/test all

@kevcube
Copy link
Contributor

kevcube commented Mar 20, 2023

Terraform didn't support sensitive until 0.14.0 so this PR should also update versions.tf @petur

I asked in CloudPosse slack for them to review this when they can.

@petur petur force-pushed the password-output branch from 5987c6d to dda18df Compare March 21, 2023 14:23
Copy link
Member

@milldr milldr left a comment

Choose a reason for hiding this comment

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

We try to avoid writing secrets to Terraform state. Instead we can save the password to SSM and add the path to the SSM parameter to outputs

@petur
Copy link
Contributor Author

petur commented Mar 22, 2023

We try to avoid writing secrets to Terraform state. Instead we can save the password to SSM and add the path to the SSM parameter to outputs

The random_password resource saves the generated password in the terraform state, so that is already happening. This PR doesn't change that.

@kevcube
Copy link
Contributor

kevcube commented Mar 22, 2023

Yes @milldr all object attributes are saved in terraform state regardless. Though the SSM parameter does make it easier to access this password from other places.

@milldr
Copy link
Member

milldr commented Mar 22, 2023

The random_password resource saves the generated password in the terraform state, so that is already happening. This PR doesn't change that.

Good point. I'll trigger the tests and approve once they pass

@milldr
Copy link
Member

milldr commented Mar 22, 2023

/test all

@milldr
Copy link
Member

milldr commented Mar 22, 2023

/rebuild-readme

@milldr
Copy link
Member

milldr commented Mar 22, 2023

/test all

@@ -1,5 +1,5 @@
terraform {
required_version = ">= 0.13.0"
required_version = ">= 0.14.0"
Copy link
Member

Choose a reason for hiding this comment

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

We should revert this change, since Cloud Posse modules still support v 0.13

Copy link
Contributor

Choose a reason for hiding this comment

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

@milldr without that change outputs can't be marked sensitive, and then we definitely need the SSM component. But see Here, it looks like CloudPosse is ok with moving forward from 0.13

Copy link
Contributor

@kevcube kevcube Mar 22, 2023

Choose a reason for hiding this comment

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

Check with Erik please (I know he was a part of the discussion with Matt), I think if we're good to move past 0.13 then this PR should jump to 1.3.0 or even 1.4.2 so that we don't face this version upgrading problem in the future in this module.

Copy link
Member

Choose a reason for hiding this comment

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

okay I've checked and it's no problem to bump that version. But in order for the tests to pass we need to update the version provided with testing

https://github.com/cloudposse/terraform-aws-documentdb-cluster/blob/master/examples/complete/versions.tf#L2

@petur petur force-pushed the password-output branch from 4318d5e to bdad586 Compare April 14, 2023 10:28
Set minimum terraform version to 0.14.0 for `sensitive` support.
@petur petur force-pushed the password-output branch from bdad586 to f57ce0b Compare April 14, 2023 12:05
@kevcube
Copy link
Contributor

kevcube commented May 22, 2023

@milldr friendly ping for re-review

@milldr
Copy link
Member

milldr commented May 23, 2023

/terratest

outputs.tf Outdated Show resolved Hide resolved
@milldr
Copy link
Member

milldr commented May 23, 2023

/terratest

Copy link
Member

@milldr milldr left a comment

Choose a reason for hiding this comment

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

Looks good! thanks for following up on this

@milldr milldr merged commit 8d03270 into cloudposse:main May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error: Invalid count argument for random_password resource
4 participants