From e32c66c6f41fed9c6d7aeb74cb151ceeb2475682 Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Sat, 6 Jun 2015 12:22:50 -0400 Subject: [PATCH 1/4] Fix typo: Ingess -> Ingress --- pkg/cloudprovider/aws/aws.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/cloudprovider/aws/aws.go b/pkg/cloudprovider/aws/aws.go index 6262f016a4e..0df4ea8ccc9 100644 --- a/pkg/cloudprovider/aws/aws.go +++ b/pkg/cloudprovider/aws/aws.go @@ -1463,7 +1463,7 @@ func isEqualIPPermission(l, r *ec2.IPPermission) bool { // Makes sure the security group includes the specified permissions // Returns true iff changes were made // The security group must already exist -func (s *AWSCloud) ensureSecurityGroupIngess(securityGroupId string, addPermissions []*ec2.IPPermission) (bool, error) { +func (s *AWSCloud) ensureSecurityGroupIngress(securityGroupId string, addPermissions []*ec2.IPPermission) (bool, error) { group, err := s.findSecurityGroup(securityGroupId) if err != nil { glog.Warning("error retrieving security group", err) @@ -1510,7 +1510,7 @@ func (s *AWSCloud) ensureSecurityGroupIngess(securityGroupId string, addPermissi // Makes sure the security group no longer includes the specified permissions // Returns true iff changes were made // Returns true if the security group no longer exists -func (s *AWSCloud) removeSecurityGroupIngess(securityGroupId string, removePermissions []*ec2.IPPermission) (bool, error) { +func (s *AWSCloud) removeSecurityGroupIngress(securityGroupId string, removePermissions []*ec2.IPPermission) (bool, error) { group, err := s.findSecurityGroup(securityGroupId) if err != nil { glog.Warning("error retrieving security group", err) @@ -1697,7 +1697,7 @@ func (s *AWSCloud) CreateTCPLoadBalancer(name, region string, publicIP net.IP, p permissions = append(permissions, permission) } - _, err = s.ensureSecurityGroupIngess(securityGroupID, permissions) + _, err = s.ensureSecurityGroupIngress(securityGroupID, permissions) if err != nil { return nil, err } @@ -1927,7 +1927,7 @@ func (s *AWSCloud) updateInstanceSecurityGroupsForLoadBalancer(lb *elb.LoadBalan permissions := []*ec2.IPPermission{permission} if add { - changed, err := s.ensureSecurityGroupIngess(instanceSecurityGroupId, permissions) + changed, err := s.ensureSecurityGroupIngress(instanceSecurityGroupId, permissions) if err != nil { return err } @@ -1935,7 +1935,7 @@ func (s *AWSCloud) updateInstanceSecurityGroupsForLoadBalancer(lb *elb.LoadBalan glog.Warning("allowing ingress was not needed; concurrent change? groupId=", instanceSecurityGroupId) } } else { - changed, err := s.removeSecurityGroupIngess(instanceSecurityGroupId, permissions) + changed, err := s.removeSecurityGroupIngress(instanceSecurityGroupId, permissions) if err != nil { return err } From 8fafefd728ad8f0ac963d6ebfe4f134fcdf7b639 Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Sat, 6 Jun 2015 12:25:50 -0400 Subject: [PATCH 2/4] Fix doc for edge-case return from removeSecurityGroupIngress --- pkg/cloudprovider/aws/aws.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cloudprovider/aws/aws.go b/pkg/cloudprovider/aws/aws.go index 0df4ea8ccc9..9b8e4a28831 100644 --- a/pkg/cloudprovider/aws/aws.go +++ b/pkg/cloudprovider/aws/aws.go @@ -1509,7 +1509,7 @@ func (s *AWSCloud) ensureSecurityGroupIngress(securityGroupId string, addPermiss // Makes sure the security group no longer includes the specified permissions // Returns true iff changes were made -// Returns true if the security group no longer exists +// If the security group no longer exists, will return (false, nil) func (s *AWSCloud) removeSecurityGroupIngress(securityGroupId string, removePermissions []*ec2.IPPermission) (bool, error) { group, err := s.findSecurityGroup(securityGroupId) if err != nil { From 17002595082c7028a9c35edbde348813740782df Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Sat, 6 Jun 2015 12:35:39 -0400 Subject: [PATCH 3/4] AWS: Ignore the UserId when determining whether we can skip revoking a security group Otherwise we weren't correctly de-authorizing the AWS LB SG from the Node SG --- pkg/cloudprovider/aws/aws.go | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/pkg/cloudprovider/aws/aws.go b/pkg/cloudprovider/aws/aws.go index 9b8e4a28831..a5fe77a6ab4 100644 --- a/pkg/cloudprovider/aws/aws.go +++ b/pkg/cloudprovider/aws/aws.go @@ -1426,7 +1426,7 @@ func isEqualStringPointer(l, r *string) bool { return *l == *r } -func isEqualIPPermission(l, r *ec2.IPPermission) bool { +func isEqualIPPermission(l, r *ec2.IPPermission, compareGroupUserIDs bool) bool { if !isEqualIntPointer(l.FromPort, r.FromPort) { return false } @@ -1452,8 +1452,10 @@ func isEqualIPPermission(l, r *ec2.IPPermission) bool { if !isEqualStringPointer(l.UserIDGroupPairs[j].GroupID, r.UserIDGroupPairs[j].GroupID) { return false } - if !isEqualStringPointer(l.UserIDGroupPairs[j].UserID, r.UserIDGroupPairs[j].UserID) { - return false + if compareGroupUserIDs { + if !isEqualStringPointer(l.UserIDGroupPairs[j].UserID, r.UserIDGroupPairs[j].UserID) { + return false + } } } @@ -1476,9 +1478,16 @@ func (s *AWSCloud) ensureSecurityGroupIngress(securityGroupId string, addPermiss changes := []*ec2.IPPermission{} for _, addPermission := range addPermissions { + hasUserID := false + for i := range addPermission.UserIDGroupPairs { + if addPermission.UserIDGroupPairs[i].UserID != nil { + hasUserID = true + } + } + found := false for _, groupPermission := range group.IPPermissions { - if isEqualIPPermission(addPermission, groupPermission) { + if isEqualIPPermission(addPermission, groupPermission, hasUserID) { found = true break } @@ -1524,16 +1533,23 @@ func (s *AWSCloud) removeSecurityGroupIngress(securityGroupId string, removePerm changes := []*ec2.IPPermission{} for _, removePermission := range removePermissions { - found := false + hasUserID := false + for i := range removePermission.UserIDGroupPairs { + if removePermission.UserIDGroupPairs[i].UserID != nil { + hasUserID = true + } + } + + var found *ec2.IPPermission for _, groupPermission := range group.IPPermissions { - if isEqualIPPermission(groupPermission, removePermission) { - found = true + if isEqualIPPermission(groupPermission, removePermission, hasUserID) { + found = groupPermission break } } - if found { - changes = append(changes, removePermission) + if found != nil { + changes = append(changes, found) } } From c2caa3f1da75e2000acfd4eaf45920a928d83cdb Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Sat, 6 Jun 2015 14:54:15 -0400 Subject: [PATCH 4/4] AWS: Fix cleanup of security group The most reliable way seems to be to deauthorize the LB security group from other groups, then delete the LB itself, then repeatedly retry to delete the LB security group. We can't delete the LB security group until the LB is actually completely deleted, but the LB is hidden from the API during deletion. So our only real option is to retry deletion of the LB security group until the expected error goes away when the LB is fully deleted. --- pkg/cloudprovider/aws/aws.go | 52 +++++++++++++++++++++++++++++------- 1 file changed, 43 insertions(+), 9 deletions(-) diff --git a/pkg/cloudprovider/aws/aws.go b/pkg/cloudprovider/aws/aws.go index a5fe77a6ab4..150aa609437 100644 --- a/pkg/cloudprovider/aws/aws.go +++ b/pkg/cloudprovider/aws/aws.go @@ -2004,20 +2004,54 @@ func (s *AWSCloud) EnsureTCPLoadBalancerDeleted(name, region string) error { } { - // Delete the security group - for _, securityGroupId := range lb.SecurityGroups { - if isNilOrEmpty(securityGroupId) { + // 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 + // deleting in the background. We get a DependencyViolation until the load balancer has deleted itself + + // Collect the security groups to delete + securityGroupIDs := map[string]struct{}{} + for _, securityGroupID := range lb.SecurityGroups { + if isNilOrEmpty(securityGroupID) { glog.Warning("Ignoring empty security group in ", name) continue } + securityGroupIDs[*securityGroupID] = struct{}{} + } - request := &ec2.DeleteSecurityGroupInput{} - request.GroupID = securityGroupId - _, err := s.ec2.DeleteSecurityGroup(request) - if err != nil { - glog.Errorf("error deleting security group (%s): %v", orEmpty(securityGroupId), err) - return err + // Loop through and try to delete them + timeoutAt := time.Now().Add(time.Second * 300) + for { + for securityGroupID := range securityGroupIDs { + request := &ec2.DeleteSecurityGroupInput{} + request.GroupID = &securityGroupID + _, err := s.ec2.DeleteSecurityGroup(request) + if err == nil { + delete(securityGroupIDs, securityGroupID) + } else { + ignore := false + if awsError, ok := err.(awserr.Error); ok { + if awsError.Code() == "DependencyViolation" { + glog.V(2).Infof("ignoring DependencyViolation while deleting load-balancer security group (%s), assuming because LB is in process of deleting", securityGroupID) + ignore = true + } + } + if !ignore { + return fmt.Errorf("error while deleting load balancer security group (%s): %v", securityGroupID, err) + } + } } + + if len(securityGroupIDs) == 0 { + break + } + + if time.Now().After(timeoutAt) { + return fmt.Errorf("timed out waiting for load-balancer deletion: %s", name) + } + + glog.V(2).Info("waiting for load-balancer to delete so we can delete security groups: ", name) + + time.Sleep(5 * time.Second) } }