From abe1e5d8a479a10499a86d338a8dd9e7feed580c Mon Sep 17 00:00:00 2001 From: Adam Barreiro Date: Tue, 10 Sep 2024 14:07:50 +0200 Subject: [PATCH] Fix issue #1262 (#1312) Signed-off-by: abarreiro --- .changes/v3.14.0/1312-bug-fixes.md | 2 + vcd/resource_vcd_vapp_vm_test.go | 134 +++++++++++++++++++++++++++++ vcd/resource_vcd_vapp_vm_tools.go | 5 +- 3 files changed, 140 insertions(+), 1 deletion(-) create mode 100644 .changes/v3.14.0/1312-bug-fixes.md diff --git a/.changes/v3.14.0/1312-bug-fixes.md b/.changes/v3.14.0/1312-bug-fixes.md new file mode 100644 index 000000000..14eea7b27 --- /dev/null +++ b/.changes/v3.14.0/1312-bug-fixes.md @@ -0,0 +1,2 @@ +* Fix [Issue 1262](https://github.com/vmware/terraform-provider-vcd/issues/1262): Panic when creating a VM with `vcd_vm` and `vcd_vapp_vm` + when the VCD provider is configured with a user without "Organization vDC Disk: View/Edit IOPS" rights [GH-1312] diff --git a/vcd/resource_vcd_vapp_vm_test.go b/vcd/resource_vcd_vapp_vm_test.go index 4e13376d6..30dac304b 100644 --- a/vcd/resource_vcd_vapp_vm_test.go +++ b/vcd/resource_vcd_vapp_vm_test.go @@ -4,6 +4,8 @@ package vcd import ( "fmt" + "github.com/vmware/go-vcloud-director/v2/types/v56" + "net/url" "testing" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" @@ -466,6 +468,138 @@ resource "vcd_vm" "boot-image-vm" { } ` +// TestAccVcdVm_WithoutOrganizationVdcDiskIopsRights tests VM creation with a user that does not have any "Organization vDC Disk: View/edit" IOPS rights. +// This test will panic without fix made in https://github.com/vmware/terraform-provider-vcd/pull/1312 +func TestAccVcdVm_WithoutOrganizationVdcDiskIopsRights(t *testing.T) { + preTestChecks(t) + skipIfNotSysAdmin(t) + + var params = StringMap{ + "ProviderVcdSystem": providerVcdSystem, + "ProviderVcdOrg": providerVcdOrg1, + "Org": testConfig.VCD.Org, + "Name": t.Name(), + "Vdc": testConfig.Nsxt.Vdc, + "Catalog": testConfig.VCD.Catalog.NsxtBackedCatalogName, + "CatalogItem": testConfig.VCD.Catalog.NsxtCatalogItem, + "Tags": "vapp vm", + } + testParamsNotEmpty(t, params) + + configText := templateFill(testAccCheckVcdVmWithoutIopsRights, params) + if vcdShortTest { + t.Skip(acceptanceTestsSkipped) + return + } + + vcdClient := createTemporaryVCDConnection(false) + // Save the rights that are manipulated to restore them when the test is finished + var modifiedRights []types.OpenApiReference + + // Restore the role with the rights that were removed, if needed, at the end of the test. + // The "modifiedRights" is set during test execution PreConfig phase + defer func() { + if len(modifiedRights) > 0 { + role, err := vcdClient.Client.GetGlobalRoleByName("Organization Administrator") + if err != nil { + t.Fatalf("could not restore rights after %s test finished, %s", t.Name(), err) + } + err = role.AddRights(modifiedRights) + if err != nil { + t.Fatalf("could not restore rights after %s test finished, %s", t.Name(), err) + } + } + }() + + debugPrintf("#[DEBUG] CONFIGURATION: %s\n", configText) + resource.Test(t, resource.TestCase{ + ProviderFactories: buildMultipleProviders(), + CheckDestroy: testAccCheckVcdStandaloneVmDestroy(t.Name(), params["Org"].(string), params["Vdc"].(string)), + Steps: []resource.TestStep{ + { + Config: configText, + PreConfig: func() { + role, err := vcdClient.Client.GetGlobalRoleByName("Organization Administrator") + if err != nil { + if govcd.ContainsNotFound(err) { + t.Skipf("could not find required global role Organization Administrator") + } + t.Fatal(err) + } + // Use a filter, so we only get the "Organization vDC Disk: ..." rights and skip the non-relevant ones + filter := make(url.Values) + filter.Set("filter", "name==Organization vDC Disk*") + existingRights, err := role.GetRights(filter) + if err != nil { + t.Fatal(err) + } + // If the rights are not present in that role, there's nothing else to do + if len(existingRights) == 0 { + return + } + // Convert the obtained rights to OpenApiReference to be able to remove them. + // We save them in the outer-scoped "modifiedRights" so they can be re-added later + for _, right := range existingRights { + modifiedRights = append(modifiedRights, types.OpenApiReference{ + Name: right.Name, + ID: right.ID, + }) + } + err = role.RemoveRights(modifiedRights) + if err != nil { + t.Fatal(err) + } + }, + Check: resource.ComposeTestCheckFunc( + // Nothing to test here, we just want to assert that no panic is thrown anymore + resource.TestCheckResourceAttr("vcd_vm.my_vm", "name", t.Name()), + ), + }, + }, + }) + postTestChecks(t) +} + +const testAccCheckVcdVmWithoutIopsRights = ` +# Acceptance tests only: +# This data source initializes the 'testAccProvider' variable and prevents a panic +# in 'testAccCheckVcdStandaloneVmDestroy' when running acceptance tests +# (check TESTING.md "Tests with multiple providers" for more info) +data "vcd_version" "dummy" { + provider = {{.ProviderVcdSystem}} +} + +data "vcd_catalog" "my-catalog" { + provider = {{.ProviderVcdOrg}} + org = "{{.Org}}" + name = "{{.Catalog}}" +} + +data "vcd_catalog_vapp_template" "template" { + provider = {{.ProviderVcdOrg}} + catalog_id = data.vcd_catalog.my-catalog.id + name = "{{.CatalogItem}}" +} + +resource "vcd_vm" "my_vm" { + provider = {{.ProviderVcdOrg}} + org = "{{.Org}}" + vdc = "{{.Vdc}}" + name = "{{.Name}}" + vapp_template_id = data.vcd_catalog_vapp_template.template.id + memory = 512 + cpus = 1 + + override_template_disk { + bus_type = "paravirtual" + size_in_mb = "16384" + bus_number = 0 + unit_number = 0 + storage_profile = "*" + } +} +` + // TestAccVcdVAppVmMetadata tests metadata CRUD on vApp VMs func TestAccVcdVAppVmMetadata(t *testing.T) { testMetadataEntryCRUD(t, diff --git a/vcd/resource_vcd_vapp_vm_tools.go b/vcd/resource_vcd_vapp_vm_tools.go index 622717179..936167801 100644 --- a/vcd/resource_vcd_vapp_vm_tools.go +++ b/vcd/resource_vcd_vapp_vm_tools.go @@ -695,7 +695,10 @@ func updateTemplateInternalDisks(d *schema.ResourceData, meta interface{}, vm go } // Update details of internal disk for disk existing in template - if value, ok := internalDiskProvidedConfig["iops"]; ok { + if value := internalDiskProvidedConfig["iops"]; value.(int) != 0 || diskCreatedByTemplate.IopsAllocation != nil { + if diskCreatedByTemplate.IopsAllocation == nil { + diskCreatedByTemplate.IopsAllocation = &types.IopsResource{} + } iops := int64(value.(int)) diskCreatedByTemplate.IopsAllocation.Reservation = iops }