-
Notifications
You must be signed in to change notification settings - Fork 4.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a cache to store storage account keys. #634
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
azurerm/config.go
Outdated
key, ok = storageKeyCache[cacheIndex] | ||
if !ok { | ||
accountKeys, err := armClient.storageServiceClient.ListKeys(resourceGroupName, storageAccountName) | ||
if accountKeys.StatusCode == http.StatusNotFound { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it'd be good to update this to use utils.ResponseWasNotFound(accountKeys)
- since that'll also handle connection issues like drops/resets
azurerm/config.go
Outdated
|
||
var ( | ||
storageKeyCacheMu sync.RWMutex | ||
storageKeyCache = make(map[rgsaTuple]string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
azurerm/config.go
Outdated
return "", false, fmt.Errorf("Nil key returned for storage account %q", storageAccountName) | ||
} | ||
|
||
key = *(*accountKeys.Keys)[0].Value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's two possible crashes here:
accountKeys.Keys
could be an empty list, so accessing this would crash(*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)
@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. |
@metacpp, yes, it is already added in the original code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
azurerm/config.go
Outdated
} | ||
|
||
cacheIndex := "" | ||
if enableKeyCache { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Let me remove it for now.
Fixes #202 |
azurerm/config.go
Outdated
} | ||
|
||
keys := *accountKeys.Keys | ||
if accountKeys.Keys == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @JunyiYi
Thanks for pushing the latest updates - I've taken a look through and this LGTM.
Thanks!
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! |
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 40listKeys
calls.This will also resolve the Azure throttling issue (#202) with ~100 storage accounts and ~1000 storage resources in total.