Skip to content

Commit eebb307

Browse files
andyzhangxandyzhangx
authored
Merge pull request #1207 from andyzhangx/useSasToken-secret
fix: generate sas token when secret is used in volume cloning
2 parents c2a1d9b + c8b644f commit eebb307

File tree

2 files changed

+55
-115
lines changed

2 files changed

+55
-115
lines changed

‎pkg/blob/controllerserver.go

Lines changed: 41 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -428,11 +428,11 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
428428
}
429429

430430
if req.GetVolumeContentSource() != nil {
431-
var accountSASToken string
432-
if accountSASToken, err = d.getSASToken(ctx, accountName, accountKey, storageEndpointSuffix, accountOptions, secrets, secretName, secretNamespace); err != nil {
433-
return nil, status.Errorf(codes.Internal, "failed to getSASToken on account(%s) rg(%s), error: %v", accountOptions.Name, accountOptions.ResourceGroup, err)
431+
accountSASToken, authAzcopyEnv, err := d.getAzcopyAuth(ctx, accountName, accountKey, storageEndpointSuffix, accountOptions, secrets, secretName, secretNamespace)
432+
if err != nil {
433+
return nil, status.Errorf(codes.Internal, "failed to getAzcopyAuth on account(%s) rg(%s), error: %v", accountOptions.Name, accountOptions.ResourceGroup, err)
434434
}
435-
if err := d.copyVolume(ctx, req, accountSASToken, validContainerName, storageEndpointSuffix); err != nil {
435+
if err := d.copyVolume(req, accountSASToken, authAzcopyEnv, validContainerName, storageEndpointSuffix); err != nil {
436436
return nil, err
437437
}
438438
} else {
@@ -727,7 +727,7 @@ func (d *Driver) DeleteBlobContainer(ctx context.Context, subsID, resourceGroupN
727727
}
728728

729729
// CopyBlobContainer copies a blob container in the same storage account
730-
func (d *Driver) copyBlobContainer(_ context.Context, req *csi.CreateVolumeRequest, accountSasToken, dstContainerName, storageEndpointSuffix string) error {
730+
func (d *Driver) copyBlobContainer(req *csi.CreateVolumeRequest, accountSasToken string, authAzcopyEnv []string, dstContainerName, storageEndpointSuffix string) error {
731731
var sourceVolumeID string
732732
if req.GetVolumeContentSource() != nil && req.GetVolumeContentSource().GetVolume() != nil {
733733
sourceVolumeID = req.GetVolumeContentSource().GetVolume().GetVolumeId()
@@ -741,13 +741,6 @@ func (d *Driver) copyBlobContainer(_ context.Context, req *csi.CreateVolumeReque
741741
return fmt.Errorf("srcContainerName(%s) or dstContainerName(%s) is empty", srcContainerName, dstContainerName)
742742
}
743743

744-
var authAzcopyEnv []string
745-
if accountSasToken == "" {
746-
if authAzcopyEnv, err = d.authorizeAzcopyWithIdentity(); err != nil {
747-
return err
748-
}
749-
}
750-
751744
timeAfter := time.After(waitForCopyTimeout)
752745
timeTick := time.Tick(waitForCopyInterval)
753746
srcPath := fmt.Sprintf("https://%s.blob.%s/%s%s", accountName, storageEndpointSuffix, srcContainerName, accountSasToken)
@@ -788,18 +781,19 @@ func (d *Driver) copyBlobContainer(_ context.Context, req *csi.CreateVolumeReque
788781
}
789782

790783
// copyVolume copies a volume form volume or snapshot, snapshot is not supported now
791-
func (d *Driver) copyVolume(ctx context.Context, req *csi.CreateVolumeRequest, accountSASToken, dstContainerName, storageEndpointSuffix string) error {
784+
func (d *Driver) copyVolume(req *csi.CreateVolumeRequest, accountSASToken string, authAzcopyEnv []string, dstContainerName, storageEndpointSuffix string) error {
792785
vs := req.VolumeContentSource
793786
switch vs.Type.(type) {
794787
case *csi.VolumeContentSource_Snapshot:
795788
return status.Errorf(codes.InvalidArgument, "copy volume from volumeSnapshot is not supported")
796789
case *csi.VolumeContentSource_Volume:
797-
return d.copyBlobContainer(ctx, req, accountSASToken, dstContainerName, storageEndpointSuffix)
790+
return d.copyBlobContainer(req, accountSASToken, authAzcopyEnv, dstContainerName, storageEndpointSuffix)
798791
default:
799792
return status.Errorf(codes.InvalidArgument, "%v is not a proper volume source", vs)
800793
}
801794
}
802795

796+
// authorizeAzcopyWithIdentity returns auth env for azcopy using cluster identity
803797
func (d *Driver) authorizeAzcopyWithIdentity() ([]string, error) {
804798
azureAuthConfig := d.cloud.Config.AzureAuthConfig
805799
var authAzcopyEnv []string
@@ -829,45 +823,55 @@ func (d *Driver) authorizeAzcopyWithIdentity() ([]string, error) {
829823
return []string{}, fmt.Errorf("service principle or managed identity are both not set")
830824
}
831825

832-
// getSASToken will only generate sas token for azcopy in following conditions:
826+
// getAzcopyAuth will only generate sas token for azcopy in following conditions:
833827
// 1. secrets is not empty
834828
// 2. driver is not using managed identity and service principal
835829
// 3. azcopy returns AuthorizationPermissionMismatch error when using service principal or managed identity
836-
func (d *Driver) getSASToken(ctx context.Context, accountName, accountKey, storageEndpointSuffix string, accountOptions *azure.AccountOptions, secrets map[string]string, secretName, secretNamespace string) (string, error) {
837-
authAzcopyEnv, _ := d.authorizeAzcopyWithIdentity()
830+
func (d *Driver) getAzcopyAuth(ctx context.Context, accountName, accountKey, storageEndpointSuffix string, accountOptions *azure.AccountOptions, secrets map[string]string, secretName, secretNamespace string) (string, []string, error) {
831+
var authAzcopyEnv []string
838832
useSasToken := false
839-
if len(authAzcopyEnv) > 0 {
840-
// search in cache first
841-
cache, err := d.azcopySasTokenCache.Get(accountName, azcache.CacheReadTypeDefault)
833+
if len(secrets) == 0 && len(secretName) == 0 {
834+
var err error
835+
authAzcopyEnv, err = d.authorizeAzcopyWithIdentity()
842836
if err != nil {
843-
return "", fmt.Errorf("get(%s) from azcopySasTokenCache failed with error: %v", accountName, err)
844-
}
845-
if cache != nil {
846-
klog.V(2).Infof("use sas token for account(%s) since this account is found in azcopySasTokenCache", accountName)
847-
useSasToken = true
837+
klog.Warningf("failed to authorize azcopy with identity, error: %v", err)
848838
} else {
849-
out, testErr := d.azcopy.TestListJobs(accountName, storageEndpointSuffix, authAzcopyEnv)
850-
if testErr != nil {
851-
return "", fmt.Errorf("azcopy list command failed with error(%v): %v", testErr, out)
852-
}
853-
if strings.Contains(out, authorizationPermissionMismatch) {
854-
klog.Warningf("azcopy list failed with AuthorizationPermissionMismatch error, should assign \"Storage Blob Data Contributor\" role to controller identity, fall back to use sas token, original output: %v", out)
855-
d.azcopySasTokenCache.Set(accountName, "")
856-
useSasToken = true
839+
if len(authAzcopyEnv) > 0 {
840+
// search in cache first
841+
cache, err := d.azcopySasTokenCache.Get(accountName, azcache.CacheReadTypeDefault)
842+
if err != nil {
843+
return "", nil, fmt.Errorf("get(%s) from azcopySasTokenCache failed with error: %v", accountName, err)
844+
}
845+
if cache != nil {
846+
klog.V(2).Infof("use sas token for account(%s) since this account is found in azcopySasTokenCache", accountName)
847+
useSasToken = true
848+
} else {
849+
out, testErr := d.azcopy.TestListJobs(accountName, storageEndpointSuffix, authAzcopyEnv)
850+
if testErr != nil {
851+
return "", nil, fmt.Errorf("azcopy list command failed with error(%v): %v", testErr, out)
852+
}
853+
if strings.Contains(out, authorizationPermissionMismatch) {
854+
klog.Warningf("azcopy list failed with AuthorizationPermissionMismatch error, should assign \"Storage Blob Data Contributor\" role to controller identity, fall back to use sas token, original output: %v", out)
855+
d.azcopySasTokenCache.Set(accountName, "")
856+
useSasToken = true
857+
}
858+
}
857859
}
858860
}
859861
}
860-
if len(secrets) > 0 || len(authAzcopyEnv) == 0 || useSasToken {
862+
863+
if len(secrets) > 0 || len(secretName) > 0 || len(authAzcopyEnv) == 0 || useSasToken {
861864
var err error
862865
if accountKey == "" {
863866
if _, accountKey, err = d.GetStorageAccesskey(ctx, accountOptions, secrets, secretName, secretNamespace); err != nil {
864-
return "", err
867+
return "", nil, err
865868
}
866869
}
867870
klog.V(2).Infof("generate sas token for account(%s)", accountName)
868-
return generateSASToken(accountName, accountKey, storageEndpointSuffix, d.sasTokenExpirationMinutes)
871+
sasToken, err := generateSASToken(accountName, accountKey, storageEndpointSuffix, d.sasTokenExpirationMinutes)
872+
return sasToken, nil, err
869873
}
870-
return "", nil
874+
return "", authAzcopyEnv, nil
871875
}
872876

873877
// isValidVolumeCapabilities validates the given VolumeCapability array is valid

‎pkg/blob/controllerserver_test.go

Lines changed: 14 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -1538,10 +1538,8 @@ func TestCopyVolume(t *testing.T) {
15381538
VolumeContentSource: &volumecontensource,
15391539
}
15401540

1541-
ctx := context.Background()
1542-
15431541
expectedErr := status.Errorf(codes.InvalidArgument, "copy volume from volumeSnapshot is not supported")
1544-
err := d.copyVolume(ctx, req, "", "", "core.windows.net")
1542+
err := d.copyVolume(req, "", nil, "", "core.windows.net")
15451543
if !reflect.DeepEqual(err, expectedErr) {
15461544
t.Errorf("Unexpected error: %v", err)
15471545
}
@@ -1570,10 +1568,8 @@ func TestCopyVolume(t *testing.T) {
15701568
VolumeContentSource: &volumecontensource,
15711569
}
15721570

1573-
ctx := context.Background()
1574-
15751571
expectedErr := status.Errorf(codes.NotFound, "error parsing volume id: \"unit-test\", should at least contain two #")
1576-
err := d.copyVolume(ctx, req, "", "dstContainer", "core.windows.net")
1572+
err := d.copyVolume(req, "", nil, "dstContainer", "core.windows.net")
15771573
if !reflect.DeepEqual(err, expectedErr) {
15781574
t.Errorf("Unexpected error: %v", err)
15791575
}
@@ -1602,10 +1598,8 @@ func TestCopyVolume(t *testing.T) {
16021598
VolumeContentSource: &volumecontensource,
16031599
}
16041600

1605-
ctx := context.Background()
1606-
16071601
expectedErr := fmt.Errorf("srcContainerName() or dstContainerName(dstContainer) is empty")
1608-
err := d.copyVolume(ctx, req, "", "dstContainer", "core.windows.net")
1602+
err := d.copyVolume(req, "", nil, "dstContainer", "core.windows.net")
16091603
if !reflect.DeepEqual(err, expectedErr) {
16101604
t.Errorf("Unexpected error: %v", err)
16111605
}
@@ -1634,42 +1628,8 @@ func TestCopyVolume(t *testing.T) {
16341628
VolumeContentSource: &volumecontensource,
16351629
}
16361630

1637-
ctx := context.Background()
1638-
16391631
expectedErr := fmt.Errorf("srcContainerName(fileshare) or dstContainerName() is empty")
1640-
err := d.copyVolume(ctx, req, "", "", "core.windows.net")
1641-
if !reflect.DeepEqual(err, expectedErr) {
1642-
t.Errorf("Unexpected error: %v", err)
1643-
}
1644-
},
1645-
},
1646-
{
1647-
name: "AADClientSecret shouldn't be nil or useManagedIdentityExtension must be set to true when accountSASToken is empty",
1648-
testFunc: func(t *testing.T) {
1649-
d := NewFakeDriver()
1650-
mp := map[string]string{}
1651-
1652-
volumeSource := &csi.VolumeContentSource_VolumeSource{
1653-
VolumeId: "vol_1#f5713de20cde511e8ba4900#fileshare#",
1654-
}
1655-
volumeContentSourceVolumeSource := &csi.VolumeContentSource_Volume{
1656-
Volume: volumeSource,
1657-
}
1658-
volumecontensource := csi.VolumeContentSource{
1659-
Type: volumeContentSourceVolumeSource,
1660-
}
1661-
1662-
req := &csi.CreateVolumeRequest{
1663-
Name: "unit-test",
1664-
VolumeCapabilities: stdVolumeCapabilities,
1665-
Parameters: mp,
1666-
VolumeContentSource: &volumecontensource,
1667-
}
1668-
1669-
ctx := context.Background()
1670-
1671-
expectedErr := fmt.Errorf("service principle or managed identity are both not set")
1672-
err := d.copyVolume(ctx, req, "", "dstContainer", "core.windows.net")
1632+
err := d.copyVolume(req, "", nil, "", "core.windows.net")
16731633
if !reflect.DeepEqual(err, expectedErr) {
16741634
t.Errorf("Unexpected error: %v", err)
16751635
}
@@ -1710,10 +1670,8 @@ func TestCopyVolume(t *testing.T) {
17101670

17111671
d.azcopy.ExecCmd = m
17121672

1713-
ctx := context.Background()
1714-
17151673
var expectedErr error
1716-
err := d.copyVolume(ctx, req, "sastoken", "dstContainer", "core.windows.net")
1674+
err := d.copyVolume(req, "sastoken", nil, "dstContainer", "core.windows.net")
17171675
if !reflect.DeepEqual(err, expectedErr) {
17181676
t.Errorf("Unexpected error: %v", err)
17191677
}
@@ -1755,10 +1713,8 @@ func TestCopyVolume(t *testing.T) {
17551713

17561714
d.azcopy.ExecCmd = m
17571715

1758-
ctx := context.Background()
1759-
17601716
var expectedErr error
1761-
err := d.copyVolume(ctx, req, "sastoken", "dstContainer", "core.windows.net")
1717+
err := d.copyVolume(req, "sastoken", nil, "dstContainer", "core.windows.net")
17621718
if !reflect.DeepEqual(err, expectedErr) {
17631719
t.Errorf("Unexpected error: %v", err)
17641720
}
@@ -1989,7 +1945,7 @@ func TestAuthorizeAzcopyWithIdentity(t *testing.T) {
19891945
}
19901946
}
19911947

1992-
func TestGetSASToken(t *testing.T) {
1948+
func TestGetAzcopyAuth(t *testing.T) {
19931949
testCases := []struct {
19941950
name string
19951951
testFunc func(t *testing.T)
@@ -2008,14 +1964,14 @@ func TestGetSASToken(t *testing.T) {
20081964
ctx := context.Background()
20091965
expectedAccountSASToken := ""
20101966
expectedErr := fmt.Errorf("could not find accountkey or azurestorageaccountkey field in secrets")
2011-
accountSASToken, err := d.getSASToken(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
1967+
accountSASToken, _, err := d.getAzcopyAuth(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
20121968
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
20131969
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
20141970
}
20151971
},
20161972
},
20171973
{
2018-
name: "failed to test azcopy list command",
1974+
name: "generate sas token using account key",
20191975
testFunc: func(t *testing.T) {
20201976
d := NewFakeDriver()
20211977
d.cloud = &azure.Cloud{
@@ -2029,21 +1985,10 @@ func TestGetSASToken(t *testing.T) {
20291985
}
20301986
secrets := map[string]string{
20311987
defaultSecretAccountName: "accountName",
1988+
defaultSecretAccountKey: "YWNjb3VudGtleQo=",
20321989
}
2033-
ctrl := gomock.NewController(t)
2034-
defer ctrl.Finish()
2035-
2036-
m := util.NewMockEXEC(ctrl)
2037-
listStr := "error"
2038-
m.EXPECT().RunCommand(gomock.Any(), gomock.Any()).Return(listStr, fmt.Errorf("error"))
2039-
2040-
d.azcopy.ExecCmd = m
2041-
2042-
ctx := context.Background()
2043-
expectedAccountSASToken := ""
2044-
expectedErr := fmt.Errorf("azcopy list command failed with error(%v): %v", fmt.Errorf("error"), "error")
2045-
accountSASToken, err := d.getSASToken(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
2046-
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
1990+
accountSASToken, _, err := d.getAzcopyAuth(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
1991+
if !reflect.DeepEqual(err, nil) || !strings.Contains(accountSASToken, "?se=") {
20471992
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
20481993
}
20491994
},
@@ -2065,19 +2010,10 @@ func TestGetSASToken(t *testing.T) {
20652010
defaultSecretAccountName: "accountName",
20662011
defaultSecretAccountKey: "fakeValue",
20672012
}
2068-
ctrl := gomock.NewController(t)
2069-
defer ctrl.Finish()
2070-
2071-
m := util.NewMockEXEC(ctrl)
2072-
listStr := "RESPONSE 403: 403 This request is not authorized to perform this operation using this permission.\nERROR CODE: AuthorizationPermissionMismatch"
2073-
m.EXPECT().RunCommand(gomock.Any(), gomock.Any()).Return(listStr, nil)
20742013

2075-
d.azcopy.ExecCmd = m
2076-
2077-
ctx := context.Background()
20782014
expectedAccountSASToken := ""
20792015
expectedErr := status.Errorf(codes.Internal, fmt.Sprintf("failed to generate sas token in creating new shared key credential, accountName: %s, err: %s", "accountName", "decode account key: illegal base64 data at input byte 8"))
2080-
accountSASToken, err := d.getSASToken(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
2016+
accountSASToken, _, err := d.getAzcopyAuth(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
20812017
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
20822018
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
20832019
}
@@ -2098,7 +2034,7 @@ func TestGetSASToken(t *testing.T) {
20982034
ctx := context.Background()
20992035
expectedAccountSASToken := ""
21002036
expectedErr := status.Errorf(codes.Internal, fmt.Sprintf("failed to generate sas token in creating new shared key credential, accountName: %s, err: %s", "accountName", "decode account key: illegal base64 data at input byte 8"))
2101-
accountSASToken, err := d.getSASToken(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
2037+
accountSASToken, _, err := d.getAzcopyAuth(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
21022038
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
21032039
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
21042040
}

0 commit comments

Comments
 (0)