Merge pull request #890 from amacaskill/zonal-from-regional

add create regional clone from zonal disk and improve tests

Author: k8s-ci-robot

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

@@ -330,7 +330,7 @@ func ValidateDiskParameters(disk *CloudDisk, params common.DiskParameters) error
 		return fmt.Errorf("actual disk replication type %v did not match expected param %s", locationType, params.ReplicationType)
 	}
 
-	if !kmsKeyEqual(
+	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)
@@ -1119,7 +1119,7 @@ func (cloud *CloudProvider) waitForSnapshotCreation(ctx context.Context, project
 // 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 {
+func KmsKeyEqual(fetchedKMSKey, storageClassKMSKey string) bool {
 	return removeCryptoKeyVersion(fetchedKMSKey) == removeCryptoKeyVersion(storageClassKMSKey)
 }
 

pkg/gce-pd-csi-driver/controller.go

@@ -243,10 +243,38 @@ func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.Cre
 					return nil, status.Error(codes.Internal, fmt.Sprintf("CreateVolume unknown get disk error when validating: %v", err))
 				}
 			}
-			// Verify the zone, region, and disk type of the clone must be the same as that of the source disk.
-			if err := gce.ValidateDiskParameters(diskFromSourceVolume, params); err != nil {
-				return nil, status.Errorf(codes.InvalidArgument, `CreateVolume source volume parameters do not match CreateVolumeRequest Parameters: %v`, err)
+
+			// Verify the disk type and encryption key of the clone are the same as that of the source disk.
+			if diskFromSourceVolume.GetPDType() != params.DiskType || !gce.KmsKeyEqual(diskFromSourceVolume.GetKMSKeyName(), params.DiskEncryptionKMSKey) {
+				return nil, status.Errorf(codes.InvalidArgument, "CreateVolume Parameters %v do not match source volume Parameters", params)
+			}
+			// Verify the disk capacity range are the same or greater as that of the source disk.
+			if diskFromSourceVolume.GetSizeGb() > common.BytesToGbRoundDown(capBytes) {
+				return nil, status.Errorf(codes.InvalidArgument, "CreateVolume disk CapacityRange %d is less than source volume CapacityRange %d", common.BytesToGbRoundDown(capBytes), diskFromSourceVolume.GetSizeGb())
+			}
+
+			if params.ReplicationType == replicationTypeNone {
+				// For zonal->zonal disk clones, verify the zone is the same as that of the source disk.
+				if sourceVolKey.Zone != volKey.Zone {
+					return nil, status.Errorf(codes.InvalidArgument, "CreateVolume disk zone %s does not match source volume zone %s", volKey.Zone, sourceVolKey.Zone)
+				}
+				// regional->zonal disk clones are not allowed.
+				if diskFromSourceVolume.LocationType() == meta.Regional {
+					return nil, status.Errorf(codes.InvalidArgument, "Cannot create a zonal disk clone from a regional disk")
+				}
 			}
+
+			if params.ReplicationType == replicationTypeNone {
+				// For regional->regional disk clones, verify the region is the same as that of the source disk.
+				if diskFromSourceVolume.LocationType() == meta.Regional && sourceVolKey.Region != volKey.Region {
+					return nil, status.Errorf(codes.InvalidArgument, "CreateVolume disk region %s does not match source volume region %s", volKey.Region, sourceVolKey.Region)
+				}
+				// For zonal->regional disk clones, verify one of the replica zones matches the source disk zone.
+				if diskFromSourceVolume.LocationType() == meta.Zonal && !containsZone(zones, sourceVolKey.Zone) {
+					return nil, status.Errorf(codes.InvalidArgument, "CreateVolume regional disk replica zones %v do not match source volume zone %s", zones, sourceVolKey.Zone)
+				}
+			}
+
 			// Verify the source disk is ready.
 			ready, err := isDiskReady(diskFromSourceVolume)
 			if err != nil {

pkg/gce-pd-csi-driver/controller_test.go

@@ -1004,97 +1004,200 @@ func TestCreateVolumeWithVolumeSourceFromVolume(t *testing.T) {
 	testSourceVolumeName := "test-volume-source-name"
 	testZonalVolumeSourceID := fmt.Sprintf("projects/%s/zones/%s/disks/%s", project, zone, testSourceVolumeName)
 	testRegionalVolumeSourceID := fmt.Sprintf("projects/%s/regions/%s/disks/%s", project, region, testSourceVolumeName)
-	testVolumeSourceIDDifferentZone := fmt.Sprintf("projects/%s/zones/%s/disks/%s", project, "different-zone", testSourceVolumeName)
+	testSecondZonalVolumeSourceID := fmt.Sprintf("projects/%s/zones/%s/disks/%s", project, "different-zone1", testSourceVolumeName)
+	zonalParams := map[string]string{
+		common.ParameterKeyType: "test-type", common.ParameterKeyReplicationType: replicationTypeNone,
+		common.ParameterKeyDiskEncryptionKmsKey: "encryption-key",
+	}
+	regionalParams := map[string]string{
+		common.ParameterKeyType: "test-type", common.ParameterKeyReplicationType: replicationTypeRegionalPD,
+		common.ParameterKeyDiskEncryptionKmsKey: "encryption-key",
+	}
 	topology := &csi.TopologyRequirement{
 		Requisite: []*csi.Topology{
 			{
-				Segments: map[string]string{common.TopologyKeyZone: region + "-b"},
+				Segments: map[string]string{common.TopologyKeyZone: zone},
 			},
 			{
-				Segments: map[string]string{common.TopologyKeyZone: region + "-c"},
+				Segments: map[string]string{common.TopologyKeyZone: secondZone},
 			},
 		},
 	}
-	regionalParams := map[string]string{
-		common.ParameterKeyType: "test-type", common.ParameterKeyReplicationType: "regional-pd",
-	}
+
 	// Define test cases
 	testCases := []struct {
-		name                string
-		volumeOnCloud       bool
-		expErrCode          codes.Code
-		sourceVolumeID      string
-		reqParameters       map[string]string
-		sourceReqParameters map[string]string
-		topology            *csi.TopologyRequirement
+		name                 string
+		volumeOnCloud        bool
+		expErrCode           codes.Code
+		sourceVolumeID       string
+		reqParameters        map[string]string
+		sourceReqParameters  map[string]string
+		sourceCapacityRange  *csi.CapacityRange
+		requestCapacityRange *csi.CapacityRange
+		sourceTopology       *csi.TopologyRequirement
+		requestTopology      *csi.TopologyRequirement
 	}{
 		{
-			name:                "success with data source of zonal volume type",
-			volumeOnCloud:       true,
-			sourceVolumeID:      testZonalVolumeSourceID,
-			reqParameters:       stdParams,
-			sourceReqParameters: stdParams,
-		},
-		{
-			name:                "success with data source of regional volume type",
-			volumeOnCloud:       true,
-			sourceVolumeID:      testRegionalVolumeSourceID,
-			reqParameters:       regionalParams,
-			sourceReqParameters: regionalParams,
-			topology:            topology,
-		},
-		{
-			name:                "fail with with data source of replication-type different from CreateVolumeRequest",
-			volumeOnCloud:       true,
-			expErrCode:          codes.InvalidArgument,
-			sourceVolumeID:      testZonalVolumeSourceID,
-			reqParameters:       stdParams,
-			sourceReqParameters: regionalParams,
-			topology:            topology,
-		},
-		{
-			name:                "fail with data source of zonal volume type that doesn't exist",
-			volumeOnCloud:       false,
-			expErrCode:          codes.NotFound,
-			sourceVolumeID:      testZonalVolumeSourceID,
-			reqParameters:       stdParams,
-			sourceReqParameters: stdParams,
-		},
-		{
-			name:                "fail with data source of zonal volume type with invalid volume id format",
-			volumeOnCloud:       false,
-			expErrCode:          codes.InvalidArgument,
-			sourceVolumeID:      testZonalVolumeSourceID + "invalid/format",
-			reqParameters:       stdParams,
-			sourceReqParameters: stdParams,
+			name:                 "success zonal disk clone of zonal source disk",
+			volumeOnCloud:        true,
+			sourceVolumeID:       testZonalVolumeSourceID,
+			requestCapacityRange: stdCapRange,
+			sourceCapacityRange:  stdCapRange,
+			reqParameters:        zonalParams,
+			sourceReqParameters:  zonalParams,
+			sourceTopology:       topology,
+			requestTopology:      topology,
+		},
+		{
+			name:                 "success regional disk clone of regional source disk",
+			volumeOnCloud:        true,
+			sourceVolumeID:       testRegionalVolumeSourceID,
+			requestCapacityRange: stdCapRange,
+			sourceCapacityRange:  stdCapRange,
+			reqParameters:        regionalParams,
+			sourceReqParameters:  regionalParams,
+			sourceTopology:       topology,
+			requestTopology:      topology,
+		},
+		{
+			name:                 "success regional disk clone of zonal data source",
+			volumeOnCloud:        true,
+			sourceVolumeID:       testZonalVolumeSourceID,
+			requestCapacityRange: stdCapRange,
+			sourceCapacityRange:  stdCapRange,
+			reqParameters:        regionalParams,
+			sourceReqParameters:  zonalParams,
+			sourceTopology:       topology,
+			requestTopology:      topology,
+		},
+		{
+			name:                 "fail regional disk clone with no matching replica zone of zonal data source",
+			volumeOnCloud:        true,
+			expErrCode:           codes.InvalidArgument,
+			sourceVolumeID:       testZonalVolumeSourceID,
+			requestCapacityRange: stdCapRange,
+			sourceCapacityRange:  stdCapRange,
+			reqParameters:        regionalParams,
+			sourceReqParameters:  zonalParams,
+			sourceTopology:       topology,
+			requestTopology: &csi.TopologyRequirement{
+				Requisite: []*csi.Topology{
+					{
+						Segments: map[string]string{common.TopologyKeyZone: "different-zone1"},
+					},
+					{
+						Segments: map[string]string{common.TopologyKeyZone: "different-zone2"},
+					},
+				},
+			},
 		},
 		{
-			name:           "fail with data source of zonal volume type with invalid disk parameters",
-			volumeOnCloud:  true,
-			expErrCode:     codes.InvalidArgument,
-			sourceVolumeID: testVolumeSourceIDDifferentZone,
-			reqParameters:  stdParams,
+			name:                 "fail zonal disk clone with different disk type",
+			volumeOnCloud:        true,
+			expErrCode:           codes.InvalidArgument,
+			sourceVolumeID:       testZonalVolumeSourceID,
+			requestCapacityRange: stdCapRange,
+			sourceCapacityRange:  stdCapRange,
+			reqParameters:        zonalParams,
 			sourceReqParameters: map[string]string{
 				common.ParameterKeyType: "different-type",
 			},
+			sourceTopology:  topology,
+			requestTopology: topology,
 		},
 		{
-			name:                "fail with data source of zonal volume type with invalid replication type",
-			volumeOnCloud:       true,
-			expErrCode:          codes.InvalidArgument,
-			sourceVolumeID:      testZonalVolumeSourceID,
-			reqParameters:       regionalParams,
-			sourceReqParameters: stdParams,
-		},
-	}
+			name:                 "fail zonal disk clone with different DiskEncryptionKMSKey",
+			volumeOnCloud:        true,
+			expErrCode:           codes.InvalidArgument,
+			sourceVolumeID:       testZonalVolumeSourceID,
+			requestCapacityRange: stdCapRange,
+			sourceCapacityRange:  stdCapRange,
+			reqParameters:        zonalParams,
+			sourceReqParameters: map[string]string{
+				common.ParameterKeyType: "test-type", common.ParameterKeyReplicationType: replicationTypeNone,
+				common.ParameterKeyDiskEncryptionKmsKey: "different-encryption-key",
+			},
+			sourceTopology:  topology,
+			requestTopology: topology,
+		},
+		{
+			name:                 "fail zonal disk clone with different zone",
+			volumeOnCloud:        true,
+			expErrCode:           codes.InvalidArgument,
+			sourceVolumeID:       testSecondZonalVolumeSourceID,
+			requestCapacityRange: stdCapRange,
+			sourceCapacityRange:  stdCapRange,
+			reqParameters:        zonalParams,
+			sourceReqParameters:  zonalParams,
+			sourceTopology: &csi.TopologyRequirement{
+				Requisite: []*csi.Topology{
+					{
+						Segments: map[string]string{common.TopologyKeyZone: "different-zone1"},
+					},
+					{
+						Segments: map[string]string{common.TopologyKeyZone: "different-zone2"},
+					},
+				},
+			},
+			requestTopology: topology,
+		},
+		{
+			name:                 "fail zonal disk clone of regional data source",
+			volumeOnCloud:        true,
+			expErrCode:           codes.InvalidArgument,
+			sourceVolumeID:       testRegionalVolumeSourceID,
+			requestCapacityRange: stdCapRange,
+			sourceCapacityRange:  stdCapRange,
+			reqParameters:        zonalParams,
+			sourceReqParameters:  regionalParams,
+			sourceTopology:       topology,
+			requestTopology:      topology,
+		},
+
+		{
+			name:                 "fail zonal source disk does not exist",
+			volumeOnCloud:        false,
+			expErrCode:           codes.NotFound,
+			sourceVolumeID:       testZonalVolumeSourceID,
+			requestCapacityRange: stdCapRange,
+			sourceCapacityRange:  stdCapRange,
+			reqParameters:        stdParams,
+			sourceReqParameters:  stdParams,
+			requestTopology:      topology,
+		},
+		{
+			name:                 "fail invalid source disk volume id format",
+			volumeOnCloud:        false,
+			expErrCode:           codes.InvalidArgument,
+			sourceVolumeID:       testZonalVolumeSourceID + "/invalid/format",
+			requestCapacityRange: stdCapRange,
+			sourceCapacityRange:  stdCapRange,
+			reqParameters:        stdParams,
+			sourceReqParameters:  stdParams,
+			requestTopology:      topology,
+		},
+		{
+			name:                 "fail zonal disk clone with smaller disk capacity",
+			volumeOnCloud:        true,
+			expErrCode:           codes.InvalidArgument,
+			sourceVolumeID:       testZonalVolumeSourceID,
+			requestCapacityRange: stdCapRange,
+			sourceCapacityRange: &csi.CapacityRange{
+				RequiredBytes: common.GbToBytes(40),
+			},
+			reqParameters:       zonalParams,
+			sourceReqParameters: zonalParams,
+			sourceTopology:      topology,
+			requestTopology:     topology,
+		}}
 
 	for _, tc := range testCases {
 		t.Logf("test case: %s", tc.name)
 		gceDriver := initGCEDriver(t, nil)
 
 		req := &csi.CreateVolumeRequest{
 			Name:               name,
-			CapacityRange:      stdCapRange,
+			CapacityRange:      tc.requestCapacityRange,
 			VolumeCapabilities: stdVolCaps,
 			Parameters:         tc.reqParameters,
 			VolumeContentSource: &csi.VolumeContentSource{
@@ -1104,18 +1207,15 @@ func TestCreateVolumeWithVolumeSourceFromVolume(t *testing.T) {
 					},
 				},
 			},
+			AccessibilityRequirements: tc.requestTopology,
 		}
 
 		sourceVolumeRequest := &csi.CreateVolumeRequest{
-			Name:               testSourceVolumeName,
-			CapacityRange:      stdCapRange,
-			VolumeCapabilities: stdVolCaps,
-			Parameters:         tc.sourceReqParameters,
-		}
-
-		if tc.topology != nil {
-			// req.AccessibilityRequirements = tc.topology
-			sourceVolumeRequest.AccessibilityRequirements = tc.topology
+			Name:                      testSourceVolumeName,
+			CapacityRange:             tc.sourceCapacityRange,
+			VolumeCapabilities:        stdVolCaps,
+			Parameters:                tc.sourceReqParameters,
+			AccessibilityRequirements: tc.sourceTopology,
 		}
 
 		if tc.volumeOnCloud {
@@ -1148,7 +1248,6 @@ func TestCreateVolumeWithVolumeSourceFromVolume(t *testing.T) {
 
 		// Make sure the response has the source volume.
 		sourceVolume := resp.GetVolume()
-		t.Logf("response has source volume: %v ", sourceVolume)
 		if sourceVolume.ContentSource == nil || sourceVolume.ContentSource.Type == nil ||
 			sourceVolume.ContentSource.GetVolume() == nil || sourceVolume.ContentSource.GetVolume().VolumeId == "" {
 			t.Fatalf("Expected volume content source to have volume ID, got none")

pkg/gce-pd-csi-driver/utils.go

@@ -199,3 +199,13 @@ func collectMountOptions(fsType string, mntFlags []string) []string {
 	}
 	return options
 }
+
+func containsZone(zones []string, zone string) bool {
+	for _, z := range zones {
+		if z == zone {
+			return true
+		}
+	}
+
+	return false
+}

test/e2e/tests/multi_zone_e2e_test.go

@@ -101,7 +101,7 @@ var _ = Describe("GCE PD CSI Driver Multi-Zone", func() {
 					Segments: map[string]string{common.TopologyKeyZone: zones[1]},
 				},
 			},
-		})
+		}, nil)
 		Expect(err).To(BeNil(), "CreateVolume failed with error: %v", err)
 
 		// Validate Disk Created