From 931cc48921b76cd24684f6249e2e9aefa004f2f0 Mon Sep 17 00:00:00 2001 From: Peter Rifel Date: Fri, 2 Oct 2020 10:12:23 -0500 Subject: [PATCH 1/2] Don't disassociate additional CIDRs for shared VPCs --- upup/pkg/fi/cloudup/awstasks/vpc.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/upup/pkg/fi/cloudup/awstasks/vpc.go b/upup/pkg/fi/cloudup/awstasks/vpc.go index d92ceadeb0..c0677ba514 100644 --- a/upup/pkg/fi/cloudup/awstasks/vpc.go +++ b/upup/pkg/fi/cloudup/awstasks/vpc.go @@ -202,7 +202,7 @@ func (_ *VPC) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *VPC) error { } func (e *VPC) FindDeletions(c *fi.Context) ([]fi.Deletion, error) { - if fi.StringValue(e.ID) == "" { + if fi.IsNilOrEmpty(e.ID) || fi.BoolValue(e.Shared) { return nil, nil } From b81f9b290fd10d84333631d94e13e87e63588f38 Mon Sep 17 00:00:00 2001 From: Peter Rifel Date: Fri, 2 Oct 2020 14:05:29 -0500 Subject: [PATCH 2/2] Add a test ensuring shared VPCs dont have unrelated CIDR blocks disassociated --- cloudmock/aws/mockec2/vpcs.go | 45 ++++++++++++ upup/pkg/fi/cloudup/awstasks/vpc_test.go | 91 ++++++++++++++++++++++++ 2 files changed, 136 insertions(+) diff --git a/cloudmock/aws/mockec2/vpcs.go b/cloudmock/aws/mockec2/vpcs.go index 1b7563a94e..b0f37c03ff 100644 --- a/cloudmock/aws/mockec2/vpcs.go +++ b/cloudmock/aws/mockec2/vpcs.go @@ -231,3 +231,48 @@ func (m *MockEC2) DeleteVpcWithContext(aws.Context, *ec2.DeleteVpcInput, ...requ func (m *MockEC2) DeleteVpcRequest(*ec2.DeleteVpcInput) (*request.Request, *ec2.DeleteVpcOutput) { panic("Not implemented") } + +func (m *MockEC2) AssociateVpcCidrBlock(request *ec2.AssociateVpcCidrBlockInput) (*ec2.AssociateVpcCidrBlockOutput, error) { + id := aws.StringValue(request.VpcId) + vpc, ok := m.Vpcs[id] + if !ok { + return nil, fmt.Errorf("VPC %q not found", id) + } + association := &ec2.VpcCidrBlockAssociation{ + CidrBlock: request.CidrBlock, + AssociationId: aws.String(fmt.Sprintf("%v-%v", id, len(vpc.main.CidrBlockAssociationSet))), + CidrBlockState: &ec2.VpcCidrBlockState{ + State: aws.String(ec2.VpcCidrBlockStateCodeAssociated), + }, + } + vpc.main.CidrBlockAssociationSet = append(vpc.main.CidrBlockAssociationSet, association) + + return &ec2.AssociateVpcCidrBlockOutput{ + CidrBlockAssociation: association, + VpcId: request.VpcId, + }, nil +} + +func (m *MockEC2) DisassociateVpcCidrBlock(request *ec2.DisassociateVpcCidrBlockInput) (*ec2.DisassociateVpcCidrBlockOutput, error) { + id := aws.StringValue(request.AssociationId) + var association *ec2.VpcCidrBlockAssociation + var vpcID *string + for _, vpc := range m.Vpcs { + for _, a := range vpc.main.CidrBlockAssociationSet { + if aws.StringValue(a.AssociationId) == id { + a.CidrBlockState.State = aws.String(ec2.VpcCidrBlockStateCodeDisassociated) + association = a + vpcID = vpc.main.VpcId + break + } + } + } + if association == nil { + return nil, fmt.Errorf("VPC association %q not found", id) + } + + return &ec2.DisassociateVpcCidrBlockOutput{ + CidrBlockAssociation: association, + VpcId: vpcID, + }, nil +} diff --git a/upup/pkg/fi/cloudup/awstasks/vpc_test.go b/upup/pkg/fi/cloudup/awstasks/vpc_test.go index a974fef267..31cddf06b4 100644 --- a/upup/pkg/fi/cloudup/awstasks/vpc_test.go +++ b/upup/pkg/fi/cloudup/awstasks/vpc_test.go @@ -129,3 +129,94 @@ func Test4758(t *testing.T) { t.Errorf("unexpected changes: +%v", changes) } } + +func TestSharedVPCAdditionalCIDR(t *testing.T) { + cloud := awsup.BuildMockAWSCloud("us-east-1", "abc") + c := &mockec2.MockEC2{} + c.CreateVpcWithId(&ec2.CreateVpcInput{ + CidrBlock: s("172.21.0.0/16"), + TagSpecifications: []*ec2.TagSpecification{ + { + ResourceType: s(ec2.ResourceTypeVpc), + Tags: []*ec2.Tag{ + { + Key: s("Name"), + Value: s("vpc-1"), + }, + }, + }, + }, + }, "vpc-1") + c.AssociateVpcCidrBlock(&ec2.AssociateVpcCidrBlockInput{ + VpcId: s("vpc-1"), + CidrBlock: s("172.22.0.0/16"), + }) + + cloud.MockEC2 = c + + // We define a function so we can rebuild the tasks, because we modify in-place when running + buildTasks := func() map[string]fi.Task { + vpc1 := &VPC{ + Name: s("vpc-1"), + CIDR: s("172.21.0.0/16"), + Tags: map[string]string{"Name": "vpc-1"}, + Shared: fi.Bool(true), + } + return map[string]fi.Task{ + "vpc-1": vpc1, + } + } + + { + allTasks := buildTasks() + vpc1 := allTasks["vpc-1"].(*VPC) + + target := &awsup.AWSAPITarget{ + Cloud: cloud, + } + + context, err := fi.NewContext(target, nil, cloud, nil, nil, nil, true, allTasks) + if err != nil { + t.Fatalf("error building context: %v", err) + } + defer context.Close() + + if err := context.RunTasks(testRunTasksOptions); err != nil { + t.Fatalf("unexpected error during Run: %v", err) + } + + if fi.StringValue(vpc1.ID) == "" { + t.Fatalf("ID not set") + } + + if len(c.Vpcs) != 1 { + t.Fatalf("Expected exactly one Vpc; found %v", c.Vpcs) + } + + expected := &ec2.Vpc{ + CidrBlock: s("172.21.0.0/16"), + IsDefault: fi.Bool(false), + VpcId: vpc1.ID, + Tags: buildTags(map[string]string{ + "Name": "vpc-1", + }), + CidrBlockAssociationSet: []*ec2.VpcCidrBlockAssociation{ + { + AssociationId: s("vpc-1-0"), + CidrBlock: s("172.22.0.0/16"), + CidrBlockState: &ec2.VpcCidrBlockState{ + State: s(ec2.VpcCidrBlockStateCodeAssociated), + }, + }, + }, + } + actual := c.FindVpc(*vpc1.ID) + if actual == nil { + t.Fatalf("VPC no longer exists") + } + if !reflect.DeepEqual(actual, expected) { + t.Fatalf("Unexpected VPC: expected=%v actual=%v", expected, actual) + } + } + +}