Backend/azure/update to latest sdks #36258

magodo · 2025-01-03T07:06:50Z

This PR updates the azure backend authentication to match the terraform-provider-azurermprovider authentication, in several ways:

github.com/hashicorp/go-azure-helpers: v0.43.0 -> v0.71.0 （The latest one so far, used by azurerm provider v4.14.0)
github.com/hashicorp/go-azure-sdk/[resource-manager/sdk]: v0.20241212.1154051. This is the new hashicorp Azure SDK, which replaces the deprecated Azure Track1 SDK used before.
github.com/tombuildsstuff/giovanni: v0.15.1 -> v0.27.0. Meanwhile, updating the azure storage API version from 2018-11-09 to 2023-11-03.

The backend configuration logic is updated to match the provider logic. As a result, some new properties are added:

use_cli
use_aks_workload_identity
client_id_file_path
client_certificate
client_id_file_path
client_secret_file_path

One implementation detail is that the using the same Azure storage dataplane SDK, the storage client requires a base URI of the storage account, which is derived by sending a GET to the storage account. This is skipped in case the storage shared access key or sas token is specified, which is to behave identically as the current version.

Also, this PR improves the acctests in following ways:

Removing the spaghettitized code that impacts the production code merely to make the test works. Now the test code and prod code is splitted clearly
All tests run in the following patter: test client build authorizer with parameters set via env vars-> test client create test resources -> clean up these env vars -> merely using the hcl config for testing out the backend/remote client -> test client cleans up test resources. This pattern works fine in all facts, except that when you want to run tests in parallel, you'd ensure the parallelism set in go test is big enough to avoid env vars clean for the single process (launched by go test) won't interfere the paused tests.

Fixes #34322

Target Release

1.11.0

Draft CHANGELOG entry

ENHANCEMENTS

Test

# Run all the tests, except 3 of them are skipped
❯ TF_ACC=1 go test -timeout=20h -parallel=20 ./...
ok      github.com/hashicorp/terraform/internal/backend/remote-state/azure      217.326s

# Run 2 MI related tests on Azure VM
magodo@acctest-magodo-backend:~/terraform/internal/backend/remote-state/azure$ TF_RUNNING_IN_AZURE=1 TF_ACC=1 go test -parallel=2 -run='TestAccBackendManagedS
erviceIdentityBasic|TestRemoteClientManagedServiceIdentityBasic'
PASS
ok  github.com/hashicorp/terraform/internal/backend/remote-state/azure 117.385s

# Run OIDC test from GitHub action
# Following is the GitHub action log output
Run cd internal/backend/remote-state/azure
  cd internal/backend/remote-state/azure
  TF_RUNNING_IN_GITHUB_ACTIONS=1 \
  TF_ACC=1 \
  ARM_SUBSCRIPTION_ID=*** \
  ARM_TENANT_ID=*** \
  ARM_CLIENT_ID=*** \
  ARM_TEST_LOCATION=westus2 \
  go test -run="TestAccBackendGithubOIDCBasic" .
  shell: /usr/bin/bash -e {0}
...
go: download...
...
ok  	github.com/hashicorp/terraform/internal/backend/remote-state/azure	106.039s

…e-to-latest-sdks

…-11-09 to 2023-11-03

magodo · 2025-01-03T10:01:53Z

The failed "Unit Tests" is not related to this PR.

crw · 2025-01-08T21:55:56Z

Thanks for this submission, I let the HashiCorp Azure team know about this PR.

magodo · 2025-01-09T06:22:58Z

I'll stop resolving the go.mod/sum file conflicts until before the final merge.

mbfrahry

Hey @magodo, I went ahead and gave it a review and it mostly looks good outside of the things I've called out. Let me know if you have any questions around any of it

mbfrahry · 2025-01-08T00:03:25Z

go.mod

@@ -240,9 +239,12 @@ require (
 	github.com/tencentyun/cos-go-sdk-v5 v0.7.42 // indirect
 	github.com/thanhpk/randstr v1.0.6 // indirect
 	github.com/thlib/go-timezone-local v0.0.3 // indirect
-	github.com/tombuildsstuff/giovanni v0.15.1 // indirect
+	github.com/tombuildsstuff/giovanni v0.27.0 // indirect


We're looking to move away from Toms sdk and this should be swapped to https://github.com/jackofallops/giovanni

Got it, thx!

mbfrahry · 2025-01-09T00:57:01Z

internal/backend/remote-state/azure/helpers_test.go

+		t.Fatalf("Missing ARM_TENANT_ID")
+	}
+
+	location := os.Getenv("ARM_TEST_LOCATION")


To keep this consistent with our current tests, we should swap this to ARM_LOCATION

Following is the grep from v4.16.0:

terraform-provider-azurerm on  HEAD (f54480d) via 🐹 v1.23.3 💤 grep -r "ARM_TEST_LOCATION" . ./DEVELOPER.md:- `ARM_TEST_LOCATION` ./DEVELOPER.md:- `ARM_TEST_LOCATION_ALT` ./DEVELOPER.md:- `ARM_TEST_LOCATION_ALT2` ./vendor/github.com/magodo/terraform-provider-azurerm-example-gen/examplegen/gen.go: // - ARM_TEST_LOCATION ./vendor/github.com/magodo/terraform-provider-azurerm-example-gen/examplegen/gen.go: // - ARM_TEST_LOCATION_ALT1 ./vendor/github.com/magodo/terraform-provider-azurerm-example-gen/examplegen/gen.go: // - ARM_TEST_LOCATION_ALT2 ./vendor/github.com/magodo/terraform-provider-azurerm-example-gen/examplegen/gen.go: if os.Getenv("ARM_TEST_LOCATION") == "" { ./vendor/github.com/magodo/terraform-provider-azurerm-example-gen/examplegen/gen.go: cmd.Env = append(os.Environ(), "ARM_TEST_LOCATION=WestEurope") ./vendor/github.com/magodo/terraform-provider-azurerm-example-gen/examplegen/gen.go: if os.Getenv("ARM_TEST_LOCATION_ALT1") == "" { ./vendor/github.com/magodo/terraform-provider-azurerm-example-gen/examplegen/gen.go: cmd.Env = append(os.Environ(), "ARM_TEST_LOCATION_ALT1=WestUS") ./vendor/github.com/magodo/terraform-provider-azurerm-example-gen/examplegen/gen.go: if os.Getenv("ARM_TEST_LOCATION_ALT2") == "" { ./vendor/github.com/magodo/terraform-provider-azurerm-example-gen/examplegen/gen.go: cmd.Env = append(os.Environ(), "ARM_TEST_LOCATION_ALT2=WestUS2") ./contributing/topics/running-the-tests.md:* `ARM_TEST_LOCATION` ./contributing/topics/running-the-tests.md:* `ARM_TEST_LOCATION_ALT` ./contributing/topics/running-the-tests.md:* `ARM_TEST_LOCATION_ALT2` ./internal/acceptance/data.go: Primary: os.Getenv("ARM_TEST_LOCATION"), ./internal/acceptance/data.go: Secondary: os.Getenv("ARM_TEST_LOCATION_ALT"), ./internal/acceptance/data.go: Ternary: os.Getenv("ARM_TEST_LOCATION_ALT2"), ./internal/acceptance/testing.go: "ARM_TEST_LOCATION", ./internal/acceptance/testing.go: "ARM_TEST_LOCATION_ALT", ./internal/acceptance/testing.go: "ARM_TEST_LOCATION_ALT2", ./internal/acceptance/locations.go: os.Getenv("ARM_TEST_LOCATION"), ./internal/acceptance/locations.go: os.Getenv("ARM_TEST_LOCATION_ALT"), ./internal/acceptance/locations.go: os.Getenv("ARM_TEST_LOCATION_ALT2"), ./.teamcity/components/build_azure.kt: hiddenVariable("env.ARM_TEST_LOCATION", locationsForEnv.primary, "The Primary region which should be used for testing") ./.teamcity/components/build_azure.kt: hiddenVariable("env.ARM_TEST_LOCATION_ALT", locationsForEnv.secondary, "The Secondary region which should be used for testing") ./.teamcity/components/build_azure.kt: hiddenVariable("env.ARM_TEST_LOCATION_ALT2", locationsForEnv.tertiary, "The Tertiary region which should be used for testing") terraform-provider-azurerm on  HEAD (f54480d) via 🐹 v1.23.3 💤 grep -r "ARM_LOCATION" . # Nothing

Could you please double confirm ARM_LOCATION is now used in preference to ARM_TEST_LOCATION?

I understand that's the way it is in the provider but it's currently ARM_LOCATION in the backend so let's leave it as ARM_LOCATION for consistency with how we're currently testing the backend. We can revisit it when pluggable backends are a thing and the backend has been moved out of the terraform repo

Assuming this env var is used somewhere else:

terraform on  backend/azure/update-to-latest-sdks via 🐹 v1.23.3 💤 grep -r "ARM_LOCATION" . # Nothing

I've made the change anyway.

mbfrahry · 2025-01-09T02:00:36Z

internal/backend/remote-state/azure/client.go

+
+// newCtx creates a context with a (meaningless) deadline.
+// This is only to make the go-azure-sdk/sdk/client Client happy.
+func newCtx() context.Context {


I don't fully understand this change. If I swap this change back to context.TODO() the backend still seems to work. Can you elaborate a bit more on what issues you are seeing with this

You'll get an error like below:

2025/01/20 13:22:18 Creating Resource Group "acctestRG-backend-25012013221826-9rd0jbe38q" 2025/01/20 13:22:18 [DEBUG] Deleting Resource Group "acctestRG-backend-25012013221826-9rd0jbe38q".. client_test.go:28: Error creating Test Resources: "failed to create test resource group: the context used must have a deadline attached for polling purposes, but got no deadline"

This is derived from, e.g. go-azure-sdk/sdk/client/resourcemanager/client.go:

func (c *Client) NewRequest(ctx context.Context, input client.RequestOptions) (*client.Request, error) { // TODO move these validations to base client method if _, ok := ctx.Deadline(); !ok { return nil, fmt.Errorf("the context used must have a deadline attached for polling purposes, but got no deadline") }

Great! Thanks for the added info

mbfrahry · 2025-01-09T16:42:28Z

internal/backend/remote-state/azure/backend.go

+			"endpoint": {
+				Type:       schema.TypeString,
+				Optional:   true,
+				Deprecated: "`endpoint` is deprecated and no longer used, it will be removed in a future version of Terraform",


Should this be deprecated?

This is marked as deprecated when I took over the PR from Tom B. I think the main reason is that this field is not used in the azurerm provider (probably is superseded by the msi_endpoint).

If we're going to use msi_endpoint over endpoint we should call that out in the deprecation message. Just saying something is deprecated without a clear alternative isn't a good user experience

Updated the comment, while I guess they are not 100% equivalent..

mbfrahry · 2025-01-14T15:45:48Z

internal/backend/remote-state/azure/client.go

 			return nil, nil
 		}
 		return nil, err
 	}

+	if blob.Contents == nil {
+		return nil, nil


Is this the expected behavior here or should we return an error?

This one is also taken from the branch of Tom. I think the sanity here is that the Client.Get() accept there is no remote state:

terraform/internal/states/remote/state.go

Lines 138 to 149 in ba8d180

payload, err := s.Client.Get()

if err != nil {

return err

}

// no remote state is OK

if payload == nil {

s.readState = nil

s.lineage = ""

s.serial = 0

return nil

}

.

The other implementation of the State do the same thing, e.g.:

terraform/internal/backend/remote-state/consul/client.go

Lines 80 to 87 in ba8d180

chunked, hash, chunks, pair, err := c.chunkedMode()

if err != nil {

return nil, err

}

if pair == nil {

return nil, nil

}

terraform/internal/backend/remote-state/cos/client.go

Lines 47 to 54 in ba8d180

exists, data, checksum, err := c.getObject(c.stateFile)

if err != nil {

return nil, err

}

if !exists {

return nil, nil

}

Great! Thanks for the added info

…e-to-latest-sdks

mbfrahry

Hey @magodo, thanks for getting those changes in. I've left another review where the bulk of it is just trying to get some consistency with the error messages and a couple more design questions.

mbfrahry · 2025-01-28T16:48:29Z

internal/backend/remote-state/azure/api_client.go

+	client.configureClient(client.storageAccountsClient.Client, resourceManagerAuth)
+
+	// Populating the storage account detail
+	said := commonids.NewStorageAccountID(config.SubscriptionID, config.ResourceGroupName, client.storageAccountName)


said is a word. Could we please make this storageAccountId or something similar

mbfrahry · 2025-01-28T16:49:11Z

internal/backend/remote-state/azure/api_client.go

+	said := commonids.NewStorageAccountID(config.SubscriptionID, config.ResourceGroupName, client.storageAccountName)
+	resp, err := client.storageAccountsClient.GetProperties(ctx, said, storageaccounts.DefaultGetPropertiesOperationOptions())
+	if err != nil {
+		return nil, fmt.Errorf("getting %s: %+v", said, err)


Suggested change

return nil, fmt.Errorf("getting %s: %+v", said, err)

return nil, fmt.Errorf("retrieving %s: %+v", said, err)

mbfrahry · 2025-01-28T16:49:54Z

internal/backend/remote-state/azure/api_client.go

+		return nil, fmt.Errorf("getting %s: %+v", said, err)
+	}
+	if resp.Model == nil {
+		return nil, fmt.Errorf("unexpected null model of %s", said)


Suggested change

return nil, fmt.Errorf("unexpected null model of %s", said)

return nil, fmt.Errorf("retrieving %s: model was nil", said)

mbfrahry · 2025-01-28T16:50:50Z

internal/backend/remote-state/azure/api_client.go

+	log.Printf("[DEBUG] Building the Blob Client from an Access Token (using user credentials)")
+	key, err := c.accountDetail.AccountKey(ctx, c.storageAccountsClient)
+	if err != nil {
+		return nil, fmt.Errorf("Error retrieving key for Storage Account %q: %s", c.storageAccountName, err)


Suggested change

return nil, fmt.Errorf("Error retrieving key for Storage Account %q: %s", c.storageAccountName, err)

return nil, fmt.Errorf("retrieving key for Storage Account %q: %s", c.storageAccountName, err)

mbfrahry · 2025-01-28T16:51:34Z

internal/backend/remote-state/azure/api_client.go

+	if err != nil {
+		return nil, fmt.Errorf("Error retrieving key for Storage Account %q: %s", c.storageAccountName, err)
+	}
+	accessKey := *key


Does this need to be pulled out this way? I only see it used in one place so we could just point to it there

mbfrahry · 2025-01-28T17:10:11Z

internal/backend/remote-state/azure/helpers_test.go

 	if err != nil {
+		return fmt.Errorf("getting %s: %+v", said, err)


Suggested change

return fmt.Errorf("getting %s: %+v", said, err)

return fmt.Errorf("retrieving %s: %+v", said, err)

mbfrahry · 2025-01-28T17:14:47Z

internal/backend/remote-state/azure/client.go

@@ -281,3 +295,7 @@ func (c *RemoteClient) Unlock(id string) error {

 	return nil
 }
+
+func responseWasNotFound(resp *http.Response) bool {


We have a helper for this in go-azure-helpers that we can use here. So we can just nil check it and then use the helper rather than define a new helper here

mbfrahry · 2025-01-28T21:16:52Z

internal/backend/remote-state/azure/helpers_test.go

+		t.Fatalf("Missing ARM_TENANT_ID")
+	}
+
+	location := os.Getenv("ARM_TEST_LOCATION")


I understand that's the way it is in the provider but it's currently ARM_LOCATION in the backend so let's leave it as ARM_LOCATION for consistency with how we're currently testing the backend. We can revisit it when pluggable backends are a thing and the backend has been moved out of the terraform repo

mbfrahry · 2025-01-28T21:17:03Z

internal/backend/remote-state/azure/client.go

+
+// newCtx creates a context with a (meaningless) deadline.
+// This is only to make the go-azure-sdk/sdk/client Client happy.
+func newCtx() context.Context {


Great! Thanks for the added info

mbfrahry · 2025-01-28T21:17:22Z

internal/backend/remote-state/azure/client.go

 			return nil, nil
 		}
 		return nil, err
 	}

+	if blob.Contents == nil {
+		return nil, nil


Great! Thanks for the added info

…e-to-latest-sdks

magodo · 2025-01-29T06:12:47Z

@mbfrahry Thanks for the new round of review! I've updated the code according to all the feedbacks you've provided.

Test passed:

backend/remote-state/azure on εéá backend/azure/update-to-latest-sdks via Go v1.23.3
$ TF_ACC=1 go test -timeout=20h -parallel=20 ./...
ok      github.com/hashicorp/terraform/internal/backend/remote-state/azure      264.298s

mbfrahry

Thanks for taking care of all the review comments @Magado. These changes look good and looks like we're about ready to merge once we get the all the github actions passing!

magodo · 2025-01-30T00:28:08Z

@mbfrahry CI passed now.

mbfrahry

Hey @magodo, it's nearly there but we should make that changelog entry a little more descriptive

mbfrahry · 2025-01-30T20:46:41Z

.changes/unreleased/ENHANCEMENTS-20250130-105700.yaml

@@ -0,0 +1,5 @@
+kind: ENHANCEMENTS
+body: Update the `azure` backend authentication mechanisms


We should make this more descriptive with some of the changes coming into this PR like the new fields and any bug fixes that might be fixed through these changes

trujillo-adam

I reviewed the docs and suggested some changes to improve the grammar and consistency with our style guidelines. I am uncertain about the accuracy of the suggestions because the existing description are vague. Please make any necessary corrections following the sentence patterns I added.

trujillo-adam · 2025-01-31T00:14:46Z