From 7b4173ffeaffa6dcff7f7988be5c6630358e166c Mon Sep 17 00:00:00 2001 From: Rich Date: Tue, 17 Mar 2026 20:09:37 +0000 Subject: [PATCH] Allow existing VPCs to have an Egress Only Internet Gateway even when other Egress Only Internet Gateways exist in the AWS account Signed-off-by: Rich --- .../aws/mockec2/egressonlyinternetgateways.go | 11 --- .../awstasks/egressonlyinternetgateway.go | 52 +++++++--- .../egressonlyinternetgateway_test.go | 98 ++++++++++++------- 3 files changed, 102 insertions(+), 59 deletions(-) diff --git a/cloudmock/aws/mockec2/egressonlyinternetgateways.go b/cloudmock/aws/mockec2/egressonlyinternetgateways.go index d2a13f240511c..c330e0d64537c 100644 --- a/cloudmock/aws/mockec2/egressonlyinternetgateways.go +++ b/cloudmock/aws/mockec2/egressonlyinternetgateways.go @@ -108,17 +108,6 @@ func (m *MockEC2) DescribeEgressOnlyInternetGateways(ctx context.Context, reques } } - case "attachment.vpc-id": - for _, v := range filter.Values { - if internetGateway.Attachments != nil { - for _, attachment := range internetGateway.Attachments { - if *attachment.VpcId == v { - match = true - } - } - } - } - default: if strings.HasPrefix(*filter.Name, "tag:") { match = m.hasTag(ec2types.ResourceTypeEgressOnlyInternetGateway, id, filter) diff --git a/upup/pkg/fi/cloudup/awstasks/egressonlyinternetgateway.go b/upup/pkg/fi/cloudup/awstasks/egressonlyinternetgateway.go index 65c2d21e87d0f..69b08aaa7c073 100644 --- a/upup/pkg/fi/cloudup/awstasks/egressonlyinternetgateway.go +++ b/upup/pkg/fi/cloudup/awstasks/egressonlyinternetgateway.go @@ -49,19 +49,47 @@ func (e *EgressOnlyInternetGateway) CompareWithID() *string { return e.ID } -func findEgressOnlyInternetGateway(ctx context.Context, cloud awsup.AWSCloud, request *ec2.DescribeEgressOnlyInternetGatewaysInput) (*ec2types.EgressOnlyInternetGateway, error) { +func findEgressOnlyInternetGateway(ctx context.Context, cloud awsup.AWSCloud, request *ec2.DescribeEgressOnlyInternetGatewaysInput, vpcId string) (*ec2types.EgressOnlyInternetGateway, error) { response, err := cloud.EC2().DescribeEgressOnlyInternetGateways(ctx, request) + if err != nil { return nil, fmt.Errorf("error listing EgressOnlyInternetGateways: %v", err) } - if response == nil || len(response.EgressOnlyInternetGateways) == 0 { + + var filteredEgressOnlyInternetGateways []ec2types.EgressOnlyInternetGateway + + if response != nil { + if vpcId == "" { + filteredEgressOnlyInternetGateways = response.EgressOnlyInternetGateways + } else { + for _, eigw := range response.EgressOnlyInternetGateways { + var vpcAttached bool = false + + if eigw.Attachments != nil { + for _, attachment := range eigw.Attachments { + if *attachment.VpcId == vpcId { + vpcAttached = true + break + } + } + } + + if vpcAttached { + filteredEgressOnlyInternetGateways = append(filteredEgressOnlyInternetGateways, eigw) + } + } + } + } + + if len(filteredEgressOnlyInternetGateways) == 0 { return nil, nil } - if len(response.EgressOnlyInternetGateways) != 1 { + if len(filteredEgressOnlyInternetGateways) != 1 { return nil, fmt.Errorf("found multiple EgressOnlyInternetGateways matching tags") } - igw := response.EgressOnlyInternetGateways[0] + + igw := filteredEgressOnlyInternetGateways[0] return &igw, nil } @@ -72,13 +100,12 @@ func (e *EgressOnlyInternetGateway) Find(c *fi.CloudupContext) (*EgressOnlyInter request := &ec2.DescribeEgressOnlyInternetGatewaysInput{} shared := fi.ValueOf(e.Shared) - if shared { - if fi.ValueOf(e.VPC.ID) == "" { - return nil, fmt.Errorf("VPC ID is required when EgressOnlyInternetGateway is shared") - } - request.Filters = []ec2types.Filter{awsup.NewEC2Filter("attachment.vpc-id", *e.VPC.ID)} - } else { + if shared && fi.ValueOf(e.VPC.ID) == "" { + return nil, fmt.Errorf("VPC ID is required when EgressOnlyInternetGateway is shared") + } + + if !shared { if e.ID != nil { request.EgressOnlyInternetGatewayIds = []string{fi.ValueOf(e.ID)} } else { @@ -86,7 +113,7 @@ func (e *EgressOnlyInternetGateway) Find(c *fi.CloudupContext) (*EgressOnlyInter } } - eigw, err := findEgressOnlyInternetGateway(ctx, cloud, request) + eigw, err := findEgressOnlyInternetGateway(ctx, cloud, request, fi.ValueOf(e.VPC.ID)) if err != nil { return nil, err } @@ -187,8 +214,7 @@ func (_ *EgressOnlyInternetGateway) RenderTerraform(t *terraform.TerraformTarget if vpcID == "" { return fmt.Errorf("VPC ID is required when EgressOnlyInternetGateway is shared") } - request.Filters = []ec2types.Filter{awsup.NewEC2Filter("attachment.vpc-id", vpcID)} - igw, err := findEgressOnlyInternetGateway(ctx, t.Cloud.(awsup.AWSCloud), request) + igw, err := findEgressOnlyInternetGateway(ctx, t.Cloud.(awsup.AWSCloud), request, vpcID) if err != nil { return err } diff --git a/upup/pkg/fi/cloudup/awstasks/egressonlyinternetgateway_test.go b/upup/pkg/fi/cloudup/awstasks/egressonlyinternetgateway_test.go index bc294aaeb185f..374893eb311c2 100644 --- a/upup/pkg/fi/cloudup/awstasks/egressonlyinternetgateway_test.go +++ b/upup/pkg/fi/cloudup/awstasks/egressonlyinternetgateway_test.go @@ -19,6 +19,7 @@ package awstasks import ( "context" "reflect" + "strconv" "testing" "github.com/aws/aws-sdk-go-v2/aws" @@ -29,59 +30,79 @@ import ( "k8s.io/kops/upup/pkg/fi/cloudup/awsup" ) -func TestSharedEgressOnlyInternetGatewayDoesNotRename(t *testing.T) { +func TestSharedEgressOnlyInternetGateway(t *testing.T) { ctx := context.TODO() cloud := awsup.BuildMockAWSCloud("us-east-1", "abc") c := &mockec2.MockEC2{} cloud.MockEC2 = c - // Pre-create the vpc / subnet - vpc, err := c.CreateVpc(ctx, &ec2.CreateVpcInput{ - CidrBlock: aws.String("172.20.0.0/16"), - }) - if err != nil { - t.Fatalf("error creating test VPC: %v", err) - } - _, err = c.CreateTags(ctx, &ec2.CreateTagsInput{ - Resources: []string{aws.ToString(vpc.Vpc.VpcId)}, - Tags: []ec2types.Tag{ - { - Key: aws.String("Name"), - Value: aws.String("ExistingVPC"), + // Pre-create multiple VPC & Egress Only Internet Gateways. + // This is a broader test scenario that will also cover filtering Egress Only + // Internet Gateways so that only the appropiate one for the VPC is selected. + var vpcs []*ec2types.Vpc + var eigws []*ec2types.EgressOnlyInternetGateway + + for index, cidr := range []string{"172.20.0.0/24", "172.20.1.0/24"} { + vpc, err := c.CreateVpc(ctx, &ec2.CreateVpcInput{ + CidrBlock: aws.String(cidr), + }) + + if err != nil { + t.Fatalf("error creating test VPC: %v", err) + } + + _, err = c.CreateTags(ctx, &ec2.CreateTagsInput{ + Resources: []string{aws.ToString(vpc.Vpc.VpcId)}, + Tags: []ec2types.Tag{ + { + Key: aws.String("Name"), + Value: aws.String("ExistingVPC"), + }, + { + Key: aws.String("Index"), + Value: aws.String(strconv.Itoa(index)), + }, }, - }, - }) - if err != nil { - t.Fatalf("error tagging test vpc: %v", err) - } + }) - internetGateway, err := c.CreateEgressOnlyInternetGateway(ctx, &ec2.CreateEgressOnlyInternetGatewayInput{ - VpcId: vpc.Vpc.VpcId, - TagSpecifications: awsup.EC2TagSpecification(ec2types.ResourceTypeEgressOnlyInternetGateway, map[string]string{ - "Name": "ExistingInternetGateway", - }), - }) - if err != nil { - t.Fatalf("error creating test eigw: %v", err) + if err != nil { + t.Fatalf("error tagging test vpc: %v", err) + } + + eigw, err := c.CreateEgressOnlyInternetGateway(ctx, &ec2.CreateEgressOnlyInternetGatewayInput{ + VpcId: vpc.Vpc.VpcId, + TagSpecifications: awsup.EC2TagSpecification(ec2types.ResourceTypeEgressOnlyInternetGateway, map[string]string{ + "Name": "ExistingInternetGateway", + "Index": strconv.Itoa(index), + }), + }) + + if err != nil { + t.Fatalf("error creating test eigw: %v", err) + } + + vpcs = append(vpcs, vpc.Vpc) + eigws = append(eigws, eigw.EgressOnlyInternetGateway) } // We define a function so we can rebuild the tasks, because we modify in-place when running + // Only use the first cloud VPC and Egress Only Internet Gateway for KOPS tasks. buildTasks := func() map[string]fi.CloudupTask { vpc1 := &VPC{ Name: s("vpc1"), Lifecycle: fi.LifecycleSync, - CIDR: s("172.20.0.0/16"), + CIDR: vpcs[0].CidrBlock, Tags: map[string]string{"kubernetes.io/cluster/cluster.example.com": "shared"}, Shared: fi.PtrTo(true), - ID: vpc.Vpc.VpcId, + ID: vpcs[0].VpcId, } eigw1 := &EgressOnlyInternetGateway{ Name: s("eigw1"), Lifecycle: fi.LifecycleSync, VPC: vpc1, Shared: fi.PtrTo(true), - ID: internetGateway.EgressOnlyInternetGateway.EgressOnlyInternetGatewayId, + ID: eigws[0].EgressOnlyInternetGatewayId, Tags: make(map[string]string), } @@ -97,26 +118,32 @@ func TestSharedEgressOnlyInternetGatewayDoesNotRename(t *testing.T) { runTasks(t, cloud, allTasks) + // Check the created Egress Only Internet Gateway has a valid ID if fi.ValueOf(eigw1.ID) == "" { t.Fatalf("ID not set after create") } - if len(c.EgressOnlyInternetGatewayIds()) != 1 { - t.Fatalf("Expected exactly one EgressOnlyInternetGateway; found %v", c.EgressOnlyInternetGatewayIds()) + // Check that there are still 2 Egress Only Internet Gateways and that none + // extra have been created (or destroyed). + if len(c.EgressOnlyInternetGatewayIds()) != 2 { + t.Fatalf("Expected exactly two EgressOnlyInternetGateway; found %v", c.EgressOnlyInternetGatewayIds()) } - actual := c.FindEgressOnlyInternetGateway(*internetGateway.EgressOnlyInternetGateway.EgressOnlyInternetGatewayId) + // Check the Egress Only Internet Gateway in our build context is the one + // that we expect to be there. + actual := c.FindEgressOnlyInternetGateway(*eigws[0].EgressOnlyInternetGatewayId) if actual == nil { t.Fatalf("EgressOnlyInternetGateway created but then not found") } expected := &ec2types.EgressOnlyInternetGateway{ EgressOnlyInternetGatewayId: aws.String("eigw-1"), Tags: buildTags(map[string]string{ - "Name": "ExistingInternetGateway", + "Name": "ExistingInternetGateway", + "Index": "0", }), Attachments: []ec2types.InternetGatewayAttachment{ { - VpcId: vpc.Vpc.VpcId, + VpcId: vpcs[0].VpcId, }, }, } @@ -130,6 +157,7 @@ func TestSharedEgressOnlyInternetGatewayDoesNotRename(t *testing.T) { } { + // Check the Egress Only Internet Gateway does not change on further runs. allTasks := buildTasks() checkNoChanges(t, ctx, cloud, allTasks) }