Skip to content

Commit 21da602

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #5602 from giantswarm/natgw-per-az
✨ Create only one nat gateway per AZ
2 parents add0e1c + c04fd7c commit 21da602

File tree

3 files changed

+176
-96
lines changed

3 files changed

+176
-96
lines changed

‎controlplane/eks/controllers/awsmanagedcontrolplane_controller_test.go‎

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

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

620623
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)