Merge pull request #563 from saad-ali/fixCmekMatch

Fix Random CMEK Disk Creation Failure (disk already exists)

Author: k8s-ci-robot

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

@@ -35,6 +35,7 @@ const (
 	operationStatusDone            = "DONE"
 	waitForSnapshotCreationTimeOut = 2 * time.Minute
 	diskKind                       = "compute#disk"
+	cryptoKeyVerDelimiter          = "/cryptoKeyVersions"
 )
 
 type GCECompute interface {
@@ -256,7 +257,9 @@ func ValidateDiskParameters(disk *CloudDisk, params common.DiskParameters) error
 		return fmt.Errorf("actual disk replication type %v did not match expected param %s", disk.Type(), "regional-pd")
 	}
 
-	if disk.GetKMSKeyName() != params.DiskEncryptionKMSKey {
+	if !kmsKeyEqual(
+		disk.GetKMSKeyName(), /* fetchedKMSKey */
+		params.DiskEncryptionKMSKey /* storageClassKMSKey */) {
 		return fmt.Errorf("actual disk KMS key name %s did not match expected param %s", disk.GetKMSKeyName(), params.DiskEncryptionKMSKey)
 	}
 
@@ -767,3 +770,21 @@ func (cloud *CloudProvider) waitForSnapshotCreation(ctx context.Context, snapsho
 		}
 	}
 }
+
+// kmsKeyEqual returns true if fetchedKMSKey and storageClassKMSKey refer to the same key.
+// fetchedKMSKey - key returned by the server
+//        example: projects/{0}/locations/{1}/keyRings/{2}/cryptoKeys/{3}/cryptoKeyVersions/{4}
+// storageClassKMSKey - key as provided by the client
+//        example: projects/{0}/locations/{1}/keyRings/{2}/cryptoKeys/{3}
+// cryptoKeyVersions should be disregarded if the rest of the key is identical.
+func kmsKeyEqual(fetchedKMSKey, storageClassKMSKey string) bool {
+	return removeCryptoKeyVersion(fetchedKMSKey) == removeCryptoKeyVersion(storageClassKMSKey)
+}
+
+func removeCryptoKeyVersion(kmsKey string) string {
+	i := strings.LastIndex(kmsKey, cryptoKeyVerDelimiter)
+	if i > 0 {
+		return kmsKey[:i]
+	}
+	return kmsKey
+}

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

@@ -0,0 +1,105 @@
+/*
+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 gcecloudprovider
+
+import (
+	"testing"
+
+	computev1 "google.golang.org/api/compute/v1"
+	"sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/common"
+)
+
+func TestValidateDiskParameters(t *testing.T) {
+	testCases := []struct {
+		fetchedKMSKey      string
+		storageClassKMSKey string
+		expectErr          bool
+	}{
+		{
+			fetchedKMSKey:      "projects/my-project/locations/us-central1/keyRings/TestKeyRing/cryptoKeys/test-key/cryptoKeyVersions/8",
+			storageClassKMSKey: "projects/my-project/locations/us-central1/keyRings/TestKeyRing/cryptoKeys/test-key",
+			expectErr:          false,
+		},
+		{
+			fetchedKMSKey:      "projects/my-project/locations/us-central1/keyRings/TestKeyRing/cryptoKeys/test-key",
+			storageClassKMSKey: "projects/my-project/locations/us-central1/keyRings/TestKeyRing/cryptoKeys/test-key",
+			expectErr:          false,
+		},
+		{
+			fetchedKMSKey:      "projects/my-project/locations/us-central1/keyRings/TestKeyRing/cryptoKeys/garbage/cryptoKeyVersions/8",
+			storageClassKMSKey: "projects/my-project/locations/us-central1/keyRings/TestKeyRing/cryptoKeys/test-key",
+			expectErr:          true,
+		},
+		{
+			fetchedKMSKey:      "projects/my-project/locations/us-central1/keyRings/TestKeyRing/cryptoKeys/garbage",
+			storageClassKMSKey: "projects/my-project/locations/us-central1/keyRings/TestKeyRing/cryptoKeys/test-key",
+			expectErr:          true,
+		},
+		{
+			fetchedKMSKey:      "projects/my-project/locations/us-central1/keyRings/TestKeyRing/cryptoKeys/test-key",
+			storageClassKMSKey: "projects/my-project/locations/us-west1/keyRings/TestKeyRing/cryptoKeys/test-key",
+			expectErr:          true,
+		},
+		{
+			fetchedKMSKey:      "projects/my-project/locations/us-central1/keyRings/TestKeyRing/cryptoKeys/foobar/cryptoKeyVersions/8",
+			storageClassKMSKey: "projects/my-project/locations/us-central1/keyRings/TestKeyRing/cryptoKeys/foo",
+			expectErr:          true,
+		},
+		{
+			fetchedKMSKey:      "projects/my-project/locations/us-central1/keyRings/TestKeyRing/cryptoKeys/foobar",
+			storageClassKMSKey: "projects/my-project/locations/us-central1/keyRings/TestKeyRing/cryptoKeys/foo",
+			expectErr:          true,
+		},
+	}
+
+	for i, tc := range testCases {
+		// Arrange
+		existingDisk := &CloudDisk{
+			ZonalDisk: &computev1.Disk{
+				Id:                546559531467326555,
+				CreationTimestamp: "2020-07-24T17:20:06.292-07:00",
+				Name:              "test-disk",
+				SizeGb:            500,
+				Zone:              "https://www.googleapis.com/compute/v1/projects/my-project/zones/us-central1-c",
+				Status:            "READY",
+				SelfLink:          "https://www.googleapis.com/compute/v1/projects/my-project/zones/us-central1-c/disks/test-disk",
+				Type:              "https://www.googleapis.com/compute/v1/projects/my-project/zones/us-central1-c/diskTypes/pd-standard",
+				DiskEncryptionKey: &computev1.CustomerEncryptionKey{
+					KmsKeyName: tc.fetchedKMSKey,
+				},
+				LabelFingerprint:       "42WmSpB8rSM=",
+				PhysicalBlockSizeBytes: 4096,
+				Kind:                   "compute#disk",
+			},
+		}
+
+		storageClassParams := common.DiskParameters{
+			DiskType:             "pd-standard",
+			ReplicationType:      "none",
+			DiskEncryptionKMSKey: tc.storageClassKMSKey,
+		}
+
+		// Act
+		err := ValidateDiskParameters(existingDisk, storageClassParams)
+
+		// Assert
+		if !tc.expectErr && err != nil {
+			t.Fatalf("Test case #%v: ValidateDiskParameters did not expect error, but got %v", i, err)
+		}
+		if tc.expectErr && err == nil {
+			t.Fatalf("Test case #%v: ValidateDiskParameters expected error, but got no error", i)
+		}
+	}
+}