fix the error prone account creation method of blob disk #59739

andyzhangx · 2018-02-12T03:35:56Z

What this PR does / why we need it:
use new account generation method for blob disk to fix the error prone account creation method of blob disk

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #59738

Special notes for your reviewer:

Release note:

fix the error prone account creation method of azure blob disk

/assign @karataliu
/sig azure

feiskyer

LGTM

karataliu · 2018-02-12T07:10:03Z

pkg/cloudprovider/providers/azure/azure_blobDiskController.go

@@ -555,7 +543,7 @@ func (c *BlobDiskController) findSANameForDisk(storageAccountType storage.SkuNam
 	countAccounts := 0 // account of this type.
 	for _, v := range c.accounts {
 		// filter out any stand-alone disks/accounts
-		if strings.Index(v.name, storageAccountNameMatch) != 0 {
+		if strings.Index(v.name, azureDiskAccountNamePrefix) != 0 {


!strings.HasPrefix better

karataliu · 2018-02-12T07:12:35Z

pkg/cloudprovider/providers/azure/azure_blobDiskController.go

-	storageAccountNameMatch = storageAccountNamePrefix
-	// Used as a template to create new names for relevant accounts
-	storageAccountNamePrefix = storageAccountNamePrefix + "%s"
+	defaultContainerName = MakeCRC32(c.common.resourceGroup + c.common.location + c.common.subscriptionID)


Any reason we need a unique container name? What about using a fixed one

just keep it, it's not related to this PR, I just used the original code.
unique or fixed name, all looks good.

karataliu · 2018-02-12T07:24:02Z

pkg/cloudprovider/providers/azure/azure_blobDiskController.go

@@ -481,7 +469,7 @@ func (c *BlobDiskController) getAllStorageAccounts() (map[string]*storageAccount

 	accounts := make(map[string]*storageAccountState)
 	for _, v := range *accountListResult.Value {
-		if strings.Index(*v.Name, storageAccountNameMatch) != 0 {
+		if strings.Index(*v.Name, azureDiskAccountNamePrefix) != 0 {


The code accesses *v.Name first, and check v.Name == nil in next block? Consider fixing it?

Good catch. fixed.

karataliu · 2018-02-12T07:25:31Z

pkg/cloudprovider/providers/azure/azure_storage.go

@@ -27,6 +27,7 @@ import (
 const (
 	defaultStorageAccountType  = string(storage.StandardLRS)
 	fileShareAccountNamePrefix = "f"
+	azureDiskAccountNamePrefix = "dk"


why not 'd', if 'k' has special meaning consider add some comments here

This is for support kind = Shared accounts. one letter d is not enough, and I also don't want to use disk it has 4 letters, so I use dk it's not a word, to differentiate customer self created accounts. A little balancing tricky ...

Update, I used ds finally, stands for disk shared

andyzhangx

@karataliu PTAL again.

andyzhangx · 2018-02-12T07:34:46Z

pkg/cloudprovider/providers/azure/azure_blobDiskController.go

@@ -481,7 +469,7 @@ func (c *BlobDiskController) getAllStorageAccounts() (map[string]*storageAccount

 	accounts := make(map[string]*storageAccountState)
 	for _, v := range *accountListResult.Value {
-		if strings.Index(*v.Name, storageAccountNameMatch) != 0 {
+		if strings.Index(*v.Name, azureDiskAccountNamePrefix) != 0 {


Good catch. fixed.

andyzhangx · 2018-02-12T07:35:30Z

pkg/cloudprovider/providers/azure/azure_blobDiskController.go

@@ -555,7 +543,7 @@ func (c *BlobDiskController) findSANameForDisk(storageAccountType storage.SkuNam
 	countAccounts := 0 // account of this type.
 	for _, v := range c.accounts {
 		// filter out any stand-alone disks/accounts
-		if strings.Index(v.name, storageAccountNameMatch) != 0 {
+		if strings.Index(v.name, azureDiskAccountNamePrefix) != 0 {


andyzhangx · 2018-02-12T07:40:02Z

pkg/cloudprovider/providers/azure/azure_storage.go

@@ -27,6 +27,7 @@ import (
 const (
 	defaultStorageAccountType  = string(storage.StandardLRS)
 	fileShareAccountNamePrefix = "f"
+	azureDiskAccountNamePrefix = "dk"


This is for support kind = Shared accounts. one letter d is not enough, and I also don't want to use disk it has 4 letters, so I use dk it's not a word, to differentiate customer self created accounts. A little balancing tricky ...

Update, I used ds finally, stands for disk shared

andyzhangx · 2018-02-12T07:41:22Z

pkg/cloudprovider/providers/azure/azure_blobDiskController.go

-	storageAccountNameMatch = storageAccountNamePrefix
-	// Used as a template to create new names for relevant accounts
-	storageAccountNamePrefix = storageAccountNamePrefix + "%s"
+	defaultContainerName = MakeCRC32(c.common.resourceGroup + c.common.location + c.common.subscriptionID)


just keep it, it's not related to this PR, I just used the original code.
unique or fixed name, all looks good.

andyzhangx · 2018-02-12T08:11:36Z

@karataliu now below are the prefix nameing:

+	fileShareAccountNamePrefix     = "f"
 +	sharedDiskAccountNamePrefix    = "ds"
 +	dedicatedDiskAccountNamePrefix = "dd"

andyzhangx · 2018-02-12T09:17:28Z

@karataliu I added a new common func SearchStorageAccount for CreateFileShare and CreateVolume funcs, this PR should be the last of series of PRs which is targeting to refactor k8s volume on azure code stating from 2017.9, I will cherry pick this PR to v1.8, v1.9. Thanks for your code review.

karataliu · 2018-02-12T09:33:22Z

pkg/cloudprovider/providers/azure/azure_storageaccount.go

@@ -75,3 +79,52 @@ func (az *Cloud) getStorageAccesskey(account string) (string, error) {
 	}
 	return "", fmt.Errorf("no valid keys")
 }
+
+// SearchStorageAccount search storage account, create one storage account(with genAccountNamePrefix) if not found, return accountName, accountKey
+func (az *Cloud) SearchStorageAccount(accountName, accountType, location, genAccountNamePrefix string) (string, string, error) {


This func is to be used in package only, consider making it lower case

Also 'search' looks like a read only operation, may be use 'ensure' better, like other places

fixed, I am also considering a good name, ensureStorageAccount is better

karataliu · 2018-02-12T09:36:44Z

pkg/cloudprovider/providers/azure/azure_storage.go

-	fileShareAccountNamePrefix = "f"
+	defaultStorageAccountType      = string(storage.StandardLRS)
+	fileShareAccountNamePrefix     = "f"
+	sharedDiskAccountNamePrefix    = "ds"


Those should be placed in the file where it is in use?
azure_storage.go now contains file-service only code, maybe we need to merge it to azure_file.go someday.

yes, just keep it in this PR.

karataliu · 2018-02-12T09:41:06Z

pkg/cloudprovider/providers/azure/azure_storage.go

-
-	// find the access key with this account
-	accountKey, err := az.getStorageAccesskey(accountName)
+	accountName, accountKey, err := az.SearchStorageAccount(accountName, accountType, location, fileShareAccountNamePrefix)


We'd better not assign value to 'accountName' here.

Consider the code in SearchStorageAccount:

+ accountKey, err := az.getStorageAccesskey(accountName) + if err != nil { + return "", "", fmt.Errorf("could not get storage key for storage account %s: %v", accountName, err) + }

If user specifies 'AccountName', but later getStorageAccesskey fails, then SearchStorageAccount returns empty for first parameter.

In this case the follow-up error reported would use empty value for accountName, which does not sound good to user.

Also for CreateVolume.

good catch, fixed both funcs.

karataliu · 2018-02-12T09:41:48Z

pkg/cloudprovider/providers/azure/azure_blobDiskController.go

@@ -555,7 +516,7 @@ func (c *BlobDiskController) findSANameForDisk(storageAccountType storage.SkuNam
 	countAccounts := 0 // account of this type.
 	for _, v := range c.accounts {
 		// filter out any stand-alone disks/accounts
-		if strings.Index(v.name, storageAccountNameMatch) != 0 {
+		if strings.Index(v.name, sharedDiskAccountNamePrefix) != 0 {


Also strings.HasPrefix ?

fix comments change azureDiskSharedAccountNamePrefix var rename sharedDiskAccountNamePrefix use default vhd container name as "vhds" use one commaon func: SearchStorageAccount fix comments

andyzhangx

@karataliu really good comments, PTAL.

andyzhangx · 2018-02-12T12:56:02Z

pkg/cloudprovider/providers/azure/azure_blobDiskController.go

@@ -555,7 +516,7 @@ func (c *BlobDiskController) findSANameForDisk(storageAccountType storage.SkuNam
 	countAccounts := 0 // account of this type.
 	for _, v := range c.accounts {
 		// filter out any stand-alone disks/accounts
-		if strings.Index(v.name, storageAccountNameMatch) != 0 {
+		if strings.Index(v.name, sharedDiskAccountNamePrefix) != 0 {


andyzhangx · 2018-02-12T12:56:47Z

pkg/cloudprovider/providers/azure/azure_storage.go

-	fileShareAccountNamePrefix = "f"
+	defaultStorageAccountType      = string(storage.StandardLRS)
+	fileShareAccountNamePrefix     = "f"
+	sharedDiskAccountNamePrefix    = "ds"


yes, just keep it in this PR.

andyzhangx · 2018-02-12T13:03:19Z

pkg/cloudprovider/providers/azure/azure_storage.go

-
-	// find the access key with this account
-	accountKey, err := az.getStorageAccesskey(accountName)
+	accountName, accountKey, err := az.SearchStorageAccount(accountName, accountType, location, fileShareAccountNamePrefix)


good catch, fixed both funcs.

andyzhangx · 2018-02-12T13:03:47Z

pkg/cloudprovider/providers/azure/azure_storageaccount.go

@@ -75,3 +79,52 @@ func (az *Cloud) getStorageAccesskey(account string) (string, error) {
 	}
 	return "", fmt.Errorf("no valid keys")
 }
+
+// SearchStorageAccount search storage account, create one storage account(with genAccountNamePrefix) if not found, return accountName, accountKey
+func (az *Cloud) SearchStorageAccount(accountName, accountType, location, genAccountNamePrefix string) (string, string, error) {


fixed, I am also considering a good name, ensureStorageAccount is better

karataliu

/lgtm

k8s-ci-robot · 2018-02-13T01:51:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, karataliu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these OWNERS Files:

~~pkg/cloudprovider/providers/azure/OWNERS~~ [andyzhangx,karataliu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-github-robot · 2018-02-13T02:24:05Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-02-13T03:03:37Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

…9739-upstream-release-1.7 Automatic merge from submit-queue. Automated cherry pick of #59739 Cherry pick of #59739 on release-1.7. #59739: use new account generation method for blob disk

…9739-upstream-release-1.9 Automatic merge from submit-queue. Automated cherry pick of #59739: use new account generation method for blob disk Cherry pick of #59739 on release-1.9. #59739: use new account generation method for blob disk

k8s-ci-robot assigned karataliu Feb 12, 2018

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/azure size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 12, 2018

k8s-ci-robot requested review from feiskyer and jdumars February 12, 2018 03:36

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 12, 2018

feiskyer added this to the v1.10 milestone Feb 12, 2018

feiskyer added the kind/bug Categorizes issue or PR as related to a bug. label Feb 12, 2018

feiskyer approved these changes Feb 12, 2018

View reviewed changes

karataliu reviewed Feb 12, 2018

View reviewed changes

andyzhangx force-pushed the azuredisk-initaccount branch from 070c4a4 to 3f7f9b3 Compare February 12, 2018 07:42

andyzhangx commented Feb 12, 2018

View reviewed changes

andyzhangx force-pushed the azuredisk-initaccount branch 2 times, most recently from 07773fb to da22599 Compare February 12, 2018 08:06

andyzhangx force-pushed the azuredisk-initaccount branch 2 times, most recently from be22aaf to 21f7e69 Compare February 12, 2018 09:13

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 12, 2018

karataliu reviewed Feb 12, 2018

View reviewed changes

use new account generation method for blob disk

8a7198b

fix comments change azureDiskSharedAccountNamePrefix var rename sharedDiskAccountNamePrefix use default vhd container name as "vhds" use one commaon func: SearchStorageAccount fix comments

andyzhangx force-pushed the azuredisk-initaccount branch from 21f7e69 to 8a7198b Compare February 12, 2018 13:10

andyzhangx commented Feb 12, 2018

View reviewed changes

karataliu approved these changes Feb 13, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 13, 2018

k8s-github-robot merged commit 91c783e into kubernetes:master Feb 13, 2018

This was referenced Mar 5, 2018

Automated cherry pick of #59739: use new account generation method for blob disk #60756

Merged

Automated cherry pick of #59739 #60758

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix the error prone account creation method of blob disk #59739

fix the error prone account creation method of blob disk #59739

andyzhangx commented Feb 12, 2018

feiskyer left a comment

karataliu Feb 12, 2018

andyzhangx Feb 12, 2018

karataliu Feb 12, 2018

andyzhangx Feb 12, 2018

karataliu Feb 12, 2018 •

edited

Loading

andyzhangx Feb 12, 2018

karataliu Feb 12, 2018

andyzhangx Feb 12, 2018 •

edited

Loading

andyzhangx left a comment

andyzhangx Feb 12, 2018

andyzhangx Feb 12, 2018

andyzhangx Feb 12, 2018 •

edited

Loading

andyzhangx Feb 12, 2018

andyzhangx commented Feb 12, 2018

andyzhangx commented Feb 12, 2018

karataliu Feb 12, 2018

andyzhangx Feb 12, 2018

karataliu Feb 12, 2018

andyzhangx Feb 12, 2018

karataliu Feb 12, 2018

andyzhangx Feb 12, 2018

karataliu Feb 12, 2018

andyzhangx Feb 12, 2018

andyzhangx left a comment

andyzhangx Feb 12, 2018

andyzhangx Feb 12, 2018

andyzhangx Feb 12, 2018

andyzhangx Feb 12, 2018

karataliu left a comment

k8s-ci-robot commented Feb 13, 2018

k8s-github-robot commented Feb 13, 2018

k8s-github-robot commented Feb 13, 2018

fix the error prone account creation method of blob disk #59739

fix the error prone account creation method of blob disk #59739

Conversation

andyzhangx commented Feb 12, 2018

feiskyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karataliu Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyzhangx Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

andyzhangx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyzhangx Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyzhangx commented Feb 12, 2018

andyzhangx commented Feb 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyzhangx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karataliu left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 13, 2018

k8s-github-robot commented Feb 13, 2018

k8s-github-robot commented Feb 13, 2018

karataliu Feb 12, 2018 •

edited

Loading

andyzhangx Feb 12, 2018 •

edited

Loading

andyzhangx Feb 12, 2018 •

edited

Loading