From 90c695083e7f31a0cf488f24fb5f86def9a7fa6c Mon Sep 17 00:00:00 2001 From: Olivia Song Date: Thu, 7 Dec 2023 17:45:50 -0800 Subject: [PATCH 1/2] use subnets of ensured LBs for update worker node SG rules --- pkg/providers/v1/aws.go | 22 ++++++++++++++++++---- pkg/providers/v1/aws_loadbalancer.go | 8 ++++---- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/pkg/providers/v1/aws.go b/pkg/providers/v1/aws.go index 73310f6a23..1642661ae3 100644 --- a/pkg/providers/v1/aws.go +++ b/pkg/providers/v1/aws.go @@ -4100,13 +4100,13 @@ func (c *Cloud) EnsureLoadBalancer(ctx context.Context, clusterName string, apiS if isNLB(annotations) { // Find the subnets that the ELB will live in - subnetIDs, err := c.getLoadBalancerSubnets(apiService, internalELB) + discoveredSubnetIDs, err := c.getLoadBalancerSubnets(apiService, internalELB) if err != nil { klog.Errorf("Error listing subnets in VPC: %q", err) return nil, err } // Bail out early if there are no subnets - if len(subnetIDs) == 0 { + if len(discoveredSubnetIDs) == 0 { return nil, fmt.Errorf("could not find any suitable subnets for creating the ELB") } @@ -4123,7 +4123,7 @@ func (c *Cloud) EnsureLoadBalancer(ctx context.Context, clusterName string, apiS loadBalancerName, v2Mappings, instanceIDs, - subnetIDs, + discoveredSubnetIDs, internalELB, annotations, ) @@ -4131,7 +4131,21 @@ func (c *Cloud) EnsureLoadBalancer(ctx context.Context, clusterName string, apiS return nil, err } - subnetCidrs, err := c.getSubnetCidrs(subnetIDs) + // try to get the existing subnets of existing LBs from AZs + var ensuredSubnetIDs []string + var subnetCidrs []string + for _, az := range v2LoadBalancer.AvailabilityZones { + ensuredSubnetIDs = append(ensuredSubnetIDs, *az.SubnetId) + } + // for newly created LBs, the ensured subnets are nil since the loadbalancer.AvailabilityZones are none + // we use discovered subnets in this case + if len(ensuredSubnetIDs) == 0 { + klog.Infof("did not find existing subnets on LB %s, use discovered subnets %d", loadBalancerName, discoveredSubnetIDs) + subnetCidrs, err = c.getSubnetCidrs(discoveredSubnetIDs) + } else { + klog.Infof("use exising subnets on LB %s, subnet IDs: %d", loadBalancerName, ensuredSubnetIDs) + subnetCidrs, err = c.getSubnetCidrs(ensuredSubnetIDs) + } if err != nil { klog.Errorf("Error getting subnet cidrs: %q", err) return nil, err diff --git a/pkg/providers/v1/aws_loadbalancer.go b/pkg/providers/v1/aws_loadbalancer.go index 964a47bb37..ce61350637 100644 --- a/pkg/providers/v1/aws_loadbalancer.go +++ b/pkg/providers/v1/aws_loadbalancer.go @@ -138,7 +138,7 @@ func getKeyValuePropertiesFromAnnotation(annotations map[string]string, annotati } // ensureLoadBalancerv2 ensures a v2 load balancer is created -func (c *Cloud) ensureLoadBalancerv2(namespacedName types.NamespacedName, loadBalancerName string, mappings []nlbPortMapping, instanceIDs, subnetIDs []string, internalELB bool, annotations map[string]string) (*elbv2.LoadBalancer, error) { +func (c *Cloud) ensureLoadBalancerv2(namespacedName types.NamespacedName, loadBalancerName string, mappings []nlbPortMapping, instanceIDs, discoveredSubnetIDs []string, internalELB bool, annotations map[string]string) (*elbv2.LoadBalancer, error) { loadBalancer, err := c.describeLoadBalancerv2(loadBalancerName) if err != nil { return nil, err @@ -165,14 +165,14 @@ func (c *Cloud) ensureLoadBalancerv2(namespacedName types.NamespacedName, loadBa var allocationIDs []string if eipList, present := annotations[ServiceAnnotationLoadBalancerEIPAllocations]; present { allocationIDs = strings.Split(eipList, ",") - if len(allocationIDs) != len(subnetIDs) { - return nil, fmt.Errorf("error creating load balancer: Must have same number of EIP AllocationIDs (%d) and SubnetIDs (%d)", len(allocationIDs), len(subnetIDs)) + if len(allocationIDs) != len(discoveredSubnetIDs) { + return nil, fmt.Errorf("error creating load balancer: Must have same number of EIP AllocationIDs (%d) and SubnetIDs (%d)", len(allocationIDs), len(discoveredSubnetIDs)) } } // We are supposed to specify one subnet per AZ. // TODO: What happens if we have more than one subnet per AZ? - createRequest.SubnetMappings = createSubnetMappings(subnetIDs, allocationIDs) + createRequest.SubnetMappings = createSubnetMappings(discoveredSubnetIDs, allocationIDs) for k, v := range tags { createRequest.Tags = append(createRequest.Tags, &elbv2.Tag{ From 18b861bf176084fc0ec7eab7450aa7360fc85458 Mon Sep 17 00:00:00 2001 From: Olivia Song Date: Fri, 15 Dec 2023 00:39:20 -0800 Subject: [PATCH 2/2] remove ensureSubnets check and update unit test to add AZ info --- pkg/providers/v1/aws.go | 11 +++-------- pkg/providers/v1/aws_test.go | 6 ++++++ 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/pkg/providers/v1/aws.go b/pkg/providers/v1/aws.go index 1642661ae3..19890348f5 100644 --- a/pkg/providers/v1/aws.go +++ b/pkg/providers/v1/aws.go @@ -4131,21 +4131,16 @@ func (c *Cloud) EnsureLoadBalancer(ctx context.Context, clusterName string, apiS return nil, err } - // try to get the existing subnets of existing LBs from AZs + // try to get the ensured subnets of the LBs from AZs var ensuredSubnetIDs []string var subnetCidrs []string for _, az := range v2LoadBalancer.AvailabilityZones { ensuredSubnetIDs = append(ensuredSubnetIDs, *az.SubnetId) } - // for newly created LBs, the ensured subnets are nil since the loadbalancer.AvailabilityZones are none - // we use discovered subnets in this case if len(ensuredSubnetIDs) == 0 { - klog.Infof("did not find existing subnets on LB %s, use discovered subnets %d", loadBalancerName, discoveredSubnetIDs) - subnetCidrs, err = c.getSubnetCidrs(discoveredSubnetIDs) - } else { - klog.Infof("use exising subnets on LB %s, subnet IDs: %d", loadBalancerName, ensuredSubnetIDs) - subnetCidrs, err = c.getSubnetCidrs(ensuredSubnetIDs) + return nil, fmt.Errorf("did not find ensured subnets on LB %s", loadBalancerName) } + subnetCidrs, err = c.getSubnetCidrs(ensuredSubnetIDs) if err != nil { klog.Errorf("Error getting subnet cidrs: %q", err) return nil, err diff --git a/pkg/providers/v1/aws_test.go b/pkg/providers/v1/aws_test.go index 40b5e4a837..94717b9187 100644 --- a/pkg/providers/v1/aws_test.go +++ b/pkg/providers/v1/aws_test.go @@ -2776,6 +2776,12 @@ func (m *MockedFakeELBV2) CreateLoadBalancer(request *elbv2.CreateLoadBalancerIn LoadBalancerName: request.Name, Type: aws.String(elbv2.LoadBalancerTypeEnumNetwork), VpcId: aws.String("vpc-abc123def456abc78"), + AvailabilityZones: []*elbv2.AvailabilityZone{ + { + ZoneName: aws.String("us-west-2a"), + SubnetId: aws.String("subnet-abc123de"), + }, + }, } m.LoadBalancers = append(m.LoadBalancers, newLB)