Repository: kubernetes-sigs/cluster-api-provider-aws
Pull request #3483 (2022-05-18 18:15:05)

fix: Do not change default security groups during EKS control plane reconcile
<!-- 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?**

<!--
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
-->
/kind bug

**What this PR does / why we need it**:
For EKS clusters, #3028 enabled the bastion security group only when the bastion is enabled. However, the implementation introduced a bug: every reconcile appends to a package-level variable!

<!-- 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)*:
Fixes #

**Special notes for your reviewer**:
I'd like to add a pair of unit tests, but haven't done that yet.

**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


k8s-ci-robot: @dlipovetsky: 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 **enxebre** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @enxebre` 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":["enxebre"]} -->
pydctw: The changes looks fine. As you mentioned, it will be great to have a unit test for this.

Also, I am interested to learn how you found the bug.
dlipovetsky: > The changes looks fine. As you mentioned, it will be great to have a unit test for this.

I'll work on one.

> Also, I am interested to learn how you found the bug.

I wanted to add the "no bastion security group" feature to the EC2 cluster flavor (https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/3482), and to do that I read through this code. That's when I spotted it.

Package-level variables can be tricky to use. There's even a linter (https://github.com/leighmcculloch/gochecknoglobals) for them.

A constant would be great, but we can't have one.

I could go a bit further in this PR: I could move the variable into a function, and replace every variable reference with a function call. I don't foresee a noticeable impact on performance.
sedefsavas: LGTM as is.
dlipovetsky: I've made some progress on creating a unit test for this, and I'd appreciate some more time to finish it.

The AWSManagedControlPlane controller did not have existing tests, so it's taken me some time to set up the scaffolding.
dlipovetsky: I wanted to create a failing test for the bug without modifying the implementation, but the only way to do that is to complete two normal reconciles of the AWSManagedControlPlane. To test a normal reconcile, I would have to mock various AWS services, and that would require extensive changes to AWSManagedControlPlane.

If that's something we want anyway, I can contribute it in a separate PR, and then use it to create the failing test. But given the bug is relatively easy to understand, I think we can choose to skip a failing test.
dlipovetsky: I'm sorry, this wasn't a clean update to the PR. I should have added another commit with the unit test, but instead I squashed it into the existing commit, _and_ I rebased on main :disappointed:.
k8s-ci-robot: @dlipovetsky: 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 | 5c46749c625d54586fe294398d7c721a777b69fb | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-aws/3483/pull-cluster-api-provider-aws-test/1545476246383628288) | 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=3483). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Adlipovetsky). 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 -->
dlipovetsky: It would be good to get this fix in to release 1.5

/cherry-pick release-1.5
k8s-infra-cherrypick-robot: @dlipovetsky: once the present PR merges, I will cherry-pick it on top of release-1.5 in a new PR and assign it to you.

<details>

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

>It would be good to get this fix in to release 1.5
>
>/cherry-pick release-1.5


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>
sedefsavas: /lgtm
richardcase: /milestone v1.5.0
richardcase: /test pull-cluster-api-provider-aws-e2e-eks
richardcase: This change looks good, thanks @dlipovetsky 👍

The e2e tests are passing as well.

/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/3483#issuecomment-1195984953" title="Approved">richardcase</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)~~ [richardcase]

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":[]} -->
k8s-ci-robot: @dlipovetsky: 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 | 4765025731f595b69795b850202e5607a2f5eb76 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-aws/3483/pull-cluster-api-provider-aws-test/1552038841831919616) | unknown | `/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=3483). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Adlipovetsky). 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 -->
dlipovetsky: I'll take a look at the test failure.
dlipovetsky: The linter is complaining. It wasn't earlier, not sure what's changed.
dlipovetsky: The test uses golangci-lint v1.47.2

```
make -C hack/tools bin/golangci-lint
make[1]: Entering directory '/home/prow/go/src/sigs.k8s.io/cluster-api-provider-aws/hack/tools'
mkdir -p bin
go build -tags=tools -o bin/golangci-lint github.com/golangci/golangci-lint/cmd/golangci-lint
go: downloading github.com/golangci/golangci-lint v1.47.2
```

But the branch uses v1.45.2:

```
> grep golangci-lint hack/tools/go.mod
	github.com/golangci/golangci-lint v1.45.2
```

I am surprised. The only way to merge this, or any PR that hits this issue, is to rebase on `main`. Frustratingly, that removes the lgtm label.

Patch:
diff --git a/controlplane/eks/controllers/awsmanagedcontrolplane_controller.go b/controlplane/eks/controllers/awsmanagedcontrolplane_controller.go
index 9c3a1e101..8ee9b31b0 100644
--- a/controlplane/eks/controllers/awsmanagedcontrolplane_controller.go
+++ b/controlplane/eks/controllers/awsmanagedcontrolplane_controller.go
@@ -57,11 +57,21 @@ const (
 	deleteRequeueAfter = 20 * time.Second
 )
 
-var (
-	eksSecurityGroupRoles = []infrav1.SecurityGroupRole{
-		infrav1.SecurityGroupEKSNodeAdditional,
+var defaultEKSSecurityGroupRoles = []infrav1.SecurityGroupRole{
+	infrav1.SecurityGroupEKSNodeAdditional,
+}
+
+// securityGroupRolesForControlPlane returns the security group roles determined by the control plane configuration.
+func securityGroupRolesForControlPlane(scope *scope.ManagedControlPlaneScope) []infrav1.SecurityGroupRole {
+	// Copy to ensure we do not modify the package-level variable.
+	roles := make([]infrav1.SecurityGroupRole, len(defaultEKSSecurityGroupRoles))
+	copy(roles, defaultEKSSecurityGroupRoles)
+
+	if scope.Bastion().Enabled {
+		roles = append(roles, infrav1.SecurityGroupBastion)
 	}
-)
+	return roles
+}
 
 // AWSManagedControlPlaneReconciler reconciles a AWSManagedControlPlane object.
 type AWSManagedControlPlaneReconciler struct {
@@ -202,14 +212,10 @@ func (r *AWSManagedControlPlaneReconciler) reconcileNormal(ctx context.Context,
 		return ctrl.Result{}, err
 	}
 
-	if awsManagedControlPlane.Spec.Bastion.Enabled {
-		eksSecurityGroupRoles = append(eksSecurityGroupRoles, infrav1.SecurityGroupBastion)
-	}
-
 	ec2Service := ec2.NewService(managedScope)
 	networkSvc := network.NewService(managedScope)
 	ekssvc := eks.NewService(managedScope)
-	sgService := securitygroup.NewService(managedScope, eksSecurityGroupRoles)
+	sgService := securitygroup.NewService(managedScope, securityGroupRolesForControlPlane(managedScope))
 	authService := iamauth.NewService(managedScope, iamauth.BackendTypeConfigMap, managedScope.Client)
 	awsnodeService := awsnode.NewService(managedScope)
 	kubeproxyService := kubeproxy.NewService(managedScope)
@@ -277,7 +283,7 @@ func (r *AWSManagedControlPlaneReconciler) reconcileDelete(ctx context.Context,
 	ekssvc := eks.NewService(managedScope)
 	ec2svc := ec2.NewService(managedScope)
 	networkSvc := network.NewService(managedScope)
-	sgService := securitygroup.NewService(managedScope, eksSecurityGroupRoles)
+	sgService := securitygroup.NewService(managedScope, securityGroupRolesForControlPlane(managedScope))
 
 	if err := ekssvc.DeleteControlPlane(); err != nil {
 		log.Error(err, "error deleting EKS cluster for EKS control plane", "namespace", controlPlane.Namespace, "name", controlPlane.Name)


Test patch:
diff --git a/controlplane/eks/controllers/awsmanagedcontrolplane_controller_unit_test.go b/controlplane/eks/controllers/awsmanagedcontrolplane_controller_unit_test.go
new file mode 100644
index 000000000..059d93f3d
--- /dev/null
+++ b/controlplane/eks/controllers/awsmanagedcontrolplane_controller_unit_test.go
@@ -0,0 +1,63 @@
+/*
+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 controllers
+
+import (
+	"testing"
+
+	. "github.com/onsi/gomega"
+
+	infrav1 "sigs.k8s.io/cluster-api-provider-aws/api/v1beta1"
+)
+
+func TestSecurityGroupRolesForCluster(t *testing.T) {
+	tests := []struct {
+		name           string
+		bastionEnabled bool
+	}{
+		{
+			name:           "Should use bastion security group when bastion is enabled",
+			bastionEnabled: true,
+		},
+		{
+			name:           "Should not use bastion security group when bastion is disabled",
+			bastionEnabled: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			c := getAWSManagedControlPlane("test", "test")
+			c.Spec.Bastion.Enabled = tt.bastionEnabled
+			s, err := getManagedControlPlaneScope(c)
+			g.Expect(err).To(BeNil(), "failed to create cluster scope for test")
+
+			got := securityGroupRolesForControlPlane(s)
+			if tt.bastionEnabled {
+				g.Expect(got).To(ContainElement(infrav1.SecurityGroupBastion))
+			} else {
+				g.Expect(got).ToNot(ContainElement(infrav1.SecurityGroupBastion))
+			}
+
+			// Verify that function does not modify the package-level variable.
+			gotAgain := securityGroupRolesForControlPlane(s)
+			g.Expect(gotAgain).To(BeEquivalentTo(got), "two identical calls return different values")
+		})
+	}
+}
diff --git a/controlplane/eks/controllers/helpers_test.go b/controlplane/eks/controllers/helpers_test.go
new file mode 100644
index 000000000..46de9c556
--- /dev/null
+++ b/controlplane/eks/controllers/helpers_test.go
@@ -0,0 +1,80 @@
+/*
+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 controllers
+
+import (
+	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"
+	ekscontrolplanev1 "sigs.k8s.io/cluster-api-provider-aws/controlplane/eks/api/v1beta1"
+	"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/scope"
+	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+)
+
+func getAWSManagedControlPlane(name, namespace string) ekscontrolplanev1.AWSManagedControlPlane {
+	return ekscontrolplanev1.AWSManagedControlPlane{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      name,
+			Namespace: namespace,
+		},
+		Spec: ekscontrolplanev1.AWSManagedControlPlaneSpec{
+			Region: "us-east-1",
+			NetworkSpec: infrav1.NetworkSpec{
+				VPC: infrav1.VPCSpec{
+					ID:        "vpc-exists",
+					CidrBlock: "10.0.0.0/8",
+				},
+				Subnets: infrav1.Subnets{
+					{
+						ID:               "subnet-1",
+						AvailabilityZone: "us-east-1a",
+						CidrBlock:        "10.0.10.0/24",
+						IsPublic:         false,
+					},
+					{
+						ID:               "subnet-2",
+						AvailabilityZone: "us-east-1c",
+						CidrBlock:        "10.0.11.0/24",
+						IsPublic:         true,
+					},
+				},
+				SecurityGroupOverrides: map[infrav1.SecurityGroupRole]string{},
+			},
+			Bastion: infrav1.Bastion{Enabled: true},
+		},
+	}
+}
+
+func getManagedControlPlaneScope(cp ekscontrolplanev1.AWSManagedControlPlane) (*scope.ManagedControlPlaneScope, error) {
+	scheme := runtime.NewScheme()
+	_ = ekscontrolplanev1.AddToScheme(scheme)
+	_ = infrav1.AddToScheme(scheme)
+	client := fake.NewClientBuilder().WithScheme(scheme).Build()
+
+	return scope.NewManagedControlPlaneScope(
+		scope.ManagedControlPlaneScopeParams{
+			Client: client,
+			Cluster: &clusterv1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-cluster",
+				},
+			},
+			ControlPlane: &cp,
+		},
+	)
+}