feat: Allow overriding Performance Insights KMS key#95
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 57 minutes and 25 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a new optional input variable Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/cluster-regional.tf (1)
37-37: Override wiring is correct; consider an ARN-format validation on the variable.
coalesce(var.performance_insights_kms_key_arn, module.kms_key_rds.key_arn)behaves as intended: when the override isnull,coalesceskips it and returns the RDS CMK ARN; when set, the user-supplied ARN wins. Note thatcoalescealso treats an empty string""as a valid value and would pass it through to the upstream module, causing an AWS API error at apply time. Since the input is semantically an ARN, consider adding a lightweight validation on the variable to reject empty strings and enforce anarn:prefix, e.g.:🛡️ Optional validation on performance_insights_kms_key_arn
variable "performance_insights_kms_key_arn" { type = string default = null description = "ARN of the KMS key for Performance Insights encryption. If null, defaults to the RDS CMK." + + validation { + condition = var.performance_insights_kms_key_arn == null || can(regex("^arn:aws[a-zA-Z-]*:kms:", var.performance_insights_kms_key_arn)) + error_message = "performance_insights_kms_key_arn must be null or a valid KMS key ARN (starting with 'arn:aws:kms:')." + } }Also worth confirming: the upstream
cloudposse/rds-cluster/awsv2.4.0 inputperformance_insights_kms_key_idaccepts an ARN (AWS allows either a key ID or ARN here), which is consistent with howkms_key_arnis used on line 36.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cluster-regional.tf` at line 37, Summary: coalesce(...) will accept an empty string and pass it through causing AWS apply errors; add validation to reject empty strings and enforce ARN format on the override variable. Update the variable declaration for performance_insights_kms_key_arn: make sure its default is null (not ""), and add a validation block that allows null OR a string matching an ARN prefix (e.g., condition: var.performance_insights_kms_key_arn == null || can(regex("^arn:", var.performance_insights_kms_key_arn))), with a clear error_message; this ensures the coalesce in performance_insights_kms_key_id continues to prefer the override but will never receive an empty string or non-ARN value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/cluster-regional.tf`:
- Line 37: Summary: coalesce(...) will accept an empty string and pass it
through causing AWS apply errors; add validation to reject empty strings and
enforce ARN format on the override variable. Update the variable declaration for
performance_insights_kms_key_arn: make sure its default is null (not ""), and
add a validation block that allows null OR a string matching an ARN prefix
(e.g., condition: var.performance_insights_kms_key_arn == null ||
can(regex("^arn:", var.performance_insights_kms_key_arn))), with a clear
error_message; this ensures the coalesce in performance_insights_kms_key_id
continues to prefer the override but will never receive an empty string or
non-ARN value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c8e84bf-0f6e-441f-8a65-91b75106c3de
📒 Files selected for processing (2)
src/cluster-regional.tfsrc/variables.tf
Add performance_insights_kms_key_arn variable to allow specifying a custom KMS key ARN for Performance Insights encryption. When null (default), falls back to the component's RDS CMK. This is needed when a cluster was initially created with PI disabled (or with a different KMS key), and PI is later enabled via Terraform. In that scenario, the cluster already has a PI KMS key assigned by AWS (typically the default aws/rds managed key), and the component unconditionally tries to set the RDS CMK — which AWS rejects because PI KMS keys cannot be changed after cluster creation. The new variable allows users to pass the ARN of the key already in use, preventing the InvalidParameterCombination error: "You can't change your Performance Insights KMS key." Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
7753c69 to
e3d73ee
Compare
|
/terratest |
What
Add a
performance_insights_kms_key_arnvariable to allow users to specify a custom KMS key ARN for Performance Insights encryption. Whennull(default), the component falls back to the existing RDS CMK — preserving current behavior.Why
If the cluster already has a PI KMS key assigned by AWS (typically the default
aws/rdsmanaged key — e.g. from a prior manual enablement or a partially applied change), AWS rejects the modification with:This is because Performance Insights KMS keys are immutable after cluster creation. There is currently no way to tell the component to use a different key, forcing users to work around this with lifecycle ignore rules or manual state manipulation.
Changes
src/variables.tf: Newperformance_insights_kms_key_arnvariable (string, defaultnull)src/cluster-regional.tf: Usecoalesce(var.performance_insights_kms_key_arn, module.kms_key_rds.key_arn)so the override takes precedence when setUsage
Generated with Claude Opus 4.6
Summary by CodeRabbit