Sorting LB security groups should prefer tagged security group

Author: JoelSpeed

pkg/providers/v1/aws.go

@@ -3869,12 +3869,16 @@ func (c *Cloud) buildELBSecurityGroupList(serviceName types.NamespacedName, load
 // from buildELBSecurityGroupList. The logic is:
 //   - securityGroups specified by ServiceAnnotationLoadBalancerSecurityGroups appears first in order
 //   - securityGroups specified by ServiceAnnotationLoadBalancerExtraSecurityGroups appears last in order
-func (c *Cloud) sortELBSecurityGroupList(securityGroupIDs []string, annotations map[string]string) {
+func (c *Cloud) sortELBSecurityGroupList(securityGroupIDs []string, annotations map[string]string, taggedLBSecurityGroups map[string]struct{}) {
 	annotatedSGList := getSGListFromAnnotation(annotations[ServiceAnnotationLoadBalancerSecurityGroups])
 	annotatedExtraSGList := getSGListFromAnnotation(annotations[ServiceAnnotationLoadBalancerExtraSecurityGroups])
 	annotatedSGIndex := make(map[string]int, len(annotatedSGList))
 	annotatedExtraSGIndex := make(map[string]int, len(annotatedExtraSGList))
 
+	if taggedLBSecurityGroups == nil {
+		taggedLBSecurityGroups = make(map[string]struct{})
+	}
+
 	for i, sgID := range annotatedSGList {
 		annotatedSGIndex[sgID] = i
 	}
@@ -3892,7 +3896,11 @@ func (c *Cloud) sortELBSecurityGroupList(securityGroupIDs []string, annotations
 		}
 	}
 	sort.Slice(securityGroupIDs, func(i, j int) bool {
-		return sgOrderMapping[securityGroupIDs[i]] < sgOrderMapping[securityGroupIDs[j]]
+		// If i is tagged but j is not, then i should be before j.
+		_, iTagged := taggedLBSecurityGroups[securityGroupIDs[i]]
+		_, jTagged := taggedLBSecurityGroups[securityGroupIDs[j]]
+
+		return sgOrderMapping[securityGroupIDs[i]] < sgOrderMapping[securityGroupIDs[j]] || iTagged && !jTagged
 	})
 }
 
@@ -4583,7 +4591,20 @@ func (c *Cloud) updateInstanceSecurityGroupsForLoadBalancer(lb *elb.LoadBalancer
 	if len(lbSecurityGroupIDs) == 0 {
 		return fmt.Errorf("could not determine security group for load balancer: %s", aws.StringValue(lb.LoadBalancerName))
 	}
-	c.sortELBSecurityGroupList(lbSecurityGroupIDs, annotations)
+
+	taggedSecurityGroups, err := c.getTaggedSecurityGroups()
+	if err != nil {
+		return fmt.Errorf("error querying for tagged security groups: %q", err)
+	}
+
+	taggedLBSecurityGroups := make(map[string]struct{})
+	for _, sg := range lbSecurityGroupIDs {
+		if _, ok := taggedSecurityGroups[sg]; ok {
+			taggedLBSecurityGroups[sg] = struct{}{}
+		}
+	}
+
+	c.sortELBSecurityGroupList(lbSecurityGroupIDs, annotations, taggedLBSecurityGroups)
 	loadBalancerSecurityGroupID := lbSecurityGroupIDs[0]
 
 	// Get the actual list of groups that allow ingress from the load-balancer
@@ -4602,11 +4623,6 @@ func (c *Cloud) updateInstanceSecurityGroupsForLoadBalancer(lb *elb.LoadBalancer
 		}
 	}
 
-	taggedSecurityGroups, err := c.getTaggedSecurityGroups()
-	if err != nil {
-		return fmt.Errorf("error querying for tagged security groups: %q", err)
-	}
-
 	// Open the firewall from the load balancer to the instance
 	// We don't actually have a trivial way to know in advance which security group the instance is in
 	// (it is probably the node security group, but we don't easily have that).
@@ -4766,6 +4782,7 @@ func (c *Cloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName strin
 	// We need to know this ahead of time so that we can check
 	// if the load balancer security group is being deleted.
 	securityGroupIDs := map[string]struct{}{}
+	taggedLBSecurityGroups := map[string]struct{}{}
 	{
 		// Delete the security group(s) for the load balancer
 		// Note that this is annoying: the load balancer disappears from the API immediately, but it is still
@@ -4805,6 +4822,8 @@ func (c *Cloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName strin
 			if !c.tagging.hasClusterTag(sg.Tags) {
 				klog.Warningf("Ignoring security group with no cluster tag in %s", service.Name)
 				continue
+			} else {
+				taggedLBSecurityGroups[sgID] = struct{}{}
 			}
 
 			// This is an extra protection of deletion of non provisioned Security Group which is annotated with `service.beta.kubernetes.io/aws-load-balancer-security-groups`.
@@ -4823,7 +4842,7 @@ func (c *Cloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName strin
 		if len(lbSecurityGroupIDs) == 0 {
 			return fmt.Errorf("could not determine security group for load balancer: %s", aws.StringValue(lb.LoadBalancerName))
 		}
-		c.sortELBSecurityGroupList(lbSecurityGroupIDs, service.Annotations)
+		c.sortELBSecurityGroupList(lbSecurityGroupIDs, service.Annotations, taggedLBSecurityGroups)
 		loadBalancerSecurityGroupID := lbSecurityGroupIDs[0]
 
 		_, isDeleteingLBSecurityGroup := securityGroupIDs[loadBalancerSecurityGroupID]

pkg/providers/v1/aws_fakes.go

@@ -450,8 +450,8 @@ func (elb *FakeELB) CreateLoadBalancer(*elb.CreateLoadBalancerInput) (*elb.Creat
 
 // DeleteLoadBalancer is not implemented but is required for interface
 // conformance
-func (elb *FakeELB) DeleteLoadBalancer(input *elb.DeleteLoadBalancerInput) (*elb.DeleteLoadBalancerOutput, error) {
-	panic("Not implemented")
+func (e *FakeELB) DeleteLoadBalancer(input *elb.DeleteLoadBalancerInput) (*elb.DeleteLoadBalancerOutput, error) {
+	return &elb.DeleteLoadBalancerOutput{}, nil
 }
 
 // DescribeLoadBalancers is not implemented but is required for interface

pkg/providers/v1/aws_test.go

@@ -66,6 +66,29 @@ func (m *MockedFakeEC2) expectDescribeSecurityGroups(clusterID, groupName string
 	}}).Return([]*ec2.SecurityGroup{{Tags: tags}})
 }
 
+func (m *MockedFakeEC2) expectDescribeSecurityGroupsAll(clusterID string) {
+	tags := []*ec2.Tag{
+		{Key: aws.String(TagNameKubernetesClusterLegacy), Value: aws.String(clusterID)},
+		{Key: aws.String(fmt.Sprintf("%s%s", TagNameKubernetesClusterPrefix, clusterID)), Value: aws.String(ResourceLifecycleOwned)},
+	}
+
+	m.On("DescribeSecurityGroups", &ec2.DescribeSecurityGroupsInput{}).Return([]*ec2.SecurityGroup{{
+		GroupId: aws.String("sg-123456"),
+		Tags:    tags,
+	}})
+}
+
+func (m *MockedFakeEC2) expectDescribeSecurityGroupsByFilter(clusterID, filterName string, filterValues ...string) {
+	tags := []*ec2.Tag{
+		{Key: aws.String(TagNameKubernetesClusterLegacy), Value: aws.String(clusterID)},
+		{Key: aws.String(fmt.Sprintf("%s%s", TagNameKubernetesClusterPrefix, clusterID)), Value: aws.String(ResourceLifecycleOwned)},
+	}
+
+	m.On("DescribeSecurityGroups", &ec2.DescribeSecurityGroupsInput{Filters: []*ec2.Filter{
+		newEc2Filter(filterName, filterValues...),
+	}}).Return([]*ec2.SecurityGroup{{Tags: tags}})
+}
+
 func (m *MockedFakeEC2) DescribeVolumes(request *ec2.DescribeVolumesInput) ([]*ec2.Volume, error) {
 	args := m.Called(request)
 	return args.Get(0).([]*ec2.Volume), nil
@@ -117,7 +140,11 @@ func (m *MockedFakeELB) DescribeLoadBalancers(input *elb.DescribeLoadBalancersIn
 
 func (m *MockedFakeELB) expectDescribeLoadBalancers(loadBalancerName string) {
 	m.On("DescribeLoadBalancers", &elb.DescribeLoadBalancersInput{LoadBalancerNames: []*string{aws.String(loadBalancerName)}}).Return(&elb.DescribeLoadBalancersOutput{
-		LoadBalancerDescriptions: []*elb.LoadBalancerDescription{{}},
+		LoadBalancerDescriptions: []*elb.LoadBalancerDescription{
+			{
+				SecurityGroups: []*string{aws.String("sg-123456")},
+			},
+		},
 	})
 }
 
@@ -1790,6 +1817,9 @@ func TestDescribeLoadBalancerOnDelete(t *testing.T) {
 	awsServices := newMockedFakeAWSServices(TestClusterID)
 	c, _ := newAWSCloud(CloudConfig{}, awsServices)
 	awsServices.elb.(*MockedFakeELB).expectDescribeLoadBalancers("aid")
+	awsServices.ec2.(*MockedFakeEC2).expectDescribeSecurityGroupsByFilter(TestClusterID, "group-id", "sg-123456")
+	awsServices.ec2.(*MockedFakeEC2).expectDescribeSecurityGroupsAll(TestClusterID)
+	awsServices.ec2.(*MockedFakeEC2).expectDescribeSecurityGroupsByFilter(TestClusterID, "ip-permission.group-id", "sg-123456")
 
 	c.EnsureLoadBalancerDeleted(context.TODO(), TestClusterName, &v1.Service{ObjectMeta: metav1.ObjectMeta{Name: "myservice", UID: "id"}})
 }
@@ -1798,6 +1828,8 @@ func TestDescribeLoadBalancerOnUpdate(t *testing.T) {
 	awsServices := newMockedFakeAWSServices(TestClusterID)
 	c, _ := newAWSCloud(CloudConfig{}, awsServices)
 	awsServices.elb.(*MockedFakeELB).expectDescribeLoadBalancers("aid")
+	awsServices.ec2.(*MockedFakeEC2).expectDescribeSecurityGroupsAll(TestClusterID)
+	awsServices.ec2.(*MockedFakeEC2).expectDescribeSecurityGroupsByFilter(TestClusterID, "ip-permission.group-id", "sg-123456")
 
 	c.UpdateLoadBalancer(context.TODO(), TestClusterName, &v1.Service{ObjectMeta: metav1.ObjectMeta{Name: "myservice", UID: "id"}}, []*v1.Node{})
 }
@@ -3344,8 +3376,9 @@ func TestAzToRegion(t *testing.T) {
 
 func TestCloud_sortELBSecurityGroupList(t *testing.T) {
 	type args struct {
-		securityGroupIDs []string
-		annotations      map[string]string
+		securityGroupIDs       []string
+		annotations            map[string]string
+		taggedLBSecurityGroups map[string]struct{}
 	}
 	tests := []struct {
 		name                 string
@@ -3391,11 +3424,21 @@ func TestCloud_sortELBSecurityGroupList(t *testing.T) {
 			},
 			wantSecurityGroupIDs: []string{"sg-3", "sg-2", "sg-1", "sg-4", "sg-6", "sg-5"},
 		},
+		{
+			name: "with an untagged, and unknown security group",
+			args: args{
+				securityGroupIDs: []string{"sg-2", "sg-1"},
+				taggedLBSecurityGroups: map[string]struct{}{
+					"sg-1": {},
+				},
+			},
+			wantSecurityGroupIDs: []string{"sg-1", "sg-2"},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			c := &Cloud{}
-			c.sortELBSecurityGroupList(tt.args.securityGroupIDs, tt.args.annotations)
+			c.sortELBSecurityGroupList(tt.args.securityGroupIDs, tt.args.annotations, tt.args.taggedLBSecurityGroups)
 			assert.Equal(t, tt.wantSecurityGroupIDs, tt.args.securityGroupIDs)
 		})
 	}