-
Notifications
You must be signed in to change notification settings - Fork 50
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
Fix racing condition using leasing on blob #23
Conversation
acb9eae
to
f81c39b
Compare
fmt.Println("Key not found. Creating a new key...") | ||
storageAccountsClient := storagemgmt.NewAccountsClient(subscriptionID) |
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.
We really don't subscription id here. You can get a token for resource by storage account name, and env (cloud name) only. This way we ask the user for less
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.
Currently we are already using subscriptionID
to get instance of keyvault to test for its existence. This is being used the same way to test for existence of the storage account.
vaultsClient := kvmgmt.NewVaultsClient(subscriptionID)
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.
Also doesnt seem like the Azure Go SDK has the AD authentication feature added for storage yet: https://github.com/Azure/azure-sdk-for-go/blob/v18.0.0/storage/client.go#L269-L276
func NewBasicClient(accountName, accountKey string) (Client, error) {
if accountName == StorageEmulatorAccountName {
return NewEmulatorClient()
}
return NewClient(accountName, accountKey, DefaultBaseURL, DefaultAPIVersion, defaultUseHTTPS)
}
return nil, err | ||
} | ||
storageAcctName := providerVaultName | ||
res, err := storageAccountsClient.ListKeys(*resourceGroup, storageAcctName) |
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.
You also don't need that.
Shouldn't the configuration file carry the storage account name used for leases? |
Instead of adding yet another field in the configuration file, I'm using the same name as the keyvault for the storage account. |
Let us have |
Tracking issue here: #26 |
fixes #22