Fix: panic and crash when size parameter of a disk is not parseab…

…le into a `Quantity` (#111)

* Fix crash if disk size is not given

Fix crash if the disk block in the virtualmachine resource does not
contain a `size` parameter and also doesn't contain either of the
`existing_volume_name` or the `container_image_name` parameters.

The Terraform provider relied on the `builder` package always behaving
nicely, but the `builder` package relied on all users checking inputs
before and only giving valid inputs. Therefore error handling in case a
non-parsable size has been given for a disk was not done, causing a
crash.
The size parameter is not mandatory in all cases and defaults to an
empty string, which is not parsable into a `resource.Quantity` object.

The fix here is to differentiate better when the `size` parameter is
taken into account at all. In case it is not taken into account, it is
completely ignored. If it is taken into account, a check is performed
that ensures it's parsable. If it isn't parsable, the Terraform provider
will return an error directly, indicating to the user where the problem
is.

related-to: harvester/harvester#7139

Signed-off-by: Moritz Röhrich <[email protected]>

* Tests: Parseable `size` Parameter

Add test to ensure that a non-parseable `size` parameter in a `disk`
subresource of a `virtualmachine` resource generates an appropriate
error message.
The `size` parameter is not mandatory in all configurations, but when it
is, it must be a parseable `resource.Quantity`, otherwise a panic and
crash would ensue.

Signed-off-by: Moritz Röhrich <[email protected]>

---------

Signed-off-by: Moritz Röhrich <[email protected]>

Dockerfile.dapper

@@ -20,6 +20,7 @@ RUN wget --quiet https://github.com/docker/buildx/releases/download/v0.13.1/buil
     mv buildx-v0.13.1.linux-${ARCH} /usr/local/bin/buildx && \
     mv terraform /usr/local/bin/terraform
 
+ENV DAPPER_RUN_ARGS="--network host -v /run/containerd/containerd.sock:/run/containerd/containerd.sock"
 ENV DAPPER_ENV REPO TAG DRONE_TAG
 ENV DAPPER_SOURCE /go/src/github.com/harvester/terraform-provider-harvester
 ENV DAPPER_OUTPUT ./bin ./dist

internal/provider/virtualmachine/resource_virtualmachine_constructor.go

@@ -8,6 +8,7 @@ import (
 	"github.com/harvester/harvester/pkg/builder"
 	harvesterutil "github.com/harvester/harvester/pkg/util"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/ptr"
 	kubevirtv1 "kubevirt.io/api/core/v1"
@@ -198,9 +199,7 @@ func (c *Constructor) Setup() util.Processors {
 						diskBus = builder.DiskBusVirtio
 					}
 				}
-				if diskSize == "" {
-					diskSize = util.If(existingVolumeName == "", "", builder.DefaultDiskSize).(string)
-				}
+
 				vmBuilder.Disk(diskName, diskBus, isCDRom, uint(bootOrder))
 				if existingVolumeName != "" {
 					vmBuilder.ExistingPVCVolume(diskName, existingVolumeName, hotPlug)
@@ -256,6 +255,14 @@ func (c *Constructor) Setup() util.Processors {
 							constants.AnnotationDiskAutoDelete: "true",
 						}
 					}
+
+					_, err := resource.ParseQuantity(diskSize)
+					if diskSize == "" {
+						diskSize = builder.DefaultDiskSize
+					} else if err != nil {
+						return fmt.Errorf("\"%v\" is not a parsable quantity: %v", diskSize, err)
+					}
+
 					vmBuilder.PVCVolume(diskName, diskSize, volumeName, hotPlug, pvcOption)
 				}
 				return nil

internal/tests/resource_virtualmachine_test.go

@@ -3,6 +3,7 @@ package tests
 import (
 	"context"
 	"fmt"
+	"regexp"
 	"testing"
 
 	"github.com/google/uuid"
@@ -137,6 +138,67 @@ func TestAccVirtualMachine_input(t *testing.T) {
 	})
 }
 
+func TestAccVirtualMachine_disk_size(t *testing.T) {
+	var (
+		testAccImageName         = "test-acc-image-leap-" + uuid.New().String()[:6]
+		testAccImageResourceName = constants.ResourceTypeImage + "." + testAccImageName
+		ctx                      = context.Background()
+	)
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckVirtualMachineDestroy(ctx),
+		Steps: []resource.TestStep{
+			{
+				Config: fmt.Sprintf(`
+resource harvester_image "%s" {
+  name = "leap-15.6"
+	namespace = "default"
+	display_name = "openSUSE-Leap-15.6"
+	source_type = "download"
+	url = "https://download.opensuse.org/repositories/Cloud:/Images:/Leap_15.6/images/openSUSE-Leap-15.6.x86_64-NoCloud.qcow2"
+}
+`,
+					testAccImageName,
+				),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr(testAccImageResourceName, constants.FieldCommonName, "leap-15.6"),
+					resource.TestCheckResourceAttr(testAccImageResourceName, constants.FieldCommonNamespace, "default"),
+				),
+			},
+			{
+				Config: `
+resource harvester_virtualmachine "disk_test" {
+	name = "disk-test"
+
+  cpu = 1
+  memory = "1Gi"
+
+  run_strategy = "RerunOnFailure"
+  machine_type = "q35"
+
+  network_interface {
+    name         = "default"
+  }
+
+  disk {
+    name       = "cdrom-disk"
+    type       = "cd-rom"
+    bus        = "sata"
+    boot_order = 1
+		size       = "foobar"
+    image      = "default/leap-15.6"
+  }
+}
+`,
+				ExpectError: regexp.MustCompile(".*is not a parsable quantity.*"),
+				Check:       resource.ComposeTestCheckFunc(),
+			},
+		},
+	})
+}
+
 func testAccVirtualMachineExists(ctx context.Context, n string, vm *kubevirtv1.VirtualMachine) resource.TestCheckFunc {
 	return func(s *terraform.State) error {
 		rs, ok := s.RootModule().Resources[n]