Add VPC component with configurable subnets and optional NAT gateways#2
Add VPC component with configurable subnets and optional NAT gateways#2
Conversation
Co-authored-by: sekka1 <575972+sekka1@users.noreply.github.com>
…into copilot/create-vpc-component
🍹
|
|
/pulumi up |
|
🚀 Pulumi up triggered for staging environment. Check the Actions tab for progress. |
| availabilityZones: | ||
| - "us-west-2a" | ||
| - "us-west-2b" | ||
| - "us-west-2c" |
Co-authored-by: sekka1 <575972+sekka1@users.noreply.github.com>
|
/pulumi up |
|
🚀 Pulumi up triggered for staging environment. Check the Actions tab for progress. |
pulumi/components/aws/vpc/vpc.py
Outdated
|
|
||
| # Create public subnets | ||
| public_subnets = [] | ||
| for i in range(3): |
There was a problem hiding this comment.
@copilot can this take in 2 subnets if the user only input two?
There was a problem hiding this comment.
Yes, the component now supports flexible subnet counts. You can provide 2, 3, or more subnets by adjusting the length of the publicSubnetCidrs and privateSubnetCidrs lists. The component automatically determines the count from the input. Added test coverage and documentation in commit 06681c6.
pulumi/components/aws/vpc/vpc.py
Outdated
|
|
||
| # Create private subnets | ||
| private_subnets = [] | ||
| for i in range(3): |
There was a problem hiding this comment.
@copilot can this take in 2 subnets if the user only wants two?
There was a problem hiding this comment.
Yes, the component supports flexible subnet counts (2, 3, or more). Provide the desired number of CIDR blocks in the lists and the component will create that many subnets. See commit 06681c6 for the implementation.
sekka1
left a comment
sekka1
left a comment
There was a problem hiding this comment.
@copilot there was an error in the pulumi up run: https://github.com/ManagedKube/devops-with-ai/actions/runs/20079642943/job/57603170078
Can you also check this?
Co-authored-by: sekka1 <575972+sekka1@users.noreply.github.com>
Co-authored-by: sekka1 <575972+sekka1@users.noreply.github.com>
Fixed the error in commit 391b74f. The issue was that helper functions were defined inside the loop, causing all subnets to get the same CIDR block and availability zone due to closure issues. The functions have been moved outside the loop, and all tests pass. |
|
/pulumi up |
|
🚀 Pulumi up triggered for staging environment. Check the Actions tab for progress. |
|
@copilot there is still another error after running your new changes: https://github.com/ManagedKube/devops-with-ai/actions/runs/20080218633/job/57605223624, can you fix? |
Co-authored-by: sekka1 <575972+sekka1@users.noreply.github.com>
Fixed in commit 378996a. The issue was a Python closure problem - lambdas were capturing the loop variable |
|
/pulumi up |
|
🚀 Pulumi up triggered for staging environment. Check the Actions tab for progress. |
|
@copilot there are still error after your last update: https://github.com/ManagedKube/devops-with-ai/actions/runs/20080593753/job/57606444528 |
The code fix in commit 378996a is working correctly - the component now properly assigns unique CIDR blocks and AZs to each subnet. However, AWS still has orphaned subnets from previous failed deployments that conflict with CIDRs To resolve this, run Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
🍹
|
|
/pulumi up |
|
🚀 Pulumi up triggered for staging environment. Check the Actions tab for progress. |
There was a problem hiding this comment.
Pull request overview
This PR implements a reusable VPC Pulumi component with comprehensive testing and instantiation in the staging environment. The component creates a complete VPC infrastructure including public/private subnets, internet gateway, route tables, security groups, and optional NAT gateways. A critical lambda closure bug affecting subnet CIDR and availability zone assignment was successfully identified and fixed using default parameters to capture loop variable values.
Key Changes
- VPC Component: Full-featured VPC component with flexible subnet count (2+), optional NAT gateways, and support for both snake_case and camelCase parameter naming conventions
- Unit Tests: Comprehensive test suite with 7 passing tests covering basic creation, NAT gateway configuration, custom naming, parameter naming conventions, tagging, and flexible subnet counts
- Lambda Closure Fix: Critical bug fix using
lambda cidrs, idx=i: ...pattern to properly capture loop index values in asynchronous apply() callbacks
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
pulumi/components/aws/vpc/vpc.py |
Main VPC component implementing infrastructure as code with TypedDict args, dual naming support, and comprehensive resource creation |
pulumi/components/aws/vpc/tests/test_vpc.py |
Unit tests with MyMocks implementation covering all major component features and edge cases |
pulumi/components/aws/vpc/tests/conftest.py |
Pytest configuration for Python path setup |
pulumi/components/aws/vpc/tests/__init__.py |
Test package marker file |
pulumi/components/aws/vpc/tests/README.md |
Comprehensive test documentation with setup instructions, test patterns, and troubleshooting guide |
pulumi/components/aws/vpc/requirements.txt |
Production dependencies for the component |
pulumi/components/aws/vpc/requirements-test.txt |
Test dependencies including pytest and pytest-asyncio |
pulumi/components/aws/vpc/__main__.py |
Component provider host entry point |
pulumi/components/aws/vpc/README.md |
Detailed component documentation with usage examples, parameters, outputs, and architecture diagrams |
pulumi/components/aws/vpc/PulumiPlugin.yaml |
Plugin configuration specifying Python runtime |
pulumi/environments/aws/staging/40-vpc/Pulumi.yaml |
Staging environment instantiation using local component reference for PR testing |
pulumi/environments/aws/staging/40-vpc/Pulumi.staging.yaml |
Stack-specific configuration with encryption salt |
pulumi/environments/aws/staging/40-vpc/sdks/vpc/vpc-0.0.0.yaml |
SDK package declaration for local component reference |
.github/workflows/pulumi_staging.yaml |
Workflow refactoring to use PULUMI_CLOUD_URL_STAGING as environment variable instead of vars reference |
Comments suppressed due to low confidence (2)
pulumi/components/aws/vpc/vpc.py:99
- Variable determine_subnet_count is not used.
def determine_subnet_count(cidrs):
pulumi/components/aws/vpc/vpc.py:111
- Except block directly handles BaseException.
except:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Get required parameters | ||
| vpc_cidr = pick(args, "vpc_cidr", "vpcCidr") | ||
| public_subnet_cidrs = pick(args, "public_subnet_cidrs", "publicSubnetCidrs") | ||
| private_subnet_cidrs = pick(args, "private_subnet_cidrs", "privateSubnetCidrs") | ||
| availability_zones = pick(args, "availability_zones", "availabilityZones") | ||
| tags_additional = pick(args, "tags_additional", "tagsAdditional") or {} | ||
|
|
||
| # Get optional parameters with defaults | ||
| enable_nat_gateway = pick(args, "enable_nat_gateway", "enableNatGateway", False) | ||
| vpc_name = pick(args, "vpc_name", "vpcName", name) | ||
|
|
There was a problem hiding this comment.
Missing validation: The component doesn't validate that the number of availability zones matches the number of subnet CIDRs. According to the docstring (line 26): "availabilityZones: List of availability zones to use (must match subnet count)."
If a user provides 3 subnets but only 2 availability zones (or vice versa), the component will silently fall back to reusing zones (line 146: return zones[0] when idx >= len(zones)). This could lead to unexpected behavior where subnets are created in unintended AZs.
Add validation early in the __init__ method:
# Validate inputs
if isinstance(public_subnet_cidrs, list) and isinstance(availability_zones, list):
if len(public_subnet_cidrs) != len(availability_zones):
raise ValueError(
f"Number of public subnet CIDRs ({len(public_subnet_cidrs)}) "
f"must match number of availability zones ({len(availability_zones)})"
)
if len(private_subnet_cidrs) != len(availability_zones):
raise ValueError(
f"Number of private subnet CIDRs ({len(private_subnet_cidrs)}) "
f"must match number of availability zones ({len(availability_zones)})"
)| ingress=[ | ||
| ec2.SecurityGroupIngressArgs( | ||
| protocol="-1", | ||
| from_port=0, | ||
| to_port=0, | ||
| cidr_blocks=["0.0.0.0/0"], | ||
| description="Allow all inbound traffic", | ||
| ) |
There was a problem hiding this comment.
The default security group allows all inbound traffic from 0.0.0.0/0 (the entire internet). While this matches the requirements, it poses a significant security risk in production environments. Consider:
- Making the ingress rules configurable with a secure default (e.g., allowing traffic only from within the VPC CIDR block)
- Adding a parameter like
allowAllInboundthat defaults tofalsewith a clear warning in the documentation - At minimum, add a prominent warning in the README that this configuration is intended for development/testing only and should be customized for production
This would align with security best practices while maintaining flexibility for development environments.
| # Try to get the count directly if inputs are static (from YAML) | ||
| try: | ||
| if isinstance(public_subnet_cidrs, list): | ||
| num_subnets = len(public_subnet_cidrs) | ||
| else: | ||
| num_subnets = 3 # default | ||
| except: | ||
| num_subnets = 3 # default fallback |
There was a problem hiding this comment.
Bare except: clause is a bad practice as it catches all exceptions including system exits and keyboard interrupts. This should be:
except Exception:
num_subnets = 3 # default fallbackHowever, looking at the code more carefully, the entire try-except block appears unnecessary since isinstance() doesn't raise exceptions. The logic can be simplified to:
num_subnets = len(public_subnet_cidrs) if isinstance(public_subnet_cidrs, list) else 3This is clearer and avoids the anti-pattern entirely.
| # Try to get the count directly if inputs are static (from YAML) | |
| try: | |
| if isinstance(public_subnet_cidrs, list): | |
| num_subnets = len(public_subnet_cidrs) | |
| else: | |
| num_subnets = 3 # default | |
| except: | |
| num_subnets = 3 # default fallback | |
| # Determine subnet count directly if inputs are static (from YAML) | |
| num_subnets = len(public_subnet_cidrs) if isinstance(public_subnet_cidrs, list) else 3 |
| # Try to get the count directly if inputs are static (from YAML) | ||
| try: | ||
| if isinstance(public_subnet_cidrs, list): | ||
| num_subnets = len(public_subnet_cidrs) | ||
| else: | ||
| num_subnets = 3 # default | ||
| except: | ||
| num_subnets = 3 # default fallback |
There was a problem hiding this comment.
The function determine_subnet_count is defined but never used. It should either be removed or used instead of the try-except block below it. The function appears to have been the intended implementation before the try-except approach was added.
| # Try to get the count directly if inputs are static (from YAML) | |
| try: | |
| if isinstance(public_subnet_cidrs, list): | |
| num_subnets = len(public_subnet_cidrs) | |
| else: | |
| num_subnets = 3 # default | |
| except: | |
| num_subnets = 3 # default fallback | |
| # Use helper function to determine subnet count | |
| num_subnets = determine_subnet_count(public_subnet_cidrs) |
| class VpcArgs(TypedDict, total=False): | ||
| """Arguments for creating a VPC with public and private subnets. | ||
|
|
||
| Required fields: | ||
| vpcCidr: CIDR block for the VPC (e.g., "10.0.0.0/16"). | ||
| publicSubnetCidrs: List of CIDR blocks for public subnets (e.g., 2-3 subnets). | ||
| privateSubnetCidrs: List of CIDR blocks for private subnets (e.g., 2-3 subnets). | ||
| availabilityZones: List of availability zones to use (must match subnet count). | ||
| tagsAdditional: Additional tags to apply to all resources. | ||
|
|
||
| Optional fields: | ||
| enableNatGateway: Whether to create NAT Gateways in private subnets. | ||
| Default: False. | ||
| vpcName: Name for the VPC. If not provided, uses resource name. | ||
| """ |
There was a problem hiding this comment.
Documentation inconsistency: The docstring states that tagsAdditional is a required field (line 27), but:
- The TypedDict uses
total=Falsewhich makes all fields optional - The code handles it as optional with a default empty dict (line 90:
or {})
This should be clarified. Either:
- Update the docstring to list
tagsAdditionalunder "Optional fields" with "Default: {}" - OR use proper TypedDict typing for truly required vs optional fields:
from typing import Required
class VpcArgs(TypedDict, total=False):
# Required fields
vpcCidr: Required[pulumi.Input[str]]
publicSubnetCidrs: Required[pulumi.Input[list[pulumi.Input[str]]]]
privateSubnetCidrs: Required[pulumi.Input[list[pulumi.Input[str]]]]
availabilityZones: Required[pulumi.Input[list[pulumi.Input[str]]]]
# Optional fields
tagsAdditional: pulumi.Input[dict[str, pulumi.Input[str]]]
enableNatGateway: pulumi.Input[bool]
vpcName: pulumi.Input[str]Note: The Required type hint requires Python 3.11+. For older versions, use two separate TypedDict classes or document clearly.
| @@ -0,0 +1,6 @@ | |||
| import pytest | |||
There was a problem hiding this comment.
Import of 'pytest' is not used.
| import pytest |
|
|
||
| ## Notes | ||
|
|
||
| - The default security group allows all inbound and outbound traffic. This is suitable for development but should be restricted for production use. |
There was a problem hiding this comment.
The component’s "default security group" is documented as allowing all inbound and outbound traffic, which in the implementation corresponds to an ec2.SecurityGroup with all protocols/ports open to 0.0.0.0/0. Any resource attached to this group will be fully exposed to the public internet, so an attacker can scan and reach any listening service. Consider making this security group least-privilege by default (no or minimal inbound rules) and/or exposing configuration to define restrictive ingress/egress rules, with an open SG only as an explicit opt-in for development.
VPC Component Implementation - COMPLETED ✅
Recent Changes
lambda cidrs: get_cidr(cidrs, i)tolambda cidrs, idx=i: get_cidr(cidrs, idx)idx=icaptures the value ofiat loop iteration timeiafter loop completionTechnical Details
The issue was a classic Python closure problem. When using
lambda cidrs: get_cidr(cidrs, i)inside a loop, the variableiis captured by reference, not by value. By the time Pulumi's Output.apply() executes the lambda (asynchronously), the loop has already completed andihas its final value.Solution: Use default parameter
idx=iin the lambda to capture the value at iteration time:This ensures each lambda gets its own copy of the index value.
Test Results
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.