Skip to content

Commit 60e4168

Browse files
andyzhangxandyzhangx
authored
Merge pull request #1208 from andyzhangx/useSasToken-secret-1.23
[release-1.23] fix: generate sas token when secret is used in volume cloning
2 parents 3146637 + f2d6459 commit 60e4168

File tree

2 files changed

+55
-116
lines changed

2 files changed

+55
-116
lines changed

‎pkg/blob/controllerserver.go‎

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

423423
if req.GetVolumeContentSource() != nil {
424-
var accountSASToken string
425-
if accountSASToken, err = d.getSASToken(ctx, accountName, accountKey, storageEndpointSuffix, accountOptions, secrets, secretName, secretNamespace); err != nil {
426-
return nil, status.Errorf(codes.Internal, "failed to getSASToken on account(%s) rg(%s), error: %v", accountOptions.Name, accountOptions.ResourceGroup, err)
424+
accountSASToken, authAzcopyEnv, err := d.getAzcopyAuth(ctx, accountName, accountKey, storageEndpointSuffix, accountOptions, secrets, secretName, secretNamespace)
425+
if err != nil {
426+
return nil, status.Errorf(codes.Internal, "failed to getAzcopyAuth on account(%s) rg(%s), error: %v", accountOptions.Name, accountOptions.ResourceGroup, err)
427427
}
428-
if err := d.copyVolume(ctx, req, accountSASToken, validContainerName, storageEndpointSuffix); err != nil {
428+
if err := d.copyVolume(req, accountSASToken, authAzcopyEnv, validContainerName, storageEndpointSuffix); err != nil {
429429
return nil, err
430430
}
431431
} else {
@@ -720,7 +720,7 @@ func (d *Driver) DeleteBlobContainer(ctx context.Context, subsID, resourceGroupN
720720
}
721721

722722
// CopyBlobContainer copies a blob container in the same storage account
723-
func (d *Driver) copyBlobContainer(_ context.Context, req *csi.CreateVolumeRequest, accountSasToken, dstContainerName, storageEndpointSuffix string) error {
723+
func (d *Driver) copyBlobContainer(req *csi.CreateVolumeRequest, accountSasToken string, authAzcopyEnv []string, dstContainerName, storageEndpointSuffix string) error {
724724
var sourceVolumeID string
725725
if req.GetVolumeContentSource() != nil && req.GetVolumeContentSource().GetVolume() != nil {
726726
sourceVolumeID = req.GetVolumeContentSource().GetVolume().GetVolumeId()
@@ -734,13 +734,6 @@ func (d *Driver) copyBlobContainer(_ context.Context, req *csi.CreateVolumeReque
734734
return fmt.Errorf("srcContainerName(%s) or dstContainerName(%s) is empty", srcContainerName, dstContainerName)
735735
}
736736

737-
var authAzcopyEnv []string
738-
if accountSasToken == "" {
739-
if authAzcopyEnv, err = d.authorizeAzcopyWithIdentity(); err != nil {
740-
return err
741-
}
742-
}
743-
744737
timeAfter := time.After(waitForCopyTimeout)
745738
timeTick := time.Tick(waitForCopyInterval)
746739
srcPath := fmt.Sprintf("https://%s.blob.%s/%s%s", accountName, storageEndpointSuffix, srcContainerName, accountSasToken)
@@ -781,18 +774,19 @@ func (d *Driver) copyBlobContainer(_ context.Context, req *csi.CreateVolumeReque
781774
}
782775

783776
// copyVolume copies a volume form volume or snapshot, snapshot is not supported now
784-
func (d *Driver) copyVolume(ctx context.Context, req *csi.CreateVolumeRequest, accountSASToken, dstContainerName, storageEndpointSuffix string) error {
777+
func (d *Driver) copyVolume(req *csi.CreateVolumeRequest, accountSASToken string, authAzcopyEnv []string, dstContainerName, storageEndpointSuffix string) error {
785778
vs := req.VolumeContentSource
786779
switch vs.Type.(type) {
787780
case *csi.VolumeContentSource_Snapshot:
788781
return status.Errorf(codes.InvalidArgument, "copy volume from volumeSnapshot is not supported")
789782
case *csi.VolumeContentSource_Volume:
790-
return d.copyBlobContainer(ctx, req, accountSASToken, dstContainerName, storageEndpointSuffix)
783+
return d.copyBlobContainer(req, accountSASToken, authAzcopyEnv, dstContainerName, storageEndpointSuffix)
791784
default:
792785
return status.Errorf(codes.InvalidArgument, "%v is not a proper volume source", vs)
793786
}
794787
}
795788

789+
// authorizeAzcopyWithIdentity returns auth env for azcopy using cluster identity
796790
func (d *Driver) authorizeAzcopyWithIdentity() ([]string, error) {
797791
azureAuthConfig := d.cloud.Config.AzureAuthConfig
798792
var authAzcopyEnv []string
@@ -822,45 +816,55 @@ func (d *Driver) authorizeAzcopyWithIdentity() ([]string, error) {
822816
return []string{}, fmt.Errorf("service principle or managed identity are both not set")
823817
}
824818

825-
// getSASToken will only generate sas token for azcopy in following conditions:
819+
// getAzcopyAuth will only generate sas token for azcopy in following conditions:
826820
// 1. secrets is not empty
827821
// 2. driver is not using managed identity and service principal
828822
// 3. azcopy returns AuthorizationPermissionMismatch error when using service principal or managed identity
829-
func (d *Driver) getSASToken(ctx context.Context, accountName, accountKey, storageEndpointSuffix string, accountOptions *azure.AccountOptions, secrets map[string]string, secretName, secretNamespace string) (string, error) {
830-
authAzcopyEnv, _ := d.authorizeAzcopyWithIdentity()
823+
func (d *Driver) getAzcopyAuth(ctx context.Context, accountName, accountKey, storageEndpointSuffix string, accountOptions *azure.AccountOptions, secrets map[string]string, secretName, secretNamespace string) (string, []string, error) {
824+
var authAzcopyEnv []string
831825
useSasToken := false
832-
if len(authAzcopyEnv) > 0 {
833-
// search in cache first
834-
cache, err := d.azcopySasTokenCache.Get(accountName, azcache.CacheReadTypeDefault)
826+
if len(secrets) == 0 && len(secretName) == 0 {
827+
var err error
828+
authAzcopyEnv, err = d.authorizeAzcopyWithIdentity()
835829
if err != nil {
836-
return "", fmt.Errorf("get(%s) from azcopySasTokenCache failed with error: %v", accountName, err)
837-
}
838-
if cache != nil {
839-
klog.V(2).Infof("use sas token for account(%s) since this account is found in azcopySasTokenCache", accountName)
840-
useSasToken = true
830+
klog.Warningf("failed to authorize azcopy with identity, error: %v", err)
841831
} else {
842-
out, testErr := d.azcopy.TestListJobs(accountName, storageEndpointSuffix, authAzcopyEnv)
843-
if testErr != nil {
844-
return "", fmt.Errorf("azcopy list command failed with error(%v): %v", testErr, out)
845-
}
846-
if strings.Contains(out, authorizationPermissionMismatch) {
847-
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)
848-
d.azcopySasTokenCache.Set(accountName, "")
849-
useSasToken = true
832+
if len(authAzcopyEnv) > 0 {
833+
// search in cache first
834+
cache, err := d.azcopySasTokenCache.Get(accountName, azcache.CacheReadTypeDefault)
835+
if err != nil {
836+
return "", nil, fmt.Errorf("get(%s) from azcopySasTokenCache failed with error: %v", accountName, err)
837+
}
838+
if cache != nil {
839+
klog.V(2).Infof("use sas token for account(%s) since this account is found in azcopySasTokenCache", accountName)
840+
useSasToken = true
841+
} else {
842+
out, testErr := d.azcopy.TestListJobs(accountName, storageEndpointSuffix, authAzcopyEnv)
843+
if testErr != nil {
844+
return "", nil, fmt.Errorf("azcopy list command failed with error(%v): %v", testErr, out)
845+
}
846+
if strings.Contains(out, authorizationPermissionMismatch) {
847+
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)
848+
d.azcopySasTokenCache.Set(accountName, "")
849+
useSasToken = true
850+
}
851+
}
850852
}
851853
}
852854
}
853-
if len(secrets) > 0 || len(authAzcopyEnv) == 0 || useSasToken {
855+
856+
if len(secrets) > 0 || len(secretName) > 0 || len(authAzcopyEnv) == 0 || useSasToken {
854857
var err error
855858
if accountKey == "" {
856859
if _, accountKey, err = d.GetStorageAccesskey(ctx, accountOptions, secrets, secretName, secretNamespace); err != nil {
857-
return "", err
860+
return "", nil, err
858861
}
859862
}
860863
klog.V(2).Infof("generate sas token for account(%s)", accountName)
861-
return generateSASToken(accountName, accountKey, storageEndpointSuffix, d.sasTokenExpirationMinutes)
864+
sasToken, err := generateSASToken(accountName, accountKey, storageEndpointSuffix, d.sasTokenExpirationMinutes)
865+
return sasToken, nil, err
862866
}
863-
return "", nil
867+
return "", authAzcopyEnv, nil
864868
}
865869

866870
// isValidVolumeCapabilities validates the given VolumeCapability array is valid

‎pkg/blob/controllerserver_test.go‎

Lines changed: 14 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -1555,10 +1555,8 @@ func TestCopyVolume(t *testing.T) {
15551555
VolumeContentSource: &volumecontensource,
15561556
}
15571557

1558-
ctx := context.Background()
1559-
15601558
expectedErr := status.Errorf(codes.InvalidArgument, "copy volume from volumeSnapshot is not supported")
1561-
err := d.copyVolume(ctx, req, "", "", "core.windows.net")
1559+
err := d.copyVolume(req, "", nil, "", "core.windows.net")
15621560
if !reflect.DeepEqual(err, expectedErr) {
15631561
t.Errorf("Unexpected error: %v", err)
15641562
}
@@ -1587,10 +1585,8 @@ func TestCopyVolume(t *testing.T) {
15871585
VolumeContentSource: &volumecontensource,
15881586
}
15891587

1590-
ctx := context.Background()
1591-
15921588
expectedErr := status.Errorf(codes.NotFound, "error parsing volume id: \"unit-test\", should at least contain two #")
1593-
err := d.copyVolume(ctx, req, "", "dstContainer", "core.windows.net")
1589+
err := d.copyVolume(req, "", nil, "dstContainer", "core.windows.net")
15941590
if !reflect.DeepEqual(err, expectedErr) {
15951591
t.Errorf("Unexpected error: %v", err)
15961592
}
@@ -1619,10 +1615,8 @@ func TestCopyVolume(t *testing.T) {
16191615
VolumeContentSource: &volumecontensource,
16201616
}
16211617

1622-
ctx := context.Background()
1623-
16241618
expectedErr := fmt.Errorf("srcContainerName() or dstContainerName(dstContainer) is empty")
1625-
err := d.copyVolume(ctx, req, "", "dstContainer", "core.windows.net")
1619+
err := d.copyVolume(req, "", nil, "dstContainer", "core.windows.net")
16261620
if !reflect.DeepEqual(err, expectedErr) {
16271621
t.Errorf("Unexpected error: %v", err)
16281622
}
@@ -1651,43 +1645,8 @@ func TestCopyVolume(t *testing.T) {
16511645
VolumeContentSource: &volumecontensource,
16521646
}
16531647

1654-
ctx := context.Background()
1655-
16561648
expectedErr := fmt.Errorf("srcContainerName(fileshare) or dstContainerName() is empty")
1657-
err := d.copyVolume(ctx, req, "", "", "core.windows.net")
1658-
if !reflect.DeepEqual(err, expectedErr) {
1659-
t.Errorf("Unexpected error: %v", err)
1660-
}
1661-
},
1662-
},
1663-
{
1664-
name: "AADClientSecret shouldn't be nil or useManagedIdentityExtension must be set to true when accountSASToken is empty",
1665-
testFunc: func(t *testing.T) {
1666-
d := NewFakeDriver()
1667-
d.cloud = &azure.Cloud{}
1668-
mp := map[string]string{}
1669-
1670-
volumeSource := &csi.VolumeContentSource_VolumeSource{
1671-
VolumeId: "vol_1#f5713de20cde511e8ba4900#fileshare#",
1672-
}
1673-
volumeContentSourceVolumeSource := &csi.VolumeContentSource_Volume{
1674-
Volume: volumeSource,
1675-
}
1676-
volumecontensource := csi.VolumeContentSource{
1677-
Type: volumeContentSourceVolumeSource,
1678-
}
1679-
1680-
req := &csi.CreateVolumeRequest{
1681-
Name: "unit-test",
1682-
VolumeCapabilities: stdVolumeCapabilities,
1683-
Parameters: mp,
1684-
VolumeContentSource: &volumecontensource,
1685-
}
1686-
1687-
ctx := context.Background()
1688-
1689-
expectedErr := fmt.Errorf("service principle or managed identity are both not set")
1690-
err := d.copyVolume(ctx, req, "", "dstContainer", "core.windows.net")
1649+
err := d.copyVolume(req, "", nil, "", "core.windows.net")
16911650
if !reflect.DeepEqual(err, expectedErr) {
16921651
t.Errorf("Unexpected error: %v", err)
16931652
}
@@ -1728,10 +1687,8 @@ func TestCopyVolume(t *testing.T) {
17281687

17291688
d.azcopy.ExecCmd = m
17301689

1731-
ctx := context.Background()
1732-
17331690
var expectedErr error
1734-
err := d.copyVolume(ctx, req, "sastoken", "dstContainer", "core.windows.net")
1691+
err := d.copyVolume(req, "sastoken", nil, "dstContainer", "core.windows.net")
17351692
if !reflect.DeepEqual(err, expectedErr) {
17361693
t.Errorf("Unexpected error: %v", err)
17371694
}
@@ -1773,10 +1730,8 @@ func TestCopyVolume(t *testing.T) {
17731730

17741731
d.azcopy.ExecCmd = m
17751732

1776-
ctx := context.Background()
1777-
17781733
var expectedErr error
1779-
err := d.copyVolume(ctx, req, "sastoken", "dstContainer", "core.windows.net")
1734+
err := d.copyVolume(req, "sastoken", nil, "dstContainer", "core.windows.net")
17801735
if !reflect.DeepEqual(err, expectedErr) {
17811736
t.Errorf("Unexpected error: %v", err)
17821737
}
@@ -1995,7 +1950,7 @@ func TestAuthorizeAzcopyWithIdentity(t *testing.T) {
19951950
}
19961951
}
19971952

1998-
func TestGetSASToken(t *testing.T) {
1953+
func TestGetAzcopyAuth(t *testing.T) {
19991954
testCases := []struct {
20001955
name string
20011956
testFunc func(t *testing.T)
@@ -2014,14 +1969,14 @@ func TestGetSASToken(t *testing.T) {
20141969
ctx := context.Background()
20151970
expectedAccountSASToken := ""
20161971
expectedErr := fmt.Errorf("could not find accountkey or azurestorageaccountkey field in secrets")
2017-
accountSASToken, err := d.getSASToken(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
1972+
accountSASToken, _, err := d.getAzcopyAuth(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
20181973
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
20191974
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
20201975
}
20211976
},
20221977
},
20231978
{
2024-
name: "failed to test azcopy list command",
1979+
name: "generate sas token using account key",
20251980
testFunc: func(t *testing.T) {
20261981
d := NewFakeDriver()
20271982
d.cloud = &azure.Cloud{
@@ -2033,21 +1988,10 @@ func TestGetSASToken(t *testing.T) {
20331988
}
20341989
secrets := map[string]string{
20351990
defaultSecretAccountName: "accountName",
1991+
defaultSecretAccountKey: "YWNjb3VudGtleQo=",
20361992
}
2037-
ctrl := gomock.NewController(t)
2038-
defer ctrl.Finish()
2039-
2040-
m := util.NewMockEXEC(ctrl)
2041-
listStr := "error"
2042-
m.EXPECT().RunCommand(gomock.Any(), gomock.Any()).Return(listStr, fmt.Errorf("error"))
2043-
2044-
d.azcopy.ExecCmd = m
2045-
2046-
ctx := context.Background()
2047-
expectedAccountSASToken := ""
2048-
expectedErr := fmt.Errorf("azcopy list command failed with error(%v): %v", fmt.Errorf("error"), "error")
2049-
accountSASToken, err := d.getSASToken(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
2050-
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
1993+
accountSASToken, _, err := d.getAzcopyAuth(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
1994+
if !reflect.DeepEqual(err, nil) || !strings.Contains(accountSASToken, "?se=") {
20511995
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
20521996
}
20531997
},
@@ -2067,19 +2011,10 @@ func TestGetSASToken(t *testing.T) {
20672011
defaultSecretAccountName: "accountName",
20682012
defaultSecretAccountKey: "fakeValue",
20692013
}
2070-
ctrl := gomock.NewController(t)
2071-
defer ctrl.Finish()
2072-
2073-
m := util.NewMockEXEC(ctrl)
2074-
listStr := "RESPONSE 403: 403 This request is not authorized to perform this operation using this permission.\nERROR CODE: AuthorizationPermissionMismatch"
2075-
m.EXPECT().RunCommand(gomock.Any(), gomock.Any()).Return(listStr, nil)
20762014

2077-
d.azcopy.ExecCmd = m
2078-
2079-
ctx := context.Background()
20802015
expectedAccountSASToken := ""
20812016
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"))
2082-
accountSASToken, err := d.getSASToken(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
2017+
accountSASToken, _, err := d.getAzcopyAuth(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
20832018
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
20842019
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
20852020
}
@@ -2100,7 +2035,7 @@ func TestGetSASToken(t *testing.T) {
21002035
ctx := context.Background()
21012036
expectedAccountSASToken := ""
21022037
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"))
2103-
accountSASToken, err := d.getSASToken(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
2038+
accountSASToken, _, err := d.getAzcopyAuth(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
21042039
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
21052040
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
21062041
}

0 commit comments

Comments
 (0)