Repository: kubernetes-sigs/cluster-api-provider-aws
Pull request #3622 (2022-07-27 11:03:20)

[release-1.5] fix: Do not change default security groups during EKS control plane reconcile
This is an automated cherry-pick of #3483

/assign dlipovetsky


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,
+		},
+	)
+}