Repository: kubernetes-sigs/cluster-api-provider-aws
Pull request #3197 (2022-02-11 06:43:59)

Added unit tests for pkg/cloud/services/network/natgateway package
<!-- 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**:
This PR increases the unit tests coverage for pkg/cloud/services/network/natgateway package.


<!-- 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
- [x] 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
Add unit tests for `pkg/cloud/services/network/natgateway` package
```


k8s-ci-robot: @Madhur97: 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: Hi @Madhur97. Thanks for your PR.

I'm waiting for a [kubernetes-sigs](https://github.com/orgs/kubernetes-sigs/people) member to verify that this patch is reasonable to test. If it is, they should reply with `/ok-to-test` on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should [join the org](https://git.k8s.io/community/community-membership.md#member) to skip this step.

Once the patch is verified, the new status will be reflected by the `ok-to-test` label.

I understand the commands that are listed [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fcluster-api-provider-aws).

<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>

Madhur97: Tests are failing in this PR due to [this](https://github.com/kubernetes-sigs/cluster-api-provider-aws/issues/3204) issue
Ankitasw: /ok-to-test
k8s-ci-robot: @Madhur97: PR needs rebase.

<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>

Patch:
diff --git a/pkg/cloud/services/network/natgateways.go b/pkg/cloud/services/network/natgateways.go
index 5cf79938b..15ccbb5b3 100644
--- a/pkg/cloud/services/network/natgateways.go
+++ b/pkg/cloud/services/network/natgateways.go
@@ -286,7 +286,7 @@ func (s *Service) deleteNatGateway(id string) error {
 			return false, err
 		}
 
-		if len(out.NatGateways) == 0 {
+		if out == nil || len(out.NatGateways) == 0 {
 			return false, errors.New(fmt.Sprintf("no NAT gateway returned for id %q", id))
 		}
 


Test patch:
diff --git a/pkg/cloud/services/network/natgateways_test.go b/pkg/cloud/services/network/natgateways_test.go
index eb91dd8ac..599e9f33e 100644
--- a/pkg/cloud/services/network/natgateways_test.go
+++ b/pkg/cloud/services/network/natgateways_test.go
@@ -23,11 +23,13 @@ import (
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/golang/mock/gomock"
+	. "github.com/onsi/gomega"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"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"
@@ -404,3 +406,325 @@ func TestReconcileNatGateways(t *testing.T) {
 		})
 	}
 }
+
+func TestDeleteNatGateways(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	testCases := []struct {
+		name           string
+		input          []infrav1.SubnetSpec
+		isUnmanagedVPC bool
+		expect         func(m *mock_ec2iface.MockEC2APIMockRecorder)
+		wantErr        bool
+	}{
+		{
+			name:           "Should skip deletion if vpc is unmanaged",
+			isUnmanagedVPC: true,
+		},
+		{
+			name: "Should skip deletion if no private subnet is present",
+			input: []infrav1.SubnetSpec{
+				{
+					ID:               "subnet-1",
+					AvailabilityZone: "us-east-1a",
+					CidrBlock:        "10.0.10.0/24",
+					IsPublic:         true,
+				},
+			},
+		},
+		{
+			name: "Should skip deletion if no public subnet is present",
+			input: []infrav1.SubnetSpec{
+				{
+					ID:               "subnet-1",
+					AvailabilityZone: "us-east-1a",
+					CidrBlock:        "10.0.10.0/24",
+					IsPublic:         false,
+				},
+			},
+		},
+		{
+			name: "Should skip deletion if no existing natgateway present",
+			input: []infrav1.SubnetSpec{
+				{
+					ID:               "subnet-1",
+					AvailabilityZone: "us-east-1a",
+					CidrBlock:        "10.0.10.0/24",
+					IsPublic:         true,
+				},
+				{
+					ID:               "subnet-2",
+					AvailabilityZone: "us-east-1a",
+					CidrBlock:        "10.0.12.0/24",
+					IsPublic:         false,
+				},
+			},
+			expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
+				m.DescribeNatGatewaysPages(
+					gomock.Eq(&ec2.DescribeNatGatewaysInput{
+						Filter: []*ec2.Filter{
+							{
+								Name:   aws.String("vpc-id"),
+								Values: []*string{aws.String("managed-vpc")},
+							},
+							{
+								Name:   aws.String("state"),
+								Values: []*string{aws.String("pending"), aws.String("available")},
+							},
+						},
+					}),
+					gomock.Any()).Return(nil)
+			},
+		},
+		{
+			name: "Should successfully delete natgateways",
+			input: []infrav1.SubnetSpec{
+				{
+					ID:               "subnet-1",
+					AvailabilityZone: "us-east-1a",
+					CidrBlock:        "10.0.10.0/24",
+					IsPublic:         true,
+				},
+				{
+					ID:               "subnet-2",
+					AvailabilityZone: "us-east-1a",
+					CidrBlock:        "10.0.12.0/24",
+					IsPublic:         false,
+				},
+			},
+			expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
+				m.DescribeNatGatewaysPages(
+					gomock.AssignableToTypeOf(&ec2.DescribeNatGatewaysInput{}),
+					gomock.Any()).Do(mockDescribeNatGatewaysOutput).Return(nil)
+
+				m.DeleteNatGateway(gomock.Eq(&ec2.DeleteNatGatewayInput{
+					NatGatewayId: aws.String("natgateway"),
+				})).Return(&ec2.DeleteNatGatewayOutput{}, nil)
+
+				m.DescribeNatGateways(gomock.Eq(&ec2.DescribeNatGatewaysInput{
+					NatGatewayIds: []*string{aws.String("natgateway")},
+				})).Return(&ec2.DescribeNatGatewaysOutput{
+					NatGateways: []*ec2.NatGateway{
+						{
+							State: aws.String("available"),
+						},
+					},
+				}, nil)
+				m.DescribeNatGateways(gomock.AssignableToTypeOf(&ec2.DescribeNatGatewaysInput{})).Return(&ec2.DescribeNatGatewaysOutput{
+					NatGateways: []*ec2.NatGateway{
+						{
+							State: aws.String("deleted"),
+						},
+					},
+				}, nil)
+			},
+		},
+		{
+			name: "Should return error if natgateway has unknown state",
+			input: []infrav1.SubnetSpec{
+				{
+					ID:               "subnet-1",
+					AvailabilityZone: "us-east-1a",
+					CidrBlock:        "10.0.10.0/24",
+					IsPublic:         true,
+				},
+				{
+					ID:               "",
+					AvailabilityZone: "us-east-1a",
+					CidrBlock:        "10.0.12.0/24",
+					IsPublic:         true,
+				},
+				{
+					ID:               "subnet-3",
+					AvailabilityZone: "us-east-1a",
+					CidrBlock:        "10.0.14.0/24",
+					IsPublic:         false,
+				},
+			},
+			expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
+				m.DescribeNatGatewaysPages(
+					gomock.AssignableToTypeOf(&ec2.DescribeNatGatewaysInput{}), gomock.Any()).Do(mockDescribeNatGatewaysOutput).Return(nil)
+
+				m.DeleteNatGateway(gomock.Eq(&ec2.DeleteNatGatewayInput{
+					NatGatewayId: aws.String("natgateway"),
+				})).Return(&ec2.DeleteNatGatewayOutput{}, nil)
+
+				m.DescribeNatGateways(gomock.Eq(&ec2.DescribeNatGatewaysInput{
+					NatGatewayIds: []*string{aws.String("natgateway")},
+				})).Return(&ec2.DescribeNatGatewaysOutput{
+					NatGateways: []*ec2.NatGateway{
+						{
+							State: aws.String("unknown"),
+						},
+					},
+				}, nil)
+			},
+			wantErr: true,
+		},
+		{
+			name: "Should return error if describe natgateway output and error, both are nil",
+			input: []infrav1.SubnetSpec{
+				{
+					ID:               "subnet-1",
+					AvailabilityZone: "us-east-1a",
+					CidrBlock:        "10.0.10.0/24",
+					IsPublic:         true,
+				},
+				{
+					ID:               "subnet-2",
+					AvailabilityZone: "us-east-1a",
+					CidrBlock:        "10.0.14.0/24",
+					IsPublic:         false,
+				},
+			},
+			expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
+				m.DescribeNatGatewaysPages(
+					gomock.AssignableToTypeOf(&ec2.DescribeNatGatewaysInput{}), gomock.Any()).Do(mockDescribeNatGatewaysOutput).Return(nil)
+
+				m.DeleteNatGateway(gomock.Eq(&ec2.DeleteNatGatewayInput{
+					NatGatewayId: aws.String("natgateway"),
+				})).Return(&ec2.DeleteNatGatewayOutput{}, nil)
+
+				m.DescribeNatGateways(gomock.Eq(&ec2.DescribeNatGatewaysInput{
+					NatGatewayIds: []*string{aws.String("natgateway")},
+				})).Return(nil, nil)
+			},
+			wantErr: true,
+		},
+		{
+			name: "Should return error if describe natgateway pages fails",
+			input: []infrav1.SubnetSpec{
+				{
+					ID:               "subnet-1",
+					AvailabilityZone: "us-east-1a",
+					CidrBlock:        "10.0.10.0/24",
+					IsPublic:         true,
+				},
+				{
+					ID:               "subnet-2",
+					AvailabilityZone: "us-east-1a",
+					CidrBlock:        "10.0.14.0/24",
+					IsPublic:         false,
+				},
+			},
+			expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
+				m.DescribeNatGatewaysPages(
+					gomock.AssignableToTypeOf(&ec2.DescribeNatGatewaysInput{}), gomock.Any()).Return(awserrors.NewFailedDependency("failed dependency"))
+			},
+			wantErr: true,
+		},
+		{
+			name: "Should return error if delete natgateway fails",
+			input: []infrav1.SubnetSpec{
+				{
+					ID:               "subnet-1",
+					AvailabilityZone: "us-east-1a",
+					CidrBlock:        "10.0.10.0/24",
+					IsPublic:         true,
+				},
+				{
+					ID:               "subnet-2",
+					AvailabilityZone: "us-east-1a",
+					CidrBlock:        "10.0.14.0/24",
+					IsPublic:         false,
+				},
+			},
+			expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
+				m.DescribeNatGatewaysPages(
+					gomock.AssignableToTypeOf(&ec2.DescribeNatGatewaysInput{}), gomock.Any()).Do(mockDescribeNatGatewaysOutput).Return(nil)
+
+				m.DeleteNatGateway(gomock.Eq(&ec2.DeleteNatGatewayInput{
+					NatGatewayId: aws.String("natgateway"),
+				})).Return(nil, awserrors.NewFailedDependency("failed dependency"))
+			},
+			wantErr: true,
+		},
+		{
+			name: "Should return error if describe natgateway fails",
+			input: []infrav1.SubnetSpec{
+				{
+					ID:               "subnet-1",
+					AvailabilityZone: "us-east-1a",
+					CidrBlock:        "10.0.10.0/24",
+					IsPublic:         true,
+				},
+				{
+					ID:               "subnet-2",
+					AvailabilityZone: "us-east-1a",
+					CidrBlock:        "10.0.14.0/24",
+					IsPublic:         false,
+				},
+			},
+			expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
+				m.DescribeNatGatewaysPages(
+					gomock.AssignableToTypeOf(&ec2.DescribeNatGatewaysInput{}), gomock.Any()).Do(mockDescribeNatGatewaysOutput).Return(nil)
+
+				m.DeleteNatGateway(gomock.Eq(&ec2.DeleteNatGatewayInput{
+					NatGatewayId: aws.String("natgateway"),
+				})).Return(&ec2.DeleteNatGatewayOutput{}, nil)
+
+				m.DescribeNatGateways(gomock.Eq(&ec2.DescribeNatGatewaysInput{
+					NatGatewayIds: []*string{aws.String("natgateway")},
+				})).Return(nil, awserrors.NewNotFound("not found"))
+			},
+			wantErr: true,
+		},
+	}
+
+	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)
+			awsCluster := &infrav1.AWSCluster{
+				ObjectMeta: metav1.ObjectMeta{Name: "test"},
+				Spec: infrav1.AWSClusterSpec{
+					NetworkSpec: infrav1.NetworkSpec{
+						VPC: infrav1.VPCSpec{
+							ID: "managed-vpc",
+							Tags: infrav1.Tags{
+								infrav1.ClusterTagKey("test-cluster"): "owned",
+							},
+						},
+						Subnets: tc.input,
+					},
+				},
+			}
+			if tc.isUnmanagedVPC {
+				awsCluster.Spec.NetworkSpec.VPC.Tags = infrav1.Tags{}
+			}
+			client := fake.NewClientBuilder().WithScheme(scheme).Build()
+			clusterScope, err := scope.NewClusterScope(scope.ClusterScopeParams{
+				Cluster: &clusterv1.Cluster{
+					ObjectMeta: metav1.ObjectMeta{Name: "test-cluster"},
+				},
+				AWSCluster: awsCluster,
+				Client:     client,
+			})
+			g.Expect(err).NotTo(HaveOccurred())
+			if tc.expect != nil {
+				tc.expect(ec2Mock.EXPECT())
+			}
+
+			s := NewService(clusterScope)
+			s.EC2Client = ec2Mock
+
+			err = s.deleteNatGateways()
+			if tc.wantErr {
+				g.Expect(err).To(HaveOccurred())
+				return
+			}
+			g.Expect(err).NotTo(HaveOccurred())
+		})
+	}
+}
+
+var mockDescribeNatGatewaysOutput = func(_, y interface{}) {
+	funct := y.(func(page *ec2.DescribeNatGatewaysOutput, lastPage bool) bool)
+	funct(&ec2.DescribeNatGatewaysOutput{NatGateways: []*ec2.NatGateway{{
+		NatGatewayId: aws.String("natgateway"),
+		SubnetId:     aws.String("subnet-1"),
+	}}}, true)
+}