feat: support network_stack parameter #19
Conversation
WalkthroughThe documentation and Terraform configuration were updated to clarify and expand networking options for the "runs-on" component. New variables and outputs were added for flexible networking and security group management, including support for embedded or external VPCs. Deprecated sections and unused modules were removed. Documentation was reorganized and expanded accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Terraform
participant AWS
User->>Terraform: Configure runs-on with networking_stack ("embedded" or "external")
Terraform->>AWS: If "embedded", create VPC, subnets, security group via CloudFormation
Terraform->>AWS: If "external", use provided vpc_id, subnet_ids, security_group_id
alt security_group_id not provided and "external"
Terraform->>AWS: Create security group using module
Terraform->>AWS: Apply security_group_rules if provided
end
AWS-->>Terraform: Return outputs (including security_group_id)
Terraform-->>User: Output networking and security group info
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)src/main.tf (1)🪛 LanguageToolsrc/README.md[uncategorized] ~291-~291: A period might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION_PERIOD) [uncategorized] ~292-~292: A comma might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA) [grammar] ~297-~297: Did you mean “too false to”? (TOO_ADJECTIVE_TO) [grammar] ~299-~299: The verb ‘keep’ seems to be in the wrong form here. (FOR_VB) [typographical] ~304-~304: Consider adding a comma after ‘Usually’ for more clarity. (RB_LY_COMMA) [uncategorized] ~304-~304: Possible missing preposition found. (AI_EN_LECTOR_MISSING_PREPOSITION) [typographical] ~305-~305: Consider adding a comma after ‘Usually’ for more clarity. (RB_LY_COMMA) [typographical] ~307-~307: Do not use a colon (:) before a series that is introduced by a preposition (‘of’). Remove the colon or add a noun or a noun phrase after the preposition. (RP_COLON) 🪛 markdownlint-cli2 (0.17.2)src/README.md306-306: Bare URL used (MD034, no-bare-urls) 🪛 Checkov (3.2.334)src/main.tf[MEDIUM] 71-80: Ensure Terraform module sources use a commit hash (CKV_TF_1) ⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (24)
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 are no real tests for this component. So we set terratest statuses to successful execution without running any tests |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
README.yaml (1)
47-53: Fill in the external networking example
The "Usage with external networking" section is currently empty. To guide users switching to an external VPC, please add a concrete YAML snippet demonstrating how to setnetworking_stack = "external", providevpc_id,subnet_ids, and optionallysecurity_group_idand rules.README.md (1)
71-76: Populate external networking example block
The new "Usage with external networking" placeholder needs a real-world example. Consider illustrating how to setnetworking_stack: external, and passvpc_id,subnet_ids, andsecurity_group_id. Would you like assistance drafting this snippet?src/variables.tf (1)
81-92: Suggest adding validation forsecurity_group_rules
Currently any string is allowed in thetypefield. To prevent user errors, add a validation block to ensuretypeis either"ingress"or"egress".Example diff:
variable "security_group_rules" { type = list(object({ type = string from_port = number to_port = number protocol = string cidr_blocks = list(string) })) description = "Security group rules..." nullable = true default = null + validation { + condition = alltrue([for r in var.security_group_rules : contains(["ingress","egress"], r.type)]) + error_message = "Each security_group_rules.type must be either 'ingress' or 'egress'." + } }src/README.md (3)
19-46: Clarify inline comment verbosity in Defaults snippet
ThePrivate: alwaysinline comment in the example (lines 39–45) is quite verbose and may distract from the core snippet. Consider moving this explanation to the variable’s description further down (Inputs table) or simplifying it, for example:- Private: always # always | true | false - Always will default place in private subnet... + Private: always # Placement strategy: always | true | false
283-299: Minor grammar & punctuation improvements
Several input descriptions (lines 283–299) have missing commas/periods or colon usage issues. A quick proofread to ensure each sentence ends with a period and prepositions/colons follow style guidelines will improve readability.🧰 Tools
🪛 LanguageTool
[uncategorized] ~283-~283: A period might be missing here.
Context: ...s_list_of_maps. Not added totagsorid`.
This is for some rare cases wher...(AI_EN_LECTOR_MISSING_PUNCTUATION_PERIOD)
[uncategorized] ~284-~284: A comma might be missing here.
Context: ...(e.g.workersorcluster) to add toid,
in the order they appear in the ...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[grammar] ~289-~289: Did you mean “too false to”?
Context: ..."> enabled | Set to false to prevent the module from creating any re...(TOO_ADJECTIVE_TO)
[grammar] ~291-~291: The verb ‘keep’ seems to be in the wrong form here.
Context: ... for unlimited length.
Set tonullfor keep the existing setting, which defaults to...(FOR_VB)
[typographical] ~296-~296: Consider adding a comma after ‘Usually’ for more clarity.
Context: ... name | ID element. Usually the component or solution name, e.g. 'a...(RB_LY_COMMA)
[uncategorized] ~296-~296: Possible missing preposition found.
Context: ...ID element not also included as atag.
The "name" tag is set to the fullids...(AI_EN_LECTOR_MISSING_PREPOSITION)
[typographical] ~297-~297: Consider adding a comma after ‘Usually’ for more clarity.
Context: ...space](#input_namespace) | ID element. Usually an abbreviation of your organization na...(RB_LY_COMMA)
[typographical] ~299-~299: Do not use a colon (:) before a series that is introduced by a preposition (‘of’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ... stack creation fails. This must be one of:DO_NOTHING,ROLLBACK, orDELETE|...(RP_COLON)
🪛 markdownlint-cli2 (0.17.2)
298-298: Bare URL used
null(MD034, no-bare-urls)
298-306: Group networking inputs for discoverability
The new inputs (networking_stack,vpc_id,subnet_ids,security_group_id,security_group_rules) are spread out in the table. Consider moving them into a contiguous block under an “Networking” sub-section in the Inputs table, and cross-reference the External networking examples to help users find them quickly.Also applies to: 312-313
🧰 Tools
🪛 LanguageTool
[typographical] ~299-~299: Do not use a colon (:) before a series that is introduced by a preposition (‘of’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ... stack creation fails. This must be one of:DO_NOTHING,ROLLBACK, orDELETE|...(RP_COLON)
🪛 markdownlint-cli2 (0.17.2)
298-298: Bare URL used
null(MD034, no-bare-urls)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.md(4 hunks)README.yaml(2 hunks)src/README.md(5 hunks)src/main.tf(3 hunks)src/outputs.tf(1 hunks)src/remote-state.tf(0 hunks)src/variables.tf(1 hunks)
💤 Files with no reviewable changes (1)
- src/remote-state.tf
🧰 Additional context used
🪛 LanguageTool
src/README.md
[uncategorized] ~283-~283: A period might be missing here.
Context: ...s_list_of_maps. Not added to tagsorid`.
This is for some rare cases wher...
(AI_EN_LECTOR_MISSING_PUNCTUATION_PERIOD)
[uncategorized] ~284-~284: A comma might be missing here.
Context: ...(e.g. workers or cluster) to add to id,
in the order they appear in the ...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[grammar] ~289-~289: Did you mean “too false to”?
Context: ..."> enabled | Set to false to prevent the module from creating any re...
(TOO_ADJECTIVE_TO)
[grammar] ~291-~291: The verb ‘keep’ seems to be in the wrong form here.
Context: ... for unlimited length.
Set to null for keep the existing setting, which defaults to...
(FOR_VB)
[typographical] ~296-~296: Consider adding a comma after ‘Usually’ for more clarity.
Context: ... name | ID element. Usually the component or solution name, e.g. 'a...
(RB_LY_COMMA)
[uncategorized] ~296-~296: Possible missing preposition found.
Context: ...ID element not also included as a tag.
The "name" tag is set to the full id s...
(AI_EN_LECTOR_MISSING_PREPOSITION)
[typographical] ~297-~297: Consider adding a comma after ‘Usually’ for more clarity.
Context: ...space](#input_namespace) | ID element. Usually an abbreviation of your organization na...
(RB_LY_COMMA)
[typographical] ~299-~299: Do not use a colon (:) before a series that is introduced by a preposition (‘of’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ... stack creation fails. This must be one of: DO_NOTHING, ROLLBACK, or DELETE |...
(RP_COLON)
🪛 markdownlint-cli2 (0.17.2)
src/README.md
298-298: Bare URL used
null
(MD034, no-bare-urls)
🪛 Checkov (3.2.334)
src/main.tf
[MEDIUM] 71-80: Ensure Terraform module sources use a commit hash
(CKV_TF_1)
🪛 GitHub Actions: Branch
src/main.tf
[error] 114-114: Terraform error: Duplicate local value definition 'vpc_id'. A local value named 'vpc_id' was already defined at main.tf:4,3-90. Local value names must be unique within a module.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (15)
src/outputs.tf (1)
49-52: Approve new security_group_id output
The addition of thesecurity_group_idoutput correctly exposes the chosen or created security group ID for downstream modules. It aligns with the new networking model.README.yaml (1)
15-15: Approve clarification of embedded networking example
Updating the snippet introduction to explicitly call out embedded networking improves clarity for users getting started.README.md (3)
39-39: Approve embedded networking snippet heading
Making it clear that the example is for embedded networking enhances readability.
78-78: Approve new embedded refarch heading
Relocating and renaming the Transit Gateway instructions under "Using embedded networking with refarch components" is clear and consistent.
80-80: Approve deprecation label for Transit Gateway
Marking the Transit Gateway setup as deprecated and advising external networking simplifies the guidance.src/variables.tf (4)
41-50: Approvenetworking_stackvariable
The newnetworking_stackinput with validation strictly enforces allowed values and defaults sensibly to"embedded".
52-61: Approvevpc_idvariable
Makingvpc_idnullable with a validation rule ensures external mode can't be misconfigured.
63-72: Approvesubnet_idsvariable
Requiringsubnet_idswhen using external networking is well-enforced via the validation block.
74-79: Approvesecurity_group_idvariable
Allowing users to provide an existing security group or default to creation covers both embedded and external scenarios.src/main.tf (1)
12-18: Approve merging networking locals into parameters
Themerge()invocation cleanly extends the CloudFormation parameters map with networking keys only when provided.src/README.md (5)
49-74: Embedded networking section is clear and comprehensive
The new “Embedded networking” heading, description, and YAML example correctly illustrate how to setnetworking_stack: embeddedand supplyVpcCidrBlock. This aligns well with the feature objective.
203-243: External networking examples look solid
The “External networking” section and accompanying_defaults.yaml/runs-on.yamlexamples demonstrate how to wire up an existing VPC, subnets, and security group via SSM. Consider adding a brief note about usingsecurity_group_rulesif consumers need to customize ingress/egress on the supplied group.
276-276: Resource addition approved
The newaws_security_group_rule.thisentry under Resources will enable fine-grained SG rule management.
326-326: Output addition approved
Includingsecurity_group_idin Outputs completes the round-trip from input → resource → output. Consumers can now reference the SG directly.
266-270:✅ Verification successful
Verify compatibility of bumped module versions
You’ve bumpediam_policyto 2.0.2 and addedsecurity_group2.2.0. Please confirm these releases are compatible and have no breaking changes for existing consumers.
🏁 Script executed:
#!/usr/bin/env bash # Check available versions for Cloud Posse modules on Terraform Registry for module in iam-policy security-group; do echo "Available versions of $module/aws:" curl -s "https://registry.terraform.io/v1/modules/cloudposse/$module/aws/versions" \ | jq -r '.modules[0].versions[].version' doneLength of output: 739
🏁 Script executed:
#!/usr/bin/env bash # Fetch CHANGELOG entries for the bumped versions to check for breaking changes for module in iam-policy security-group; do if [ "$module" = "iam-policy" ]; then repo="terraform-aws-iam-policy" version="2.0.2" else repo="terraform-aws-security-group" version="2.2.0" fi echo "=== Changelog for $module (v$version) ===" curl -s "https://raw.githubusercontent.com/cloudposse/${repo}/v${version}/CHANGELOG.md" \ | sed -n '1,50p' echo doneLength of output: 727
🏁 Script executed:
#!/usr/bin/env bash # Check GitHub releases for release notes on the bumped versions repos=("terraform-aws-iam-policy" "terraform-aws-security-group") versions=("2.0.2" "2.2.0") for i in "${!repos[@]}"; do repo="${repos[$i]}" version="${versions[$i]}" echo "=== Checking releases for $repo @ $version ===" found=false for tag in "v${version}" "${version}"; do echo "-- Attempting tag '$tag' --" response=$(curl -s "https://api.github.com/repos/cloudposse/$repo/releases/tags/$tag") tag_name=$(echo "$response" | jq -r '.tag_name // empty') if [ -n "$tag_name" ]; then echo "Found release: $tag_name" echo "Release notes (first 20 lines):" echo "$response" | jq -r '.body' | head -n 20 found=true break else echo "No release found for tag '$tag'" fi done if ! $found; then echo "⚠️ Could not locate a GitHub release for $repo@$version. Please check the repository directly." fi echo doneLength of output: 33027
Module versions 2.0.2 and 2.2.0 have no breaking changes
Release notes for both bumped modules do not list any breaking changes:• iam-policy v2.0.2 – adds support for policy attachments (new feature only)
• security-group v2.2.0 – minor fixes (editorconfig typo, template sync)Both upgrades are safe to merge.
|
/terratest |
|
/terratest |
|
/terratest |
|
💥 This pull request now has conflicts. Could you fix it @dudymas? 🙏 |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces support for an external networking stack alongside the existing embedded option, cleans up unused VPC peering state, and updates documentation and outputs to reflect the new parameters.
- Added
networking_stack,vpc_id,subnet_ids,security_group_id, andsecurity_group_rulesvariables with conditional validation - Removed the VPC peering remote-state module and introduced a
security_groupmodule plus associated data sources and rules - Updated outputs and README (both Markdown and YAML) to document embedded vs. external networking usage
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/variables.tf | Defined new networking variables (networking_stack, vpc_id, subnet_ids, etc.) |
| src/remote-state.tf | Removed deprecated VPC peering remote-state module |
| src/main.tf | Added locals for external networking, security group module, AWS data sources, and merge logic |
| src/outputs.tf | Added security_group_id to outputs |
| src/README.md | Expanded usage section with embedded/external networking examples |
| README.yaml | Synced metadata and usage updates for external networking |
| README.md | Mirrored README changes to root file |
Comments suppressed due to low confidence (3)
src/main.tf:77
local.vpc_idis referenced before it's declared (the later locals block defines it). Consider usingvar.vpc_iddirectly or moving thelocal.vpc_iddefinition into the first locals block so it's available here.
vpc_id = local.vpc_id
src/main.tf:85
local.security_group_idisn't defined in the initial locals block, causing a reference error here. You may want to coalescevar.security_group_idandmodule.security_group.idor define this local earlier.
security_group_id = local.security_group_id
src/variables.tf:81
- Add a
validationblock forsecurity_group_rulesto enforce that each rule'stypeis only"ingress"or"egress", preventing invalid inputs.
variable "security_group_rules" {
Benbentwo
added
major
|
These changes were released in v2.0.0. |
what
why
customized VPCs
Summary by CodeRabbit
New Features
Documentation
Chores