[V2] Migrate all builders to hashicorp/go-azure-sdk #326

JenGoldstrich · 2023-08-02T22:45:08Z

This PR combines several other PRs which remove the now deprecated "track 1" SDK we were using and switching over to the same SDK that the Terraform AzureRM provider uses https://github.com/hashicorp/go-azure-sdk

Removes VHD Deprecation warning

Removes use_interactive_auth from ARM/DTL builders, we recommend using the Azure CLI via az login --use-device-code and then using Azure CLI auth

adds winrm_expiration_time field to the ARM builder

#306 - ARM Builder
JenGoldstrich#4 - DTL Builder
JenGoldstrich#5 - Chroot Builder
JenGoldstrich#3 - Unwind template capture TODO
JenGoldstrich#6 - Add Expiry Winrm Field, fail fast on in valid SIG version, and poll consistently as new SDK expects

closes #243
closes #250
closes #235
closes #187
closes #315

JenGoldstrich · 2023-08-03T22:35:01Z

builder/azure/arm/step_deploy_template.go

@@ -112,13 +126,14 @@ func (s *StepDeployTemplate) Cleanup(state multistep.StateBag) {
 		}
 		if !state.Get(constants.ArmKeepOSDisk).(bool) {
 			ui.Say(fmt.Sprintf(" Deleting -> %s : '%s'", imageType, imageName))
-			err = s.deleteDisk(context.TODO(), imageName, resourceGroupName, isManagedDisk)
+			err = s.deleteDisk(ctx, imageName, resourceGroupName, (isManagedDisk || isSIGImage), subscriptionId, armStorageAccountName)


This is the change that fixes #315, since before SIG images would use the VHD deletion path

JenGoldstrich · 2023-08-03T22:55:53Z

go.mod

 	github.com/hashicorp/hcl/v2 v2.16.2
-	github.com/hashicorp/packer-plugin-sdk v0.5.1
+	github.com/hashicorp/packer-plugin-sdk v0.4.0


I had downgraded temporarily this please let me know what version I should upgrade this back to for the release!

When ready you will need github.com/hashicorp/packer-plugin-sdk v0.5.1 with the go-cty fix for the Azure plugin

go get github.com/hashicorp/packer-plugin-sdk@v0.5.1 make install-packer-sdc packer-sdc fix . go mod tidy

JenGoldstrich · 2023-08-03T22:56:20Z

go.mod

@@ -3,32 +3,32 @@ module github.com/hashicorp/packer-plugin-azure
 go 1.19

 require (
-	github.com/Azure/azure-sdk-for-go v64.0.0+incompatible


nywilken

Jenna this is looking good already. Thank you a 💯 times for taking on this work and doing such a great job leading the effort.

I believe most of these changes have been reviewed from the previous PRs correct?

I've slated time to go over the changes on Monday. To help with the review a bit could you rebase and squash the commits into logical PR merges? It would make it easy to cross reference.

nywilken · 2023-08-04T01:01:00Z

go.mod

 	github.com/hashicorp/hcl/v2 v2.16.2
-	github.com/hashicorp/packer-plugin-sdk v0.5.1
+	github.com/hashicorp/packer-plugin-sdk v0.4.0


When ready you will need github.com/hashicorp/packer-plugin-sdk v0.5.1 with the go-cty fix for the Azure plugin

go get github.com/hashicorp/packer-plugin-sdk@v0.5.1 make install-packer-sdc packer-sdc fix . go mod tidy

Rebasing entire ARM builder migration on top of main Remove some missed old SDK stuff, including the autorest To package from ARM Builder Update oidc-example.json Pass SIG ID subscription through to state Use Hamilton MSGraph SDK to fetch Object ID for Windows builds Format and add comment commit generated code Switch AuthType back to private variable and use helper function to get it, switch from MeClient to ServicePrincipals client to get objectID Print MSgraph error again Don't use Hamilton, get it from the Access Token fmt and pass in correct context on cleanup Mess with backoff time while testing to see if this can be a bit smoother of a process for users Don't use context.TODOs in ARM Builder client calls Use hcl file in oidc test Use Giovanni instead of Blob Container serivce Implement new SDK environment logic Address fmt Remove unused function, go mod tidy Switch to forked release process

…eplicated to DTL/Chroot

…ing to create it Implement Lucas's feedback Make generate

======= DTL almost done! Just need to get VMID instead of capture template method and start testing Port DTL provisioner to new SDK Make generate Accidently commited playing around with this field, but was missing comma Appease the lint lord DTL never supported VHDs, it just had all the code of it, so I didn't need to port some of this Use retry config instead of an infinite loop Delete more unused code Update comments Address linter

==== EOD WIP: I've ported all the client code over, on Monday I will test that it works in a VM before porting the unit tests as the mocking strategy used is heavily reliant on auto rest, and I want to port it to stretchify mocks that don't care as much about the autorest specifics Don't use Subscription ID as Snapshot URL Large commit: Chroot unit tests passing, azure-sdk-for-go no longer imported by go.mod, remove references to device code, clear up missing pieces of DTL builder Fix linux disk attatcher Remove unnused mock that I added earlier Re-add Fetching of Tenant ID Forgot to use New Steps null check was wrong way for SDK model see above Don't try and get tenant ID if we're using CLI auth Address linter, fix linux test StepCreateNewDiskTest -> Return proper autorest future fake remove device code acceptance tests, handle flakey unit test by sorting resulting objects to insure consistant comparison Update builder/azure/chroot/step_verify_source_disk_test.go Co-authored-by: Lucas Bajolet <105649352+lbajolet-hashicorp@users.noreply.github.com> Remove CHRoot Acceptance Test && rename all hashi* imports removing prefix Make generate and fix lint Readd subscription, tenant ID fetching for CLI auth, so we know which subscription to save stuff to, this caused acceptance test failures which this commit fixes Lint

…ntext timeout throughout codebase Reset readme, version file, and workflows to state in main origin

[chroot] PollingDelay != Polling Duration >_>

… remove VHD Deprecation

Okay linter..

JenGoldstrich · 2023-08-04T22:15:30Z

@nywilken done and done!

JenGoldstrich · 2023-08-04T22:16:39Z

docs/builders/arm.mdx

@@ -526,27 +526,6 @@ prevent running afoul of Azure decency controls.
 The password alphabet used for random values is
 **0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ**.

-### Deprecation Warning


Will post a longer clarification on the removal of the deprecation warning for VHDs in the release notes for v2.0.0

nywilken

This is looking good. I have a few suggestions to help improve readability. I'm still working through a few more of the commits.

nywilken · 2023-08-07T15:48:10Z

builder/azure/arm/artifact.go

@@ -56,7 +50,7 @@ type Artifact struct {
 	StateData map[string]interface{}
 }

-func NewManagedImageArtifact(osType, resourceGroup, name, location, id, osDiskSnapshotName, dataDiskSnapshotPrefix string, generatedData map[string]interface{}, keepOSDisk bool, template *CaptureTemplate, getSasUrl func(name string) string) (*Artifact, error) {
+func NewManagedImageArtifact(osType, resourceGroup, name, location, id, osDiskSnapshotName, dataDiskSnapshotPrefix string, generatedData map[string]interface{}, osDiskUri string) (*Artifact, error) {


Future todo - we should rethink the number of parameters for this function.

nywilken · 2023-08-07T16:00:32Z

builder/azure/arm/artifact.go

-		return nil, fmt.Errorf("malformed capture template, expected one resource")
-	}
+func NewArtifact(vmInternalID string, storageAccountUrl string, storageAccountLocation string, osType string, additionalDiskCount int, generatedData map[string]interface{}) (*Artifact, error) {
+	vhdUri := fmt.Sprintf("%ssystem/Microsoft.Compute/Images/images/packer-osDisk.%s.vhd", storageAccountUrl, vmInternalID)


If I was to create an Artifact without this constructor artifact := &Artifact{//...} how would I know the values to be used for OSDiskUri and TemplateUri?

Should there be functions for generating these values with valid inputs?

When it comes to generating the vhdUri do we need to validate that the storageAccountUrl ends with a /?
If so maybe we use the url package for joining URL strings.

Just to close the loop - as we discuss internally we will hold on refactors to not introduce untested changes.

nywilken · 2023-08-07T16:02:47Z

builder/azure/arm/artifact.go

-	if err != nil {
-		return nil, err
-	}
+	templateUri := fmt.Sprintf("%ssystem/Microsoft.Compute/Images/images/packer-vmTemplate.%s.json", storageAccountUrl, vmInternalID)


ditto on having a generate template URI function

builder/azure/arm/azure_client.go

builder/azure/arm/builder.go

builder/azure/arm/step_capture_image.go

…thType error

builder/azure/dtl/template_factory.go

builder/azure/arm/builder.go

builder/azure/arm/step_capture_image.go

nywilken

Nicely done. Please use the upgrade notes sections once released to communicate major breaking changes and migration steps for folks using v1 of the plugin.

lbajolet-hashicorp

Left a few nits, but overall this looks good @JenGoldstrich! Good job on that

lbajolet-hashicorp · 2023-08-11T14:19:29Z

builder/azure/arm/config.go

@@ -1239,6 +1245,11 @@ func assertRequiredParametersSet(c *Config, errs *packersdk.MultiError) {
 		}
 		if c.SharedGalleryDestination.SigDestinationImageVersion == "" {
 			errs = packersdk.MultiErrorAppend(errs, fmt.Errorf("An image_version must be specified for shared_image_gallery_destination"))


I don't know if this is a pattern in the codebase, but I would think we can only check for the regex match and combine the two messages in one like An image_version must be specified for shared_image_gallery_destination and must follow the Major(int).Minor(int).Patch(int) format.
This can be done later of course as there's already a lot here, but I would think this gives a user more information on how to solve the issue while only checking once in the code.

Great point, I will make that change

lbajolet-hashicorp · 2023-08-11T14:25:42Z

builder/azure/arm/config_test.go

@@ -525,25 +525,25 @@ func TestConfigInstantiatesCorrectAzureEnvironment(t *testing.T) {
 		"communicator":           "none",
 	}

-	// user input is fun :)
+	// user input is fun :D


Best change of the whole PR

lbajolet-hashicorp · 2023-08-11T14:39:20Z

builder/azure/arm/step_capture_image.go

 		s.say(s.client.LastError.Error())
+		return err


I would think you can return err in the main path as you did for captureImageFromVM

lbajolet-hashicorp · 2023-08-11T15:12:05Z

builder/azure/arm/step_deploy_template.go

 	if err != nil {
 		return err
 	}
-
-	return f.WaitForCompletionRef(ctx, s.client.DeploymentsClient.Client)
+	return nil


This could become a return err if you remove the error check

NOTE: I see this in several places in the code, so maybe that's a cleanup to do for later as well

builder/azure/common/client/config.go

This was referenced Aug 3, 2023

support to add expiration date when setting winrm certificate in the KeyVault #235

Closed

Improve the validation of image version #187

Closed

Update packer arm builder to use updated microsoft azure.storage packages to remediate CVE-2022-30187 #274

Closed

JenGoldstrich force-pushed the main branch 2 times, most recently from 591af90 to 32dd4a9 Compare August 3, 2023 20:33

JenGoldstrich commented Aug 3, 2023

View reviewed changes

JenGoldstrich mentioned this pull request Aug 3, 2023

Failed to Delete Temporary VM Disk - Authentication Not Supported #315

Closed

JenGoldstrich marked this pull request as ready for review August 3, 2023 22:52

JenGoldstrich requested a review from a team as a code owner August 3, 2023 22:52

JenGoldstrich commented Aug 3, 2023

View reviewed changes

nywilken reviewed Aug 4, 2023

View reviewed changes

JenGoldstrich added 10 commits August 4, 2023 13:26

Azure Germany was shutdown in 2021

c46a7ce

Remove Template Capture from ARM Builder, this work will need to be r…

61152df

…eplicated to DTL/Chroot

Fail fast: Check if a SIG Destination Image Version exists before try…

dc5d2d8

…ing to create it Implement Lucas's feedback Make generate

Add WinRM secret expiration, Fast Fail on invalid SIG version, Use co…

4d0c3ba

…ntext timeout throughout codebase Reset readme, version file, and workflows to state in main origin

Pass in expiry time when making a new Key Vault as well, lint

78b7d4b

[chroot] PollingDelay != Polling Duration >_>

Fix rebasing error where we were logging for skip deletion always and…

c6731f0

… remove VHD Deprecation

Update goreleaser and codeowners to remove jenna's fork logic

e3e5955

Okay linter..

JenGoldstrich force-pushed the main branch from 820ebb5 to e3e5955 Compare August 4, 2023 20:29

Packer Plugin SDK v0.5.1

06a217b

JenGoldstrich commented Aug 4, 2023

View reviewed changes

nywilken reviewed Aug 7, 2023

View reviewed changes

builder/azure/arm/step_capture_image.go Show resolved Hide resolved

JenGoldstrich changed the title ~~V2.0.0: Migrate all builders to hashicorp/go-azure-sdk~~ [V2] Migrate all builders to hashicorp/go-azure-sdk Aug 7, 2023

Partially implement Wilken's feedback

e1dc7d1

Add comments based on Wilken's feedback, don't panic on unexpected Au…

4a53467

…thType error

nywilken reviewed Aug 10, 2023

View reviewed changes

builder/azure/dtl/template_factory.go Outdated Show resolved Hide resolved

builder/azure/arm/builder.go Outdated Show resolved Hide resolved

builder/azure/arm/step_capture_image.go Outdated Show resolved Hide resolved

Implement Wilken's feedback on Null Model SDK Error

ae9ef52

nywilken added enhancement breaking-change version/bump major A PR that breaks backwards compatibility. security Security issues/fixes. labels Aug 11, 2023

nywilken approved these changes Aug 11, 2023

View reviewed changes

lbajolet-hashicorp approved these changes Aug 11, 2023

View reviewed changes

Implement Lucas's feeback

c5762d3

JenGoldstrich merged commit a810cc7 into hashicorp:main Aug 11, 2023
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[V2] Migrate all builders to hashicorp/go-azure-sdk #326

[V2] Migrate all builders to hashicorp/go-azure-sdk #326

JenGoldstrich commented Aug 2, 2023 •

edited

Loading

JenGoldstrich Aug 3, 2023

JenGoldstrich Aug 3, 2023

nywilken Aug 4, 2023 •

edited by JenGoldstrich

Loading

JenGoldstrich Aug 3, 2023

nywilken left a comment

nywilken Aug 4, 2023 •

edited by JenGoldstrich

Loading

JenGoldstrich commented Aug 4, 2023

JenGoldstrich Aug 4, 2023

nywilken left a comment

nywilken Aug 7, 2023

nywilken Aug 7, 2023

nywilken Aug 11, 2023

nywilken Aug 7, 2023

nywilken left a comment

lbajolet-hashicorp left a comment

lbajolet-hashicorp Aug 11, 2023

JenGoldstrich Aug 11, 2023

lbajolet-hashicorp Aug 11, 2023

lbajolet-hashicorp Aug 11, 2023

lbajolet-hashicorp Aug 11, 2023

lbajolet-hashicorp Aug 11, 2023

[V2] Migrate all builders to hashicorp/go-azure-sdk #326

[V2] Migrate all builders to hashicorp/go-azure-sdk #326

Conversation

JenGoldstrich commented Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nywilken Aug 4, 2023 • edited by JenGoldstrich Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nywilken left a comment

Choose a reason for hiding this comment

nywilken Aug 4, 2023 • edited by JenGoldstrich Loading

Choose a reason for hiding this comment

JenGoldstrich commented Aug 4, 2023

Choose a reason for hiding this comment

nywilken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nywilken left a comment

Choose a reason for hiding this comment

lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JenGoldstrich commented Aug 2, 2023 •

edited

Loading

nywilken Aug 4, 2023 •

edited by JenGoldstrich

Loading

nywilken Aug 4, 2023 •

edited by JenGoldstrich

Loading