Skip to content

Commit 4ee1d55

Browse files
JoelSpeedJoelSpeed
committed
Sorting LB security groups should prefer tagged security group
1 parent 6cbff2d commit 4ee1d55

File tree

3 files changed

+94
-32
lines changed

3 files changed

+94
-32
lines changed

‎pkg/providers/v1/aws.go

+28-9
Original file line numberDiff line numberDiff line change
@@ -1946,12 +1946,16 @@ func (c *Cloud) buildELBSecurityGroupList(serviceName types.NamespacedName, load
19461946
// from buildELBSecurityGroupList. The logic is:
19471947
// - securityGroups specified by ServiceAnnotationLoadBalancerSecurityGroups appears first in order
19481948
// - securityGroups specified by ServiceAnnotationLoadBalancerExtraSecurityGroups appears last in order
1949-
func (c *Cloud) sortELBSecurityGroupList(securityGroupIDs []string, annotations map[string]string) {
1949+
func (c *Cloud) sortELBSecurityGroupList(securityGroupIDs []string, annotations map[string]string, taggedLBSecurityGroups map[string]struct{}) {
19501950
annotatedSGList := getSGListFromAnnotation(annotations[ServiceAnnotationLoadBalancerSecurityGroups])
19511951
annotatedExtraSGList := getSGListFromAnnotation(annotations[ServiceAnnotationLoadBalancerExtraSecurityGroups])
19521952
annotatedSGIndex := make(map[string]int, len(annotatedSGList))
19531953
annotatedExtraSGIndex := make(map[string]int, len(annotatedExtraSGList))
19541954

1955+
if taggedLBSecurityGroups == nil {
1956+
taggedLBSecurityGroups = make(map[string]struct{})
1957+
}
1958+
19551959
for i, sgID := range annotatedSGList {
19561960
annotatedSGIndex[sgID] = i
19571961
}
@@ -1969,7 +1973,11 @@ func (c *Cloud) sortELBSecurityGroupList(securityGroupIDs []string, annotations
19691973
}
19701974
}
19711975
sort.Slice(securityGroupIDs, func(i, j int) bool {
1972-
return sgOrderMapping[securityGroupIDs[i]] < sgOrderMapping[securityGroupIDs[j]]
1976+
// If i is tagged but j is not, then i should be before j.
1977+
_, iTagged := taggedLBSecurityGroups[securityGroupIDs[i]]
1978+
_, jTagged := taggedLBSecurityGroups[securityGroupIDs[j]]
1979+
1980+
return sgOrderMapping[securityGroupIDs[i]] < sgOrderMapping[securityGroupIDs[j]] || iTagged && !jTagged
19731981
})
19741982
}
19751983

@@ -2669,7 +2677,20 @@ func (c *Cloud) updateInstanceSecurityGroupsForLoadBalancer(lb *elb.LoadBalancer
26692677
if len(lbSecurityGroupIDs) == 0 {
26702678
return fmt.Errorf("could not determine security group for load balancer: %s", aws.StringValue(lb.LoadBalancerName))
26712679
}
2672-
c.sortELBSecurityGroupList(lbSecurityGroupIDs, annotations)
2680+
2681+
taggedSecurityGroups, err := c.getTaggedSecurityGroups()
2682+
if err != nil {
2683+
return fmt.Errorf("error querying for tagged security groups: %q", err)
2684+
}
2685+
2686+
taggedLBSecurityGroups := make(map[string]struct{})
2687+
for _, sg := range lbSecurityGroupIDs {
2688+
if _, ok := taggedSecurityGroups[sg]; ok {
2689+
taggedLBSecurityGroups[sg] = struct{}{}
2690+
}
2691+
}
2692+
2693+
c.sortELBSecurityGroupList(lbSecurityGroupIDs, annotations, taggedLBSecurityGroups)
26732694
loadBalancerSecurityGroupID := lbSecurityGroupIDs[0]
26742695

26752696
// Get the actual list of groups that allow ingress from the load-balancer
@@ -2688,11 +2709,6 @@ func (c *Cloud) updateInstanceSecurityGroupsForLoadBalancer(lb *elb.LoadBalancer
26882709
}
26892710
}
26902711

2691-
taggedSecurityGroups, err := c.getTaggedSecurityGroups()
2692-
if err != nil {
2693-
return fmt.Errorf("error querying for tagged security groups: %q", err)
2694-
}
2695-
26962712
// Open the firewall from the load balancer to the instance
26972713
// We don't actually have a trivial way to know in advance which security group the instance is in
26982714
// (it is probably the node security group, but we don't easily have that).
@@ -2852,6 +2868,7 @@ func (c *Cloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName strin
28522868
// We need to know this ahead of time so that we can check
28532869
// if the load balancer security group is being deleted.
28542870
securityGroupIDs := map[string]struct{}{}
2871+
taggedLBSecurityGroups := map[string]struct{}{}
28552872
{
28562873
// Delete the security group(s) for the load balancer
28572874
// Note that this is annoying: the load balancer disappears from the API immediately, but it is still
@@ -2891,6 +2908,8 @@ func (c *Cloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName strin
28912908
if !c.tagging.hasClusterTag(sg.Tags) {
28922909
klog.Warningf("Ignoring security group with no cluster tag in %s", service.Name)
28932910
continue
2911+
} else {
2912+
taggedLBSecurityGroups[sgID] = struct{}{}
28942913
}
28952914

28962915
// 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`.
@@ -2909,7 +2928,7 @@ func (c *Cloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName strin
29092928
if len(lbSecurityGroupIDs) == 0 {
29102929
return fmt.Errorf("could not determine security group for load balancer: %s", aws.StringValue(lb.LoadBalancerName))
29112930
}
2912-
c.sortELBSecurityGroupList(lbSecurityGroupIDs, service.Annotations)
2931+
c.sortELBSecurityGroupList(lbSecurityGroupIDs, service.Annotations, taggedLBSecurityGroups)
29132932
loadBalancerSecurityGroupID := lbSecurityGroupIDs[0]
29142933

29152934
_, isDeleteingLBSecurityGroup := securityGroupIDs[loadBalancerSecurityGroupID]

‎pkg/providers/v1/aws_fakes.go

+19-19
Original file line numberDiff line numberDiff line change
@@ -522,108 +522,108 @@ type FakeELB struct {
522522

523523
// CreateLoadBalancer is not implemented but is required for interface
524524
// conformance
525-
func (elb *FakeELB) CreateLoadBalancer(*elb.CreateLoadBalancerInput) (*elb.CreateLoadBalancerOutput, error) {
525+
func (e *FakeELB) CreateLoadBalancer(*elb.CreateLoadBalancerInput) (*elb.CreateLoadBalancerOutput, error) {
526526
panic("Not implemented")
527527
}
528528

529529
// DeleteLoadBalancer is not implemented but is required for interface
530530
// conformance
531-
func (elb *FakeELB) DeleteLoadBalancer(input *elb.DeleteLoadBalancerInput) (*elb.DeleteLoadBalancerOutput, error) {
532-
panic("Not implemented")
531+
func (e *FakeELB) DeleteLoadBalancer(input *elb.DeleteLoadBalancerInput) (*elb.DeleteLoadBalancerOutput, error) {
532+
return &elb.DeleteLoadBalancerOutput{}, nil
533533
}
534534

535535
// DescribeLoadBalancers is not implemented but is required for interface
536536
// conformance
537-
func (elb *FakeELB) DescribeLoadBalancers(input *elb.DescribeLoadBalancersInput) (*elb.DescribeLoadBalancersOutput, error) {
537+
func (e *FakeELB) DescribeLoadBalancers(input *elb.DescribeLoadBalancersInput) (*elb.DescribeLoadBalancersOutput, error) {
538538
panic("Not implemented")
539539
}
540540

541541
// AddTags is not implemented but is required for interface conformance
542-
func (elb *FakeELB) AddTags(input *elb.AddTagsInput) (*elb.AddTagsOutput, error) {
542+
func (e *FakeELB) AddTags(input *elb.AddTagsInput) (*elb.AddTagsOutput, error) {
543543
panic("Not implemented")
544544
}
545545

546546
// RegisterInstancesWithLoadBalancer is not implemented but is required for
547547
// interface conformance
548-
func (elb *FakeELB) RegisterInstancesWithLoadBalancer(*elb.RegisterInstancesWithLoadBalancerInput) (*elb.RegisterInstancesWithLoadBalancerOutput, error) {
548+
func (e *FakeELB) RegisterInstancesWithLoadBalancer(*elb.RegisterInstancesWithLoadBalancerInput) (*elb.RegisterInstancesWithLoadBalancerOutput, error) {
549549
panic("Not implemented")
550550
}
551551

552552
// DeregisterInstancesFromLoadBalancer is not implemented but is required for
553553
// interface conformance
554-
func (elb *FakeELB) DeregisterInstancesFromLoadBalancer(*elb.DeregisterInstancesFromLoadBalancerInput) (*elb.DeregisterInstancesFromLoadBalancerOutput, error) {
554+
func (e *FakeELB) DeregisterInstancesFromLoadBalancer(*elb.DeregisterInstancesFromLoadBalancerInput) (*elb.DeregisterInstancesFromLoadBalancerOutput, error) {
555555
panic("Not implemented")
556556
}
557557

558558
// DetachLoadBalancerFromSubnets is not implemented but is required for
559559
// interface conformance
560-
func (elb *FakeELB) DetachLoadBalancerFromSubnets(*elb.DetachLoadBalancerFromSubnetsInput) (*elb.DetachLoadBalancerFromSubnetsOutput, error) {
560+
func (e *FakeELB) DetachLoadBalancerFromSubnets(*elb.DetachLoadBalancerFromSubnetsInput) (*elb.DetachLoadBalancerFromSubnetsOutput, error) {
561561
panic("Not implemented")
562562
}
563563

564564
// AttachLoadBalancerToSubnets is not implemented but is required for interface
565565
// conformance
566-
func (elb *FakeELB) AttachLoadBalancerToSubnets(*elb.AttachLoadBalancerToSubnetsInput) (*elb.AttachLoadBalancerToSubnetsOutput, error) {
566+
func (e *FakeELB) AttachLoadBalancerToSubnets(*elb.AttachLoadBalancerToSubnetsInput) (*elb.AttachLoadBalancerToSubnetsOutput, error) {
567567
panic("Not implemented")
568568
}
569569

570570
// CreateLoadBalancerListeners is not implemented but is required for interface
571571
// conformance
572-
func (elb *FakeELB) CreateLoadBalancerListeners(*elb.CreateLoadBalancerListenersInput) (*elb.CreateLoadBalancerListenersOutput, error) {
572+
func (e *FakeELB) CreateLoadBalancerListeners(*elb.CreateLoadBalancerListenersInput) (*elb.CreateLoadBalancerListenersOutput, error) {
573573
panic("Not implemented")
574574
}
575575

576576
// DeleteLoadBalancerListeners is not implemented but is required for interface
577577
// conformance
578-
func (elb *FakeELB) DeleteLoadBalancerListeners(*elb.DeleteLoadBalancerListenersInput) (*elb.DeleteLoadBalancerListenersOutput, error) {
578+
func (e *FakeELB) DeleteLoadBalancerListeners(*elb.DeleteLoadBalancerListenersInput) (*elb.DeleteLoadBalancerListenersOutput, error) {
579579
panic("Not implemented")
580580
}
581581

582582
// ApplySecurityGroupsToLoadBalancer is not implemented but is required for
583583
// interface conformance
584-
func (elb *FakeELB) ApplySecurityGroupsToLoadBalancer(*elb.ApplySecurityGroupsToLoadBalancerInput) (*elb.ApplySecurityGroupsToLoadBalancerOutput, error) {
584+
func (e *FakeELB) ApplySecurityGroupsToLoadBalancer(*elb.ApplySecurityGroupsToLoadBalancerInput) (*elb.ApplySecurityGroupsToLoadBalancerOutput, error) {
585585
panic("Not implemented")
586586
}
587587

588588
// ConfigureHealthCheck is not implemented but is required for interface
589589
// conformance
590-
func (elb *FakeELB) ConfigureHealthCheck(*elb.ConfigureHealthCheckInput) (*elb.ConfigureHealthCheckOutput, error) {
590+
func (e *FakeELB) ConfigureHealthCheck(*elb.ConfigureHealthCheckInput) (*elb.ConfigureHealthCheckOutput, error) {
591591
panic("Not implemented")
592592
}
593593

594594
// CreateLoadBalancerPolicy is not implemented but is required for interface
595595
// conformance
596-
func (elb *FakeELB) CreateLoadBalancerPolicy(*elb.CreateLoadBalancerPolicyInput) (*elb.CreateLoadBalancerPolicyOutput, error) {
596+
func (e *FakeELB) CreateLoadBalancerPolicy(*elb.CreateLoadBalancerPolicyInput) (*elb.CreateLoadBalancerPolicyOutput, error) {
597597
panic("Not implemented")
598598
}
599599

600600
// SetLoadBalancerPoliciesForBackendServer is not implemented but is required
601601
// for interface conformance
602-
func (elb *FakeELB) SetLoadBalancerPoliciesForBackendServer(*elb.SetLoadBalancerPoliciesForBackendServerInput) (*elb.SetLoadBalancerPoliciesForBackendServerOutput, error) {
602+
func (e *FakeELB) SetLoadBalancerPoliciesForBackendServer(*elb.SetLoadBalancerPoliciesForBackendServerInput) (*elb.SetLoadBalancerPoliciesForBackendServerOutput, error) {
603603
panic("Not implemented")
604604
}
605605

606606
// SetLoadBalancerPoliciesOfListener is not implemented but is required for
607607
// interface conformance
608-
func (elb *FakeELB) SetLoadBalancerPoliciesOfListener(input *elb.SetLoadBalancerPoliciesOfListenerInput) (*elb.SetLoadBalancerPoliciesOfListenerOutput, error) {
608+
func (e *FakeELB) SetLoadBalancerPoliciesOfListener(input *elb.SetLoadBalancerPoliciesOfListenerInput) (*elb.SetLoadBalancerPoliciesOfListenerOutput, error) {
609609
panic("Not implemented")
610610
}
611611

612612
// DescribeLoadBalancerPolicies is not implemented but is required for
613613
// interface conformance
614-
func (elb *FakeELB) DescribeLoadBalancerPolicies(input *elb.DescribeLoadBalancerPoliciesInput) (*elb.DescribeLoadBalancerPoliciesOutput, error) {
614+
func (e *FakeELB) DescribeLoadBalancerPolicies(input *elb.DescribeLoadBalancerPoliciesInput) (*elb.DescribeLoadBalancerPoliciesOutput, error) {
615615
panic("Not implemented")
616616
}
617617

618618
// DescribeLoadBalancerAttributes is not implemented but is required for
619619
// interface conformance
620-
func (elb *FakeELB) DescribeLoadBalancerAttributes(*elb.DescribeLoadBalancerAttributesInput) (*elb.DescribeLoadBalancerAttributesOutput, error) {
620+
func (e *FakeELB) DescribeLoadBalancerAttributes(*elb.DescribeLoadBalancerAttributesInput) (*elb.DescribeLoadBalancerAttributesOutput, error) {
621621
panic("Not implemented")
622622
}
623623

624624
// ModifyLoadBalancerAttributes is not implemented but is required for
625625
// interface conformance
626-
func (elb *FakeELB) ModifyLoadBalancerAttributes(*elb.ModifyLoadBalancerAttributesInput) (*elb.ModifyLoadBalancerAttributesOutput, error) {
626+
func (e *FakeELB) ModifyLoadBalancerAttributes(*elb.ModifyLoadBalancerAttributesInput) (*elb.ModifyLoadBalancerAttributesOutput, error) {
627627
panic("Not implemented")
628628
}
629629

‎pkg/providers/v1/aws_test.go

+47-4
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,29 @@ func (m *MockedFakeEC2) expectDescribeSecurityGroups(clusterID, groupName string
6767
}}).Return([]*ec2.SecurityGroup{{Tags: tags}})
6868
}
6969

70+
func (m *MockedFakeEC2) expectDescribeSecurityGroupsAll(clusterID string) {
71+
tags := []*ec2.Tag{
72+
{Key: aws.String(TagNameKubernetesClusterLegacy), Value: aws.String(clusterID)},
73+
{Key: aws.String(fmt.Sprintf("%s%s", TagNameKubernetesClusterPrefix, clusterID)), Value: aws.String(ResourceLifecycleOwned)},
74+
}
75+
76+
m.On("DescribeSecurityGroups", &ec2.DescribeSecurityGroupsInput{}).Return([]*ec2.SecurityGroup{{
77+
GroupId: aws.String("sg-123456"),
78+
Tags: tags,
79+
}})
80+
}
81+
82+
func (m *MockedFakeEC2) expectDescribeSecurityGroupsByFilter(clusterID, filterName string, filterValues ...string) {
83+
tags := []*ec2.Tag{
84+
{Key: aws.String(TagNameKubernetesClusterLegacy), Value: aws.String(clusterID)},
85+
{Key: aws.String(fmt.Sprintf("%s%s", TagNameKubernetesClusterPrefix, clusterID)), Value: aws.String(ResourceLifecycleOwned)},
86+
}
87+
88+
m.On("DescribeSecurityGroups", &ec2.DescribeSecurityGroupsInput{Filters: []*ec2.Filter{
89+
newEc2Filter(filterName, filterValues...),
90+
}}).Return([]*ec2.SecurityGroup{{Tags: tags}})
91+
}
92+
7093
func (m *MockedFakeEC2) DescribeSecurityGroups(request *ec2.DescribeSecurityGroupsInput) ([]*ec2.SecurityGroup, error) {
7194
args := m.Called(request)
7295
return args.Get(0).([]*ec2.SecurityGroup), nil
@@ -84,7 +107,11 @@ func (m *MockedFakeELB) DescribeLoadBalancers(input *elb.DescribeLoadBalancersIn
84107

85108
func (m *MockedFakeELB) expectDescribeLoadBalancers(loadBalancerName string) {
86109
m.On("DescribeLoadBalancers", &elb.DescribeLoadBalancersInput{LoadBalancerNames: []*string{aws.String(loadBalancerName)}}).Return(&elb.DescribeLoadBalancersOutput{
87-
LoadBalancerDescriptions: []*elb.LoadBalancerDescription{{}},
110+
LoadBalancerDescriptions: []*elb.LoadBalancerDescription{
111+
{
112+
SecurityGroups: []*string{aws.String("sg-123456")},
113+
},
114+
},
88115
})
89116
}
90117

@@ -1647,6 +1674,9 @@ func TestDescribeLoadBalancerOnDelete(t *testing.T) {
16471674
awsServices := newMockedFakeAWSServices(TestClusterID)
16481675
c, _ := newAWSCloud(config.CloudConfig{}, awsServices)
16491676
awsServices.elb.(*MockedFakeELB).expectDescribeLoadBalancers("aid")
1677+
awsServices.ec2.(*MockedFakeEC2).expectDescribeSecurityGroupsByFilter(TestClusterID, "group-id", "sg-123456")
1678+
awsServices.ec2.(*MockedFakeEC2).expectDescribeSecurityGroupsAll(TestClusterID)
1679+
awsServices.ec2.(*MockedFakeEC2).expectDescribeSecurityGroupsByFilter(TestClusterID, "ip-permission.group-id", "sg-123456")
16501680

16511681
c.EnsureLoadBalancerDeleted(context.TODO(), TestClusterName, &v1.Service{ObjectMeta: metav1.ObjectMeta{Name: "myservice", UID: "id"}})
16521682
}
@@ -1655,6 +1685,8 @@ func TestDescribeLoadBalancerOnUpdate(t *testing.T) {
16551685
awsServices := newMockedFakeAWSServices(TestClusterID)
16561686
c, _ := newAWSCloud(config.CloudConfig{}, awsServices)
16571687
awsServices.elb.(*MockedFakeELB).expectDescribeLoadBalancers("aid")
1688+
awsServices.ec2.(*MockedFakeEC2).expectDescribeSecurityGroupsAll(TestClusterID)
1689+
awsServices.ec2.(*MockedFakeEC2).expectDescribeSecurityGroupsByFilter(TestClusterID, "ip-permission.group-id", "sg-123456")
16581690

16591691
c.UpdateLoadBalancer(context.TODO(), TestClusterName, &v1.Service{ObjectMeta: metav1.ObjectMeta{Name: "myservice", UID: "id"}}, []*v1.Node{})
16601692
}
@@ -3121,8 +3153,9 @@ func TestAzToRegion(t *testing.T) {
31213153

31223154
func TestCloud_sortELBSecurityGroupList(t *testing.T) {
31233155
type args struct {
3124-
securityGroupIDs []string
3125-
annotations map[string]string
3156+
securityGroupIDs []string
3157+
annotations map[string]string
3158+
taggedLBSecurityGroups map[string]struct{}
31263159
}
31273160
tests := []struct {
31283161
name string
@@ -3168,11 +3201,21 @@ func TestCloud_sortELBSecurityGroupList(t *testing.T) {
31683201
},
31693202
wantSecurityGroupIDs: []string{"sg-3", "sg-2", "sg-1", "sg-4", "sg-6", "sg-5"},
31703203
},
3204+
{
3205+
name: "with an untagged, and unknown security group",
3206+
args: args{
3207+
securityGroupIDs: []string{"sg-2", "sg-1"},
3208+
taggedLBSecurityGroups: map[string]struct{}{
3209+
"sg-1": {},
3210+
},
3211+
},
3212+
wantSecurityGroupIDs: []string{"sg-1", "sg-2"},
3213+
},
31713214
}
31723215
for _, tt := range tests {
31733216
t.Run(tt.name, func(t *testing.T) {
31743217
c := &Cloud{}
3175-
c.sortELBSecurityGroupList(tt.args.securityGroupIDs, tt.args.annotations)
3218+
c.sortELBSecurityGroupList(tt.args.securityGroupIDs, tt.args.annotations, tt.args.taggedLBSecurityGroups)
31763219
assert.Equal(t, tt.wantSecurityGroupIDs, tt.args.securityGroupIDs)
31773220
})
31783221
}

0 commit comments

Comments
 (0)