Repository: kubernetes-sigs/cluster-api-provider-aws
Pull request #3196 (2022-02-11 04:58:03)

Added test cases for pkg/cloud/services/securitygroup
<!-- Thanks for this PR! If this is your first PR please read the [contributing guide](../CONTRIBUTING.md) -->
<!-- If this PR is still work-in-progress and is being open for visibility please prefix the title with `WIP:` -->

**What type of PR is this?**
/area testing
<!--
Add one of the following kinds:
/kind feature
/kind bug
/kind api-change
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind flake
/kind regression
/kind support
-->

**What this PR does / why we need it**:
Added test cases for pkg/cloud/services/securitygroup 
<!-- Enter a description of the change and why this change is needed -->

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Part of #2910 

**Special notes for your reviewer**:

**Checklist**:
<!-- Put an "X" character inside the brackets of each completed task. Some may be optional depending on the PR in which case these can be deleted -->

- [x] squashed commits
- [ ] includes documentation
- [ ] adds unit tests
- [ ] adds or updates e2e tests

**Release note**:
<!--  Write your release note:
1. Enter your extended release note in the below block. 
2. If the PR requires additional action from users switching to the new release, include the string "action required".
2. If no release note is required, just write "NONE"....however we encourage contributors to never use this as release notes are incredible useful.
-->
```release-note
Added test cases for pkg/cloud/services/securitygroup and increased coverage to 78%
```


k8s-ci-robot: @shivi28: This issue is currently awaiting triage.

If CAPA/CAPI contributors determines this is a relevant issue, they will accept it by applying the `triage/accepted` label and provide further guidance.

The `triage/accepted` label can be added by org members by writing `/triage accepted` in a comment.


<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
k8s-ci-robot: @shivi28: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-cluster-api-provider-aws-test | d1f4e94f2117dbb7c93febee7d4bd37d839355e1 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-aws/3196/pull-cluster-api-provider-aws-test/1491999955391352832) | true | `/test pull-cluster-api-provider-aws-test`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=cluster-api-provider-aws&pr=3196). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Ashivi28). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/cluster-api-provider-aws/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
k8s-ci-robot: @shivi28: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-cluster-api-provider-aws-test | 17bf34e8193ac984729aa5692dc3d4d31470bbaf | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-aws/3196/pull-cluster-api-provider-aws-test/1492002585974935552) | true | `/test pull-cluster-api-provider-aws-test`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=cluster-api-provider-aws&pr=3196). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Ashivi28). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/cluster-api-provider-aws/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
k8s-ci-robot: @shivi28: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-cluster-api-provider-aws-test | 4494a4913a1d9c0358262add4ec2e0e399afd9ba | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-aws/3196/pull-cluster-api-provider-aws-test/1492069999097942016) | true | `/test pull-cluster-api-provider-aws-test`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=cluster-api-provider-aws&pr=3196). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Ashivi28). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/cluster-api-provider-aws/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
k8s-ci-robot: @shivi28: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-cluster-api-provider-aws-test | ad1c2f889854a36dac98aadf907821111f4e2868 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-aws/3196/pull-cluster-api-provider-aws-test/1492081205338705920) | true | `/test pull-cluster-api-provider-aws-test`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=cluster-api-provider-aws&pr=3196). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Ashivi28). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/cluster-api-provider-aws/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
shivi28: There is some issue with `pull-cluster-api-provider-aws-test` . It's failing in all the new PRs. Created a issue for [this](https://github.com/kubernetes-sigs/cluster-api-provider-aws/issues/3204).

Patch:
diff --git a/pkg/cloud/services/securitygroup/securitygroups.go b/pkg/cloud/services/securitygroup/securitygroups.go
index b30f4607b..316a02f89 100644
--- a/pkg/cloud/services/securitygroup/securitygroups.go
+++ b/pkg/cloud/services/securitygroup/securitygroups.go
@@ -423,7 +423,7 @@ func (s *Service) revokeAllSecurityGroupIngressRules(id string) error {
 
 	securityGroups, err := s.EC2Client.DescribeSecurityGroups(describeInput)
 	if err != nil {
-		return errors.Wrapf(err, "failed to query security group %q", id)
+		return err
 	}
 
 	for _, sg := range securityGroups.SecurityGroups {
@@ -434,7 +434,7 @@ func (s *Service) revokeAllSecurityGroupIngressRules(id string) error {
 			}
 			if _, err := s.EC2Client.RevokeSecurityGroupIngress(revokeInput); err != nil {
 				record.Warnf(s.scope.InfraCluster(), "FailedRevokeSecurityGroupIngressRules", "Failed to revoke all security group ingress rules for SecurityGroup %q: %v", *sg.GroupId, err)
-				return errors.Wrapf(err, "failed to revoke security group %q ingress rules", id)
+				return err
 			}
 			record.Eventf(s.scope.InfraCluster(), "SuccessfulRevokeSecurityGroupIngressRules", "Revoked all security group ingress rules for SecurityGroup %q", *sg.GroupId)
 		}


Test patch:
diff --git a/pkg/cloud/services/securitygroup/securitygroups_test.go b/pkg/cloud/services/securitygroup/securitygroups_test.go
index ef11a28d2..912f964ad 100644
--- a/pkg/cloud/services/securitygroup/securitygroups_test.go
+++ b/pkg/cloud/services/securitygroup/securitygroups_test.go
@@ -17,11 +17,11 @@ limitations under the License.
 package securitygroup
 
 import (
-	"context"
 	"strings"
 	"testing"
 
 	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/aws/awserr"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/golang/mock/gomock"
 	. "github.com/onsi/gomega"
@@ -32,6 +32,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	infrav1 "sigs.k8s.io/cluster-api-provider-aws/api/v1beta1"
+	"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/awserrors"
 	"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/scope"
 	"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/services"
 	"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/services/ec2/mock_ec2iface"
@@ -340,7 +341,7 @@ func TestReconcileSecurityGroups(t *testing.T) {
 			scheme := runtime.NewScheme()
 			_ = infrav1.AddToScheme(scheme)
 			client := fake.NewClientBuilder().WithScheme(scheme).Build()
-			scope, err := scope.NewClusterScope(scope.ClusterScopeParams{
+			cs, err := scope.NewClusterScope(scope.ClusterScopeParams{
 				Client: client,
 				Cluster: &clusterv1.Cluster{
 					ObjectMeta: metav1.ObjectMeta{Name: "test-cluster"},
@@ -358,7 +359,7 @@ func TestReconcileSecurityGroups(t *testing.T) {
 
 			tc.expect(ec2Mock.EXPECT())
 
-			s := NewService(scope, testSecurityGroupRoles)
+			s := NewService(cs, testSecurityGroupRoles)
 			s.EC2Client = ec2Mock
 
 			if err := s.ReconcileSecurityGroups(); err != nil && tc.err != nil {
@@ -376,7 +377,7 @@ func TestControlPlaneSecurityGroupNotOpenToAnyCIDR(t *testing.T) {
 	scheme := runtime.NewScheme()
 	_ = infrav1.AddToScheme(scheme)
 	client := fake.NewClientBuilder().WithScheme(scheme).Build()
-	scope, err := scope.NewClusterScope(scope.ClusterScopeParams{
+	cs, err := scope.NewClusterScope(scope.ClusterScopeParams{
 		Client: client,
 		Cluster: &clusterv1.Cluster{
 			ObjectMeta: metav1.ObjectMeta{Name: "test-cluster"},
@@ -387,7 +388,7 @@ func TestControlPlaneSecurityGroupNotOpenToAnyCIDR(t *testing.T) {
 		t.Fatalf("Failed to create test context: %v", err)
 	}
 
-	s := NewService(scope, testSecurityGroupRoles)
+	s := NewService(cs, testSecurityGroupRoles)
 	rules, err := s.getSecurityGroupIngressRules(infrav1.SecurityGroupControlPlane)
 	if err != nil {
 		t.Fatalf("Failed to lookup controlplane security group ingress rules: %v", err)
@@ -405,10 +406,10 @@ func TestDeleteSecurityGroups(t *testing.T) {
 	defer mockCtrl.Finish()
 
 	testCases := []struct {
-		name   string
-		input  *infrav1.NetworkSpec
-		expect func(m *mock_ec2iface.MockEC2APIMockRecorder)
-		err    error
+		name    string
+		input   *infrav1.NetworkSpec
+		expect  func(m *mock_ec2iface.MockEC2APIMockRecorder)
+		wantErr bool
 	}{
 		{
 			name: "do not delete overridden security groups",
@@ -439,17 +440,115 @@ func TestDeleteSecurityGroups(t *testing.T) {
 				},
 			},
 			expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
-				m.DescribeSecurityGroupsPages(gomock.Any(), gomock.Any()).Return(nil)
+				m.DescribeSecurityGroupsPages(gomock.AssignableToTypeOf(&ec2.DescribeSecurityGroupsInput{}), gomock.Any()).Return(nil)
+			},
+		},
+		{
+			name: "Should skip SG deletion if VPC ID not present",
+			input: &infrav1.NetworkSpec{
+				VPC: infrav1.VPCSpec{},
+			},
+		},
+		{
+			name: "Should return error if unable to find cluster-owned security groups in vpc",
+			input: &infrav1.NetworkSpec{
+				VPC: infrav1.VPCSpec{ID: "vpc-id"},
+			},
+			expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
+				m.DescribeSecurityGroupsPages(gomock.AssignableToTypeOf(&ec2.DescribeSecurityGroupsInput{}), gomock.Any()).Return(awserrors.NewFailedDependency("dependency-failure"))
+			},
+			wantErr: true,
+		},
+		{
+			name: "Should return error if unable to describe any SG present in VPC and owned by cluster",
+			input: &infrav1.NetworkSpec{
+				VPC: infrav1.VPCSpec{ID: "vpc-id"},
+			},
+			expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
+				m.DescribeSecurityGroupsPages(gomock.AssignableToTypeOf(&ec2.DescribeSecurityGroupsInput{}), gomock.Any()).
+					Do(processSecurityGroupsPage).Return(nil)
+				m.DescribeSecurityGroups(gomock.AssignableToTypeOf(&ec2.DescribeSecurityGroupsInput{})).Return(nil, awserr.New("dependency-failure", "dependency-failure", errors.Errorf("dependency-failure")))
+			},
+			wantErr: true,
+		},
+		{
+			name: "Should not revoke Ingress rules for a SG if IP permissions are not set and able to delete the SG",
+			input: &infrav1.NetworkSpec{
+				VPC: infrav1.VPCSpec{ID: "vpc-id"},
+			},
+			expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
+				m.DescribeSecurityGroupsPages(gomock.AssignableToTypeOf(&ec2.DescribeSecurityGroupsInput{}), gomock.Any()).
+					Do(processSecurityGroupsPage).Return(nil)
+				m.DescribeSecurityGroups(gomock.AssignableToTypeOf(&ec2.DescribeSecurityGroupsInput{})).Return(&ec2.DescribeSecurityGroupsOutput{
+					SecurityGroups: []*ec2.SecurityGroup{
+						{
+							GroupId:   aws.String("group-id"),
+							GroupName: aws.String("group-name"),
+						},
+					},
+				}, nil)
+				m.DeleteSecurityGroup(gomock.AssignableToTypeOf(&ec2.DeleteSecurityGroupInput{})).Return(nil, nil)
+			},
+		},
+		{
+			name: "Should return error if failed to revoke Ingress rules for a SG",
+			input: &infrav1.NetworkSpec{
+				VPC: infrav1.VPCSpec{ID: "vpc-id"},
+			},
+			expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
+				m.DescribeSecurityGroupsPages(gomock.AssignableToTypeOf(&ec2.DescribeSecurityGroupsInput{}), gomock.Any()).
+					Do(processSecurityGroupsPage).Return(nil)
+				m.DescribeSecurityGroups(gomock.AssignableToTypeOf(&ec2.DescribeSecurityGroupsInput{})).Return(&ec2.DescribeSecurityGroupsOutput{
+					SecurityGroups: []*ec2.SecurityGroup{
+						{
+							GroupId:   aws.String("group-id"),
+							GroupName: aws.String("group-name"),
+							IpPermissions: []*ec2.IpPermission{
+								{
+									ToPort: aws.Int64(4),
+								},
+							},
+						},
+					},
+				}, nil)
+				m.RevokeSecurityGroupIngress(gomock.AssignableToTypeOf(&ec2.RevokeSecurityGroupIngressInput{})).Return(nil, awserr.New("failure", "failure", errors.Errorf("failure")))
+			},
+			wantErr: true,
+		},
+		{
+			name: "Should delete SG successfully",
+			input: &infrav1.NetworkSpec{
+				VPC: infrav1.VPCSpec{ID: "vpc-id"},
+			},
+			expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
+				m.DescribeSecurityGroupsPages(gomock.AssignableToTypeOf(&ec2.DescribeSecurityGroupsInput{}), gomock.Any()).
+					Do(processSecurityGroupsPage).Return(nil)
+				m.DescribeSecurityGroups(gomock.AssignableToTypeOf(&ec2.DescribeSecurityGroupsInput{})).Return(&ec2.DescribeSecurityGroupsOutput{
+					SecurityGroups: []*ec2.SecurityGroup{
+						{
+							GroupId:   aws.String("group-id"),
+							GroupName: aws.String("group-name"),
+							IpPermissions: []*ec2.IpPermission{
+								{
+									ToPort: aws.Int64(4),
+								},
+							},
+						},
+					},
+				}, nil)
+				m.RevokeSecurityGroupIngress(gomock.AssignableToTypeOf(&ec2.RevokeSecurityGroupIngressInput{})).Return(nil, nil)
+				m.DeleteSecurityGroup(gomock.AssignableToTypeOf(&ec2.DeleteSecurityGroupInput{})).Return(nil, nil)
 			},
 		},
 	}
-
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
+			g := NewWithT(t)
 			ec2Mock := mock_ec2iface.NewMockEC2API(mockCtrl)
 
 			scheme := runtime.NewScheme()
-			_ = infrav1.AddToScheme(scheme)
+			g.Expect(infrav1.AddToScheme(scheme)).NotTo(HaveOccurred())
+
 			awsCluster := &infrav1.AWSCluster{
 				TypeMeta: metav1.TypeMeta{
 					APIVersion: infrav1.GroupVersion.String(),
@@ -460,34 +559,31 @@ func TestDeleteSecurityGroups(t *testing.T) {
 					NetworkSpec: *tc.input,
 				},
 			}
-			client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(awsCluster).Build()
 
-			ctx := context.TODO()
-			client.Create(ctx, awsCluster)
+			client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(awsCluster).Build()
 
-			scope, err := scope.NewClusterScope(scope.ClusterScopeParams{
+			cs, err := scope.NewClusterScope(scope.ClusterScopeParams{
 				Client: client,
 				Cluster: &clusterv1.Cluster{
 					ObjectMeta: metav1.ObjectMeta{Name: "test-cluster"},
 				},
 				AWSCluster: awsCluster,
 			})
-			if err != nil {
-				t.Fatalf("Failed to create test context: %v", err)
-			}
+			g.Expect(err).NotTo(HaveOccurred())
 
-			tc.expect(ec2Mock.EXPECT())
+			if tc.expect != nil {
+				tc.expect(ec2Mock.EXPECT())
+			}
 
-			s := NewService(scope, testSecurityGroupRoles)
+			s := NewService(cs, testSecurityGroupRoles)
 			s.EC2Client = ec2Mock
 
-			if err := s.DeleteSecurityGroups(); err != nil && tc.err != nil {
-				if !strings.Contains(err.Error(), tc.err.Error()) {
-					t.Fatalf("was expecting error to look like '%v', but got '%v'", tc.err, err)
-				}
-			} else if err != nil {
-				t.Fatalf("got an unexpected error: %v", err)
+			err = s.DeleteSecurityGroups()
+			if tc.wantErr {
+				g.Expect(err).To(HaveOccurred())
+				return
 			}
+			g.Expect(err).NotTo(HaveOccurred())
 		})
 	}
 }
@@ -575,3 +671,15 @@ func TestIngressRulesFromSDKType(t *testing.T) {
 		})
 	}
 }
+
+var processSecurityGroupsPage = func(_, y interface{}) {
+	funcType := y.(func(out *ec2.DescribeSecurityGroupsOutput, last bool) bool)
+	funcType(&ec2.DescribeSecurityGroupsOutput{
+		SecurityGroups: []*ec2.SecurityGroup{
+			{
+				GroupId:   aws.String("group-id"),
+				GroupName: aws.String("group-name"),
+			},
+		},
+	}, true)
+}