Merge pull request #2620 from sedefsavas/sgfix

🐛 Managed SecurityGroups should be filtered correctly

go.mod

@@ -35,7 +35,7 @@ require (
 	k8s.io/klog/v2 v2.1.0
 	k8s.io/utils v0.0.0-20210709001253-0e1f9d693477
 	sigs.k8s.io/aws-iam-authenticator v0.5.3
-	sigs.k8s.io/cluster-api v0.3.21
+	sigs.k8s.io/cluster-api v0.3.22
 	sigs.k8s.io/controller-runtime v0.5.14
 	sigs.k8s.io/yaml v1.2.0
 )

go.sum

@@ -1186,8 +1186,8 @@ modernc.org/xc v1.0.0/go.mod h1:mRNCo0bvLjGhHO9WsyuKVU4q0ceiDDDoEeWDJHrNx8I=
 rsc.io/binaryregexp v0.2.0/go.mod h1:qTv7/COck+e2FymRvadv62gMdZztPaShugOCi3I+8D8=
 sigs.k8s.io/aws-iam-authenticator v0.5.3 h1:EyqQ/uxzbe2mDETZZmuMnv0xHITnyLhZfPlGb6Mma20=
 sigs.k8s.io/aws-iam-authenticator v0.5.3/go.mod h1:DIq7gy0lvnyaG88AgFyJzUVeix+ia5msHEp4RL0102I=
-sigs.k8s.io/cluster-api v0.3.21 h1:AOumFs+wJ/kJDNNzdqk1IQjhM+0x6nZbzaSpytX9Xms=
-sigs.k8s.io/cluster-api v0.3.21/go.mod h1:1DBZEj6GmcWxe77d8YOeac1JIa8ttP21uTHUlAyji2g=
+sigs.k8s.io/cluster-api v0.3.22 h1:yxl/YHujBqTx+OBcX1FVlJ0DSwXH4LXg8hmDeh8Lick=
+sigs.k8s.io/cluster-api v0.3.22/go.mod h1:1DBZEj6GmcWxe77d8YOeac1JIa8ttP21uTHUlAyji2g=
 sigs.k8s.io/controller-runtime v0.5.14 h1:lmoRaPvLg9877ZOnjFivjtyIdqyLbWfcCEilxHXTEj4=
 sigs.k8s.io/controller-runtime v0.5.14/go.mod h1:OTqxLuz7gVcrq+BHGUgedRu6b2VIKCEc7Pu4Jbwui0A=
 sigs.k8s.io/kind v0.7.1-0.20200303021537-981bd80d3802 h1:L6/8hETA7jvdx3xBcbDifrIN2xaYHE7tA58n+Kdp2Zw=

pkg/cloud/services/securitygroup/securitygroups.go

@@ -326,7 +326,7 @@ func (s *Service) describeClusterOwnedSecurityGroups() ([]infrav1.SecurityGroup,
 	input := &ec2.DescribeSecurityGroupsInput{
 		Filters: []*ec2.Filter{
 			filter.EC2.VPC(s.scope.VPC().ID),
-			filter.EC2.ProviderOwned(s.scope.Name()),
+			filter.EC2.ClusterOwned(s.scope.Name()),
 		},
 	}
 

pkg/cloud/services/securitygroup/securitygroups_test.go

@@ -17,20 +17,23 @@ limitations under the License.
 package securitygroup
 
 import (
-	"github.com/pkg/errors"
+	"context"
 	"strings"
 	"testing"
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/golang/mock/gomock"
+	"github.com/pkg/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/sets"
 	infrav1 "sigs.k8s.io/cluster-api-provider-aws/api/v1alpha3"
 	"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/scope"
 	"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/services"
 	"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/services/ec2/mock_ec2iface"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 )
 
 func TestReconcileSecurityGroups(t *testing.T) {
@@ -385,3 +388,123 @@ func TestControlPlaneSecurityGroupNotOpenToAnyCIDR(t *testing.T) {
 		}
 	}
 }
+
+func TestDeleteSecurityGroups(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	testCases := []struct {
+		name   string
+		input  *infrav1.NetworkSpec
+		expect func(m *mock_ec2iface.MockEC2APIMockRecorder)
+		err    error
+	}{
+		{
+			name: "do not delete overridden security groups, only delete 'owned' SGs",
+			input: &infrav1.NetworkSpec{
+				VPC: infrav1.VPCSpec{
+					ID:                "vpc-securitygroups",
+					InternetGatewayID: aws.String("igw-01"),
+				},
+				Subnets: infrav1.Subnets{
+					&infrav1.SubnetSpec{
+						ID:               "subnet-securitygroups-private",
+						IsPublic:         false,
+						AvailabilityZone: "us-east-1a",
+					},
+					&infrav1.SubnetSpec{
+						ID:               "subnet-securitygroups-public",
+						IsPublic:         true,
+						NatGatewayID:     aws.String("nat-01"),
+						AvailabilityZone: "us-east-1a",
+					},
+				},
+				SecurityGroupOverrides: map[infrav1.SecurityGroupRole]string{
+					infrav1.SecurityGroupBastion:      "sg-bastion",
+					infrav1.SecurityGroupAPIServerLB:  "sg-apiserver-lb",
+					infrav1.SecurityGroupLB:           "sg-lb",
+					infrav1.SecurityGroupControlPlane: "sg-control",
+					infrav1.SecurityGroupNode:         "sg-node",
+				},
+			},
+			expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
+				m.DescribeSecurityGroupsPages(gomock.Any(), gomock.Any()).Do(func(_, y interface{}) {
+					funct := y.(func(output *ec2.DescribeSecurityGroupsOutput, lastPage bool) bool)
+					funct(&ec2.DescribeSecurityGroupsOutput{
+						SecurityGroups: []*ec2.SecurityGroup{
+							{
+								GroupName: aws.String("sg-bastion"),
+								GroupId:   aws.String("sg-bastion"),
+
+								Tags: []*ec2.Tag{
+									{
+										Key:   aws.String("Name"),
+										Value: aws.String("test-cluster-nat"),
+									},
+									{
+										Key:   aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"),
+										Value: aws.String("owned"),
+									},
+									{
+										Key:   aws.String("sigs.k8s.io/cluster-api-provider-aws/role"),
+										Value: aws.String("common"),
+									},
+								},
+							},
+						},
+					}, true)
+				}).Return(nil)
+
+				m.DescribeSecurityGroups(gomock.Any()).Return(&ec2.DescribeSecurityGroupsOutput{}, nil)
+				m.DeleteSecurityGroup(gomock.Any()).Return(nil, nil)
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			ec2Mock := mock_ec2iface.NewMockEC2API(mockCtrl)
+
+			scheme := runtime.NewScheme()
+			_ = infrav1.AddToScheme(scheme)
+			awsCluster := &infrav1.AWSCluster{
+				TypeMeta: metav1.TypeMeta{
+					APIVersion: infrav1.GroupVersion.String(),
+					Kind:       "AWSCluster",
+				},
+				ObjectMeta: metav1.ObjectMeta{Name: "test"},
+				Spec: infrav1.AWSClusterSpec{
+					NetworkSpec: *tc.input,
+				},
+			}
+			client := fake.NewFakeClientWithScheme(scheme, awsCluster)
+
+			ctx := context.TODO()
+			client.Create(ctx, awsCluster)
+
+			scope, err := scope.NewClusterScope(scope.ClusterScopeParams{
+				Client: client,
+				Cluster: &clusterv1.Cluster{
+					ObjectMeta: metav1.ObjectMeta{Name: "test-cluster"},
+				},
+				AWSCluster: awsCluster,
+			})
+			if err != nil {
+				t.Fatalf("Failed to create test context: %v", err)
+			}
+
+			tc.expect(ec2Mock.EXPECT())
+
+			s := NewService(scope)
+			s.EC2Client = ec2Mock
+
+			if err := s.DeleteSecurityGroups(); err != nil && tc.err != nil {
+				if !strings.Contains(err.Error(), tc.err.Error()) {
+					t.Fatalf("was expecting error to look like '%v', but got '%v'", tc.err, err)
+				}
+			} else if err != nil {
+				t.Fatalf("got an unexpected error: %v", err)
+			}
+		})
+	}
+}

test/e2e/data/e2e_conf.yaml

@@ -14,11 +14,11 @@
 # For more details, run `go run ./cmd/clusterawsadm bootstrap credentials`
 
 images:
-  - name: gcr.io/k8s-staging-cluster-api/cluster-api-controller:v0.3.21
+  - name: gcr.io/k8s-staging-cluster-api/cluster-api-controller:v0.3.22
     loadBehavior: tryLoad
-  - name: gcr.io/k8s-staging-cluster-api/kubeadm-bootstrap-controller:v0.3.21
+  - name: gcr.io/k8s-staging-cluster-api/kubeadm-bootstrap-controller:v0.3.22
     loadBehavior: tryLoad
-  - name: gcr.io/k8s-staging-cluster-api/kubeadm-control-plane-controller:v0.3.21
+  - name: gcr.io/k8s-staging-cluster-api/kubeadm-control-plane-controller:v0.3.22
     loadBehavior: tryLoad
   # Use local dev images built source tree;
   - name: gcr.io/k8s-staging-cluster-api/capa-manager:e2e
@@ -35,9 +35,9 @@ providers:
   - name: cluster-api
     type: CoreProvider
     versions:
-      - name: v0.3.21
+      - name: v0.3.22
         # Use manifest from source files
-        value: "https://github.com/kubernetes-sigs/cluster-api/releases/download/v0.3.21/core-components.yaml"
+        value: "https://github.com/kubernetes-sigs/cluster-api/releases/download/v0.3.22/core-components.yaml"
         type: "url"
         replacements:
           - old: "imagePullPolicy: Always"
@@ -50,9 +50,9 @@ providers:
   - name: kubeadm
     type: BootstrapProvider
     versions:
-      - name: v0.3.21
+      - name: v0.3.22
         # Use manifest from source files
-        value: "https://github.com/kubernetes-sigs/cluster-api/releases/download/v0.3.21/bootstrap-components.yaml"
+        value: "https://github.com/kubernetes-sigs/cluster-api/releases/download/v0.3.22/bootstrap-components.yaml"
         type: "url"
         replacements:
           - old: "imagePullPolicy: Always"
@@ -65,9 +65,9 @@ providers:
   - name: kubeadm
     type: ControlPlaneProvider
     versions:
-      - name: v0.3.21
+      - name: v0.3.22
         # Use manifest from source files
-        value: "https://github.com/kubernetes-sigs/cluster-api/releases/download/v0.3.21/control-plane-components.yaml"
+        value: "https://github.com/kubernetes-sigs/cluster-api/releases/download/v0.3.22/control-plane-components.yaml"
         type: "url"
         replacements:
           - old: "imagePullPolicy: Always"
@@ -116,21 +116,21 @@ providers:
       - sourcePath: "./infrastructure-aws/cluster-template-nested-multitenancy.yaml"
         targetName: "cluster-template-nested-multitenancy.yaml"
 variables:
-  KUBERNETES_VERSION: "v1.21.2"
+  KUBERNETES_VERSION: "v1.21.3"
   CNI: "../../data/cni/calico.yaml"
   EXP_CLUSTER_RESOURCE_SET: "true"
   EVENT_BRIDGE_INSTANCE_STATE: "true"
   AWS_CONTROL_PLANE_MACHINE_TYPE: t3.large
   AWS_NODE_MACHINE_TYPE: t3.large
   AWS_SSH_KEY_NAME: "cluster-api-provider-aws-sigs-k8s-io"
-  CONFORMANCE_CI_ARTIFACTS_KUBERNETES_VERSION: "v1.21.2"
+  CONFORMANCE_CI_ARTIFACTS_KUBERNETES_VERSION: "v1.21.3"
   CONFORMANCE_WORKER_MACHINE_COUNT: "5"
   CONFORMANCE_CONTROL_PLANE_MACHINE_COUNT: "1"
   EXP_MACHINE_POOL: "true"
   ETCD_VERSION_UPGRADE_TO: "3.4.13-0"
   COREDNS_VERSION_UPGRADE_TO: "1.7.1"
-  KUBERNETES_VERSION_UPGRADE_TO: "v1.21.2"
-  KUBERNETES_VERSION_UPGRADE_FROM: "v1.20.8"
+  KUBERNETES_VERSION_UPGRADE_TO: "v1.21.3"
+  KUBERNETES_VERSION_UPGRADE_FROM: "v1.20.9"
   MULTI_TENANCY_ROLE_NAME: "multi-tenancy-role"
   MULTI_TENANCY_NESTED_ROLE_NAME: "multi-tenancy-nested-role"