Merge branch 'kubernetes-sigs:master' into master

charts/aws-ebs-csi-driver/templates/csidriver.yaml

@@ -7,3 +7,4 @@ metadata:
 spec:
   attachRequired: true
   podInfoOnMount: false
+  fsGroupPolicy: File

deploy/kubernetes/base/csidriver.yaml

@@ -9,3 +9,4 @@ metadata:
 spec:
   attachRequired: true
   podInfoOnMount: false
+  fsGroupPolicy: File

docs/parameters.md

@@ -7,13 +7,21 @@ The AWS EBS CSI Driver supports [tagging](tagging.md) through `StorageClass.para
 |-----------------------------|----------------------------------------|----------|---------------------|
 | "csi.storage.k8s.io/fstype" | xfs, ext2, ext3, ext4                  | ext4     | File system type that will be formatted during volume creation. This parameter is case sensitive! |
 | "type"                      | io1, io2, gp2, gp3, sc1, st1,standard  | gp3*     | EBS volume type.     |
-| "iopsPerGB"                 |                                        |          | I/O operations per second per GiB. Required when io1 or io2 volume type is specified. If this value multiplied by the size of a requested volume produces a value above the maximum IOPs allowed for the volume type, as documented [here](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ebs-volume-types.html), AWS will cap the IOPS to maximum supported value. If the value is lower than minimal supported IOPS value per volume, either error is returned (the default behavior) or the value is increased to fit into the supported range when `allowautoiopspergbincrease` is `"true"`.|
+| "iopsPerGB"                 |                                        |          | I/O operations per second per GiB. Can be specified for IO1, IO2, and GP3 volumes. |
 | "allowAutoIOPSPerGBIncrease"| true, false                            | false    | When `"true"`, the CSI driver increases IOPS for a volume when `iopsPerGB * <volume size>` is too low to fit into IOPS range supported by AWS. This allows dynamic provisioning to always succeed, even when user specifies too small PVC capacity or `iopsPerGB` value. On the other hand, it may introduce additional costs, as such volumes have higher IOPS than requested in `iopsPerGB`.|
-| "iops"                      |                                        | 3000     | I/O operations per second. Only effective when gp3 volume type is specified. If empty, it will set to 3000 as documented [here](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ebs-volume-types.html). |
+| "iops"                      |                                        |          | I/O operations per second. Can be specified for IO1, IO2, and GP3 volumes. |
 | "throughput"                |                                        | 125      | Throughput in MiB/s. Only effective when gp3 volume type is specified. If empty, it will set to 125MiB/s as documented [here](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ebs-volume-types.html). |
 | "encrypted"                 |                                        |          | Whether the volume should be encrypted or not. Valid values are "true" or "false". |
 | "kmsKeyId"                  |                                        |          | The full ARN of the key to use when encrypting the volume. If not specified, AWS will use the default KMS key for the region the volume is in. This will be an auto-generated key called `/aws/ebs` if not changed. |
 
-**Notes**:
+**Appendix**
 * `gp3` is currently not supported on outposts. Outpost customers need to use a different type for their volumes.
-* Unless explicitly noted, all parameters are case insensitive (e.g. "kmsKeyId", "kmskeyid" and any other combination of upper/lowercase characters can be used).
+* Unless explicitly noted, all parameters are case insensitive (e.g. "kmsKeyId", "kmskeyid" and any other combination of upper/lowercase characters can be used).
+* If the requested IOPs (either directly from `iops` or from `iopsPerGB` multiplied by the volume's capacity) produces a value above the maximum IOPs allowed for the [volume type](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ebs-volume-types.html), the IOPS will be capped at the maximum value allowed. If the value is lower than the minimal supported IOPS value per volume, either an error is returned (the default behavior), or the value is increased to fit into the supported range when `allowautoiopspergbincrease` is `"true"`.
+* You may specify either the "iops" or "iopsPerGb" parameters, not both. Specifying both parameters will result in an invalid StorageClass.
+
+| Volume Type                 | Min total IOPS                         | Max total IOPS   | Max IOPS per GB   |
+|-----------------------------|----------------------------------------|------------------|-------------------|
+| IO1                         | 100                                    | 64000            | 50                |
+| IO2                         | 100                                    | 64000            | 500               |
+| GP3                         | 3000                                   | 16000            | 500               |

pkg/cloud/cloud.go

@@ -68,6 +68,9 @@ const (
 	io2MinTotalIOPS = 100
 	io2MaxTotalIOPS = 64000
 	io2MaxIOPSPerGB = 500
+	gp3MaxTotalIOPS = 16000
+	gp3MinTotalIOPS = 3000
+	gp3MaxIOPSPerGB = 500
 )
 
 var (
@@ -115,8 +118,6 @@ const (
 const (
 	// DefaultVolumeSize represents the default volume size.
 	DefaultVolumeSize int64 = 100 * util.GiB
-	// DefaultVolumeType specifies which storage to use for newly created Volumes.
-	DefaultVolumeType = VolumeTypeGP3
 )
 
 // Tags
@@ -276,40 +277,59 @@ func newEC2Cloud(region string, awsSdkDebugLog bool) (Cloud, error) {
 
 func (c *cloud) CreateDisk(ctx context.Context, volumeName string, diskOptions *DiskOptions) (*Disk, error) {
 	var (
-		createType string
-		iops       int64
-		throughput int64
-		err        error
+		createType    string
+		iops          int64
+		throughput    int64
+		err           error
+		maxIops       int64
+		minIops       int64
+		maxIopsPerGb  int64
+		requestedIops int64
 	)
+
 	capacityGiB := util.BytesToGiB(diskOptions.CapacityBytes)
 
-	switch diskOptions.VolumeType {
+	if diskOptions.IOPS > 0 && diskOptions.IOPSPerGB > 0 {
+		return nil, fmt.Errorf("invalid StorageClass parameters; specify either IOPS or IOPSPerGb, not both")
+	}
+
+	createType = diskOptions.VolumeType
+	// If no volume type is specified, GP3 is used as default for newly created volumes.
+	if createType == "" {
+		createType = VolumeTypeGP3
+	}
+
+	switch createType {
 	case VolumeTypeGP2, VolumeTypeSC1, VolumeTypeST1, VolumeTypeSBG1, VolumeTypeSBP1, VolumeTypeStandard:
-		createType = diskOptions.VolumeType
 	case VolumeTypeIO1:
-		createType = diskOptions.VolumeType
-		iops, err = capIOPS(diskOptions.VolumeType, capacityGiB, int64(diskOptions.IOPSPerGB), io1MinTotalIOPS, io1MaxTotalIOPS, io1MaxIOPSPerGB, diskOptions.AllowIOPSPerGBIncrease)
-		if err != nil {
-			return nil, err
-		}
+		maxIops = io1MaxTotalIOPS
+		minIops = io1MinTotalIOPS
+		maxIopsPerGb = io1MaxIOPSPerGB
 	case VolumeTypeIO2:
-		createType = diskOptions.VolumeType
-		iops, err = capIOPS(diskOptions.VolumeType, capacityGiB, int64(diskOptions.IOPSPerGB), io2MinTotalIOPS, io2MaxTotalIOPS, io2MaxIOPSPerGB, diskOptions.AllowIOPSPerGBIncrease)
-		if err != nil {
-			return nil, err
-		}
+		maxIops = io2MaxTotalIOPS
+		minIops = io2MinTotalIOPS
+		maxIopsPerGb = io2MaxIOPSPerGB
 	case VolumeTypeGP3:
-		createType = diskOptions.VolumeType
-		iops = int64(diskOptions.IOPS)
-		throughput = int64(diskOptions.Throughput)
-	case "":
-		createType = DefaultVolumeType
-		iops = int64(diskOptions.IOPS)
+		maxIops = gp3MaxTotalIOPS
+		minIops = gp3MinTotalIOPS
+		maxIopsPerGb = gp3MaxIOPSPerGB
 		throughput = int64(diskOptions.Throughput)
 	default:
 		return nil, fmt.Errorf("invalid AWS VolumeType %q", diskOptions.VolumeType)
 	}
 
+	if maxIops > 0 {
+		if diskOptions.IOPS > 0 {
+			requestedIops = int64(diskOptions.IOPS)
+		} else if diskOptions.IOPSPerGB > 0 {
+			requestedIops = int64(diskOptions.IOPSPerGB) * capacityGiB
+		}
+		iops, err = capIOPS(createType, capacityGiB, requestedIops, minIops, maxIops, maxIopsPerGb, diskOptions.AllowIOPSPerGBIncrease)
+		if err != nil {
+			return nil, err
+		}
+	}
+
 	var tags []*ec2.Tag
 	for key, value := range diskOptions.Tags {
 		copiedKey := key
@@ -1221,26 +1241,29 @@ func getVolumeAttachmentsList(volume *ec2.Volume) []string {
 }
 
 // Calculate actual IOPS for a volume and cap it at supported AWS limits.
-// Using requstedIOPSPerGB allows users to create a "fast" storage class
-// (requstedIOPSPerGB = 50 for io1), which can provide the maximum iops
-// that AWS supports for any requestedCapacityGiB.
-func capIOPS(volumeType string, requestedCapacityGiB int64, requstedIOPSPerGB, minTotalIOPS, maxTotalIOPS, maxIOPSPerGB int64, allowIncrease bool) (int64, error) {
-	iops := requestedCapacityGiB * requstedIOPSPerGB
+func capIOPS(volumeType string, requestedCapacityGiB int64, requestedIops int64, minTotalIOPS, maxTotalIOPS, maxIOPSPerGB int64, allowIncrease bool) (int64, error) {
+	// If requestedIops is zero the user did not request a specific amount, and the default will be used instead
+	if requestedIops == 0 {
+		return 0, nil
+	}
+
+	iops := requestedIops
 
 	if iops < minTotalIOPS {
 		if allowIncrease {
 			iops = minTotalIOPS
 			klog.V(5).Infof("[Debug] Increased IOPS for %s %d GB volume to the min supported limit: %d", volumeType, requestedCapacityGiB, iops)
 		} else {
-			return 0, fmt.Errorf("invalid combination of volume size %d GB and iopsPerGB %d: the resulting IOPS %d is too low for AWS, it must be at least %d", requestedCapacityGiB, requstedIOPSPerGB, iops, minTotalIOPS)
+			return 0, fmt.Errorf("invalid IOPS: %d is too low, it must be at least %d", iops, minTotalIOPS)
 		}
 	}
 	if iops > maxTotalIOPS {
 		iops = maxTotalIOPS
 		klog.V(5).Infof("[Debug] Capped IOPS for %s %d GB volume at the max supported limit: %d", volumeType, requestedCapacityGiB, iops)
 	}
-	if iops > maxIOPSPerGB*requestedCapacityGiB {
-		iops = maxIOPSPerGB * requestedCapacityGiB
+	maxIopsByCapacity := maxIOPSPerGB * requestedCapacityGiB
+	if iops > maxIopsByCapacity && maxIopsByCapacity >= minTotalIOPS {
+		iops = maxIopsByCapacity
 		klog.V(5).Infof("[Debug] Capped IOPS for %s %d GB volume at %d IOPS/GB: %d", volumeType, requestedCapacityGiB, maxIOPSPerGB, iops)
 	}
 	return iops, nil

pkg/cloud/cloud_test.go

@@ -72,13 +72,13 @@ func TestCreateDisk(t *testing.T) {
 			name:       "success: normal with iops",
 			volumeName: "vol-test-name",
 			diskOptions: &DiskOptions{
-				CapacityBytes: util.GiBToBytes(1),
+				CapacityBytes: util.GiBToBytes(500),
 				Tags:          map[string]string{VolumeNameTagKey: "vol-test", AwsEbsDriverTagKey: "true"},
 				IOPS:          6000,
 			},
 			expDisk: &Disk{
 				VolumeID:         "vol-test",
-				CapacityGiB:      1,
+				CapacityGiB:      500,
 				AvailabilityZone: defaultZone,
 			},
 			expCreateVolumeInput: &ec2.CreateVolumeInput{
@@ -122,18 +122,56 @@ func TestCreateDisk(t *testing.T) {
 			expErr: nil,
 		},
 		{
-			name:       "success: normal with gp3 options",
+			name:       "success: io1 with IOPS parameter",
+			volumeName: "vol-test-name",
+			diskOptions: &DiskOptions{
+				CapacityBytes: util.GiBToBytes(200),
+				Tags:          map[string]string{VolumeNameTagKey: "vol-test", AwsEbsDriverTagKey: "true"},
+				VolumeType:    VolumeTypeIO1,
+				IOPS:          100,
+			},
+			expDisk: &Disk{
+				VolumeID:         "vol-test",
+				CapacityGiB:      200,
+				AvailabilityZone: defaultZone,
+			},
+			expCreateVolumeInput: &ec2.CreateVolumeInput{
+				Iops: aws.Int64(100),
+			},
+			expErr: nil,
+		},
+		{
+			name:       "success: io2 with IOPS parameter",
 			volumeName: "vol-test-name",
 			diskOptions: &DiskOptions{
 				CapacityBytes: util.GiBToBytes(1),
 				Tags:          map[string]string{VolumeNameTagKey: "vol-test", AwsEbsDriverTagKey: "true"},
+				VolumeType:    VolumeTypeIO2,
+				IOPS:          100,
+			},
+			expDisk: &Disk{
+				VolumeID:         "vol-test",
+				CapacityGiB:      1,
+				AvailabilityZone: defaultZone,
+			},
+			expCreateVolumeInput: &ec2.CreateVolumeInput{
+				Iops: aws.Int64(100),
+			},
+			expErr: nil,
+		},
+		{
+			name:       "success: normal with gp3 options",
+			volumeName: "vol-test-name",
+			diskOptions: &DiskOptions{
+				CapacityBytes: util.GiBToBytes(400),
+				Tags:          map[string]string{VolumeNameTagKey: "vol-test", AwsEbsDriverTagKey: "true"},
 				VolumeType:    VolumeTypeGP3,
 				IOPS:          3000,
 				Throughput:    125,
 			},
 			expDisk: &Disk{
 				VolumeID:         "vol-test",
-				CapacityGiB:      1,
+				CapacityGiB:      400,
 				AvailabilityZone: defaultZone,
 			},
 			expCreateVolumeInput: &ec2.CreateVolumeInput{
@@ -310,6 +348,24 @@ func TestCreateDisk(t *testing.T) {
 			},
 			expErr: nil,
 		},
+		{
+			name:       "fail: invalid StorageClass parameters; specified both IOPS and IOPSPerGb",
+			volumeName: "vol-test-name",
+			diskOptions: &DiskOptions{
+				CapacityBytes: util.GiBToBytes(4),
+				Tags:          map[string]string{VolumeNameTagKey: "vol-test", AwsEbsDriverTagKey: "true"},
+				VolumeType:    VolumeTypeIO1,
+				IOPS:          1,
+				IOPSPerGB:     1,
+			},
+			expDisk: &Disk{
+				VolumeID:         "vol-test",
+				CapacityGiB:      4,
+				AvailabilityZone: defaultZone,
+			},
+			expCreateVolumeInput: nil,
+			expErr:               fmt.Errorf("invalid StorageClass parameters; specify either IOPS or IOPSPerGb, not both"),
+		},
 		{
 			name:       "fail: io1 with too low iopsPerGB",
 			volumeName: "vol-test-name",
@@ -325,7 +381,7 @@ func TestCreateDisk(t *testing.T) {
 				AvailabilityZone: defaultZone,
 			},
 			expCreateVolumeInput: nil,
-			expErr:               fmt.Errorf("invalid combination of volume size 4 GB and iopsPerGB 1: the resulting IOPS 4 is too low for AWS, it must be at least 100"),
+			expErr:               fmt.Errorf("invalid IOPS: 4 is too low, it must be at least 100"),
 		},
 		{
 			name:       "success: small io1 with too high iopsPerGB",
@@ -400,7 +456,7 @@ func TestCreateDisk(t *testing.T) {
 				AvailabilityZone: defaultZone,
 			},
 			expCreateVolumeInput: nil,
-			expErr:               fmt.Errorf("invalid combination of volume size 4 GB and iopsPerGB 1: the resulting IOPS 4 is too low for AWS, it must be at least 100"),
+			expErr:               fmt.Errorf("invalid IOPS: 4 is too low, it must be at least 100"),
 		},
 		{
 			name:       "success: small io2 with too high iopsPerGB",

pkg/driver/controller.go

@@ -567,6 +567,29 @@ func (d *controllerService) CreateSnapshot(ctx context.Context, req *csi.CreateS
 		cloud.SnapshotNameTagKey: snapshotName,
 		cloud.AwsEbsDriverTagKey: isManagedByDriver,
 	}
+
+	var vscTags []string
+	for key, value := range req.GetParameters() {
+		if strings.HasPrefix(key, TagKeyPrefix) {
+			vscTags = append(vscTags, value)
+		} else {
+			return nil, status.Errorf(codes.InvalidArgument, "Invalid parameter key %s for CreateSnapshot", key)
+		}
+	}
+
+	addTags, err := template.Evaluate(vscTags, nil, d.driverOptions.warnOnInvalidTag)
+	if err != nil {
+		return nil, status.Errorf(codes.InvalidArgument, "Error interpolating the tag value: %v", err)
+	}
+
+	if err := validateExtraTags(addTags, d.driverOptions.warnOnInvalidTag); err != nil {
+		return nil, status.Errorf(codes.InvalidArgument, "Invalid tag value: %v", err)
+	}
+
+	for k, v := range addTags {
+		snapshotTags[k] = v
+	}
+
 	if d.driverOptions.kubernetesClusterID != "" {
 		resourceLifecycleTag := ResourceLifecycleTagPrefix + d.driverOptions.kubernetesClusterID
 		snapshotTags[resourceLifecycleTag] = ResourceLifecycleOwned

pkg/driver/controller_test.go

@@ -2295,6 +2295,63 @@ func TestCreateSnapshot(t *testing.T) {
 				checkExpectedErrorCode(t, err, codes.Aborted)
 			},
 		},
+		{
+			name: "success with VolumeSnapshotClass tags",
+			testFunc: func(t *testing.T) {
+				const (
+					snapshotName  = "test-snapshot"
+					extraTagKey   = "test-key"
+					extraTagValue = "test-value"
+				)
+
+				req := &csi.CreateSnapshotRequest{
+					Name: snapshotName,
+					Parameters: map[string]string{
+						"tagSpecification_1": fmt.Sprintf("%s=%s", extraTagKey, extraTagValue),
+					},
+					SourceVolumeId: "vol-test",
+				}
+				expSnapshot := &csi.Snapshot{
+					ReadyToUse: true,
+				}
+
+				ctx := context.Background()
+				mockSnapshot := &cloud.Snapshot{
+					SnapshotID:     fmt.Sprintf("snapshot-%d", rand.New(rand.NewSource(time.Now().UnixNano())).Uint64()),
+					SourceVolumeID: req.SourceVolumeId,
+					Size:           1,
+					CreationTime:   time.Now(),
+				}
+				mockCtl := gomock.NewController(t)
+				defer mockCtl.Finish()
+
+				snapshotOptions := &cloud.SnapshotOptions{
+					Tags: map[string]string{
+						cloud.SnapshotNameTagKey: snapshotName,
+						cloud.AwsEbsDriverTagKey: isManagedByDriver,
+						extraTagKey:              extraTagValue,
+					},
+				}
+
+				mockCloud := cloud.NewMockCloud(mockCtl)
+				mockCloud.EXPECT().CreateSnapshot(gomock.Eq(ctx), gomock.Eq(req.SourceVolumeId), gomock.Eq(snapshotOptions)).Return(mockSnapshot, nil)
+				mockCloud.EXPECT().GetSnapshotByName(gomock.Eq(ctx), gomock.Eq(req.GetName())).Return(nil, cloud.ErrNotFound)
+
+				awsDriver := controllerService{
+					cloud:         mockCloud,
+					inFlight:      internal.NewInFlight(),
+					driverOptions: &DriverOptions{},
+				}
+				resp, err := awsDriver.CreateSnapshot(context.Background(), req)
+				if err != nil {
+					t.Fatalf("Unexpected error: %v", err)
+				}
+
+				if snap := resp.GetSnapshot(); snap == nil {
+					t.Fatalf("Expected snapshot %v, got nil", expSnapshot)
+				}
+			},
+		},
 	}
 
 	for _, tc := range testCases {

tests/e2e/driver/ebs_csi_driver.go

@@ -160,8 +160,9 @@ func IOPSForVolumeType(volumeType string) string {
 	switch volumeType {
 	case "gp3":
 		// Maximum IOPS for gp3 is 16000. However, maximum IOPS/GB for gp3 is 500.
-		// Since the tests will run using minimum volume capacity (1GB), set to 500.
-		return "500"
+		// Since the tests will run using minimum volume capacity (1GB), set to 3000
+		// because minimum IOPS for gp3 is 3000.
+		return "3000"
 	default:
 		return ""
 	}