From 8d01d1b397f19cb965015ccb820be7a6a6899fb1 Mon Sep 17 00:00:00 2001 From: Abin Simon Date: Fri, 3 Feb 2023 12:39:05 +0530 Subject: [PATCH] Disable OneDrive permissions backup by default (#2374) ## Description Since the backup permissions increases the backup time by a lot(mostly from higher number of requests and increased throttling), it was decided to disable it by default. It is still enabled in tests so as to make sure the code does not have regressions. ## Does this PR need a docs update or release note? - [ ] :white_check_mark: Yes, it's included - [ ] :clock1: Yes, but in a later PR - [x] :no_entry: No ## Type of change - [x] :sunflower: Feature - [ ] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Test - [ ] :computer: CI/Deployment - [ ] :broom: Tech Debt/Cleanup ## Issue(s) * # ## Test Plan - [ ] :muscle: Manual - [ ] :zap: Unit test - [x] :green_heart: E2E --- src/cli/backup/onedrive.go | 2 +- src/cli/backup/onedrive_integration_test.go | 16 +++- src/cli/options/options.go | 22 +++--- .../connector/graph_connector_test.go | 74 +++++++++++++++---- src/internal/connector/onedrive/collection.go | 7 +- .../connector/onedrive/collection_test.go | 6 +- .../connector/onedrive/collections_test.go | 4 +- src/internal/connector/onedrive/drive_test.go | 2 +- .../operations/backup_integration_test.go | 2 +- src/pkg/control/options.go | 4 +- 10 files changed, 101 insertions(+), 38 deletions(-) diff --git a/src/cli/backup/onedrive.go b/src/cli/backup/onedrive.go index f74f98916..9dfb20b79 100644 --- a/src/cli/backup/onedrive.go +++ b/src/cli/backup/onedrive.go @@ -79,7 +79,7 @@ func addOneDriveCommands(cmd *cobra.Command) *cobra.Command { switch cmd.Use { case createCommand: c, fs = utils.AddCommand(cmd, oneDriveCreateCmd()) - options.AddFeatureToggle(cmd, options.DisablePermissionsBackup()) + options.AddFeatureToggle(cmd, options.EnablePermissionsBackup()) c.Use = c.Use + " " + oneDriveServiceCommandCreateUseSuffix c.Example = oneDriveServiceCommandCreateExamples diff --git a/src/cli/backup/onedrive_integration_test.go b/src/cli/backup/onedrive_integration_test.go index e24cba34f..05231fd11 100644 --- a/src/cli/backup/onedrive_integration_test.go +++ b/src/cli/backup/onedrive_integration_test.go @@ -72,7 +72,13 @@ func (suite *NoBackupOneDriveIntegrationSuite) SetupSuite() { suite.m365UserID = tester.M365UserID(t) // init the repo first - suite.repo, err = repository.Initialize(ctx, suite.acct, suite.st, control.Options{}) + suite.repo, err = repository.Initialize( + ctx, + suite.acct, + suite.st, + control.Options{ + ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}, + }) require.NoError(t, err) } @@ -152,7 +158,13 @@ func (suite *BackupDeleteOneDriveIntegrationSuite) SetupSuite() { defer flush() // init the repo first - suite.repo, err = repository.Initialize(ctx, suite.acct, suite.st, control.Options{}) + suite.repo, err = repository.Initialize( + ctx, + suite.acct, + suite.st, + control.Options{ + ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}, + }) require.NoError(t, err) m365UserID := tester.M365UserID(t) diff --git a/src/cli/options/options.go b/src/cli/options/options.go index 32defc5bb..2b423836c 100644 --- a/src/cli/options/options.go +++ b/src/cli/options/options.go @@ -15,7 +15,7 @@ func Control() control.Options { opt.DisableMetrics = noStats opt.RestorePermissions = restorePermissions opt.ToggleFeatures.DisableIncrementals = disableIncrementals - opt.ToggleFeatures.DisablePermissionsBackup = disablePermissionsBackup + opt.ToggleFeatures.EnablePermissionsBackup = enablePermissionsBackup return opt } @@ -48,6 +48,8 @@ func AddGlobalOperationFlags(cmd *cobra.Command) { func AddRestorePermissionsFlag(cmd *cobra.Command) { fs := cmd.Flags() fs.BoolVar(&restorePermissions, "restore-permissions", false, "Restore permissions for files and folders") + // TODO: reveal this flag once backing up permissions becomes default + cobra.CheckErr(fs.MarkHidden("restore-permissions")) } // --------------------------------------------------------------------------- @@ -55,8 +57,8 @@ func AddRestorePermissionsFlag(cmd *cobra.Command) { // --------------------------------------------------------------------------- var ( - disableIncrementals bool - disablePermissionsBackup bool + disableIncrementals bool + enablePermissionsBackup bool ) type exposeFeatureFlag func(*pflag.FlagSet) @@ -83,15 +85,15 @@ func DisableIncrementals() func(*pflag.FlagSet) { } } -// Adds the hidden '--disable-permissions-backup' cli flag which, when -// set, disables backing up permissions. -func DisablePermissionsBackup() func(*pflag.FlagSet) { +// Adds the hidden '--enable-permissions-backup' cli flag which, when +// set, enables backing up permissions. +func EnablePermissionsBackup() func(*pflag.FlagSet) { return func(fs *pflag.FlagSet) { fs.BoolVar( - &disablePermissionsBackup, - "disable-permissions-backup", + &enablePermissionsBackup, + "enable-permissions-backup", false, - "Disable backing up item permissions for OneDrive") - cobra.CheckErr(fs.MarkHidden("disable-permissions-backup")) + "Enable backing up item permissions for OneDrive") + cobra.CheckErr(fs.MarkHidden("enable-permissions-backup")) } } diff --git a/src/internal/connector/graph_connector_test.go b/src/internal/connector/graph_connector_test.go index 1cdb0b59e..b3b55a15e 100644 --- a/src/internal/connector/graph_connector_test.go +++ b/src/internal/connector/graph_connector_test.go @@ -238,7 +238,10 @@ func (suite *GraphConnectorIntegrationSuite) TestRestoreFailsBadService() { acct, sel, dest, - control.Options{}, + control.Options{ + RestorePermissions: true, + ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}, + }, nil, ) assert.Error(t, err) @@ -312,7 +315,10 @@ func (suite *GraphConnectorIntegrationSuite) TestEmptyCollections() { suite.acct, test.sel, dest, - control.Options{RestorePermissions: true}, + control.Options{ + RestorePermissions: true, + ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}, + }, test.col, ) require.NoError(t, err) @@ -448,7 +454,15 @@ func runRestoreBackupTest( t.Logf("Selective backup of %s\n", backupSel) start = time.Now() - dcs, excludes, err := backupGC.DataCollections(ctx, backupSel, nil, control.Options{RestorePermissions: true}) + dcs, excludes, err := backupGC.DataCollections( + ctx, + backupSel, + nil, + control.Options{ + RestorePermissions: true, + ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}, + }, + ) require.NoError(t, err) // No excludes yet because this isn't an incremental backup. assert.Empty(t, excludes) @@ -568,7 +582,15 @@ func runRestoreBackupTestVersion0( backupSel := backupSelectorForExpected(t, test.service, expectedDests) start = time.Now() - dcs, excludes, err := backupGC.DataCollections(ctx, backupSel, nil, control.Options{RestorePermissions: true}) + dcs, excludes, err := backupGC.DataCollections( + ctx, + backupSel, + nil, + control.Options{ + RestorePermissions: true, + ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}, + }, + ) require.NoError(t, err) // No excludes yet because this isn't an incremental backup. assert.Empty(t, excludes) @@ -1026,7 +1048,10 @@ func (suite *GraphConnectorIntegrationSuite) TestRestoreAndBackup() { test, suite.connector.tenant, []string{suite.user}, - control.Options{RestorePermissions: true}, + control.Options{ + RestorePermissions: true, + ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}, + }, ) }) } @@ -1275,7 +1300,10 @@ func (suite *GraphConnectorIntegrationSuite) TestRestoreAndBackupVersion0() { test, suite.connector.tenant, []string{suite.user}, - control.Options{RestorePermissions: true}, + control.Options{ + RestorePermissions: true, + ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}, + }, ) }) } @@ -1391,7 +1419,10 @@ func (suite *GraphConnectorIntegrationSuite) TestMultiFolderBackupDifferentNames suite.acct, restoreSel, dest, - control.Options{RestorePermissions: true}, + control.Options{ + RestorePermissions: true, + ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}, + }, collections, ) require.NoError(t, err) @@ -1414,7 +1445,15 @@ func (suite *GraphConnectorIntegrationSuite) TestMultiFolderBackupDifferentNames backupSel := backupSelectorForExpected(t, test.service, expectedDests) t.Log("Selective backup of", backupSel) - dcs, excludes, err := backupGC.DataCollections(ctx, backupSel, nil, control.Options{RestorePermissions: true}) + dcs, excludes, err := backupGC.DataCollections( + ctx, + backupSel, + nil, + control.Options{ + RestorePermissions: true, + ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}, + }, + ) require.NoError(t, err) // No excludes yet because this isn't an incremental backup. assert.Empty(t, excludes) @@ -1446,7 +1485,7 @@ func (suite *GraphConnectorIntegrationSuite) TestPermissionsRestoreAndBackup() { table := []restoreBackupInfo{ { - name: "FilePermissionsResote", + name: "FilePermissionsRestore", service: path.OneDriveService, resource: Users, collections: []colInfo{ @@ -1677,7 +1716,10 @@ func (suite *GraphConnectorIntegrationSuite) TestPermissionsRestoreAndBackup() { test, suite.connector.tenant, []string{suite.user}, - control.Options{RestorePermissions: true}, + control.Options{ + RestorePermissions: true, + ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}, + }, ) }) } @@ -1697,7 +1739,7 @@ func (suite *GraphConnectorIntegrationSuite) TestPermissionsBackupAndNoRestore() table := []restoreBackupInfo{ { - name: "FilePermissionsResote", + name: "FilePermissionsRestore", service: path.OneDriveService, resource: Users, collections: []colInfo{ @@ -1733,7 +1775,10 @@ func (suite *GraphConnectorIntegrationSuite) TestPermissionsBackupAndNoRestore() test, suite.connector.tenant, []string{suite.user}, - control.Options{RestorePermissions: false}, + control.Options{ + RestorePermissions: true, + ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}, + }, ) }) } @@ -1769,6 +1814,9 @@ func (suite *GraphConnectorIntegrationSuite) TestRestoreAndBackup_largeMailAttac test, suite.connector.tenant, []string{suite.user}, - control.Options{RestorePermissions: true}, + control.Options{ + RestorePermissions: true, + ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}, + }, ) } diff --git a/src/internal/connector/onedrive/collection.go b/src/internal/connector/onedrive/collection.go index 2de05c073..343a8911e 100644 --- a/src/internal/connector/onedrive/collection.go +++ b/src/internal/connector/onedrive/collection.go @@ -279,10 +279,11 @@ func (oc *Collection) populateItems(ctx context.Context) { if oc.source == OneDriveSource { // Fetch metadata for the file for i := 1; i <= maxRetries; i++ { - if oc.ctrl.ToggleFeatures.DisablePermissionsBackup { + if !oc.ctrl.ToggleFeatures.EnablePermissionsBackup { // We are still writing the metadata file but with - // empty permissions as we are not sure how the - // restore will be called. + // empty permissions as we don't have a way to + // signify that the permissions was explicitly + // not added. itemMeta = io.NopCloser(strings.NewReader("{}")) itemMetaSize = 2 diff --git a/src/internal/connector/onedrive/collection_test.go b/src/internal/connector/onedrive/collection_test.go index b8e5fe446..734009d72 100644 --- a/src/internal/connector/onedrive/collection_test.go +++ b/src/internal/connector/onedrive/collection_test.go @@ -168,7 +168,7 @@ func (suite *CollectionUnitTestSuite) TestCollection() { suite, suite.testStatusUpdater(&wg, &collStatus), test.source, - control.Options{}) + control.Options{ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}}) require.NotNil(t, coll) assert.Equal(t, folderPath, coll.FullPath()) @@ -301,7 +301,7 @@ func (suite *CollectionUnitTestSuite) TestCollectionReadError() { suite, suite.testStatusUpdater(&wg, &collStatus), test.source, - control.Options{}) + control.Options{ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}}) mockItem := models.NewDriveItem() mockItem.SetId(&testItemID) @@ -372,7 +372,7 @@ func (suite *CollectionUnitTestSuite) TestCollectionDisablePermissionsBackup() { suite, suite.testStatusUpdater(&wg, &collStatus), test.source, - control.Options{ToggleFeatures: control.Toggles{DisablePermissionsBackup: true}}) + control.Options{ToggleFeatures: control.Toggles{}}) now := time.Now() mockItem := models.NewDriveItem() diff --git a/src/internal/connector/onedrive/collections_test.go b/src/internal/connector/onedrive/collections_test.go index 3316a10c5..f784bad62 100644 --- a/src/internal/connector/onedrive/collections_test.go +++ b/src/internal/connector/onedrive/collections_test.go @@ -635,7 +635,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() { testFolderMatcher{tt.scope}, &MockGraphService{}, nil, - control.Options{}) + control.Options{ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}}) err := c.UpdateCollections( ctx, @@ -1380,7 +1380,7 @@ func (suite *OneDriveCollectionsSuite) TestGet() { testFolderMatcher{anyFolder}, &MockGraphService{}, func(*support.ConnectorOperationStatus) {}, - control.Options{}, + control.Options{ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}}, ) c.drivePagerFunc = drivePagerFunc c.itemPagerFunc = itemPagerFunc diff --git a/src/internal/connector/onedrive/drive_test.go b/src/internal/connector/onedrive/drive_test.go index 0ba3ec1c2..a67c89ab1 100644 --- a/src/internal/connector/onedrive/drive_test.go +++ b/src/internal/connector/onedrive/drive_test.go @@ -462,7 +462,7 @@ func (suite *OneDriveSuite) TestOneDriveNewCollections() { testFolderMatcher{scope}, service, service.updateStatus, - control.Options{}, + control.Options{ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}}, ).Get(ctx, nil) assert.NoError(t, err) // Don't expect excludes as this isn't an incremental backup. diff --git a/src/internal/operations/backup_integration_test.go b/src/internal/operations/backup_integration_test.go index 5e9af1c46..b3ea617d9 100644 --- a/src/internal/operations/backup_integration_test.go +++ b/src/internal/operations/backup_integration_test.go @@ -1081,7 +1081,7 @@ func (suite *BackupOpIntegrationSuite) TestBackup_Run_oneDrive() { sel.Include(sel.AllData()) - bo, _, _, _, closer := prepNewTestBackupOp(t, ctx, mb, sel.Selector, control.Toggles{}) + bo, _, _, _, closer := prepNewTestBackupOp(t, ctx, mb, sel.Selector, control.Toggles{EnablePermissionsBackup: true}) defer closer() runAndCheckBackup(t, ctx, &bo, mb) diff --git a/src/pkg/control/options.go b/src/pkg/control/options.go index 6f53839ca..7348e8fa6 100644 --- a/src/pkg/control/options.go +++ b/src/pkg/control/options.go @@ -76,8 +76,8 @@ type Toggles struct { // forcing a new, complete backup of all data regardless of prior state. DisableIncrementals bool `json:"exchangeIncrementals,omitempty"` - // DisablePermissionsBackup is used to disable backups of item + // EnablePermissionsBackup is used to enable backups of item // permissions. Permission metadata increases graph api call count, // so disabling their retrieval when not needed is advised. - DisablePermissionsBackup bool `json:"disablePermissionsBackup,omitempty"` + EnablePermissionsBackup bool `json:"enablePermissionsBackup,omitempty"` }