Add a cache to store storage account keys.

[JunyiYi]

Each time we refresh/create a Queue/Table/Blob, a new client is retrieved through listKeys call to a specific storage account, even they are within one storage account. That is not necessary, the keys should not be changed during one Terraform session.

Creating two storage accounts with four resources each (one queue, one blob, one table and one container) only takes 8 listKeys calls instead of the original 40 listKeys calls.

This will also resolve the Azure throttling issue (#202) with ~100 storage accounts and ~1000 storage resources in total.

[tombuildsstuff]

Hey @JunyiYi

Thanks for this PR - I've taken a look through and left some comments in-line, but this is off to a good start.

It's worth noting that this PR assumes that the Storage Account Access Keys aren't changed during a Terraform run (and will cause errors within the Data Sources/Resources if they have). I can't think it'd impact too many users since frequent Access Key rotation is challenging (given there's currently only 2 keys available for Storage Accounts) - as such I think we should be able to work around this through a feature-flag if it does cause issues - and that this is probably a safe assumption.

Thanks!

[tombuildsstuff]

it'd be good to update this to use utils.ResponseWasNotFound(accountKeys) - since that'll also handle connection issues like drops/resets

[tombuildsstuff]

given these are both alphanumeric strings - I think we should be able to concatenate the two strings with a / divider and remove the need for this struct?

[tombuildsstuff]

There's two possible crashes here:

1. accountKeys.Keys could be an empty list, so accessing this would crash
2. (*accountKeys.Keys[0]).Value could be nil

As this is - it'd be hard to debug this from a stack trace at the moment - as such can we update this to make this clearer via:

if keys := accountKeys.Keys; keys != nil {
  if len(keys) > 0 {
    key := *keys[0].Value
    storageKeyCache[cacheIndex] = key
    return key, true, nil
  }

  return "", false, fmt.Errorf("No keys returned for storage account %q", storageAccountName)
}

return "", false, fmt.Errorf("Nil key returned for storage account %q", storageAccountName)

[metacpp]

@JunyiYi As we talked yesterday, you should add the fallback logic to handle the case that keys are not valid one(cache miss).

If we store the key in some variable, no matter it's shared by one resource or all resources, there's always possibility that keys are changed during processing period, it just depends how fast the keys are rotated(1 sec, 1 min, 1 hour, 1 day...) and how long the terraform runs each time. Adding cache-miss handling logic will benefit the performance while keeping the same safety.

[JunyiYi]

@metacpp, yes, it is already added in the original code.

[tombuildsstuff]

Hey @JunyiYi

Thanks for pushing those updates - I've taken another look through and left a comment in-line. With regard to the feature toggle we can remove this for the moment; I meant that we can add this in if we need it later - I don't believe it's needed at this point. Otherwise this LGTM (I'm assuming the Storage Account tests still pass, but I can run them once those updates have been pushed 😄)

Thanks!

[tombuildsstuff]

I think we can remove this feature flag for the moment - in this comment I was alluding that we can add a feature flag in if any users are affected (I'm doubtful it should, if I'm honest - given key rotation within Storage Accounts is challenging as there's only a Primary/Secondary key available) - and we'll add it back in if users have issues?

[JunyiYi]

Sure. Let me remove it for now.

[tombuildsstuff]

Fixes #202

[tombuildsstuff]

going to push a commit to check for accountKeys.Keys being nil before accessing it, which is done in the line above (otherwise they'll be a crash)

[tombuildsstuff]

hey @JunyiYi

Thanks for pushing the latest updates - I've taken a look through and this LGTM.

Thanks!

[tombuildsstuff]

Tests pass:

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!