From 416edd7734292daa14ff239b8f94e831dfe0a556 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Moritz=20R=C3=B6hrich?= Date: Mon, 30 Dec 2024 05:35:29 +0100 Subject: [PATCH] Fix: panic and crash when `size` parameter of a `disk` is not parseable into a `Quantity` (#111) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 * 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 --------- Signed-off-by: Moritz Röhrich --- Dockerfile.dapper | 1 + .../resource_virtualmachine_constructor.go | 13 +++- .../tests/resource_virtualmachine_test.go | 62 +++++++++++++++++++ 3 files changed, 73 insertions(+), 3 deletions(-) diff --git a/Dockerfile.dapper b/Dockerfile.dapper index f329839e..0812fe27 100644 --- a/Dockerfile.dapper +++ b/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 diff --git a/internal/provider/virtualmachine/resource_virtualmachine_constructor.go b/internal/provider/virtualmachine/resource_virtualmachine_constructor.go index 4f79a3c2..b0e6d343 100644 --- a/internal/provider/virtualmachine/resource_virtualmachine_constructor.go +++ b/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 diff --git a/internal/tests/resource_virtualmachine_test.go b/internal/tests/resource_virtualmachine_test.go index 6514b399..2e1e5f2c 100644 --- a/internal/tests/resource_virtualmachine_test.go +++ b/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]