From 190c829ac5e5aa628e613a635af9e7ac153108ab Mon Sep 17 00:00:00 2001 From: Dogan Narinc and James Ravn Date: Tue, 26 Jan 2016 11:01:15 +0000 Subject: [PATCH 1/3] 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") + } +} From b4bbc3b3efe636004a0111e0b7704a40e8b971c9 Mon Sep 17 00:00:00 2001 From: James Ravn Date: Tue, 26 Jan 2016 13:57:52 +0000 Subject: [PATCH 2/3] 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") } From c3383b34221bc43d952d61950ed57eeb45c14067 Mon Sep 17 00:00:00 2001 From: James Ravn Date: Tue, 26 Jan 2016 15:13:25 +0000 Subject: [PATCH 3/3] Fix formatting of aws_test.go --- pkg/cloudprovider/providers/aws/aws_test.go | 46 ++++++++++----------- 1 file changed, 22 insertions(+), 24 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws_test.go b/pkg/cloudprovider/providers/aws/aws_test.go index 5d46bdb356b..0f65bde8cce 100644 --- a/pkg/cloudprovider/providers/aws/aws_test.go +++ b/pkg/cloudprovider/providers/aws/aws_test.go @@ -801,27 +801,26 @@ func TestSubnetIDsinVPC(t *testing.T) { } func TestIpPermissionExistsHandlesMultipleGroupIds(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")}, + oldIpPermission := ec2.IpPermission{ + UserIdGroupPairs: []*ec2.UserIdGroupPair{ + {GroupId: aws.String("firstGroupId")}, + {GroupId: aws.String("secondGroupId")}, + {GroupId: aws.String("thirdGroupId")}, }, } - existingIpPermission := ec2.IpPermission { - UserIdGroupPairs: []*ec2.UserIdGroupPair { - &ec2.UserIdGroupPair{GroupId: aws.String("secondGroupId")}, + existingIpPermission := ec2.IpPermission{ + UserIdGroupPairs: []*ec2.UserIdGroupPair{ + {GroupId: aws.String("secondGroupId")}, }, } - newIpPermission := ec2.IpPermission { - UserIdGroupPairs: []*ec2.UserIdGroupPair { - &ec2.UserIdGroupPair{GroupId: aws.String("fourthGroupId")}, + 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") @@ -834,27 +833,26 @@ func TestIpPermissionExistsHandlesMultipleGroupIds(t *testing.T) { } func TestIpPermissionExistsHandlesMultipleGroupIdsWithUserIds(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")}, + 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 { - &ec2.UserIdGroupPair{GroupId: aws.String("secondGroupId"), UserId: aws.String("secondUserId")}, + existingIpPermission := ec2.IpPermission{ + UserIdGroupPairs: []*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")}, + 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")