Merge pull request #817 from willkutler/OCM-11521

OCM-11521 | fix: validate custom machinepool disk size

go.mod

@@ -15,7 +15,7 @@ require (
 	github.com/hashicorp/terraform-plugin-sdk v1.17.2
 	github.com/onsi/ginkgo/v2 v2.17.1
 	github.com/onsi/gomega v1.30.0
-	github.com/openshift-online/ocm-common v0.0.11
+	github.com/openshift-online/ocm-common v0.0.12
 	github.com/openshift-online/ocm-sdk-go v0.1.443
 	github.com/pkg/errors v0.9.1
 	github.com/sirupsen/logrus v1.9.3

go.sum

@@ -500,6 +500,8 @@ github.com/onsi/gomega v1.30.0 h1:hvMK7xYz4D3HapigLTeGdId/NcfQx1VHMJc60ew99+8=
 github.com/onsi/gomega v1.30.0/go.mod h1:9sxs+SwGrKI0+PWe4Fxa9tFQQBG5xSsSbMXOI8PPpoQ=
 github.com/openshift-online/ocm-common v0.0.11 h1:DOj7fB59q0vAUFxSEQpLPp2AkReCCFq3r3NMaoZU20I=
 github.com/openshift-online/ocm-common v0.0.11/go.mod h1:6MWje2NFNJ3IWpGs7BYj6DWagWXHyp8EnmYY7XFTtI4=
+github.com/openshift-online/ocm-common v0.0.12 h1:2oGZCXd8O/nZVlM0pvTtm3hDGbW/ncTtvSLLB+nuQf0=
+github.com/openshift-online/ocm-common v0.0.12/go.mod h1:6MWje2NFNJ3IWpGs7BYj6DWagWXHyp8EnmYY7XFTtI4=
 github.com/openshift-online/ocm-sdk-go v0.1.443 h1:wb79sOFAzA2f4hvJMOz2YJ6Q0HTIXY3kbDJoy4/xnBg=
 github.com/openshift-online/ocm-sdk-go v0.1.443/go.mod h1:CiAu2jwl3ITKOxkeV0Qnhzv4gs35AmpIzVABQLtcI2Y=
 github.com/pierrec/lz4 v2.0.5+incompatible/go.mod h1:pdkljMzZIN41W+lC3N2tnIh5sFi+IEE17M5jbnwPHcY=

internal/ocm/resource/cluster.go

@@ -6,6 +6,7 @@ import (
 	"regexp"
 
 	"github.com/openshift-online/ocm-common/pkg/cluster/validations"
+	diskValidator "github.com/openshift-online/ocm-common/pkg/machinepool/validations"
 	kmsArnRegexpValidator "github.com/openshift-online/ocm-common/pkg/resource/validations"
 	cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"
 	rosaTypes "github.com/terraform-redhat/terraform-provider-rhcs/provider/clusterrosa/common/types"
@@ -35,7 +36,7 @@ func (c *Cluster) Build() (object *cmv1.Cluster, err error) {
 
 func (c *Cluster) CreateNodes(clusterTopology rosaTypes.ClusterTopology, autoScalingEnabled bool, replicas *int64, minReplicas *int64,
 	maxReplicas *int64, computeMachineType *string, labels map[string]string,
-	availabilityZones []string, multiAZ bool, workerDiskSize *int64) error {
+	availabilityZones []string, multiAZ bool, workerDiskSize *int64, version *string) error {
 	nodes := cmv1.NewClusterNodes()
 	if computeMachineType != nil {
 		nodes.ComputeMachineType(
@@ -44,6 +45,20 @@ func (c *Cluster) CreateNodes(clusterTopology rosaTypes.ClusterTopology, autoSca
 	}
 
 	if workerDiskSize != nil {
+		if clusterTopology == rosaTypes.Hcp {
+			err := diskValidator.ValidateNodePoolRootDiskSize(int(*workerDiskSize))
+			if err != nil {
+				return err
+			}
+		} else {
+			if version == nil {
+				return errors.New("Version must be set if a custom root disk size is configured")
+			}
+			err := diskValidator.ValidateMachinePoolRootDiskSize(*version, int(*workerDiskSize))
+			if err != nil {
+				return err
+			}
+		}
 		nodes.ComputeRootVolume(
 			cmv1.NewRootVolume().AWS(
 				cmv1.NewAWSVolume().Size(int(*workerDiskSize)),

internal/ocm/resource/cluster_test.go

@@ -17,22 +17,22 @@ var _ = Describe("Cluster", func() {
 	})
 	Context("CreateNodes validation", func() {
 		It("Autoscaling disabled minReplicas set - failure", func() {
-			err := cluster.CreateNodes(rosaTypes.Classic, false, nil, pointer(int64(2)), nil, nil, nil, nil, false, nil)
+			err := cluster.CreateNodes(rosaTypes.Classic, false, nil, pointer(int64(2)), nil, nil, nil, nil, false, nil, nil)
 			Expect(err).To(HaveOccurred())
 			Expect(err.Error()).To(Equal("Autoscaling must be enabled in order to set min and max replicas"))
 		})
 		It("Autoscaling disabled maxReplicas set - failure", func() {
-			err := cluster.CreateNodes(rosaTypes.Classic, false, nil, nil, pointer(int64(2)), nil, nil, nil, false, nil)
+			err := cluster.CreateNodes(rosaTypes.Classic, false, nil, nil, pointer(int64(2)), nil, nil, nil, false, nil, nil)
 			Expect(err).To(HaveOccurred())
 			Expect(err.Error()).To(Equal("Autoscaling must be enabled in order to set min and max replicas"))
 		})
 		It("Autoscaling disabled replicas smaller than 2 - failure", func() {
-			err := cluster.CreateNodes(rosaTypes.Classic, false, pointer(int64(1)), nil, nil, nil, nil, nil, false, nil)
+			err := cluster.CreateNodes(rosaTypes.Classic, false, pointer(int64(1)), nil, nil, nil, nil, nil, false, nil, nil)
 			Expect(err).To(HaveOccurred())
 			Expect(err.Error()).To(Equal("Cluster requires at least 2 compute nodes"))
 		})
 		It("Autoscaling disabled default replicas - success", func() {
-			err := cluster.CreateNodes(rosaTypes.Classic, false, nil, nil, nil, nil, nil, nil, false, nil)
+			err := cluster.CreateNodes(rosaTypes.Classic, false, nil, nil, nil, nil, nil, nil, false, nil, nil)
 			Expect(err).NotTo(HaveOccurred())
 			ocmCluster, err := cluster.Build()
 			Expect(err).NotTo(HaveOccurred())
@@ -46,7 +46,7 @@ var _ = Describe("Cluster", func() {
 			Expect(autoscaleCompute).To(BeNil())
 		})
 		It("Autoscaling disabled 3 replicas - success", func() {
-			err := cluster.CreateNodes(rosaTypes.Classic, false, pointer(int64(3)), nil, nil, nil, nil, nil, false, nil)
+			err := cluster.CreateNodes(rosaTypes.Classic, false, pointer(int64(3)), nil, nil, nil, nil, nil, false, nil, nil)
 			Expect(err).NotTo(HaveOccurred())
 			ocmCluster, err := cluster.Build()
 			Expect(err).NotTo(HaveOccurred())
@@ -60,12 +60,12 @@ var _ = Describe("Cluster", func() {
 			Expect(autoscaleCompute).To(BeNil())
 		})
 		It("Autoscaling enabled replicas set - failure", func() {
-			err := cluster.CreateNodes(rosaTypes.Classic, true, pointer(int64(2)), nil, nil, nil, nil, nil, false, nil)
+			err := cluster.CreateNodes(rosaTypes.Classic, true, pointer(int64(2)), nil, nil, nil, nil, nil, false, nil, nil)
 			Expect(err).To(HaveOccurred())
 			Expect(err.Error()).To(Equal("When autoscaling is enabled, replicas should not be configured"))
 		})
 		It("Autoscaling enabled default minReplicas & maxReplicas - success", func() {
-			err := cluster.CreateNodes(rosaTypes.Classic, true, nil, nil, nil, nil, nil, nil, false, nil)
+			err := cluster.CreateNodes(rosaTypes.Classic, true, nil, nil, nil, nil, nil, nil, false, nil, nil)
 			Expect(err).NotTo(HaveOccurred())
 			ocmCluster, err := cluster.Build()
 			Expect(err).NotTo(HaveOccurred())
@@ -81,12 +81,12 @@ var _ = Describe("Cluster", func() {
 			Expect(autoscaleCompute.MaxReplicas()).To(Equal(2))
 		})
 		It("Autoscaling enabled default maxReplicas smaller than minReplicas - failure", func() {
-			err := cluster.CreateNodes(rosaTypes.Classic, true, nil, pointer(int64(4)), pointer(int64(3)), nil, nil, nil, false, nil)
+			err := cluster.CreateNodes(rosaTypes.Classic, true, nil, pointer(int64(4)), pointer(int64(3)), nil, nil, nil, false, nil, nil)
 			Expect(err).To(HaveOccurred())
 			Expect(err.Error()).To(Equal("max-replicas must be greater or equal to min-replicas"))
 		})
 		It("Autoscaling enabled set minReplicas & maxReplicas - success", func() {
-			err := cluster.CreateNodes(rosaTypes.Classic, true, nil, pointer(int64(2)), pointer(int64(4)), nil, nil, nil, false, nil)
+			err := cluster.CreateNodes(rosaTypes.Classic, true, nil, pointer(int64(2)), pointer(int64(4)), nil, nil, nil, false, nil, nil)
 			Expect(err).NotTo(HaveOccurred())
 			ocmCluster, err := cluster.Build()
 			Expect(err).NotTo(HaveOccurred())
@@ -102,7 +102,7 @@ var _ = Describe("Cluster", func() {
 			Expect(autoscaleCompute.MaxReplicas()).To(Equal(4))
 		})
 		It("Autoscaling disabled set ComputeMachineType - success", func() {
-			err := cluster.CreateNodes(rosaTypes.Classic, false, nil, nil, nil, pointer("asdf"), nil, nil, false, nil)
+			err := cluster.CreateNodes(rosaTypes.Classic, false, nil, nil, nil, pointer("asdf"), nil, nil, false, nil, nil)
 			Expect(err).NotTo(HaveOccurred())
 			ocmCluster, err := cluster.Build()
 			Expect(err).NotTo(HaveOccurred())
@@ -118,7 +118,7 @@ var _ = Describe("Cluster", func() {
 			Expect(autoscaleCompute).To(BeNil())
 		})
 		It("Autoscaling disabled set compute labels - success", func() {
-			err := cluster.CreateNodes(rosaTypes.Classic, false, nil, nil, nil, nil, map[string]string{"key1": "val1"}, nil, false, nil)
+			err := cluster.CreateNodes(rosaTypes.Classic, false, nil, nil, nil, nil, map[string]string{"key1": "val1"}, nil, false, nil, nil)
 			Expect(err).NotTo(HaveOccurred())
 			ocmCluster, err := cluster.Build()
 			Expect(err).NotTo(HaveOccurred())
@@ -134,7 +134,7 @@ var _ = Describe("Cluster", func() {
 			Expect(autoscaleCompute).To(BeNil())
 		})
 		It("Autoscaling disabled multiAZ false set one availability zone - success", func() {
-			err := cluster.CreateNodes(rosaTypes.Classic, false, nil, nil, nil, nil, nil, []string{"us-east-1a"}, false, nil)
+			err := cluster.CreateNodes(rosaTypes.Classic, false, nil, nil, nil, nil, nil, []string{"us-east-1a"}, false, nil, nil)
 			Expect(err).NotTo(HaveOccurred())
 			ocmCluster, err := cluster.Build()
 			Expect(err).NotTo(HaveOccurred())
@@ -149,17 +149,17 @@ var _ = Describe("Cluster", func() {
 			Expect(autoscaleCompute).To(BeNil())
 		})
 		It("Autoscaling disabled multiAZ false set three availability zones - failure", func() {
-			err := cluster.CreateNodes(rosaTypes.Classic, false, nil, nil, nil, nil, nil, []string{"us-east-1a", "us-east-1b", "us-east-1c"}, false, nil)
+			err := cluster.CreateNodes(rosaTypes.Classic, false, nil, nil, nil, nil, nil, []string{"us-east-1a", "us-east-1b", "us-east-1c"}, false, nil, nil)
 			Expect(err).To(HaveOccurred())
 			Expect(err.Error()).To(Equal("The number of availability zones for a single AZ cluster should be 1, instead received: 3"))
 		})
 		It("Autoscaling disabled multiAZ true set three availability zones and two replicas - failure", func() {
-			err := cluster.CreateNodes(rosaTypes.Classic, false, pointer(int64(2)), nil, nil, nil, nil, []string{"us-east-1a", "us-east-1b", "us-east-1c"}, true, nil)
+			err := cluster.CreateNodes(rosaTypes.Classic, false, pointer(int64(2)), nil, nil, nil, nil, []string{"us-east-1a", "us-east-1b", "us-east-1c"}, true, nil, nil)
 			Expect(err).To(HaveOccurred())
 			Expect(err.Error()).To(Equal("Multi AZ cluster requires at least 3 compute nodes"))
 		})
 		It("Autoscaling disabled multiAZ true set three availability zones and three replicas - success", func() {
-			err := cluster.CreateNodes(rosaTypes.Classic, false, pointer(int64(3)), nil, nil, nil, nil, []string{"us-east-1a", "us-east-1b", "us-east-1c"}, true, nil)
+			err := cluster.CreateNodes(rosaTypes.Classic, false, pointer(int64(3)), nil, nil, nil, nil, []string{"us-east-1a", "us-east-1b", "us-east-1c"}, true, nil, nil)
 			Expect(err).NotTo(HaveOccurred())
 			ocmCluster, err := cluster.Build()
 			Expect(err).NotTo(HaveOccurred())
@@ -174,13 +174,14 @@ var _ = Describe("Cluster", func() {
 			Expect(autoscaleCompute).To(BeNil())
 		})
 		It("Autoscaling disabled multiAZ true set one zone - failure", func() {
-			err := cluster.CreateNodes(rosaTypes.Classic, false, nil, nil, nil, nil, nil, []string{"us-east-1a", "us-east-1b", "us-east-1c"}, true, nil)
+			err := cluster.CreateNodes(rosaTypes.Classic, false, nil, nil, nil, nil, nil, []string{"us-east-1a", "us-east-1b", "us-east-1c"}, true, nil, nil)
 			Expect(err).To(HaveOccurred())
 			Expect(err.Error()).To(Equal("Multi AZ cluster requires at least 3 compute nodes"))
 		})
 		It("Custom disk size", func() {
 			diskSize := int64(543)
-			err := cluster.CreateNodes(rosaTypes.Classic, false, pointer(int64(3)), nil, nil, nil, nil, nil, false, &diskSize)
+			version := "4.14.0"
+			err := cluster.CreateNodes(rosaTypes.Classic, false, pointer(int64(3)), nil, nil, nil, nil, nil, false, &diskSize, &version)
 			Expect(err).NotTo(HaveOccurred())
 			ocmCluster, err := cluster.Build()
 			Expect(err).NotTo(HaveOccurred())
@@ -192,6 +193,13 @@ var _ = Describe("Cluster", func() {
 			Expect(awsVolume).NotTo(BeNil())
 			Expect(awsVolume.Size()).To(Equal(int(diskSize)))
 		})
+		It("Custom disk size with version before 4.14", func() {
+			diskSize := int64(2000)
+			version := "4.10.0"
+			err := cluster.CreateNodes(rosaTypes.Classic, false, pointer(int64(3)), nil, nil, nil, nil, nil, false, &diskSize, &version)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("Invalid root disk size"))
+		})
 	})
 	Context("CreateAWSBuilder validation", func() {
 		It("PrivateLink true subnets IDs empty - failure", func() {

provider/clusterrosa/classic/cluster_rosa_classic_resource.go

@@ -594,9 +594,11 @@ func createClassicClusterObject(ctx context.Context,
 		return nil, err
 	}
 	workerDiskSize := common.OptionalInt64(state.WorkerDiskSize)
+	openshiftVersion := common.OptionalString(state.Version)
 
 	if err = ocmClusterResource.CreateNodes(rosaTypes.Classic, autoScalingEnabled, replicas, minReplicas, maxReplicas,
-		computeMachineType, labels, availabilityZones, multiAZ, workerDiskSize); err != nil {
+		computeMachineType, labels, availabilityZones, multiAZ, workerDiskSize,
+		openshiftVersion); err != nil {
 		return nil, err
 	}
 

provider/clusterrosa/hcp/resource.go

@@ -504,7 +504,7 @@ func createHcpClusterObject(ctx context.Context,
 	workerDiskSize := common.OptionalInt64(state.WorkerDiskSize)
 
 	if err := ocmClusterResource.CreateNodes(rosaTypes.Hcp, false, replicas, nil, nil,
-		computeMachineType, nil, availabilityZones, true, workerDiskSize); err != nil {
+		computeMachineType, nil, availabilityZones, true, workerDiskSize, nil); err != nil {
 		return nil, err
 	}
 

provider/machinepool/classic/machine_pool_resource.go

@@ -42,6 +42,7 @@ import (
 	"github.com/hashicorp/terraform-plugin-framework/schema/validator"
 	"github.com/hashicorp/terraform-plugin-framework/types"
 	"github.com/hashicorp/terraform-plugin-log/tflog"
+	diskValidator "github.com/openshift-online/ocm-common/pkg/machinepool/validations"
 	sdk "github.com/openshift-online/ocm-sdk-go"
 	cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"
 	"github.com/terraform-redhat/terraform-provider-rhcs/provider/common"
@@ -303,6 +304,31 @@ func (r *MachinePoolResource) Create(ctx context.Context, req resource.CreateReq
 	builder.ID(plan.Name.ValueString())
 
 	if workerDiskSize := common.OptionalInt64(plan.DiskSize); workerDiskSize != nil {
+		// Get cluster version to pass to disk size validator
+		getCluster, err := resource.Get().SendContext(ctx)
+		if err != nil {
+			diags.AddError(
+				"Can't find cluster",
+				fmt.Sprintf(
+					"Can't find cluster with identifier '%s': %v",
+					plan.Cluster.ValueString(), err,
+				),
+			)
+			return
+		}
+
+		err = diskValidator.ValidateMachinePoolRootDiskSize(
+			getCluster.Body().Version().RawID(),
+			int(*workerDiskSize),
+		)
+		if err != nil {
+			resp.Diagnostics.AddError(
+				"Cannot build machine pool",
+				err.Error(),
+			)
+			return
+		}
+
 		builder.RootVolume(
 			cmv1.NewRootVolume().AWS(
 				cmv1.NewAWSVolume().Size(int(*workerDiskSize)),

provider/machinepool/hcp/machine_pool_resource.go

@@ -41,6 +41,7 @@ import (
 	"github.com/hashicorp/terraform-plugin-framework/schema/validator"
 	"github.com/hashicorp/terraform-plugin-framework/types"
 	"github.com/hashicorp/terraform-plugin-log/tflog"
+	diskValidator "github.com/openshift-online/ocm-common/pkg/machinepool/validations"
 	ocmUtils "github.com/openshift-online/ocm-common/pkg/ocm/utils"
 	sdk "github.com/openshift-online/ocm-sdk-go"
 	cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"
@@ -322,6 +323,14 @@ func (r *HcpMachinePoolResource) Create(ctx context.Context, req resource.Create
 		awsNodePoolBuilder.Ec2MetadataHttpTokens(cmv1.Ec2MetadataHttpTokens(plan.AWSNodePool.Ec2MetadataHttpTokens.ValueString()))
 
 		if workerDiskSize := common.OptionalInt64(plan.AWSNodePool.DiskSize); workerDiskSize != nil {
+			err := diskValidator.ValidateNodePoolRootDiskSize(int(*workerDiskSize))
+			if err != nil {
+				resp.Diagnostics.AddError(
+					"Cannot build machine pool",
+					err.Error(),
+				)
+				return
+			}
 			awsNodePoolBuilder.RootVolume(cmv1.NewAWSVolume().Size(int(*workerDiskSize)))
 		}
 

subsystem/classic/cluster_resource_rosa_create_test.go

@@ -1335,6 +1335,7 @@ var _ = Describe("rhcs_cluster_rosa_classic - create", func() {
 					}
 				}
 				worker_disk_size = 400
+				version = "4.14.0"
 			  }
 			`)
 			runOutput := Terraform.Apply()
@@ -1344,6 +1345,39 @@ var _ = Describe("rhcs_cluster_rosa_classic - create", func() {
 			Expect(resource).To(MatchJQ(".attributes.worker_disk_size", 400.0))
 		})
 
+		It("Fails to create cluster with invalid custom disk size", func() {
+			// Prepare the server:
+			TestServer.AppendHandlers(
+				CombineHandlers(
+					VerifyRequest(http.MethodGet, "/api/clusters_mgmt/v1/versions"),
+					RespondWithJSON(http.StatusOK, versionListPage1),
+				),
+			)
+
+			// Run the apply command:
+			Terraform.Source(`
+			  resource "rhcs_cluster_rosa_classic" "my_cluster" {
+				name           = "my-cluster"
+				cloud_region   = "us-west-1"
+				aws_account_id = "123456789012"
+				sts = {
+					operator_role_prefix = "test"
+					role_arn = "",
+					support_role_arn = "",
+					instance_iam_roles = {
+						master_role_arn = "",
+						worker_role_arn = "",
+					}
+				}
+				worker_disk_size = 20
+				version = "4.14.0"
+			  }
+			`)
+			runOutput := Terraform.Apply()
+			Expect(runOutput.ExitCode).ToNot(BeZero())
+			runOutput.VerifyErrorContainsSubstring("Invalid root disk size")
+		})
+
 		It("Creates basic cluster with properties", func() {
 			prop_key := "my_prop_key"
 			prop_val := "my_prop_val"

subsystem/classic/machine_pool_resource_test.go

@@ -1635,6 +1635,23 @@ var _ = Describe("Classic Machine Pool", func() {
 		It("Can create machine pool with custom disk size", func() {
 			// Prepare the server:
 			TestServer.AppendHandlers(
+				CombineHandlers(
+					VerifyRequest(http.MethodGet, "/api/clusters_mgmt/v1/clusters/123"),
+					RespondWithJSON(http.StatusOK, `{
+					  "id": "123",
+					  "name": "my-cluster",
+					  "multi_az": false,
+					  "nodes": {
+						"availability_zones": [
+						  "us-east-1a"
+						]
+					  },
+					  "version": {
+						"raw_id": "4.14.0"
+					  },
+					  "state": "ready"
+					}`),
+				),
 				CombineHandlers(
 					VerifyRequest(
 						http.MethodPost,