Skip to content

feat: allow secrets manager option #22

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

Merged
merged 14 commits into from
Jun 12, 2025
Merged

feat: allow secrets manager option #22

merged 14 commits into from
Jun 12, 2025

Conversation

nitrocode
Copy link
Contributor

@nitrocode nitrocode commented May 13, 2025

what

  • feat: allow secrets manager option
  • var to override db_port
  • var to override entire ssm param prefix to avoid org-account-env
  • vars to override aws asm secret inputs (policy, key, retention, etc)
  • Testing
  • Address rabbit code review

why

  • Secrets Manager integrates well with aurora
  • The implementation of SSM param store in this component will create a new param for each attribute.
  • The implementation of ASM will create the secrets in a single JSON string in a single secret due to cost which is $0.40 per secret per month.
  • This pr would be repeatable with aurora mysql component
  • Should this implementation be in a separate module?

references

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features
    • Added support for selecting the secrets store type (AWS Secrets Manager or SSM) for saving database credentials.
    • Introduced a configuration option to choose between "ASM" and "SSM" for secrets storage.
    • Outputs now display the location of the admin password based on the selected secrets store type.
    • Added AWS Secrets Manager resources for storing database credentials when ASM is enabled.
  • Enhancements
    • Secrets and parameter store resources are now conditionally created based on the chosen secrets store type.
    • Database port configuration is now managed through a variable for improved flexibility.
    • Conditional enabling of parameter store write operations based on secrets store type.
  • Bug Fixes
    • Improved accuracy of output descriptions to reflect the actual secrets store in use.

@nitrocode nitrocode requested review from a team as code owners May 13, 2025 15:35
Copy link

coderabbitai bot commented May 13, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

The changes introduce support for selecting between AWS Secrets Manager (ASM) and AWS Systems Manager Parameter Store (SSM) as the secrets storage backend. A new variable controls this selection, and conditional logic is applied throughout the configuration to enable or disable ASM/SSM resources accordingly. Dynamic port configuration and conditional resource creation are also implemented.

Changes

File(s) Change Summary
src/variables.tf Added secrets_store_type variable with validation for "SSM" or "ASM"; added mysql_db_port variable.
src/main.tf Added local variables asm_enabled and ssm_enabled to conditionally enable ASM/SSM features.
src/asm.tf Introduced conditional creation of AWS Secrets Manager resources for secrets and secret versions.
src/ssm.tf Made DB port dynamic using module.aurora_mysql.port and added conditional enablement of SSM parameter writes.
src/cluster-regional.tf Changed hardcoded DB port 3306 to variable var.mysql_db_port in Aurora MySQL module.
src/outputs.tf Modified outputs to conditionally show ASM or SSM password keys and generalized password location description.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Terraform
    participant ASM
    participant SSM

    User->>Terraform: Set secrets_store_type (ASM or SSM)
    Terraform->>Terraform: Evaluate asm_enabled and ssm_enabled
    alt ASM enabled
        Terraform->>ASM: Create secrets and secret versions
    else SSM enabled
        Terraform->>SSM: Write parameters to Parameter Store
    end
Loading

Assessment against linked issues

Objective Addressed Explanation
Support RDS managed DB password in Secrets Manager (#21): Input for secrets store type, conditional ASM resource creation
Mutually exclusive logic for secrets store type selection (#21)
Dynamic configuration for DB port and resource enablement (#21)

Poem

The secrets now can safely hide,
In ASM or SSM, you decide!
Ports are dynamic, no more fuss,
Conditionals keep things neat for us.
With a twitch of my nose and a hop of delight,
This code makes secrets storage just right!
((\
( -.-)
o_(")(")


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4671b6c and 32d5069.

📒 Files selected for processing (2)
  • src/asm.tf (1 hunks)
  • src/variables.tf (2 hunks)

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
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@mergify mergify bot added triage Needs triage needs-test Needs testing labels May 13, 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: 1

🧹 Nitpick comments (3)
src/variables.tf (1)

228-238: Inaccurate variable description for Aurora secrets store type.

The description refers to Datadog API/app keys, but this variable actually controls whether Aurora MySQL credentials are stored in SSM or ASM. Please update the description to accurately reflect its purpose.

src/cluster-regional.tf (1)

1-4: Parameterize the DB port instead of hardcoding.

Defining local.db_port = 3306 works, but making the port configurable via a Terraform variable (e.g., var.db_port) could improve flexibility if non-default ports are needed in the future.

src/asm.tf (1)

14-25: JSON payload for secret version is well-defined.

The jsonencode block includes all required fields (domain, host, port, name, credentials) and uses local.db_port for the port. This aligns with SSM usage and keeps consistency. Consider adding optional kms_key_id support if custom encryption keys are needed.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 43ded8c and 181d90f.

📒 Files selected for processing (5)
  • src/asm.tf (1 hunks)
  • src/cluster-regional.tf (2 hunks)
  • src/main.tf (1 hunks)
  • src/ssm.tf (2 hunks)
  • src/variables.tf (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: Mergify Merge Protections
  • GitHub Check: Mergify Merge Protections
  • GitHub Check: Summary
🔇 Additional comments (4)
src/cluster-regional.tf (1)

21-22: Consistency: Reference the local DB port.

Great job replacing the hardcoded 3306 with local.db_port. Ensure that the module’s db_port input expects a numeric value and aligns with this change.

Could you verify that the cloudposse/rds-cluster/aws module’s db_port input is of type number?

src/main.tf (1)

4-6: Conditional flags for ASM/SSM selection look correct.

The new local.asm_enabled and local.ssm_enabled flags appropriately gate resource creation based on var.secrets_store_type. This aligns with the PR objective to toggle between Secrets Manager and SSM.

src/ssm.tf (1)

78-79: Correct conditional enabling of SSM writes.

Using enabled = local.ssm_enabled correctly toggles the parameter store writes. This implements the intended conditional logic for SSM.

src/asm.tf (1)

1-6: Conditional creation of ASM secret resource is correct.

Using count = local.asm_enabled ? 1 : 0 properly gates the secret resource based on the selected store type. Good implementation.

Copy link
Contributor

There are no real tests for this component. So we set terratest statuses to successful execution without running any tests

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: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 181d90f and 4629302.

📒 Files selected for processing (6)
  • src/asm.tf (1 hunks)
  • src/cluster-regional.tf (2 hunks)
  • src/main.tf (1 hunks)
  • src/outputs.tf (1 hunks)
  • src/ssm.tf (2 hunks)
  • src/variables.tf (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
  • src/cluster-regional.tf
  • src/ssm.tf
  • src/main.tf
  • src/asm.tf
  • src/variables.tf
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Summary
🔇 Additional comments (2)
src/outputs.tf (2)

33-35: Conditional SSM key output
The aurora_mysql_master_password_ssm_key output properly exposes the SSM parameter name only when SSM is enabled and the DB is provisioned.


37-40: Conditional ASM key output
The new aurora_mysql_master_password_asm_key output cleanly exposes the Secrets Manager secret identifier when ASM is enabled, keeping outputs consistent.

nitrocode and others added 4 commits June 3, 2025 08:48
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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

🧹 Nitpick comments (3)
src/variables.tf (3)

36-36: Consider using null as the default for mysql_db_name
Currently an empty string ("") signals "no database," which can be ambiguous in Terraform. Switching to default = null and updating your logic (var.mysql_db_name != null) makes the intent clearer and leverages Terraform's native handling of null values.


40-44: Add validation for mysql_db_port to enforce a valid port range
While the default of 3306 is correct, it’s easy for users to misconfigure this. Add a validation block to ensure the port is between 1 and 65535, e.g.:

validation {
  condition     = var.mysql_db_port >= 1 && var.mysql_db_port <= 65535
  error_message = "mysql_db_port must be between 1 and 65535."
}

This will fail fast on invalid input.


235-244: Refactor secrets_store_type validation to use contains for easier future extensions
Great addition of this toggle. You can simplify the logic and make it more extensible by using:

validation {
  condition     = contains(["SSM", "ASM"], var.secrets_store_type)
  error_message = "secrets_store_type must be one of: SSM, ASM."
}

This approach scales better if you add more backends later.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4629302 and 4671b6c.

📒 Files selected for processing (3)
  • src/cluster-regional.tf (1 hunks)
  • src/ssm.tf (2 hunks)
  • src/variables.tf (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/cluster-regional.tf
  • src/ssm.tf
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Summary

@goruha goruha enabled auto-merge June 12, 2025 08:25
@mergify mergify bot removed the triage Needs triage label Jun 12, 2025
@goruha goruha added this pull request to the merge queue Jun 12, 2025
Merged via the queue into main with commit d77d6df Jun 12, 2025
13 of 14 checks passed
@goruha goruha deleted the asm branch June 12, 2025 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-test Needs testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support RDS managed DB password in Secrets Manager
2 participants