From 68ef58d030ed3e0d51cba19fbcdfe41db5a2f15e Mon Sep 17 00:00:00 2001 From: andyzhangx Date: Tue, 2 Jan 2024 12:56:35 +0000 Subject: [PATCH] fix: add cache in sastoken fallback fix --- pkg/blob/blob.go | 5 +++++ pkg/blob/blob_test.go | 1 + pkg/blob/controllerserver.go | 27 +++++++++++++++++++-------- pkg/blob/controllerserver_test.go | 8 ++++---- 4 files changed, 29 insertions(+), 12 deletions(-) diff --git a/pkg/blob/blob.go b/pkg/blob/blob.go index 012c0a6a2..cac487f09 100644 --- a/pkg/blob/blob.go +++ b/pkg/blob/blob.go @@ -212,6 +212,8 @@ type Driver struct { accountSearchCache azcache.Resource // a timed cache storing volume stats volStatsCache azcache.Resource + // a timed cache storing account which should use sastoken for azcopy based volume cloning + azcopySasTokenCache azcache.Resource // sas expiry time for azcopy in volume clone sasTokenExpirationMinutes int // azcopy for provide exec mock for ut @@ -256,6 +258,9 @@ func NewDriver(options *DriverOptions) *Driver { if d.dataPlaneAPIVolCache, err = azcache.NewTimedCache(10*time.Minute, getter, false); err != nil { klog.Fatalf("%v", err) } + if d.azcopySasTokenCache, err = azcache.NewTimedCache(15*time.Minute, getter, false); err != nil { + klog.Fatalf("%v", err) + } if options.VolStatsCacheExpireInMinutes <= 0 { options.VolStatsCacheExpireInMinutes = 10 // default expire in 10 minutes diff --git a/pkg/blob/blob_test.go b/pkg/blob/blob_test.go index 3b5ddf1c2..ab6db6661 100644 --- a/pkg/blob/blob_test.go +++ b/pkg/blob/blob_test.go @@ -91,6 +91,7 @@ func TestNewDriver(t *testing.T) { fakedriver.Version = driverVersion fakedriver.accountSearchCache = driver.accountSearchCache fakedriver.dataPlaneAPIVolCache = driver.dataPlaneAPIVolCache + fakedriver.azcopySasTokenCache = driver.azcopySasTokenCache fakedriver.volStatsCache = driver.volStatsCache assert.Equal(t, driver, fakedriver) } diff --git a/pkg/blob/controllerserver.go b/pkg/blob/controllerserver.go index 4d810dfcd..7c1059958 100644 --- a/pkg/blob/controllerserver.go +++ b/pkg/blob/controllerserver.go @@ -828,18 +828,29 @@ func (d *Driver) authorizeAzcopyWithIdentity() ([]string, error) { // 3. azcopy returns AuthorizationPermissionMismatch error when using service principal or managed identity func (d *Driver) getSASToken(ctx context.Context, accountName, accountKey, storageEndpointSuffix string, accountOptions *azure.AccountOptions, secrets map[string]string, secretName, secretNamespace string) (string, error) { authAzcopyEnv, _ := d.authorizeAzcopyWithIdentity() - useSasTokenFallBack := false + useSasToken := false if len(authAzcopyEnv) > 0 { - out, testErr := d.azcopy.TestListJobs(accountName, storageEndpointSuffix, authAzcopyEnv) - if testErr != nil { - return "", fmt.Errorf("azcopy list command failed with error(%v): %v", testErr, out) + // search in cache first + cache, err := d.azcopySasTokenCache.Get(accountName, azcache.CacheReadTypeDefault) + if err != nil { + return "", fmt.Errorf("get(%s) from azcopySasTokenCache failed with error: %v", accountName, err) } - if strings.Contains(out, authorizationPermissionMismatch) { - 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) - useSasTokenFallBack = true + if cache != nil { + klog.V(2).Infof("use sas token for account(%s) since this account is found in azcopySasTokenCache", accountName) + useSasToken = true + } else { + out, testErr := d.azcopy.TestListJobs(accountName, storageEndpointSuffix, authAzcopyEnv) + if testErr != nil { + return "", fmt.Errorf("azcopy list command failed with error(%v): %v", testErr, out) + } + if strings.Contains(out, authorizationPermissionMismatch) { + 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) + d.azcopySasTokenCache.Set(accountName, "") + useSasToken = true + } } } - if len(secrets) > 0 || len(authAzcopyEnv) == 0 || useSasTokenFallBack { + if len(secrets) > 0 || len(authAzcopyEnv) == 0 || useSasToken { var err error if accountKey == "" { if _, accountKey, err = d.GetStorageAccesskey(ctx, accountOptions, secrets, secretName, secretNamespace); err != nil { diff --git a/pkg/blob/controllerserver_test.go b/pkg/blob/controllerserver_test.go index 612fec6b0..6d6e3ec6c 100644 --- a/pkg/blob/controllerserver_test.go +++ b/pkg/blob/controllerserver_test.go @@ -1789,7 +1789,7 @@ func TestCopyVolume(t *testing.T) { } } -func Test_parseDays(t *testing.T) { +func TestParseDays(t *testing.T) { type args struct { dayStr string } @@ -1838,7 +1838,7 @@ func Test_parseDays(t *testing.T) { } } -func Test_generateSASToken(t *testing.T) { +func TestGenerateSASToken(t *testing.T) { storageEndpointSuffix := "core.windows.net" tests := []struct { name string @@ -1876,7 +1876,7 @@ func Test_generateSASToken(t *testing.T) { } } -func Test_authorizeAzcopyWithIdentity(t *testing.T) { +func TestAuthorizeAzcopyWithIdentity(t *testing.T) { testCases := []struct { name string testFunc func(t *testing.T) @@ -1995,7 +1995,7 @@ func Test_authorizeAzcopyWithIdentity(t *testing.T) { } } -func Test_getSASToken(t *testing.T) { +func TestGetSASToken(t *testing.T) { testCases := []struct { name string testFunc func(t *testing.T)