Merge pull request #857 from M00nF1sh/fix-sg

fix issue 838

Author: k8s-ci-robot

internal/alb/sg/association.go

@@ -237,7 +237,7 @@ func (c *associationController) ensureSGInstance(ctx context.Context, groupName
 
 func (c *associationController) deleteSGInstance(ctx context.Context, instance *ec2.SecurityGroup) error {
 	albctx.GetLogger(ctx).Infof("deleting securityGroup %v:%v", aws.StringValue(instance.GroupName), aws.StringValue(instance.Description))
-	return c.cloud.DeleteSecurityGroupByID(aws.StringValue(instance.GroupId))
+	return c.cloud.DeleteSecurityGroupByID(ctx, aws.StringValue(instance.GroupId))
 }
 
 func (c *associationController) buildAssociationConfig(ctx context.Context, ingress *extensions.Ingress) (associationConfig, error) {

internal/alb/sg/instance_attachment.go

@@ -5,6 +5,8 @@ import (
 	"fmt"
 	"sync"
 
+	"k8s.io/apimachinery/pkg/util/sets"
+
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/aws/aws-sdk-go/service/elbv2"
 	"github.com/kubernetes-sigs/aws-alb-ingress-controller/internal/alb/tg"
@@ -37,38 +39,46 @@ func (controller *instanceAttachmentController) Reconcile(ctx context.Context, g
 	if err != nil {
 		return fmt.Errorf("failed to get cluster ENIs due to %v", err)
 	}
+
+	// sgAttachedENIs is not always an subnet of instanceENIs :(, e.g. this CNI bug: https://github.com/aws/amazon-vpc-cni-k8s/issues/69
+	sgAttachedENIs, err := controller.getSGAttachedENIs(ctx, groupID)
+	if err != nil {
+		return fmt.Errorf("failed to get ENIs attached to %s due to %v", groupID, err)
+	}
+
 	supportingENIs := controller.findENIsSupportingTargets(instanceENIs, tgGroup)
 	for _, enis := range instanceENIs {
 		for _, eni := range enis {
-			if _, ok := supportingENIs[aws.StringValue(eni.NetworkInterfaceId)]; ok {
-				err := controller.ensureSGAttachedToENI(ctx, groupID, eni)
-				if err != nil {
-					return err
-				}
-			} else {
-				err := controller.ensureSGDetachedFromENI(ctx, groupID, eni)
-				if err != nil {
+			if supportingENIs.Has(aws.StringValue(eni.NetworkInterfaceId)) {
+				if err := controller.ensureSGAttachedToENI(ctx, groupID, eni); err != nil {
 					return err
 				}
 			}
 		}
 	}
+
+	for _, eni := range sgAttachedENIs {
+		if !supportingENIs.Has(aws.StringValue(eni.NetworkInterfaceId)) {
+			if err := controller.ensureSGDetachedFromENI(ctx, groupID, eni); err != nil {
+				return err
+			}
+		}
+	}
+
 	return nil
 }
 
 func (controller *instanceAttachmentController) Delete(ctx context.Context, groupID string) error {
 	clusterInstanceENILock.Lock()
 	defer clusterInstanceENILock.Unlock()
-	instanceENIs, err := controller.getClusterInstanceENIs()
+
+	sgAttachedENIs, err := controller.getSGAttachedENIs(ctx, groupID)
 	if err != nil {
-		return fmt.Errorf("failed to get cluster enis due to %v", err)
+		return fmt.Errorf("failed to get ENIs attached to %s due to %v", groupID, err)
 	}
-	for _, enis := range instanceENIs {
-		for _, eni := range enis {
-			err := controller.ensureSGDetachedFromENI(ctx, groupID, eni)
-			if err != nil {
-				return err
-			}
+	for _, eni := range sgAttachedENIs {
+		if err := controller.ensureSGDetachedFromENI(ctx, groupID, eni); err != nil {
+			return err
 		}
 	}
 	return nil
@@ -92,7 +102,7 @@ func (controller *instanceAttachmentController) ensureSGAttachedToENI(ctx contex
 	return err
 }
 
-func (controller *instanceAttachmentController) ensureSGDetachedFromENI(ctx context.Context, sgID string, eni *ec2.InstanceNetworkInterface) error {
+func (controller *instanceAttachmentController) ensureSGDetachedFromENI(ctx context.Context, sgID string, eni *ec2.NetworkInterface) error {
 	sgAttached := false
 	desiredGroups := []string{}
 	for _, group := range eni.Groups {
@@ -116,17 +126,15 @@ func (controller *instanceAttachmentController) ensureSGDetachedFromENI(ctx cont
 }
 
 // findENIsSupportingTargets find the ID of ENIs that are used to supporting ingress traffic to targets
-func (controller *instanceAttachmentController) findENIsSupportingTargets(instanceENIs map[string][]*ec2.InstanceNetworkInterface, tgGroup tg.TargetGroupGroup) map[string]bool {
-	result := make(map[string]bool)
+func (controller *instanceAttachmentController) findENIsSupportingTargets(instanceENIs map[string][]*ec2.InstanceNetworkInterface, tgGroup tg.TargetGroupGroup) sets.String {
+	result := sets.NewString()
 	for _, tg := range tgGroup.TGByBackend {
 		if tg.TargetType == elbv2.TargetTypeEnumInstance {
-			for _, eniID := range controller.findENIsSupportingTargetGroupOfTypeInstance(instanceENIs, tg) {
-				result[eniID] = true
-			}
+			eniIDs := controller.findENIsSupportingTargetGroupOfTypeInstance(instanceENIs, tg)
+			result.Insert(eniIDs...)
 		} else {
-			for _, eniID := range controller.findENIsSupportingTargetGroupOfTypeIP(instanceENIs, tg) {
-				result[eniID] = true
-			}
+			eniIDs := controller.findENIsSupportingTargetGroupOfTypeIP(instanceENIs, tg)
+			result.Insert(eniIDs...)
 		}
 	}
 	return result
@@ -173,7 +181,7 @@ func (controller *instanceAttachmentController) findENIsSupportingTargetGroupOfT
 	return result
 }
 
-// getClusterInstanceENIs retrives all ENIs attached to instances indexed by instanceID
+// getClusterInstanceENIs retrieves all ENIs attached to instances indexed by instanceID
 func (controller *instanceAttachmentController) getClusterInstanceENIs() (map[string][]*ec2.InstanceNetworkInterface, error) {
 	instanceIDs, err := controller.store.GetClusterInstanceIDs()
 	if err != nil {
@@ -189,3 +197,15 @@ func (controller *instanceAttachmentController) getClusterInstanceENIs() (map[st
 	}
 	return result, nil
 }
+
+// getSGAttachedENIs retrieves all ENIs attached with specified securityGroup.
+func (controller *instanceAttachmentController) getSGAttachedENIs(ctx context.Context, sgID string) ([]*ec2.NetworkInterface, error) {
+	return controller.cloud.DescribeNetworkInterfaces(ctx, &ec2.DescribeNetworkInterfacesInput{
+		Filters: []*ec2.Filter{
+			{
+				Name:   aws.String("group-id"),
+				Values: []*string{aws.String(sgID)},
+			},
+		},
+	})
+}

internal/aws/ec2.go

@@ -4,9 +4,12 @@ import (
 	"context"
 	"fmt"
 	"strings"
+	"time"
 
-	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/aws/awserr"
+	"k8s.io/apimachinery/pkg/util/wait"
+
+	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/aws/request"
 
 	"github.com/aws/aws-sdk-go/service/ec2"
@@ -42,7 +45,10 @@ type EC2API interface {
 	GetSecurityGroupsByName(context.Context, []string) ([]*ec2.SecurityGroup, error)
 
 	// DeleteSecurityGroupByID delete securityGroup by securityGroupID
-	DeleteSecurityGroupByID(string) error
+	DeleteSecurityGroupByID(context.Context, string) error
+
+	// DescribeNetworkInterfaces list network interfaces.
+	DescribeNetworkInterfaces(context.Context, *ec2.DescribeNetworkInterfacesInput) ([]*ec2.NetworkInterface, error)
 
 	ModifyNetworkInterfaceAttributeWithContext(context.Context, *ec2.ModifyNetworkInterfaceAttributeInput) (*ec2.ModifyNetworkInterfaceAttributeOutput, error)
 	CreateSecurityGroupWithContext(context.Context, *ec2.CreateSecurityGroupInput) (*ec2.CreateSecurityGroupOutput, error)
@@ -78,6 +84,15 @@ func (c *Cloud) DeleteEC2TagsWithContext(ctx context.Context, i *ec2.DeleteTagsI
 	return c.ec2.DeleteTagsWithContext(ctx, i)
 }
 
+func (c *Cloud) DescribeNetworkInterfaces(ctx context.Context, input *ec2.DescribeNetworkInterfacesInput) ([]*ec2.NetworkInterface, error) {
+	var result []*ec2.NetworkInterface
+	err := c.ec2.DescribeNetworkInterfacesPagesWithContext(ctx, input, func(output *ec2.DescribeNetworkInterfacesOutput, _ bool) bool {
+		result = append(result, output.NetworkInterfaces...)
+		return true
+	})
+	return result, err
+}
+
 func (c *Cloud) GetSubnetsByNameOrID(ctx context.Context, nameOrIDs []string) (subnets []*ec2.Subnet, err error) {
 	var filters [][]*ec2.Filter
 	var names []string
@@ -201,20 +216,22 @@ func (c *Cloud) GetSecurityGroupByName(groupName string) (*ec2.SecurityGroup, er
 	return securityGroups[0], nil
 }
 
-func (c *Cloud) DeleteSecurityGroupByID(groupID string) error {
+func (c *Cloud) DeleteSecurityGroupByID(ctx context.Context, groupID string) error {
 	input := &ec2.DeleteSecurityGroupInput{
 		GroupId: aws.String(groupID),
 	}
 
-	retryOption := func(req *request.Request) {
-		req.Retryer = &deleteSecurityGroupRetryer{
-			req.Retryer,
+	ctx, cancel := context.WithTimeout(ctx, 2*time.Minute)
+	defer cancel()
+	return wait.PollImmediateUntil(2*time.Second, func() (done bool, err error) {
+		if _, err := c.ec2.DeleteSecurityGroupWithContext(ctx, input); err != nil {
+			if awsErr, ok := err.(awserr.Error); ok && awsErr.Code() == "DependencyViolation" {
+				return false, nil
+			}
+			return false, err
 		}
-	}
-	if _, err := c.ec2.DeleteSecurityGroupWithContext(aws.BackgroundContext(), input, retryOption); err != nil {
-		return err
-	}
-	return nil
+		return true, nil
+	}, ctx.Done())
 }
 
 // describeSecurityGroups is an helper to handle pagination for DescribeSecurityGroups API call
@@ -277,21 +294,3 @@ func (c *Cloud) IsNodeHealthy(instanceid string) (bool, error) {
 
 	return false, nil
 }
-
-type deleteSecurityGroupRetryer struct {
-	request.Retryer
-}
-
-func (r *deleteSecurityGroupRetryer) ShouldRetry(req *request.Request) bool {
-	if awsErr, ok := req.Error.(awserr.Error); ok {
-		if awsErr.Code() == "DependencyViolation" {
-			return true
-		}
-	}
-	// Fallback to built in retry rules
-	return r.Retryer.ShouldRetry(req)
-}
-
-func (r *deleteSecurityGroupRetryer) MaxRetries() int {
-	return 20
-}