Merge pull request #58874 from sorenmat/sg_tag_verbose

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Include more information when multiple security groups are tagged **What this PR does / why we need it**: When trying to create ELB we can sometime fail if there is more then one AWS security group tagged. It very useful to get the list of security groups printed in the error message. **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: Fixes # **Special notes for your reviewer**: **Release note**: ```release-note Include the list of security groups when failing with the errors that more then one is tagged ```

Merge pull request #58874 from sorenmat/sg_tag_verbose
d09cd75e · Kubernetes Submit Queue · GitHub · ca767341 · 7c7e691c · d09cd75e
Unverified Commit d09cd75e authored May 09, 2018 by Kubernetes Submit Queue Committed by GitHub May 09, 2018
Hide whitespace changes
Inline Side-by-side

Showing with 28 additions and 1 deletion

aws.go pkg/cloudprovider/providers/aws/aws.go +5 -1

aws_test.go pkg/cloudprovider/providers/aws/aws_test.go +23 -0

No files found.
--- a/pkg/cloudprovider/providers/aws/aws.go
+++ b/pkg/cloudprovider/providers/aws/aws.go
@@ -3683,7 +3683,11 @@ func findSecurityGroupForInstance(instance *ec2.Instance, taggedSecurityGroups m
 		// We create instances with one SG
 		// If users create multiple SGs, they must tag one of them as being k8s owned
 		if len(tagged) != 1 {
-			return nil, fmt.Errorf("Multiple tagged security groups found for instance %s; ensure only the k8s security group is tagged", instanceID)
+			taggedGroups := ""
+			for _, v := range tagged {
+				taggedGroups += fmt.Sprintf("%s(%s) ", *v.GroupId, *v.GroupName)
+			}
+			return nil, fmt.Errorf("Multiple tagged security groups found for instance %s; ensure only the k8s security group is tagged; the tagged groups were %v", instanceID, taggedGroups)
 		}
 		return tagged[0], nil
 	}

--- a/pkg/cloudprovider/providers/aws/aws_test.go
+++ b/pkg/cloudprovider/providers/aws/aws_test.go
@@ -1319,6 +1319,29 @@ func TestEnsureLoadBalancerHealthCheck(t *testing.T) {
 	})
 }
+func TestFindSecurityGroupForInstance(t *testing.T) {
+	groups := map[string]*ec2.SecurityGroup{"sg123": {GroupId: aws.String("sg123")}}
+	id, err := findSecurityGroupForInstance(&ec2.Instance{SecurityGroups: []*ec2.GroupIdentifier{{GroupId: aws.String("sg123"), GroupName: aws.String("my_group")}}}, groups)
+	if err != nil {
+		t.Error()
+	}
+	assert.Equal(t, *id.GroupId, "sg123")
+	assert.Equal(t, *id.GroupName, "my_group")
+}
+func TestFindSecurityGroupForInstanceMultipleTagged(t *testing.T) {
+	groups := map[string]*ec2.SecurityGroup{"sg123": {GroupId: aws.String("sg123")}}
+	_, err := findSecurityGroupForInstance(&ec2.Instance{
+		SecurityGroups: []*ec2.GroupIdentifier{
+			{GroupId: aws.String("sg123"), GroupName: aws.String("my_group")},
+			{GroupId: aws.String("sg123"), GroupName: aws.String("another_group")},
+		},
+	}, groups)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "sg123(my_group)")
+	assert.Contains(t, err.Error(), "sg123(another_group)")
+}
 func newMockedFakeAWSServices(id string) *FakeAWSServices {
 	s := NewFakeAWSServices(id)
 	s.ec2 = &MockedFakeEC2{FakeEC2Impl: s.ec2.(*FakeEC2Impl)}