Repository: kubernetes-sigs/cluster-api-provider-aws
Pull request #3110 (2022-01-27 23:24:08)

Remove unnecessary calls to AWS API during subnet deletion
**What type of PR is this?**
/kind cleanup

**What this PR does / why we need it**:
`deleteSubnets` function calls `describeVpcSubnets` that makes multiple calls to AWS API although it only needs subnet IDs. 
Refactored the code to remove unnecessary calls, DescribeRouteTables and DescribeNatGatewaysPages, to AWS API during subnet deletion.

I also set up tests for deleteSubnets, which was missing. Plan to add more test cases as part of #3074.

**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**:

**Checklist**:
- [ ] squashed commits
- [ ] includes documentation
- [x] adds unit tests
- [ ] adds or updates e2e tests

**Release note**:
```release-note
NONE
```


k8s-ci-robot: @pydctw: 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>
pydctw: /test?
k8s-ci-robot: @pydctw: The following commands are available to trigger required jobs:
* `/test pull-cluster-api-provider-aws-build`
* `/test pull-cluster-api-provider-aws-test`
* `/test pull-cluster-api-provider-aws-verify`

The following commands are available to trigger optional jobs:
* `/test pull-cluster-api-provider-aws-e2e`
* `/test pull-cluster-api-provider-aws-e2e-blocking`
* `/test pull-cluster-api-provider-aws-e2e-conformance`
* `/test pull-cluster-api-provider-aws-e2e-conformance-with-ci-artifacts`
* `/test pull-cluster-api-provider-aws-e2e-eks`

Use `/test all` to run the following jobs that were automatically triggered:
* `pull-cluster-api-provider-aws-build`
* `pull-cluster-api-provider-aws-test`
* `pull-cluster-api-provider-aws-verify`



<details>

In response to [this](https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/3110#issuecomment-1023733351):

>/test?


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>
pydctw: /test pull-cluster-api-provider-aws-e2e
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: @pydctw: 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 | 885c978a39cb57ea350f6ae1bf51ae547ea8d757 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-aws/3110/pull-cluster-api-provider-aws-test/1486842493889679360) | 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=3110). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Apydctw). 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 -->
pydctw: /test pull-cluster-api-provider-aws-e2e
sedefsavas: /lgtm
/approve
k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/3110#issuecomment-1024032060" title="Approved">sedefsavas</a>*

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

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

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

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

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":[]} -->

Patch:
diff --git a/pkg/cloud/services/network/subnets.go b/pkg/cloud/services/network/subnets.go
index daf8bf302..1dde1b69b 100644
--- a/pkg/cloud/services/network/subnets.go
+++ b/pkg/cloud/services/network/subnets.go
@@ -248,13 +248,13 @@ func (s *Service) deleteSubnets() error {
 	}
 
 	// Describe subnets in the vpc.
-	existing, err := s.describeVpcSubnets()
+	existing, err := s.describeSubnets()
 	if err != nil {
 		return err
 	}
 
-	for _, sn := range existing {
-		if err := s.deleteSubnet(sn.ID); err != nil {
+	for _, sn := range existing.Subnets {
+		if err := s.deleteSubnet(aws.StringValue(sn.SubnetId)); err != nil {
 			return err
 		}
 	}
@@ -263,22 +263,9 @@ func (s *Service) deleteSubnets() error {
 }
 
 func (s *Service) describeVpcSubnets() (infrav1.Subnets, error) {
-	input := &ec2.DescribeSubnetsInput{
-		Filters: []*ec2.Filter{
-			filter.EC2.SubnetStates(ec2.SubnetStatePending, ec2.SubnetStateAvailable),
-		},
-	}
-
-	if s.scope.VPC().ID == "" {
-		input.Filters = append(input.Filters, filter.EC2.Cluster(s.scope.Name()))
-	} else {
-		input.Filters = append(input.Filters, filter.EC2.VPC(s.scope.VPC().ID))
-	}
-
-	out, err := s.EC2Client.DescribeSubnets(input)
+	sns, err := s.describeSubnets()
 	if err != nil {
-		record.Eventf(s.scope.InfraCluster(), "FailedDescribeSubnet", "Failed to describe subnets in vpc %q: %v", s.scope.VPC().ID, err)
-		return nil, errors.Wrapf(err, "failed to describe subnets in vpc %q", s.scope.VPC().ID)
+		return nil, err
 	}
 
 	routeTables, err := s.describeVpcRouteTablesBySubnet()
@@ -291,10 +278,10 @@ func (s *Service) describeVpcSubnets() (infrav1.Subnets, error) {
 		return nil, err
 	}
 
-	subnets := make([]infrav1.SubnetSpec, 0, len(out.Subnets))
+	subnets := make([]infrav1.SubnetSpec, 0, len(sns.Subnets))
 	// Besides what the AWS API tells us directly about the subnets, we also want to discover whether the subnet is "public" (i.e. directly connected to the internet) and if there are any associated NAT gateways.
 	// We also look for a tag indicating that a particular subnet should be public, to try and determine whether a managed VPC's subnet should have such a route, but does not.
-	for _, ec2sn := range out.Subnets {
+	for _, ec2sn := range sns.Subnets {
 		spec := infrav1.SubnetSpec{
 			ID:               *ec2sn.SubnetId,
 			CidrBlock:        *ec2sn.CidrBlock,
@@ -332,6 +319,27 @@ func (s *Service) describeVpcSubnets() (infrav1.Subnets, error) {
 	return subnets, nil
 }
 
+func (s *Service) describeSubnets() (*ec2.DescribeSubnetsOutput, error) {
+	input := &ec2.DescribeSubnetsInput{
+		Filters: []*ec2.Filter{
+			filter.EC2.SubnetStates(ec2.SubnetStatePending, ec2.SubnetStateAvailable),
+		},
+	}
+
+	if s.scope.VPC().ID == "" {
+		input.Filters = append(input.Filters, filter.EC2.Cluster(s.scope.Name()))
+	} else {
+		input.Filters = append(input.Filters, filter.EC2.VPC(s.scope.VPC().ID))
+	}
+
+	out, err := s.EC2Client.DescribeSubnets(input)
+	if err != nil {
+		record.Eventf(s.scope.InfraCluster(), "FailedDescribeSubnet", "Failed to describe subnets in vpc %q: %v", s.scope.VPC().ID, err)
+		return nil, errors.Wrapf(err, "failed to describe subnets in vpc %q", s.scope.VPC().ID)
+	}
+	return out, nil
+}
+
 func (s *Service) createSubnet(sn *infrav1.SubnetSpec) (*infrav1.SubnetSpec, error) {
 	out, err := s.EC2Client.CreateSubnet(&ec2.CreateSubnetInput{
 		VpcId:            aws.String(s.scope.VPC().ID),


Test patch:
diff --git a/pkg/cloud/services/network/subnets_test.go b/pkg/cloud/services/network/subnets_test.go
index 95c49807c..ca3f8ee98 100644
--- a/pkg/cloud/services/network/subnets_test.go
+++ b/pkg/cloud/services/network/subnets_test.go
@@ -1740,3 +1740,117 @@ func TestDiscoverSubnets(t *testing.T) {
 		})
 	}
 }
+
+func TestDeleteSubnets(t *testing.T) {
+	testCases := []struct {
+		name          string
+		input         *infrav1.NetworkSpec
+		expect        func(m *mock_ec2iface.MockEC2APIMockRecorder)
+		errorExpected bool
+	}{
+		{
+			name: "managed vpc - success",
+			input: &infrav1.NetworkSpec{
+				VPC: infrav1.VPCSpec{
+					ID: subnetsVPCID,
+					Tags: infrav1.Tags{
+						infrav1.ClusterTagKey("test-cluster"): "owned",
+					},
+				},
+				Subnets: []infrav1.SubnetSpec{
+					{
+						ID: "subnet-1",
+					},
+					{
+						ID: "subnet-2",
+					},
+				},
+			},
+			expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
+				m.DescribeSubnets(gomock.Eq(&ec2.DescribeSubnetsInput{
+					Filters: []*ec2.Filter{
+						{
+							Name:   aws.String("state"),
+							Values: []*string{aws.String("pending"), aws.String("available")},
+						},
+						{
+							Name:   aws.String("vpc-id"),
+							Values: []*string{aws.String(subnetsVPCID)},
+						},
+					},
+				})).
+					Return(&ec2.DescribeSubnetsOutput{
+						Subnets: []*ec2.Subnet{
+							{
+								VpcId:               aws.String(subnetsVPCID),
+								SubnetId:            aws.String("subnet-1"),
+								AvailabilityZone:    aws.String("us-east-1a"),
+								CidrBlock:           aws.String("10.0.10.0/24"),
+								MapPublicIpOnLaunch: aws.Bool(true),
+							},
+							{
+								VpcId:               aws.String(subnetsVPCID),
+								SubnetId:            aws.String("subnet-2"),
+								AvailabilityZone:    aws.String("us-east-1a"),
+								CidrBlock:           aws.String("10.0.20.0/24"),
+								MapPublicIpOnLaunch: aws.Bool(false),
+							},
+						},
+					}, nil)
+
+				m.DeleteSubnet(&ec2.DeleteSubnetInput{
+					SubnetId: aws.String("subnet-1"),
+				}).
+					Return(nil, nil)
+
+				m.DeleteSubnet(&ec2.DeleteSubnetInput{
+					SubnetId: aws.String("subnet-2"),
+				}).
+					Return(nil, nil)
+			},
+			errorExpected: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			mockCtrl := gomock.NewController(t)
+			defer mockCtrl.Finish()
+
+			ec2Mock := mock_ec2iface.NewMockEC2API(mockCtrl)
+
+			scheme := runtime.NewScheme()
+			_ = infrav1.AddToScheme(scheme)
+
+			client := fake.NewClientBuilder().WithScheme(scheme).Build()
+			scope, err := scope.NewClusterScope(scope.ClusterScopeParams{
+				Client: client,
+				Cluster: &clusterv1.Cluster{
+					ObjectMeta: metav1.ObjectMeta{Name: "test-cluster"},
+				},
+				AWSCluster: &infrav1.AWSCluster{
+					ObjectMeta: metav1.ObjectMeta{Name: "test"},
+					Spec: infrav1.AWSClusterSpec{
+						NetworkSpec: *tc.input,
+					},
+				},
+			})
+			if err != nil {
+				t.Fatalf("Failed to create test context: %v", err)
+			}
+
+			tc.expect(ec2Mock.EXPECT())
+
+			s := NewService(scope)
+			s.EC2Client = ec2Mock
+
+			err = s.deleteSubnets()
+			if tc.errorExpected && err == nil {
+				t.Fatal("expected error but not no error")
+			}
+			if !tc.errorExpected && err != nil {
+				t.Fatalf("got an unexpected error: %v", err)
+			}
+		})
+	}
+}