From b4bbc3b3efe636004a0111e0b7704a40e8b971c9 Mon Sep 17 00:00:00 2001
From: James Ravn <james.ravn@sky.uk>
Date: Tue, 26 Jan 2016 13:57:52 +0000
Subject: [PATCH] Fix removal of aws security group ingress

The ip permission method now checks for containment, not equality, so
order of parameters matter. This change fixes
`removeSecurityGroupIngress` to pass in the removal permission first to
compare against the existing permission.
---
 pkg/cloudprovider/providers/aws/aws.go      | 26 +++++++++++----------
 pkg/cloudprovider/providers/aws/aws_test.go | 13 +++++------
 2 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go
index fac2fe1e35f..33e1fc2f316 100644
--- a/pkg/cloudprovider/providers/aws/aws.go
+++ b/pkg/cloudprovider/providers/aws/aws.go
@@ -1418,6 +1418,7 @@ func isEqualIntPointer(l, r *int64) bool {
 	}
 	return *l == *r
 }
+
 func isEqualStringPointer(l, r *string) bool {
 	if l == nil {
 		return r == nil
@@ -1428,27 +1429,27 @@ func isEqualStringPointer(l, r *string) bool {
 	return *l == *r
 }
 
-func isEqualIPPermission(l, r *ec2.IpPermission, compareGroupUserIDs bool) bool {
-	if !isEqualIntPointer(l.FromPort, r.FromPort) {
+func ipPermissionExists(newPermission, existing *ec2.IpPermission, compareGroupUserIDs bool) bool {
+	if !isEqualIntPointer(newPermission.FromPort, existing.FromPort) {
 		return false
 	}
-	if !isEqualIntPointer(l.ToPort, r.ToPort) {
+	if !isEqualIntPointer(newPermission.ToPort, existing.ToPort) {
 		return false
 	}
-	if !isEqualStringPointer(l.IpProtocol, r.IpProtocol) {
+	if !isEqualStringPointer(newPermission.IpProtocol, existing.IpProtocol) {
 		return false
 	}
-	if len(l.IpRanges) != len(r.IpRanges) {
+	if len(newPermission.IpRanges) != len(existing.IpRanges) {
 		return false
 	}
-	for j := range l.IpRanges {
-		if !isEqualStringPointer(l.IpRanges[j].CidrIp, r.IpRanges[j].CidrIp) {
+	for j := range newPermission.IpRanges {
+		if !isEqualStringPointer(newPermission.IpRanges[j].CidrIp, existing.IpRanges[j].CidrIp) {
 			return false
 		}
 	}
 
-	for _, leftPair := range l.UserIdGroupPairs {
-		for _, rightPair := range r.UserIdGroupPairs {
+	for _, leftPair := range newPermission.UserIdGroupPairs {
+		for _, rightPair := range existing.UserIdGroupPairs {
 			if isEqualUserGroupPair(leftPair, rightPair, compareGroupUserIDs) {
 				return true
 			}
@@ -1460,6 +1461,7 @@ func isEqualIPPermission(l, r *ec2.IpPermission, compareGroupUserIDs bool) bool
 }
 
 func isEqualUserGroupPair(l, r *ec2.UserIdGroupPair, compareGroupUserIDs bool) bool {
+	glog.V(2).Infof("Comparing %v to %v", *l.GroupId, *r.GroupId)
 	if isEqualStringPointer(l.GroupId, r.GroupId) {
 		if compareGroupUserIDs {
 			if isEqualStringPointer(l.UserId, r.UserId) {
@@ -1500,7 +1502,7 @@ func (s *AWSCloud) ensureSecurityGroupIngress(securityGroupId string, addPermiss
 
 		found := false
 		for _, groupPermission := range group.IpPermissions {
-			if isEqualIPPermission(addPermission, groupPermission, hasUserID) {
+			if ipPermissionExists(addPermission, groupPermission, hasUserID) {
 				found = true
 				break
 			}
@@ -1555,8 +1557,8 @@ func (s *AWSCloud) removeSecurityGroupIngress(securityGroupId string, removePerm
 
 		var found *ec2.IpPermission
 		for _, groupPermission := range group.IpPermissions {
-			if isEqualIPPermission(groupPermission, removePermission, hasUserID) {
-				found = groupPermission
+			if ipPermissionExists(removePermission, groupPermission, hasUserID) {
+				found = removePermission
 				break
 			}
 		}
diff --git a/pkg/cloudprovider/providers/aws/aws_test.go b/pkg/cloudprovider/providers/aws/aws_test.go
index c604a36872a..5d46bdb356b 100644
--- a/pkg/cloudprovider/providers/aws/aws_test.go
+++ b/pkg/cloudprovider/providers/aws/aws_test.go
@@ -800,7 +800,7 @@ func TestSubnetIDsinVPC(t *testing.T) {
 
 }
 
-func TestIsEqualIPPermissionHandlesMultipleGroupIds(t *testing.T) {
+func TestIpPermissionExistsHandlesMultipleGroupIds(t *testing.T) {
 	oldIpPermission := ec2.IpPermission {
 		UserIdGroupPairs: []*ec2.UserIdGroupPair {
 			&ec2.UserIdGroupPair{GroupId: aws.String("firstGroupId")},
@@ -822,19 +822,18 @@ func TestIsEqualIPPermissionHandlesMultipleGroupIds(t *testing.T) {
 	}
 
 
-	equals := isEqualIPPermission(&existingIpPermission, &oldIpPermission, false)
+	equals := ipPermissionExists(&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)
+	equals = ipPermissionExists(&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) {
+func TestIpPermissionExistsHandlesMultipleGroupIdsWithUserIds(t *testing.T) {
 	oldIpPermission := ec2.IpPermission {
 		UserIdGroupPairs: []*ec2.UserIdGroupPair {
 			&ec2.UserIdGroupPair{GroupId: aws.String("firstGroupId"), UserId: aws.String("firstUserId")},
@@ -856,12 +855,12 @@ func TestIsEqualIPPermissionHandlesMultipleGroupIdsWithUserIds(t *testing.T) {
 	}
 
 
-	equals := isEqualIPPermission(&existingIpPermission, &oldIpPermission, true)
+	equals := ipPermissionExists(&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)
+	equals = ipPermissionExists(&newIpPermission, &oldIpPermission, true)
 	if equals {
 		t.Errorf("Should have not been considered equal since first is not in the second array of groups")
 	}