ignore-deleting-ebs-volume-from-diferent-cluster (#2855)

CHANGELOG.md

@@ -12,6 +12,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Update dependencies to next major versions.
 
+### Fixed
+
+- During a deletion of a cluster, ignore volumes that are mounted to an instance in a different cluster.
+
 ## [9.3.0] - 2020-11-09
 
 ### Added

service/controller/resource/cleanupebsvolumes/delete.go

@@ -43,7 +43,7 @@ func (r *Resource) EnsureDeleted(ctx context.Context, obj interface{}) error {
 		ebs.NewEtcdVolumeFilter(cr),
 		ebs.NewPersistentVolumeFilter(cr),
 	}
-	volumes, err := ebsService.ListVolumes(cr, filterFuncs...)
+	volumes, err := ebsService.ListVolumes(ctx, cr, filterFuncs...)
 	if err != nil {
 		return microerror.Mask(err)
 	}

service/internal/ebs/ebs.go

@@ -12,6 +12,7 @@ import (
 	"github.com/giantswarm/microerror"
 	"github.com/giantswarm/micrologger"
 
+	"github.com/giantswarm/aws-operator/pkg/awstags"
 	"github.com/giantswarm/aws-operator/service/controller/key"
 )
 
@@ -184,7 +185,7 @@ func (e *EBS) DetachVolume(ctx context.Context, volumeID string, attachment Volu
 // the Etcd volume for the master instance will be returned. If persistentVolume
 // is set then any Persistent Volumes associated with the cluster will be
 // returned.
-func (e *EBS) ListVolumes(cr infrastructurev1alpha2.AWSCluster, filterFuncs ...func(t *ec2.Tag) bool) ([]Volume, error) {
+func (e *EBS) ListVolumes(ctx context.Context, cr infrastructurev1alpha2.AWSCluster, filterFuncs ...func(t *ec2.Tag) bool) ([]Volume, error) {
 	var volumes []Volume
 
 	// We filter to only select clusters with the cluster cloud provider tag.
@@ -208,18 +209,49 @@ func (e *EBS) ListVolumes(cr infrastructurev1alpha2.AWSCluster, filterFuncs ...f
 		if !IsFiltered(v, filterFuncs) {
 			continue
 		}
-
+		ignoreVolume := false
 		attachments := []VolumeAttachment{}
 
 		if len(v.Attachments) > 0 {
 			for _, a := range v.Attachments {
+
+				if *a.InstanceId != "" {
+					i := &ec2.DescribeInstancesInput{
+						InstanceIds: aws.StringSlice([]string{*a.InstanceId}),
+					}
+
+					o, err := e.client.DescribeInstances(i)
+					if err != nil {
+						return nil, microerror.Mask(err)
+					}
+
+					hasWrongClusterID := false
+					if len(o.Reservations) > 0 && len(o.Reservations[0].Instances) > 0 {
+						hasWrongClusterID = awstags.ValueForKey(o.Reservations[0].Instances[0].Tags, key.TagCluster) != cr.Labels[key.TagCluster]
+					}
+
+					// if the instance does not have the proper cluster-id tag than ignore the volume
+					if hasWrongClusterID {
+						e.logger.LogCtx(ctx, "level", "warning", "message", fmt.Sprintf("EBS volume %#q is attached to instance %#q which does not belong to the cluster %#q, ignoring the volume", *a.VolumeId, *a.InstanceId, cr.Labels[key.TagCluster]))
+
+						// do not add this volume to the volume list
+						// this volume will be ignored by the operator
+						ignoreVolume = true
+						break
+					}
+				}
+
 				attachments = append(attachments, VolumeAttachment{
 					Device:     *a.Device,
 					InstanceID: *a.InstanceId,
 				})
 			}
 		}
 
+		if ignoreVolume {
+			continue
+		}
+
 		volume := Volume{
 			VolumeID:    *v.VolumeId,
 			Attachments: attachments,

service/internal/ebs/ebs_test.go

@@ -1,6 +1,7 @@
 package ebs
 
 import (
+	"context"
 	"reflect"
 	"testing"
 
@@ -326,10 +327,72 @@ func Test_ListVolumes(t *testing.T) {
 				},
 			},
 		},
+		{
+			description: "case 8: ignore volumes that are mounted to a instance that belongs to different cluster",
+			obj:         customObject,
+			filterFuncs: []func(t *ec2.Tag) bool{
+				NewPersistentVolumeFilter(customObject),
+			},
+			expectedVolumes: []Volume{
+				{
+					Attachments: []VolumeAttachment{
+						{
+							InstanceID: "i-12345",
+							Device:     "/dev/sdh",
+						},
+					},
+					VolumeID: "vol-1234",
+				},
+			},
+			ebsVolumes: []ebsVolumeMock{
+				{
+					volumeID: "vol-1234",
+					attachments: []*ec2.VolumeAttachment{
+						{
+							Device:     aws.String("/dev/sdh"),
+							InstanceId: aws.String("i-12345"),
+							VolumeId:   aws.String("vol-1234"),
+						},
+					},
+					tags: []*ec2.Tag{
+						{
+							Key:   aws.String("kubernetes.io/cluster/test-cluster"),
+							Value: aws.String("owned"),
+						},
+						{
+							Key:   aws.String("kubernetes.io/created-for/pv/name"),
+							Value: aws.String("pvc-1234"),
+						},
+					},
+				},
+				{
+					volumeID: "vol-5678",
+					attachments: []*ec2.VolumeAttachment{
+						{
+							Device:     aws.String("/dev/sdh"),
+							InstanceId: aws.String("i-555555"),
+							VolumeId:   aws.String("vol-5678"),
+						},
+					},
+					tags: []*ec2.Tag{
+						{
+							Key:   aws.String("kubernetes.io/cluster/test-cluster"),
+							Value: aws.String("owned"),
+						},
+						{
+							Key:   aws.String("kubernetes.io/created-for/pv/name"),
+							Value: aws.String("pvc-5678"),
+						},
+					},
+				},
+			},
+		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.description, func(t *testing.T) {
+			ctx := context.TODO()
+
 			c := Config{
 				Client: &EC2ClientMock{
 					customObject: tc.obj,
@@ -342,7 +405,7 @@ func Test_ListVolumes(t *testing.T) {
 				t.Fatal("expected", nil, "got", err)
 			}
 
-			result, err := e.ListVolumes(tc.obj, tc.filterFuncs...)
+			result, err := e.ListVolumes(ctx, tc.obj, tc.filterFuncs...)
 			if err != nil {
 				t.Fatal("expected", nil, "got", err)
 			}

service/internal/ebs/mock_test.go

@@ -23,6 +23,30 @@ func (e *EC2ClientMock) DeleteVolume(*ec2.DeleteVolumeInput) (*ec2.DeleteVolumeO
 	return nil, nil
 }
 
+func (e *EC2ClientMock) DescribeInstances(input *ec2.DescribeInstancesInput) (*ec2.DescribeInstancesOutput, error) {
+	o := &ec2.DescribeInstancesOutput{}
+
+	// test case for instance that does not belong to the cluster
+	// to test behavior of ignoring volume when its mounted to an instance from different cluster
+	if *input.InstanceIds[0] == "i-555555" {
+
+		t := &ec2.Tag{
+			Key:   aws.String(key.TagCluster),
+			Value: aws.String("invalid-cluster"),
+		}
+		i := &ec2.Instance{
+			Tags: []*ec2.Tag{t},
+		}
+		r := &ec2.Reservation{
+			Instances: []*ec2.Instance{i},
+		}
+
+		o.Reservations = []*ec2.Reservation{r}
+	}
+
+	return o, nil
+}
+
 func (e *EC2ClientMock) DescribeVolumes(input *ec2.DescribeVolumesInput) (*ec2.DescribeVolumesOutput, error) {
 	output := &ec2.DescribeVolumesOutput{}
 	volumes := []*ec2.Volume{}

service/internal/ebs/spec.go

@@ -18,12 +18,13 @@ type Interface interface {
 	// the Etcd volume for the master instance will be returned. If
 	// persistentVolume is true then any Persistent Volumes associated with the
 	// cluster will be returned.
-	ListVolumes(customObject infrastructurev1alpha2.AWSCluster, filterFuncs ...func(t *ec2.Tag) bool) ([]Volume, error)
+	ListVolumes(ctx context.Context, customObject infrastructurev1alpha2.AWSCluster, filterFuncs ...func(t *ec2.Tag) bool) ([]Volume, error)
 }
 
 // EC2Client describes the methods required to be implemented by an EC2 AWS client.
 type EC2Client interface {
 	DeleteVolume(*ec2.DeleteVolumeInput) (*ec2.DeleteVolumeOutput, error)
+	DescribeInstances(*ec2.DescribeInstancesInput) (*ec2.DescribeInstancesOutput, error)
 	DescribeVolumes(*ec2.DescribeVolumesInput) (*ec2.DescribeVolumesOutput, error)
 	DetachVolume(*ec2.DetachVolumeInput) (*ec2.VolumeAttachment, error)
 	StopInstances(*ec2.StopInstancesInput) (*ec2.StopInstancesOutput, error)