auth: support caching of values retrieves from Azure CLI to avoid unnecessary repeated invocations

[manicminer]

This implements caching of results from Azure CLI to avoid unnecessary repeated forks of az. The first time a particular az command invocation is made, if the cacheable argument to azurecli.JSONUnmarshalAzCmd() is true, the resulting bytes will be cached and can be reused later on for unmarshalling (either the same, or additional data fields). On subsequent function calls using the same az arguments (e.g. from additional authorizers being set up), the result is retrieved from the cache instead, yielding much-improved performance.

Before

After

The initial implementation of this happened within the authorizer, rather than at the lower-level azurecli internal helper package. However, with the latter, the caching is more robust as the command result is cached rather than the parsed data, so it can be re-used for unmarshalling other data.

Resolves: #752
Replaces: #707
Related: hashicorp/terraform-provider-azurerm#23977
Downstream PR: hashicorp/terraform-provider-azurerm#24056

[tombuildsstuff]

I've taken a look through and left some comments inline, but on the whole this change looks good, however I think we probably should look to:

1. Remove the CachingEnabled toggle - is there a use-case where this wouldn't be beneficial? In the event there's an issue that arises from this, we can do another release of hashicorp/go-azure-sdk (which is more work, but we're going to want this cache enabled everywhere?)
2. Change how the cache is built, so that we're building this only when the object is nil (which achieves this one-time load without the enabled flag)
3. Centralise the Azure CLI checks, as we're obtaining details about the Azure CLI in newAzureCliConfig - meaning we don't need to re-check this in Token and AuxiliaryTokens

WDYT?

[tombuildsstuff]

I think we only need to check the Azure CLI version once (in newAzureCliConfig) - rather than at each usage? If newAzureCliConfig returns an error (because the version is outdated) we'd return an error from this method - so we wouldn't have an AzureCliAuthorizer instance?

[tombuildsstuff]

Perhaps we should move this into azurecli so that the only place this is checked is in CheckAzVersion?

[tombuildsstuff]

rather than doing this in both Token and AuxiliaryTokens, as we're already doing this in newAzureCliConfig we can remove this check?

Suggested change

	var azVersion *string
	if a.conf.CacheEnabled && cliCache.AzVersion != nil {
	azVersion = cliCache.AzVersion
	} else {
	if azVersion, err = azurecli.GetAzVersion(); err != nil {
	return nil, fmt.Errorf("%s. Please ensure you have installed Azure CLI version %s or newer", err, AzureCliMinimumVersion)
	}
	}
	if err = azurecli.CheckAzVersion(*azVersion, azurecli.MsalVersion, nil); err != nil {
	return nil, fmt.Errorf("checking the version of the Azure CLI: %+v", err)
	}

[tombuildsstuff]

(as above)

Suggested change

	var azVersion *string
	if a.conf.CacheEnabled && cliCache.AzVersion != nil {
	azVersion = cliCache.AzVersion
	} else {
	if azVersion, err = azurecli.GetAzVersion(); err != nil {
	return nil, fmt.Errorf("%s. Please ensure you have installed Azure CLI version %s or newer", err, AzureCliMinimumVersion)
	}
	}
	if err = azurecli.CheckAzVersion(*azVersion, azurecli.MsalVersion, nil); err != nil {
	return nil, fmt.Errorf("checking the version of the Azure CLI: %+v", err)
	}

[tombuildsstuff]

(for below)

Suggested change

var err error

[tombuildsstuff]

Is there a benefit to making caching configurable? This feels like something we'd always want on?

Suggested change

	// EnableCaching permits module-scoped caching of the parsed results of `az` command invocations, and can be set to improve
	// performance when instantiating multiple authorizers, at the cost of detectability during long-running applications.
	EnableCaching bool

[manicminer]

Sounds good to me, I've refactored accordingly 👍

[tombuildsstuff]

If we make cliCache a pointer, we should be able to populate the entire object one-time, which'd achieve the same thing albeit a little simpler?

Suggested change

	var azVersion *string
	if cacheEnabled && cliCache.AzVersion != nil {
	azVersion = cliCache.AzVersion
	} else {
	azVersion, err = azurecli.GetAzVersion()
	if err != nil {
	return nil, fmt.Errorf("%s. Please ensure you have installed Azure CLI version %s or newer", err, AzureCliMinimumVersion)
	}
	if cacheEnabled {
	cliCache.AzVersion = azVersion
	}
	}
	nextMajor := AzureCliNextMajorVersion
	if err = azurecli.CheckAzVersion(*azVersion, azurecli.MsalVersion, &nextMajor); err != nil {
	return nil, err
	}
	if cliCache == nil {
	var err error
	azVersion, err = azurecli.GetAzVersion()
	if err != nil {
	return nil, fmt.Errorf("%s. Please ensure you have installed Azure CLI version %s or newer", err, AzureCliMinimumVersion)
	}
	var defaultTenantId *string
	if defaultTenantId, err = azurecli.CheckTenantID(tenantId); err != nil {
	return nil, err
	}
	if defaultTenantId == nil {
	return nil, errors.New("invalid tenantId or unable to determine tenantId")
	}
	var defaultSubscriptionId *string
	if defaultSubscriptionId, err = azurecli.GetDefaultSubscriptionID(); err != nil {
	return nil, err
	}
	cliCache = &cliCache{
	AzVersion: azVersion,
	DefaultSubscriptionId: defaultSubscriptionId,
	DefaultTenantId: defaultTenantId,
	}
	}
	nextMajor := AzureCliNextMajorVersion
	if err = azurecli.CheckAzVersion(*azVersion, azurecli.MsalVersion, &nextMajor); err != nil {
	return nil, err
	}

[tombuildsstuff]

Which'd mean we could remove this?

Suggested change

	// AzureCLIUseCache specifies whether caching should be enabled for idempotent operation results
	AzureCLIUseCache bool

[manicminer]

@tombuildsstuff Thanks for the feedback. I think it's probably fine to remove the toggle and just assume caching for whitelisted az commands.

I've refactored this so the caching now happens within the azurecli helper package. Instead of explicitly caching specific primitives, the cache is now automatically indexed by the az command arguments and populated with the resulting bytes. This improves the efficiency (number of invocations is now down to 4), as now the same command can be re-used - as is the same for obtaining a default tenant ID and subscription ID - and achieves a cache hit every time. It also reduces complexity in the auth package, or anywhere else we might interface with az-cli. WDYT?

[tombuildsstuff]

Totally missed this was sitting for a re-review, sorry 👀

[tombuildsstuff]

LGTM 👍

[tombuildsstuff]

nit / not a blocker but since GetDefaultSubscriptionID exists perhaps this method wants splitting into GetDefaultTenantID and CheckTenantID?

[manicminer]

done :)

[orgads]

Thank you! Much better now.