diff --git a/api/v1beta1/awscluster_conversion.go b/api/v1beta1/awscluster_conversion.go index 65e797ffba..a84d5368a3 100644 --- a/api/v1beta1/awscluster_conversion.go +++ b/api/v1beta1/awscluster_conversion.go @@ -84,6 +84,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 565df1c3aa..5384c81839 100644 --- a/api/v1beta1/zz_generated.conversion.go +++ b/api/v1beta1/zz_generated.conversion.go @@ -2313,6 +2313,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 // WARNING: in.ElasticIPPool requires manual conversion: does not exist in peer-type diff --git a/api/v1beta2/awscluster_webhook.go b/api/v1beta2/awscluster_webhook.go index 5dbfd7a598..3a8249aa13 100644 --- a/api/v1beta2/awscluster_webhook.go +++ b/api/v1beta2/awscluster_webhook.go @@ -283,6 +283,10 @@ func (r *AWSCluster) validateNetwork() field.ErrorList { } } + if err := r.Spec.NetworkSpec.VPC.ValidateAvailabilityZones(); err != nil { + allErrs = append(allErrs, err) + } + return allErrs } diff --git a/api/v1beta2/defaults.go b/api/v1beta2/defaults.go index f10bb895c1..d2be67c5c0 100644 --- a/api/v1beta2/defaults.go +++ b/api/v1beta2/defaults.go @@ -18,6 +18,7 @@ package v1beta2 import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" clusterv1 "sigs.k8s.io/cluster-api/cmd/clusterctl/api/v1alpha3" ) @@ -51,6 +52,15 @@ func SetDefaults_NetworkSpec(obj *NetworkSpec) { //nolint:golint,stylecheck }, } } + // If AvailabilityZones are not set, set defaults for AZ selection + if obj.VPC.AvailabilityZones == nil { + if obj.VPC.AvailabilityZoneUsageLimit == nil { + obj.VPC.AvailabilityZoneUsageLimit = ptr.To(3) + } + if obj.VPC.AvailabilityZoneSelection == nil { + obj.VPC.AvailabilityZoneSelection = &AZSelectionSchemeOrdered + } + } } // SetDefaults_AWSClusterSpec is used by defaulter-gen. diff --git a/api/v1beta2/network_types.go b/api/v1beta2/network_types.go index 94442ce6f4..77eae41f2f 100644 --- a/api/v1beta2/network_types.go +++ b/api/v1beta2/network_types.go @@ -23,6 +23,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" + "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/utils/ptr" ) @@ -424,7 +425,7 @@ type VPCSpec struct { // should be used in a region when automatically creating subnets. If a region has more // than this number of AZs then this number of AZs will be picked randomly when creating // default subnets. Defaults to 3 - // +kubebuilder:default=3 + // +optional // +kubebuilder:validation:Minimum=1 AvailabilityZoneUsageLimit *int `json:"availabilityZoneUsageLimit,omitempty"` @@ -433,10 +434,16 @@ type VPCSpec struct { // Ordered - selects based on alphabetical order // Random - selects AZs randomly in a region // Defaults to Ordered - // +kubebuilder:default=Ordered + // +optional // +kubebuilder:validation:Enum=Ordered;Random AvailabilityZoneSelection *AZSelectionScheme `json:"availabilityZoneSelection,omitempty"` + // AvailabilityZones defines a list of Availability Zones in which to create network resources in. + // Cannot be defined at the same time as AvailabilityZoneSelection and AvailabilityZoneUsageLimit. + // +optional + // +kubebuilder:validation:MinItems=1 + AvailabilityZones []string `json:"availabilityZones,omitempty"` + // EmptyRoutesDefaultVPCSecurityGroup specifies whether the default VPC security group ingress // and egress rules should be removed. // @@ -499,6 +506,15 @@ func (v *VPCSpec) GetPublicIpv4Pool() *string { return nil } +// ValidateAvailabilityZones returns an error if the availability zones field combination is invalid. +func (v *VPCSpec) ValidateAvailabilityZones() *field.Error { + if len(v.AvailabilityZones) > 0 && (v.AvailabilityZoneSelection != nil || v.AvailabilityZoneUsageLimit != nil) { + availabilityZonesField := field.NewPath("spec", "network", "vpc", "availabilityZones") + return field.Invalid(availabilityZonesField, v.AvailabilityZoneSelection, "availabilityZones cannot be set if availabilityZoneUsageLimit and availabilityZoneSelection are set") + } + return nil +} + // SubnetSpec configures an AWS Subnet. type SubnetSpec struct { // ID defines a unique identifier to reference this resource. diff --git a/api/v1beta2/zz_generated.deepcopy.go b/api/v1beta2/zz_generated.deepcopy.go index d3e08bbb0c..86988c1ca4 100644 --- a/api/v1beta2/zz_generated.deepcopy.go +++ b/api/v1beta2/zz_generated.deepcopy.go @@ -2182,6 +2182,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 7ab077b658..fa1da40965 100644 --- a/config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml +++ b/config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml @@ -598,7 +598,6 @@ spec: description: VPC configuration. properties: availabilityZoneSelection: - default: Ordered description: |- AvailabilityZoneSelection specifies how AZs should be selected if there are more AZs in a region than specified by AvailabilityZoneUsageLimit. There are 2 selection schemes: @@ -610,7 +609,6 @@ spec: - Random type: string availabilityZoneUsageLimit: - default: 3 description: |- AvailabilityZoneUsageLimit specifies the maximum number of availability zones (AZ) that should be used in a region when automatically creating subnets. If a region has more @@ -618,6 +616,14 @@ spec: 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 + minItems: 1 + type: array carrierGatewayId: description: |- CarrierGatewayID is the id of the internet gateway associated with the VPC, @@ -2589,7 +2595,6 @@ spec: description: VPC configuration. properties: availabilityZoneSelection: - default: Ordered description: |- AvailabilityZoneSelection specifies how AZs should be selected if there are more AZs in a region than specified by AvailabilityZoneUsageLimit. There are 2 selection schemes: @@ -2601,7 +2606,6 @@ spec: - Random type: string availabilityZoneUsageLimit: - default: 3 description: |- AvailabilityZoneUsageLimit specifies the maximum number of availability zones (AZ) that should be used in a region when automatically creating subnets. If a region has more @@ -2609,6 +2613,14 @@ spec: 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 + minItems: 1 + type: array carrierGatewayId: description: |- CarrierGatewayID is the id of the internet gateway associated with the VPC, 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 cadff4f653..c9c7a13b17 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml @@ -1538,7 +1538,6 @@ spec: description: VPC configuration. properties: availabilityZoneSelection: - default: Ordered description: |- AvailabilityZoneSelection specifies how AZs should be selected if there are more AZs in a region than specified by AvailabilityZoneUsageLimit. There are 2 selection schemes: @@ -1550,7 +1549,6 @@ spec: - Random type: string availabilityZoneUsageLimit: - default: 3 description: |- AvailabilityZoneUsageLimit specifies the maximum number of availability zones (AZ) that should be used in a region when automatically creating subnets. If a region has more @@ -1558,6 +1556,14 @@ spec: 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 + minItems: 1 + type: array carrierGatewayId: description: |- CarrierGatewayID is the id of the internet gateway associated with the VPC, 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 c353bcff30..b37449fbf3 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml @@ -1136,7 +1136,6 @@ spec: description: VPC configuration. properties: availabilityZoneSelection: - default: Ordered description: |- AvailabilityZoneSelection specifies how AZs should be selected if there are more AZs in a region than specified by AvailabilityZoneUsageLimit. There are 2 selection schemes: @@ -1148,7 +1147,6 @@ spec: - Random type: string availabilityZoneUsageLimit: - default: 3 description: |- AvailabilityZoneUsageLimit specifies the maximum number of availability zones (AZ) that should be used in a region when automatically creating subnets. If a region has more @@ -1156,6 +1154,14 @@ spec: 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 + minItems: 1 + type: array carrierGatewayId: description: |- CarrierGatewayID is the id of the internet gateway associated with the VPC, diff --git a/controlplane/eks/api/v1beta2/awsmanagedcontrolplane_webhook.go b/controlplane/eks/api/v1beta2/awsmanagedcontrolplane_webhook.go index 4b44508b65..ba937f12da 100644 --- a/controlplane/eks/api/v1beta2/awsmanagedcontrolplane_webhook.go +++ b/controlplane/eks/api/v1beta2/awsmanagedcontrolplane_webhook.go @@ -27,6 +27,7 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apimachinery/pkg/util/version" "k8s.io/klog/v2" + "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" @@ -426,6 +427,10 @@ func (r *AWSManagedControlPlane) validateNetwork() field.ErrorList { allErrs = append(allErrs, field.Invalid(ipamPoolField, r.Spec.NetworkSpec.VPC.IPv6.IPAMPool, "ipamPool must have either id or name")) } + if err := r.Spec.NetworkSpec.VPC.ValidateAvailabilityZones(); err != nil { + allErrs = append(allErrs, err) + } + return allErrs } @@ -452,6 +457,16 @@ func (r *AWSManagedControlPlane) Default() { } } + // If AvailabilityZones are not set, set defaults for AZ selection + if r.Spec.NetworkSpec.VPC.AvailabilityZones == nil { + if r.Spec.NetworkSpec.VPC.AvailabilityZoneUsageLimit == nil { + r.Spec.NetworkSpec.VPC.AvailabilityZoneUsageLimit = ptr.To(3) + } + if r.Spec.NetworkSpec.VPC.AvailabilityZoneSelection == nil { + r.Spec.NetworkSpec.VPC.AvailabilityZoneSelection = &infrav1.AZSelectionSchemeOrdered + } + } + infrav1.SetDefaults_Bastion(&r.Spec.Bastion) infrav1.SetDefaults_NetworkSpec(&r.Spec.NetworkSpec) } diff --git a/pkg/cloud/services/network/subnets.go b/pkg/cloud/services/network/subnets.go index f125c7486e..531ec53806 100644 --- a/pkg/cloud/services/network/subnets.go +++ b/pkg/cloud/services/network/subnets.go @@ -247,32 +247,18 @@ func (s *Service) reconcileZoneInfo(subnets infrav1.Subnets) 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 @@ -339,6 +325,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 b196536d6b..9db38982cc 100644 --- a/pkg/cloud/services/network/subnets_test.go +++ b/pkg/cloud/services/network/subnets_test.go @@ -3071,6 +3071,176 @@ func TestReconcileSubnets(t *testing.T) { stubMockCreateTagsWithContext(m, "test-cluster", "subnet-az-1a-private", "us-east-1a", "private", false).AnyTimes() }, }, + { + 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.DescribeAvailabilityZonesWithContext(context.TODO(), gomock.Any()). + Return(&ec2.DescribeAvailabilityZonesOutput{ + AvailabilityZones: []*ec2.AvailabilityZone{ + { + ZoneName: aws.String("us-east-1b"), + ZoneType: aws.String("availability-zone"), + }, + }, + }, nil) + + m.DescribeAvailabilityZonesWithContext(context.TODO(), gomock.Any()). + Return(&ec2.DescribeAvailabilityZonesOutput{ + AvailabilityZones: []*ec2.AvailabilityZone{ + { + ZoneName: aws.String("us-east-1b"), + ZoneType: aws.String("availability-zone"), + }, + }, + }, nil) + + m.DescribeAvailabilityZonesWithContext(context.TODO(), gomock.Any()). + Return(&ec2.DescribeAvailabilityZonesOutput{ + AvailabilityZones: []*ec2.AvailabilityZone{ + { + ZoneName: aws.String("us-east-1b"), + ZoneType: aws.String("availability-zone"), + }, + }, + }, 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("owned"), + }, + { + 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("owned"), + }, + { + 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 {