Skip to content

Commit ffe66d8

Browse files
fiunchinhofiunchinho
committed
✨ Create only one nat gateway per AZ
1 parent 483f3a9 commit ffe66d8

File tree

3 files changed

+174
-96
lines changed

3 files changed

+174
-96
lines changed

‎controlplane/eks/controllers/awsmanagedcontrolplane_controller_test.go

Lines changed: 57 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -551,71 +551,72 @@ func mockedCallsForMissingEverything(ec2Rec *mocks.MockEC2APIMockRecorder, subne
551551
eipAllocationID := strconv.Itoa(1234 + subnetIndex)
552552
natGatewayID := fmt.Sprintf("nat-%d", subnetIndex+1)
553553

554-
ec2Rec.AllocateAddress(context.TODO(), gomock.Eq(&ec2.AllocateAddressInput{
555-
Domain: ec2types.DomainTypeVpc,
556-
TagSpecifications: []ec2types.TagSpecification{
557-
{
558-
ResourceType: ec2types.ResourceTypeElasticIp,
559-
Tags: []ec2types.Tag{
560-
{
561-
Key: aws.String("Name"),
562-
Value: aws.String("test-cluster-eip-common"),
563-
},
564-
{
565-
Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"),
566-
Value: aws.String("owned"),
567-
},
568-
{
569-
Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"),
570-
Value: aws.String("common"),
554+
// We only expect to create NAT gateways for the public subnet AZ used by the private subnet
555+
if subnet.ID == "my-managed-subnet-pub1" {
556+
ec2Rec.AllocateAddress(context.TODO(), gomock.Eq(&ec2.AllocateAddressInput{
557+
Domain: ec2types.DomainTypeVpc,
558+
TagSpecifications: []ec2types.TagSpecification{
559+
{
560+
ResourceType: ec2types.ResourceTypeElasticIp,
561+
Tags: []ec2types.Tag{
562+
{
563+
Key: aws.String("Name"),
564+
Value: aws.String("test-cluster-eip-common"),
565+
},
566+
{
567+
Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"),
568+
Value: aws.String("owned"),
569+
},
570+
{
571+
Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"),
572+
Value: aws.String("common"),
573+
},
571574
},
572575
},
573576
},
574-
},
575-
})).Return(&ec2.AllocateAddressOutput{
576-
AllocationId: aws.String(eipAllocationID),
577-
}, nil)
578-
579-
ec2Rec.CreateNatGateway(context.TODO(), gomock.Eq(&ec2.CreateNatGatewayInput{
580-
AllocationId: aws.String(eipAllocationID),
581-
SubnetId: aws.String(subnetID),
582-
TagSpecifications: []ec2types.TagSpecification{
583-
{
584-
ResourceType: ec2types.ResourceTypeNatgateway,
585-
Tags: []ec2types.Tag{
586-
{
587-
Key: aws.String("Name"),
588-
Value: aws.String("test-cluster-nat"),
589-
},
590-
{
591-
Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"),
592-
Value: aws.String("owned"),
593-
},
594-
{
595-
Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"),
596-
Value: aws.String("common"),
577+
})).Return(&ec2.AllocateAddressOutput{
578+
AllocationId: aws.String(eipAllocationID),
579+
}, nil)
580+
581+
ec2Rec.CreateNatGateway(context.TODO(), gomock.Eq(&ec2.CreateNatGatewayInput{
582+
AllocationId: aws.String(eipAllocationID),
583+
SubnetId: aws.String(subnetID),
584+
TagSpecifications: []ec2types.TagSpecification{
585+
{
586+
ResourceType: ec2types.ResourceTypeNatgateway,
587+
Tags: []ec2types.Tag{
588+
{
589+
Key: aws.String("Name"),
590+
Value: aws.String("test-cluster-nat"),
591+
},
592+
{
593+
Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"),
594+
Value: aws.String("owned"),
595+
},
596+
{
597+
Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"),
598+
Value: aws.String("common"),
599+
},
597600
},
598601
},
599602
},
600-
},
601-
})).Return(&ec2.CreateNatGatewayOutput{
602-
NatGateway: &ec2types.NatGateway{
603-
NatGatewayId: aws.String(natGatewayID),
604-
SubnetId: aws.String(subnetID),
605-
},
606-
}, nil)
607-
608-
ec2Rec.DescribeNatGateways(gomock.Any(), gomock.Eq(&ec2.DescribeNatGatewaysInput{
609-
NatGatewayIds: []string{natGatewayID},
610-
}), gomock.Any()).Return(&ec2.DescribeNatGatewaysOutput{
611-
NatGateways: []ec2types.NatGateway{
612-
{
603+
})).Return(&ec2.CreateNatGatewayOutput{
604+
NatGateway: &ec2types.NatGateway{
613605
NatGatewayId: aws.String(natGatewayID),
614606
SubnetId: aws.String(subnetID),
615-
State: ec2types.NatGatewayStateAvailable,
616607
},
617-
},
618-
}, nil)
608+
}, nil)
609+
610+
ec2Rec.DescribeNatGateways(gomock.Any(), gomock.Any(), gomock.Any()).Return(&ec2.DescribeNatGatewaysOutput{
611+
NatGateways: []ec2types.NatGateway{
612+
{
613+
NatGatewayId: aws.String(natGatewayID),
614+
SubnetId: aws.String(subnetID),
615+
State: ec2types.NatGatewayStateAvailable,
616+
},
617+
},
618+
}, nil)
619+
}
619620
}
620621

621622
routeTableID := fmt.Sprintf("rtb-%d", subnetIndex+1)

‎pkg/cloud/services/network/natgateways.go

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,11 +115,29 @@ func (s *Service) updateNatGatewayIPs(updateTags bool) ([]string, error) {
115115
natGatewaysIPs := []string{}
116116
subnetIDs := []string{}
117117

118+
// Find AZs that have private subnets
119+
privateSubnetAZs := make(map[string]bool)
120+
for _, sn := range s.scope.Subnets().FilterPrivate().FilterNonCni() {
121+
if sn.GetResourceID() != "" {
122+
privateSubnetAZs[sn.AvailabilityZone] = true
123+
}
124+
}
125+
126+
// For each AZ with private subnets, find a public subnet and check for NAT gateway
127+
processedAZs := make(map[string]bool)
118128
for _, sn := range s.scope.Subnets().FilterPublic().FilterNonCni() {
119129
if sn.GetResourceID() == "" {
120130
continue
121131
}
122132

133+
// Only process this AZ if it has private subnets and we haven't processed it yet
134+
if !privateSubnetAZs[sn.AvailabilityZone] || processedAZs[sn.AvailabilityZone] {
135+
continue
136+
}
137+
138+
// Mark this AZ as processed
139+
processedAZs[sn.AvailabilityZone] = true
140+
123141
if ngw, ok := existing[sn.GetResourceID()]; ok {
124142
if len(ngw.NatGatewayAddresses) > 0 && ngw.NatGatewayAddresses[0].PublicIp != nil {
125143
natGatewaysIPs = append(natGatewaysIPs, *ngw.NatGatewayAddresses[0].PublicIp)

‎pkg/cloud/services/network/natgateways_test.go

Lines changed: 99 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,7 @@ func TestReconcileNatGateways(t *testing.T) {
183183
},
184184
},
185185
{
186-
name: "two public & 1 private subnet, and one NAT gateway exists",
186+
name: "two public & 1 private subnet, and one NAT gateway exists, should not create additional NAT gateway",
187187
input: []infrav1.SubnetSpec{
188188
{
189189
ID: "subnet-1",
@@ -224,10 +224,74 @@ func TestReconcileNatGateways(t *testing.T) {
224224
{
225225
NatGatewayId: aws.String("gateway"),
226226
SubnetId: aws.String("subnet-1"),
227+
Tags: []types.Tag{
228+
{
229+
Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"),
230+
Value: aws.String("common"),
231+
},
232+
{
233+
Key: aws.String("Name"),
234+
Value: aws.String("test-cluster-nat"),
235+
},
236+
{
237+
Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"),
238+
Value: aws.String("owned"),
239+
},
240+
},
227241
},
228242
},
229243
}, nil)
230244

245+
// Should not create any new NAT gateways because subnet-3 (public subnet in us-east-1b) has no private subnets
246+
m.DescribeAddresses(context.TODO(), gomock.Any()).Times(0)
247+
m.AllocateAddress(context.TODO(), gomock.Any()).Times(0)
248+
m.CreateNatGateway(context.TODO(), gomock.Any()).Times(0)
249+
},
250+
},
251+
{
252+
name: "multiple AZs with private subnets, should create one NAT gateway per AZ",
253+
input: []infrav1.SubnetSpec{
254+
{
255+
ID: "subnet-1",
256+
AvailabilityZone: "us-east-1a",
257+
CidrBlock: "10.0.10.0/24",
258+
IsPublic: true,
259+
},
260+
{
261+
ID: "subnet-2",
262+
AvailabilityZone: "us-east-1a",
263+
CidrBlock: "10.0.12.0/24",
264+
IsPublic: false,
265+
},
266+
{
267+
ID: "subnet-3",
268+
AvailabilityZone: "us-east-1b",
269+
CidrBlock: "10.0.13.0/24",
270+
IsPublic: true,
271+
},
272+
{
273+
ID: "subnet-4",
274+
AvailabilityZone: "us-east-1b",
275+
CidrBlock: "10.0.14.0/24",
276+
IsPublic: false,
277+
},
278+
},
279+
expect: func(m *mocks.MockEC2APIMockRecorder) {
280+
m.DescribeNatGateways(context.TODO(),
281+
gomock.Eq(&ec2.DescribeNatGatewaysInput{
282+
Filter: []types.Filter{
283+
{
284+
Name: aws.String("vpc-id"),
285+
Values: []string{subnetsVPCID},
286+
},
287+
{
288+
Name: aws.String("state"),
289+
Values: []string{"pending", "available"},
290+
},
291+
},
292+
}),
293+
gomock.Any()).Return(&ec2.DescribeNatGatewaysOutput{}, nil)
294+
231295
m.DescribeAddresses(context.TODO(), gomock.Any()).
232296
Return(&ec2.DescribeAddressesOutput{}, nil)
233297

@@ -254,51 +318,46 @@ func TestReconcileNatGateways(t *testing.T) {
254318
},
255319
}).Return(&ec2.AllocateAddressOutput{
256320
AllocationId: aws.String(ElasticIPAllocationID),
257-
}, nil)
321+
}, nil).Times(2)
258322

259-
m.CreateNatGateway(context.TODO(), &ec2.CreateNatGatewayInput{
260-
AllocationId: aws.String(ElasticIPAllocationID),
261-
SubnetId: aws.String("subnet-3"),
262-
TagSpecifications: []types.TagSpecification{
263-
{
264-
ResourceType: types.ResourceTypeNatgateway,
265-
Tags: []types.Tag{
266-
{
267-
Key: aws.String("Name"),
268-
Value: aws.String("test-cluster-nat"),
269-
},
270-
{
271-
Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"),
272-
Value: aws.String("owned"),
273-
},
274-
{
275-
Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"),
276-
Value: aws.String("common"),
277-
},
278-
},
323+
// Should create NAT gateways for both AZs since both have private subnets
324+
m.CreateNatGateway(context.TODO(), gomock.Any()).
325+
Return(&ec2.CreateNatGatewayOutput{
326+
NatGateway: &types.NatGateway{
327+
NatGatewayId: aws.String("natgateway-1"),
328+
SubnetId: aws.String("subnet-1"),
279329
},
280-
},
281-
}).Return(&ec2.CreateNatGatewayOutput{
282-
NatGateway: &types.NatGateway{
283-
NatGatewayId: aws.String("natgateway"),
284-
SubnetId: aws.String("subnet-3"),
285-
},
286-
}, nil)
330+
}, nil)
287331

288-
m.DescribeNatGateways(gomock.Any(), &ec2.DescribeNatGatewaysInput{
289-
NatGatewayIds: []string{"natgateway"},
290-
}, gomock.Any()).Return(&ec2.DescribeNatGatewaysOutput{
291-
NatGateways: []types.NatGateway{
292-
{
293-
State: types.NatGatewayStateAvailable,
294-
NatGatewayId: aws.String("natgateway"),
332+
m.CreateNatGateway(context.TODO(), gomock.Any()).
333+
Return(&ec2.CreateNatGatewayOutput{
334+
NatGateway: &types.NatGateway{
335+
NatGatewayId: aws.String("natgateway-2"),
295336
SubnetId: aws.String("subnet-3"),
296337
},
297-
},
298-
}, nil)
338+
}, nil)
339+
340+
m.DescribeNatGateways(gomock.Any(), gomock.Any(), gomock.Any()).
341+
Return(&ec2.DescribeNatGatewaysOutput{
342+
NatGateways: []types.NatGateway{
343+
{
344+
State: types.NatGatewayStateAvailable,
345+
NatGatewayId: aws.String("natgateway-1"),
346+
SubnetId: aws.String("subnet-1"),
347+
},
348+
},
349+
}, nil)
299350

300-
m.CreateTags(context.TODO(), gomock.AssignableToTypeOf(&ec2.CreateTagsInput{})).
301-
Return(nil, nil).Times(1)
351+
m.DescribeNatGateways(gomock.Any(), gomock.Any(), gomock.Any()).
352+
Return(&ec2.DescribeNatGatewaysOutput{
353+
NatGateways: []types.NatGateway{
354+
{
355+
State: types.NatGatewayStateAvailable,
356+
NatGatewayId: aws.String("natgateway-2"),
357+
SubnetId: aws.String("subnet-3"),
358+
},
359+
},
360+
}, nil)
302361
},
303362
},
304363
{

0 commit comments

Comments
 (0)