From bab4798abe990854b142df89ef2da3d9c7abbd67 Mon Sep 17 00:00:00 2001 From: Jenna Goldstrich Date: Thu, 28 Aug 2025 15:01:36 -0700 Subject: [PATCH] Fix Acceptance Tests, Fix issue with Revoke Disk Access not being called on failures, clarify VHD requirements in v2.5.0 --- .web-docs/components/builder/arm/README.md | 2 +- builder/azure/arm/builder_acc_test.go | 6 +-- builder/azure/arm/config.go | 2 +- builder/azure/arm/step_capture_image.go | 40 ++++++++++--------- builder/azure/arm/step_capture_image_test.go | 32 ++++++--------- .../builder/azure/arm/Config-not-required.mdx | 2 +- terraform/main.tf | 5 +++ 7 files changed, 45 insertions(+), 44 deletions(-) diff --git a/.web-docs/components/builder/arm/README.md b/.web-docs/components/builder/arm/README.md index 004c0041..6b1fcfe2 100644 --- a/.web-docs/components/builder/arm/README.md +++ b/.web-docs/components/builder/arm/README.md @@ -147,7 +147,7 @@ Providing `temp_resource_group_name` or `location` in combination with - `capture_name_prefix` (string) - VHD prefix. -- `capture_container_name` (string) - Destination container name. +- `capture_container_name` (string) - Destination container name. This must be created before the build in the storage account - `shared_image_gallery` (SharedImageGallery) - Use a [Shared Gallery image](https://azure.microsoft.com/en-us/blog/announcing-the-public-preview-of-shared-image-gallery/) diff --git a/builder/azure/arm/builder_acc_test.go b/builder/azure/arm/builder_acc_test.go index 9e4cc311..6a50d422 100644 --- a/builder/azure/arm/builder_acc_test.go +++ b/builder/azure/arm/builder_acc_test.go @@ -348,7 +348,7 @@ func testBuilderUserDataLinux(userdata string) string { "storage_account": "{{user `+"`storage_account`"+`}}", "resource_group_name": "{{user `+"`resource_group_name`"+`}}", - "capture_container_name": "test", + "capture_container_name": "packeracc", "capture_name_prefix": "testBuilderUserDataLinux", "os_type": "Linux", @@ -528,7 +528,7 @@ const testBuilderAccBlobWindows = ` "storage_account": "{{user ` + "`storage_account`" + `}}", "resource_group_name": "{{user ` + "`resource_group_name`" + `}}", - "capture_container_name": "azure-arm", + "capture_container_name": "packeracc", "capture_name_prefix": "testBuilderAccBlobWin", "os_type": "Windows", @@ -566,7 +566,7 @@ const testBuilderAccBlobLinux = ` "storage_account": "{{user ` + "`storage_account`" + `}}", "resource_group_name": "{{user ` + "`resource_group_name`" + `}}", - "capture_container_name": "test", + "capture_container_name": "packeracc", "capture_name_prefix": "testBuilderAccBlobLinux", "os_type": "Linux", diff --git a/builder/azure/arm/config.go b/builder/azure/arm/config.go index c8c5fc97..913d66b2 100644 --- a/builder/azure/arm/config.go +++ b/builder/azure/arm/config.go @@ -181,7 +181,7 @@ type Config struct { // VHD prefix. CaptureNamePrefix string `mapstructure:"capture_name_prefix"` - // Destination container name. + // Destination container name. This must be created before the build in the storage account CaptureContainerName string `mapstructure:"capture_container_name"` // Use a [Shared Gallery // image](https://azure.microsoft.com/en-us/blog/announcing-the-public-preview-of-shared-image-gallery/) diff --git a/builder/azure/arm/step_capture_image.go b/builder/azure/arm/step_capture_image.go index d629cb09..a5313ec5 100644 --- a/builder/azure/arm/step_capture_image.go +++ b/builder/azure/arm/step_capture_image.go @@ -24,16 +24,17 @@ import ( ) type StepCaptureImage struct { - client *AzureClient - config *Config - generalizeVM func(ctx context.Context, vmId virtualmachines.VirtualMachineId) error - getVMInternalID func(ctx context.Context, vmId virtualmachines.VirtualMachineId) (string, error) - captureManagedImage func(ctx context.Context, subscriptionId string, resourceGroupName string, imageName string, parameters *images.Image) error - grantAccess func(ctx context.Context, subscriptionId string, resourceGroupName string, osDiskName string) (string, error) - revokeAccess func(ctx context.Context, subscriptionId string, resourceGroupName string, osDiskName string) error - copyToStorage func(ctx context.Context, storageContainerName string, captureNamePrefix string, osDiskName string, accessUri string) error - say func(message string) - error func(e error) + client *AzureClient + config *Config + generalizeVM func(ctx context.Context, vmId virtualmachines.VirtualMachineId) error + getVMInternalID func(ctx context.Context, vmId virtualmachines.VirtualMachineId) (string, error) + captureManagedImage func(ctx context.Context, subscriptionId string, resourceGroupName string, imageName string, parameters *images.Image) error + grantAccess func(ctx context.Context, subscriptionId string, resourceGroupName string, osDiskName string) (string, error) + revokeAccess func(ctx context.Context, subscriptionId string, resourceGroupName string, osDiskName string) error + copyToStorage func(ctx context.Context, storageContainerName string, captureNamePrefix string, osDiskName string, accessUri string) error + say func(message string) + error func(e error) + diskNameToRevokeAccessTo string } type GrantAccessOperationResponse struct { @@ -212,7 +213,15 @@ func (s *StepCaptureImage) Run(ctx context.Context, state multistep.StateBag) mu return multistep.ActionContinue } -func (*StepCaptureImage) Cleanup(multistep.StateBag) { +func (s *StepCaptureImage) Cleanup(state multistep.StateBag) { + if s.diskNameToRevokeAccessTo != "" { + var subscriptionId = state.Get(constants.ArmSubscription).(string) + var resourceGroupName = state.Get(constants.ArmResourceGroupName).(string) + err := s.revokeAccess(context.Background(), subscriptionId, resourceGroupName, s.diskNameToRevokeAccessTo) + if err != nil { + state.Get("ui").(packersdk.Ui).Errorf("Failed to revoke access to disk, this will lead to failures cleaning up resources. Err: %s", err.Error()) + } + } } func (s *StepCaptureImage) captureVHD(ctx context.Context, subscriptionId string, resourceGroupName string, diskName string) error { @@ -221,7 +230,8 @@ func (s *StepCaptureImage) captureVHD(ctx context.Context, subscriptionId string err = fmt.Errorf("failed to grant access with err : %s", err) return err } - + // If the code is canceled or fails after this point, we must call to cleanup this granted access, otherwise delete operations will fail + s.diskNameToRevokeAccessTo = s.config.tmpOSDiskName s.say(fmt.Sprintf(" -> accessUri : '%s'", accessUri)) var storageContainerName = s.config.CaptureContainerName @@ -233,12 +243,6 @@ func (s *StepCaptureImage) captureVHD(ctx context.Context, subscriptionId string return err } - err = s.revokeAccess(ctx, subscriptionId, resourceGroupName, diskName) - if err != nil { - err = fmt.Errorf("failed to revoke access with err : %s", err) - return err - } - return nil } diff --git a/builder/azure/arm/step_capture_image_test.go b/builder/azure/arm/step_capture_image_test.go index fbf0e99d..e06b566c 100644 --- a/builder/azure/arm/step_capture_image_test.go +++ b/builder/azure/arm/step_capture_image_test.go @@ -10,6 +10,7 @@ import ( "github.com/hashicorp/go-azure-sdk/resource-manager/compute/2022-03-01/images" "github.com/hashicorp/go-azure-sdk/resource-manager/compute/2022-03-01/virtualmachines" + "github.com/hashicorp/packer-plugin-azure/builder/azure/common" "github.com/hashicorp/packer-plugin-azure/builder/azure/common/constants" "github.com/hashicorp/packer-plugin-sdk/multistep" ) @@ -110,26 +111,16 @@ func TestStepCaptureImageShouldFailIfCopyToStorageFails(t *testing.T) { } } -func TestStepCaptureImageShouldFailIfRevokeAccessFails(t *testing.T) { +func TestStepCaptureImageShouldRevokeAccessOnCleanup(t *testing.T) { + revokeAccessCalled := false + revokeAccessPtr := &revokeAccessCalled var testSubject = &StepCaptureImage{ config: &Config{ - tmpOSDiskName: "tmpOSDiskName", CaptureContainerName: "CaptureContainerName", CaptureNamePrefix: "CaptureNamePrefix", }, - grantAccess: func(ctx context.Context, subscriptionId string, resourceGroupName string, osDiskName string) (string, error) { - return "accessuri", nil - }, - copyToStorage: func(ctx context.Context, storageContainerName string, captureNamePrefix string, osDiskName string, accessUri string) error { - return nil - }, revokeAccess: func(ctx context.Context, subscriptionId string, resourceGroupName string, osDiskName string) error { - return fmt.Errorf("!! Unit Test FAIL !!") - }, - getVMInternalID: func(context.Context, virtualmachines.VirtualMachineId) (string, error) { - return "id", nil - }, - generalizeVM: func(context.Context, virtualmachines.VirtualMachineId) error { + revokeAccessPtr = common.BoolPtr(true) return nil }, say: func(message string) {}, @@ -138,13 +129,14 @@ func TestStepCaptureImageShouldFailIfRevokeAccessFails(t *testing.T) { stateBag := createTestStateBagStepCaptureImage() - var result = testSubject.Run(context.Background(), stateBag) - if result != multistep.ActionHalt { - t.Fatalf("Expected the step to return 'ActionHalt', but got '%d'.", result) + testSubject.Cleanup(stateBag) + if *revokeAccessPtr { + t.Fatal("Revoke Access should not be called on TestCaptureImage.Cleanup when a tmpOSDiskName is set, but was") } - - if _, ok := stateBag.GetOk(constants.Error); ok == false { - t.Fatalf("Expected the step to set stateBag['%s'], but it was not.", constants.Error) + testSubject.diskNameToRevokeAccessTo = "tmpOSDiskName" + testSubject.Cleanup(stateBag) + if !*revokeAccessPtr { + t.Fatal("Revoke Access should be called on TestCaptureImage.Cleanup when a tmpOSDiskName is set, but wasn't") } } diff --git a/docs-partials/builder/azure/arm/Config-not-required.mdx b/docs-partials/builder/azure/arm/Config-not-required.mdx index ab0b5a19..b560583c 100644 --- a/docs-partials/builder/azure/arm/Config-not-required.mdx +++ b/docs-partials/builder/azure/arm/Config-not-required.mdx @@ -9,7 +9,7 @@ - `capture_name_prefix` (string) - VHD prefix. -- `capture_container_name` (string) - Destination container name. +- `capture_container_name` (string) - Destination container name. This must be created before the build in the storage account - `shared_image_gallery` (SharedImageGallery) - Use a [Shared Gallery image](https://azure.microsoft.com/en-us/blog/announcing-the-public-preview-of-shared-image-gallery/) diff --git a/terraform/main.tf b/terraform/main.tf index 5b0ed6e2..c5c97321 100644 --- a/terraform/main.tf +++ b/terraform/main.tf @@ -15,6 +15,11 @@ resource "azurerm_storage_account" "storage-account" { account_replication_type = "GRS" } +resource "azurerm_storage_container" "example" { + name = "packeracc" + storage_account_id = azurerm_storage_account.storage-account.id +} + resource "azurerm_shared_image_gallery" "gallery" { name = "${var.resource_prefix}_acctestgallery" resource_group_name = azurerm_resource_group.rg.name