Optional user and password #119

nitrocode · 2021-07-07T21:21:39Z

what

Optional user and password
Added some of the ssm params from https://github.com/cloudposse/terraform-aws-components/blob/master/deprecated/aws/backing-services/rds.tf

why

This will allow the user and password to be autogenerated and optionally saved as an SSM parameter

references

Closes Generate database password #118

notes

SSM
1. Should this even use SSM ? I thought it should if we made the module generate a random password.
2. SSM param creation is disabled by default
3. Should ssm_enabled be renamed to ssm_parameters_enabled to be more explicit ?
4. Should there also be a ssm_key_prefix instead of having to modify the ssm_key_format ?
5. Should I also save the module.rds_instance.hostname and var.database_port to ssm ?
6. Should all the SSM logic be put into its own systems-manager.tf file ?
7. Should the SSM logic require its own aws provider passed in so it can use its own region ? or should it use its own provider in the module itself via var.ssm_region and if one isn't passed in, it could use the current region ?
If the database_name = "" or null, should the name of the database be the module.this.id ?
- This could be done in a follow up PR
- The issue is with SQL server which requires the database_name = null so that means the ssm key itself would have the term null in there. Perhaps for ssm, if the database_name == null then we can use the unique module.this.id or module.this.name ?
- Decided to use module.this.name
Bridgecrew fixes
- This could be done in a follow up PR

output

enable ssm with defaults

With ssm_parameters_enabled = true

$ terraform plan
Terraform will perform the following actions:

  # module.rds_instance.aws_ssm_parameter.rds_database["hostname"] will be created
  + resource "aws_ssm_parameter" "rds_database" {
      + arn         = (known after apply)
      + data_type   = (known after apply)
      + description = "RDS DB hostname"
      + id          = (known after apply)
      + key_id      = (known after apply)
      + name        = "/rds/mydb/admin/db_hostname"
      + overwrite   = true
      + tags_all    = (known after apply)
      + tier        = "Standard"
      + type        = "String"
      + value       = (sensitive value)
      + version     = (known after apply)
    }

  # module.rds_instance.aws_ssm_parameter.rds_database["password"] will be created
  + resource "aws_ssm_parameter" "rds_database" {
      + arn         = (known after apply)
      + data_type   = (known after apply)
      + description = "RDS DB password"
      + id          = (known after apply)
      + key_id      = "alias/aws/ssm"
      + name        = "/rds/mydb/admin/db_password"
      + overwrite   = true
      + tags_all    = (known after apply)
      + tier        = "Standard"
      + type        = "SecureString"
      + value       = (sensitive value)
      + version     = (known after apply)
    }

  # module.rds_instance.aws_ssm_parameter.rds_database["port"] will be created
  + resource "aws_ssm_parameter" "rds_database" {
      + arn         = (known after apply)
      + data_type   = (known after apply)
      + description = "RDS DB port"
      + id          = (known after apply)
      + key_id      = (known after apply)
      + name        = "/rds/mydb/admin/db_port"
      + overwrite   = true
      + tags_all    = (known after apply)
      + tier        = "Standard"
      + type        = "String"
      + value       = (sensitive value)
      + version     = (known after apply)
    }

  # module.rds_instance.aws_ssm_parameter.rds_database["user"] will be created
  + resource "aws_ssm_parameter" "rds_database" {
      + arn         = (known after apply)
      + data_type   = (known after apply)
      + description = "RDS DB user"
      + id          = (known after apply)
      + key_id      = (known after apply)
      + name        = "/rds/mydb/admin/db_user"
      + overwrite   = true
      + tags_all    = (known after apply)
      + tier        = "Standard"
      + type        = "String"
      + value       = (sensitive value)
      + version     = (known after apply)
    }

Plan: 4 to add, 0 to change, 0 to destroy.

enable ssm with merge defaults

With ssm_parameters_enabled = true and

  save_parameter_ssm_map_merge = {
    user = {
      suffix      = "admin/db_user_hello"
      description = "RDS DB user hihi"
    }
  }

$ terraform plan

  # hide previous 3 as seen above and only show the 1 difference

  # module.rds_instance.aws_ssm_parameter.rds_database["user"] will be created
  + resource "aws_ssm_parameter" "rds_database" {
      + arn         = (known after apply)
      + data_type   = (known after apply)
      + description = "RDS DB user hihi"
      + id          = (known after apply)
      + key_id      = (known after apply)
      + name        = "/rds/mydb/admin/db_user_hello"
      + overwrite   = true
      + tags_all    = (known after apply)
      + tier        = "Standard"
      + type        = "String"
      + value       = (sensitive value)
      + version     = (known after apply)
    }

bridgecrew

Bridgecrew has found 2 infrastructure configuration errors in this PR ⬇️

main.tf

bridgecrew

Bridgecrew has found 2 infrastructure configuration errors in this PR ⬇️

main.tf

bridgecrew

Bridgecrew has found 2 infrastructure configuration errors in this PR ⬇️

main.tf

bridgecrew

Bridgecrew has found 2 infrastructure configuration errors in this PR ⬇️

main.tf

bridgecrew

Bridgecrew has found 2 infrastructure configuration errors in this PR ⬇️

main.tf

bridgecrew

Bridgecrew has found 2 infrastructure configuration errors in this PR ⬇️

main.tf

bridgecrew

Bridgecrew has found 2 infrastructure configuration errors in this PR ⬇️

main.tf

bridgecrew

Bridgecrew has found 2 infrastructure configuration errors in this PR ⬇️

main.tf

bridgecrew

Bridgecrew has found 2 infrastructure configuration errors in this PR ⬇️

main.tf

bridgecrew

Bridgecrew has found 2 infrastructure configuration errors in this PR ⬇️

main.tf

bridgecrew

Bridgecrew has found 2 infrastructure configuration errors in this PR ⬇️

main.tf

bridgecrew

Bridgecrew has found 2 infrastructure configuration errors in this PR ⬇️

main.tf

bridgecrew

Bridgecrew has found 2 infrastructure configuration errors in this PR ⬇️

main.tf

bridgecrew

Bridgecrew has found 2 infrastructure configuration errors in this PR ⬇️

main.tf

nitrocode · 2021-07-07T23:43:54Z

/test terratest

bridgecrew

Bridgecrew has found 2 infrastructure configuration errors in this PR ⬇️

main.tf

systems-manager.tf

See comment above regarding database_name = null

bridgecrew

Bridgecrew has found 2 infrastructure configuration errors in this PR ⬇️

main.tf

nitrocode · 2021-07-09T00:05:54Z

/test all

Gowiem · 2021-07-19T23:41:23Z

main.tf

+  # Leave special characters out to avoid quoting and other issues.
+  # Special characters have no additional security compared to increasing length.
+  special          = false
+  override_special = "!#$%^&*()<>-_"


AFAIU, you don't need override_specialif you includespecial = false`. No harm no foul, but just mentioning.

Gowiem · 2021-07-19T23:46:35Z

systems-manager.tf

@@ -0,0 +1,96 @@
+# AWS KMS alias used for encryption/decryption of SSM secure strings
+variable "kms_alias_name_ssm" {


Why put the variable and output block into this file? I get that they're isolated to systems manager related functionality, but I'd still prefer to see them all live in their typical places (i.e. variables.tf and outputs.tf).

I think because this was a precursor to putting it in its own module eventually. That way it would be easier to migrate. We can put it into variables.tf, it would just make migration slightly more difficult.

We frequently do this in our components. Have a specific <service>-variables.tf or variables-<service>.tf file so it's easier for copy pasta. I figured the same methodology could be applied in the modules too.

Gowiem · 2021-07-19T23:48:09Z

systems-manager.tf

+variable "ssm_parameters_enabled" {
+  type        = bool
+  default     = false
+  description = "If `true` create SSM keys for the database user and password."


This is controlling more than just the PStore params for user + pass since you're also including hostname + port. I'd update or better yet make this a more generic "for the database info".

Gowiem

@nitrocode looks good overall. One thing that bugs me is co-locating vars + outputs in the systems-manager.tf file. I don't like that because even with autogenerated READMEs, in small child modules like this it means I need to dig around when I can't find a variable block.

Also, I wonder if outputting important values like you're doing here to SSM PStore is something only components should do and the child modules should be simpler? Thoughts?

nitrocode · 2021-07-20T15:31:31Z

I think we need a separated module for SSM / Secrets Manager like @jamengual mentioned. Then we can easily turn it on and off and have the variables located in the upstream module instead of here.

We can do some of it by using https://github.com/cloudposse/terraform-aws-ssm-parameter-store but not the password creation.

I was thinking about doing that later but maybe it should be a prerequisite for this PR.

mergify · 2021-08-24T03:36:46Z

This pull request is now in conflict. Could you fix it @nitrocode? 🙏

arischow · 2021-09-18T09:51:10Z

@nitrocode It would be great if you could make database_name optional as well by specifying an empty string default value in variables.tf.

Possible scenario would be I would like to do some modifications after I created the db instance from a snapshot and if database_name is required and then it would force a replacement.

Thanks!

nitrocode · 2021-09-18T14:16:10Z

@arischow thanks for the suggestion. This PR is only for generating password credentials. Could you write that into a separate issue so we can track it?

mergify · 2021-12-03T17:31:24Z

This pull request is now in conflict. Could you fix it @nitrocode? 🙏

bridgecrew bot reviewed Jul 7, 2021

View reviewed changes

main.tf Show resolved Hide resolved

main.tf Show resolved Hide resolved

bridgecrew bot reviewed Jul 7, 2021

View reviewed changes

main.tf Show resolved Hide resolved

main.tf Show resolved Hide resolved

nitrocode force-pushed the optional_user_and_password branch from 1ce66d2 to 76b906c Compare July 7, 2021 22:32

bridgecrew bot reviewed Jul 7, 2021

View reviewed changes

main.tf Show resolved Hide resolved

main.tf Show resolved Hide resolved

nitrocode force-pushed the optional_user_and_password branch from 76b906c to 55c544f Compare July 7, 2021 22:33

bridgecrew bot reviewed Jul 7, 2021

View reviewed changes

main.tf Show resolved Hide resolved

main.tf Show resolved Hide resolved

bridgecrew bot reviewed Jul 7, 2021

View reviewed changes

main.tf Show resolved Hide resolved

main.tf Show resolved Hide resolved

nitrocode force-pushed the optional_user_and_password branch from a856295 to 1ea1d29 Compare July 7, 2021 22:41

bridgecrew bot reviewed Jul 7, 2021

View reviewed changes

main.tf Show resolved Hide resolved

main.tf Show resolved Hide resolved

bridgecrew bot reviewed Jul 7, 2021

View reviewed changes

main.tf Show resolved Hide resolved

main.tf Show resolved Hide resolved

nitrocode force-pushed the optional_user_and_password branch from bd54666 to 3e4c2b1 Compare July 7, 2021 22:43

bridgecrew bot reviewed Jul 7, 2021

View reviewed changes

main.tf Show resolved Hide resolved

main.tf Show resolved Hide resolved

bridgecrew bot reviewed Jul 7, 2021

View reviewed changes

main.tf Show resolved Hide resolved

main.tf Show resolved Hide resolved

nitrocode force-pushed the optional_user_and_password branch from 1fefa0b to 15c41e7 Compare July 7, 2021 23:19

bridgecrew bot reviewed Jul 7, 2021

View reviewed changes

main.tf Show resolved Hide resolved

main.tf Show resolved Hide resolved

nitrocode marked this pull request as ready for review July 7, 2021 23:20

nitrocode requested review from a team as code owners July 7, 2021 23:20

nitrocode requested review from jamengual and joe-niland July 7, 2021 23:20

bridgecrew bot reviewed Jul 7, 2021

View reviewed changes

main.tf Show resolved Hide resolved

main.tf Show resolved Hide resolved

nitrocode force-pushed the optional_user_and_password branch from d5b3c25 to d007100 Compare July 7, 2021 23:23

bridgecrew bot reviewed Jul 7, 2021

View reviewed changes

main.tf Show resolved Hide resolved

main.tf Show resolved Hide resolved

Allow database user and password to be optional

f5b85a4

nitrocode force-pushed the optional_user_and_password branch from ff54317 to f5b85a4 Compare July 7, 2021 23:24

bridgecrew bot reviewed Jul 7, 2021

View reviewed changes

main.tf Show resolved Hide resolved

main.tf Show resolved Hide resolved

Auto Format

080ff05

bridgecrew bot reviewed Jul 7, 2021

View reviewed changes

main.tf Show resolved Hide resolved

main.tf Show resolved Hide resolved

bridgecrew bot reviewed Jul 8, 2021

View reviewed changes

main.tf Show resolved Hide resolved

main.tf Show resolved Hide resolved

nitrocode requested a review from korenyoni July 8, 2021 03:05

jamengual previously approved these changes Jul 8, 2021

View reviewed changes

systems-manager.tf Outdated Show resolved Hide resolved

nitrocode commented Jul 8, 2021

View reviewed changes

systems-manager.tf Outdated Show resolved Hide resolved

nitrocode requested a review from jamengual July 8, 2021 21:46

SSM Params map, default, and merge

82b534f

bridgecrew bot reviewed Jul 9, 2021

View reviewed changes

main.tf Show resolved Hide resolved

main.tf Show resolved Hide resolved

nitrocode requested a review from Gowiem July 9, 2021 00:05

Gowiem reviewed Jul 19, 2021

View reviewed changes

nitrocode requested a review from Gowiem July 20, 2021 16:50

nitrocode and others added 2 commits August 11, 2021 17:13

Merge branch 'master' into optional_user_and_password

b4018be

Auto Format

d808a07

cloudposse deleted a comment from mergify bot Aug 11, 2021

nitrocode and others added 6 commits August 23, 2021 23:52

Merge branch 'master' into optional_user_and_password

ce444e6

Auto Format

a6f466e

Update README.yaml

f0814f7

Auto Format

1d91b00

Update systems-manager.tf

5c2c7cb

Auto Format

008c607

hans-d closed this Mar 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optional user and password #119

Optional user and password #119

nitrocode commented Jul 7, 2021 •

edited

Loading

bridgecrew bot left a comment

bridgecrew bot left a comment

bridgecrew bot left a comment

bridgecrew bot left a comment

bridgecrew bot left a comment

bridgecrew bot left a comment

bridgecrew bot left a comment

bridgecrew bot left a comment

bridgecrew bot left a comment

bridgecrew bot left a comment

bridgecrew bot left a comment

bridgecrew bot left a comment

bridgecrew bot left a comment

bridgecrew bot left a comment

nitrocode commented Jul 7, 2021

bridgecrew bot left a comment

bridgecrew bot left a comment

nitrocode commented Jul 9, 2021

Gowiem Jul 19, 2021

Gowiem Jul 19, 2021

nitrocode Jul 20, 2021

Gowiem Jul 19, 2021

Gowiem left a comment

nitrocode commented Jul 20, 2021 •

edited

Loading

mergify bot commented Aug 24, 2021

arischow commented Sep 18, 2021

nitrocode commented Sep 18, 2021

mergify bot commented Dec 3, 2021

		@@ -0,0 +1,96 @@
		# AWS KMS alias used for encryption/decryption of SSM secure strings
		variable "kms_alias_name_ssm" {

Optional user and password #119

Optional user and password #119

Conversation

nitrocode commented Jul 7, 2021 • edited Loading

what

why

references

notes

output

bridgecrew bot left a comment

Choose a reason for hiding this comment

bridgecrew bot left a comment

Choose a reason for hiding this comment

bridgecrew bot left a comment

Choose a reason for hiding this comment

bridgecrew bot left a comment

Choose a reason for hiding this comment

bridgecrew bot left a comment

Choose a reason for hiding this comment

bridgecrew bot left a comment

Choose a reason for hiding this comment

bridgecrew bot left a comment

Choose a reason for hiding this comment

bridgecrew bot left a comment

Choose a reason for hiding this comment

bridgecrew bot left a comment

Choose a reason for hiding this comment

bridgecrew bot left a comment

Choose a reason for hiding this comment

bridgecrew bot left a comment

Choose a reason for hiding this comment

bridgecrew bot left a comment

Choose a reason for hiding this comment

bridgecrew bot left a comment

Choose a reason for hiding this comment

bridgecrew bot left a comment

Choose a reason for hiding this comment

nitrocode commented Jul 7, 2021

bridgecrew bot left a comment

Choose a reason for hiding this comment

bridgecrew bot left a comment

Choose a reason for hiding this comment

nitrocode commented Jul 9, 2021

Gowiem Jul 19, 2021

Choose a reason for hiding this comment

Gowiem Jul 19, 2021

Choose a reason for hiding this comment

nitrocode Jul 20, 2021

Choose a reason for hiding this comment

Gowiem Jul 19, 2021

Choose a reason for hiding this comment

Gowiem left a comment

Choose a reason for hiding this comment

nitrocode commented Jul 20, 2021 • edited Loading

mergify bot commented Aug 24, 2021

arischow commented Sep 18, 2021

nitrocode commented Sep 18, 2021

mergify bot commented Dec 3, 2021

nitrocode commented Jul 7, 2021 •

edited

Loading

nitrocode commented Jul 20, 2021 •

edited

Loading