From 25cffcdf8de61f7fe9aac8f253ba34423edddeb6 Mon Sep 17 00:00:00 2001 From: Keepers Date: Thu, 30 Mar 2023 09:56:53 -0600 Subject: [PATCH] create a new http client on every delete (#2970) in order to avoid errors cascading from graph API's populated 204 responses containing `0/r/n/r/n`, in particular in response to deletes, this change hacks in the creation of a unique http client for each delete call. This is dangerous to do, since we already know that generating clients per calls can leak consumption of system resources such as sockets. This change presumes that runtime deletes are minimally, if ever, called during standard backup and restore. The permission deletion in onedrive is the stand-out exception that might need some additional investigation. --- #### Does this PR need a docs update or release note? - [x] :no_entry: No #### Type of change - [x] :robot: Supportability/Tests #### Issue(s) * #2707 #### Test Plan - [x] :zap: Unit test - [x] :green_heart: E2E --- src/internal/connector/data_collections.go | 2 +- .../connector/exchange/api/contacts.go | 9 +++- src/internal/connector/exchange/api/events.go | 9 +++- src/internal/connector/exchange/api/mail.go | 9 +++- src/internal/connector/onedrive/drive.go | 2 + src/internal/connector/onedrive/drive_test.go | 4 +- src/internal/connector/onedrive/permission.go | 16 +++++- src/internal/connector/onedrive/restore.go | 16 +++++- .../connector/sharepoint/api/pages.go | 2 + .../connector/sharepoint/collection_test.go | 1 + src/internal/connector/sharepoint/list.go | 2 + src/internal/connector/sharepoint/restore.go | 1 + .../operations/backup_integration_test.go | 51 +++++++++++++------ 13 files changed, 100 insertions(+), 24 deletions(-) diff --git a/src/internal/connector/data_collections.go b/src/internal/connector/data_collections.go index 41489ff4a..728e3b52f 100644 --- a/src/internal/connector/data_collections.go +++ b/src/internal/connector/data_collections.go @@ -218,7 +218,7 @@ func (gc *GraphConnector) RestoreDataCollections( case selectors.ServiceExchange: status, err = exchange.RestoreExchangeDataCollections(ctx, creds, gc.Service, dest, dcs, deets, errs) case selectors.ServiceOneDrive: - status, err = onedrive.RestoreCollections(ctx, backupVersion, gc.Service, dest, opts, dcs, deets, errs) + status, err = onedrive.RestoreCollections(ctx, creds, backupVersion, gc.Service, dest, opts, dcs, deets, errs) case selectors.ServiceSharePoint: status, err = sharepoint.RestoreCollections(ctx, backupVersion, creds, gc.Service, dest, dcs, deets, errs) default: diff --git a/src/internal/connector/exchange/api/contacts.go b/src/internal/connector/exchange/api/contacts.go index c9cf824fe..90af25bac 100644 --- a/src/internal/connector/exchange/api/contacts.go +++ b/src/internal/connector/exchange/api/contacts.go @@ -60,7 +60,14 @@ func (c Contacts) DeleteContainer( ctx context.Context, user, folderID string, ) error { - err := c.stable.Client().UsersById(user).ContactFoldersById(folderID).Delete(ctx, nil) + // deletes require unique http clients + // https://github.com/alcionai/corso/issues/2707 + srv, err := newService(c.Credentials) + if err != nil { + return graph.Stack(ctx, err) + } + + err = srv.Client().UsersById(user).ContactFoldersById(folderID).Delete(ctx, nil) if err != nil { return graph.Stack(ctx, err) } diff --git a/src/internal/connector/exchange/api/events.go b/src/internal/connector/exchange/api/events.go index cfb79e80b..421e4a66b 100644 --- a/src/internal/connector/exchange/api/events.go +++ b/src/internal/connector/exchange/api/events.go @@ -62,7 +62,14 @@ func (c Events) DeleteContainer( ctx context.Context, user, calendarID string, ) error { - err := c.stable.Client().UsersById(user).CalendarsById(calendarID).Delete(ctx, nil) + // deletes require unique http clients + // https://github.com/alcionai/corso/issues/2707 + srv, err := newService(c.Credentials) + if err != nil { + return graph.Stack(ctx, err) + } + + err = srv.Client().UsersById(user).CalendarsById(calendarID).Delete(ctx, nil) if err != nil { return graph.Stack(ctx, err) } diff --git a/src/internal/connector/exchange/api/mail.go b/src/internal/connector/exchange/api/mail.go index 890276c54..095ab1525 100644 --- a/src/internal/connector/exchange/api/mail.go +++ b/src/internal/connector/exchange/api/mail.go @@ -89,7 +89,14 @@ func (c Mail) DeleteContainer( ctx context.Context, user, folderID string, ) error { - err := c.stable.Client().UsersById(user).MailFoldersById(folderID).Delete(ctx, nil) + // deletes require unique http clients + // https://github.com/alcionai/corso/issues/2707 + srv, err := newService(c.Credentials) + if err != nil { + return graph.Stack(ctx, err) + } + + err = srv.Client().UsersById(user).MailFoldersById(folderID).Delete(ctx, nil) if err != nil { return graph.Stack(ctx, err) } diff --git a/src/internal/connector/onedrive/drive.go b/src/internal/connector/onedrive/drive.go index 0d0175a0b..f443cb8c7 100644 --- a/src/internal/connector/onedrive/drive.go +++ b/src/internal/connector/onedrive/drive.go @@ -367,6 +367,8 @@ func GetAllFolders( return res, el.Failure() } +// deletes require unique http clients +// https://github.com/alcionai/corso/issues/2707 func DeleteItem( ctx context.Context, gs graph.Servicer, diff --git a/src/internal/connector/onedrive/drive_test.go b/src/internal/connector/onedrive/drive_test.go index 87c346599..1bbc28779 100644 --- a/src/internal/connector/onedrive/drive_test.go +++ b/src/internal/connector/onedrive/drive_test.go @@ -317,7 +317,9 @@ func (suite *OneDriveSuite) TestCreateGetDeleteFolder() { defer func() { for _, id := range folderIDs { - err := DeleteItem(ctx, gs, driveID, id) + // deletes require unique http clients + // https://github.com/alcionai/corso/issues/2707 + err := DeleteItem(ctx, loadTestService(t), driveID, id) if err != nil { logger.Ctx(ctx).Warnw("deleting folder", "id", id, "error", err) } diff --git a/src/internal/connector/onedrive/permission.go b/src/internal/connector/onedrive/permission.go index c1d734f40..938f6d1f5 100644 --- a/src/internal/connector/onedrive/permission.go +++ b/src/internal/connector/onedrive/permission.go @@ -11,6 +11,7 @@ import ( "github.com/alcionai/corso/src/internal/connector/graph" "github.com/alcionai/corso/src/internal/data" "github.com/alcionai/corso/src/internal/version" + "github.com/alcionai/corso/src/pkg/account" "github.com/alcionai/corso/src/pkg/path" ) @@ -88,6 +89,7 @@ func getCollectionMetadata( // Passing nil for the permissions results in just creating the folder(s). func createRestoreFoldersWithPermissions( ctx context.Context, + creds account.M365Config, service graph.Servicer, drivePath *path.DrivePath, restoreFolders []string, @@ -105,6 +107,7 @@ func createRestoreFoldersWithPermissions( err = RestorePermissions( ctx, + creds, service, drivePath.DriveID, id, @@ -169,6 +172,7 @@ func diffPermissions( // the necessary permissions on onedrive objects. func RestorePermissions( ctx context.Context, + creds account.M365Config, service graph.Servicer, driveID string, itemID string, @@ -189,7 +193,17 @@ func RestorePermissions( permAdded, permRemoved := diffPermissions(currentPermissions, meta.Permissions) for _, p := range permRemoved { - err := service.Client(). + // deletes require unique http clients + // https://github.com/alcionai/corso/issues/2707 + // this is bad citizenship, and could end up consuming a lot of + // system resources if servicers leak client connections (sockets, etc). + a, err := graph.CreateAdapter(creds.AzureTenantID, creds.AzureClientID, creds.AzureClientSecret) + if err != nil { + return graph.Wrap(ctx, err, "creating delete client") + } + + err = graph.NewService(a). + Client(). DrivesById(driveID). ItemsById(itemID). PermissionsById(p.ID). diff --git a/src/internal/connector/onedrive/restore.go b/src/internal/connector/onedrive/restore.go index 24a9e6f8e..dc1c870a3 100644 --- a/src/internal/connector/onedrive/restore.go +++ b/src/internal/connector/onedrive/restore.go @@ -18,6 +18,7 @@ import ( "github.com/alcionai/corso/src/internal/diagnostics" "github.com/alcionai/corso/src/internal/observe" "github.com/alcionai/corso/src/internal/version" + "github.com/alcionai/corso/src/pkg/account" "github.com/alcionai/corso/src/pkg/backup/details" "github.com/alcionai/corso/src/pkg/control" "github.com/alcionai/corso/src/pkg/fault" @@ -33,6 +34,7 @@ const copyBufferSize = 5 * 1024 * 1024 // RestoreCollections will restore the specified data collections into OneDrive func RestoreCollections( ctx context.Context, + creds account.M365Config, backupVersion int, service graph.Servicer, dest control.RestoreDestination, @@ -80,6 +82,7 @@ func RestoreCollections( metrics, folderMetas, err = RestoreCollection( ictx, + creds, backupVersion, service, dc, @@ -121,6 +124,7 @@ func RestoreCollections( // - error, if any besides recoverable func RestoreCollection( ctx context.Context, + creds account.M365Config, backupVersion int, service graph.Servicer, dc data.RestoreCollection, @@ -177,6 +181,7 @@ func RestoreCollection( // Create restore folders and get the folder ID of the folder the data stream will be restored in restoreFolderID, err := createRestoreFoldersWithPermissions( ctx, + creds, service, drivePath, restoreFolderElements, @@ -209,6 +214,7 @@ func RestoreCollection( itemInfo, skipped, err := restoreItem( ctx, + creds, dc, backupVersion, source, @@ -260,6 +266,7 @@ func RestoreCollection( // returns the item info, a bool (true = restore was skipped), and an error func restoreItem( ctx context.Context, + creds account.M365Config, dc data.RestoreCollection, backupVersion int, source driveSource, @@ -327,6 +334,7 @@ func restoreItem( itemInfo, err := restoreV1File( ctx, source, + creds, service, drivePath, dc, @@ -346,6 +354,7 @@ func restoreItem( itemInfo, err := restoreV6File( ctx, source, + creds, service, drivePath, dc, @@ -392,6 +401,7 @@ type fileFetcher interface { func restoreV1File( ctx context.Context, source driveSource, + creds account.M365Config, service graph.Servicer, drivePath *path.DrivePath, fetcher fileFetcher, @@ -431,6 +441,7 @@ func restoreV1File( err = RestorePermissions( ctx, + creds, service, drivePath.DriveID, itemID, @@ -445,6 +456,7 @@ func restoreV1File( func restoreV6File( ctx context.Context, source driveSource, + creds account.M365Config, service graph.Servicer, drivePath *path.DrivePath, fetcher fileFetcher, @@ -495,11 +507,11 @@ func restoreV6File( err = RestorePermissions( ctx, + creds, service, drivePath.DriveID, itemID, - meta, - ) + meta) if err != nil { return details.ItemInfo{}, clues.Wrap(err, "restoring item permissions") } diff --git a/src/internal/connector/sharepoint/api/pages.go b/src/internal/connector/sharepoint/api/pages.go index cd7c0ec65..6b5800d20 100644 --- a/src/internal/connector/sharepoint/api/pages.go +++ b/src/internal/connector/sharepoint/api/pages.go @@ -155,6 +155,8 @@ func fetchPageOptions() *betasites.ItemPagesRequestBuilderGetRequestConfiguratio // DeleteSitePage removes the selected page from the SharePoint Site // https://learn.microsoft.com/en-us/graph/api/sitepage-delete?view=graph-rest-beta +// deletes require unique http clients +// https://github.com/alcionai/corso/issues/2707 func DeleteSitePage( ctx context.Context, serv *dapi.BetaService, diff --git a/src/internal/connector/sharepoint/collection_test.go b/src/internal/connector/sharepoint/collection_test.go index e32528582..08c9e8c21 100644 --- a/src/internal/connector/sharepoint/collection_test.go +++ b/src/internal/connector/sharepoint/collection_test.go @@ -256,6 +256,7 @@ func (suite *SharePointCollectionSuite) TestRestoreLocation() { require.NoError(t, err, clues.ToCore(err)) driveID := ptr.Val(siteDrive.GetId()) + err = onedrive.DeleteItem(ctx, service, driveID, folderID) assert.NoError(t, err, clues.ToCore(err)) } diff --git a/src/internal/connector/sharepoint/list.go b/src/internal/connector/sharepoint/list.go index c0d8c7c58..fd8b42a06 100644 --- a/src/internal/connector/sharepoint/list.go +++ b/src/internal/connector/sharepoint/list.go @@ -389,6 +389,8 @@ func fetchColumnLinks( } // DeleteList removes a list object from a site. +// deletes require unique http clients +// https://github.com/alcionai/corso/issues/2707 func DeleteList( ctx context.Context, gs graph.Servicer, diff --git a/src/internal/connector/sharepoint/restore.go b/src/internal/connector/sharepoint/restore.go index 8bb1a4a32..afa7a2dd8 100644 --- a/src/internal/connector/sharepoint/restore.go +++ b/src/internal/connector/sharepoint/restore.go @@ -69,6 +69,7 @@ func RestoreCollections( case path.LibrariesCategory: metrics, _, err = onedrive.RestoreCollection( ictx, + creds, backupVersion, service, dc, diff --git a/src/internal/operations/backup_integration_test.go b/src/internal/operations/backup_integration_test.go index d7604b008..3ae3d8a72 100644 --- a/src/internal/operations/backup_integration_test.go +++ b/src/internal/operations/backup_integration_test.go @@ -1183,7 +1183,7 @@ func (suite *BackupOpIntegrationSuite) TestBackup_Run_oneDriveIncrementals() { genDests = []string{container1, container2} ) - m365, err := acct.M365Config() + creds, err := acct.M365Config() require.NoError(t, err, clues.ToCore(err)) gc, err := connector.NewGraphConnector( @@ -1215,7 +1215,7 @@ func (suite *BackupOpIntegrationSuite) TestBackup_Run_oneDriveIncrementals() { acct, path.FilesCategory, selectors.NewOneDriveRestore(owners).Selector, - m365.AzureTenantID, suite.user, driveID, destName, + creds.AzureTenantID, suite.user, driveID, destName, 2, // Use an old backup version so we don't need metadata files. 0, @@ -1299,6 +1299,7 @@ func (suite *BackupOpIntegrationSuite) TestBackup_Run_oneDriveIncrementals() { driveItem.SetFile(models.NewFile()) err = onedrive.RestorePermissions( ctx, + creds, gc.Service, driveID, *newFile.GetId(), @@ -1310,8 +1311,7 @@ func (suite *BackupOpIntegrationSuite) TestBackup_Run_oneDriveIncrementals() { EntityID: suite.user, }, }, - }, - ) + }) require.NoErrorf(t, err, "add permission to file %v", clues.ToCore(err)) }, itemsRead: 1, // .data file for newitem @@ -1325,14 +1325,14 @@ func (suite *BackupOpIntegrationSuite) TestBackup_Run_oneDriveIncrementals() { driveItem.SetFile(models.NewFile()) err = onedrive.RestorePermissions( ctx, + creds, gc.Service, driveID, *newFile.GetId(), onedrive.Metadata{ SharingMode: onedrive.SharingModeCustom, Permissions: []onedrive.UserPermission{}, - }, - ) + }) require.NoError(t, err, "add permission to file", clues.ToCore(err)) }, itemsRead: 1, // .data file for newitem @@ -1347,6 +1347,7 @@ func (suite *BackupOpIntegrationSuite) TestBackup_Run_oneDriveIncrementals() { driveItem.SetFile(models.NewFile()) err = onedrive.RestorePermissions( ctx, + creds, gc.Service, driveID, targetContainer, @@ -1358,8 +1359,7 @@ func (suite *BackupOpIntegrationSuite) TestBackup_Run_oneDriveIncrementals() { EntityID: suite.user, }, }, - }, - ) + }) require.NoError(t, err, "add permission to file", clues.ToCore(err)) }, itemsRead: 0, @@ -1374,14 +1374,14 @@ func (suite *BackupOpIntegrationSuite) TestBackup_Run_oneDriveIncrementals() { driveItem.SetFile(models.NewFile()) err = onedrive.RestorePermissions( ctx, + creds, gc.Service, driveID, targetContainer, onedrive.Metadata{ SharingMode: onedrive.SharingModeCustom, Permissions: []onedrive.UserPermission{}, - }, - ) + }) require.NoError(t, err, "add permission to file", clues.ToCore(err)) }, itemsRead: 0, @@ -1447,7 +1447,9 @@ func (suite *BackupOpIntegrationSuite) TestBackup_Run_oneDriveIncrementals() { { name: "delete file", updateUserData: func(t *testing.T) { - err = gc.Service. + // deletes require unique http clients + // https://github.com/alcionai/corso/issues/2707 + err = newDeleteServicer(t). Client(). DrivesById(driveID). ItemsById(ptr.Val(newFile.GetId())). @@ -1506,7 +1508,9 @@ func (suite *BackupOpIntegrationSuite) TestBackup_Run_oneDriveIncrementals() { name: "delete a folder", updateUserData: func(t *testing.T) { container := containerIDs[container2] - err := gc.Service. + // deletes require unique http clients + // https://github.com/alcionai/corso/issues/2707 + err = newDeleteServicer(t). Client(). DrivesById(driveID). ItemsById(container). @@ -1527,7 +1531,7 @@ func (suite *BackupOpIntegrationSuite) TestBackup_Run_oneDriveIncrementals() { acct, path.FilesCategory, selectors.NewOneDriveRestore(owners).Selector, - m365.AzureTenantID, suite.user, driveID, container3, + creds.AzureTenantID, suite.user, driveID, container3, 2, 0, fileDBF) @@ -1568,11 +1572,10 @@ func (suite *BackupOpIntegrationSuite) TestBackup_Run_oneDriveIncrementals() { incBO.Results.BackupID, kw, ms, - m365.AzureTenantID, + creds.AzureTenantID, suite.user, path.OneDriveService, - categories, - ) + categories) // do some additional checks to ensure the incremental dealt with fewer items. // +2 on read/writes to account for metadata: 1 delta and 1 path. @@ -1611,3 +1614,19 @@ func (suite *BackupOpIntegrationSuite) TestBackup_Run_sharePoint() { runAndCheckBackup(t, ctx, &bo, mb, false) checkBackupIsInManifests(t, ctx, kw, &bo, sel.Selector, suite.site, path.LibrariesCategory) } + +// --------------------------------------------------------------------------- +// helpers +// --------------------------------------------------------------------------- + +func newDeleteServicer(t *testing.T) graph.Servicer { + acct := tester.NewM365Account(t) + + m365, err := acct.M365Config() + require.NoError(t, err, clues.ToCore(err)) + + a, err := graph.CreateAdapter(acct.ID(), m365.AzureClientID, m365.AzureClientSecret) + require.NoError(t, err, clues.ToCore(err)) + + return graph.NewService(a) +}