SDK Migration - V2 Beta #306

JenGoldstrich · 2023-05-31T17:18:54Z

Description

This PR switches from the ARM Builder from the "track 1" SDK we've been using to an SDK maintained by the Terraform Azure Provider team, this new SDK is located here. It has drop in support for track 2 SDKs to help us address the deprecation of the existing SDK with fewer breaking changes. As the new SDK adds track 2 support we can migrate them over time

Related to #243

Breaking Changes

All ARM builder functionality moved to github.com/hashicorp/go-azure-sdk
Address Connect to Azure Storage account using Managed identity and not Account key. #250 for the ARM Builder as we are currently considering removing the Read Only SAS's that are generated during VHD builds. And no longer create a connection to the storage account via an account key
Device Code authentication is removed, we are reccomending using az login --use-device-code instead to replicate this functionality

Other Changes

Removed 5 minute expiration check on JWT tokens, as OIDC tokens don't work with this expiration check

What's not done

Usage of PollingDurationTimeout as the actual polling duration timeout consistently throughout new code
DTL Builder and CHroot are not tested or guaranteed to work with this beta release, they will be migrated after we have validated the migration of the ARM builder and gotten feedback from users
The new SDK supports has some endpoints on track 2, these endpoints require the creation of new request/response handlers that will have to be added before the full v2 release

…rom ARM Builder

JenGoldstrich · 2023-05-31T17:25:02Z

builder/azure/common/client/config.go

-			if claims.ExpiresAt < time.Now().Add(5*time.Minute).Unix() {
-				errs = packersdk.MultiErrorAppend(errs, fmt.Errorf("client_jwt will expire within 5 minutes, please use a JWT that is valid for at least 5 minutes"))
-			}
+			//if claims.ExpiresAt < time.Now().Add(5*time.Minute).Unix() {


OIDC Access Tokens have a short validity period (60 seconds), so throwing this error prevents proper OIDC support, removing it fixes this, I have a slack link with some more context from the provider folks if needed

…et it, switch from MeClient to ServicePrincipals client to get objectID

…ther of a process for users

JenGoldstrich · 2023-06-01T23:03:26Z

builder/azure/common/client/config.go

+	return c.authType
+}
+
+// TODO I still need to port this to the new SDK


This logic is used by every builder, I'll port is towards the end of the process and leave this todo until then

JenGoldstrich · 2023-06-01T23:18:56Z

builder/azure/arm/step_deploy_template.go

@@ -305,12 +314,13 @@ func (s *StepDeployTemplate) deleteDeploymentResources(ctx context.Context, depl
 			defer wg.Done()
 			retryConfig := retry.Config{
 				Tries:      10,
-				RetryDelay: (&retry.Backoff{InitialBackoff: 10 * time.Second, MaxBackoff: 600 * time.Second, Multiplier: 2}).Linear,
+				RetryDelay: (&retry.Backoff{InitialBackoff: 5 * time.Second, MaxBackoff: 60 * time.Second, Multiplier: 1.5}).Linear,


This change seems to speed up builds quite a bit, if the delete failed a few times here we could end up waiting for quite a long time, this cuts off the max backoff at 1 minute, and lowers the multiplier that increases the backoff, anecdotally acceptance tests feel way faster, and the OIDC test build only took 5 minutes.

nywilken · 2023-06-02T20:40:41Z

builder/azure/arm/step_deploy_template.go

@@ -107,11 +107,11 @@ func (s *StepDeployTemplate) Cleanup(state multistep.StateBag) {
 		// 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(), subscriptionId, resourceGroupName, computeName)
+		imageType, imageName, err := s.disk(ctx, subscriptionId, resourceGroupName, computeName)


nywilken · 2023-06-02T20:44:38Z

builder/azure/arm/builder.go

@@ -65,7 +65,7 @@ func (b *Builder) Run(ctx context.Context, ui packersdk.Ui, hook packersdk.Hook)

 	ui.Say("Running builder ...")

-	ctx, cancel := context.WithCancel(ctx)
+	ctx, cancel := context.WithTimeout(ctx, time.Minute*60)


Does it make sense to make this timeout configurable via some configuration option?

nywilken · 2023-06-02T20:46:56Z

builder/azure/arm/builder.go

 	if b.config.ClientConfig.ObjectID == "" {
-		b.config.ClientConfig.ObjectID = getObjectIdFromToken(ui, spnCloud)
+		b.config.ClientConfig.ObjectID = *objectID


Is there ever a case where objectID is nil and this throws a nil pointer deference error?

nywilken · 2023-06-02T20:48:02Z

builder/azure/arm/builder.go

-				if err == nil {
-					err = f.WaitForCompletionRef(ctx, azureClient.ImagesClient.Client)
-				}
+				err := azureClient.ImagesClient.DeleteThenPoll(ctx, imageId)


nywilken · 2023-06-02T20:55:40Z

builder/azure/arm/resource_resolver.go

-	images, err := client.ImagesClient.ListByResourceGroupComplete(context.TODO(), resourceGroupName)
+func findManagedImageByName(client *AzureClient, name, subscriptionId, resourceGroupName string) (*hashiImagesSDK.Image, error) {
+	id := commonids.NewResourceGroupID(subscriptionId, resourceGroupName)
+	images, err := client.ImagesClient.ListByResourceGroupComplete(context.TODO(), id)


Not really part of the review but I'm curious why the code has context.TODO() as opposed to reusing the ctx created during runtime? Guessing there is no context being passed in would be my first guess.

Yes! So the context for the clients is mostly used for canceling and polling deadlines, I have not passed it through to here yet since it was only one get request used outside of a step, happy to add a real context here if you think it'd be better

go.mod

lbajolet-hashicorp

Reviewed ~half for now, will continue this tomorrow, overall there's a ton of changes and I left more questions than proper comments, but this looks good to me at first glance.
I notice that only the arm builder is updated now, will you refactor the rest later?

lbajolet-hashicorp · 2023-06-05T18:25:36Z

builder/azure/arm/artifact.go

@@ -56,7 +53,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) {


I'm very ignorant of this plugin, but what's a SAS URL, and why do we remove that here? I presume this is not necessary anymore?

Related but not too much, I see the only other place where there's a reference to a getSasUrl is in the NewArtifact function for the dtl builder (in builder/azure/dtl/artifact.go line 56), which is never used in the code according to gopls, so I presume we could remove this as well?

Good question, a SAS URL is a link to an Azure Storage account resource with an embedded token, we create SAS tokens in VHD builds and when using managed image builds without deleting the disk I believe, however we've been asked by a lot of our users to remove this here #250, because of the security concerns of using these they are not recommended by Microsoft, we are doing them in every build. To top it off the new SDK does not provide the same helper we currently use to generate these tokens.

So for v2 after chatting with Wilken, and given the above issue, I am proposing removing them, they can still be created by users outside of Packer, and this was never used in the resulting Azure Artifact, but was simply printed in the terminal output of the build.

Yes we should remove it from the DTL builder but I would like to hold of any changes to the DTL builder until we have signed off on the way we've handling this for the ARM builder

lbajolet-hashicorp · 2023-06-05T18:32:42Z

builder/azure/arm/azure_client.go

-	azureClient.DisksClient.UserAgent = fmt.Sprintf("%s %s", useragent.String(version.AzurePluginVersion.FormattedVersion()), azureClient.DisksClient.UserAgent)
+	// Clients that have been ported to hashicorp/go-azure-sdk
+	azureClient.DisksClient = hashiDisksSDK.NewDisksClientWithBaseURI(cloud.ResourceManagerEndpoint)
+	azureClient.DisksClient.Client.Authorizer = authWrapper.AutorestAuthorizer(resourceManagerAuthorizer)


Not sure if that's the same package referred to here, but from our discussions I remember autorest being a contentious choice, is this the same package referred to here? Won't we still experience crashes because of this one?

So autorest is an intermediary library used in track 1 SDKs, hashicorp/go-azure-sdk is still migrating from auto-rest to its own transit layer.

This code has been converted to use a different auth layer than Autorest, but track 1 SDKs still rely on autorest, so we can use the helper method in autoWrapper to convert this for SDKs not converted, then each time a client is moved to track 2 we will just replace authWrapper.AutorestAuthorizer(resourceManagerAuthorizer) with resourceManagerAuthorizer, as that helper function just turns the "new" auth object into the old one

lbajolet-hashicorp · 2023-06-05T18:36:11Z

builder/azure/arm/azure_client.go

+}
+
+// Returns an Azure Client used for the Azure Resource Manager
+// Also returns the Azure object ID for the authentication method used in the build


I wonder, why is the object ID returned as a separate value here? Shouldn't it be embedded in the AzureClient?

It used to be defined at the builder level and then passed into the client so I just kept it that way, also historically users could add the ObjectID in their templates instead of relying on us to grab it from the token claims

lbajolet-hashicorp · 2023-06-05T18:58:29Z

builder/azure/arm/azure_client.go

+		return "", err
+	}
+	return claims["oid"].(string), nil
+


Nit: superfluous empty line

lbajolet-hashicorp · 2023-06-05T19:02:38Z

builder/azure/arm/builder.go

@@ -80,28 +80,33 @@ func (b *Builder) Run(ctx context.Context, ui packersdk.Ui, hook packersdk.Hook)
 		b.stateBag.Put(constants.ArmManagedImageSubscription, b.config.ClientConfig.SubscriptionID)
 	}

+	b.stateBag.Put(constants.ArmSubscription, b.config.ClientConfig.SubscriptionID)


Since this is part of the config, do we need to keep this in the state? Are the steps that use this unable to access the full config?

Note: I see some other config options are being stored in the state so if that's a convention for this plugin feel free to disregard my comment.

lbajolet-hashicorp · 2023-06-05T19:38:59Z

builder/azure/arm/builder_acc_test.go

@@ -56,6 +57,7 @@ func TestBuilderAcc_SharedImageGallery_ARM64SpecializedLinuxSIG(t *testing.T) {
 		Setup: func() error {
 			createSharedImageGalleryDefinition(t, CreateSharedImageGalleryDefinitionParameters{
 				galleryImageName: "arm-linux-specialized-sig",
+				subscriptionId:   os.Getenv("ARM_SUBSCRIPTION_ID"),


Is there a reason why this is part of the environment rather than a constant for this test?

These acceptance tests rely on env variables for most of the Azure related fields that are in interpreted in the templates, this just grabs them to use in these functions

lbajolet-hashicorp · 2023-06-05T20:33:38Z

builder/azure/arm/resource_resolver.go

 	}

 	return nil, fmt.Errorf("Cannot find an image named '%s' in the resource group '%s'", name, resourceGroupName)
 }

-func findVirtualNetworkResourceGroup(client *AzureClient, name string) (string, error) {
-	virtualNetworks, err := client.VirtualNetworksClient.ListAllComplete(context.TODO())
+func findVirtualNetworkResourceGroup(client *AzureClient, subscriptionId, name string) (string, error) {


To fuel my curiosity, I see that a bunch of calls now include the subscriptionId, for those of us unitiated to Azure's offering, what's that and why do we need it now in so many places?

Azure Subscriptions are user created groups where resources are organized. Every resource (VM, Managed Image) has a Subscription, an a Resource Group which are sub-divisions of subscriptions.

In the old SDK we passed in the subscription ID into the Azure Client, in the new SDK they ask for the subscription ID on every call but the resulting API call is the same regardless.

lbajolet-hashicorp · 2023-06-05T20:38:35Z

builder/azure/arm/step_capture_image.go


-		return multistep.ActionHalt
+		// HACK(chrboum): I do not like this.  The capture method should be returning this value


Who's cherboum and paulmey? Are they working on the SDK?
Oh nevermind that's an older comment that was moved, still, is this hack still relevant with the refactor?

Yes but I haven't fully vetted that, there are two helper methods we use in azure client that still rely on autorest specific libraries, these are working on the new SDK currently but I need to migrate them still

lbajolet-hashicorp · 2023-06-05T20:45:23Z

builder/azure/arm/step_create_resource_group.go

 		s.say(s.client.LastError.Error())
 	}

-	return exists.Response.StatusCode != 404, err
+	return exists.HttpResponse.StatusCode != 404, nil


Does that happen? If the error might be linked to a 404, should we do the check in both places?

lbajolet-hashicorp · 2023-06-05T20:46:09Z

builder/azure/arm/step_create_resource_group.go

 	if err != nil {
+		if exists.HttpResponse.StatusCode == 404 {


Is there a chance that HttpResponse might be nil here? If so this will panic.

Yes but this was true in the old SDK and it never happened, definitely worth guarding against but unlikely to happen I believe

lbajolet-hashicorp · 2023-06-06T13:57:40Z

builder/azure/arm/step_get_source_image_name.go

@@ -1,6 +1,3 @@
-// Copyright (c) HashiCorp, Inc.
-// SPDX-License-Identifier: MPL-2.0


Unless I'm mistaken, this should stay part of the file for compliance

lbajolet-hashicorp · 2023-06-06T14:02:35Z

builder/azure/arm/step_publish_to_shared_image_gallery.go

 	"github.com/hashicorp/packer-plugin-azure/builder/azure/common/constants"
 	"github.com/hashicorp/packer-plugin-sdk/multistep"
 	packersdk "github.com/hashicorp/packer-plugin-sdk/packer"
 )

 type StepPublishToSharedImageGallery struct {
 	client  *AzureClient
-	publish func(ctx context.Context, sourceID string, sharedImageGallery SharedImageGalleryDestination, miSGImageVersionEndOfLifeDate string, miSGImageVersionExcludeFromLatest bool, miSigReplicaCount int32, location string, diskEncryptionSetId string, tags map[string]*string) (string, error)
+	publish func(ctx context.Context, subscriptionID string, sourceID string, sharedImageGallery SharedImageGalleryDestination, miSGImageVersionEndOfLifeDate string, miSGImageVersionExcludeFromLatest bool, miSigReplicaCount int64, location string, diskEncryptionSetId string, tags map[string]string) (string, error)


Out of curiosity, what's the mi in miSGImageVersion... referring to?
Machine image maybe?

I guess so, not 100% sure tbh

lbajolet-hashicorp · 2023-06-06T14:40:54Z

builder/azure/arm/template_factory_test.TestVirtualMachineDeployment13.approved.json

@@ -130,7 +130,7 @@
              "listeners": [
                {
                  "certificateUrl": "",
-                  "protocol": "https"
+                  "protocol": "Https"


Weird casing here, is this voluntary?

This is what the new SDK sends, they resolve the same but we do a file compare to check in the test that causes failures cause https != https, Azure interprets them the same

lbajolet-hashicorp · 2023-06-06T14:43:57Z

builder/azure/common/pointer.go

+package common
+
+// StringPtr returns a pointer to the passed string.
+func StringPtr(s string) *string {


I'm surprised we have to make our own, isn't there something like this in the Azure SDK already?

Might be a good idea to push it to the Hashicorp Azure SDK if not, especially if there's a bunch of string pointers in the API calls.

This was in the old SDK as an annoying helper tbh, and I didn't want to have to have a bunch of duplicated code. The new SDK does not have this and tbh I wasn't sure how it'd be received as a PR so I felt this was simpler

lbajolet-hashicorp · 2023-06-06T14:46:05Z

builder/azure/common/template/template.go

@@ -4,8 +4,14 @@
 package template


Out of curiosity again, what is being templated here? I'm not sure I understand what's the use for this package.

Azure ARM templates are kinda like packer/terraform templates, in that they are codied infrastructure, but its an Azure concept for defining certain types of deployments https://learn.microsoft.com/en-us/azure/azure-resource-manager/templates/overview

The way the Azure Packer plugin works is that it first deploys a template to create a VM, and then it captures that VM into an image artifact

maxilampert · 2023-06-18T21:30:33Z

builder/azure/common/client/config.go

+			"CHINACLOUD":      "china",
+			"AZURECHINACLOUD": "china",
+
+			// TODO Implement AzureGermanCloud


FYI Azure Germany doesn't exist anymore since October 2021. There are Azure regions in Germany but they are part of the general public cloud. Ref: Azure Germany

JenGoldstrich · 2023-06-21T16:57:36Z

rc1 with ARM builder support available here: https://github.com/JenGoldstrich/packer-plugin-azure/releases/tag/v2.0.0-betarc1

JenGoldstrich added 2 commits May 30, 2023 17:15

Rebasing entire ARM builder migration on top of main

e141427

Remove some missed old SDK stuff, including the autorest To package f…

fed4710

…rom ARM Builder

JenGoldstrich commented May 31, 2023

View reviewed changes

JenGoldstrich and others added 4 commits May 31, 2023 10:31

Update oidc-example.json

52b86d7

Pass SIG ID subscription through to state

337b9d9

Use Hamilton MSGraph SDK to fetch Object ID for Windows builds

2161a3d

Format and add comment

841570a

JenGoldstrich force-pushed the move-to-new-disks-sdk branch from 1ec98f0 to 841570a Compare May 31, 2023 22:42

JenGoldstrich added 2 commits May 31, 2023 15:50

commit generated code

64b5a43

Switch AuthType back to private variable and use helper function to g…

04a4a81

…et it, switch from MeClient to ServicePrincipals client to get objectID

JenGoldstrich force-pushed the move-to-new-disks-sdk branch from 505414e to 04a4a81 Compare June 1, 2023 18:30

JenGoldstrich added 4 commits June 1, 2023 11:37

Print MSgraph error again

a6971a6

Don't use Hamilton, get it from the Access Token

09e6211

fmt and pass in correct context on cleanup

fdc53ce

Mess with backoff time while testing to see if this can be a bit smoo…

083f230

…ther of a process for users

JenGoldstrich commented Jun 1, 2023

View reviewed changes

nywilken reviewed Jun 2, 2023

View reviewed changes

go.mod Outdated Show resolved Hide resolved

JenGoldstrich added 3 commits June 5, 2023 12:41

Don't use context.TODOs in ARM Builder client calls

1cd9b80

Update with newest cty fork

b8c600a

Use hcl file in oidc test

994b7f4

lbajolet-hashicorp reviewed Jun 5, 2023

View reviewed changes

lbajolet-hashicorp reviewed Jun 6, 2023

View reviewed changes

JenGoldstrich added 2 commits June 14, 2023 18:07

Use Giovanni instead of Blob Container serivce

a37cd44

Implement new SDK environment logic

ffd128b

JenGoldstrich added 2 commits June 15, 2023 15:08

Address fmt

2f80201

Remove unused function, go mod tidy

015fabd

JenGoldstrich mentioned this pull request Jun 15, 2023

ARM v2 Beta JenGoldstrich/packer-plugin-azure#1

Merged

maxilampert reviewed Jun 18, 2023

View reviewed changes

JenGoldstrich mentioned this pull request Aug 2, 2023

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

Merged

JenGoldstrich closed this Aug 2, 2023


		return multistep.ActionHalt
		// HACK(chrboum): I do not like this. The capture method should be returning this value

		@@ -1,6 +1,3 @@
		// Copyright (c) HashiCorp, Inc.
		// SPDX-License-Identifier: MPL-2.0

SDK Migration - V2 Beta #306

SDK Migration - V2 Beta #306

Conversation

JenGoldstrich commented May 31, 2023 • edited Loading

Description

Breaking Changes

Other Changes

What's not done

JenGoldstrich May 31, 2023 • edited Loading

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

Choose a reason for hiding this 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JenGoldstrich commented Jun 21, 2023

JenGoldstrich commented May 31, 2023 •

edited

Loading

JenGoldstrich May 31, 2023 •

edited

Loading