Skip to content

Commit

Permalink
Merge pull request #752 from JoelSpeed/remove-sg-rules-untagged-groups
Browse files Browse the repository at this point in the history 
Ensure removal of security group rules on deleting load balancers
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Aug 20, 2024
2 parents ec9bfe1 + 912f047 commit d7e05d5
Show file tree
Hide file tree
Showing 3 changed files with 144 additions and 67 deletions.
122 changes: 78 additions & 44 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 @@ -2498,7 +2506,7 @@ func (c *Cloud) EnsureLoadBalancer(ctx context.Context, clusterName string, apiS
}
}

err = c.updateInstanceSecurityGroupsForLoadBalancer(loadBalancer, instances, annotations)
err = c.updateInstanceSecurityGroupsForLoadBalancer(loadBalancer, instances, annotations, false)
if err != nil {
klog.Warningf("Error opening ingress rules for the load balancer to the instances: %q", err)
return nil, err
Expand Down Expand Up @@ -2659,7 +2667,7 @@ func (c *Cloud) getTaggedSecurityGroups() (map[string]*ec2.SecurityGroup, error)

// Open security group ingress rules on the instances so that the load balancer can talk to them
// Will also remove any security groups ingress rules for the load balancer that are _not_ needed for allInstances
func (c *Cloud) updateInstanceSecurityGroupsForLoadBalancer(lb *elb.LoadBalancerDescription, instances map[InstanceID]*ec2.Instance, annotations map[string]string) error {
func (c *Cloud) updateInstanceSecurityGroupsForLoadBalancer(lb *elb.LoadBalancerDescription, instances map[InstanceID]*ec2.Instance, annotations map[string]string, isDeleting bool) error {
if c.cfg.Global.DisableSecurityGroupIngress {
return nil
}
Expand All @@ -2669,11 +2677,24 @@ 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
var actualGroups []*ec2.SecurityGroup
actualGroups := make(map[*ec2.SecurityGroup]bool)
{
describeRequest := &ec2.DescribeSecurityGroupsInput{}
describeRequest.Filters = []*ec2.Filter{
Expand All @@ -2684,18 +2705,10 @@ func (c *Cloud) updateInstanceSecurityGroupsForLoadBalancer(lb *elb.LoadBalancer
return fmt.Errorf("error querying security groups for ELB: %q", err)
}
for _, sg := range response {
if !c.tagging.hasClusterTag(sg.Tags) {
continue
}
actualGroups = append(actualGroups, sg)
actualGroups[sg] = c.tagging.hasClusterTag(sg.Tags)
}
}

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 @@ -2725,7 +2738,7 @@ func (c *Cloud) updateInstanceSecurityGroupsForLoadBalancer(lb *elb.LoadBalancer
}

// Compare to actual groups
for _, actualGroup := range actualGroups {
for actualGroup, hasClusterTag := range actualGroups {
actualGroupID := aws.StringValue(actualGroup.GroupId)
if actualGroupID == "" {
klog.Warning("Ignoring group without ID: ", actualGroup)
Expand All @@ -2737,8 +2750,12 @@ func (c *Cloud) updateInstanceSecurityGroupsForLoadBalancer(lb *elb.LoadBalancer
// We don't need to make a change; the permission is already in place
delete(instanceSecurityGroupIds, actualGroupID)
} else {
// This group is not needed by allInstances; delete it
instanceSecurityGroupIds[actualGroupID] = false
if hasClusterTag || isDeleting {
// If the group is tagged, and we don't need the rule, we should remove it.
// If the security group is deleting, we should also remove the rule else
// we cannot remove the security group, we wiil get a dependency violation.
instanceSecurityGroupIds[actualGroupID] = false
}
}
}

Expand Down Expand Up @@ -2847,28 +2864,11 @@ func (c *Cloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName strin
return nil
}

{
// De-authorize the load balancer security group from the instances security group
err = c.updateInstanceSecurityGroupsForLoadBalancer(lb, nil, service.Annotations)
if err != nil {
klog.Errorf("Error deregistering load balancer from instance security groups: %q", err)
return err
}
}

{
// Delete the load balancer itself
request := &elb.DeleteLoadBalancerInput{}
request.LoadBalancerName = lb.LoadBalancerName

_, err = c.elb.DeleteLoadBalancer(request)
if err != nil {
// TODO: Check if error was because load balancer was concurrently deleted
klog.Errorf("Error deleting load balancer: %q", err)
return err
}
}

// Collect the security groups to delete.
// 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 All @@ -2884,9 +2884,6 @@ func (c *Cloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName strin
if err != nil {
return fmt.Errorf("error querying security groups for ELB: %q", err)
}

// Collect the security groups to delete
securityGroupIDs := map[string]struct{}{}
annotatedSgSet := map[string]bool{}
annotatedSgsList := getSGListFromAnnotation(service.Annotations[ServiceAnnotationLoadBalancerSecurityGroups])
annotatedExtraSgsList := getSGListFromAnnotation(service.Annotations[ServiceAnnotationLoadBalancerExtraSecurityGroups])
Expand All @@ -2911,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 @@ -2921,6 +2920,41 @@ func (c *Cloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName strin

securityGroupIDs[sgID] = struct{}{}
}
}

{
// Determine the load balancer security group id
lbSecurityGroupIDs := aws.StringValueSlice(lb.SecurityGroups)
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, taggedLBSecurityGroups)
loadBalancerSecurityGroupID := lbSecurityGroupIDs[0]

_, isDeleteingLBSecurityGroup := securityGroupIDs[loadBalancerSecurityGroupID]

// De-authorize the load balancer security group from the instances security group
err = c.updateInstanceSecurityGroupsForLoadBalancer(lb, nil, service.Annotations, isDeleteingLBSecurityGroup)
if err != nil {
klog.Errorf("Error deregistering load balancer from instance security groups: %q", err)
return err
}
}

{
// Delete the load balancer itself
request := &elb.DeleteLoadBalancerInput{}
request.LoadBalancerName = lb.LoadBalancerName

_, err = c.elb.DeleteLoadBalancer(request)
if err != nil {
// TODO: Check if error was because load balancer was concurrently deleted
klog.Errorf("Error deleting load balancer: %q", err)
return err
}
}

{

// Loop through and try to delete them
timeoutAt := time.Now().Add(time.Second * 600)
Expand Down Expand Up @@ -3017,7 +3051,7 @@ func (c *Cloud) UpdateLoadBalancer(ctx context.Context, clusterName string, serv
return err
}

err = c.updateInstanceSecurityGroupsForLoadBalancer(lb, instances, service.Annotations)
err = c.updateInstanceSecurityGroupsForLoadBalancer(lb, instances, service.Annotations, false)
if err != nil {
return err
}
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
Loading

0 comments on commit d7e05d5

Please sign in to comment.