From bd7d29961549bc17a0b7f9f410f77bb00912bc7a Mon Sep 17 00:00:00 2001 From: Adele Reed Date: Tue, 29 Oct 2024 06:22:49 -0700 Subject: [PATCH 1/3] Catch an object named after the root with a trailing / --- cmd/syncProcessor.go | 21 +++++++++++++------ cmd/zc_traverser_blob.go | 5 +++++ e2etest/zt_newe2e_fns_dir_test.go | 34 ++++++++++++++++++++++--------- 3 files changed, 44 insertions(+), 16 deletions(-) diff --git a/cmd/syncProcessor.go b/cmd/syncProcessor.go index 96bed1572..fc54ea5d1 100644 --- a/cmd/syncProcessor.go +++ b/cmd/syncProcessor.go @@ -278,6 +278,15 @@ func (b *remoteResourceDeleter) delete(object StoredObject) error { b.clientOptions.PerCallPolicies = append([]policy.Policy{common.NewRecursivePolicy()}, b.clientOptions.PerCallPolicies...) } */ + if object.relativePath == "\x00" { + return nil // Do nothing, we don't want to accidentally delete the root. + } + objectPath := path.Join(b.rootPath, object.relativePath) + 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 +303,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 +315,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 +329,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 +365,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 +378,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 +392,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..bc86c2602 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 { diff --git a/e2etest/zt_newe2e_fns_dir_test.go b/e2etest/zt_newe2e_fns_dir_test.go index cd34488c8..c839e46a9 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,12 +124,18 @@ 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) From 1bb9a639ab9be5f9332a9e29d679b26f13618e04 Mon Sep 17 00:00:00 2001 From: Adele Reed Date: Tue, 29 Oct 2024 08:19:42 -0700 Subject: [PATCH 2/3] Delete caught blob trailing / --- cmd/syncProcessor.go | 7 +++- e2etest/zt_newe2e_fns_dir_test.go | 62 +++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 2 deletions(-) diff --git a/cmd/syncProcessor.go b/cmd/syncProcessor.go index fc54ea5d1..22599937b 100644 --- a/cmd/syncProcessor.go +++ b/cmd/syncProcessor.go @@ -278,10 +278,13 @@ func (b *remoteResourceDeleter) delete(object StoredObject) error { b.clientOptions.PerCallPolicies = append([]policy.Policy{common.NewRecursivePolicy()}, b.clientOptions.PerCallPolicies...) } */ - if object.relativePath == "\x00" { + 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 } - objectPath := path.Join(b.rootPath, object.relativePath) + 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. diff --git a/e2etest/zt_newe2e_fns_dir_test.go b/e2etest/zt_newe2e_fns_dir_test.go index c839e46a9..6515e7a2f 100644 --- a/e2etest/zt_newe2e_fns_dir_test.go +++ b/e2etest/zt_newe2e_fns_dir_test.go @@ -140,3 +140,65 @@ func (*FNSSuite) Scenario_IncludeRootDirectoryStub(a *ScenarioVariationManager) }, }, 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) +} From a6dd6dbf291b30a64e7d1daa60b28902cd482996 Mon Sep 17 00:00:00 2001 From: Adele Reed Date: Tue, 29 Oct 2024 11:09:52 -0700 Subject: [PATCH 3/3] Add catch to seriallist --- cmd/zc_traverser_blob.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cmd/zc_traverser_blob.go b/cmd/zc_traverser_blob.go index bc86c2602..4cfeb629e 100644 --- a/cmd/zc_traverser_blob.go +++ b/cmd/zc_traverser_blob.go @@ -559,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{}