diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index 514fe4d97ea..f794a28ccf6 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/pkg/cloudprovider/providers/aws/aws.go @@ -1427,6 +1427,7 @@ func isEqualIntPointer(l, r *int64) bool { } return *l == *r } + func isEqualStringPointer(l, r *string) bool { if l == nil { return r == nil @@ -1437,42 +1438,52 @@ 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 } } - 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 newPermission.UserIdGroupPairs { + for _, rightPair := range existing.UserIdGroupPairs { + if isEqualUserGroupPair(leftPair, rightPair, compareGroupUserIDs) { + return true } } + return false } return true } +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) { + 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 @@ -1487,6 +1498,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 @@ -1498,7 +1511,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 } @@ -1553,8 +1566,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 44d701d2347..0f65bde8cce 100644 --- a/pkg/cloudprovider/providers/aws/aws_test.go +++ b/pkg/cloudprovider/providers/aws/aws_test.go @@ -799,3 +799,67 @@ func TestSubnetIDsinVPC(t *testing.T) { } } + +func TestIpPermissionExistsHandlesMultipleGroupIds(t *testing.T) { + oldIpPermission := ec2.IpPermission{ + UserIdGroupPairs: []*ec2.UserIdGroupPair{ + {GroupId: aws.String("firstGroupId")}, + {GroupId: aws.String("secondGroupId")}, + {GroupId: aws.String("thirdGroupId")}, + }, + } + + existingIpPermission := ec2.IpPermission{ + UserIdGroupPairs: []*ec2.UserIdGroupPair{ + {GroupId: aws.String("secondGroupId")}, + }, + } + + newIpPermission := ec2.IpPermission{ + UserIdGroupPairs: []*ec2.UserIdGroupPair{ + {GroupId: aws.String("fourthGroupId")}, + }, + } + + equals := ipPermissionExists(&existingIpPermission, &oldIpPermission, false) + if !equals { + t.Errorf("Should have been considered equal since first is in the second array of groups") + } + + 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 TestIpPermissionExistsHandlesMultipleGroupIdsWithUserIds(t *testing.T) { + oldIpPermission := ec2.IpPermission{ + UserIdGroupPairs: []*ec2.UserIdGroupPair{ + {GroupId: aws.String("firstGroupId"), UserId: aws.String("firstUserId")}, + {GroupId: aws.String("secondGroupId"), UserId: aws.String("secondUserId")}, + {GroupId: aws.String("thirdGroupId"), UserId: aws.String("thirdUserId")}, + }, + } + + existingIpPermission := ec2.IpPermission{ + UserIdGroupPairs: []*ec2.UserIdGroupPair{ + {GroupId: aws.String("secondGroupId"), UserId: aws.String("secondUserId")}, + }, + } + + newIpPermission := ec2.IpPermission{ + UserIdGroupPairs: []*ec2.UserIdGroupPair{ + {GroupId: aws.String("secondGroupId"), UserId: aws.String("anotherUserId")}, + }, + } + + equals := ipPermissionExists(&existingIpPermission, &oldIpPermission, true) + if !equals { + t.Errorf("Should have been considered equal since first is in the second array of groups") + } + + 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") + } +}