Skip to content

Fix Egress Only Internet Gateway lookup when there are multiple in the account #18104

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.

Sign up for GitHub

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

Open
rlees85 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix Egress Only Internet Gateway lookup when there are multiple in the account #18104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions cloudmock/aws/mockec2/egressonlyinternetgateways.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
52 changes: 39 additions & 13 deletions upup/pkg/fi/cloudup/awstasks/egressonlyinternetgateway.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Copy Markdown
Member

@hakman hakman Apr 2, 2026

Choose a reason for hiding this comment

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

The gateway should be tagged with "kubernetes.io/cluster/<cluster-name>" = "shared", I believe that is the expectation for any kOps resource. Filters should be able to work with it with tag-key:

tag-key - The key of a tag assigned to the resource. Use this filter to find all resources assigned a tag with a specific key, regardless of the tag value.

Copy link
Copy Markdown
Contributor Author

@rlees85 rlees85 Apr 2, 2026

Choose a reason for hiding this comment

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

I will experiment with that way of doing it. I know you can get away without tagging a lot of resources (internet gateway, possibly others) but if the intention is that all resources (not just VPC, subnet) are tagged I will see if I can make it work by looking up with the tag name

Copy link
Copy Markdown
Member

@hakman hakman Apr 2, 2026

Choose a reason for hiding this comment

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

The intention was to have everything tagged, but AWS took a while to add tags and filters to everything...

Copy link
Copy Markdown
Contributor Author

@rlees85 rlees85 Apr 3, 2026

Choose a reason for hiding this comment

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

Ah fair enough. Looking through the docs and code it looks like KOPS will add the tags for you "kubernetes.io/cluster/<cluster-name>" = "shared" if they do not already exist. It just so happens I use Terraform to pre-tag to stop endless diffs.

So having to "pre-tag" the egress only Internet gateway would be a significant change in behaviour to how VPC, subnet, everything else is currently handled.

If we don't "pre-tag" I am still not sure how Kops is going to find the correct egress only Internet gateway without either a manual VPC filter (as per this PR) or having to specify the egress only Internet gateway ID in the KOPS manifest (and command line parameter for those that do not use KOPS with a manifest).


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
}

Expand All @@ -72,21 +100,20 @@ 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 {
request.Filters = cloud.BuildFilters(e.Name)
}
}

eigw, err := findEgressOnlyInternetGateway(ctx, cloud, request)
eigw, err := findEgressOnlyInternetGateway(ctx, cloud, request, fi.ValueOf(e.VPC.ID))
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -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
}
Expand Down
98 changes: 63 additions & 35 deletions upup/pkg/fi/cloudup/awstasks/egressonlyinternetgateway_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package awstasks
import (
"context"
"reflect"
"strconv"
"testing"

"github.com/aws/aws-sdk-go-v2/aws"
Expand All @@ -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),
}

Expand All @@ -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,
},
},
}
Expand All @@ -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)
}
Expand Down
Loading