feat: add the ability to write msk connection strings to ssm #35
Conversation
WalkthroughAdds optional SSM Parameter Store support for MSK connection details: new variables, Terraform logic to build and write SSM parameters, an output exposing SSM key paths, README/test updates, and minor Go test type updates and assertion for the new output. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TerraformModule as Module
participant SSM as SSMParameterStore
User->>Module: configure var.ssm_parameters_enabled (true/false)
Module->>Module: collect Kafka & ZK outputs
alt enabled
Module->>SSM: write filtered SecureString parameters (via parameter_store_write module)
SSM-->>Module: return names (module.parameter_store_write[*].names)
Module->>User: expose ssm_key_paths output
else disabled
Module-->>SSM: skip writing parameters
Module->>User: ssm_key_paths = null
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ Finishing Touches🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/fixtures/stacks/catalog/usecase/basic.yaml (1)
57-59: Same note as the disabled stackIf the fix in
src/ssm.tfis applied, this fixture is fine. Otherwise the plan will also break when the MSK component is toggled off in other scenarios.
🧹 Nitpick comments (2)
README.yaml (1)
72-74: Missing documentation for the two complementary variablesThe snippet now shows
ssm_parameters_enabled, butssm_path_prefixandssm_cluster_name_override– which are required to control the actual SSM path – are absent. Mentioning them (even with defaults) will spare users a trip to the codebase.src/ssm.tf (1)
72-76: Pin the module source to a commit SHA for supply-chain safetyCheckov flagged
CKV_TF_1. Using a fixed version is good, but pinning to a commit hash (e.g.?ref=sha) prevents unexpected changes if0.13.0is re-tagged.-source = "cloudposse/ssm-parameter-store/aws" -version = "0.13.0" +source = "cloudposse/ssm-parameter-store/aws?ref=6c4d7e5"Update when bumping versions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.yaml(1 hunks)src/ssm.tf(1 hunks)src/variables.tf(1 hunks)test/fixtures/stacks/catalog/usecase/basic.yaml(1 hunks)test/fixtures/stacks/catalog/usecase/disabled.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: RoseSecurity
PR: cloudposse-terraform-components/aws-msk#21
File: src/main.tf:10-10
Timestamp: 2025-05-14T15:10:36.045Z
Learning: The CloudPosse MSK Apache Kafka Cluster module (cloudposse/msk-apache-kafka-cluster/aws) supports the `enabled` flag for conditionally creating/destroying resources, even though this input variable might not be explicitly documented. This is consistent with the CloudPosse module pattern where modules inherit behavior from the context object.
test/fixtures/stacks/catalog/usecase/disabled.yaml (1)
Learnt from: RoseSecurity
PR: cloudposse-terraform-components/aws-msk#21
File: src/main.tf:10-10
Timestamp: 2025-05-14T15:10:36.045Z
Learning: The CloudPosse MSK Apache Kafka Cluster module (cloudposse/msk-apache-kafka-cluster/aws) supports the `enabled` flag for conditionally creating/destroying resources, even though this input variable might not be explicitly documented. This is consistent with the CloudPosse module pattern where modules inherit behavior from the context object.
test/fixtures/stacks/catalog/usecase/basic.yaml (1)
Learnt from: RoseSecurity
PR: cloudposse-terraform-components/aws-msk#21
File: src/main.tf:10-10
Timestamp: 2025-05-14T15:10:36.045Z
Learning: The CloudPosse MSK Apache Kafka Cluster module (cloudposse/msk-apache-kafka-cluster/aws) supports the `enabled` flag for conditionally creating/destroying resources, even though this input variable might not be explicitly documented. This is consistent with the CloudPosse module pattern where modules inherit behavior from the context object.
README.yaml (1)
Learnt from: RoseSecurity
PR: cloudposse-terraform-components/aws-msk#21
File: src/main.tf:10-10
Timestamp: 2025-05-14T15:10:36.045Z
Learning: The CloudPosse MSK Apache Kafka Cluster module (cloudposse/msk-apache-kafka-cluster/aws) supports the `enabled` flag for conditionally creating/destroying resources, even though this input variable might not be explicitly documented. This is consistent with the CloudPosse module pattern where modules inherit behavior from the context object.
src/ssm.tf (1)
Learnt from: RoseSecurity
PR: cloudposse-terraform-components/aws-msk#21
File: src/main.tf:10-10
Timestamp: 2025-05-14T15:10:36.045Z
Learning: The CloudPosse MSK Apache Kafka Cluster module (cloudposse/msk-apache-kafka-cluster/aws) supports the `enabled` flag for conditionally creating/destroying resources, even though this input variable might not be explicitly documented. This is consistent with the CloudPosse module pattern where modules inherit behavior from the context object.
src/variables.tf (1)
Learnt from: RoseSecurity
PR: cloudposse-terraform-components/aws-msk#21
File: src/main.tf:10-10
Timestamp: 2025-05-14T15:10:36.045Z
Learning: The CloudPosse MSK Apache Kafka Cluster module (cloudposse/msk-apache-kafka-cluster/aws) supports the `enabled` flag for conditionally creating/destroying resources, even though this input variable might not be explicitly documented. This is consistent with the CloudPosse module pattern where modules inherit behavior from the context object.
🪛 Checkov (3.2.334)
src/ssm.tf
[MEDIUM] 72-81: Ensure Terraform module sources use a commit hash
(CKV_TF_1)
⏰ 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 (1)
test/fixtures/stacks/catalog/usecase/disabled.yaml (1)
57-59: Potentially inconsistent flags when the component is disabled
enabled: falsedisables the whole component, yetssm_parameters_enabled: trueis set.
Becausesrc/ssm.tfignores the top-levelenabledflag, Terraform will still attempt to
evaluatemodule.kafka.*outputs and fail when the module count is zero (see comment insrc/ssm.tf).
Please keep these flags aligned or fix the gating logic.
|
/terratest |
goruha
left a comment
goruha
left a comment
There was a problem hiding this comment.
@RoseSecurity could you add an assertion in tests to verify connection strings saved to the SSM parameter store?
logic to avoid invalid count
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/ssm.tf (1)
1-70: Critical: Locals block still doesn't honor the component-levelenabledflagThe locals block directly references
module.kafka.*outputs without conditional guards. When the kafka module is disabled (enabled = false), these outputs don't exist, causing "Invalid reference" errors during Terraform planning.Apply the suggested fix from the previous review:
locals { ssm_path_prefix = length(var.ssm_cluster_name_override) > 0 ? format("/%s/%s", var.ssm_path_prefix, var.ssm_cluster_name_override) : format("/%s/%s", var.ssm_path_prefix, module.this.name) - all_kafka_parameters = [ + all_kafka_parameters = var.ssm_parameters_enabled && local.enabled ? [ { name = format("%s/broker_endpoints", local.ssm_path_prefix) value = join(",", module.kafka.broker_endpoints) description = "List of broker endpoints" type = "SecureString" }, # ... rest of parameters unchanged ... - ] + ] : [] + kafka_parameters = [ for p in local.all_kafka_parameters : p if p.value != "" && p.value != null ] }
🧹 Nitpick comments (1)
src/ssm.tf (1)
72-81: Consider pinning the module to a specific commit hash for better reproducibilityWhile the current version constraint is acceptable, using a commit hash provides better reproducibility and security.
Consider updating the module source to use a commit hash:
module "parameter_store_write" { - source = "cloudposse/ssm-parameter-store/aws" - version = "0.13.0" + source = "git::https://github.com/cloudposse/terraform-aws-ssm-parameter-store.git?ref=<commit-hash>"However, using version constraints is also a valid approach and may be preferred in some organizations for easier dependency management.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/outputs.tf(1 hunks)src/ssm.tf(1 hunks)test/component_test.go(2 hunks)test/fixtures/stacks/catalog/usecase/basic.yaml(1 hunks)test/fixtures/stacks/catalog/usecase/disabled.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/fixtures/stacks/catalog/usecase/disabled.yaml
- test/fixtures/stacks/catalog/usecase/basic.yaml
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: RoseSecurity
PR: cloudposse-terraform-components/aws-msk#21
File: src/main.tf:10-10
Timestamp: 2025-05-14T15:10:36.045Z
Learning: The CloudPosse MSK Apache Kafka Cluster module (cloudposse/msk-apache-kafka-cluster/aws) supports the `enabled` flag for conditionally creating/destroying resources, even though this input variable might not be explicitly documented. This is consistent with the CloudPosse module pattern where modules inherit behavior from the context object.
src/outputs.tf (1)
Learnt from: RoseSecurity
PR: cloudposse-terraform-components/aws-msk#21
File: src/main.tf:10-10
Timestamp: 2025-05-14T15:10:36.045Z
Learning: The CloudPosse MSK Apache Kafka Cluster module (cloudposse/msk-apache-kafka-cluster/aws) supports the `enabled` flag for conditionally creating/destroying resources, even though this input variable might not be explicitly documented. This is consistent with the CloudPosse module pattern where modules inherit behavior from the context object.
src/ssm.tf (1)
Learnt from: RoseSecurity
PR: cloudposse-terraform-components/aws-msk#21
File: src/main.tf:10-10
Timestamp: 2025-05-14T15:10:36.045Z
Learning: The CloudPosse MSK Apache Kafka Cluster module (cloudposse/msk-apache-kafka-cluster/aws) supports the `enabled` flag for conditionally creating/destroying resources, even though this input variable might not be explicitly documented. This is consistent with the CloudPosse module pattern where modules inherit behavior from the context object.
🪛 Checkov (3.2.334)
src/ssm.tf
[MEDIUM] 72-81: Ensure Terraform module sources use a commit hash
(CKV_TF_1)
⏰ 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 (3)
src/outputs.tf (1)
100-103: LGTM! Well-structured output addition.The new output follows the established pattern and correctly exposes the SSM parameter key paths created by the parameter store module.
test/component_test.go (2)
27-27: Good modernization of Go type declaration.Using
anyinstead ofinterface{}is the preferred approach in Go 1.18+.
58-59: Appropriate test coverage for the new SSM functionality.The assertion validates that the expected number of SSM parameters are created, providing good test coverage for the new feature.
|
/terratest |
1 similar comment
|
/terratest |
|
I may have to rework the logic as it appears to fail with an |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
mergify
bot
added
stale
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
mergify
bot
added
stale
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
mergify
bot
added
stale
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
mergify
bot
added
stale
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
mergify
bot
added
stale
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
mergify
bot
added
stale
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
mergify
bot
added
stale
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
mergify
bot
added
stale
Why
mskcomponent to write connection strings to SSM.What
ssm.tffile that writes MSK and Zookeeper connection strings to SSMUsage
Testing
atmos validate stacksatmos terraform planon componentReferences
Summary by CodeRabbit
New Features
Documentation
Tests