From e442088253c54b125a8df2a3970fe8875aebeee4 Mon Sep 17 00:00:00 2001 From: Nicolas Humbert Date: Mon, 22 May 2023 11:54:00 -0400 Subject: [PATCH] BB-401 Incompatibility of ExpiredObjectDeleteMarker logic with AWS Backbeat performs automatic ExpiredObjectDeleteMarker cleanup for compatibility with Amazon S3, - either when the delete markers meet the age criteria - or when the ExpiredObjectDeleteMarker tag is set to true. Furthermore, the process of "verifying whether the deletion marker was generated by a lifecycle service account" is ineffective due to the implementation of the assumeRole logic. Consequently, this verification will consistently yield false results, leading us to remove it. --- extensions/lifecycle/tasks/LifecycleTask.js | 25 ++-- tests/unit/lifecycle/LifecycleTask.spec.js | 124 +++++++++++++------- 2 files changed, 88 insertions(+), 61 deletions(-) diff --git a/extensions/lifecycle/tasks/LifecycleTask.js b/extensions/lifecycle/tasks/LifecycleTask.js index d8ab44fa9..e0f4d6ea2 100644 --- a/extensions/lifecycle/tasks/LifecycleTask.js +++ b/extensions/lifecycle/tasks/LifecycleTask.js @@ -25,12 +25,6 @@ const transitionOneDayEarlier = process.env.TRANSITION_ONE_DAY_EARLIER === 'true // moves lifecycle expiration deadlines 1 day earlier, mostly for testing const expireOneDayEarlier = process.env.EXPIRE_ONE_DAY_EARLIER === 'true'; -function isLifecycleUser(canonicalID) { - const canonicalIDArray = canonicalID.split('/'); - const serviceName = canonicalIDArray[canonicalIDArray.length - 1]; - return serviceName === 'lifecycle'; -} - /** * compare 2 version by their stale dates returning: * - LT (-1) if v1 is less than v2 @@ -1074,24 +1068,21 @@ class LifecycleTask extends BackbeatTask { const eodm = rules.Expiration && rules.Expiration.ExpiredObjectDeleteMarker; + // Backbeat performs automatic ExpiredObjectDeleteMarker cleanup + // for compatibility with Amazon S3, + // - either when the delete markers meet the age criteria + // - or when the ExpiredObjectDeleteMarker tag is set to true. const applicableExpRule = rules.Expiration && ( (rules.Expiration.Days !== undefined && daysSinceInitiated >= rules.Expiration.Days) || (rules.Expiration.Date !== undefined && rules.Expiration.Date < Date.now()) - || eodm !== false - ); - const validLifecycleUserCase = ( - isLifecycleUser(deleteMarker.Owner.ID) - && eodm !== false + || eodm === true ); - // if there are no other versions with the same Key as this DM, - // if a valid Expiration rule exists or if the DM was created - // by a lifecycle service account and eodm rule is not - // explicitly set to false, apply and permanently delete this DM - if (matchingNoncurrentKeys.length === 0 && (applicableExpRule - || validLifecycleUserCase)) { + // if there are no other versions with the same Key as this DM and + // if a valid Expiration rule exists, apply and permanently delete this DM + if (matchingNoncurrentKeys.length === 0 && applicableExpRule) { const entry = ActionQueueEntry.create('deleteObject') .addContext({ origin: 'lifecycle', diff --git a/tests/unit/lifecycle/LifecycleTask.spec.js b/tests/unit/lifecycle/LifecycleTask.spec.js index b42bb016e..b5945507b 100644 --- a/tests/unit/lifecycle/LifecycleTask.spec.js +++ b/tests/unit/lifecycle/LifecycleTask.spec.js @@ -304,24 +304,27 @@ describe('lifecycle task helper methods', () => { describe('_checkAndApplyEODMRule', () => { let lct2; + const oldLastModified = new Date(Date.now() - (2 * DAY)).toISOString(); + const lastModified = new Date(Date.now()).toISOString(); + const bucketData = { target: { owner: 'test-user', bucket: 'test-bucket', }, }; - // lifecycle service account created delete marker - const deleteMarkerLC = { + // user created delete marker + const deleteMarker = { Owner: { - DisplayName: 'Lifecycle Service Account', - ID: '86346e5bda4c2158985574c9942089c36ca650dc509/lifecycle', + DisplayName: 'Not Lifecycle Service Account', + ID: '86346e5bda4c2158985574c9942089c36ca650dc509', }, Key: 'test-key', VersionId: '834373731313631393339313839393939393952473030312020353820', + LastModified: lastModified, }; - // user created delete marker - const deleteMarkerNotLC = { + const deleteMarkerOld = { Owner: { DisplayName: 'Not Lifecycle Service Account', ID: '86346e5bda4c2158985574c9942089c36ca650dc509', @@ -329,6 +332,7 @@ describe('lifecycle task helper methods', () => { Key: 'test-key', VersionId: '834373731313631393339313839393939393952473030312020353820', + LastModified: oldLastModified, }; const listOfVersions = [ { @@ -362,100 +366,132 @@ describe('lifecycle task helper methods', () => { lct2.reset(); }); - it('should send an entry to Kafka when ExpiredObjectDeleteMarker is ' + - 'enabled and delete marker was created by the lifecycle service ' + - 'account', () => { + it('should NOT send any entry to Kafka when delete marker is not eligible based on its age', () => { const rules = { - Expiration: { ExpiredObjectDeleteMarker: true }, + Expiration: { Days: 5 }, }; - lct2._checkAndApplyEODMRule(bucketData, deleteMarkerLC, + lct2._checkAndApplyEODMRule(bucketData, deleteMarker, listOfVersions, rules, fakeLogger, err => { assert.ifError(err); const latestEntry = lct2.getLatestEntry(); - const expectedTarget = Object.assign({}, bucketData.target, { - key: deleteMarkerLC.Key, - version: deleteMarkerLC.VersionId, - }); - assert.strictEqual(latestEntry.getActionType(), 'deleteObject'); - assert.deepStrictEqual( - latestEntry.getAttribute('target'), expectedTarget); + assert.equal(latestEntry, undefined); }); }); - it('should send an entry to Kafka when the delete marker was created ' + - 'by the lifecycle service account and ExpiredObjectDeleteMarker rule ' + - 'was NOT explicitly set to false', () => { - const rules = {}; + it('should send any entry to Kafka when delete marker meets the age criteria and ' + + 'ExpiredObjectDeleteMarker is not set', () => { + const rules = { + Expiration: { Days: 1 }, + }; - lct2._checkAndApplyEODMRule(bucketData, deleteMarkerLC, + lct2._checkAndApplyEODMRule(bucketData, deleteMarkerOld, listOfVersions, rules, fakeLogger, err => { assert.ifError(err); const latestEntry = lct2.getLatestEntry(); const expectedTarget = Object.assign({}, bucketData.target, { - key: deleteMarkerLC.Key, - version: deleteMarkerLC.VersionId, + key: deleteMarkerOld.Key, + version: deleteMarkerOld.VersionId, }); + assert(latestEntry, 'entry has not been sent'); assert.strictEqual(latestEntry.getActionType(), 'deleteObject'); assert.deepStrictEqual( latestEntry.getAttribute('target'), expectedTarget); }); }); - it('should NOT send any entry to Kafka when Expiration rule is not ' + - 'enabled and the delete marker was not created by the lifecycle ' + - 'service account', () => { - const rules = {}; + it('should send any entry to Kafka when delete marker meets the age criteria and ' + + 'ExpiredObjectDeleteMarker is set to false', () => { + const rules = { + Expiration: { Days: 1, ExpiredObjectDeleteMarker: false }, + }; - lct2._checkAndApplyEODMRule(bucketData, deleteMarkerNotLC, + lct2._checkAndApplyEODMRule(bucketData, deleteMarkerOld, listOfVersions, rules, fakeLogger, err => { assert.ifError(err); const latestEntry = lct2.getLatestEntry(); - assert.equal(latestEntry, undefined); + const expectedTarget = Object.assign({}, bucketData.target, { + key: deleteMarkerOld.Key, + version: deleteMarkerOld.VersionId, + }); + assert(latestEntry, 'entry has not been sent'); + assert.strictEqual(latestEntry.getActionType(), 'deleteObject'); + assert.deepStrictEqual( + latestEntry.getAttribute('target'), expectedTarget); }); }); - it('should NOT send an entry to Kafka when the delete marker was ' + - 'created by the lifecycle service account but ' + - 'ExpiredObjectDeleteMarker rule is explicitly set to false', () => { + it('should send an entry to Kafka when ExpiredObjectDeleteMarker is enabled', () => { const rules = { - Expiration: { ExpiredObjectDeleteMarker: false }, + Expiration: { ExpiredObjectDeleteMarker: true }, }; - lct2._checkAndApplyEODMRule(bucketData, deleteMarkerLC, + lct2._checkAndApplyEODMRule(bucketData, deleteMarker, listOfVersions, rules, fakeLogger, err => { assert.ifError(err); const latestEntry = lct2.getLatestEntry(); - assert.equal(latestEntry, undefined); + const expectedTarget = Object.assign({}, bucketData.target, { + key: deleteMarker.Key, + version: deleteMarker.VersionId, + }); + assert(latestEntry, 'entry has not been sent'); + assert.strictEqual(latestEntry.getActionType(), 'deleteObject'); + assert.deepStrictEqual( + latestEntry.getAttribute('target'), expectedTarget); }); }); it('should send an entry to Kafka when ExpiredObjectDeleteMarker is ' + - 'enabled and delete marker was not created by the lifecycle service ' + - 'account', () => { + 'enabled and delete marker is not eligible based on its age', () => { const rules = { - Expiration: { ExpiredObjectDeleteMarker: true }, + Expiration: { Days: 5, ExpiredObjectDeleteMarker: true }, }; - lct2._checkAndApplyEODMRule(bucketData, deleteMarkerNotLC, + lct2._checkAndApplyEODMRule(bucketData, deleteMarker, listOfVersions, rules, fakeLogger, err => { assert.ifError(err); const latestEntry = lct2.getLatestEntry(); - const expectedTarget = Object.assign({}, bucketData.target, { - key: deleteMarkerNotLC.Key, - version: deleteMarkerNotLC.VersionId, + key: deleteMarker.Key, + version: deleteMarker.VersionId, }); + assert(latestEntry, 'entry has not been sent'); assert.strictEqual(latestEntry.getActionType(), 'deleteObject'); assert.deepStrictEqual( latestEntry.getAttribute('target'), expectedTarget); }); }); + + it('should NOT send an entry to Kafka when no Expiration rule is set', () => { + const rules = {}; + + lct2._checkAndApplyEODMRule(bucketData, deleteMarker, + listOfVersions, rules, fakeLogger, err => { + assert.ifError(err); + + const latestEntry = lct2.getLatestEntry(); + assert.equal(latestEntry, undefined); + }); + }); + + it('should NOT send an entry to Kafka if ExpiredObjectDeleteMarker rule is explicitly set to false', () => { + const rules = { + Expiration: { ExpiredObjectDeleteMarker: false }, + }; + + lct2._checkAndApplyEODMRule(bucketData, deleteMarker, + listOfVersions, rules, fakeLogger, err => { + assert.ifError(err); + + const latestEntry = lct2.getLatestEntry(); + assert.equal(latestEntry, undefined); + }); + }); }); describe('_rulesHaveTag', () => {