-
-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
Caution Review failedThe pull request is closed. WalkthroughThe 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
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
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed 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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this 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 useslocal.db_port
for the port. This aligns with SSM usage and keeps consistency. Consider adding optionalkms_key_id
support if custom encryption keys are needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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
withlocal.db_port
. Ensure that the module’sdb_port
input expects a numeric value and aligns with this change.Could you verify that the
cloudposse/rds-cluster/aws
module’sdb_port
input is of typenumber
?src/main.tf (1)
4-6
: Conditional flags for ASM/SSM selection look correct.The new
local.asm_enabled
andlocal.ssm_enabled
flags appropriately gate resource creation based onvar.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.
There are no real tests for this component. So we set terratest statuses to successful execution without running any tests |
There was a problem hiding this 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
📒 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
Theaurora_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 newaurora_mysql_master_password_asm_key
output cleanly exposes the Secrets Manager secret identifier when ASM is enabled, keeping outputs consistent.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this 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 usingnull
as the default formysql_db_name
Currently an empty string (""
) signals "no database," which can be ambiguous in Terraform. Switching todefault = 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 formysql_db_port
to enforce a valid port range
While the default of3306
is correct, it’s easy for users to misconfigure this. Add avalidation
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
: Refactorsecrets_store_type
validation to usecontains
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
📒 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
what
db_port
why
references
Summary by CodeRabbit
Summary by CodeRabbit