From 572a58d7ec5e12df51b23d56e62da04b8216b3de Mon Sep 17 00:00:00 2001 From: adreed-msft <49764384+adreed-msft@users.noreply.github.com> Date: Tue, 29 Oct 2024 12:48:48 -0700 Subject: [PATCH] Catch an object named after the root with a trailing `/` & carefully handle trailing `/` files on sync (#2847) --- cmd/syncProcessor.go | 24 ++++++-- cmd/zc_traverser_blob.go | 10 ++++ e2etest/zt_newe2e_fns_dir_test.go | 96 +++++++++++++++++++++++++++---- 3 files changed, 114 insertions(+), 16 deletions(-) diff --git a/cmd/syncProcessor.go b/cmd/syncProcessor.go index 96bed1572..22599937b 100644 --- a/cmd/syncProcessor.go +++ b/cmd/syncProcessor.go @@ -278,6 +278,18 @@ func (b *remoteResourceDeleter) delete(object StoredObject) error { b.clientOptions.PerCallPolicies = append([]policy.Policy{common.NewRecursivePolicy()}, b.clientOptions.PerCallPolicies...) } */ + objectPath := path.Join(b.rootPath, object.relativePath) + if object.relativePath == "\x00" && b.targetLocation != common.ELocation.Blob() { + return nil // Do nothing, we don't want to accidentally delete the root. + } else if object.relativePath == "\x00" { // this is acceptable on blob, though. Dir stubs are a thing, and they aren't necessary for normal function. + objectPath = b.rootPath + } + + if strings.HasSuffix(object.relativePath, "/") && !strings.HasSuffix(objectPath, "/") && b.targetLocation == common.ELocation.Blob() { + // If we were targeting a directory, we still need to be. path.join breaks that. + // We also want to defensively code around this, and make sure we are not putting folder// or trying to put a weird URI in to an endpoint that can't do this. + objectPath += "/" + } sc := b.remoteClient if object.entityType == common.EEntityType.File() { @@ -294,7 +306,7 @@ func (b *remoteResourceDeleter) delete(object StoredObject) error { switch b.targetLocation { case common.ELocation.Blob(): bsc, _ := sc.BlobServiceClient() - var blobClient *blob.Client = bsc.NewContainerClient(b.containerName).NewBlobClient(path.Join(b.rootPath, object.relativePath)) + var blobClient *blob.Client = bsc.NewContainerClient(b.containerName).NewBlobClient(objectPath) objURL, err = b.getObjectURL(blobClient.URL()) if err != nil { @@ -306,7 +318,7 @@ func (b *remoteResourceDeleter) delete(object StoredObject) error { _, err = blobClient.Delete(b.ctx, nil) case common.ELocation.File(): fsc, _ := sc.FileServiceClient() - fileClient := fsc.NewShareClient(b.containerName).NewRootDirectoryClient().NewFileClient(path.Join(b.rootPath, object.relativePath)) + fileClient := fsc.NewShareClient(b.containerName).NewRootDirectoryClient().NewFileClient(objectPath) objURL, err = b.getObjectURL(fileClient.URL()) if err != nil { @@ -320,7 +332,7 @@ func (b *remoteResourceDeleter) delete(object StoredObject) error { }, fileClient, b.forceIfReadOnly) case common.ELocation.BlobFS(): dsc, _ := sc.DatalakeServiceClient() - fileClient := dsc.NewFileSystemClient(b.containerName).NewFileClient(path.Join(b.rootPath, object.relativePath)) + fileClient := dsc.NewFileSystemClient(b.containerName).NewFileClient(objectPath) objURL, err = b.getObjectURL(fileClient.DFSURL()) if err != nil { @@ -356,7 +368,7 @@ func (b *remoteResourceDeleter) delete(object StoredObject) error { switch b.targetLocation { case common.ELocation.Blob(): bsc, _ := sc.BlobServiceClient() - blobClient := bsc.NewContainerClient(b.containerName).NewBlobClient(path.Join(b.rootPath, object.relativePath)) + blobClient := bsc.NewContainerClient(b.containerName).NewBlobClient(objectPath) // HNS endpoint doesn't like delete snapshots on a directory objURL, err = b.getObjectURL(blobClient.URL()) if err != nil { @@ -369,7 +381,7 @@ func (b *remoteResourceDeleter) delete(object StoredObject) error { } case common.ELocation.File(): fsc, _ := sc.FileServiceClient() - dirClient := fsc.NewShareClient(b.containerName).NewDirectoryClient(path.Join(b.rootPath, object.relativePath)) + dirClient := fsc.NewShareClient(b.containerName).NewDirectoryClient(objectPath) objURL, err = b.getObjectURL(dirClient.URL()) if err != nil { return err @@ -383,7 +395,7 @@ func (b *remoteResourceDeleter) delete(object StoredObject) error { } case common.ELocation.BlobFS(): dsc, _ := sc.DatalakeServiceClient() - directoryClient := dsc.NewFileSystemClient(b.containerName).NewDirectoryClient(path.Join(b.rootPath, object.relativePath)) + directoryClient := dsc.NewFileSystemClient(b.containerName).NewDirectoryClient(objectPath) objURL, err = b.getObjectURL(directoryClient.DFSURL()) if err != nil { return err diff --git a/cmd/zc_traverser_blob.go b/cmd/zc_traverser_blob.go index 626d328a5..4cfeb629e 100644 --- a/cmd/zc_traverser_blob.go +++ b/cmd/zc_traverser_blob.go @@ -419,6 +419,11 @@ func (t *blobTraverser) parallelList(containerClient *container.Client, containe storedObject := t.createStoredObjectForBlob(preprocessor, blobInfo, strings.TrimPrefix(*blobInfo.Name, searchPrefix), containerName) + // edge case, blob name happens to be the same as root and ends in / + if storedObject.relativePath == "" && strings.HasSuffix(storedObject.name, "/") { + storedObject.relativePath = "\x00" // Short circuit, letting the backend know we *really* meant root/. + } + if t.s2sPreserveSourceTags && blobInfo.BlobTags != nil { blobTagsMap := common.BlobTags{} for _, blobTag := range blobInfo.BlobTags.BlobTagSet { @@ -554,6 +559,11 @@ func (t *blobTraverser) serialList(containerClient *container.Client, containerN storedObject := t.createStoredObjectForBlob(preprocessor, blobInfo, relativePath, containerName) + // edge case, blob name happens to be the same as root and ends in / + if storedObject.relativePath == "" && strings.HasSuffix(storedObject.name, "/") { + storedObject.relativePath = "\x00" // Short circuit, letting the backend know we *really* meant root/. + } + // Setting blob tags if t.s2sPreserveSourceTags && blobInfo.BlobTags != nil { blobTagsMap := common.BlobTags{} diff --git a/e2etest/zt_newe2e_fns_dir_test.go b/e2etest/zt_newe2e_fns_dir_test.go index cd34488c8..6515e7a2f 100644 --- a/e2etest/zt_newe2e_fns_dir_test.go +++ b/e2etest/zt_newe2e_fns_dir_test.go @@ -15,15 +15,16 @@ func init() { } func (*FNSSuite) Scenario_CopyToOverlappableDirectoryMarker(a *ScenarioVariationManager) { + DirMeta := ResolveVariation(a, []string{"", common.POSIXFolderMeta}) tgtVerb := ResolveVariation(a, []AzCopyVerb{AzCopyVerbCopy, AzCopyVerbSync}) // Target a fns account destRm := ObjectResourceMappingFlat{ "foobar/": ResourceDefinitionObject{ ObjectProperties: ObjectProperties{ - Metadata: common.Metadata{ + Metadata: common.Iff(DirMeta != "", common.Metadata{ common.POSIXFolderMeta: pointerTo("true"), - }, + }, nil), }, Body: NewZeroObjectContentContainer(0), }, @@ -71,9 +72,9 @@ func (*FNSSuite) Scenario_CopyToOverlappableDirectoryMarker(a *ScenarioVariation }, "foobar/": ResourceDefinitionObject{ ObjectProperties: ObjectProperties{ - Metadata: common.Metadata{ + Metadata: common.Iff(DirMeta != "", common.Metadata{ common.POSIXFolderMeta: pointerTo("true"), - }, + }, nil), }, ObjectShouldExist: pointerTo(true), }, @@ -83,15 +84,22 @@ func (*FNSSuite) Scenario_CopyToOverlappableDirectoryMarker(a *ScenarioVariation // Scenario_IncludeRootDirectoryStub tests that the root directory (and sub directories) appropriately get their files picked up. func (*FNSSuite) Scenario_IncludeRootDirectoryStub(a *ScenarioVariationManager) { + DirMeta := ResolveVariation(a, []string{"", common.POSIXFolderMeta}) + dst := CreateResource[ContainerResourceManager](a, GetRootResource(a, common.ELocation.Blob()), ResourceDefinitionContainer{}) src := CreateResource[ContainerResourceManager](a, GetRootResource(a, common.ELocation.Blob()), ResourceDefinitionContainer{ Objects: ObjectResourceMappingFlat{ - "foobar": ResourceDefinitionObject{Body: NewRandomObjectContentContainer(512), ObjectProperties: ObjectProperties{Metadata: common.Metadata{"dontcopyme": pointerTo("")}}}, // Object w/ same name as root dir - "foobar/": ResourceDefinitionObject{ObjectProperties: ObjectProperties{EntityType: common.EEntityType.Folder(), Metadata: common.Metadata{"asdf": pointerTo("qwerty")}}}, // Folder w/ same name as object, add special prop to ensure + "foobar": ResourceDefinitionObject{Body: NewRandomObjectContentContainer(512), ObjectProperties: ObjectProperties{Metadata: common.Metadata{"dontcopyme": pointerTo("")}}}, // Object w/ same name as root dir + "foobar/": ResourceDefinitionObject{ + ObjectProperties: ObjectProperties{ + EntityType: common.Iff(DirMeta != "", common.EEntityType.Folder(), common.EEntityType.File()), + Metadata: common.Metadata{"asdf": pointerTo("qwerty")}, + }, + }, // Folder w/ same name as object, add special prop to ensure "foobar/foo": ResourceDefinitionObject{Body: NewZeroObjectContentContainer(0)}, "foobar/bar": ResourceDefinitionObject{Body: NewZeroObjectContentContainer(0)}, "foobar/baz": ResourceDefinitionObject{Body: NewZeroObjectContentContainer(0)}, - "foobar/folder/": ResourceDefinitionObject{ObjectProperties: ObjectProperties{EntityType: common.EEntityType.Folder()}}, + "foobar/folder/": ResourceDefinitionObject{ObjectProperties: ObjectProperties{EntityType: common.Iff(DirMeta != "", common.EEntityType.Folder(), common.EEntityType.File())}}, "foobar/folder/foobar": ResourceDefinitionObject{Body: NewZeroObjectContentContainer(0)}, }, }) @@ -116,13 +124,81 @@ func (*FNSSuite) Scenario_IncludeRootDirectoryStub(a *ScenarioVariationManager) ValidateResource(a, dst, ResourceDefinitionContainer{ Objects: ObjectResourceMappingFlat{ - "foobar": ResourceDefinitionObject{ObjectShouldExist: pointerTo(false)}, // We shouldn't have captured foobar, but foobar/ should exist as a directory. - "foobar/": ResourceDefinitionObject{ObjectProperties: ObjectProperties{EntityType: common.EEntityType.Folder(), Metadata: common.Metadata{common.POSIXFolderMeta: pointerTo("true"), "asdf": pointerTo("qwerty")}}}, + "foobar": ResourceDefinitionObject{ObjectShouldExist: pointerTo(false)}, // We shouldn't have captured foobar, but foobar/ should exist as a directory. + "foobar/": ResourceDefinitionObject{ObjectProperties: ObjectProperties{ + EntityType: common.Iff(DirMeta != "", common.EEntityType.Folder(), common.EEntityType.File()), + Metadata: common.Metadata{ + "asdf": pointerTo("qwerty"), + }, + }, + }, "foobar/foo": ResourceDefinitionObject{Body: NewZeroObjectContentContainer(0)}, "foobar/bar": ResourceDefinitionObject{Body: NewZeroObjectContentContainer(0)}, "foobar/baz": ResourceDefinitionObject{Body: NewZeroObjectContentContainer(0)}, - "foobar/folder/": ResourceDefinitionObject{ObjectProperties: ObjectProperties{EntityType: common.EEntityType.Folder()}}, + "foobar/folder/": ResourceDefinitionObject{ObjectProperties: ObjectProperties{EntityType: common.Iff(DirMeta != "", common.EEntityType.Folder(), common.EEntityType.File())}}, "foobar/folder/foobar": ResourceDefinitionObject{Body: NewZeroObjectContentContainer(0)}, }, }, false) } + +/* +Scenario_SyncTrailingSlashDeletion tests against a potential accidental deletion bug that could occur when `folder/` exists at the destination, but not the source +and `folder/` happens to have an overlapping file at `folder`. +*/ +func (*FNSSuite) Scenario_SyncTrailingSlashDeletion(a *ScenarioVariationManager) { + folderStyle := ResolveVariation(a, []common.EntityType{common.EEntityType.File(), common.EEntityType.Folder()}) + + dest := CreateResource[ContainerResourceManager](a, GetRootResource(a, common.ELocation.Blob()), ResourceDefinitionContainer{ + Objects: ObjectResourceMappingFlat{ + "foobar": ResourceDefinitionObject{ + Body: NewRandomObjectContentContainer(1024), + }, + "foobar/": ResourceDefinitionObject{ + ObjectProperties: ObjectProperties{ + EntityType: folderStyle, + }, + }, + "foobar/bar/": ResourceDefinitionObject{ + Body: NewRandomObjectContentContainer(1024), + }, + }, + }) + + src := CreateResource[ContainerResourceManager](a, GetRootResource(a, common.ELocation.Blob()), ResourceDefinitionContainer{ + Objects: ObjectResourceMappingFlat{ + "foobar": ResourceDefinitionObject{ + Body: NewRandomObjectContentContainer(1024), + }, // We don't care about anything other than the overlap. We merely want to trigger a delete op against dest's folder/. + }, + }) + + RunAzCopy(a, AzCopyCommand{ + Verb: AzCopyVerbSync, + Targets: []ResourceManager{ + src.GetObject(a, "foobar/", common.EEntityType.Folder()), + dest.GetObject(a, "foobar/", common.EEntityType.Folder()), + }, + Flags: SyncFlags{ + CopySyncCommonFlags: CopySyncCommonFlags{ + Recursive: pointerTo(true), + GlobalFlags: GlobalFlags{ + OutputType: pointerTo(common.EOutputFormat.Text()), + }, + IncludeDirectoryStubs: pointerTo(true), + }, + DeleteDestination: pointerTo(true), + }, + }) + + ValidateResource(a, dest, ResourceDefinitionContainer{ + Objects: ObjectResourceMappingFlat{ + "foobar": ResourceDefinitionObject{}, // We just care this guy exists + "foobar/": ResourceDefinitionObject{ // and this guy doesn't. + ObjectShouldExist: pointerTo(false), + }, + "foobar/bar/": ResourceDefinitionObject{ + ObjectShouldExist: pointerTo(false), + }, + }, + }, false) +}