Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add a cache to store storage account keys. #634

Merged
merged 5 commits into from
Jan 5, 2018
Merged

Add a cache to store storage account keys. #634

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b6e626e
Add a cache to store storage account keys.
Dec 14, 2017
860cebe
Adjust the code according to the comments.
Dec 19, 2017
1df8ee2
Add feature flag for storage account keys cache (enabled by default).
Dec 19, 2017
d8a3c46
Remove feature flags for now
Jan 4, 2018
38ea143
Checking `keys` isn't nil before accessing it
tombuildsstuff Jan 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 38 additions & 12 deletions azurerm/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"net/http"
"net/http/httputil"
"os"
"sync"

"github.com/Azure/azure-sdk-for-go/arm/appinsights"
"github.com/Azure/azure-sdk-for-go/arm/authorization"
Expand Down Expand Up @@ -838,23 +839,48 @@ func (c *ArmClient) registerResourcesClients(endpoint, subscriptionId string, au
c.managementLocksClient = locksClient
}

type rgsaTuple struct {
resourceGroup, storageAccount string
}

var (
storageKeyCacheMu sync.RWMutex
storageKeyCache = make(map[rgsaTuple]string)
Copy link
Contributor

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?

)

func (armClient *ArmClient) getKeyForStorageAccount(resourceGroupName, storageAccountName string) (string, bool, error) {
accountKeys, err := armClient.storageServiceClient.ListKeys(resourceGroupName, storageAccountName)
if accountKeys.StatusCode == http.StatusNotFound {
return "", false, nil
}
if err != nil {
// We assume this is a transient error rather than a 404 (which is caught above), so assume the
// account still exists.
return "", true, fmt.Errorf("Error retrieving keys for storage account %q: %s", storageAccountName, err)
cacheIndex := rgsaTuple{resourceGroupName, storageAccountName}
storageKeyCacheMu.RLock()
key, ok := storageKeyCache[cacheIndex]
storageKeyCacheMu.RUnlock()

if ok {
return key, true, nil
}

if accountKeys.Keys == nil {
return "", false, fmt.Errorf("Nil key returned for storage account %q", storageAccountName)
storageKeyCacheMu.Lock()
defer storageKeyCacheMu.Unlock()
key, ok = storageKeyCache[cacheIndex]
if !ok {
accountKeys, err := armClient.storageServiceClient.ListKeys(resourceGroupName, storageAccountName)
if accountKeys.StatusCode == http.StatusNotFound {
Copy link
Contributor

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

return "", false, nil
}
if err != nil {
// We assume this is a transient error rather than a 404 (which is caught above), so assume the
// account still exists.
return "", true, fmt.Errorf("Error retrieving keys for storage account %q: %s", storageAccountName, err)
}

if accountKeys.Keys == nil {
Copy link
Contributor

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)

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

key = *(*accountKeys.Keys)[0].Value
Copy link
Contributor

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:

  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)

storageKeyCache[cacheIndex] = key
}

keys := *accountKeys.Keys
return *keys[0].Value, true, nil
return key, true, nil
}

func (armClient *ArmClient) getBlobStorageClientForStorageAccount(resourceGroupName, storageAccountName string) (*mainStorage.BlobStorageClient, bool, error) {
Expand Down