From 190c829ac5e5aa628e613a635af9e7ac153108ab Mon Sep 17 00:00:00 2001
From: Dogan Narinc and James Ravn <james.ravn@sky.uk>
Date: Tue, 26 Jan 2016 11:01:15 +0000
Subject: [PATCH] Fix AWS ip permission comparison on security group

Change isEqualIPPermission to consider the entire list of security group
ids on when checking if a security group id has already been added.

This is used for example when adding and removing ingress rules to the
cluster nodes from an elastic load balancer. Without this, once there
are multiple load balancers, the method as it stands incorrectly returns
false even if the security group id is in the list of group ids. This
causes a few problems: dangling security groups which fill up an
account's limit since they don't get removed, and inability to recreate
load balancers in certain situations (receiving an
InvalidPermission.Duplicate from AWS when adding the same security
group).
---
 pkg/cloudprovider/providers/aws/aws.go      | 31 +++++++---
 pkg/cloudprovider/providers/aws/aws_test.go | 67 +++++++++++++++++++++
 2 files changed, 88 insertions(+), 10 deletions(-)

diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go
index d2114e54c1f..fac2fe1e35f 100644
--- a/pkg/cloudprovider/providers/aws/aws.go
+++ b/pkg/cloudprovider/providers/aws/aws.go
@@ -1447,23 +1447,32 @@ func isEqualIPPermission(l, r *ec2.IpPermission, compareGroupUserIDs bool) bool
 		}
 	}
 
-	if len(l.UserIdGroupPairs) != len(r.UserIdGroupPairs) {
-		return false
-	}
-	for j := range l.UserIdGroupPairs {
-		if !isEqualStringPointer(l.UserIdGroupPairs[j].GroupId, r.UserIdGroupPairs[j].GroupId) {
-			return false
-		}
-		if compareGroupUserIDs {
-			if !isEqualStringPointer(l.UserIdGroupPairs[j].UserId, r.UserIdGroupPairs[j].UserId) {
-				return false
+	for _, leftPair := range l.UserIdGroupPairs {
+		for _, rightPair := range r.UserIdGroupPairs {
+			if isEqualUserGroupPair(leftPair, rightPair, compareGroupUserIDs) {
+				return true
 			}
 		}
+		return false
 	}
 
 	return true
 }
 
+func isEqualUserGroupPair(l, r *ec2.UserIdGroupPair, compareGroupUserIDs bool) bool {
+	if isEqualStringPointer(l.GroupId, r.GroupId) {
+		if compareGroupUserIDs {
+			if isEqualStringPointer(l.UserId, r.UserId) {
+				return true
+			}
+		} else {
+			return true
+		}
+	}
+
+	return false
+}
+
 // Makes sure the security group includes the specified permissions
 // Returns true if and only if changes were made
 // The security group must already exist
@@ -1478,6 +1487,8 @@ func (s *AWSCloud) ensureSecurityGroupIngress(securityGroupId string, addPermiss
 		return false, fmt.Errorf("security group not found: %s", securityGroupId)
 	}
 
+	glog.V(2).Infof("Existing security group ingress: %s %v", securityGroupId, group.IpPermissions)
+
 	changes := []*ec2.IpPermission{}
 	for _, addPermission := range addPermissions {
 		hasUserID := false
diff --git a/pkg/cloudprovider/providers/aws/aws_test.go b/pkg/cloudprovider/providers/aws/aws_test.go
index 44d701d2347..c604a36872a 100644
--- a/pkg/cloudprovider/providers/aws/aws_test.go
+++ b/pkg/cloudprovider/providers/aws/aws_test.go
@@ -799,3 +799,70 @@ func TestSubnetIDsinVPC(t *testing.T) {
 	}
 
 }
+
+func TestIsEqualIPPermissionHandlesMultipleGroupIds(t *testing.T) {
+	oldIpPermission := ec2.IpPermission {
+		UserIdGroupPairs: []*ec2.UserIdGroupPair {
+			&ec2.UserIdGroupPair{GroupId: aws.String("firstGroupId")},
+			&ec2.UserIdGroupPair{GroupId: aws.String("secondGroupId")},
+			&ec2.UserIdGroupPair{GroupId: aws.String("thirdGroupId")},
+		},
+	}
+
+	existingIpPermission := ec2.IpPermission {
+		UserIdGroupPairs: []*ec2.UserIdGroupPair {
+			&ec2.UserIdGroupPair{GroupId: aws.String("secondGroupId")},
+		},
+	}
+
+	newIpPermission := ec2.IpPermission {
+		UserIdGroupPairs: []*ec2.UserIdGroupPair {
+			&ec2.UserIdGroupPair{GroupId: aws.String("fourthGroupId")},
+		},
+	}
+
+
+	equals := isEqualIPPermission(&existingIpPermission, &oldIpPermission, false)
+	if !equals {
+		t.Errorf("Should have been considered equal since first is in the second array of groups")
+	}
+
+	equals = isEqualIPPermission(&newIpPermission, &oldIpPermission, false)
+	if equals {
+		t.Errorf("Should have not been considered equal since first is not in the second array of groups")
+	}
+
+}
+
+func TestIsEqualIPPermissionHandlesMultipleGroupIdsWithUserIds(t *testing.T) {
+	oldIpPermission := ec2.IpPermission {
+		UserIdGroupPairs: []*ec2.UserIdGroupPair {
+			&ec2.UserIdGroupPair{GroupId: aws.String("firstGroupId"), UserId: aws.String("firstUserId")},
+			&ec2.UserIdGroupPair{GroupId: aws.String("secondGroupId"), UserId: aws.String("secondUserId")},
+			&ec2.UserIdGroupPair{GroupId: aws.String("thirdGroupId"), UserId: aws.String("thirdUserId")},
+		},
+	}
+
+	existingIpPermission := ec2.IpPermission {
+		UserIdGroupPairs: []*ec2.UserIdGroupPair {
+			&ec2.UserIdGroupPair{GroupId: aws.String("secondGroupId"), UserId: aws.String("secondUserId")},
+		},
+	}
+
+	newIpPermission := ec2.IpPermission {
+		UserIdGroupPairs: []*ec2.UserIdGroupPair {
+			&ec2.UserIdGroupPair{GroupId: aws.String("secondGroupId"), UserId: aws.String("anotherUserId")},
+		},
+	}
+
+
+	equals := isEqualIPPermission(&existingIpPermission, &oldIpPermission, true)
+	if !equals {
+		t.Errorf("Should have been considered equal since first is in the second array of groups")
+	}
+
+	equals = isEqualIPPermission(&newIpPermission, &oldIpPermission, true)
+	if equals {
+		t.Errorf("Should have not been considered equal since first is not in the second array of groups")
+	}
+}