feat: use cluster identity for using azcopy in volume cloning #1654

umagnus · 2024-01-03T08:59:45Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

azcopy already supports managed identity in a different way: Authorize access to blobs with AzCopy & Microsoft Entra ID | Microsoft Learn, and it supports file in the latest version.
if user only provides storage account key, then we should use sas token; otherwise we should leverage aks cluster identity(managed identity or spn) for volume cloning which is better way.
Which issue(s) this PR fixes:

Fixes #

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
tested upgrade from previous version

Special notes for your reviewer:

Release note:

none

k8s-ci-robot · 2024-01-03T08:59:55Z

Hi @umagnus. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

andyzhangx · 2024-01-03T11:07:32Z

/ok-to-test

andyzhangx · 2024-01-03T11:35:20Z

pkg/azurefile/controllerserver_test.go

@@ -2904,3 +2936,256 @@ func TestGenerateSASToken(t *testing.T) {
 		})
 	}
 }
+
+func Test_authorizeAzcopyWithIdentity(t *testing.T) {


TestAuthorizeAzcopyWithIdentity

andyzhangx · 2024-01-05T13:23:43Z

pkg/azurefile/azurefile.go

+						klog.Warningf("azcopy list failed with AuthorizationPermissionMismatch error, should assign \"Storage File Data SMB Share Elevated Contributor\" role to controller identity, fall back to use sas token, original output: %v", string(out))
+						d.azcopySasTokenCache.Set(accountName, "")
+						var accountKey, sasToken string
+						if accountKey, err = d.GetStorageAccesskey(ctx, accountOptions, secrets, secretName, secretNamespace); err != nil {


you could leverage getSASToken func to get sas token? thus don't need to duplicate code

done, give a useSasToken parameter to get sas token

andyzhangx · 2024-01-05T13:25:37Z

pkg/azurefile/azurefile.go

+						srcPath := srcPath + sasToken
+						dstPath := dstPath + sasToken
+						cmd := exec.Command("azcopy", "copy", srcPath, dstPath, "--recursive", "--check-length=false")
+						if len(authAzcopyEnv) > 0 {


remove this since it's already using sastoken

andyzhangx · 2024-01-05T13:28:21Z

pkg/azurefile/azurefile.go

 				if copyErr != nil {
+					if strings.Contains(string(out), authorizationPermissionMismatch) {


if len(accountSASToken) > 0 && strings.Contains(string(out), authorizationPermissionMismatch) {

I think it should be accountSASToken =="" which means we use identity and get permission mismatch

andyzhangx · 2024-01-05T13:29:43Z

pkg/azurefile/azurefile.go

+						if sasToken, err = generateSASToken(accountName, accountKey, storageEndpointSuffix, d.sasTokenExpirationMinutes); err != nil {
+							return err
+						}
+						srcPath := srcPath + sasToken


this is a loop, don't change srcPath, dstPath, otherwise this append may happen multiple times

andyzhangx

pls also follow this PR to refine getSASToken func:

kubernetes-sigs/blob-csi-driver#1207

umagnus · 2024-01-08T05:08:05Z

/retest

umagnus · 2024-01-08T06:29:12Z

/retest

umagnus · 2024-01-08T09:35:40Z

/retest

andyzhangx · 2024-01-08T12:19:00Z

pkg/azurefile/controllerserver.go

+
+		return authAzcopyEnv, nil
+	}
+	return []string{}, fmt.Errorf("service principle or managed identity are both not set")


neither the service principal nor the managed identity has been set

andyzhangx · 2024-01-08T12:19:41Z

pkg/azurefile/controllerserver.go

+// 4. 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
+	if len(secrets) == 0 && len(secretName) == 0 {


if !useSasToken && len(secrets) == 0 && len(secretName) == 0

andyzhangx · 2024-01-08T12:20:53Z

pkg/azurefile/azurefile.go

 			klog.V(2).Infof("azcopy job status: %s, copy percent: %s%%, error: %v", jobState, percent, err)
 			switch jobState {
 			case fileutil.AzcopyJobError, fileutil.AzcopyJobCompleted:
 				return err
 			case fileutil.AzcopyJobNotFound:
 				klog.V(2).Infof("copy fileshare %s to %s", srcFileShareName, dstFileShareName)
-				out, copyErr := exec.Command("azcopy", "copy", srcPath, dstPath, "--recursive", "--check-length=false").CombinedOutput()
+				cmd := exec.Command("azcopy", "copy", srcPath, dstPath, "--recursive", "--check-length=false")


it's better set a defaultAzcopyMountOption var to replace "--recursive", "--check-length=false"

andyzhangx · 2024-01-08T12:21:00Z

pkg/azurefile/azurefile.go

+					if sasToken, _, err = d.getAzcopyAuth(ctx, accountName, "", storageEndpointSuffix, accountOptions, secrets, secretName, secretNamespace, true); err != nil {
+						return err
+					}
+					out, copyErr = exec.Command("azcopy", "copy", srcPath+sasToken, dstPath+sasToken, "--recursive", "--check-length=false").CombinedOutput()


it's better set a defaultAzcopyMountOption var to replace "--recursive", "--check-length=false"

umagnus · 2024-01-09T03:12:40Z

/retest

andyzhangx · 2024-01-09T05:17:34Z

can you squash all commits? thanks.

umagnus · 2024-01-09T08:52:35Z

/retest

andyzhangx · 2024-01-09T11:01:34Z

/retest

andyzhangx · 2024-01-09T12:20:29Z

/retest

andyzhangx

/lgtm

k8s-ci-robot · 2024-01-09T12:23:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, umagnus

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [andyzhangx]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

andyzhangx · 2024-01-09T13:40:34Z

/retest

andyzhangx · 2024-01-10T01:36:39Z

/cherrypick release-1.29

k8s-infra-cherrypick-robot · 2024-01-10T01:37:30Z

@andyzhangx: new pull request created: #1657

In response to this:

/cherrypick release-1.29

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

andyzhangx · 2024-01-11T12:51:23Z

/cherrypick release-1.28

k8s-infra-cherrypick-robot · 2024-01-11T12:52:00Z

@andyzhangx: #1654 failed to apply on top of branch "release-1.28":

Applying: use cluster identity for azcopy
error: mode change for hack/update-mock.sh, which is not in current HEAD
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 use cluster identity for azcopy
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-1.28

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

andyzhangx · 2024-04-15T06:13:09Z

/cherrypick release-1.29

k8s-infra-cherrypick-robot · 2024-04-15T06:13:47Z

@andyzhangx: #1654 failed to apply on top of branch "release-1.29":

Applying: use cluster identity for azcopy
Using index info to reconstruct a base tree...
M	pkg/azurefile/azurefile.go
M	pkg/azurefile/controllerserver.go
M	pkg/azurefile/controllerserver_test.go
M	pkg/util/util_mock.go
M	pkg/util/util_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/util/util_test.go
Auto-merging pkg/util/util_mock.go
Auto-merging pkg/azurefile/controllerserver_test.go
Auto-merging pkg/azurefile/controllerserver.go
Auto-merging pkg/azurefile/azurefile.go
CONFLICT (content): Merge conflict in pkg/azurefile/azurefile.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 use cluster identity for azcopy
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-1.29

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 3, 2024

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 3, 2024

k8s-ci-robot requested review from gnufied and ZeroMagic January 3, 2024 09:00

andyzhangx reviewed Jan 5, 2024

View reviewed changes

andyzhangx reviewed Jan 8, 2024

View reviewed changes

use cluster identity for azcopy

b4926fc

umagnus force-pushed the use_identity_for_azcopy branch from 0785325 to b4926fc Compare January 9, 2024 05:58

andyzhangx approved these changes Jan 9, 2024

View reviewed changes

k8s-ci-robot assigned andyzhangx Jan 9, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 9, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 9, 2024

k8s-ci-robot merged commit c74f393 into kubernetes-sigs:master Jan 9, 2024
27 checks passed

k8s-infra-cherrypick-robot mentioned this pull request Jan 10, 2024

[release-1.29] feat: use cluster identity for using azcopy in volume cloning #1657

Closed

		if copyErr != nil {
		if strings.Contains(string(out), authorizationPermissionMismatch) {

feat: use cluster identity for using azcopy in volume cloning #1654

feat: use cluster identity for using azcopy in volume cloning #1654

Conversation

umagnus commented Jan 3, 2024

k8s-ci-robot commented Jan 3, 2024

andyzhangx commented Jan 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyzhangx left a comment

Choose a reason for hiding this comment

umagnus commented Jan 8, 2024

umagnus commented Jan 8, 2024

umagnus commented Jan 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umagnus commented Jan 9, 2024

andyzhangx commented Jan 9, 2024

umagnus commented Jan 9, 2024

andyzhangx commented Jan 9, 2024

andyzhangx commented Jan 9, 2024

andyzhangx left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 9, 2024

andyzhangx commented Jan 9, 2024

andyzhangx commented Jan 10, 2024

k8s-infra-cherrypick-robot commented Jan 10, 2024

andyzhangx commented Jan 11, 2024

k8s-infra-cherrypick-robot commented Jan 11, 2024

andyzhangx commented Apr 15, 2024

k8s-infra-cherrypick-robot commented Apr 15, 2024