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") + } +}