Skip to content

Commit

Permalink
Sorting LB security groups should prefer tagged security group
Browse files Browse the repository at this point in the history
  • Loading branch information
@JoelSpeed
JoelSpeed committed Aug 20, 2024
1 parent a84ea61 commit 912f047
Show file tree
Hide file tree
Showing 3 changed files with 94 additions and 32 deletions.
37 changes: 28 additions & 9 deletions pkg/providers/v1/aws.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
})
}

Expand Down Expand Up @@ -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
Expand All @@ -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).
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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`.
Expand All @@ -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]
Expand Down
38 changes: 19 additions & 19 deletions pkg/providers/v1/aws_fakes.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}

Expand Down
51 changes: 47 additions & 4 deletions pkg/providers/v1/aws_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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")},
},
},
})
}

Expand Down Expand Up @@ -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"}})
}
Expand All @@ -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{})
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
})
}
Expand Down

0 comments on commit 912f047

Please sign in to comment.