Repository: kubernetes-sigs/cluster-api-provider-aws
Pull request #3202 (2022-02-11 11:08:44)

Test cases added for eips.go
<!-- 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 eips.go
<!-- 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 -->

- [ ] 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 eips.go and increase 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: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **richardcase** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @richardcase` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fcluster-api-provider-aws).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["richardcase"]} -->
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 | 71ec7aa2a773e39867d629b74310f674984ce571 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-aws/3202/pull-cluster-api-provider-aws-test/1492093245897838592) | 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=3202). [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 -->
Ankitasw: The current failure in this PR is not because of the flake that we are facing, as this test ran for 3hr

Patch:
diff --git a/pkg/cloud/services/network/eips.go b/pkg/cloud/services/network/eips.go
index 09f06401c..f17446ffe 100644
--- a/pkg/cloud/services/network/eips.go
+++ b/pkg/cloud/services/network/eips.go
@@ -109,20 +109,21 @@ func (s *Service) releaseAddresses() error {
 	if err != nil {
 		return errors.Wrapf(err, "failed to describe elastic IPs %q", err)
 	}
-
+	if out == nil {
+		return nil
+	}
 	for i := range out.Addresses {
 		ip := out.Addresses[i]
 		if ip.AssociationId != nil {
-			_, err := s.EC2Client.DisassociateAddress(&ec2.DisassociateAddressInput{
+			if _, err := s.EC2Client.DisassociateAddress(&ec2.DisassociateAddressInput{
 				AssociationId: ip.AssociationId,
-			})
-			if err != nil {
+			}); err != nil {
 				record.Warnf(s.scope.InfraCluster(), "FailedDisassociateEIP", "Failed to disassociate Elastic IP %q: %v", *ip.AllocationId, err)
 				return errors.Errorf("failed to disassociate Elastic IP %q with allocation ID %q: Still associated with association ID %q", *ip.PublicIp, *ip.AllocationId, *ip.AssociationId)
 			}
 		}
 
-		err := wait.WaitForWithRetryable(wait.NewBackoff(), func() (bool, error) {
+		if err := wait.WaitForWithRetryable(wait.NewBackoff(), func() (bool, error) {
 			_, err := s.EC2Client.ReleaseAddress(&ec2.ReleaseAddressInput{AllocationId: ip.AllocationId})
 			if err != nil {
 				if ip.AssociationId != nil {
@@ -132,10 +133,8 @@ func (s *Service) releaseAddresses() error {
 				}
 				return false, err
 			}
-
 			return true, nil
-		}, awserrors.AuthFailure, awserrors.InUseIPAddress)
-		if err != nil {
+		}, awserrors.AuthFailure, awserrors.InUseIPAddress); err != nil {
 			record.Warnf(s.scope.InfraCluster(), "FailedReleaseEIP", "Failed to disassociate Elastic IP %q: %v", *ip.AllocationId, err)
 			return errors.Wrapf(err, "failed to release ElasticIP %q", *ip.AllocationId)
 		}


Test patch:
diff --git a/pkg/cloud/services/network/eips_test.go b/pkg/cloud/services/network/eips_test.go
new file mode 100644
index 000000000..edc9372b8
--- /dev/null
+++ b/pkg/cloud/services/network/eips_test.go
@@ -0,0 +1,174 @@
+/*
+Copyright 2022 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package network
+
+import (
+	"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"
+	"github.com/pkg/errors"
+	"k8s.io/apimachinery/pkg/runtime"
+	"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/ec2/mock_ec2iface"
+	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+)
+
+func TestService_releaseAddresses(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	tests := []struct {
+		name    string
+		expect  func(m *mock_ec2iface.MockEC2APIMockRecorder)
+		wantErr bool
+	}{
+		{
+			name: "Should return error if failed to describe IP addresses",
+			expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
+				m.DescribeAddresses(gomock.AssignableToTypeOf(&ec2.DescribeAddressesInput{})).Return(nil, awserrors.NewFailedDependency("dependency failure"))
+			},
+			wantErr: true,
+		},
+		{
+			name: "Should ignore releasing elastic IP addresses if not found",
+			expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
+				m.DescribeAddresses(gomock.AssignableToTypeOf(&ec2.DescribeAddressesInput{})).Return(nil, nil)
+			},
+		},
+		{
+			name: "Should return error if failed to disassociate IP address",
+			expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
+				m.DescribeAddresses(gomock.AssignableToTypeOf(&ec2.DescribeAddressesInput{})).Return(&ec2.DescribeAddressesOutput{
+					Addresses: []*ec2.Address{
+						{
+							AssociationId: aws.String("association-id-1"),
+							PublicIp:      aws.String("public-ip"),
+							AllocationId:  aws.String("allocation-id"),
+						},
+					},
+				}, nil)
+				m.DisassociateAddress(gomock.AssignableToTypeOf(&ec2.DisassociateAddressInput{})).Return(nil, awserrors.NewFailedDependency("dependency-failure"))
+			},
+			wantErr: true,
+		},
+		{
+			name: "Should be able to release the IP address",
+			expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
+				m.DescribeAddresses(gomock.AssignableToTypeOf(&ec2.DescribeAddressesInput{})).Return(&ec2.DescribeAddressesOutput{
+					Addresses: []*ec2.Address{
+						{
+							AssociationId: aws.String("association-id-1"),
+							PublicIp:      aws.String("public-ip"),
+							AllocationId:  aws.String("allocation-id"),
+						},
+					},
+				}, nil)
+				m.DisassociateAddress(gomock.AssignableToTypeOf(&ec2.DisassociateAddressInput{})).Return(nil, nil)
+				m.ReleaseAddress(gomock.AssignableToTypeOf(&ec2.ReleaseAddressInput{})).Return(nil, nil)
+			},
+		},
+		{
+			name: "Should retry if unable to release the IP address because of Auth Failure",
+			expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
+				m.DescribeAddresses(gomock.AssignableToTypeOf(&ec2.DescribeAddressesInput{})).Return(&ec2.DescribeAddressesOutput{
+					Addresses: []*ec2.Address{
+						{
+							AssociationId: aws.String("association-id-1"),
+							PublicIp:      aws.String("public-ip"),
+							AllocationId:  aws.String("allocation-id"),
+						},
+					},
+				}, nil)
+				m.DisassociateAddress(gomock.AssignableToTypeOf(&ec2.DisassociateAddressInput{})).Return(nil, nil).Times(2)
+				m.ReleaseAddress(gomock.AssignableToTypeOf(&ec2.ReleaseAddressInput{})).Return(nil, awserr.New(awserrors.AuthFailure, awserrors.AuthFailure, errors.Errorf(awserrors.AuthFailure)))
+				m.ReleaseAddress(gomock.AssignableToTypeOf(&ec2.ReleaseAddressInput{})).Return(nil, nil)
+			},
+		},
+		{
+			name: "Should retry if unable to release the IP address because IP is already in use",
+			expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
+				m.DescribeAddresses(gomock.AssignableToTypeOf(&ec2.DescribeAddressesInput{})).Return(&ec2.DescribeAddressesOutput{
+					Addresses: []*ec2.Address{
+						{
+							AssociationId: aws.String("association-id-1"),
+							PublicIp:      aws.String("public-ip"),
+							AllocationId:  aws.String("allocation-id"),
+						},
+					},
+				}, nil)
+				m.DisassociateAddress(gomock.AssignableToTypeOf(&ec2.DisassociateAddressInput{})).Return(nil, nil).Times(2)
+				m.ReleaseAddress(gomock.AssignableToTypeOf(&ec2.ReleaseAddressInput{})).Return(nil, awserr.New(awserrors.InUseIPAddress, awserrors.InUseIPAddress, errors.Errorf(awserrors.InUseIPAddress)))
+				m.ReleaseAddress(gomock.AssignableToTypeOf(&ec2.ReleaseAddressInput{})).Return(nil, nil)
+			},
+		},
+		{
+			name: "Should not retry if unable to release the IP address due to dependency failure",
+			expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
+				m.DescribeAddresses(gomock.AssignableToTypeOf(&ec2.DescribeAddressesInput{})).Return(&ec2.DescribeAddressesOutput{
+					Addresses: []*ec2.Address{
+						{
+							AssociationId: aws.String("association-id-1"),
+							PublicIp:      aws.String("public-ip"),
+							AllocationId:  aws.String("allocation-id"),
+						},
+					},
+				}, nil)
+				m.DisassociateAddress(gomock.AssignableToTypeOf(&ec2.DisassociateAddressInput{})).Return(nil, nil).Times(2)
+				m.ReleaseAddress(gomock.AssignableToTypeOf(&ec2.ReleaseAddressInput{})).Return(nil, awserr.New("dependency-failure", "dependency-failure", errors.Errorf("dependency-failure")))
+			},
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			scheme := runtime.NewScheme()
+			err := infrav1.AddToScheme(scheme)
+			g.Expect(err).NotTo(HaveOccurred())
+			client := fake.NewClientBuilder().WithScheme(scheme).Build()
+
+			ec2Mock := mock_ec2iface.NewMockEC2API(mockCtrl)
+
+			cs, err := scope.NewClusterScope(scope.ClusterScopeParams{
+				Client:     client,
+				Cluster:    &clusterv1.Cluster{},
+				AWSCluster: &infrav1.AWSCluster{},
+			})
+			g.Expect(err).NotTo(HaveOccurred())
+
+			s := NewService(cs)
+			s.EC2Client = ec2Mock
+
+			if tt.expect != nil {
+				tt.expect(ec2Mock.EXPECT())
+			}
+
+			if err := s.releaseAddresses(); (err != nil) != tt.wantErr {
+				t.Errorf("releaseAddresses() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}