Backend/azure: subscription_id infer from Azure CLI & a way to skip *unnecessary* management plane API call
#36623
Conversation
There are four auth scenarios of the blob/container: 1. User specifies the SAS token 2. User specifies the shared access key 3. User specified to use AAD auth (and credential provided) 4. None of the above, management plane API call needed to list the shared access key and use it to auth For 1, 2 and 3, the management plane API can be skipped in most of the cases, except the target storage account is using private DNS zone. The blob/container data plane client requires the user to specify the base URI, which is not deterministic if the storage account opt in the private DNS zone. In this case, an additional management plane `GET` is required against the storage account, to retrieve the blob endpoint. While if private DNS zone is not used, the base URI can be composed in a fixed pattern, with the storage account name and container&blob name. Hence no management plane API call is needed. The user is expected to use the `subscription_id` and `resource_group_name` to indicate the above intent: Only if both are specified, the additional `GET` call will be invoked to get the accurate blob endpoint. (NOTE: the `subscription_id` can be inferred from the Azure CLI if unspecified)
Changelog WarningCurrently this PR would target a v1.12 release. Please add a changelog entry for in the .changes/v1.12 folder, or discuss which release you'd like to target with your reviewer. If you believe this change does not need a changelog entry, please add the 'no-changelog-needed' label. |
|
@magodo For clarity, where you mention Further, this is still a breaking change for most users, since up until now supplying An alternative solution here could be to have an explicit setting to look up the endpoint for those customers using the preview feature. Something like I'm only suggesting this to avoid impacting existing users when they move to 1.11. I feel that the smoother experience for them would be beneficial and the docs can help anyone using the preview feature. I can help with the docs if you like, as I made the last major update to them. UPDATE: To be clear, rightly or wrongly, the docs currently indicate that
As such, the majority of people would have included that in their backend config (even if it isn't really required). The breaking change here would be that including If we can revert to the previous behaviour (where I am guessing it just ignores |
|
The starting point for this PR should be the behaviour as per 1.10 i.e. Neither subscription id nor resource group are mandatory. Managed Identity Principal mode ( terraform {
backend "azurerm" {
storage_account_name = "acctesttfstate"
container_name = "tfstate"
key = "prod.terraform.tfstate"
use_azuread_auth = true
use_msi = true
}
}OIDC auth can work in the same way ( We need a |
You are right, sorry for the wrong wording... I've updated the description of this PR.
Unfortunately, the backend document seems not correct in some places. E.g. even When authenticating using a Managed Identity (MSI), the Regarding the I'll try to summarize all the scenarios and test them out. A basic illustration about what's going on:
(Again, the complexity comes from the fact that mixing AutN for different levels together) |
I understand the docs are incorrect, but if we move forward with this it will impact a lot of people. I appreciate they will just need to remove Could be we add a deprecation warning in 1.11.x and then make the breaking change in 1.12.x? My suggestion is:
Further suggestions: I appreciate it is probably too late for this, but I am personally against the inclusion or exclusion of certain parameters to determine auth methods. I think it is confusing for end users and is not transparent. This is proven by the fact that the peer reviewed docs are incorrect, even the people writing it don't know how it works... It would be much better to explicitly specify the auth method they want to use and there is precedent for that already with In the future I think we should consider adding an |
|
@jaredfholgate I agree that nclusion or exclusion of certain parameters to determine different behavior is not a good choice! I'll update the code and flow to be as what it was before, especially for the SA endpoint lookup part, and silently ignore those redundent properties if users have specified. Meanwhile, I'll add a new attribute to allow users to opt in the endpoint lookup. |
|
A new property is introduced: Meanwhile, I've performed the following test.
Note, when either Storage Auth Method is unspecified, or Lookup EP is set, the Lastly, I've only used Azure CLI for the AAD auth method here, as the other AAD auth method shall behave the same, except the Update I've also verified with client ID + client secret for the AAD auth. |
| var err error | ||
| client.azureAdStorageAuth, err = auth.NewAuthorizerFromCredentials(ctx, *config.AuthConfig, config.AuthConfig.Environment.Storage) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("unable to build authorizer for Storage API: %+v", err) | ||
| } | ||
| default: | ||
| // ARM authN is required only when no auth method is specified, which falls back to listing the access key via ARM API. |
There was a problem hiding this comment.
Can you expand what authN is? I believe it's ARM Authentication but I just want it to be clear in the comments for when we come back to this
There was a problem hiding this comment.
Done. AuthN: Authentication, AuthZ: Authorization.
| resourceManagerAuth, err := auth.NewAuthorizerFromCredentials(ctx, *config.AuthConfig, config.AuthConfig.Environment.ResourceManager) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("unable to build authorizer for Resource Manager API: %+v", err) | ||
| // Besides, ARM authN is required to lookup the blob endpoint. |
There was a problem hiding this comment.
| // Besides, ARM authN is required to lookup the blob endpoint. | |
| // if `config.LookupBlobEndpoint` is true, we need to authenticate with ARM to lookup the blob endpoint |
| if err != nil { | ||
| return nil, fmt.Errorf("new shared key authorizer: %v", err) | ||
| default: | ||
| // Neither shared access key nor sas token specified, not using AAD auth either. Calling management plane API to get the key. |
There was a problem hiding this comment.
| // Neither shared access key nor sas token specified, not using AAD auth either. Calling management plane API to get the key. | |
| // Neither shared access key, sas token, or AAD Auth were specified so we have to call the management plane API to get the key. |
| if err != nil { | ||
| return nil, fmt.Errorf("new shared key authorizer: %v", err) | ||
| default: | ||
| // Neither shared access key nor sas token specified, not using AAD auth either. Calling management plane API to get the key. |
There was a problem hiding this comment.
| // Neither shared access key nor sas token specified, not using AAD auth either. Calling management plane API to get the key. | |
| // Neither shared access key, sas token, or AAD Auth were specified so we have to call the management plane API to get the key. |
| @@ -49,6 +49,13 @@ func New() backend.Backend { | |||
| Description: "The blob key to use in the Storage Container.", | |||
| }, | |||
|
|
|||
| "lookup_blob_endpoint": { | |||
There was a problem hiding this comment.
Could we add a test for this?
I do see that other PR around docs but I think we should merge that PR into this one so we don't accidentally have conflicting information if they don't get merged at the same time
| Type: schema.TypeBool, | ||
| Optional: true, | ||
| DefaultFunc: schema.EnvDefaultFunc("ARM_USE_DNS_ZONE_ENDPOINT", false), | ||
| Description: "Whether to look up the storage account blob endpoint, instead of composing the endpoint in a fixed pattern. This is necessary when the storage account uses the Azure DNS zone endpoint.", |
There was a problem hiding this comment.
I'm unclear on what this line means instead of composing the endpoint in a fixed pattern. I'd recommend expanding on what that means more or just removing it
| Description: "Whether to look up the storage account blob endpoint, instead of composing the endpoint in a fixed pattern. This is necessary when the storage account uses the Azure DNS zone endpoint.", | |
| Description: "Whether to look up the storage account blob endpoint. This is necessary when the storage account uses the Azure DNS zone endpoint.", |
There was a problem hiding this comment.
Removed. The endpoint is always needed, if not looking it up, we'll construct it using the fixed pattern: ``https://.blob.core.windows.net, which ends up with URL https://.blob.core.windows.net//` for a blob.
|
@mbfrahry I've added the test and passed it: $ TF_ACC=1 go test -v -timeout=20h -parallel=20 -run='TestAccBackendAzureADAuthBasicWithBlobEndpointLookup'
=== RUN TestAccBackendAzureADAuthBasicWithBlobEndpointLookup
=== PAUSE TestAccBackendAzureADAuthBasicWithBlobEndpointLookup
=== CONT TestAccBackendAzureADAuthBasicWithBlobEndpointLookup
...
--- PASS: TestAccBackendAzureADAuthBasicWithBlobEndpointLookup (166.12s)
PASS
ok github.com/hashicorp/terraform/internal/backend/remote-state/azure 166.126s |
This PR consists of two changes:
Infer
subscription_idfrom Azure CLI if not specified. Thesubscription_idis required sinceterraform-provider-azurermv4 (see: provider: makesubscription_ida required provider property in v4.0 terraform-provider-azurerm#26621). My last PR Backend/azure/update to latest sdks #36258 aligns this with the azurerm provider, whilst causing breaking changes unfortunately.The commit 0c57798 chooses to fallback to the Azure CLI default subscription, only if the user is using CLI to authenticate.
If the maintainer accepts breaking change and is fine to align with the azurerm behavior, I'll revert the above commit and mark
subscription_idas required.Avoid management plane call if not necessary.
There are four auth scenarios of the blob/container:
For 1, 2 and 3, the management plane API can be skipped in most of the cases, except the target storage account is using
privateDNS zone endpoint. The blob/container data plane client requires the user to specify the base URI, which is not deterministic if the storage account opt in theprivateDNS zone endpoint. In this case, an additional management planeGETis required against the storage account, to retrieve the blob endpoint. While ifprivateDNS zone endpoint is not used, the base URI can be composed in a fixed pattern, with the storage account name and container&blob name. Hence no management plane API call is needed.The user is expected to use the
subscription_idandresource_group_nameto indicate the above intent: Only if both are specified, the additionalGETcall will be invoked to get the accurate blob endpoint.(NOTE: the
subscription_idcan be inferred from the Azure CLI if unspecified)Fixes #36596
Fixes #36595
Target Release
1.11.1
Test
Unit Test
$ TF_ACC=1 go test -timeout=20h -parallel=20 ./... PASS ok github.com/hashicorp/terraform/internal/backend/remote-state/azure 217.074sScenario Test
The following test is done locally, with an empty terraform state stored in a storage blob. The goal is to test that the management plane APIs are called as expected.
SAS auth without
resource_group_namespecifiedExpect no management plane API.
SAS auth with
resource_group_namespecifiedExpect management plane API.
Shared access key auth without
resource_group_namespecifiedExpect no management plane API.
Shared access key auth with
resource_group_namespecifiedExpect management plane API.
AAD auth with CLI without
resource_group_namespecifiedExpect no management plane API.
AAD auth with CLI with
resource_group_namespecifiedExpect management plane API.
No auth method specified, defaults to listing shared access key and use it
Expect management plane API call
CHANGELOG entry