Skip to content
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

Fix IPV6 cidr blocks #197

Merged
merged 4 commits into from
Nov 7, 2024
Merged

Fix IPV6 cidr blocks #197

merged 4 commits into from
Nov 7, 2024

Conversation

corymhall
Copy link
Contributor

Depends on pulumi/pulumi-aws-native#1797

Because of pulumi/pulumi-aws-native#1798 we need to add a special case for Ipv6 cidr blocks that are added to the VPC via a VPCCidrBlock resource.

This PR adds special handling where we track both the Vpc and the VpcCidrBlock resource and swap any references.

Depends on pulumi/pulumi-aws-native#1797

Because of pulumi/pulumi-aws-native#1798 we
need to add a special case for Ipv6 cidr blocks that are added to the
VPC via a `VPCCidrBlock` resource.

This PR adds special handling where we track both the `Vpc` and the
`VpcCidrBlock` resource and swap any references.
@corymhall corymhall marked this pull request as ready for review November 6, 2024 13:46
@corymhall corymhall self-assigned this Nov 6, 2024
src/graph.ts Outdated
@@ -115,15 +115,19 @@ export class GraphBuilder {
// Map of resource logicalId to GraphNode. Allows for easy lookup by logicalId
cfnElementNodes: Map<string, GraphNode>;

// If the app has a VpcCidrBlock resource, this will be set to the GraphNode representing it
vpcCidrBlockNode?: GraphNode;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'd prefer if we'd save that in the result of GraphBuilder::build. E.g. turn this into a GraphResult struct that has the necessary params we need.
I wouldn't expect a Builder to store state beyond what's needed to build

src/graph.ts Outdated
this.vpcCidrBlockNode = node;
}
if (resource.Type === 'AWS::EC2::VPC') {
this.vpcNode = node;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can a stack only have a single VPC?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that case would be exceedingly rare, but I should be able to update this to handle that case

src/graph.ts Outdated
// Here we switching the dependency to be on the `VpcCidrBlock` resource (since that will also have a dependency
// on the VPC resource)
if (
logicalId === this.vpcNode?.logicalId &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Along the lines of my earlier Q, what happens if the stack has multiple VPCs?

Copy link
Contributor

@flostadler flostadler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@corymhall corymhall merged commit add3044 into main Nov 7, 2024
11 checks passed
@corymhall corymhall deleted the corymhall/vpc-ipv6 branch November 7, 2024 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants