Skip to content

Commit

Permalink
Deploy template error handling cleanup (#296)
Browse files Browse the repository at this point in the history 
* Seperate cleanup for Virtual Machines and KeyVaults to reduce noise during ARM builds

* Add isVirtualMachineDeployment param to existing tests

* implement Wilken's feedback

* Not sure why this change is generated by Mac
  • Loading branch information
@JenGoldstrich
JenGoldstrich authored Apr 25, 2023
1 parent ad491dd commit a1b2693
Show file tree
Hide file tree
Showing 4 changed files with 104 additions and 66 deletions.
6 changes: 3 additions & 3 deletions builder/azure/arm/builder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ func (b *Builder) Run(ctx context.Context, ui packersdk.Ui, hook packersdk.Hook)
},
NewStepCreateResourceGroup(azureClient, ui),
NewStepValidateTemplate(azureClient, ui, &b.config, deploymentName, GetVirtualMachineDeployment),
NewStepDeployTemplate(azureClient, ui, &b.config, deploymentName, GetVirtualMachineDeployment),
NewStepDeployTemplate(azureClient, ui, &b.config, deploymentName, GetVirtualMachineDeployment, VirtualMachineTemplate),
NewStepGetIPAddress(azureClient, ui, endpointConnectType),
&communicator.StepConnectSSH{
Config: &b.config.Comm,
Expand Down Expand Up @@ -257,7 +257,7 @@ func (b *Builder) Run(ctx context.Context, ui packersdk.Ui, hook packersdk.Hook)
keyVaultDeploymentName := b.stateBag.Get(constants.ArmKeyVaultDeploymentName).(string)
steps = append(steps,
NewStepValidateTemplate(azureClient, ui, &b.config, keyVaultDeploymentName, GetCommunicatorSpecificKeyVaultDeployment),
NewStepDeployTemplate(azureClient, ui, &b.config, keyVaultDeploymentName, GetCommunicatorSpecificKeyVaultDeployment),
NewStepDeployTemplate(azureClient, ui, &b.config, keyVaultDeploymentName, GetCommunicatorSpecificKeyVaultDeployment, KeyVaultTemplate),
)
} else if b.config.Comm.Type == "winrm" {
steps = append(steps, NewStepCertificateInKeyVault(&azureClient.VaultClient, ui, &b.config, b.config.winrmCertificate))
Expand All @@ -281,7 +281,7 @@ func (b *Builder) Run(ctx context.Context, ui packersdk.Ui, hook packersdk.Hook)
NewStepGetCertificate(azureClient, ui),
NewStepSetCertificate(&b.config, ui),
NewStepValidateTemplate(azureClient, ui, &b.config, deploymentName, GetVirtualMachineDeployment),
NewStepDeployTemplate(azureClient, ui, &b.config, deploymentName, GetVirtualMachineDeployment),
NewStepDeployTemplate(azureClient, ui, &b.config, deploymentName, GetVirtualMachineDeployment, VirtualMachineTemplate),
NewStepGetIPAddress(azureClient, ui, endpointConnectType),
)

Expand Down
112 changes: 64 additions & 48 deletions builder/azure/arm/step_deploy_template.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,13 @@ import (
"github.com/hashicorp/packer-plugin-sdk/retry"
)

type DeploymentTemplateType int

const (
VirtualMachineTemplate DeploymentTemplateType = iota
KeyVaultTemplate
)

type StepDeployTemplate struct {
client *AzureClient
deploy func(ctx context.Context, resourceGroupName string, deploymentName string) error
Expand All @@ -30,16 +37,18 @@ type StepDeployTemplate struct {
config *Config
factory templateFactoryFunc
name string
templateType DeploymentTemplateType
}

func NewStepDeployTemplate(client *AzureClient, ui packersdk.Ui, config *Config, deploymentName string, factory templateFactoryFunc) *StepDeployTemplate {
func NewStepDeployTemplate(client *AzureClient, ui packersdk.Ui, config *Config, deploymentName string, factory templateFactoryFunc, templateType DeploymentTemplateType) *StepDeployTemplate {
var step = &StepDeployTemplate{
client: client,
say: func(message string) { ui.Say(message) },
error: func(e error) { ui.Error(e.Error()) },
config: config,
factory: factory,
name: deploymentName,
client: client,
say: func(message string) { ui.Say(message) },
error: func(e error) { ui.Error(e.Error()) },
config: config,
factory: factory,
name: deploymentName,
templateType: templateType,
}

step.deploy = step.deployTemplate
Expand Down Expand Up @@ -71,52 +80,59 @@ func (s *StepDeployTemplate) Cleanup(state multistep.StateBag) {
}()

ui := state.Get("ui").(packersdk.Ui)
ui.Say("\nDeleting individual resources ...")

deploymentName := s.name
resourceGroupName := state.Get(constants.ArmResourceGroupName).(string)
// Get image disk details before deleting the image; otherwise we won't be able to
// delete the disk as the image request will return a 404
computeName := state.Get(constants.ArmComputeName).(string)
isManagedDisk := state.Get(constants.ArmIsManagedImage).(bool)
imageType, imageName, err := s.disk(context.TODO(), resourceGroupName, computeName)

if err != nil && !strings.Contains(err.Error(), "ResourceNotFound") {
ui.Error(fmt.Sprintf("Could not retrieve OS Image details: %s", err))
}
err = s.delete(context.TODO(), deploymentName, resourceGroupName)
if err != nil {
s.reportIfError(err, resourceGroupName)
}
if s.templateType == KeyVaultTemplate {
ui.Say("\nDeleting KeyVault created during build")
err := s.delete(context.TODO(), deploymentName, resourceGroupName)
if err != nil {
s.reportIfError(err, resourceGroupName)
}

// The disk was not found on the VM, this is an error.
if imageType == "" && imageName == "" {
ui.Error(fmt.Sprintf("Failed to find temporary OS disk on VM. Please delete manually.\n\n"+
"VM Name: %s\n"+
"Error: %s", computeName, err))
return
}
if !state.Get(constants.ArmKeepOSDisk).(bool) {
ui.Say(fmt.Sprintf(" Deleting -> %s : '%s'", imageType, imageName))
err = s.deleteDisk(context.TODO(), imageName, resourceGroupName, isManagedDisk)
} else {
ui.Say("\nDeleting Virtual Machine deployment and its attatched resources...")
// Get image disk details before deleting the image; otherwise we won't be able to
// delete the disk as the image request will return a 404
computeName := state.Get(constants.ArmComputeName).(string)
isManagedDisk := state.Get(constants.ArmIsManagedImage).(bool)
imageType, imageName, err := s.disk(context.TODO(), resourceGroupName, computeName)
if err != nil {
ui.Error(fmt.Sprintf("Error deleting resource. Please delete manually.\n\n"+
"Name: %s\n"+
"Error: %s", imageName, err))
ui.Error(fmt.Sprintf("Could not retrieve OS Image details: %s", err))
}
err = s.delete(context.TODO(), deploymentName, resourceGroupName)
if err != nil {
s.reportIfError(err, resourceGroupName)
}
// The disk was not found on the VM, this is an error.
if imageType == "" && imageName == "" {
ui.Error(fmt.Sprintf("Failed to find temporary OS disk on VM. Please delete manually.\n\n"+
"VM Name: %s\n"+
"Error: %s", computeName, err))
return
}
if !state.Get(constants.ArmKeepOSDisk).(bool) {
ui.Say(fmt.Sprintf(" Deleting -> %s : '%s'", imageType, imageName))
err = s.deleteDisk(context.TODO(), imageName, resourceGroupName, isManagedDisk)
if err != nil {
ui.Error(fmt.Sprintf("Error deleting resource. Please delete manually.\n\n"+
"Name: %s\n"+
"Error: %s", imageName, err))
}
}
}

var dataDisks []string
if disks := state.Get(constants.ArmAdditionalDiskVhds); disks != nil {
dataDisks = disks.([]string)
}
for i, additionaldisk := range dataDisks {
s.say(fmt.Sprintf(" Deleting Additional Disk -> %d: '%s'", i+1, additionaldisk))
var dataDisks []string
if disks := state.Get(constants.ArmAdditionalDiskVhds); disks != nil {
dataDisks = disks.([]string)
}
for i, additionaldisk := range dataDisks {
s.say(fmt.Sprintf(" Deleting Additional Disk -> %d: '%s'", i+1, additionaldisk))

err := s.deleteImage(context.TODO(), additionaldisk, resourceGroupName, isManagedDisk)
if err != nil {
s.say("Failed to delete the managed Additional Disk!")
err := s.deleteImage(context.TODO(), additionaldisk, resourceGroupName, isManagedDisk)
if err != nil {
s.say("Failed to delete the managed Additional Disk!")
}
}

}
}

Expand Down Expand Up @@ -299,9 +315,9 @@ func (s *StepDeployTemplate) deleteDeploymentResources(ctx context.Context, depl
resourceName,
resourceGroupName)
if err != nil {
s.say(fmt.Sprintf("Error deleting resource. Will retry.\n"+
"Name: %s\n"+
"Error: %s\n", resourceName, err.Error()))
s.say(fmt.Sprintf("Couldn't delete %s resource. Will retry.\n"+
"Name: %s",
resourceType, resourceName))
}
return err
})
Expand Down
50 changes: 36 additions & 14 deletions builder/azure/arm/step_deploy_template_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,10 @@ func TestStepDeployTemplateShouldTakeStepArgumentsFromStateBag(t *testing.T) {

return nil
},
say: func(message string) {},
error: func(e error) {},
name: "--deployment-name--",
say: func(message string) {},
error: func(e error) {},
name: "--deployment-name--",
templateType: VirtualMachineTemplate,
}

stateBag := createTestStateBagStepValidateTemplate()
Expand All @@ -89,9 +90,10 @@ func TestStepDeployTemplateShouldTakeStepArgumentsFromStateBag(t *testing.T) {

func TestStepDeployTemplateDeleteImageShouldFailWhenImageUrlCannotBeParsed(t *testing.T) {
var testSubject = &StepDeployTemplate{
say: func(message string) {},
error: func(e error) {},
name: "--deployment-name--",
say: func(message string) {},
error: func(e error) {},
name: "--deployment-name--",
templateType: VirtualMachineTemplate,
}
// Invalid URL per https://golang.org/src/net/url/url_test.go
err := testSubject.deleteImage(context.TODO(), "http://[fe80::1%en0]/", "Unit Test: ResourceGroupName", false)
Expand All @@ -102,9 +104,10 @@ func TestStepDeployTemplateDeleteImageShouldFailWhenImageUrlCannotBeParsed(t *te

func TestStepDeployTemplateDeleteImageShouldFailWithInvalidImage(t *testing.T) {
var testSubject = &StepDeployTemplate{
say: func(message string) {},
error: func(e error) {},
name: "--deployment-name--",
say: func(message string) {},
error: func(e error) {},
name: "--deployment-name--",
templateType: VirtualMachineTemplate,
}
err := testSubject.deleteImage(context.TODO(), "storage.blob.core.windows.net/abc", "Unit Test: ResourceGroupName", false)
if err == nil {
Expand All @@ -114,7 +117,7 @@ func TestStepDeployTemplateDeleteImageShouldFailWithInvalidImage(t *testing.T) {

func TestStepDeployTemplateCleanupShouldDeleteManagedOSImageInExistingResourceGroup(t *testing.T) {
var deleteDiskCounter = 0
var testSubject = createTestStepDeployTemplateDeleteOSImage(&deleteDiskCounter)
var testSubject = createTestStepDeployTemplateDeleteOSImage(&deleteDiskCounter, VirtualMachineTemplate)

stateBag := createTestStateBagStepDeployTemplate()
stateBag.Put(constants.ArmIsManagedImage, true)
Expand All @@ -132,7 +135,7 @@ func TestStepDeployTemplateCleanupShouldDeleteManagedOSImageInExistingResourceGr

func TestStepDeployTemplateCleanupShouldDeleteManagedOSImageInTemporaryResourceGroup(t *testing.T) {
var deleteDiskCounter = 0
var testSubject = createTestStepDeployTemplateDeleteOSImage(&deleteDiskCounter)
var testSubject = createTestStepDeployTemplateDeleteOSImage(&deleteDiskCounter, VirtualMachineTemplate)

stateBag := createTestStateBagStepDeployTemplate()
stateBag.Put(constants.ArmIsManagedImage, true)
Expand All @@ -150,7 +153,7 @@ func TestStepDeployTemplateCleanupShouldDeleteManagedOSImageInTemporaryResourceG

func TestStepDeployTemplateCleanupShouldDeleteVHDOSImageInExistingResourceGroup(t *testing.T) {
var deleteDiskCounter = 0
var testSubject = createTestStepDeployTemplateDeleteOSImage(&deleteDiskCounter)
var testSubject = createTestStepDeployTemplateDeleteOSImage(&deleteDiskCounter, VirtualMachineTemplate)

stateBag := createTestStateBagStepDeployTemplate()
stateBag.Put(constants.ArmIsManagedImage, false)
Expand All @@ -168,7 +171,7 @@ func TestStepDeployTemplateCleanupShouldDeleteVHDOSImageInExistingResourceGroup(

func TestStepDeployTemplateCleanupShouldVHDOSImageInTemporaryResourceGroup(t *testing.T) {
var deleteDiskCounter = 0
var testSubject = createTestStepDeployTemplateDeleteOSImage(&deleteDiskCounter)
var testSubject = createTestStepDeployTemplateDeleteOSImage(&deleteDiskCounter, VirtualMachineTemplate)

stateBag := createTestStateBagStepDeployTemplate()
stateBag.Put(constants.ArmIsManagedImage, false)
Expand All @@ -184,6 +187,24 @@ func TestStepDeployTemplateCleanupShouldVHDOSImageInTemporaryResourceGroup(t *te
}
}

func TestStepDeployTemplateCleanupShouldNotDeleteDiskForKeyVaultDeployments(t *testing.T) {
var deleteDiskCounter = 0
var testSubject = createTestStepDeployTemplateDeleteOSImage(&deleteDiskCounter, KeyVaultTemplate)

stateBag := createTestStateBagStepDeployTemplate()
stateBag.Put(constants.ArmIsManagedImage, false)
stateBag.Put(constants.ArmIsSIGImage, false)
stateBag.Put(constants.ArmIsExistingResourceGroup, true)
stateBag.Put(constants.ArmIsResourceGroupCreated, true)
stateBag.Put(constants.ArmKeepOSDisk, false)
stateBag.Put("ui", packersdk.TestUi(t))

testSubject.Cleanup(stateBag)
if deleteDiskCounter != 0 {
t.Fatalf("Expected DeployTemplate Cleanup to not invoke deleteDisk, but invoked %d times", deleteDiskCounter)
}
}

func createTestStateBagStepDeployTemplate() multistep.StateBag {
stateBag := new(multistep.BasicStateBag)

Expand All @@ -194,7 +215,7 @@ func createTestStateBagStepDeployTemplate() multistep.StateBag {
return stateBag
}

func createTestStepDeployTemplateDeleteOSImage(deleteDiskCounter *int) *StepDeployTemplate {
func createTestStepDeployTemplateDeleteOSImage(deleteDiskCounter *int, templateType DeploymentTemplateType) *StepDeployTemplate {
return &StepDeployTemplate{
deploy: func(context.Context, string, string) error { return nil },
say: func(message string) {},
Expand All @@ -212,5 +233,6 @@ func createTestStepDeployTemplateDeleteOSImage(deleteDiskCounter *int) *StepDepl
deleteDeployment: func(ctx context.Context, state multistep.StateBag) error {
return nil
},
templateType: templateType,
}
}
2 changes: 1 addition & 1 deletion docs-partials/builder/azure/arm/Config-not-required.mdx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@
[Windows](https://learn.microsoft.com/en-us/azure/virtual-machines/windows/hybrid-use-benefit-licensing)
or
[Linux](https://learn.microsoft.com/en-us/azure/virtual-machines/linux/azure-hybrid-benefit-linux)

- `secure_boot_enabled` (bool) - Specifies if Secure Boot and Trusted Launch is enabled for the Virtual Machine.

- `vtpm_enabled` (bool) - Specifies if vTPM (virtual Trusted Platform Module) and Trusted Launch is enabled for the Virtual Machine.
Expand Down

0 comments on commit a1b2693

Please sign in to comment.