diff --git a/controlplane/eks/controllers/awsmanagedcontrolplane_controller_test.go b/controlplane/eks/controllers/awsmanagedcontrolplane_controller_test.go index 2247b92ea5..767b8ca8a0 100644 --- a/controlplane/eks/controllers/awsmanagedcontrolplane_controller_test.go +++ b/controlplane/eks/controllers/awsmanagedcontrolplane_controller_test.go @@ -551,71 +551,74 @@ func mockedCallsForMissingEverything(ec2Rec *mocks.MockEC2APIMockRecorder, subne eipAllocationID := strconv.Itoa(1234 + subnetIndex) natGatewayID := fmt.Sprintf("nat-%d", subnetIndex+1) - ec2Rec.AllocateAddress(context.TODO(), gomock.Eq(&ec2.AllocateAddressInput{ - Domain: ec2types.DomainTypeVpc, - TagSpecifications: []ec2types.TagSpecification{ - { - ResourceType: ec2types.ResourceTypeElasticIp, - Tags: []ec2types.Tag{ - { - Key: aws.String("Name"), - Value: aws.String("test-cluster-eip-common"), - }, - { - Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"), - Value: aws.String("owned"), - }, - { - Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"), - Value: aws.String("common"), + // We only expect to create NAT gateways for the public subnet AZ used by the private subnet + if subnet.ID == "my-managed-subnet-pub1" { + ec2Rec.AllocateAddress(context.TODO(), gomock.Eq(&ec2.AllocateAddressInput{ + Domain: ec2types.DomainTypeVpc, + TagSpecifications: []ec2types.TagSpecification{ + { + ResourceType: ec2types.ResourceTypeElasticIp, + Tags: []ec2types.Tag{ + { + Key: aws.String("Name"), + Value: aws.String("test-cluster-eip-common"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"), + Value: aws.String("owned"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"), + Value: aws.String("common"), + }, }, }, }, - }, - })).Return(&ec2.AllocateAddressOutput{ - AllocationId: aws.String(eipAllocationID), - }, nil) - - ec2Rec.CreateNatGateway(context.TODO(), gomock.Eq(&ec2.CreateNatGatewayInput{ - AllocationId: aws.String(eipAllocationID), - SubnetId: aws.String(subnetID), - TagSpecifications: []ec2types.TagSpecification{ - { - ResourceType: ec2types.ResourceTypeNatgateway, - Tags: []ec2types.Tag{ - { - Key: aws.String("Name"), - Value: aws.String("test-cluster-nat"), - }, - { - Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"), - Value: aws.String("owned"), - }, - { - Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"), - Value: aws.String("common"), + })).Return(&ec2.AllocateAddressOutput{ + AllocationId: aws.String(eipAllocationID), + }, nil) + + ec2Rec.CreateNatGateway(context.TODO(), gomock.Eq(&ec2.CreateNatGatewayInput{ + AllocationId: aws.String(eipAllocationID), + SubnetId: aws.String(subnetID), + TagSpecifications: []ec2types.TagSpecification{ + { + ResourceType: ec2types.ResourceTypeNatgateway, + Tags: []ec2types.Tag{ + { + Key: aws.String("Name"), + Value: aws.String("test-cluster-nat"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"), + Value: aws.String("owned"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"), + Value: aws.String("common"), + }, }, }, }, - }, - })).Return(&ec2.CreateNatGatewayOutput{ - NatGateway: &ec2types.NatGateway{ - NatGatewayId: aws.String(natGatewayID), - SubnetId: aws.String(subnetID), - }, - }, nil) - - ec2Rec.DescribeNatGateways(gomock.Any(), gomock.Eq(&ec2.DescribeNatGatewaysInput{ - NatGatewayIds: []string{natGatewayID}, - }), gomock.Any()).Return(&ec2.DescribeNatGatewaysOutput{ - NatGateways: []ec2types.NatGateway{ - { + })).Return(&ec2.CreateNatGatewayOutput{ + NatGateway: &ec2types.NatGateway{ NatGatewayId: aws.String(natGatewayID), SubnetId: aws.String(subnetID), - State: ec2types.NatGatewayStateAvailable, }, - }, - }, nil) + }, nil) + + ec2Rec.DescribeNatGateways(gomock.Any(), gomock.Eq(&ec2.DescribeNatGatewaysInput{ + NatGatewayIds: []string{natGatewayID}, + }), gomock.Any()).Return(&ec2.DescribeNatGatewaysOutput{ + NatGateways: []ec2types.NatGateway{ + { + NatGatewayId: aws.String(natGatewayID), + SubnetId: aws.String(subnetID), + State: ec2types.NatGatewayStateAvailable, + }, + }, + }, nil) + } } routeTableID := fmt.Sprintf("rtb-%d", subnetIndex+1) diff --git a/pkg/cloud/services/network/natgateways.go b/pkg/cloud/services/network/natgateways.go index f3c7b33ece..6bde5f5a64 100644 --- a/pkg/cloud/services/network/natgateways.go +++ b/pkg/cloud/services/network/natgateways.go @@ -115,11 +115,29 @@ func (s *Service) updateNatGatewayIPs(updateTags bool) ([]string, error) { natGatewaysIPs := []string{} subnetIDs := []string{} + // Find AZs that have private subnets + privateSubnetAZs := make(map[string]bool) + for _, sn := range s.scope.Subnets().FilterPrivate().FilterNonCni() { + if sn.GetResourceID() != "" { + privateSubnetAZs[sn.AvailabilityZone] = true + } + } + + // For each AZ with private subnets, find a public subnet and check for NAT gateway + processedAZs := make(map[string]bool) for _, sn := range s.scope.Subnets().FilterPublic().FilterNonCni() { if sn.GetResourceID() == "" { continue } + // Only process this AZ if it has private subnets and we haven't processed it yet + if !privateSubnetAZs[sn.AvailabilityZone] || processedAZs[sn.AvailabilityZone] { + continue + } + + // Mark this AZ as processed + processedAZs[sn.AvailabilityZone] = true + if ngw, ok := existing[sn.GetResourceID()]; ok { if len(ngw.NatGatewayAddresses) > 0 && ngw.NatGatewayAddresses[0].PublicIp != nil { natGatewaysIPs = append(natGatewaysIPs, *ngw.NatGatewayAddresses[0].PublicIp) diff --git a/pkg/cloud/services/network/natgateways_test.go b/pkg/cloud/services/network/natgateways_test.go index ae3cb26718..da1586e1db 100644 --- a/pkg/cloud/services/network/natgateways_test.go +++ b/pkg/cloud/services/network/natgateways_test.go @@ -183,7 +183,7 @@ func TestReconcileNatGateways(t *testing.T) { }, }, { - name: "two public & 1 private subnet, and one NAT gateway exists", + name: "two public & 1 private subnet, and one NAT gateway exists, should not create additional NAT gateway", input: []infrav1.SubnetSpec{ { ID: "subnet-1", @@ -224,10 +224,74 @@ func TestReconcileNatGateways(t *testing.T) { { NatGatewayId: aws.String("gateway"), SubnetId: aws.String("subnet-1"), + Tags: []types.Tag{ + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"), + Value: aws.String("common"), + }, + { + Key: aws.String("Name"), + Value: aws.String("test-cluster-nat"), + }, + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"), + Value: aws.String("owned"), + }, + }, }, }, }, nil) + // Should not create any new NAT gateways because subnet-3 (public subnet in us-east-1b) has no private subnets + m.DescribeAddresses(context.TODO(), gomock.Any()).Times(0) + m.AllocateAddress(context.TODO(), gomock.Any()).Times(0) + m.CreateNatGateway(context.TODO(), gomock.Any()).Times(0) + }, + }, + { + name: "multiple AZs with private subnets, should create one NAT gateway per AZ", + input: []infrav1.SubnetSpec{ + { + ID: "subnet-1", + AvailabilityZone: "us-east-1a", + CidrBlock: "10.0.10.0/24", + IsPublic: true, + }, + { + ID: "subnet-2", + AvailabilityZone: "us-east-1a", + CidrBlock: "10.0.12.0/24", + IsPublic: false, + }, + { + ID: "subnet-3", + AvailabilityZone: "us-east-1b", + CidrBlock: "10.0.13.0/24", + IsPublic: true, + }, + { + ID: "subnet-4", + AvailabilityZone: "us-east-1b", + CidrBlock: "10.0.14.0/24", + IsPublic: false, + }, + }, + expect: func(m *mocks.MockEC2APIMockRecorder) { + m.DescribeNatGateways(context.TODO(), + gomock.Eq(&ec2.DescribeNatGatewaysInput{ + Filter: []types.Filter{ + { + Name: aws.String("vpc-id"), + Values: []string{subnetsVPCID}, + }, + { + Name: aws.String("state"), + Values: []string{"pending", "available"}, + }, + }, + }), + gomock.Any()).Return(&ec2.DescribeNatGatewaysOutput{}, nil) + m.DescribeAddresses(context.TODO(), gomock.Any()). Return(&ec2.DescribeAddressesOutput{}, nil) @@ -254,51 +318,46 @@ func TestReconcileNatGateways(t *testing.T) { }, }).Return(&ec2.AllocateAddressOutput{ AllocationId: aws.String(ElasticIPAllocationID), - }, nil) + }, nil).Times(2) - m.CreateNatGateway(context.TODO(), &ec2.CreateNatGatewayInput{ - AllocationId: aws.String(ElasticIPAllocationID), - SubnetId: aws.String("subnet-3"), - TagSpecifications: []types.TagSpecification{ - { - ResourceType: types.ResourceTypeNatgateway, - Tags: []types.Tag{ - { - Key: aws.String("Name"), - Value: aws.String("test-cluster-nat"), - }, - { - Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"), - Value: aws.String("owned"), - }, - { - Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"), - Value: aws.String("common"), - }, - }, + // Should create NAT gateways for both AZs since both have private subnets + m.CreateNatGateway(context.TODO(), gomock.Any()). + Return(&ec2.CreateNatGatewayOutput{ + NatGateway: &types.NatGateway{ + NatGatewayId: aws.String("natgateway-1"), + SubnetId: aws.String("subnet-1"), }, - }, - }).Return(&ec2.CreateNatGatewayOutput{ - NatGateway: &types.NatGateway{ - NatGatewayId: aws.String("natgateway"), - SubnetId: aws.String("subnet-3"), - }, - }, nil) + }, nil) - m.DescribeNatGateways(gomock.Any(), &ec2.DescribeNatGatewaysInput{ - NatGatewayIds: []string{"natgateway"}, - }, gomock.Any()).Return(&ec2.DescribeNatGatewaysOutput{ - NatGateways: []types.NatGateway{ - { - State: types.NatGatewayStateAvailable, - NatGatewayId: aws.String("natgateway"), + m.CreateNatGateway(context.TODO(), gomock.Any()). + Return(&ec2.CreateNatGatewayOutput{ + NatGateway: &types.NatGateway{ + NatGatewayId: aws.String("natgateway-2"), SubnetId: aws.String("subnet-3"), }, - }, - }, nil) + }, nil) + + m.DescribeNatGateways(gomock.Any(), gomock.Any(), gomock.Any()). + Return(&ec2.DescribeNatGatewaysOutput{ + NatGateways: []types.NatGateway{ + { + State: types.NatGatewayStateAvailable, + NatGatewayId: aws.String("natgateway-1"), + SubnetId: aws.String("subnet-1"), + }, + }, + }, nil) - m.CreateTags(context.TODO(), gomock.AssignableToTypeOf(&ec2.CreateTagsInput{})). - Return(nil, nil).Times(1) + m.DescribeNatGateways(gomock.Any(), gomock.Any(), gomock.Any()). + Return(&ec2.DescribeNatGatewaysOutput{ + NatGateways: []types.NatGateway{ + { + State: types.NatGatewayStateAvailable, + NatGatewayId: aws.String("natgateway-2"), + SubnetId: aws.String("subnet-3"), + }, + }, + }, nil) }, }, {