Fix ACL copying with Blob (#2525)

* Generate a DFS service client when working with Blob as well

* Fix testing, default include directory stubs when sensible

Also removes leftover isHNStoHNS flag from Sync.

* Use HNS acct with BlobFS location

* Adapt to using HNS account instead when using BlobFS

* Fix tests

* Hopefully final test fixes?

* Revert ps cli auth tests

* Accidental leftovers of manual local testing

* Clean up other test reductions

---------

Co-authored-by: Gauri Lamunion <51212198+gapra-msft@users.noreply.github.com>

Author: adreed-msft

cmd/copy.go

@@ -686,6 +686,10 @@ func (raw rawCopyCmdArgs) cook() (CookedCopyCmdArgs, error) {
 	cooked.asSubdir = raw.asSubdir
 
 	cooked.IncludeDirectoryStubs = raw.includeDirectoryStubs
+	if cooked.preservePermissions.IsTruthy() && cooked.FromTo.From() == common.ELocation.Blob() {
+		// If a user is trying to persist from Blob storage with ACLs, they probably want directories too, because ACLs only exist in HNS.
+		cooked.IncludeDirectoryStubs = true
+	}
 
 	cooked.backupMode = raw.backupMode
 	if err = validateBackupMode(cooked.backupMode, cooked.FromTo); err != nil {

cmd/sync.go

@@ -258,9 +258,6 @@ func (raw *rawSyncCmdArgs) cook() (cookedSyncCmdArgs, error) {
 	//	return cooked, err
 	// }
 	cooked.preservePermissions = common.NewPreservePermissionsOption(isUserPersistingPermissions, raw.preserveOwner, cooked.fromTo)
-	if cooked.fromTo == common.EFromTo.BlobBlob() && cooked.preservePermissions.IsTruthy() {
-		cooked.isHNSToHNS = true // override HNS settings, since if a user is tx'ing blob->blob and copying permissions, it's DEFINITELY going to be HNS (since perms don't exist w/o HNS).
-	}
 
 	cooked.preservePOSIXProperties = raw.preservePOSIXProperties
 	if cooked.preservePOSIXProperties && !areBothLocationsPOSIXAware(cooked.fromTo) {
@@ -380,7 +377,6 @@ type cookedSyncCmdArgs struct {
 	fromTo                  common.FromTo
 	credentialInfo          common.CredentialInfo
 	s2sSourceCredentialType common.CredentialType
-	isHNSToHNS              bool // Because DFS sources and destinations are obscured, this is necessary for folder property transfers on ADLS Gen 2.
 
 	// filters
 	recursive             bool

cmd/syncEnumerator.go

@@ -55,12 +55,14 @@ func (cca *cookedSyncCmdArgs) initEnumerator(ctx context.Context) (enumerator *s
 		}
 	}
 
+	includeDirStubs := cca.fromTo.From().SupportsHnsACLs() && cca.fromTo.To().SupportsHnsACLs() && cca.preservePermissions.IsTruthy()
+
 	// TODO: enable symlink support in a future release after evaluating the implications
 	// TODO: Consider passing an errorChannel so that enumeration errors during sync can be conveyed to the caller.
 	// GetProperties is enabled by default as sync supports both upload and download.
 	// This property only supports Files and S3 at the moment, but provided that Files sync is coming soon, enable to avoid stepping on Files sync work
 	dest := cca.fromTo.To()
-	sourceTraverser, err := InitResourceTraverser(cca.source, cca.fromTo.From(), &ctx, &srcCredInfo, common.ESymlinkHandlingType.Skip(), nil, cca.recursive, true, cca.isHNSToHNS, common.EPermanentDeleteOption.None(), func(entityType common.EntityType) {
+	sourceTraverser, err := InitResourceTraverser(cca.source, cca.fromTo.From(), &ctx, &srcCredInfo, common.ESymlinkHandlingType.Skip(), nil, cca.recursive, true, includeDirStubs, common.EPermanentDeleteOption.None(), func(entityType common.EntityType) {
 		if entityType == common.EEntityType.File() {
 			atomic.AddUint64(&cca.atomicSourceFilesScanned, 1)
 		}
@@ -81,7 +83,7 @@ func (cca *cookedSyncCmdArgs) initEnumerator(ctx context.Context) (enumerator *s
 	// TODO: enable symlink support in a future release after evaluating the implications
 	// GetProperties is enabled by default as sync supports both upload and download.
 	// This property only supports Files and S3 at the moment, but provided that Files sync is coming soon, enable to avoid stepping on Files sync work
-	destinationTraverser, err := InitResourceTraverser(cca.destination, cca.fromTo.To(), &ctx, &dstCredInfo, common.ESymlinkHandlingType.Skip(), nil, cca.recursive, true, cca.isHNSToHNS, common.EPermanentDeleteOption.None(), func(entityType common.EntityType) {
+	destinationTraverser, err := InitResourceTraverser(cca.destination, cca.fromTo.To(), &ctx, &dstCredInfo, common.ESymlinkHandlingType.Skip(), nil, cca.recursive, true, includeDirStubs, common.EPermanentDeleteOption.None(), func(entityType common.EntityType) {
 		if entityType == common.EEntityType.File() {
 			atomic.AddUint64(&cca.atomicDestinationFilesScanned, 1)
 		}

cmd/zc_traverser_blob.go

@@ -256,8 +256,9 @@ func (t *blobTraverser) Traverse(preprocessor objectMorpher, processor objectPro
 		if !t.includeDeleted && (isBlob || err != nil) {
 			return err
 		}
-	} else if blobURLParts.BlobName == "" && t.preservePermissions.IsTruthy() {
-		// if the root is a container and we're copying "folders", we should persist the ACLs there too.
+	} else if blobURLParts.BlobName == "" && (t.preservePermissions.IsTruthy() || t.isDFS) {
+		// If the root is a container and we're copying "folders", we should persist the ACLs there too.
+		// For DFS, we should always include the container root.
 		if azcopyScanningLogger != nil {
 			azcopyScanningLogger.Log(common.LogDebug, "Detected the root as a container.")
 		}

common/util.go

@@ -116,7 +116,7 @@ func GetServiceClientForLocation(loc Location,
 ) (*ServiceClient, error) {
 	ret := &ServiceClient{}
 	switch loc {
-	case ELocation.BlobFS():
+	case ELocation.BlobFS(), ELocation.Blob(): // Since we always may need to interact with DFS while working with Blob, we should just attach both.
 		datalakeURLParts, err := azdatalake.ParseURL(resourceURL)
 		if err != nil {
 			return nil, err
@@ -150,9 +150,6 @@ func GetServiceClientForLocation(loc Location,
 
 		ret.dsc = dsc
 
-		// For BlobFS, we additionally create a blob client as well. We interact with both endpoints.
-		fallthrough
-	case ELocation.Blob():
 		blobURLParts, err := blob.ParseURL(resourceURL)
 		if err != nil {
 			return nil, err
@@ -162,23 +159,23 @@ func GetServiceClientForLocation(loc Location,
 		// In case we are creating a blob client for a datalake target, correct the endpoint
 		blobURLParts.Host = strings.Replace(blobURLParts.Host, ".dfs", ".blob", 1)
 		resourceURL = blobURLParts.String()
-		var o *blobservice.ClientOptions
+		var bso *blobservice.ClientOptions
 		var bsc *blobservice.Client
 		if policyOptions != nil {
-			o = &blobservice.ClientOptions{ClientOptions: *policyOptions}
+			bso = &blobservice.ClientOptions{ClientOptions: *policyOptions}
 		}
 
 		if credType.IsAzureOAuth() {
-			bsc, err = blobservice.NewClient(resourceURL, cred, o)
+			bsc, err = blobservice.NewClient(resourceURL, cred, bso)
 		} else if credType.IsSharedKey() {
 			var sharedKeyCred *blob.SharedKeyCredential
 			sharedKeyCred, err = GetBlobSharedKeyCredential()
 			if err != nil {
 				return nil, err
 			}
-			bsc, err = blobservice.NewClientWithSharedKeyCredential(resourceURL, sharedKeyCred, o)
+			bsc, err = blobservice.NewClientWithSharedKeyCredential(resourceURL, sharedKeyCred, bso)
 		} else {
-			bsc, err = blobservice.NewClientWithNoCredential(resourceURL, o)
+			bsc, err = blobservice.NewClientWithNoCredential(resourceURL, bso)
 		}
 
 		if err != nil {

e2etest/declarativeHelpers.go

@@ -460,6 +460,11 @@ func (tft TestFromTo) getValues(op Operation) []common.FromTo {
 			if op == eOperation.Sync() {
 				switch fromTo {
 				case common.EFromTo.BlobBlob(),
+					common.EFromTo.BlobFSBlob(),
+					common.EFromTo.BlobBlobFS(),
+					common.EFromTo.BlobFSBlobFS(),
+					common.EFromTo.BlobFSLocal(),
+					common.EFromTo.LocalBlobFS(),
 					common.EFromTo.FileFile(),
 					common.EFromTo.LocalBlob(),
 					common.EFromTo.BlobLocal(),
@@ -475,8 +480,7 @@ func (tft TestFromTo) getValues(op Operation) []common.FromTo {
 
 			// TODO: remove this temp block
 			// temp
-			if fromTo.From() == common.ELocation.S3() ||
-				fromTo.From() == common.ELocation.BlobFS() || fromTo.To() == common.ELocation.BlobFS() {
+			if fromTo.From() == common.ELocation.S3() {
 				continue // until we implement the declarativeResourceManagers
 			}
 

e2etest/declarativeResourceManagers.go

@@ -212,6 +212,7 @@ func (r *resourceLocal) createSourceSnapshot(a asserter) {
 
 type resourceBlobContainer struct {
 	accountType     AccountType
+	isBlobFS        bool
 	containerClient *container.Client
 	rawSasURL       *url.URL
 }
@@ -383,7 +384,7 @@ func (r *resourceBlobContainer) getParam(a asserter, stripTopDir, withSas bool,
 		uri = bURLParts.String()
 	}
 
-	if r.accountType == EAccountType.HierarchicalNamespaceEnabled() {
+	if r.isBlobFS {
 		uri = strings.ReplaceAll(uri, "blob", "dfs")
 	}
 

e2etest/declarativeRunner.go

@@ -111,16 +111,16 @@ func RunScenarios(
 	validate Validate, // TODO: do we really want the test author to have to nominate which validation should happen?  Pros: better perf of tests. Cons: they have to tell us, and if they tell us wrong test may not test what they think it tests
 	// _ interface{}, // TODO if we want it??, blockBlobsOnly or specific/all blob types
 
-// It would be a pain to list out every combo by hand,
-// In addition to the fact that not every credential type is sensible.
-// Thus, the E2E framework takes in a requested set of credential types, and applies them where sensible.
-// This allows you to make tests use OAuth only, SAS only, etc.
+	// It would be a pain to list out every combo by hand,
+	// In addition to the fact that not every credential type is sensible.
+	// Thus, the E2E framework takes in a requested set of credential types, and applies them where sensible.
+	// This allows you to make tests use OAuth only, SAS only, etc.
 	requestedCredentialTypesSrc []common.CredentialType,
 	requestedCredentialTypesDst []common.CredentialType,
 	p params,
 	hs *hooks,
 	fs testFiles,
-// TODO: do we need something here to explicitly say that we expect success or failure? For now, we are just inferring that from the elements of sourceFiles
+	// TODO: do we need something here to explicitly say that we expect success or failure? For now, we are just inferring that from the elements of sourceFiles
 	destAccountType AccountType,
 	srcAccountType AccountType,
 	scenarioSuffix string) {
@@ -140,8 +140,9 @@ func RunScenarios(
 		}
 
 		seenFromTos := make(map[common.FromTo]bool)
+		fromTos := testFromTo.getValues(op)
 
-		for _, fromTo := range testFromTo.getValues(op) {
+		for _, fromTo := range fromTos {
 			// dedupe the scenarios
 			if _, ok := seenFromTos[fromTo]; ok {
 				continue
@@ -166,9 +167,20 @@ func RunScenarios(
 					subtestName += "-" + scenarioSuffix
 				}
 
+				usedSrc, usedDst := srcAccountType, destAccountType
+				if fromTo.From() == common.ELocation.BlobFS() {
+					// switch to an account made for dfs
+					usedSrc = EAccountType.HierarchicalNamespaceEnabled()
+				}
+
+				if fromTo.To() == common.ELocation.BlobFS() {
+					// switch to an account made for dfs
+					usedDst = EAccountType.HierarchicalNamespaceEnabled()
+				}
+
 				s := scenario{
-					srcAccountType:      srcAccountType,
-					destAccountType:     destAccountType,
+					srcAccountType:      usedSrc,
+					destAccountType:     usedDst,
 					subtestName:         subtestName,
 					compactScenarioName: compactScenarioName,
 					fullScenarioName:    fullScenarioName,

e2etest/declarativeScenario.go

@@ -107,13 +107,13 @@ func (s *scenario) Run() {
 	// setup scenario
 	// First, validate the accounts make sense for the source/dests
 	if s.srcAccountType.IsBlobOnly() {
-		s.a.Assert(s.fromTo.From(), equals(), common.ELocation.Blob())
+		s.a.Assert(true, equals(), s.fromTo.From() == common.ELocation.Blob() || s.fromTo.From() == common.ELocation.BlobFS())
 	}
 
-	if s.destAccountType.IsBlobOnly() {
+	if s.destAccountType.IsManagedDisk() {
 		s.a.Assert(s.destAccountType, notEquals(), EAccountType.StdManagedDisk(), "Upload is not supported in MD testing yet")
 		s.a.Assert(s.destAccountType, notEquals(), EAccountType.OAuthManagedDisk(), "Upload is not supported in MD testing yet")
-		s.a.Assert(s.fromTo.To(), equals(), common.ELocation.Blob())
+		s.a.Assert(true, equals(), s.fromTo.From() == common.ELocation.Blob() || s.fromTo.From() == common.ELocation.BlobFS())
 	}
 
 	// setup
@@ -261,7 +261,7 @@ func (s *scenario) assignSourceAndDest() {
 			return &resourceLocal{common.Iff[string](s.p.destNull && !isSourceAcc, common.Dev_Null, "")}
 		case common.ELocation.File():
 			return &resourceAzureFileShare{accountType: accType}
-		case common.ELocation.Blob():
+		case common.ELocation.Blob(), common.ELocation.BlobFS():
 			// TODO: handle the multi-container (whole account) scenario
 			// TODO: handle wider variety of account types
 			if accType.IsManagedDisk() {
@@ -270,10 +270,7 @@ func (s *scenario) assignSourceAndDest() {
 				return &resourceManagedDisk{config: *mdCfg}
 			}
 
-			return &resourceBlobContainer{accountType: accType}
-		case common.ELocation.BlobFS():
-			s.a.Error("Not implemented yet for blob FS")
-			return &resourceDummy{}
+			return &resourceBlobContainer{accountType: accType, isBlobFS: loc == common.ELocation.BlobFS()}
 		case common.ELocation.S3():
 			s.a.Error("Not implemented yet for S3")
 			return &resourceDummy{}
@@ -484,7 +481,7 @@ func (s *scenario) getTransferInfo() (srcRoot string, dstRoot string, expectFold
 	expectFolders = (s.fromTo.From().IsFolderAware() &&
 		s.fromTo.To().IsFolderAware() &&
 		s.p.allowsFolderTransfers()) ||
-		(s.p.preserveSMBPermissions && s.FromTo() == common.EFromTo.BlobBlob()) ||
+		(s.p.preserveSMBPermissions && s.FromTo().From().SupportsHnsACLs() && s.FromTo().To().SupportsHnsACLs()) ||
 		(s.p.preservePOSIXProperties && (s.FromTo() == common.EFromTo.LocalBlob() || s.FromTo() == common.EFromTo.BlobBlob() || s.FromTo() == common.EFromTo.BlobLocal()))
 	expectRootFolder := expectFolders
 
@@ -583,7 +580,7 @@ func (s *scenario) validateProperties() {
 		// validate all the different things
 		s.validatePOSIXProperties(f, actual.nameValueMetadata)
 		s.validateSymlink(f, actual.nameValueMetadata)
-		s.validateMetadata(expected.nameValueMetadata, actual.nameValueMetadata)
+		s.validateMetadata(f, expected.nameValueMetadata, actual.nameValueMetadata)
 		s.validateBlobTags(expected.blobTags, actual.blobTags)
 		s.validateContentHeaders(expected.contentHeaders, actual.contentHeaders)
 		s.validateCreateTime(expected.creationTime, actual.creationTime)
@@ -733,20 +730,22 @@ func metadataWithProperCasing(original map[string]*string) map[string]*string {
 }
 
 // // Individual property validation routines
-func (s *scenario) validateMetadata(expected, actual map[string]*string) {
+func (s *scenario) validateMetadata(f *testObject, expected, actual map[string]*string) {
+	cased := metadataWithProperCasing(actual)
+
 	for _, v := range common.AllLinuxProperties { // properties are evaluated elsewhere
 		delete(expected, v)
-		delete(actual, v)
+		delete(cased, v)
 	}
 
-	s.a.Assert(len(actual), equals(), len(expected), "Both should have same number of metadata entries")
-	cased := metadataWithProperCasing(actual)
+	s.a.Assert(len(cased), equals(), len(expected), "Both should have same number of metadata entries")
+
 	for key := range expected {
 		exValue := expected[key]
 		actualValue, ok := cased[key]
-		s.a.Assert(ok, equals(), true, fmt.Sprintf("expect key '%s' to be found in destination metadata", key))
+		s.a.Assert(ok, equals(), true, fmt.Sprintf("%s: expect key '%s' to be found in destination metadata", f.name, key))
 		if ok {
-			s.a.Assert(exValue, equals(), actualValue, fmt.Sprintf("Expect value for key '%s' to be '%s' but found '%s'", key, *exValue, *actualValue))
+			s.a.Assert(exValue, equals(), actualValue, fmt.Sprintf("%s: Expect value for key '%s' to be '%s' but found '%s'", f.name, key, *exValue, *actualValue))
 		}
 	}
 }

e2etest/zt_basic_cli_ps_auth_test.go

@@ -17,7 +17,6 @@
 // LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
 // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
 // THE SOFTWARE.
-
 package e2etest
 
 import (

e2etest/zt_basic_copy_sync_remove_test.go

@@ -368,7 +368,7 @@ func TestBasic_CopyRemoveFolderHNS(t *testing.T) {
 		desc:      "AllRemove",
 		useAllTos: true,
 		froms: []common.Location{
-			common.ELocation.Blob(), // blobfs isn't technically supported; todo: support it properly rather than jank through Blob
+			common.ELocation.BlobFS(),
 		},
 		tos: []common.Location{
 			common.ELocation.Unknown(),
@@ -419,8 +419,19 @@ func TestBasic_CopyRemoveFolderHNS(t *testing.T) {
 }
 
 func TestBasic_CopyRemoveContainer(t *testing.T) {
+	allButBfsRemove := TestFromTo{
+		desc:      "AllRemove",
+		useAllTos: true,
+		froms: []common.Location{
+			common.ELocation.Blob(), // If you have a container-level SAS and a HNS account, you can't delete the container. HNS should not be included here.
+			common.ELocation.File(),
+		},
+		tos: []common.Location{
+			common.ELocation.Unknown(),
+		},
+	}
 
-	RunScenarios(t, eOperation.Remove(), eTestFromTo.AllRemove(), eValidate.Auto(), anonymousAuthOnly, anonymousAuthOnly, params{
+	RunScenarios(t, eOperation.Remove(), allButBfsRemove, eValidate.Auto(), anonymousAuthOnly, anonymousAuthOnly, params{
 		recursive:          true,
 		relativeSourcePath: "",
 	}, nil, testFiles{
@@ -438,7 +449,7 @@ func TestBasic_CopyRemoveContainerHNS(t *testing.T) {
 		desc:      "AllRemove",
 		useAllTos: true,
 		froms: []common.Location{
-			common.ELocation.Blob(), // blobfs isn't technically supported; todo: support it properly rather than jank through Blob
+			common.ELocation.BlobFS(),
 		},
 		tos: []common.Location{
 			common.ELocation.Unknown(),
@@ -472,7 +483,6 @@ func TestBasic_CopyRemoveContainerHNS(t *testing.T) {
 				_, err = fsURL.GetAccessControl(ctx, nil)
 				a.Assert(err, notEquals(), nil)
 				a.Assert(datalakeerror.HasCode(err, "FilesystemNotFound"), equals(), true)
-
 			},
 		},
 		testFiles{
@@ -869,7 +879,7 @@ func TestBasic_OverwriteHNSDirWithChildren(t *testing.T) {
 	RunScenarios(
 		t,
 		eOperation.Copy(),
-		eTestFromTo.Other(common.EFromTo.LocalBlobFS()),
+		eTestFromTo.Other(common.EFromTo.BlobFSBlobFS()),
 		eValidate.Auto(),
 		anonymousAuthOnly,
 		anonymousAuthOnly,
@@ -1066,7 +1076,7 @@ func TestBasic_SyncRemoveFoldersHNS(t *testing.T) {
 	RunScenarios(
 		t,
 		eOperation.Sync(),
-		eTestFromTo.Other(common.EFromTo.BlobBlob()),
+		eTestFromTo.Other(common.EFromTo.BlobFSBlobFS()),
 		eValidate.Auto(),
 		anonymousAuthOnly,
 		anonymousAuthOnly,

e2etest/zt_enumeration_filter_test.go

@@ -162,21 +162,6 @@ func TestFilter_RemoveFolder(t *testing.T) {
 	}, EAccountType.Standard(), EAccountType.Standard(), "")
 }
 
-func TestFilter_RemoveContainer(t *testing.T) {
-
-	RunScenarios(t, eOperation.Remove(), eTestFromTo.AllRemove(), eValidate.Auto(), anonymousAuthOnly, anonymousAuthOnly, params{
-		recursive:          true,
-		relativeSourcePath: "",
-	}, nil, testFiles{
-		defaultSize: "1K",
-		shouldTransfer: []interface{}{
-			"file1.txt",
-			"folder1/file11.txt",
-			"folder1/file12.txt",
-		},
-	}, EAccountType.Standard(), EAccountType.Standard(), "")
-}
-
 func TestFilter_ExcludePath(t *testing.T) {
 	RunScenarios(t, eOperation.Copy(), eTestFromTo.AllSourcesToOneDest(), eValidate.Auto(), anonymousAuthOnly, anonymousAuthOnly, params{
 		recursive:   true,

e2etest/zt_preserve_properties_test.go

@@ -57,7 +57,7 @@ func TestProperties_NameValueMetadataCanBeUploaded(t *testing.T) {
 }
 
 func TestProperties_HNSACLs(t *testing.T) {
-	RunScenarios(t, eOperation.CopyAndSync(), eTestFromTo.Other(common.EFromTo.BlobBlob()), eValidate.Auto(), anonymousAuthOnly, anonymousAuthOnly, params{
+	RunScenarios(t, eOperation.CopyAndSync(), eTestFromTo.Other(common.EFromTo.BlobBlob(), common.EFromTo.BlobFSBlobFS(), common.EFromTo.BlobBlobFS(), common.EFromTo.BlobFSBlob()), eValidate.Auto(), anonymousAuthOnly, anonymousAuthOnly, params{
 		recursive:              true,
 		preserveSMBPermissions: true, // this flag is deprecated, but still held over to avoid breaking.
 	}, nil, testFiles{