Fix SG deletion (SGs are cluster owned, not provider owned)

Author: sedefsavas

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