Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: generate sas token when secret is used in volume cloning #1207

Merged
merged 2 commits into from
Jan 4, 2024
Merged

fix: generate sas token when secret is used in volume cloning #1207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a3a2f15
fix: only generate sas token when secret is used in volume cloning
andyzhangx Jan 3, 2024
c8b644f
fix: only authorize azcopy once
andyzhangx Jan 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 41 additions & 37 deletions pkg/blob/controllerserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,11 +428,11 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
}

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

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

var authAzcopyEnv []string
if accountSasToken == "" {
if authAzcopyEnv, err = d.authorizeAzcopyWithIdentity(); err != nil {
return err
}
}

timeAfter := time.After(waitForCopyTimeout)
timeTick := time.Tick(waitForCopyInterval)
srcPath := fmt.Sprintf("https://%s.blob.%s/%s%s", accountName, storageEndpointSuffix, srcContainerName, accountSasToken)
Expand Down Expand Up @@ -788,18 +781,19 @@ func (d *Driver) copyBlobContainer(_ context.Context, req *csi.CreateVolumeReque
}

// copyVolume copies a volume form volume or snapshot, snapshot is not supported now
func (d *Driver) copyVolume(ctx context.Context, req *csi.CreateVolumeRequest, accountSASToken, dstContainerName, storageEndpointSuffix string) error {
func (d *Driver) copyVolume(req *csi.CreateVolumeRequest, accountSASToken string, authAzcopyEnv []string, dstContainerName, storageEndpointSuffix string) error {
vs := req.VolumeContentSource
switch vs.Type.(type) {
case *csi.VolumeContentSource_Snapshot:
return status.Errorf(codes.InvalidArgument, "copy volume from volumeSnapshot is not supported")
case *csi.VolumeContentSource_Volume:
return d.copyBlobContainer(ctx, req, accountSASToken, dstContainerName, storageEndpointSuffix)
return d.copyBlobContainer(req, accountSASToken, authAzcopyEnv, dstContainerName, storageEndpointSuffix)
default:
return status.Errorf(codes.InvalidArgument, "%v is not a proper volume source", vs)
}
}

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

// getSASToken will only generate sas token for azcopy in following conditions:
// getAzcopyAuth will only generate sas token for azcopy in following conditions:
// 1. secrets is not empty
// 2. driver is not using managed identity and service principal
// 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()
func (d *Driver) getAzcopyAuth(ctx context.Context, accountName, accountKey, storageEndpointSuffix string, accountOptions *azure.AccountOptions, secrets map[string]string, secretName, secretNamespace string) (string, []string, error) {
var authAzcopyEnv []string
useSasToken := false
if len(authAzcopyEnv) > 0 {
// search in cache first
cache, err := d.azcopySasTokenCache.Get(accountName, azcache.CacheReadTypeDefault)
if len(secrets) == 0 && len(secretName) == 0 {
var err error
authAzcopyEnv, err = d.authorizeAzcopyWithIdentity()
if err != nil {
return "", fmt.Errorf("get(%s) from azcopySasTokenCache failed with error: %v", accountName, err)
}
if cache != nil {
klog.V(2).Infof("use sas token for account(%s) since this account is found in azcopySasTokenCache", accountName)
useSasToken = true
klog.Warningf("failed to authorize azcopy with identity, error: %v", err)
} 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(authAzcopyEnv) > 0 {
// search in cache first
cache, err := d.azcopySasTokenCache.Get(accountName, azcache.CacheReadTypeDefault)
if err != nil {
return "", nil, fmt.Errorf("get(%s) from azcopySasTokenCache failed with error: %v", accountName, err)
}
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 "", nil, 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 || useSasToken {

if len(secrets) > 0 || len(secretName) > 0 || len(authAzcopyEnv) == 0 || useSasToken {
var err error
if accountKey == "" {
if _, accountKey, err = d.GetStorageAccesskey(ctx, accountOptions, secrets, secretName, secretNamespace); err != nil {
return "", err
return "", nil, err
}
}
klog.V(2).Infof("generate sas token for account(%s)", accountName)
return generateSASToken(accountName, accountKey, storageEndpointSuffix, d.sasTokenExpirationMinutes)
sasToken, err := generateSASToken(accountName, accountKey, storageEndpointSuffix, d.sasTokenExpirationMinutes)
return sasToken, nil, err
}
return "", nil
return "", authAzcopyEnv, nil
}

// isValidVolumeCapabilities validates the given VolumeCapability array is valid
Expand Down
92 changes: 14 additions & 78 deletions pkg/blob/controllerserver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1538,10 +1538,8 @@ func TestCopyVolume(t *testing.T) {
VolumeContentSource: &volumecontensource,
}

ctx := context.Background()

expectedErr := status.Errorf(codes.InvalidArgument, "copy volume from volumeSnapshot is not supported")
err := d.copyVolume(ctx, req, "", "", "core.windows.net")
err := d.copyVolume(req, "", nil, "", "core.windows.net")
if !reflect.DeepEqual(err, expectedErr) {
t.Errorf("Unexpected error: %v", err)
}
Expand Down Expand Up @@ -1570,10 +1568,8 @@ func TestCopyVolume(t *testing.T) {
VolumeContentSource: &volumecontensource,
}

ctx := context.Background()

expectedErr := status.Errorf(codes.NotFound, "error parsing volume id: \"unit-test\", should at least contain two #")
err := d.copyVolume(ctx, req, "", "dstContainer", "core.windows.net")
err := d.copyVolume(req, "", nil, "dstContainer", "core.windows.net")
if !reflect.DeepEqual(err, expectedErr) {
t.Errorf("Unexpected error: %v", err)
}
Expand Down Expand Up @@ -1602,10 +1598,8 @@ func TestCopyVolume(t *testing.T) {
VolumeContentSource: &volumecontensource,
}

ctx := context.Background()

expectedErr := fmt.Errorf("srcContainerName() or dstContainerName(dstContainer) is empty")
err := d.copyVolume(ctx, req, "", "dstContainer", "core.windows.net")
err := d.copyVolume(req, "", nil, "dstContainer", "core.windows.net")
if !reflect.DeepEqual(err, expectedErr) {
t.Errorf("Unexpected error: %v", err)
}
Expand Down Expand Up @@ -1634,42 +1628,8 @@ func TestCopyVolume(t *testing.T) {
VolumeContentSource: &volumecontensource,
}

ctx := context.Background()

expectedErr := fmt.Errorf("srcContainerName(fileshare) or dstContainerName() is empty")
err := d.copyVolume(ctx, req, "", "", "core.windows.net")
if !reflect.DeepEqual(err, expectedErr) {
t.Errorf("Unexpected error: %v", err)
}
},
},
{
name: "AADClientSecret shouldn't be nil or useManagedIdentityExtension must be set to true when accountSASToken is empty",
testFunc: func(t *testing.T) {
d := NewFakeDriver()
mp := map[string]string{}

volumeSource := &csi.VolumeContentSource_VolumeSource{
VolumeId: "vol_1#f5713de20cde511e8ba4900#fileshare#",
}
volumeContentSourceVolumeSource := &csi.VolumeContentSource_Volume{
Volume: volumeSource,
}
volumecontensource := csi.VolumeContentSource{
Type: volumeContentSourceVolumeSource,
}

req := &csi.CreateVolumeRequest{
Name: "unit-test",
VolumeCapabilities: stdVolumeCapabilities,
Parameters: mp,
VolumeContentSource: &volumecontensource,
}

ctx := context.Background()

expectedErr := fmt.Errorf("service principle or managed identity are both not set")
err := d.copyVolume(ctx, req, "", "dstContainer", "core.windows.net")
err := d.copyVolume(req, "", nil, "", "core.windows.net")
if !reflect.DeepEqual(err, expectedErr) {
t.Errorf("Unexpected error: %v", err)
}
Expand Down Expand Up @@ -1710,10 +1670,8 @@ func TestCopyVolume(t *testing.T) {

d.azcopy.ExecCmd = m

ctx := context.Background()

var expectedErr error
err := d.copyVolume(ctx, req, "sastoken", "dstContainer", "core.windows.net")
err := d.copyVolume(req, "sastoken", nil, "dstContainer", "core.windows.net")
if !reflect.DeepEqual(err, expectedErr) {
t.Errorf("Unexpected error: %v", err)
}
Expand Down Expand Up @@ -1755,10 +1713,8 @@ func TestCopyVolume(t *testing.T) {

d.azcopy.ExecCmd = m

ctx := context.Background()

var expectedErr error
err := d.copyVolume(ctx, req, "sastoken", "dstContainer", "core.windows.net")
err := d.copyVolume(req, "sastoken", nil, "dstContainer", "core.windows.net")
if !reflect.DeepEqual(err, expectedErr) {
t.Errorf("Unexpected error: %v", err)
}
Expand Down Expand Up @@ -1989,7 +1945,7 @@ func TestAuthorizeAzcopyWithIdentity(t *testing.T) {
}
}

func TestGetSASToken(t *testing.T) {
func TestGetAzcopyAuth(t *testing.T) {
testCases := []struct {
name string
testFunc func(t *testing.T)
Expand All @@ -2008,14 +1964,14 @@ func TestGetSASToken(t *testing.T) {
ctx := context.Background()
expectedAccountSASToken := ""
expectedErr := fmt.Errorf("could not find accountkey or azurestorageaccountkey field in secrets")
accountSASToken, err := d.getSASToken(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
accountSASToken, _, err := d.getAzcopyAuth(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
}
},
},
{
name: "failed to test azcopy list command",
name: "generate sas token using account key",
testFunc: func(t *testing.T) {
d := NewFakeDriver()
d.cloud = &azure.Cloud{
Expand All @@ -2029,21 +1985,10 @@ func TestGetSASToken(t *testing.T) {
}
secrets := map[string]string{
defaultSecretAccountName: "accountName",
defaultSecretAccountKey: "YWNjb3VudGtleQo=",
}
ctrl := gomock.NewController(t)
defer ctrl.Finish()

m := util.NewMockEXEC(ctrl)
listStr := "error"
m.EXPECT().RunCommand(gomock.Any(), gomock.Any()).Return(listStr, fmt.Errorf("error"))

d.azcopy.ExecCmd = m

ctx := context.Background()
expectedAccountSASToken := ""
expectedErr := fmt.Errorf("azcopy list command failed with error(%v): %v", fmt.Errorf("error"), "error")
accountSASToken, err := d.getSASToken(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
accountSASToken, _, err := d.getAzcopyAuth(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
if !reflect.DeepEqual(err, nil) || !strings.Contains(accountSASToken, "?se=") {
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
}
},
Expand All @@ -2065,19 +2010,10 @@ func TestGetSASToken(t *testing.T) {
defaultSecretAccountName: "accountName",
defaultSecretAccountKey: "fakeValue",
}
ctrl := gomock.NewController(t)
defer ctrl.Finish()

m := util.NewMockEXEC(ctrl)
listStr := "RESPONSE 403: 403 This request is not authorized to perform this operation using this permission.\nERROR CODE: AuthorizationPermissionMismatch"
m.EXPECT().RunCommand(gomock.Any(), gomock.Any()).Return(listStr, nil)

d.azcopy.ExecCmd = m

ctx := context.Background()
expectedAccountSASToken := ""
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"))
accountSASToken, err := d.getSASToken(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
accountSASToken, _, err := d.getAzcopyAuth(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
}
Expand All @@ -2098,7 +2034,7 @@ func TestGetSASToken(t *testing.T) {
ctx := context.Background()
expectedAccountSASToken := ""
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"))
accountSASToken, err := d.getSASToken(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
accountSASToken, _, err := d.getAzcopyAuth(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
}
Expand Down
Loading