Skip to content

Commit c8b644f

Browse files
andyzhangxandyzhangx
committed
fix: only authorize azcopy once
test: fix goling
1 parent a3a2f15 commit c8b644f

File tree

2 files changed

+26
-76
lines changed

2 files changed

+26
-76
lines changed

‎pkg/blob/controllerserver.go

Lines changed: 15 additions & 21 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,13 +781,13 @@ 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
}
@@ -830,11 +823,11 @@ func (d *Driver) authorizeAzcopyWithIdentity() ([]string, error) {
830823
return []string{}, fmt.Errorf("service principle or managed identity are both not set")
831824
}
832825

833-
// getSASToken will only generate sas token for azcopy in following conditions:
826+
// getAzcopyAuth will only generate sas token for azcopy in following conditions:
834827
// 1. secrets is not empty
835828
// 2. driver is not using managed identity and service principal
836829
// 3. azcopy returns AuthorizationPermissionMismatch error when using service principal or managed identity
837-
func (d *Driver) getSASToken(ctx context.Context, accountName, accountKey, storageEndpointSuffix string, accountOptions *azure.AccountOptions, secrets map[string]string, secretName, secretNamespace string) (string, error) {
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) {
838831
var authAzcopyEnv []string
839832
useSasToken := false
840833
if len(secrets) == 0 && len(secretName) == 0 {
@@ -847,15 +840,15 @@ func (d *Driver) getSASToken(ctx context.Context, accountName, accountKey, stora
847840
// search in cache first
848841
cache, err := d.azcopySasTokenCache.Get(accountName, azcache.CacheReadTypeDefault)
849842
if err != nil {
850-
return "", fmt.Errorf("get(%s) from azcopySasTokenCache failed with error: %v", accountName, err)
843+
return "", nil, fmt.Errorf("get(%s) from azcopySasTokenCache failed with error: %v", accountName, err)
851844
}
852845
if cache != nil {
853846
klog.V(2).Infof("use sas token for account(%s) since this account is found in azcopySasTokenCache", accountName)
854847
useSasToken = true
855848
} else {
856849
out, testErr := d.azcopy.TestListJobs(accountName, storageEndpointSuffix, authAzcopyEnv)
857850
if testErr != nil {
858-
return "", fmt.Errorf("azcopy list command failed with error(%v): %v", testErr, out)
851+
return "", nil, fmt.Errorf("azcopy list command failed with error(%v): %v", testErr, out)
859852
}
860853
if strings.Contains(out, authorizationPermissionMismatch) {
861854
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)
@@ -871,13 +864,14 @@ func (d *Driver) getSASToken(ctx context.Context, accountName, accountKey, stora
871864
var err error
872865
if accountKey == "" {
873866
if _, accountKey, err = d.GetStorageAccesskey(ctx, accountOptions, secrets, secretName, secretNamespace); err != nil {
874-
return "", err
867+
return "", nil, err
875868
}
876869
}
877870
klog.V(2).Infof("generate sas token for account(%s)", accountName)
878-
return generateSASToken(accountName, accountKey, storageEndpointSuffix, d.sasTokenExpirationMinutes)
871+
sasToken, err := generateSASToken(accountName, accountKey, storageEndpointSuffix, d.sasTokenExpirationMinutes)
872+
return sasToken, nil, err
879873
}
880-
return "", nil
874+
return "", authAzcopyEnv, nil
881875
}
882876

883877
// isValidVolumeCapabilities validates the given VolumeCapability array is valid

‎pkg/blob/controllerserver_test.go

Lines changed: 11 additions & 55 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,7 +1964,7 @@ 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
}
@@ -2031,7 +1987,7 @@ func TestGetSASToken(t *testing.T) {
20311987
defaultSecretAccountName: "accountName",
20321988
defaultSecretAccountKey: "YWNjb3VudGtleQo=",
20331989
}
2034-
accountSASToken, err := d.getSASToken(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
1990+
accountSASToken, _, err := d.getAzcopyAuth(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
20351991
if !reflect.DeepEqual(err, nil) || !strings.Contains(accountSASToken, "?se=") {
20361992
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
20371993
}
@@ -2057,7 +2013,7 @@ func TestGetSASToken(t *testing.T) {
20572013

20582014
expectedAccountSASToken := ""
20592015
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"))
2060-
accountSASToken, err := d.getSASToken(context.Background(), "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")
20612017
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
20622018
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
20632019
}
@@ -2078,7 +2034,7 @@ func TestGetSASToken(t *testing.T) {
20782034
ctx := context.Background()
20792035
expectedAccountSASToken := ""
20802036
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"))
2081-
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")
20822038
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
20832039
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
20842040
}

0 commit comments

Comments
 (0)