Skip to content

Conversation

@RoseSecurity
Copy link
Contributor

@RoseSecurity RoseSecurity commented Oct 9, 2025

what

This updates the module to support AWS-managed master user passwords for DocumentDB clusters, improves password management logic, and updates provider requirements. The most significant changes are grouped below:

Password Management Enhancements:

  • Added a new variable manage_master_user_password to enable AWS Secrets Manager to manage the master user password, with validation to ensure only true or null are accepted.
  • Refactored local password generation logic in src/main.tf to handle three cases: AWS-managed password, user-provided password, or module-generated random password.
  • Updated the documentdb_cluster module configuration to pass the correct values for master_password and manage_master_user_password based on the new logic.

Provider and Resource Updates:

  • Updated the AWS provider version constraint to require at least version 5.29.0, ensuring compatibility with the new password management features.
  • Fixed a typo in the random_password resource by changing the argument from number to numeric for correct password generation.

why

  • This update introduces the manage_master_user_password variable, allowing the module to use AWS Secrets Manager for managing the DocumentDB master user password

references

Summary by CodeRabbit

  • New Features
    • Added an option to manage the master user password via AWS Secrets Manager.
    • Automatically generates a secure master password when not managed by Secrets Manager.
  • Changes
    • Updated password generation to use a refined numeric character set.
  • Chores
    • Raised the minimum required AWS provider version to 5.29.0.

This update introduces the `manage_master_user_password` variable, allowing
the module to use AWS Secrets Manager for managing the DocumentDB master
user password. The logic in `main.tf` is updated to handle three cases:
1) AWS-managed password, 2) user-provided password, and 3) auto-generated
password. Also updates the AWS provider version constraint and fixes a
typo in the random_password resource.
@RoseSecurity
Copy link
Contributor Author

/terratest

@coderabbitai
Copy link

coderabbitai bot commented Oct 9, 2025

Walkthrough

Introduces a conditional master password management flow, adds a new input variable manage_master_user_password, adjusts module wiring to pass this input and conditionally set master_password, updates random_password character option from number to numeric, and tightens AWS provider constraint to >= 5.29.0 and < 6.0.0.

Changes

Cohort / File(s) Summary
Password management flow + module wiring
src/main.tf, src/variables.tf
Adds variable manage_master_user_password (nullable bool with validation). Updates local/master password logic: create_password requires both master_password and manage_master_user_password to be null; computes local master_password via generated random password when applicable. Passes manage_master_user_password to module "documentdb_cluster" and sets module master_password to null when managing via Secrets Manager.
Password generation option tweak
src/ssm.tf
Changes random_password attribute from number to numeric, altering allowed character classes while keeping other constraints intact.
Provider version constraint
src/versions.tf
Updates AWS provider requirement to ">= 5.29.0, < 6.0.0".

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor U as User/Caller
  participant T as Terraform Root Module
  participant RP as random_password
  participant M as module "documentdb_cluster"
  participant SM as AWS Secrets Manager

  U->>T: Apply with variables (master_password, manage_master_user_password)
  alt manage_master_user_password == true
    T-->>M: master_password = null, manage_master_user_password = true
    M->>SM: Manage master user password in Secrets Manager
  else master_password == null and manage_master_user_password == null
    T->>RP: Generate random master password
    RP-->>T: random value
    T-->>M: master_password = local computed value, manage_master_user_password = null
  else master_password provided
    T-->>M: master_password = provided value, manage_master_user_password = null
  end
  note over M,SM: Module behavior uses either SM-managed secret or provided/computed password
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested labels

enhancement, needs-test

Suggested reviewers

  • goruha
  • oycyc

Poem

A whisk of whiskers, a hop through code,
I stash a secret on a safer road.
If flags are true, to Secrets I dart—
Else I nibble numbers, randomly smart.
Provider’s path now firmly set,
Passwords snug—no worries yet. 🐇🔐

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly highlights the addition of support for AWS-managed master passwords, reflecting the main feature and updated password management logic introduced in this PR.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch improve-manage-master-password-logic

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@mergify mergify bot requested review from a team October 9, 2025 13:35
@mergify mergify bot added the triage Needs triage label Oct 9, 2025
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/ssm.tf (1)

9-24: Do not generate or store a password when AWS manages it or user provides one

Both random_password.master_password and aws_ssm_parameter.master_password are created whenever local.enabled, causing:

  • Unused secrets generated and stored in SSM when manage_master_user_password = true.
  • A random secret stored even when a user-supplied master_password is used.

Gate these resources on local.create_password to align with main.tf logic and avoid leaking/creating unused secrets.

 resource "random_password" "master_password" {
-  count = local.enabled ? 1 : 0
+  count = local.create_password ? 1 : 0
   # character length
   length = 33

   special = false
   upper   = true
   lower   = true
   numeric = true
   min_special = 0
   min_upper   = 1
   min_lower   = 1
   min_numeric = 1
 }

 resource "aws_ssm_parameter" "master_password" {
-  count = local.enabled ? 1 : 0
+  count = local.create_password ? 1 : 0

   name  = "/${module.this.name}/master_password"
   type  = "SecureString"
   value = one(random_password.master_password[*].result)
 }

Also applies to: 26-32

🧹 Nitpick comments (2)
src/main.tf (1)

3-8: Password selection logic LGTM; add a mutual‑exclusivity guard

Current logic correctly handles the three cases. However, if both master_password and manage_master_user_password=true are set, the user password is silently ignored. Add a precondition to fail fast.

# For Terraform >= 1.2
module "documentdb_cluster" {
  # ...
  lifecycle {
    precondition {
      condition     = !(var.manage_master_user_password == true && var.master_password != null)
      error_message = "Do not set both master_password and manage_master_user_password=true."
    }
  }
  # ...
}
src/variables.tf (1)

43-51: Validation matches intended semantics

Allowing only null or true prevents accidental false and documents intent. Pair with a module-level precondition to forbid setting this alongside master_password.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3705896 and 2ec6acb.

📒 Files selected for processing (4)
  • src/main.tf (2 hunks)
  • src/ssm.tf (1 hunks)
  • src/variables.tf (1 hunks)
  • src/versions.tf (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Summary
🔇 Additional comments (2)
src/main.tf (1)

35-39: Module wiring correct; confirm module input & provider constraint
Null master_password when manage_master_user_password is set matches AWS semantics. Manually verify that cloudposse/documentdb-cluster/aws v0.30.2 declares the manage_master_user_password input and constrains the AWS provider to ≥ 5.29.0.

src/versions.tf (1)

7-7: Verify Terraform core version constraint
Confirm AWS provider v5.29.0’s minimum Terraform core version via the Terraform Registry or HashiCorp release notes (likely ≥ 1.3.0), then update required_version accordingly (e.g., tighten from “>= 1.0.0” to the verified minimum).

@goruha goruha added this pull request to the merge queue Oct 14, 2025
@mergify mergify bot removed the triage Needs triage label Oct 14, 2025
Merged via the queue into main with commit b30f951 Oct 14, 2025
19 checks passed
@goruha goruha deleted the improve-manage-master-password-logic branch October 14, 2025 19:18
@github-actions
Copy link

These changes were released in v1.537.4.

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.

3 participants