Merge pull request #20790 from Clarifai/ip-permission-iprange

pkg/cloudprovider/providers/aws/aws.go

@@ -1525,15 +1525,25 @@ func ipPermissionExists(newPermission, existing *ec2.IpPermission, compareGroupU
 	if !isEqualStringPointer(newPermission.IpProtocol, existing.IpProtocol) {
 		return false
 	}
-	if len(newPermission.IpRanges) != len(existing.IpRanges) {
+	// Check only if newPermission is a subset of existing. Usually it has zero or one elements.
+	// Not doing actual CIDR math yet; not clear it's needed, either.
+	glog.V(4).Infof("Comparing %v to %v", newPermission, existing)
+	if len(newPermission.IpRanges) > len(existing.IpRanges) {
 		return false
 	}
+
 	for j := range newPermission.IpRanges {
-		if !isEqualStringPointer(newPermission.IpRanges[j].CidrIp, existing.IpRanges[j].CidrIp) {
+		found := false
+		for k := range existing.IpRanges {
+			if isEqualStringPointer(newPermission.IpRanges[j].CidrIp, existing.IpRanges[k].CidrIp) {
+				found = true
+				break
+			}
+		}
+		if found == false {
 			return false
 		}
 	}
-
 	for _, leftPair := range newPermission.UserIdGroupPairs {
 		for _, rightPair := range existing.UserIdGroupPairs {
 			if isEqualUserGroupPair(leftPair, rightPair, compareGroupUserIDs) {

pkg/cloudprovider/providers/aws/aws_test.go

@@ -849,6 +849,63 @@ func TestIpPermissionExistsHandlesMultipleGroupIds(t *testing.T) {
 	}
 }
 
+func TestIpPermissionExistsHandlesRangeSubsets(t *testing.T) {
+	// Two existing scenarios we'll test against
+	emptyIpPermission := ec2.IpPermission{}
+
+	oldIpPermission := ec2.IpPermission{
+		IpRanges: []*ec2.IpRange{
+			{CidrIp: aws.String("10.0.0.0/8")},
+			{CidrIp: aws.String("192.168.1.0/24")},
+		},
+	}
+
+	// Two already existing ranges and a new one
+	existingIpPermission := ec2.IpPermission{
+		IpRanges: []*ec2.IpRange{
+			{CidrIp: aws.String("10.0.0.0/8")},
+		},
+	}
+	existingIpPermission2 := ec2.IpPermission{
+		IpRanges: []*ec2.IpRange{
+			{CidrIp: aws.String("192.168.1.0/24")},
+		},
+	}
+
+	newIpPermission := ec2.IpPermission{
+		IpRanges: []*ec2.IpRange{
+			{CidrIp: aws.String("172.16.0.0/16")},
+		},
+	}
+
+	exists := ipPermissionExists(&emptyIpPermission, &emptyIpPermission, false)
+	if !exists {
+		t.Errorf("Should have been considered existing since we're comparing a range array against itself")
+	}
+	exists = ipPermissionExists(&oldIpPermission, &oldIpPermission, false)
+	if !exists {
+		t.Errorf("Should have been considered existing since we're comparing a range array against itself")
+	}
+
+	exists = ipPermissionExists(&existingIpPermission, &oldIpPermission, false)
+	if !exists {
+		t.Errorf("Should have been considered existing since 10.* is in oldIpPermission's array of ranges")
+	}
+	exists = ipPermissionExists(&existingIpPermission2, &oldIpPermission, false)
+	if !exists {
+		t.Errorf("Should have been considered existing since 192.* is in oldIpPermission2's array of ranges")
+	}
+
+	exists = ipPermissionExists(&newIpPermission, &emptyIpPermission, false)
+	if exists {
+		t.Errorf("Should have not been considered existing since we compared against a missing array of ranges")
+	}
+	exists = ipPermissionExists(&newIpPermission, &oldIpPermission, false)
+	if exists {
+		t.Errorf("Should have not been considered existing since 172.* is not in oldIpPermission's array of ranges")
+	}
+}
+
 func TestIpPermissionExistsHandlesMultipleGroupIdsWithUserIds(t *testing.T) {
 	oldIpPermission := ec2.IpPermission{
 		UserIdGroupPairs: []*ec2.UserIdGroupPair{