From 4ee1d5506e363b7e119aad9c038d0cd1d11b8771 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Mon, 27 Nov 2023 15:23:37 +0000 Subject: [PATCH] Sorting LB security groups should prefer tagged security group --- pkg/providers/v1/aws.go | 37 ++++++++++++++++++------- pkg/providers/v1/aws_fakes.go | 38 +++++++++++++------------- pkg/providers/v1/aws_test.go | 51 ++++++++++++++++++++++++++++++++--- 3 files changed, 94 insertions(+), 32 deletions(-) diff --git a/pkg/providers/v1/aws.go b/pkg/providers/v1/aws.go index f38bef3e36..17f8ee43d8 100644 --- a/pkg/providers/v1/aws.go +++ b/pkg/providers/v1/aws.go @@ -1946,12 +1946,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 } @@ -1969,7 +1973,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 }) } @@ -2669,7 +2677,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 @@ -2688,11 +2709,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). @@ -2852,6 +2868,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 @@ -2891,6 +2908,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`. @@ -2909,7 +2928,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] diff --git a/pkg/providers/v1/aws_fakes.go b/pkg/providers/v1/aws_fakes.go index ec090af069..2e4525d4bc 100644 --- a/pkg/providers/v1/aws_fakes.go +++ b/pkg/providers/v1/aws_fakes.go @@ -522,108 +522,108 @@ type FakeELB struct { // CreateLoadBalancer is not implemented but is required for interface // conformance -func (elb *FakeELB) CreateLoadBalancer(*elb.CreateLoadBalancerInput) (*elb.CreateLoadBalancerOutput, error) { +func (e *FakeELB) CreateLoadBalancer(*elb.CreateLoadBalancerInput) (*elb.CreateLoadBalancerOutput, error) { panic("Not implemented") } // 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 // conformance -func (elb *FakeELB) DescribeLoadBalancers(input *elb.DescribeLoadBalancersInput) (*elb.DescribeLoadBalancersOutput, error) { +func (e *FakeELB) DescribeLoadBalancers(input *elb.DescribeLoadBalancersInput) (*elb.DescribeLoadBalancersOutput, error) { panic("Not implemented") } // AddTags is not implemented but is required for interface conformance -func (elb *FakeELB) AddTags(input *elb.AddTagsInput) (*elb.AddTagsOutput, error) { +func (e *FakeELB) AddTags(input *elb.AddTagsInput) (*elb.AddTagsOutput, error) { panic("Not implemented") } // RegisterInstancesWithLoadBalancer is not implemented but is required for // interface conformance -func (elb *FakeELB) RegisterInstancesWithLoadBalancer(*elb.RegisterInstancesWithLoadBalancerInput) (*elb.RegisterInstancesWithLoadBalancerOutput, error) { +func (e *FakeELB) RegisterInstancesWithLoadBalancer(*elb.RegisterInstancesWithLoadBalancerInput) (*elb.RegisterInstancesWithLoadBalancerOutput, error) { panic("Not implemented") } // DeregisterInstancesFromLoadBalancer is not implemented but is required for // interface conformance -func (elb *FakeELB) DeregisterInstancesFromLoadBalancer(*elb.DeregisterInstancesFromLoadBalancerInput) (*elb.DeregisterInstancesFromLoadBalancerOutput, error) { +func (e *FakeELB) DeregisterInstancesFromLoadBalancer(*elb.DeregisterInstancesFromLoadBalancerInput) (*elb.DeregisterInstancesFromLoadBalancerOutput, error) { panic("Not implemented") } // DetachLoadBalancerFromSubnets is not implemented but is required for // interface conformance -func (elb *FakeELB) DetachLoadBalancerFromSubnets(*elb.DetachLoadBalancerFromSubnetsInput) (*elb.DetachLoadBalancerFromSubnetsOutput, error) { +func (e *FakeELB) DetachLoadBalancerFromSubnets(*elb.DetachLoadBalancerFromSubnetsInput) (*elb.DetachLoadBalancerFromSubnetsOutput, error) { panic("Not implemented") } // AttachLoadBalancerToSubnets is not implemented but is required for interface // conformance -func (elb *FakeELB) AttachLoadBalancerToSubnets(*elb.AttachLoadBalancerToSubnetsInput) (*elb.AttachLoadBalancerToSubnetsOutput, error) { +func (e *FakeELB) AttachLoadBalancerToSubnets(*elb.AttachLoadBalancerToSubnetsInput) (*elb.AttachLoadBalancerToSubnetsOutput, error) { panic("Not implemented") } // CreateLoadBalancerListeners is not implemented but is required for interface // conformance -func (elb *FakeELB) CreateLoadBalancerListeners(*elb.CreateLoadBalancerListenersInput) (*elb.CreateLoadBalancerListenersOutput, error) { +func (e *FakeELB) CreateLoadBalancerListeners(*elb.CreateLoadBalancerListenersInput) (*elb.CreateLoadBalancerListenersOutput, error) { panic("Not implemented") } // DeleteLoadBalancerListeners is not implemented but is required for interface // conformance -func (elb *FakeELB) DeleteLoadBalancerListeners(*elb.DeleteLoadBalancerListenersInput) (*elb.DeleteLoadBalancerListenersOutput, error) { +func (e *FakeELB) DeleteLoadBalancerListeners(*elb.DeleteLoadBalancerListenersInput) (*elb.DeleteLoadBalancerListenersOutput, error) { panic("Not implemented") } // ApplySecurityGroupsToLoadBalancer is not implemented but is required for // interface conformance -func (elb *FakeELB) ApplySecurityGroupsToLoadBalancer(*elb.ApplySecurityGroupsToLoadBalancerInput) (*elb.ApplySecurityGroupsToLoadBalancerOutput, error) { +func (e *FakeELB) ApplySecurityGroupsToLoadBalancer(*elb.ApplySecurityGroupsToLoadBalancerInput) (*elb.ApplySecurityGroupsToLoadBalancerOutput, error) { panic("Not implemented") } // ConfigureHealthCheck is not implemented but is required for interface // conformance -func (elb *FakeELB) ConfigureHealthCheck(*elb.ConfigureHealthCheckInput) (*elb.ConfigureHealthCheckOutput, error) { +func (e *FakeELB) ConfigureHealthCheck(*elb.ConfigureHealthCheckInput) (*elb.ConfigureHealthCheckOutput, error) { panic("Not implemented") } // CreateLoadBalancerPolicy is not implemented but is required for interface // conformance -func (elb *FakeELB) CreateLoadBalancerPolicy(*elb.CreateLoadBalancerPolicyInput) (*elb.CreateLoadBalancerPolicyOutput, error) { +func (e *FakeELB) CreateLoadBalancerPolicy(*elb.CreateLoadBalancerPolicyInput) (*elb.CreateLoadBalancerPolicyOutput, error) { panic("Not implemented") } // SetLoadBalancerPoliciesForBackendServer is not implemented but is required // for interface conformance -func (elb *FakeELB) SetLoadBalancerPoliciesForBackendServer(*elb.SetLoadBalancerPoliciesForBackendServerInput) (*elb.SetLoadBalancerPoliciesForBackendServerOutput, error) { +func (e *FakeELB) SetLoadBalancerPoliciesForBackendServer(*elb.SetLoadBalancerPoliciesForBackendServerInput) (*elb.SetLoadBalancerPoliciesForBackendServerOutput, error) { panic("Not implemented") } // SetLoadBalancerPoliciesOfListener is not implemented but is required for // interface conformance -func (elb *FakeELB) SetLoadBalancerPoliciesOfListener(input *elb.SetLoadBalancerPoliciesOfListenerInput) (*elb.SetLoadBalancerPoliciesOfListenerOutput, error) { +func (e *FakeELB) SetLoadBalancerPoliciesOfListener(input *elb.SetLoadBalancerPoliciesOfListenerInput) (*elb.SetLoadBalancerPoliciesOfListenerOutput, error) { panic("Not implemented") } // DescribeLoadBalancerPolicies is not implemented but is required for // interface conformance -func (elb *FakeELB) DescribeLoadBalancerPolicies(input *elb.DescribeLoadBalancerPoliciesInput) (*elb.DescribeLoadBalancerPoliciesOutput, error) { +func (e *FakeELB) DescribeLoadBalancerPolicies(input *elb.DescribeLoadBalancerPoliciesInput) (*elb.DescribeLoadBalancerPoliciesOutput, error) { panic("Not implemented") } // DescribeLoadBalancerAttributes is not implemented but is required for // interface conformance -func (elb *FakeELB) DescribeLoadBalancerAttributes(*elb.DescribeLoadBalancerAttributesInput) (*elb.DescribeLoadBalancerAttributesOutput, error) { +func (e *FakeELB) DescribeLoadBalancerAttributes(*elb.DescribeLoadBalancerAttributesInput) (*elb.DescribeLoadBalancerAttributesOutput, error) { panic("Not implemented") } // ModifyLoadBalancerAttributes is not implemented but is required for // interface conformance -func (elb *FakeELB) ModifyLoadBalancerAttributes(*elb.ModifyLoadBalancerAttributesInput) (*elb.ModifyLoadBalancerAttributesOutput, error) { +func (e *FakeELB) ModifyLoadBalancerAttributes(*elb.ModifyLoadBalancerAttributesInput) (*elb.ModifyLoadBalancerAttributesOutput, error) { panic("Not implemented") } diff --git a/pkg/providers/v1/aws_test.go b/pkg/providers/v1/aws_test.go index 577f5d72cf..5562dd20b5 100644 --- a/pkg/providers/v1/aws_test.go +++ b/pkg/providers/v1/aws_test.go @@ -67,6 +67,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) DescribeSecurityGroups(request *ec2.DescribeSecurityGroupsInput) ([]*ec2.SecurityGroup, error) { args := m.Called(request) return args.Get(0).([]*ec2.SecurityGroup), nil @@ -84,7 +107,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")}, + }, + }, }) } @@ -1647,6 +1674,9 @@ func TestDescribeLoadBalancerOnDelete(t *testing.T) { awsServices := newMockedFakeAWSServices(TestClusterID) c, _ := newAWSCloud(config.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"}}) } @@ -1655,6 +1685,8 @@ func TestDescribeLoadBalancerOnUpdate(t *testing.T) { awsServices := newMockedFakeAWSServices(TestClusterID) c, _ := newAWSCloud(config.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{}) } @@ -3121,8 +3153,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 @@ -3168,11 +3201,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) }) }