Fix azure key vault cleanup failure #8905

SwampDragons · 2020-03-18T20:40:23Z

Replace the nonfunctional delete mechanisms in the home-grown keyvault client with the official sdk vault client.

Fix the logic around deciding whether to delete the vault.

Closes #8493
Closes #8903

nywilken

Looks good to me. Happy to see that the SDK removes so much of the deletion logic. Nicely done 👍

nywilken · 2020-03-18T21:51:18Z

builder/azure/arm/step_delete_resource_group.go

@@ -45,7 +45,8 @@ func (s *StepDeleteResourceGroup) deleteResourceGroup(ctx context.Context, state

 		if keyVaultDeploymentName, ok := state.GetOk(constants.ArmKeyVaultDeploymentName); ok {
 			// Only delete if custom keyvault was not provided.
-			if exists := state.Get(constants.ArmIsExistingKeyVault).(bool); exists {
+			if exists := state.Get(constants.ArmIsExistingKeyVault).(bool); !exists {


Ah, nice catch here.

nywilken · 2020-03-18T21:52:35Z

builder/azure/arm/step_deploy_template.go

@@ -117,8 +117,7 @@ func deleteResource(ctx context.Context, client *AzureClient, resourceType strin
 		}
 		return err
 	case "Microsoft.KeyVault/vaults":
-		// TODO(paulmey): not sure why VaultClient doesn't do cancellation
-		_, err := client.VaultClientDelete.Delete(resourceGroupName, resourceName)
+		_, err := client.VaultClientDelete.Delete(ctx, resourceGroupName, resourceName)


I'll take a context for $2000 👍

nywilken · 2020-03-18T21:53:39Z

builder/azure/common/vault.go

@@ -137,72 +133,6 @@ func (client *VaultClient) SetSecret(vaultName, secretName string, secretValue s
 	return nil
 }

-// Delete deletes the specified Azure key vault.


dipesharora · 2020-03-19T19:16:54Z

Will it be released with ver 1.5.5 and what is the expected release date?

SwampDragons · 2020-03-23T17:28:25Z

It will be released with v1.5.5. We are hoping to release this week.

…atching Before change ``` 413e19b Merge pull request #8942 from desolatorxxl/google-fix-ssh-keys-metadata b81800d Merge pull request #8935 from zaventh/feature/start-on-boot 9486316 Merge pull request #8922 from hashicorp/f-vsphere_iso-export-ovf-options 56aebbe Merge pull request #8920 from rhencke/patch-1 d068430 make sure locals are evaluated only once variables are + test this (#8918) 3dae5df Merge pull request #8905 from hashicorp/fix_8493 811a730 Merge pull request #8907 from hashicorp/fix_8428 fa49d21 Merge pull request #8906 from hashicorp/fix_8904 23f5603 Merge pull request #8889 from hashicorp/hcl2_singular_blocks dc9259f Merge pull request #8892 from zaventh/feature/vga-adapter fc35f02 Merge pull request #8890 from hashicorp/fix_8880 7972ab7 Merge pull request #8735 from hashicorp/fix_plugin_loading 890d7b2 Merge pull request #8875 from hashicorp/fix_8812 e94ff70 Merge pull request #8883 from hashicorp/fix_8835 9075b80 Merge pull request #8891 from rhencke/patch-1 6477d8a Merge pull request #8882 from hashicorp/fix-var-file-hcl 6008f91 Merge pull request #8847 from takaishi/support-keyboard-interactive 5604561 Merge pull request #8877 from paulcichonski/remote-esxi-bastion 698f744 Merge pull request #8887 from hashicorp/untangle_ssh_docs_from_aws aeedc9a Merge pull request #8879 from mbrancato/specify_keyvault_sku 5365fda Merge pull request #8884 from hashicorp/fix_codecov_config 4bd7b14 Merge pull request #8732 from jhawk28/reorder_cdrom_drive 072a71b Merge pull request #8863 from hashicorp/update_go-cty_regex 8a1caaa Merge pull request #8837 from hashicorp/fix_8730 7873cab Merge pull request #8858 from hashicorp/fix_8791 7e382d0 Merge pull request #8828 from mvitaly/fix_8816 8832b3e Merge pull request #8787 from jhawk28/vsphere_iso_multiple_disks 5281740 Merge pull request #8831 from rjhornsby/master e35a872 Merge pull request #8830 from hashicorp/d-var-file-hcl2-not-yet ``` After change ``` ⇶ git log v1.5.4...v1.5.5 --first-parent --oneline --grep="Merge pull request #[0-9]\+" --grep="(#[0-9]\+)$" 413e19b Merge pull request #8942 from desolatorxxl/google-fix-ssh-keys-metadata c387dc2 builder/vsphere-clone: Find the vm within the folder (#8938) b17b211 Add cleanup_remote_cache config option to vmware-iso (#8917) e6368b9 Fix azure winrm_password attribution and allow to set winrm_username (#8928) fcf10e9 Replace Amazon with Outscale for OSC BSU doc (#8944) 9240fb7 Fix typo in title (#8943) 2c6f096 Allow accepting image for the members in OpenStack builder (#8931) b81800d Merge pull request #8935 from zaventh/feature/start-on-boot daffd9c CONTRIBUTING: Update documentation for linting on Travis (#8933) 3a9d356 golangci-lint: Update --new-from-rev option to check only newly added commits (#8923) 97d797d Fix small typos in osc-bsuvolume.html.md (#8926) 9486316 Merge pull request #8922 from hashicorp/f-vsphere_iso-export-ovf-options 56aebbe Merge pull request #8920 from rhencke/patch-1 99b0b98 Add ovf export capability to vsphere builders (#8764) d068430 make sure locals are evaluated only once variables are + test this (#8918) ad8dafa HCL: add tests and fixes around var-file and var args (#8914) 7979ab0 Add after_n_builds to codecov.yml (#8913) 3dae5df Merge pull request #8905 from hashicorp/fix_8493 811a730 Merge pull request #8907 from hashicorp/fix_8428 fa49d21 Merge pull request #8906 from hashicorp/fix_8904 b94937c Update provisioner_test.go (#8900) 2319521 Add iso config test for checksum from file specific case (#8897) 23f5603 Merge pull request #8889 from hashicorp/hcl2_singular_blocks dc9259f Merge pull request #8892 from zaventh/feature/vga-adapter 690bf71 Add Codecov badge and remove report style (#8896) fc35f02 Merge pull request #8890 from hashicorp/fix_8880 7972ab7 Merge pull request #8735 from hashicorp/fix_plugin_loading 890d7b2 Merge pull request #8875 from hashicorp/fix_8812 e94ff70 Merge pull request #8883 from hashicorp/fix_8835 ```

ghost · 2020-04-18T01:50:07Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

SwampDragons requested review from paulmey and a team as code owners March 18, 2020 20:40

SwampDragons added bug builder/azure labels Mar 18, 2020

SwampDragons added this to the 1.5.5 milestone Mar 18, 2020

SwampDragons changed the title ~~Fix 8493~~ Fix azure key vault cleanup failure Mar 18, 2020

SwampDragons added 3 commits March 18, 2020 13:48

fix error cleaning up existing keyvault

e7dba3e

replace the homegrown vault delete functions in azure with the sdk ones.

28797dd

add keyvault portion of azure sdk

564c889

SwampDragons force-pushed the fix_8493 branch from e0143d8 to 564c889 Compare March 18, 2020 20:49

nywilken approved these changes Mar 18, 2020

View reviewed changes

SwampDragons merged commit 3dae5df into master Mar 18, 2020

azr deleted the fix_8493 branch March 19, 2020 09:06

ghost locked and limited conversation to collaborators Apr 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix azure key vault cleanup failure #8905

Fix azure key vault cleanup failure #8905

SwampDragons commented Mar 18, 2020 •

edited

Loading

nywilken left a comment

nywilken Mar 18, 2020

nywilken Mar 18, 2020

nywilken Mar 18, 2020

dipesharora commented Mar 19, 2020 •

edited

Loading

SwampDragons commented Mar 23, 2020

ghost commented Apr 18, 2020

Fix azure key vault cleanup failure #8905

Fix azure key vault cleanup failure #8905

Conversation

SwampDragons commented Mar 18, 2020 • edited Loading

nywilken left a comment

Choose a reason for hiding this comment

nywilken Mar 18, 2020

Choose a reason for hiding this comment

nywilken Mar 18, 2020

Choose a reason for hiding this comment

nywilken Mar 18, 2020

Choose a reason for hiding this comment

dipesharora commented Mar 19, 2020 • edited Loading

SwampDragons commented Mar 23, 2020

ghost commented Apr 18, 2020

SwampDragons commented Mar 18, 2020 •

edited

Loading

dipesharora commented Mar 19, 2020 •

edited

Loading