diff --git a/api/v1beta1/awscluster_conversion.go b/api/v1beta1/awscluster_conversion.go index 382a4cd4d3..2438e48524 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 10842bb9ae..4ed3ee8c24 100644 --- a/api/v1beta1/zz_generated.conversion.go +++ b/api/v1beta1/zz_generated.conversion.go @@ -2311,6 +2311,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/awscluster_webhook.go b/api/v1beta2/awscluster_webhook.go index ae9c80f5b4..e4d5fde837 100644 --- a/api/v1beta2/awscluster_webhook.go +++ b/api/v1beta2/awscluster_webhook.go @@ -269,6 +269,11 @@ func (r *AWSCluster) validateNetwork() field.ErrorList { } } + if r.Spec.NetworkSpec.VPC.AvailabilityZones != nil && (r.Spec.NetworkSpec.VPC.AvailabilityZoneSelection != nil || r.Spec.NetworkSpec.VPC.AvailabilityZoneUsageLimit != nil) { + availabilityZonesField := field.NewPath("spec", "networkSpec", "vpc", "availabilityZones") + allErrs = append(allErrs, field.Invalid(availabilityZonesField, r.Spec.NetworkSpec.VPC.AvailabilityZoneSelection, "availabilityZones cannot be set if availabilityZoneUsageLimit and availabilityZoneSelection are set")) + } + return allErrs } diff --git a/api/v1beta2/defaults.go b/api/v1beta2/defaults.go index f10bb895c1..d5790cc12a 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,16 @@ 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 83b5d3a7d1..52c67f58f5 100644 --- a/api/v1beta2/network_types.go +++ b/api/v1beta2/network_types.go @@ -424,7 +424,6 @@ 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 // +kubebuilder:validation:Minimum=1 AvailabilityZoneUsageLimit *int `json:"availabilityZoneUsageLimit,omitempty"` @@ -433,10 +432,14 @@ type VPCSpec struct { // Ordered - selects based on alphabetical order // Random - selects AZs randomly in a region // Defaults to Ordered - // +kubebuilder:default=Ordered // +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 81b8a8d314..0a52cf0078 100644 --- a/api/v1beta2/zz_generated.deepcopy.go +++ b/api/v1beta2/zz_generated.deepcopy.go @@ -2152,6 +2152,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 c9ffb5ecd8..5991b598d4 100644 --- a/config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml +++ b/config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml @@ -594,7 +594,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: @@ -606,7 +605,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 @@ -622,6 +620,13 @@ spec: x-kubernetes-validations: - message: Carrier Gateway ID must start with 'cagw-' rule: self.startsWith('cagw-') + 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. @@ -2544,7 +2549,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: @@ -2556,7 +2560,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 @@ -2572,6 +2575,13 @@ spec: x-kubernetes-validations: - message: Carrier Gateway ID must start with 'cagw-' rule: self.startsWith('cagw-') + 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. 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 f2d4b882b5..97f69244a6 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml @@ -1530,7 +1530,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: @@ -1542,7 +1541,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,13 @@ spec: x-kubernetes-validations: - message: Carrier Gateway ID must start with 'cagw-' rule: self.startsWith('cagw-') + 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. 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 ccc966dbb2..cc53bf26b3 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml @@ -1128,7 +1128,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: @@ -1140,7 +1139,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,13 @@ spec: x-kubernetes-validations: - message: Carrier Gateway ID must start with 'cagw-' rule: self.startsWith('cagw-') + 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. diff --git a/controlplane/eks/api/v1beta2/awsmanagedcontrolplane_webhook.go b/controlplane/eks/api/v1beta2/awsmanagedcontrolplane_webhook.go index 4b44508b65..6b2d227689 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,11 @@ 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 r.Spec.NetworkSpec.VPC.AvailabilityZones != nil && (r.Spec.NetworkSpec.VPC.AvailabilityZoneSelection != nil || r.Spec.NetworkSpec.VPC.AvailabilityZoneUsageLimit != nil) { + availabilityZonesField := field.NewPath("spec", "networkSpec", "vpc", "availabilityZones") + allErrs = append(allErrs, field.Invalid(availabilityZonesField, r.Spec.NetworkSpec.VPC.AvailabilityZoneSelection, "availabilityZones cannot be set if availabilityZoneUsageLimit and availabilityZoneSelection are set")) + } + return allErrs } @@ -452,6 +458,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 498a8b649d..29dafe1513 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 6daa99c9ca..2881d7f584 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("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 {