Skip to content

Commit

Permalink
Merge pull request #467 from davidz627/fix/vvc
Browse files Browse the repository at this point in the history
Implement ValidateVolumeCapabilities and refactor parameter handling for more comprehensive validation of existing disks in all cloud calls
  • Loading branch information
k8s-ci-robot authored Feb 13, 2020
2 parents 7552602 + 57dd986 commit a38158b
Show file tree
Hide file tree
Showing 7 changed files with 294 additions and 150 deletions.
5 changes: 0 additions & 5 deletions pkg/common/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down
75 changes: 75 additions & 0 deletions pkg/common/parameters.go
Original file line number Diff line number Diff line change
@@ -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
}
89 changes: 89 additions & 0 deletions pkg/common/parameters_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
})
}
}
30 changes: 27 additions & 3 deletions pkg/gce-cloud-provider/compute/cloud-disk.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ limitations under the License.
package gcecloudprovider

import (
"strings"

computev1 "google.golang.org/api/compute/v1"
)

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
28 changes: 10 additions & 18 deletions pkg/gce-cloud-provider/compute/fake-gce.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand All @@ -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 {
Expand All @@ -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)
Expand All @@ -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)
Expand Down
Loading

0 comments on commit a38158b

Please sign in to comment.