Skip to content

Backend/azure/update to latest sdks#36258

Merged
SarahFrench merged 31 commits intohashicorp:mainfrom
magodo:backend/azure/update-to-latest-sdks
Feb 4, 2025
Merged

Backend/azure/update to latest sdks#36258
SarahFrench merged 31 commits intohashicorp:mainfrom
magodo:backend/azure/update-to-latest-sdks

Conversation

@magodo
Copy link
Copy Markdown
Contributor

@magodo magodo commented Jan 3, 2025

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

@magodo magodo requested review from a team as code owners January 3, 2025 07:06
@magodo magodo requested a review from mikegolus January 3, 2025 07:06
@magodo
Copy link
Copy Markdown
Contributor Author

magodo commented Jan 3, 2025

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

@crw
Copy link
Copy Markdown
Contributor

crw commented Jan 8, 2025

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

@magodo
Copy link
Copy Markdown
Contributor Author

magodo commented Jan 29, 2025

@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

Copy link
Copy Markdown
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 magodo requested a review from a team as a code owner January 29, 2025 23:52
@magodo
Copy link
Copy Markdown
Contributor Author

magodo commented Jan 30, 2025

@mbfrahry CI passed now.

Copy link
Copy Markdown
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Copy Markdown
Contributor

@trujillo-adam trujillo-adam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* `endpoint` - (Optional) The Custom Endpoint for Azure Resource Manager. This can also be sourced from the `ARM_ENDPOINT` environment variable.

~> **NOTE:** An `endpoint` should only be configured when using Azure Stack.
* `environment` - (Optional) The Azure Environment which should be used. This can also be sourced from the `ARM_ENVIRONMENT` environment variable. Possible values are `public`, `china` and `usgovernment`. Defaults to `public`.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `environment` - (Optional) The Azure Environment which should be used. This can also be sourced from the `ARM_ENVIRONMENT` environment variable. Possible values are `public`, `china` and `usgovernment`. Defaults to `public`.
- `environment`: (Optional) Specifies the Azure environment to use. Specify one of the following values:
- `public`,
- `china`
- `usgovernment`.
The default is `public`.
You can set the `ARM_ENVIRONMENT` environment variable to configure this option.

Addresses grammar issues and writing style violations.


* `snapshot` - (Optional) Should the Blob used to store the Terraform Statefile be snapshotted before use? Defaults to `false`. This value can also be sourced from the `ARM_SNAPSHOT` environment variable.

* `use_cli` - (Optional) Should Azure CLI be used for authentication? Defaults to `false`. This value can also be sourced from the `ARM_USE_CLI` environment variable.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `use_cli` - (Optional) Should Azure CLI be used for authentication? Defaults to `false`. This value can also be sourced from the `ARM_USE_CLI` environment variable.
- `use_cli`: (Optional) Enables Terraform to use the Azure CLI for authentication. The default is `false`. You can also set the `ARM_USE_CLI` environment variable to configure this option.

Not sure why some of these are phrased as questions, but we should directly and concretely state what the option does.

Copy link
Copy Markdown
Contributor Author

@magodo magodo Jan 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question style is what we've been using for the azreurm document all the time, for bools.


* `use_oidc` - (Optional) Should OIDC authentication be used? This can also be sourced from the `ARM_USE_OIDC` environment variable.

* `use_aks_workload_identity` (Optional) Should Azure AKS Workload Identity be used for Authentication? This can also be sourced from the `ARM_USE_AKS_WORKLOAD_IDENTITY` environment variable.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `use_aks_workload_identity` (Optional) Should Azure AKS Workload Identity be used for Authentication? This can also be sourced from the `ARM_USE_AKS_WORKLOAD_IDENTITY` environment variable.
- `use_aks_workload_identity`: (Optional) Enables Terraform to use the Azure AKS workload identity for authentication. You can set the `ARM_USE_AKS_WORKLOAD_IDENTITY` environment variable to configure this option.


* `client_id` - (Optional) The Client ID of the Service Principal. This can also be sourced from the `ARM_CLIENT_ID` environment variable.

* `client_id_file_path` (Optional) The path to a file containing the Client ID which should be used. This can also be sourced from the `ARM_CLIENT_ID_FILE_PATH` Environment Variable.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `client_id_file_path` (Optional) The path to a file containing the Client ID which should be used. This can also be sourced from the `ARM_CLIENT_ID_FILE_PATH` Environment Variable.
- `client_id_file_path`: (Optional) Specifies the path to a file containing the client ID. Terraform presents the client ID when authenticating with Azure. You can set the `ARM_CLIENT_ID_FILE_PATH` environment variable to configure this option.

What does Terraform do with the client ID? Please correct this suggestion as necessary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Terraform presents the client ID when authenticating with Azure


* `client_certificate_path` - (Optional) The path to the PFX file used as the Client Certificate when authenticating as a Service Principal. This can also be sourced from the `ARM_CLIENT_CERTIFICATE_PATH` environment variable.

* `client_certificate` - (Optional) Base64 encoded PKCS#12 certificate bundle to use when authenticating as a Service Principal using a Client Certificate. This can also be sourced from the `ARM_CLIENT_CERTIFICATE` environment variable.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `client_certificate` - (Optional) Base64 encoded PKCS#12 certificate bundle to use when authenticating as a Service Principal using a Client Certificate. This can also be sourced from the `ARM_CLIENT_CERTIFICATE` environment variable.
- `client_certificate`: (Optional) Specifies a base64-encoded PKCS#12 certificate bundle to use for authenticating as a service principal using a client certificate. You can set the `ARM_CLIENT_CERTIFICATE` environment variable to configure this option.


* `client_id` - (Optional) The Client ID of the Service Principal. This can also be sourced from the `ARM_CLIENT_ID` environment variable.

* `client_id_file_path` (Optional) The path to a file containing the Client ID which should be used. This can also be sourced from the `ARM_CLIENT_ID_FILE_PATH` Environment Variable.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `client_id_file_path` (Optional) The path to a file containing the Client ID which should be used. This can also be sourced from the `ARM_CLIENT_ID_FILE_PATH` Environment Variable.
- `client_id_file_path`: (Optional) Specifies the path to a file containing the client ID. Terraform presents the file client ID when authenticating with Azure. This can also be sourced from the `ARM_CLIENT_ID_FILE_PATH` Environment Variable.

Again, not sure if I'm describing the process correctly, but we should use active voice to make this information clear.


* `client_secret` - (Optional) The Client Secret of the Service Principal. This can also be sourced from the `ARM_CLIENT_SECRET` environment variable.

* `client_secret_file_path` - (Optional) The path to a file containing the Client Secret which should be used. This can also be sourced from the `ARM_CLIENT_SECRET_FILE_PATH` Environment Variable.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `client_secret_file_path` - (Optional) The path to a file containing the Client Secret which should be used. This can also be sourced from the `ARM_CLIENT_SECRET_FILE_PATH` Environment Variable.
- `client_secret_file_path`: (Optional) Specifies the path to a file containing the client secret. Terraform must present the client secret to authenticate with Azure. You can set the `ARM_CLIENT_SECRET_FILE_PATH` environment variable to configure this option.

@magodo
Copy link
Copy Markdown
Contributor Author

magodo commented Jan 31, 2025

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.

Thank you for pointing this out! Whilst by scanning the suggested changes, it makes this document a little bit inconsistent itself. I suggest we can create a dedicated PR to rephrase the document here to be consistent with the other docs in this repo. WDYT?

@trujillo-adam
Copy link
Copy Markdown
Contributor

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.

Thank you for pointing this out! Whilst by scanning the suggested changes, it makes this document a little bit inconsistent itself. I suggest we can create a dedicated PR to rephrase the document here to be consistent with the other docs in this repo. WDYT?

I don't mind if the styles are a little inconsistent on this page until we can open a new PR to address the rest of it. We do not expect people to read this type of reference information from top to bottom, so the inconsistencies might not even be that noticeable. And besides, progress over perfection :)

mbfrahry
mbfrahry previously approved these changes Jan 31, 2025
Copy link
Copy Markdown
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for taking the time to get this in @magodo

@SarahFrench SarahFrench added the 1.11-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Feb 1, 2025
Copy link
Copy Markdown
Member

@SarahFrench SarahFrench left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @magodo - could you please move the location of your change file (see comment). This is necessary for automation that will pull this change into the branch for the minor release we're currently preparing, v1.11

Copy link
Copy Markdown
Member

@SarahFrench SarahFrench left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 7, 2025

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

1.11-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged backend/azure enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enhancement Request: azurerm backend authentication upgrade to match provider

5 participants