Repository: kubernetes-sigs/aws-ebs-csi-driver
Pull request #530 (2020-07-10 12:12:59)

Add tags that the in-tree volume plugin uses
**Is this a bug fix or adding new feature?**
Bugfix, to reach 100% compatibility with the in-tree volume plugin, the CSI driver should create also the same tags on dynamically provisioned volumes.

**What is this PR about? / Why do we need it?**
* When the external-provisioner passes PV/PVC name/namespace via `CreateVolumeRequest.Parameters`, use those to tag volumes (i.e. the provisioner must run with `--extra-create-metadata=true`).
* When `--k8s-tag-cluster-id` was provided on cmdline, use those to tag the provisioned volumes as owned by the cluster.

Both set of tags are optional.

Fixes: #529

**What testing is done?** 
* Tested with Kubernetes 1.18 and external-provisioner 1.6.


Patch:
diff --git a/cmd/main.go b/cmd/main.go
index 855bbb63..82df25ff 100644
--- a/cmd/main.go
+++ b/cmd/main.go
@@ -33,6 +33,7 @@ func main() {
 		driver.WithExtraVolumeTags(options.ControllerOptions.ExtraVolumeTags),
 		driver.WithMode(options.DriverMode),
 		driver.WithVolumeAttachLimit(options.NodeOptions.VolumeAttachLimit),
+		driver.WithKubernetesClusterID(options.ControllerOptions.KubernetesClusterID),
 	)
 	if err != nil {
 		klog.Fatalln(err)
diff --git a/cmd/options/controller_options.go b/cmd/options/controller_options.go
index 388c3874..314b7c39 100644
--- a/cmd/options/controller_options.go
+++ b/cmd/options/controller_options.go
@@ -27,8 +27,12 @@ type ControllerOptions struct {
 	// ExtraVolumeTags is a map of tags that will be attached to each dynamically provisioned
 	// volume.
 	ExtraVolumeTags map[string]string
+	// ID of the kubernetes cluster. This is used only to create the same tags on volumes that
+	// in-tree volume volume plugin does.
+	KubernetesClusterID string
 }
 
 func (s *ControllerOptions) AddFlags(fs *flag.FlagSet) {
 	fs.Var(cliflag.NewMapStringString(&s.ExtraVolumeTags), "extra-volume-tags", "Extra volume tags to attach to each dynamically provisioned volume. It is a comma separated list of key value pairs like '<key1>=<value1>,<key2>=<value2>'")
+	fs.StringVar(&s.KubernetesClusterID, "k8s-tag-cluster-id", "", "ID of the Kubernetes cluster used for tagging provisioned EBS volumes (optional).")
 }
diff --git a/docs/README.md b/docs/README.md
index 53355c58..181938bb 100644
--- a/docs/README.md
+++ b/docs/README.md
@@ -132,7 +132,12 @@ Make sure you follow the [Prerequisites](README.md#Prerequisites) before the exa
 * [Volume Resizing](../examples/kubernetes/resizing)
 
 ## Migrating from in-tree EBS plugin
-Starting from Kubernetes 1.14, CSI migration is supported as alpha feature. If you have persistence volumes that are created with in-tree `kubernetes.io/aws-ebs` plugin, you could migrate to use EBS CSI driver. To turn on the migration, set `CSIMigration` and `CSIMigrationAWS` feature gates to `true` for `kube-controller-manager` and `kubelet`.
+Starting from Kubernetes 1.17, CSI migration is supported as beta feature (alpha since 1.14). If you have persistence volumes that are created with in-tree `kubernetes.io/aws-ebs` plugin, you could migrate to use EBS CSI driver. To turn on the migration, set `CSIMigration` and `CSIMigrationAWS` feature gates to `true` for `kube-controller-manager` and `kubelet`.
+
+To make sure dynamically provisioned EBS volumes have all tags that the in-tree volume plugin used:
+* Run the external-provisioner sidecar with `--extra-create-metadata=true` cmdline option. External-provisioner v1.6 or newer is required.
+* Run the CSI driver with `--k8s-tag-cluster-id=<ID of the Kubernetes cluster>` command line option.
+
 
 ## Development
 Please go through [CSI Spec](https://github.com/container-storage-interface/spec/blob/master/spec.md) and [General CSI driver development guideline](https://kubernetes-csi.github.io/docs/Development.html) to get some basic understanding of CSI driver before you start.
diff --git a/pkg/driver/constants.go b/pkg/driver/constants.go
index f8b02ef3..1856d245 100644
--- a/pkg/driver/constants.go
+++ b/pkg/driver/constants.go
@@ -36,6 +36,51 @@ const (
 
 	// KmsKeyId represents key for KMS encryption key
 	KmsKeyIDKey = "kmskeyid"
+
+	// PVCNameKey contains name of the PVC for which is a volume provisioned.
+	PVCNameKey = "csi.storage.k8s.io/pvc/name"
+
+	// PVCNamespaceKey contains namespace of the PVC for which is a volume provisioned.
+	PVCNamespaceKey = "csi.storage.k8s.io/pvc/namespace"
+
+	// PVNameKey contains name of the final PV that will be used for the dynamically
+	// provisioned volume
+	PVNameKey = "csi.storage.k8s.io/pv/name"
+)
+
+// constants for volume tags and their values
+const (
+	// ResourceLifecycleTagPrefix is prefix of tag for provisioned EBS volume that
+	// marks them as owned by the cluster. Used only when --cluster-id is set.
+	ResourceLifecycleTagPrefix = "kubernetes.io/cluster/"
+
+	// ResourceLifecycleOwned is the value we use when tagging resources to indicate
+	// that the resource is considered owned and managed by the cluster,
+	// and in particular that the lifecycle is tied to the lifecycle of the cluster.
+	// From k8s.io/legacy-cloud-providers/aws/tags.go.
+	ResourceLifecycleOwned = "owned"
+
+	// NameTag is tag applied to provisioned EBS volume for backward compatibility with
+	// in-tree volume plugin. Used only when --cluster-id is set.
+	NameTag = "Name"
+
+	// PVCNameTag is tag applied to provisioned EBS volume for backward compatibility
+	// with in-tree volume plugin. Value of the tag is PVC name. It is applied only when
+	// the external provisioner sidecar is started with --extra-create-metadata=true and
+	// thus provides such metadata to the CSI driver.
+	PVCNameTag = "kubernetes.io/created-for/pvc/name"
+
+	// PVCNamespaceTag is tag applied to provisioned EBS volume for backward compatibility
+	// with in-tree volume plugin. Value of the tag is PVC namespace. It is applied only when
+	// the external provisioner sidecar is started with --extra-create-metadata=true and
+	// thus provides such metadata to the CSI driver.
+	PVCNamespaceTag = "kubernetes.io/created-for/pvc/namespace"
+
+	// PVNameTag is tag applied to provisioned EBS volume for backward compatibility
+	// with in-tree volume plugin. Value of the tag is PV name. It is applied only when
+	// the external provisioner sidecar is started with --extra-create-metadata=true and
+	// thus provides such metadata to the CSI driver.
+	PVNameTag = "kubernetes.io/created-for/pv/name"
 )
 
 // constants for default command line flag values
diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go
index a2ed3531..97a1888b 100644
--- a/pkg/driver/controller.go
+++ b/pkg/driver/controller.go
@@ -128,6 +128,9 @@ func (d *controllerService) CreateVolume(ctx context.Context, req *csi.CreateVol
 		iopsPerGB   int
 		isEncrypted bool
 		kmsKeyID    string
+		volumeTags  = map[string]string{
+			cloud.VolumeNameTagKey: volName,
+		}
 	)
 
 	for key, value := range req.GetParameters() {
@@ -149,6 +152,12 @@ func (d *controllerService) CreateVolume(ctx context.Context, req *csi.CreateVol
 			}
 		case KmsKeyIDKey:
 			kmsKeyID = value
+		case PVCNameKey:
+			volumeTags[PVCNameTag] = value
+		case PVCNamespaceKey:
+			volumeTags[PVCNamespaceTag] = value
+		case PVNameKey:
+			volumeTags[PVNameTag] = value
 		default:
 			return nil, status.Errorf(codes.InvalidArgument, "Invalid parameter key %s for CreateVolume", key)
 		}
@@ -178,8 +187,11 @@ func (d *controllerService) CreateVolume(ctx context.Context, req *csi.CreateVol
 	// create a new volume
 	zone := pickAvailabilityZone(req.GetAccessibilityRequirements())
 
-	volumeTags := map[string]string{
-		cloud.VolumeNameTagKey: volName,
+	// fill volume tags
+	if d.driverOptions.kubernetesClusterID != "" {
+		resourceLifecycleTag := ResourceLifecycleTagPrefix + d.driverOptions.kubernetesClusterID
+		volumeTags[resourceLifecycleTag] = ResourceLifecycleOwned
+		volumeTags[NameTag] = d.driverOptions.kubernetesClusterID + "-dynamic-" + volName
 	}
 	for k, v := range d.driverOptions.extraVolumeTags {
 		volumeTags[k] = v
diff --git a/pkg/driver/driver.go b/pkg/driver/driver.go
index 60305d5b..1be2e86d 100644
--- a/pkg/driver/driver.go
+++ b/pkg/driver/driver.go
@@ -53,10 +53,11 @@ type Driver struct {
 }
 
 type DriverOptions struct {
-	endpoint          string
-	extraVolumeTags   map[string]string
-	mode              Mode
-	volumeAttachLimit int64
+	endpoint            string
+	extraVolumeTags     map[string]string
+	mode                Mode
+	volumeAttachLimit   int64
+	kubernetesClusterID string
 }
 
 func NewDriver(options ...func(*DriverOptions)) (*Driver, error) {
@@ -162,3 +163,9 @@ func WithVolumeAttachLimit(volumeAttachLimit int64) func(*DriverOptions) {
 		o.volumeAttachLimit = volumeAttachLimit
 	}
 }
+
+func WithKubernetesClusterID(clusterID string) func(*DriverOptions) {
+	return func(o *DriverOptions) {
+		o.kubernetesClusterID = clusterID
+	}
+}


Test patch:
diff --git a/cmd/options/controller_options_test.go b/cmd/options/controller_options_test.go
index 5460b636..9c89ae88 100644
--- a/cmd/options/controller_options_test.go
+++ b/cmd/options/controller_options_test.go
@@ -32,6 +32,11 @@ func TestControllerOptions(t *testing.T) {
 			flag:  "extra-volume-tags",
 			found: true,
 		},
+		{
+			name:  "lookup k8s-tag-cluster-id",
+			flag:  "k8s-tag-cluster-id",
+			found: true,
+		},
 		{
 			name:  "fail for non-desired flag",
 			flag:  "some-other-flag",
diff --git a/pkg/driver/controller_test.go b/pkg/driver/controller_test.go
index ba59eef8..04ef901b 100644
--- a/pkg/driver/controller_test.go
+++ b/pkg/driver/controller_test.go
@@ -1048,6 +1048,129 @@ func TestCreateVolume(t *testing.T) {
 					},
 				}
 
+				_, err := awsDriver.CreateVolume(ctx, req)
+				if err != nil {
+					srvErr, ok := status.FromError(err)
+					if !ok {
+						t.Fatalf("Could not get error status code from error: %v", srvErr)
+					}
+					t.Fatalf("Unexpected error: %v", srvErr.Code())
+				}
+			},
+		},
+		{
+			name: "success with cluster-id",
+			testFunc: func(t *testing.T) {
+				const (
+					volumeName            = "random-vol-name"
+					clusterID             = "test-cluster-id"
+					expectedOwnerTag      = "kubernetes.io/cluster/test-cluster-id"
+					expectedOwnerTagValue = "owned"
+					expectedNameTag       = "Name"
+					expectedNameTagValue  = "test-cluster-id-dynamic-random-vol-name"
+				)
+				req := &csi.CreateVolumeRequest{
+					Name:               volumeName,
+					CapacityRange:      stdCapRange,
+					VolumeCapabilities: stdVolCap,
+					Parameters:         nil,
+				}
+
+				ctx := context.Background()
+
+				mockDisk := &cloud.Disk{
+					VolumeID:         req.Name,
+					AvailabilityZone: expZone,
+					CapacityGiB:      util.BytesToGiB(stdVolSize),
+				}
+
+				diskOptions := &cloud.DiskOptions{
+					CapacityBytes: stdVolSize,
+					Tags: map[string]string{
+						cloud.VolumeNameTagKey: volumeName,
+						expectedOwnerTag:       expectedOwnerTagValue,
+						expectedNameTag:        expectedNameTagValue,
+					},
+				}
+
+				mockCtl := gomock.NewController(t)
+				defer mockCtl.Finish()
+
+				mockCloud := mocks.NewMockCloud(mockCtl)
+				mockCloud.EXPECT().GetDiskByName(gomock.Eq(ctx), gomock.Eq(req.Name), gomock.Eq(stdVolSize)).Return(nil, cloud.ErrNotFound)
+				mockCloud.EXPECT().CreateDisk(gomock.Eq(ctx), gomock.Eq(req.Name), gomock.Eq(diskOptions)).Return(mockDisk, nil)
+
+				awsDriver := controllerService{
+					cloud: mockCloud,
+					driverOptions: &DriverOptions{
+						kubernetesClusterID: clusterID,
+					},
+				}
+
+				_, err := awsDriver.CreateVolume(ctx, req)
+				if err != nil {
+					srvErr, ok := status.FromError(err)
+					if !ok {
+						t.Fatalf("Could not get error status code from error: %v", srvErr)
+					}
+					t.Fatalf("Unexpected error: %v", srvErr.Code())
+				}
+			},
+		},
+		{
+			name: "success with legacy tags",
+			testFunc: func(t *testing.T) {
+				const (
+					volumeName              = "random-vol-name"
+					clusterID               = "test-cluster-id"
+					expectedPVCNameTag      = "kubernetes.io/created-for/pvc/name"
+					expectedPVCNamespaceTag = "kubernetes.io/created-for/pvc/namespace"
+					expectedPVNameTag       = "kubernetes.io/created-for/pv/name"
+					pvcNamespace            = "default"
+					pvcName                 = "my-pvc"
+					pvName                  = volumeName
+				)
+				req := &csi.CreateVolumeRequest{
+					Name:               volumeName,
+					CapacityRange:      stdCapRange,
+					VolumeCapabilities: stdVolCap,
+					Parameters: map[string]string{
+						"csi.storage.k8s.io/pvc/name":      pvcName,
+						"csi.storage.k8s.io/pvc/namespace": pvcNamespace,
+						"csi.storage.k8s.io/pv/name":       pvName,
+					},
+				}
+
+				ctx := context.Background()
+
+				mockDisk := &cloud.Disk{
+					VolumeID:         req.Name,
+					AvailabilityZone: expZone,
+					CapacityGiB:      util.BytesToGiB(stdVolSize),
+				}
+
+				diskOptions := &cloud.DiskOptions{
+					CapacityBytes: stdVolSize,
+					Tags: map[string]string{
+						cloud.VolumeNameTagKey:  volumeName,
+						expectedPVCNameTag:      pvcName,
+						expectedPVCNamespaceTag: pvcNamespace,
+						expectedPVNameTag:       pvName,
+					},
+				}
+
+				mockCtl := gomock.NewController(t)
+				defer mockCtl.Finish()
+
+				mockCloud := mocks.NewMockCloud(mockCtl)
+				mockCloud.EXPECT().GetDiskByName(gomock.Eq(ctx), gomock.Eq(req.Name), gomock.Eq(stdVolSize)).Return(nil, cloud.ErrNotFound)
+				mockCloud.EXPECT().CreateDisk(gomock.Eq(ctx), gomock.Eq(req.Name), gomock.Eq(diskOptions)).Return(mockDisk, nil)
+
+				awsDriver := controllerService{
+					cloud:         mockCloud,
+					driverOptions: &DriverOptions{},
+				}
+
 				_, err := awsDriver.CreateVolume(ctx, req)
 				if err != nil {
 					srvErr, ok := status.FromError(err)
diff --git a/pkg/driver/driver_test.go b/pkg/driver/driver_test.go
index 70f86002..c26231ad 100644
--- a/pkg/driver/driver_test.go
+++ b/pkg/driver/driver_test.go
@@ -56,3 +56,12 @@ func TestWithVolumeAttachLimit(t *testing.T) {
 		t.Fatalf("expected volumeAttachLimit option got set to %d but is set to %d", value, options.volumeAttachLimit)
 	}
 }
+
+func TestWithClusterID(t *testing.T) {
+	var id string = "test-cluster-id"
+	options := &DriverOptions{}
+	WithKubernetesClusterID(id)(options)
+	if options.kubernetesClusterID != id {
+		t.Fatalf("expected kubernetesClusterID option got set to %s but is set to %s", id, options.kubernetesClusterID)
+	}
+}