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

[release-1.23] fix: add fallback to sas token on azcopy copy command #1568

Merged
merged 1 commit into from
Sep 1, 2024
Merged

[release-1.23] fix: add fallback to sas token on azcopy copy command #1568

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 0 additions & 1 deletion deploy/example/storageclass-blob-nfs.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ metadata:
provisioner: blob.csi.azure.com
parameters:
protocol: nfs
useDataPlaneAPI: "false"
volumeBindingMode: Immediate
allowVolumeExpansion: true
mountOptions:
Expand Down
1 change: 0 additions & 1 deletion deploy/example/storageclass-blobfuse.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ metadata:
provisioner: blob.csi.azure.com
parameters:
skuName: Premium_LRS # available values: Standard_LRS, Premium_LRS, Standard_GRS, Standard_RAGRS
useDataPlaneAPI: "false"
reclaimPolicy: Delete
volumeBindingMode: Immediate
allowVolumeExpansion: true
Expand Down
1 change: 0 additions & 1 deletion deploy/example/storageclass-blobfuse2.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ provisioner: blob.csi.azure.com
parameters:
skuName: Premium_LRS # available values: Standard_LRS, Premium_LRS, Standard_GRS, Standard_RAGRS
protocol: fuse2
useDataPlaneAPI: "false"
reclaimPolicy: Delete
volumeBindingMode: Immediate
allowVolumeExpansion: true
Expand Down
36 changes: 17 additions & 19 deletions pkg/blob/controllerserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,12 +430,22 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
return nil, status.Errorf(codes.Internal, "failed to create container(%s) on account(%s) type(%s) rg(%s) location(%s) size(%d), error: %v", validContainerName, accountName, storageAccountType, resourceGroup, location, requestGiB, err)
}
if volContentSource != nil {
accountSASToken, authAzcopyEnv, err := d.getAzcopyAuth(ctx, accountName, accountKey, storageEndpointSuffix, accountOptions, secrets, secretName, secretNamespace)
accountSASToken, authAzcopyEnv, err := d.getAzcopyAuth(ctx, accountName, accountKey, storageEndpointSuffix, accountOptions, secrets, secretName, secretNamespace, false)
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, accountName, accountSASToken, authAzcopyEnv, validContainerName, secretNamespace, accountOptions, storageEndpointSuffix); err != nil {
return nil, err
var copyErr error
copyErr = d.copyVolume(ctx, req, accountName, accountSASToken, authAzcopyEnv, validContainerName, secretNamespace, accountOptions, storageEndpointSuffix)
if accountSASToken == "" && copyErr != nil && strings.Contains(copyErr.Error(), authorizationPermissionMismatch) {
klog.Warningf("azcopy copy failed with AuthorizationPermissionMismatch error, should assign \"Storage Blob Data Contributor\" role to controller identity, fall back to use sas token, original error: %v", copyErr)
accountSASToken, authAzcopyEnv, err := d.getAzcopyAuth(ctx, accountName, accountKey, storageEndpointSuffix, accountOptions, secrets, secretName, secretNamespace, true)
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to getAzcopyAuth on account(%s) rg(%s), error: %v", accountOptions.Name, accountOptions.ResourceGroup, err)
}
copyErr = d.copyVolume(ctx, req, accountName, accountSASToken, authAzcopyEnv, validContainerName, secretNamespace, accountOptions, storageEndpointSuffix)
}
if copyErr != nil {
return nil, copyErr
}
}

Expand Down Expand Up @@ -748,7 +758,7 @@ func (d *Driver) copyBlobContainer(ctx context.Context, req *csi.CreateVolumeReq
SubscriptionID: srcSubscriptionID,
GetLatestAccountKey: accountOptions.GetLatestAccountKey,
}
if srcAccountSasToken, _, err = d.getAzcopyAuth(ctx, srcAccountName, "", storageEndpointSuffix, srcAccountOptions, nil, "", secretNamespace); err != nil {
if srcAccountSasToken, _, err = d.getAzcopyAuth(ctx, srcAccountName, "", storageEndpointSuffix, srcAccountOptions, nil, "", secretNamespace, true); err != nil {
return err
}
}
Expand Down Expand Up @@ -841,12 +851,11 @@ func (d *Driver) authorizeAzcopyWithIdentity() ([]string, error) {
// 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) getAzcopyAuth(ctx context.Context, accountName, accountKey, storageEndpointSuffix string, accountOptions *azure.AccountOptions, secrets map[string]string, secretName, secretNamespace string) (string, []string, error) {
// 3. parameter useSasToken is true
func (d *Driver) getAzcopyAuth(ctx context.Context, accountName, accountKey, storageEndpointSuffix string, accountOptions *azure.AccountOptions, secrets map[string]string, secretName, secretNamespace string, useSasToken bool) (string, []string, error) {
var authAzcopyEnv []string
var err error
useSasToken := false
if !d.useDataPlaneAPI("", accountName) && len(secrets) == 0 && len(secretName) == 0 {
if !useSasToken && !d.useDataPlaneAPI("", accountName) && len(secrets) == 0 && len(secretName) == 0 {
// search in cache first
if cache, err := d.azcopySasTokenCache.Get(accountName, azcache.CacheReadTypeDefault); err == nil && cache != nil {
klog.V(2).Infof("use sas token for account(%s) since this account is found in azcopySasTokenCache", accountName)
Expand All @@ -856,17 +865,6 @@ func (d *Driver) getAzcopyAuth(ctx context.Context, accountName, accountKey, sto
authAzcopyEnv, err = d.authorizeAzcopyWithIdentity()
if err != nil {
klog.Warningf("failed to authorize azcopy with identity, error: %v", err)
} else {
if len(authAzcopyEnv) > 0 {
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)
useSasToken = true
}
}
}
}

Expand Down
14 changes: 4 additions & 10 deletions pkg/blob/controllerserver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -817,12 +817,6 @@ func TestCreateVolume(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

m := util.NewMockEXEC(ctrl)

listStr := "no error"
m.EXPECT().RunCommand(gomock.Any(), gomock.Any()).Return(listStr, nil)
d.azcopy.ExecCmd = m

expectedErr := status.Errorf(codes.NotFound, "error parsing volume id: \"unit-test\", should at least contain two #")
_, err := d.CreateVolume(context.Background(), req)
if !reflect.DeepEqual(err, expectedErr) {
Expand Down Expand Up @@ -1929,7 +1923,7 @@ func TestGetAzcopyAuth(t *testing.T) {
ctx := context.Background()
expectedAccountSASToken := ""
expectedErr := fmt.Errorf("could not find accountkey or azurestorageaccountkey field in secrets")
accountSASToken, _, err := d.getAzcopyAuth(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
accountSASToken, _, err := d.getAzcopyAuth(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace", false)
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
}
Expand All @@ -1952,7 +1946,7 @@ func TestGetAzcopyAuth(t *testing.T) {
defaultSecretAccountName: "accountName",
defaultSecretAccountKey: "YWNjb3VudGtleQo=",
}
accountSASToken, _, err := d.getAzcopyAuth(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
accountSASToken, _, err := d.getAzcopyAuth(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace", false)
if !reflect.DeepEqual(err, nil) || !strings.Contains(accountSASToken, "?se=") {
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
}
Expand All @@ -1978,7 +1972,7 @@ func TestGetAzcopyAuth(t *testing.T) {

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.getAzcopyAuth(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
accountSASToken, _, err := d.getAzcopyAuth(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace", false)
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
}
Expand All @@ -1999,7 +1993,7 @@ func TestGetAzcopyAuth(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.getAzcopyAuth(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
accountSASToken, _, err := d.getAzcopyAuth(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace", false)
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
}
Expand Down
24 changes: 8 additions & 16 deletions test/e2e/dynamic_provisioning_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -898,7 +898,6 @@ var _ = ginkgo.Describe("[blob-csi-e2e] Dynamic Provisioning", func() {
Pod: pod,
PodWithClonedVolume: podWithClonedVolume,
StorageClassParameters: map[string]string{
"useDataPlaneAPI": "true",
"skuName": "Premium_LRS",
"protocol": "nfs",
"mountPermissions": "0755",
Expand Down Expand Up @@ -931,7 +930,6 @@ var _ = ginkgo.Describe("[blob-csi-e2e] Dynamic Provisioning", func() {
Pod: pod,
PodWithClonedVolume: podWithClonedVolume,
StorageClassParameters: map[string]string{
"useDataPlaneAPI": "true",
"skuName": "Premium_LRS",
"protocol": "nfs",
"mountPermissions": "0755",
Expand Down Expand Up @@ -965,9 +963,8 @@ var _ = ginkgo.Describe("[blob-csi-e2e] Dynamic Provisioning", func() {
Pod: pod,
PodWithClonedVolume: podWithClonedVolume,
StorageClassParameters: map[string]string{
"useDataPlaneAPI": "true",
"skuName": "Standard_LRS",
"protocol": "fuse2",
"skuName": "Standard_LRS",
"protocol": "fuse2",
},
}
test.Run(ctx, cs, ns)
Expand Down Expand Up @@ -998,9 +995,8 @@ var _ = ginkgo.Describe("[blob-csi-e2e] Dynamic Provisioning", func() {
Pod: pod,
PodWithClonedVolume: podWithClonedVolume,
StorageClassParameters: map[string]string{
"useDataPlaneAPI": "true",
"skuName": "Standard_LRS",
"protocol": "fuse2",
"skuName": "Standard_LRS",
"protocol": "fuse2",
},
}
test.Run(ctx, cs, ns)
Expand Down Expand Up @@ -1030,14 +1026,12 @@ var _ = ginkgo.Describe("[blob-csi-e2e] Dynamic Provisioning", func() {
Pod: pod,
PodWithClonedVolume: podWithClonedVolume,
StorageClassParameters: map[string]string{
"useDataPlaneAPI": "true",
"skuName": "Premium_LRS",
"protocol": "nfs",
"mountPermissions": "0755",
"allowsharedkeyaccess": "true",
},
ClonedStorageClassParameters: map[string]string{
"useDataPlaneAPI": "true",
"skuName": "Standard_LRS",
"protocol": "nfs",
"mountPermissions": "0755",
Expand Down Expand Up @@ -1072,14 +1066,12 @@ var _ = ginkgo.Describe("[blob-csi-e2e] Dynamic Provisioning", func() {
Pod: pod,
PodWithClonedVolume: podWithClonedVolume,
StorageClassParameters: map[string]string{
"useDataPlaneAPI": "true",
"skuName": "Standard_LRS",
"protocol": "fuse2",
"skuName": "Standard_LRS",
"protocol": "fuse2",
},
ClonedStorageClassParameters: map[string]string{
"useDataPlaneAPI": "true",
"skuName": "Premium_LRS",
"protocol": "fuse2",
"skuName": "Premium_LRS",
"protocol": "fuse2",
},
}
test.Run(ctx, cs, ns)
Expand Down
4 changes: 0 additions & 4 deletions test/external-e2e/run.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,6 @@ setup_e2e_binaries() {
sed -i "s/blob.csi.azure.com/$DRIVER.csi.azure.com/g" deploy/example/storageclass-blobfuse.yaml
sed -i "s/blob.csi.azure.com/$DRIVER.csi.azure.com/g" deploy/example/storageclass-blobfuse2.yaml
sed -i "s/blob.csi.azure.com/$DRIVER.csi.azure.com/g" deploy/example/storageclass-blob-nfs.yaml
# workaround: use useDataPlaneAPI as true for blobfuse and nfs copy volume tests
sed -i "s/\"false\"/\"true\"/g" deploy/example/storageclass-blobfuse.yaml
sed -i "s/\"false\"/\"true\"/g" deploy/example/storageclass-blobfuse2.yaml
sed -i "s/\"false\"/\"true\"/g" deploy/example/storageclass-blob-nfs.yaml

make e2e-bootstrap
sed -i "s/csi-blob-controller/csi-$DRIVER-controller/g" deploy/example/metrics/csi-blob-controller-svc.yaml
Expand Down
Loading