From c426f23bbb972de74d43cc5cb8fccf2bfbfcffb6 Mon Sep 17 00:00:00 2001 From: Gergely Brautigam <182850+Skarlso@users.noreply.github.com> Date: Wed, 4 Oct 2023 09:46:25 +0200 Subject: [PATCH] feat: create vpc objects in explicitly provided availability zones --- api/v1beta1/awscluster_conversion.go | 1 + api/v1beta1/zz_generated.conversion.go | 1 + api/v1beta2/network_types.go | 5 + api/v1beta2/zz_generated.deepcopy.go | 5 + ...ster.x-k8s.io_awsmanagedcontrolplanes.yaml | 16 ++ ...tructure.cluster.x-k8s.io_awsclusters.yaml | 8 + ....cluster.x-k8s.io_awsclustertemplates.yaml | 8 + pkg/cloud/services/network/subnets.go | 66 ++++++--- pkg/cloud/services/network/subnets_test.go | 140 ++++++++++++++++++ 9 files changed, 226 insertions(+), 24 deletions(-) diff --git a/api/v1beta1/awscluster_conversion.go b/api/v1beta1/awscluster_conversion.go index a3763d22c8..318203aaa9 100644 --- a/api/v1beta1/awscluster_conversion.go +++ b/api/v1beta1/awscluster_conversion.go @@ -82,6 +82,7 @@ func (src *AWSCluster) ConvertTo(dstRaw conversion.Hub) error { } dst.Spec.NetworkSpec.AdditionalControlPlaneIngressRules = restored.Spec.NetworkSpec.AdditionalControlPlaneIngressRules + dst.Spec.NetworkSpec.VPC.AvailabilityZones = restored.Spec.NetworkSpec.VPC.AvailabilityZones if restored.Spec.NetworkSpec.VPC.IPAMPool != nil { if dst.Spec.NetworkSpec.VPC.IPAMPool == nil { diff --git a/api/v1beta1/zz_generated.conversion.go b/api/v1beta1/zz_generated.conversion.go index 30c7102779..920945c464 100644 --- a/api/v1beta1/zz_generated.conversion.go +++ b/api/v1beta1/zz_generated.conversion.go @@ -2303,6 +2303,7 @@ func autoConvert_v1beta2_VPCSpec_To_v1beta1_VPCSpec(in *v1beta2.VPCSpec, out *VP out.Tags = *(*Tags)(unsafe.Pointer(&in.Tags)) out.AvailabilityZoneUsageLimit = (*int)(unsafe.Pointer(in.AvailabilityZoneUsageLimit)) out.AvailabilityZoneSelection = (*AZSelectionScheme)(unsafe.Pointer(in.AvailabilityZoneSelection)) + // WARNING: in.AvailabilityZones requires manual conversion: does not exist in peer-type // WARNING: in.EmptyRoutesDefaultVPCSecurityGroup requires manual conversion: does not exist in peer-type // WARNING: in.PrivateDNSHostnameTypeOnLaunch requires manual conversion: does not exist in peer-type return nil diff --git a/api/v1beta2/network_types.go b/api/v1beta2/network_types.go index ea5d627a61..f5113fcd6f 100644 --- a/api/v1beta2/network_types.go +++ b/api/v1beta2/network_types.go @@ -341,6 +341,11 @@ type VPCSpec struct { // +kubebuilder:validation:Enum=Ordered;Random AvailabilityZoneSelection *AZSelectionScheme `json:"availabilityZoneSelection,omitempty"` + // AvailabilityZones defines a list of Availability Zones in which to create network resources in. + // If defined, both AvailabilityZoneUsageLimit and AvailabilityZoneSelection are ignored. + // +optional + AvailabilityZones []string `json:"availabilityZones,omitempty"` + // EmptyRoutesDefaultVPCSecurityGroup specifies whether the default VPC security group ingress // and egress rules should be removed. // diff --git a/api/v1beta2/zz_generated.deepcopy.go b/api/v1beta2/zz_generated.deepcopy.go index ee0dc510c2..2a0469ee41 100644 --- a/api/v1beta2/zz_generated.deepcopy.go +++ b/api/v1beta2/zz_generated.deepcopy.go @@ -2025,6 +2025,11 @@ func (in *VPCSpec) DeepCopyInto(out *VPCSpec) { *out = new(AZSelectionScheme) **out = **in } + if in.AvailabilityZones != nil { + in, out := &in.AvailabilityZones, &out.AvailabilityZones + *out = make([]string, len(*in)) + copy(*out, *in) + } if in.PrivateDNSHostnameTypeOnLaunch != nil { in, out := &in.PrivateDNSHostnameTypeOnLaunch, &out.PrivateDNSHostnameTypeOnLaunch *out = new(string) diff --git a/config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml b/config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml index 3227df4d81..7d1975ed59 100644 --- a/config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml +++ b/config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml @@ -565,6 +565,14 @@ spec: to 3 minimum: 1 type: integer + availabilityZones: + description: AvailabilityZones defines a list of Availability + Zones in which to create network resources in. If defined, + both AvailabilityZoneUsageLimit and AvailabilityZoneSelection + are ignored. + items: + type: string + type: array cidrBlock: description: CidrBlock is the CIDR block to be used when the provider creates a managed VPC. Defaults to 10.0.0.0/16. @@ -2404,6 +2412,14 @@ spec: to 3 minimum: 1 type: integer + availabilityZones: + description: AvailabilityZones defines a list of Availability + Zones in which to create network resources in. If defined, + both AvailabilityZoneUsageLimit and AvailabilityZoneSelection + are ignored. + items: + type: string + type: array cidrBlock: description: CidrBlock is the CIDR block to be used when the provider creates a managed VPC. Defaults to 10.0.0.0/16. diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml index a74b9c7e82..18092a0c86 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml @@ -1399,6 +1399,14 @@ spec: to 3 minimum: 1 type: integer + availabilityZones: + description: AvailabilityZones defines a list of Availability + Zones in which to create network resources in. If defined, + both AvailabilityZoneUsageLimit and AvailabilityZoneSelection + are ignored. + items: + type: string + type: array cidrBlock: description: CidrBlock is the CIDR block to be used when the provider creates a managed VPC. Defaults to 10.0.0.0/16. diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml index e8ef04c449..837897b547 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml @@ -1018,6 +1018,14 @@ spec: when creating default subnets. Defaults to 3 minimum: 1 type: integer + availabilityZones: + description: AvailabilityZones defines a list of Availability + Zones in which to create network resources in. If + defined, both AvailabilityZoneUsageLimit and AvailabilityZoneSelection + are ignored. + items: + type: string + type: array cidrBlock: description: CidrBlock is the CIDR block to be used when the provider creates a managed VPC. Defaults diff --git a/pkg/cloud/services/network/subnets.go b/pkg/cloud/services/network/subnets.go index 65a70e7445..b45a3253ae 100644 --- a/pkg/cloud/services/network/subnets.go +++ b/pkg/cloud/services/network/subnets.go @@ -212,32 +212,18 @@ func (s *Service) reconcileSubnets() error { } func (s *Service) getDefaultSubnets() (infrav1.Subnets, error) { - zones, err := s.getAvailableZones() - if err != nil { - return nil, err - } - - maxZones := defaultMaxNumAZs - if s.scope.VPC().AvailabilityZoneUsageLimit != nil { - maxZones = *s.scope.VPC().AvailabilityZoneUsageLimit - } - selectionScheme := infrav1.AZSelectionSchemeOrdered - if s.scope.VPC().AvailabilityZoneSelection != nil { - selectionScheme = *s.scope.VPC().AvailabilityZoneSelection - } + var ( + // by default, we will take the set availability zones, if they are defined. + // if not, we fall back to the two other settings. + zones = s.scope.VPC().AvailabilityZones + err error + ) - if len(zones) > maxZones { - s.scope.Debug("region has more than AvailabilityZoneUsageLimit availability zones, picking zones to use", "region", s.scope.Region(), "AvailabilityZoneUsageLimit", maxZones) - if selectionScheme == infrav1.AZSelectionSchemeRandom { - rand.Shuffle(len(zones), func(i, j int) { - zones[i], zones[j] = zones[j], zones[i] - }) - } - if selectionScheme == infrav1.AZSelectionSchemeOrdered { - sort.Strings(zones) + if len(zones) == 0 { + zones, err = s.selectZones() + if err != nil { + return nil, errors.Wrap(err, "failed to select availability zones") } - zones = zones[:maxZones] - s.scope.Debug("zones selected", "region", s.scope.Region(), "zones", zones) } // 1 private subnet for each AZ plus 1 other subnet that will be further sub-divided for the public subnets @@ -304,6 +290,38 @@ func (s *Service) getDefaultSubnets() (infrav1.Subnets, error) { return subnets, nil } +func (s *Service) selectZones() ([]string, error) { + zones, err := s.getAvailableZones() + if err != nil { + return nil, err + } + + maxZones := defaultMaxNumAZs + if s.scope.VPC().AvailabilityZoneUsageLimit != nil { + maxZones = *s.scope.VPC().AvailabilityZoneUsageLimit + } + selectionScheme := infrav1.AZSelectionSchemeOrdered + if s.scope.VPC().AvailabilityZoneSelection != nil { + selectionScheme = *s.scope.VPC().AvailabilityZoneSelection + } + + if len(zones) > maxZones { + s.scope.Debug("region has more than AvailabilityZoneUsageLimit availability zones, picking zones to use", "region", s.scope.Region(), "AvailabilityZoneUsageLimit", maxZones) + if selectionScheme == infrav1.AZSelectionSchemeRandom { + rand.Shuffle(len(zones), func(i, j int) { + zones[i], zones[j] = zones[j], zones[i] + }) + } + if selectionScheme == infrav1.AZSelectionSchemeOrdered { + sort.Strings(zones) + } + zones = zones[:maxZones] + s.scope.Debug("zones selected", "region", s.scope.Region(), "zones", zones) + } + + return zones, nil +} + func (s *Service) deleteSubnets() error { if s.scope.VPC().IsUnmanaged(s.scope.Name()) { s.scope.Trace("Skipping subnets deletion in unmanaged mode") diff --git a/pkg/cloud/services/network/subnets_test.go b/pkg/cloud/services/network/subnets_test.go index 840583a37c..faa4e802ce 100644 --- a/pkg/cloud/services/network/subnets_test.go +++ b/pkg/cloud/services/network/subnets_test.go @@ -2316,6 +2316,146 @@ func TestReconcileSubnets(t *testing.T) { After(zone2PrivateSubnet) }, }, + { + name: "Managed VPC, no existing subnets exist, one az is explicitly defined, expect one private and one public from default", + input: NewClusterScope().WithNetwork(&infrav1.NetworkSpec{ + VPC: infrav1.VPCSpec{ + ID: subnetsVPCID, + Tags: infrav1.Tags{ + infrav1.ClusterTagKey("test-cluster"): "owned", + }, + CidrBlock: defaultVPCCidr, + AvailabilityZones: []string{"us-east-1b"}, + }, + Subnets: []infrav1.SubnetSpec{}, + }), + expect: func(m *mocks.MockEC2APIMockRecorder) { + describeCall := m.DescribeSubnetsWithContext(context.TODO(), gomock.Eq(&ec2.DescribeSubnetsInput{ + Filters: []*ec2.Filter{ + { + Name: aws.String("state"), + Values: []*string{aws.String("pending"), aws.String("available")}, + }, + { + Name: aws.String("vpc-id"), + Values: []*string{aws.String(subnetsVPCID)}, + }, + }, + })).Return(&ec2.DescribeSubnetsOutput{}, nil) + + m.DescribeRouteTablesWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.DescribeRouteTablesInput{})). + Return(&ec2.DescribeRouteTablesOutput{}, nil) + + m.DescribeNatGatewaysPagesWithContext(context.TODO(), + gomock.Eq(&ec2.DescribeNatGatewaysInput{ + Filter: []*ec2.Filter{ + { + Name: aws.String("vpc-id"), + Values: []*string{aws.String(subnetsVPCID)}, + }, + { + Name: aws.String("state"), + Values: []*string{aws.String("pending"), aws.String("available")}, + }, + }, + }), + gomock.Any()).Return(nil) + + zone1PublicSubnet := m.CreateSubnetWithContext(context.TODO(), gomock.Eq(&ec2.CreateSubnetInput{ + VpcId: aws.String(subnetsVPCID), + CidrBlock: aws.String("10.0.0.0/17"), + AvailabilityZone: aws.String("us-east-1b"), + TagSpecifications: []*ec2.TagSpecification{ + { + ResourceType: aws.String("subnet"), + Tags: []*ec2.Tag{ + { + Key: aws.String("Name"), + Value: aws.String("test-cluster-subnet-public-us-east-1b"), + }, + { + Key: aws.String("kubernetes.io/cluster/test-cluster"), + Value: aws.String("shared"), + }, + { + Key: aws.String("kubernetes.io/role/elb"), + Value: aws.String("1"), + }, + { + 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("public"), + }, + }, + }, + }, + })).Return(&ec2.CreateSubnetOutput{ + Subnet: &ec2.Subnet{ + VpcId: aws.String(subnetsVPCID), + SubnetId: aws.String("subnet-1"), + CidrBlock: aws.String("10.0.0.0/17"), + AvailabilityZone: aws.String("us-east-1b"), + MapPublicIpOnLaunch: aws.Bool(false), + }, + }, nil).After(describeCall) + + m.WaitUntilSubnetAvailableWithContext(context.TODO(), gomock.Any()).After(zone1PublicSubnet) + + m.ModifySubnetAttributeWithContext(context.TODO(), &ec2.ModifySubnetAttributeInput{ + MapPublicIpOnLaunch: &ec2.AttributeBooleanValue{ + Value: aws.Bool(true), + }, + SubnetId: aws.String("subnet-1"), + }).Return(&ec2.ModifySubnetAttributeOutput{}, nil).After(zone1PublicSubnet) + + zone1PrivateSubnet := m.CreateSubnetWithContext(context.TODO(), gomock.Eq(&ec2.CreateSubnetInput{ + VpcId: aws.String(subnetsVPCID), + CidrBlock: aws.String("10.0.128.0/17"), + AvailabilityZone: aws.String("us-east-1b"), + TagSpecifications: []*ec2.TagSpecification{ + { + ResourceType: aws.String("subnet"), + Tags: []*ec2.Tag{ + { + Key: aws.String("Name"), + Value: aws.String("test-cluster-subnet-private-us-east-1b"), + }, + { + Key: aws.String("kubernetes.io/cluster/test-cluster"), + Value: aws.String("shared"), + }, + { + Key: aws.String("kubernetes.io/role/internal-elb"), + Value: aws.String("1"), + }, + { + 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("private"), + }, + }, + }, + }, + })).Return(&ec2.CreateSubnetOutput{ + Subnet: &ec2.Subnet{ + VpcId: aws.String(subnetsVPCID), + SubnetId: aws.String("subnet-2"), + CidrBlock: aws.String("10.0.128.0/17"), + AvailabilityZone: aws.String("us-east-1b"), + MapPublicIpOnLaunch: aws.Bool(false), + }, + }, nil).After(zone1PublicSubnet) + + m.WaitUntilSubnetAvailableWithContext(context.TODO(), gomock.Any()). + After(zone1PrivateSubnet) + }, + }, } for _, tc := range testCases {