OCPBUGS-41184: GCP Validate Disk and Instance Type

** Update the error text and the way that disks and instances are validated. Before, the
error message was backwards on indicating if the instance type or the disk type was the problem.
Now, the disk type is validated against the instance type (rather than the opposite).

pkg/asset/installconfig/gcp/validation.go

@@ -73,20 +73,29 @@ func Validate(client API, ic *types.InstallConfig) error {
 	return allErrs.ToAggregate()
 }
 
-func validateInstanceType(fldPath *field.Path, instanceType string, validInstanceTypes []string, arch string) *field.Error {
-	if len(validInstanceTypes) == 0 {
-		return field.InternalError(fldPath, fmt.Errorf("no valid instance types found"))
-	}
-
+func validateInstanceAndDiskType(fldPath *field.Path, diskType, instanceType, arch string) *field.Error {
+	diskTypes := []string{}
+	ok := false
 	if instanceType != "" {
 		family, _, _ := strings.Cut(instanceType, "-")
-		acceptedInstanceTypes := sets.New(validInstanceTypes...)
-		if !acceptedInstanceTypes.Has(family) {
-			return field.NotSupported(fldPath.Child("type"), family, sets.List(acceptedInstanceTypes))
+		diskTypes, ok = gcp.DiskTypeToInstanceTypeMap[family]
+		if !ok {
+			return field.NotFound(fldPath.Child("type"), family)
+		}
+
+		acceptedArmFamilies := sets.New("t2a")
+		if arch == types.ArchitectureARM64 && !acceptedArmFamilies.Has(family) {
+			return field.NotSupported(fldPath.Child("type"), family, sets.List(acceptedArmFamilies))
 		}
+	}
 
-		if arch == types.ArchitectureARM64 && family != "t2a" {
-			return field.InternalError(fldPath, fmt.Errorf("instance type %s requires %s architecture", instanceType, types.ArchitectureARM64))
+	if len(diskTypes) > 0 && diskType != "" {
+		if !sets.New(diskTypes...).Has(diskType) {
+			return field.Invalid(
+				fldPath.Child("diskType"),
+				diskType,
+				fmt.Sprintf("%s instance requires one of the following disk types: %v", instanceType, diskTypes),
+			)
 		}
 	}
 	return nil
@@ -104,16 +113,8 @@ func ValidateInstanceType(client API, fieldPath *field.Path, project, region str
 		return append(allErrs, field.InternalError(nil, err))
 	}
 
-	instanceTypes := []string{}
-	ok := false
-	if diskType != "" {
-		instanceTypes, ok = gcp.DiskTypeToInstanceTypeMap[diskType]
-		if !ok {
-			return append(allErrs, field.NotFound(fieldPath.Child("diskType"), diskType))
-		}
-	}
-	if err := validateInstanceType(fieldPath, instanceType, instanceTypes, arch); err != nil {
-		allErrs = append(allErrs, err)
+	if fieldErr := validateInstanceAndDiskType(fieldPath, diskType, instanceType, arch); fieldErr != nil {
+		return append(allErrs, fieldErr)
 	}
 
 	userZones := sets.New(zones...)
@@ -226,18 +227,28 @@ func validateInstanceTypes(client API, ic *types.InstallConfig) field.ErrorList
 			}
 		}
 	}
-	allErrs = append(allErrs,
-		ValidateInstanceType(
-			client,
-			field.NewPath("controlPlane", "platform", "gcp"),
-			ic.GCP.ProjectID,
-			ic.GCP.Region,
-			zones,
-			cpDiskType,
-			instanceType,
-			controlPlaneReq,
-			arch,
-		)...)
+
+	// The IOPS minimum Control plane requirements are not met for pd-standard machines.
+	if cpDiskType == "pd-standard" {
+		allErrs = append(allErrs,
+			field.NotSupported(field.NewPath("controlPlane").Child("type"),
+				cpDiskType,
+				sets.List(gcp.ControlPlaneSupportedDisks)),
+		)
+	} else {
+		allErrs = append(allErrs,
+			ValidateInstanceType(
+				client,
+				field.NewPath("controlPlane", "platform", "gcp"),
+				ic.GCP.ProjectID,
+				ic.GCP.Region,
+				zones,
+				cpDiskType,
+				instanceType,
+				controlPlaneReq,
+				arch,
+			)...)
+	}
 
 	for idx, compute := range ic.Compute {
 		fieldPath := field.NewPath("compute").Index(idx)

pkg/asset/installconfig/gcp/validation_test.go

@@ -93,6 +93,10 @@ var (
 		ic.Platform.GCP.DefaultMachinePlatform.InstanceType = "n1-dne-1"
 	}
 
+	invalidateControlPlaneDiskTypes = func(ic *types.InstallConfig) {
+		ic.ControlPlane.Platform.GCP.DiskType = "pd-standard"
+	}
+
 	invalidateNetwork        = func(ic *types.InstallConfig) { ic.GCP.Network = "invalid-vpc" }
 	invalidateComputeSubnet  = func(ic *types.InstallConfig) { ic.GCP.ComputeSubnet = "invalid-compute-subnet" }
 	invalidateCPSubnet       = func(ic *types.InstallConfig) { ic.GCP.ControlPlaneSubnet = "invalid-cp-subnet" }
@@ -234,6 +238,12 @@ func TestGCPInstallConfigValidation(t *testing.T) {
 			expectedError:  true,
 			expectedErrMsg: `\[platform.gcp.defaultMachinePlatform.type: Invalid value: "n1-standard-1": instance type does not meet minimum resource requirements of 4 vCPUs, platform.gcp.defaultMachinePlatform.type: Invalid value: "n1-standard-1": instance type does not meet minimum resource requirements of 15360 MB Memory, controlPlane.platform.gcp.type: Invalid value: "n1-standard-1": instance type does not meet minimum resource requirements of 4 vCPUs, controlPlane.platform.gcp.type: Invalid value: "n1-standard-1": instance type does not meet minimum resource requirements of 15360 MB Memory, compute\[0\].platform.gcp.type: Invalid value: "n1-standard-1": instance type does not meet minimum resource requirements of 2 vCPUs, compute\[0\].platform.gcp.type: Invalid value: "n1-standard-1": instance type does not meet minimum resource requirements of 7680 MB Memory\]`,
 		},
+		{
+			name:           "Invalid control plane machine disk types",
+			edits:          editFunctions{validMachineTypes, invalidateControlPlaneDiskTypes},
+			expectedError:  true,
+			expectedErrMsg: `controlPlane.type: Unsupported value: "pd-standard": supported values: "hyperdisk-balanced", "pd-balanced", "pd-ssd"`,
+		},
 		{
 			name:           "Invalid control plane machine types",
 			edits:          editFunctions{invalidateControlPlaneMachineTypes},
@@ -851,7 +861,7 @@ func TestValidateInstanceType(t *testing.T) {
 			instanceType:   "n2-standard-4",
 			diskType:       "hyperdisk-balanced",
 			expectedError:  true,
-			expectedErrMsg: `[instance.diskType: Unsupported value: "n2": supported values: "c3", "c3d", "c4", m1", "n4"]`,
+			expectedErrMsg: `^\[instance.diskType: Invalid value: "hyperdisk\-balanced": n2\-standard\-4 instance requires one of the following disk types: \[pd\-standard pd\-ssd pd\-balanced\]\]$`,
 		},
 	}
 

pkg/types/gcp/machinepools.go

@@ -27,13 +27,23 @@ var (
 	// ComputeSupportedDisks contains the supported disk types for control plane nodes.
 	ComputeSupportedDisks = sets.New(HyperDiskBalanced, PDBalanced, PDSSD, PDStandard)
 
-	// DiskTypeToInstanceTypeMap contains a map where the key is the Disk Type, and the values are a list of
-	// instance types that are supported by the installer and correlate to the Disk Type.
+	// DiskTypeToInstanceTypeMap contains a map where the key is the Instance Type, and the
+	// values are a list of disk types that are supported by the installer and correlate to the Instance Type.
 	DiskTypeToInstanceTypeMap = map[string][]string{
-		PDStandard:        {"a2", "e2", "n1", "n2", "n2d", "t2a", "t2d"},
-		PDSSD:             {"a2", "a3", "c3", "c3d", "e2", "m1", "n1", "n2", "n2d", "t2a", "t2d"},
-		PDBalanced:        {"a2", "a3", "c3", "c3d", "e2", "m1", "n1", "n2", "n2d", "t2a", "t2d"},
-		HyperDiskBalanced: {"c3", "c3d", "m1", "n4"},
+		"a2":  {PDStandard, PDSSD, PDBalanced},
+		"a3":  {PDSSD, PDBalanced},
+		"c2":  {PDStandard, PDSSD, PDBalanced},
+		"c2d": {PDStandard, PDSSD, PDBalanced},
+		"c3":  {PDSSD, PDBalanced, HyperDiskBalanced},
+		"c3d": {PDSSD, PDBalanced, HyperDiskBalanced},
+		"e2":  {PDStandard, PDSSD, PDBalanced},
+		"m1":  {PDSSD, PDBalanced, HyperDiskBalanced},
+		"n1":  {PDStandard, PDSSD, PDBalanced},
+		"n2":  {PDStandard, PDSSD, PDBalanced},
+		"n2d": {PDStandard, PDSSD, PDBalanced},
+		"n4":  {HyperDiskBalanced},
+		"t2a": {PDStandard, PDSSD, PDBalanced},
+		"t2d": {PDStandard, PDSSD, PDBalanced},
 	}
 )