Module upgrade tries to empty the password of existing DB

[Spritekin]

Description

When upgrading the module from version = "3.1.0" to version = "4.1.1", terraform tries to update my database username and password (which have not changed) however it fails with:

Error: Error modifying DB Instance au-devops-logs: InvalidParameterValue: The parameter MasterUserPassword must be provided and must not be blank. status code: 400, request id: c79cd027-bd63-4d4b-af3a-71942ec6251e

Notes:
I'm using Terraform Cloud to run my code, so all projects follow the best practices provided by Terraform Cloud.

Versions

• Terraform: 1.1.3 in Terraform Cloud
• This is the output of my local configuration from a fresh terraform init:

> terraform providers -version
Terraform v1.1.3
on darwin_amd64
+ provider registry.terraform.io/hashicorp/aws v4.1.0
+ provider registry.terraform.io/hashicorp/cloudinit v2.2.0
+ provider registry.terraform.io/hashicorp/helm v2.0.1
+ provider registry.terraform.io/hashicorp/kubernetes v2.0.3
+ provider registry.terraform.io/hashicorp/local v2.1.0
+ provider registry.terraform.io/hashicorp/null v3.1.0
+ provider registry.terraform.io/hashicorp/random v3.1.0
+ provider registry.terraform.io/hashicorp/template v2.2.0
+ provider registry.terraform.io/terraform-aws-modules/http v2.4.1

• Module:

   source  = "terraform-aws-modules/rds/aws"
  version = "4.1.1"

Reproduction

Steps to reproduce the behavior:

I'm using Terraform Cloud Workspaces.

I don't run locally. Runs are executed in Terraform Cloud. The problem is reported by Terraform Cloud.

a. I upgraded my AWS provider from 3.46.0 to 4.1.0 From my Git:

     aws = {
       source  = "hashicorp/aws"
-      version = "3.46.0"
+      version = "4.1.0"
       configuration_aliases = [ aws.global, aws.dns, aws.storage ]
     }

b. I upgraded the module version:

 module "logs-db" {
   source  = "terraform-aws-modules/rds/aws"
-  version = "3.1.0"                           <<< Version change here
+  version = "4.1.1"

... more code
 
-  name = "au-devops-logs"                          <<< Changed the name parameter to db_name parameter
+  db_name = "au-devops-logs"
   username = var.logs_db_admin_user            <<< Notice the username and password didn't change
   password = var.logs_db_admin_password    <<< These values come from an encrypted source, no changes at all
          << This was verified ok because the values are also saved as Kubernetes secrets and the values are ok and unchanged
          << This is a development test logs DB so the values are:
          << LOGS_DB_ADMIN_USER: logs
          << LOGS_DB_ADMIN_PASSWORD: adminpassword

... more code

-  final_snapshot_identifier = "au-devops-logs-final"
+  final_snapshot_identifier_prefix = "final"
+  create_db_subnet_group = true
}

The final code (simplified so its easier to replicate the code:

module "logs-db" {
  source  = "terraform-aws-modules/rds/aws"
  version = "4.1.1"

  identifier            = "au-devops-logs"
  snapshot_identifier   = "au-devops-logs-backup"    <<< IMPORTANT: This DB was initialised from a backup.

  engine                = "postgres"
  engine_version        = "13.2"
  auto_minor_version_upgrade = false

  family                = "postgres13" # DB parameter group
  major_engine_version  = "13"         # DB option group
  instance_class        = "db.t3.small"

  allocated_storage     = 10
  max_allocated_storage = 1000
  storage_encrypted     = true
  kms_key_id            = "aws/rds"

  db_name     = "au-devops-logs"
  username = var.logs_db_admin_user
  password = var.logs_db_admin_password
  port     = 5432

  vpc_security_group_ids = [sg-replace_your_sg_id]

  maintenance_window              = "Mon:00:00-Mon:03:00"
  backup_window                   = "03:00-06:00"

  backup_retention_period = 0
  deletion_protection     = false

  performance_insights_enabled          = true
  performance_insights_retention_period = 7

  parameters = [
    {
      name  = "autovacuum"
      value = 1
    },
    {
      name  = "client_encoding"
      value = "utf8"
    },
    {
      name  = "timezone"
      value = "UTC"
    }    
  ]

  tags = {
    Environment = "au-devops"
  }

  # Subnets with access to the DB
  subnet_ids = var.private_subnets.

  # Snapshot name upon DB deletion
  final_snapshot_identifier_prefix = "final"
  skip_final_snapshot = true
  copy_tags_to_snapshot = true

  option_group_description = "Option group for au-devops-logs"
  option_group_timeouts = { "delete": "45m" }
  parameter_group_description = "Database parameter group for au-devops-logs"

  create_db_subnet_group = true
  db_subnet_group_description = "Database subnet group for au-devops-logs"

  multi_az = var.logs_db_multi_az
}

Expected behavior

Some parameters should update that's ok, but there shouldn't be any problems with the DB username and password as they have been unchanged.

Actual behavior

Along other updates, the module attempts to update the username and password. This is an output from Terraform Cloud:

~ password : Sensitive value   << Tries to update password
~ username : Sensitive value   << Tries to update username

This is harder to debug as TFC won't show the value because of its sensitive nature.

Terminal Output Screenshot(s)

Additional context

After reading the code of the terraform-aws-rds module, my suspicion is in the following lines:

terraform-aws-rds/module/db_instance/main.tf: Lines 7-9
  metadata_already_exists = var.snapshot_identifier != null || var.replicate_source_db != null
  username                = local.metadata_already_exists ? null : var.username
  password                = local.metadata_already_exists ? null : var.password

My suspicion is that in this case, this code might work fine as long as the module is creating a DB from scratch. However in this case, the snapshot_identifier variable exists and is not null, therefore the local.metadata_already_exists variable will be true.
Given the local.metadata_already_exists is true, then the local.username and local.password vars will be set to null. Not sure the effect this might have on the DB change but the error message "The parameter MasterUserPassword must be provided and must not be blank." suggests a null value might not be welcome for the password.

In addition this kind of code immediately cancels the ability to change the master username and password. Imagine I would like to change those values because they have become compromised, then it's not possible because the local.username will take the null value if the snapshot_identifier is set.

Thanks!

[Spritekin]

I went into the code for version 3.1.0 and this is how the password reads at that version:

local {
 master_password      = var.create_db_instance && var.create_random_password ? random_password.master_password[0].result : var.password
...
}

This local.master_password was then used to feel the "password" variable.

The new version of making the password change dependent of the existence of the snapshot identifier makes no sense.

Personally, I would accept the password as it is sent from the var.password variable. Let the user set this password any way they want, generate it randomly, get it from an encrypted source... for whatever reason, do not mess with it. The user will have a reason for the value of this variable.

Even more, if I create this instance using terraform, the user will have a logic to setup the username and password even if the DB was restored form a snapshot, I wouldn't touch the values at all. My suggestion, make the values straight:

  username                = var.username
  password                = var.password

Luck!

[bryantbiggs]

please refer to the upgrade guide https://github.com/terraform-aws-modules/terraform-aws-rds/blob/master/UPGRADE-4.0.md#after-4x-example

[Spritekin]

@bryantbiggs

Thanks but your answer is not ok:

a. The upgrade guide mentions a master_password env var but I'm looking at the docs at https://registry.terraform.io/modules/terraform-aws-modules/rds/aws/latest?tab=inputs and there is no mention to a master_password.

I still tried the suggestion and added this to my module:

  create_random_password = false
  master_password        = var.logs_db_admin_password

However when testing, terraform output this:

+ terraform -chdir=test validate
╷
│ Error: Unsupported argument
│ 
│   on ../databases.tf line 51, in module "logs-db":
│   51:   master_password        = var.logs_db_admin_password  
│ 
│ An argument named "master_password" is not expected here.

So not really sure what is the upgrade procedure then.

b. The code I mention in the original post makes it impossible to change the password of the DB if the DB has been initialised from a snapshot:

metadata_already_exists = var.snapshot_identifier != null || var.replicate_source_db != null
  username                = local.metadata_already_exists ? null : var.username
  password                = local.metadata_already_exists ? null : var.password

Say the password is compromised then I would like to change it. How can that be done?

Regards,

[bryantbiggs]

1. It looks like there is a typo in the upgrade guide: master_password -> password
2. Changing the password on a snapshot would be a feature request. We do not default to changing the password because the password is already set if its a replica or being restored from a snapshot

[Spritekin]

Ok but then I'm still stuck in the same place.

I got a DB created in module 3.1.0, when I upgrade to 4.1.0 I get an error that says the master password needs to be provided and I'm providing the password.

So is this a bug or not?

Please read my report. I'm sending the password variable:

  password = var.logs_db_admin_password

[Spritekin]

Another problem is that the behaviour is inconsistent. If the DB is created from scratch and the snapshot_identifier is empty then I can change the master password, but if it comes from a snapshot I can't.

Seem like an arbitrary conditional. The previous module would let me change the password regardless of the DB source.

[Spritekin]

@bryantbiggs

I have gone through all the migration doc, even set the create_random_password variable to false. However that doesn't help and the module still fails with the same error.

This makes sense as the random password creation also depends on the existence of the snapshot_identifier.

create_random_password = var.create_db_instance && var.create_random_password && var.snapshot_identifier == null

Also, please read my two comments above.

I'm really sorry but I'm stuck with this.

Regards,

[bryantbiggs]

yes yes, calm down - I am looking. please take a look at #384 and try doing a plan on your end with it to see if this resolves your issue

[Spritekin]

@bryantbiggs no wait a second... I pushed the invalid version. Please give me a minute so it redeploys with the create_random_password option.

[Spritekin]

@bryantbiggs

Ok so setting create_random_password = false allowed it to finish successfully!

I have no idea what voodoo magic is involved but I will take it. Thanks!

Thank you so much!

[bryantbiggs]

on current module version or my PR?

[Spritekin]

On current version. I got it pinned to 4.1.0.

No idea why the random password variable would have that effect. Haven't read the full code.

[Spritekin]

Thanks for the changes in your PR!