Merge pull request kubernetes-sigs#467 from davidz627/fix/vvc

Implement ValidateVolumeCapabilities and refactor parameter handling for more comprehensive validation of existing disks in all cloud calls

Author: k8s-ci-robot

pkg/common/constants.go

@@ -17,11 +17,6 @@ limitations under the License.
 package common
 
 const (
-	// Keys for Storage Class Parameters
-	ParameterKeyType                 = "type"
-	ParameterKeyReplicationType      = "replication-type"
-	ParameterKeyDiskEncryptionKmsKey = "disk-encryption-kms-key"
-
 	// Keys for Topology. This key will be shared amongst drivers from GCP
 	TopologyKeyZone = "topology.gke.io/zone"
 

pkg/common/parameters.go

@@ -0,0 +1,75 @@
+/*
+Copyright 2020 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package common
+
+import (
+	"fmt"
+	"strings"
+)
+
+const (
+	ParameterKeyType                 = "type"
+	ParameterKeyReplicationType      = "replication-type"
+	ParameterKeyDiskEncryptionKmsKey = "disk-encryption-kms-key"
+
+	replicationTypeNone = "none"
+)
+
+// DiskParameters contains normalized and defaulted disk parameters
+type DiskParameters struct {
+	// Values: pd-standard OR pd-ssd
+	// Default: pd-standard
+	DiskType string
+	// Values: "none", regional-pd
+	// Default: "none"
+	ReplicationType string
+	// Values: {string}
+	// Default: ""
+	DiskEncryptionKMSKey string
+}
+
+// ExtractAndDefaultParameters will take the relevant parameters from a map and
+// put them into a well defined struct making sure to default unspecified fields
+func ExtractAndDefaultParameters(parameters map[string]string) (DiskParameters, error) {
+	p := DiskParameters{
+		DiskType:             "pd-standard",       // Default
+		ReplicationType:      replicationTypeNone, // Default
+		DiskEncryptionKMSKey: "",                  // Default
+	}
+	for k, v := range parameters {
+		if k == "csiProvisionerSecretName" || k == "csiProvisionerSecretNamespace" {
+			// These are hardcoded secrets keys required to function but not needed by GCE PD
+			continue
+		}
+		switch strings.ToLower(k) {
+		case ParameterKeyType:
+			if v != "" {
+				p.DiskType = strings.ToLower(v)
+			}
+		case ParameterKeyReplicationType:
+			if v != "" {
+				p.ReplicationType = strings.ToLower(v)
+			}
+		case ParameterKeyDiskEncryptionKmsKey:
+			// Resource names (e.g. "keyRings", "cryptoKeys", etc.) are case sensitive, so do not change case
+			p.DiskEncryptionKMSKey = v
+		default:
+			return p, fmt.Errorf("parameters contains invalid option %q", k)
+		}
+	}
+	return p, nil
+}

pkg/common/parameters_test.go

@@ -0,0 +1,89 @@
+/*
+Copyright 2020 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package common
+
+import (
+	"reflect"
+	"testing"
+)
+
+func TestExtractAndDefaultParameters(t *testing.T) {
+	tests := []struct {
+		name         string
+		parameters   map[string]string
+		expectParams DiskParameters
+		expectErr    bool
+	}{
+		{
+			name:       "defaults",
+			parameters: map[string]string{},
+			expectParams: DiskParameters{
+				DiskType:             "pd-standard",
+				ReplicationType:      "none",
+				DiskEncryptionKMSKey: "",
+			},
+		},
+		{
+			name:       "specified empties",
+			parameters: map[string]string{ParameterKeyType: "", ParameterKeyReplicationType: "", ParameterKeyDiskEncryptionKmsKey: ""},
+			expectParams: DiskParameters{
+				DiskType:             "pd-standard",
+				ReplicationType:      "none",
+				DiskEncryptionKMSKey: "",
+			},
+		},
+		{
+			name:       "random keys",
+			parameters: map[string]string{ParameterKeyType: "", "foo": "", ParameterKeyDiskEncryptionKmsKey: ""},
+			expectErr:  true,
+		},
+		{
+			name:       "real values",
+			parameters: map[string]string{ParameterKeyType: "pd-ssd", ParameterKeyReplicationType: "regional-pd", ParameterKeyDiskEncryptionKmsKey: "foo/key"},
+			expectParams: DiskParameters{
+				DiskType:             "pd-ssd",
+				ReplicationType:      "regional-pd",
+				DiskEncryptionKMSKey: "foo/key",
+			},
+		},
+		{
+			name:       "partial spec",
+			parameters: map[string]string{ParameterKeyDiskEncryptionKmsKey: "foo/key"},
+			expectParams: DiskParameters{
+				DiskType:             "pd-standard",
+				ReplicationType:      "none",
+				DiskEncryptionKMSKey: "foo/key",
+			},
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			p, err := ExtractAndDefaultParameters(tc.parameters)
+			if gotErr := err != nil; gotErr != tc.expectErr {
+				t.Fatalf("ExtractAndDefaultParameters(%+v) = %v; expectedErr: %v", tc.parameters, err, tc.expectErr)
+			}
+			if err != nil {
+				return
+			}
+
+			if !reflect.DeepEqual(p, tc.expectParams) {
+				t.Errorf("ExtractAndDefaultParameters(%+v) = %v; expected params: %v", tc.parameters, p, tc.expectParams)
+			}
+		})
+	}
+}

pkg/gce-cloud-provider/compute/cloud-disk.go

@@ -15,6 +15,8 @@ limitations under the License.
 package gcecloudprovider
 
 import (
+	"strings"
+
 	computev1 "google.golang.org/api/compute/v1"
 )
 
@@ -90,15 +92,21 @@ func (d *CloudDisk) GetKind() string {
 	}
 }
 
-func (d *CloudDisk) GetType() string {
+// GetPDType returns the type of the PD as either 'pd-standard' or 'pd-ssd' The
+// "Type" field on the compute disk is stored as a url like
+// projects/project/zones/zone/diskTypes/pd-standard
+func (d *CloudDisk) GetPDType() string {
+	var pdType string
 	switch d.Type() {
 	case Zonal:
-		return d.ZonalDisk.Type
+		pdType = d.ZonalDisk.Type
 	case Regional:
-		return d.RegionalDisk.Type
+		pdType = d.RegionalDisk.Type
 	default:
 		return ""
 	}
+	respType := strings.Split(pdType, "/")
+	return strings.TrimSpace(respType[len(respType)-1])
 }
 
 func (d *CloudDisk) GetSelfLink() string {
@@ -155,3 +163,19 @@ func (d *CloudDisk) GetSnapshotId() string {
 		return ""
 	}
 }
+
+func (d *CloudDisk) GetKMSKeyName() string {
+	var dek *computev1.CustomerEncryptionKey
+	switch d.Type() {
+	case Zonal:
+		dek = d.ZonalDisk.DiskEncryptionKey
+	case Regional:
+		dek = d.RegionalDisk.DiskEncryptionKey
+	default:
+		return ""
+	}
+	if dek == nil {
+		return ""
+	}
+	return dek.KmsKeyName
+}

pkg/gce-cloud-provider/compute/fake-gce.go

@@ -215,7 +215,7 @@ func (cloud *FakeCloudProvider) GetDisk(ctx context.Context, volKey *meta.Key) (
 	return disk, nil
 }
 
-func (cloud *FakeCloudProvider) ValidateExistingDisk(ctx context.Context, resp *CloudDisk, diskType string, reqBytes, limBytes int64) error {
+func (cloud *FakeCloudProvider) ValidateExistingDisk(ctx context.Context, resp *CloudDisk, params common.DiskParameters, reqBytes, limBytes int64) error {
 	if resp == nil {
 		return fmt.Errorf("disk does not exist")
 	}
@@ -227,20 +227,12 @@ func (cloud *FakeCloudProvider) ValidateExistingDisk(ctx context.Context, resp *
 			reqBytes, common.GbToBytes(resp.GetSizeGb()), limBytes)
 	}
 
-	respType := strings.Split(resp.GetType(), "/")
-	typeMatch := strings.TrimSpace(respType[len(respType)-1]) == strings.TrimSpace(diskType)
-	typeDefault := diskType == "" && strings.TrimSpace(respType[len(respType)-1]) == "pd-standard"
-	if !typeMatch && !typeDefault {
-		return fmt.Errorf("disk already exists with incompatible type. Need %v. Got %v",
-			diskType, respType[len(respType)-1])
-	}
-	klog.V(4).Infof("Compatible disk already exists")
-	return nil
+	return ValidateDiskParameters(resp, params)
 }
 
-func (cloud *FakeCloudProvider) InsertDisk(ctx context.Context, volKey *meta.Key, diskType string, capBytes int64, capacityRange *csi.CapacityRange, replicaZones []string, snapshotID, diskEncryptionKmsKey string) error {
+func (cloud *FakeCloudProvider) InsertDisk(ctx context.Context, volKey *meta.Key, params common.DiskParameters, capBytes int64, capacityRange *csi.CapacityRange, replicaZones []string, snapshotID string) error {
 	if disk, ok := cloud.disks[volKey.Name]; ok {
-		err := cloud.ValidateExistingDisk(ctx, disk, diskType,
+		err := cloud.ValidateExistingDisk(ctx, disk, params,
 			int64(capacityRange.GetRequiredBytes()),
 			int64(capacityRange.GetLimitBytes()))
 		if err != nil {
@@ -255,13 +247,13 @@ func (cloud *FakeCloudProvider) InsertDisk(ctx context.Context, volKey *meta.Key
 			Name:             volKey.Name,
 			SizeGb:           common.BytesToGb(capBytes),
 			Description:      "Disk created by GCE-PD CSI Driver",
-			Type:             cloud.GetDiskTypeURI(volKey, diskType),
+			Type:             cloud.GetDiskTypeURI(volKey, params.DiskType),
 			SelfLink:         fmt.Sprintf("projects/%s/zones/%s/disks/%s", cloud.project, volKey.Zone, volKey.Name),
 			SourceSnapshotId: snapshotID,
 		}
-		if diskEncryptionKmsKey != "" {
+		if params.DiskEncryptionKMSKey != "" {
 			diskToCreateGA.DiskEncryptionKey = &computev1.CustomerEncryptionKey{
-				KmsKeyName: diskEncryptionKmsKey,
+				KmsKeyName: params.DiskEncryptionKMSKey,
 			}
 		}
 		diskToCreate = ZonalCloudDisk(diskToCreateGA)
@@ -270,13 +262,13 @@ func (cloud *FakeCloudProvider) InsertDisk(ctx context.Context, volKey *meta.Key
 			Name:             volKey.Name,
 			SizeGb:           common.BytesToGb(capBytes),
 			Description:      "Regional disk created by GCE-PD CSI Driver",
-			Type:             cloud.GetDiskTypeURI(volKey, diskType),
+			Type:             cloud.GetDiskTypeURI(volKey, params.DiskType),
 			SelfLink:         fmt.Sprintf("projects/%s/regions/%s/disks/%s", cloud.project, volKey.Region, volKey.Name),
 			SourceSnapshotId: snapshotID,
 		}
-		if diskEncryptionKmsKey != "" {
+		if params.DiskEncryptionKMSKey != "" {
 			diskToCreateV1.DiskEncryptionKey = &computev1.CustomerEncryptionKey{
-				KmsKeyName: diskEncryptionKmsKey,
+				KmsKeyName: params.DiskEncryptionKMSKey,
 			}
 		}
 		diskToCreate = RegionalCloudDisk(diskToCreateV1)