From 3985e2a712560ce712fd79b5e8d9ef8bf05b72bb Mon Sep 17 00:00:00 2001 From: Jonathan Gramain Date: Tue, 10 Oct 2023 16:32:38 -0700 Subject: [PATCH 1/2] bf: CLDSRV-458 fix bucketd params on null version update On in-place updates of "legacy" null versions (those without the "isNull2" attribute, using the "nullVersionId" chain instead of null keys), we mustn't pass the "isNull" query parameter when sending the update request to bucketd. Otherwise, it creates a null key which causes issues when deleting the null version later. Use a helper to pass the right set of parameters in all request types that update versions in-place. --- lib/api/apiUtils/object/versioning.js | 99 +++++++---- lib/api/objectDeleteTagging.js | 10 +- lib/api/objectPutACL.js | 10 +- lib/api/objectPutLegalHold.js | 10 +- lib/api/objectPutRetention.js | 10 +- lib/api/objectPutTagging.js | 10 +- .../versioning/legacyNullVersionCompat.js | 156 ++++++++++++++++++ tests/unit/api/apiUtils/versioning.js | 63 +++++++ 8 files changed, 292 insertions(+), 76 deletions(-) create mode 100644 tests/functional/aws-node-sdk/test/versioning/legacyNullVersionCompat.js diff --git a/lib/api/apiUtils/object/versioning.js b/lib/api/apiUtils/object/versioning.js index 98174dd6dd..aa7645e012 100644 --- a/lib/api/apiUtils/object/versioning.js +++ b/lib/api/apiUtils/object/versioning.js @@ -360,60 +360,86 @@ function versioningPreprocessing(bucketName, bucketMD, objectKey, objMD, }); } +/** Return options to pass to Metadata layer for version-specific + * operations with the given requested version ID + * + * @param {object} objectMD - object metadata + * @param {boolean} nullVersionCompatMode - if true, behaves in null + * version compatibility mode + * @return {object} options object with params: + * {string} [options.versionId] - specific versionId to update + * {boolean} [options.isNull=true|false|undefined] - if set, tells the + * Metadata backend if we're updating or deleting a new-style null + * version (stored in master or null key), or not a null version. + */ +function getVersionSpecificMetadataOptions(objectMD, nullVersionCompatMode) { + // Use the internal versionId if it is a "real" null version (not + // non-versioned) + // + // If the target object is non-versioned: do not specify a + // "versionId" attribute nor "isNull" + // + // If the target version is a null version, i.e. has the "isNull" + // attribute: + // + // - send the "isNull=true" param to Metadata if the version is + // already a null key put by a non-compat mode Cloudserver, to + // let Metadata know that the null key is to be updated or + // deleted. This is the case if the "isNull2" metadata attribute + // exists + // + // - otherwise, do not send the "isNull" parameter to hint + // Metadata that it is a legacy null version + // + // If the target version is not a null version and is versioned: + // + // - send the "isNull=false" param to Metadata in non-compat + // mode (mandatory for v1 format) + // + // - otherwise, do not send the "isNull" parameter to hint + // Metadata that an existing null version may not be stored in a + // null key + // + // + if (objectMD.versionId === undefined) { + return {}; + } + const options = { versionId: objectMD.versionId }; + if (objectMD.isNull) { + if (objectMD.isNull2) { + options.isNull = true; + } + } else if (!nullVersionCompatMode) { + options.isNull = false; + } + return options; +} + /** preprocessingVersioningDelete - return versioning information for S3 to * manage deletion of objects and versions, including creation of delete markers * @param {string} bucketName - name of bucket * @param {object} bucketMD - bucket metadata * @param {object} objectMD - obj metadata * @param {string} [reqVersionId] - specific version ID sent as part of request - * @param {boolean} nullVersionCompatMode - if true, behaves in null - * version compatibility mode and return appropriate values: - * - in normal mode, returns an 'isNull' boolean sent to Metadata (true or false) - * - in compatibility mode, does not return an 'isNull' property + * @param {boolean} nullVersionCompatMode - if true, behaves in null version compatibility mode * @return {object} options object with params: * {boolean} [options.deleteData=true|undefined] - whether to delete data (if undefined * means creating a delete marker instead) * {string} [options.versionId] - specific versionId to delete * {boolean} [options.isNull=true|false|undefined] - if set, tells the - * Metadata backend if we're deleting a null version or not a null - * version. Not set if `nullVersionCompatMode` is true. + * Metadata backend if we're deleting a new-style null version (stored + * in master or null key), or not a null version. */ function preprocessingVersioningDelete(bucketName, bucketMD, objectMD, reqVersionId, nullVersionCompatMode) { - const options = {}; + let options = {}; + if (bucketMD.getVersioningConfiguration() && reqVersionId) { + options = getVersionSpecificMetadataOptions(objectMD, nullVersionCompatMode); + } if (!bucketMD.getVersioningConfiguration() || reqVersionId) { // delete data if bucket is non-versioned or the request // deletes a specific version options.deleteData = true; } - if (bucketMD.getVersioningConfiguration() && reqVersionId) { - if (reqVersionId === 'null') { - // deleting the 'null' version if it exists: - // - // - use its internal versionId if it is a "real" null - // version (not non-versioned) - // - // - send the "isNull" param to Metadata if: - // - // - in non-compat mode (mandatory for v1 format) - // - // - OR if the version is already a null key put by a - // non-compat mode Cloudserver, to let Metadata know that - // the null key is to be deleted. This is the case if the - // "isNull2" param is set. - if (objectMD.versionId !== undefined) { - options.versionId = objectMD.versionId; - if (objectMD.isNull2) { - options.isNull = true; - } - } - } else { - // deleting a specific version - options.versionId = reqVersionId; - if (!nullVersionCompatMode) { - options.isNull = false; - } - } - } return options; } @@ -424,5 +450,6 @@ module.exports = { processVersioningState, getMasterState, versioningPreprocessing, + getVersionSpecificMetadataOptions, preprocessingVersioningDelete, }; diff --git a/lib/api/objectDeleteTagging.js b/lib/api/objectDeleteTagging.js index e16fd37df1..b2a465298d 100644 --- a/lib/api/objectDeleteTagging.js +++ b/lib/api/objectDeleteTagging.js @@ -1,7 +1,7 @@ const async = require('async'); const { errors } = require('arsenal'); -const { decodeVersionId, getVersionIdResHeader } +const { decodeVersionId, getVersionIdResHeader, getVersionSpecificMetadataOptions } = require('./apiUtils/object/versioning'); const { metadataValidateBucketAndObj } = require('../metadata/metadataUtils'); @@ -75,13 +75,7 @@ function objectDeleteTagging(authInfo, request, log, callback) { (bucket, objectMD, next) => { // eslint-disable-next-line no-param-reassign objectMD.tags = {}; - const params = {}; - if (objectMD.versionId) { - params.versionId = objectMD.versionId; - if (!config.nullVersionCompatMode) { - params.isNull = objectMD.isNull || false; - } - } + const params = getVersionSpecificMetadataOptions(objectMD, config.nullVersionCompatMode); const replicationInfo = getReplicationInfo(objectKey, bucket, true, 0, REPLICATION_ACTION, objectMD); if (replicationInfo) { diff --git a/lib/api/objectPutACL.js b/lib/api/objectPutACL.js index ec040cb322..52cbefc8be 100644 --- a/lib/api/objectPutACL.js +++ b/lib/api/objectPutACL.js @@ -7,7 +7,7 @@ const { pushMetric } = require('../utapi/utilities'); const collectCorsHeaders = require('../utilities/collectCorsHeaders'); const constants = require('../../constants'); const vault = require('../auth/vault'); -const { decodeVersionId, getVersionIdResHeader } +const { decodeVersionId, getVersionIdResHeader, getVersionSpecificMetadataOptions } = require('./apiUtils/object/versioning'); const { metadataValidateBucketAndObj } = require('../metadata/metadataUtils'); const monitoring = require('../utilities/metrics'); @@ -281,13 +281,7 @@ function objectPutACL(authInfo, request, log, cb) { }, function addAclsToObjMD(bucket, objectMD, ACLParams, next) { // Add acl's to object metadata - const params = {}; - if (objectMD.versionId) { - params.versionId = objectMD.versionId; - if (!config.nullVersionCompatMode) { - params.isNull = objectMD.isNull || false; - } - } + const params = getVersionSpecificMetadataOptions(objectMD, config.nullVersionCompatMode); acl.addObjectACL(bucket, objectKey, objectMD, ACLParams, params, log, err => next(err, bucket, objectMD)); }, diff --git a/lib/api/objectPutLegalHold.js b/lib/api/objectPutLegalHold.js index 5d4a62708e..6d4c123de5 100644 --- a/lib/api/objectPutLegalHold.js +++ b/lib/api/objectPutLegalHold.js @@ -2,7 +2,7 @@ const async = require('async'); const { errors, s3middleware } = require('arsenal'); const collectCorsHeaders = require('../utilities/collectCorsHeaders'); -const { decodeVersionId, getVersionIdResHeader } = +const { decodeVersionId, getVersionIdResHeader, getVersionSpecificMetadataOptions } = require('./apiUtils/object/versioning'); const getReplicationInfo = require('./apiUtils/object/getReplicationInfo'); const metadata = require('../metadata/wrapper'); @@ -86,13 +86,7 @@ function objectPutLegalHold(authInfo, request, log, callback) { (bucket, legalHold, objectMD, next) => { // eslint-disable-next-line no-param-reassign objectMD.legalHold = legalHold; - const params = {}; - if (objectMD.versionId) { - params.versionId = objectMD.versionId; - if (!config.nullVersionCompatMode) { - params.isNull = objectMD.isNull || false; - } - } + const params = getVersionSpecificMetadataOptions(objectMD, config.nullVersionCompatMode); const replicationInfo = getReplicationInfo(objectKey, bucket, true, 0, REPLICATION_ACTION, objectMD); if (replicationInfo) { diff --git a/lib/api/objectPutRetention.js b/lib/api/objectPutRetention.js index da8eb79b23..b77cccbe46 100644 --- a/lib/api/objectPutRetention.js +++ b/lib/api/objectPutRetention.js @@ -1,7 +1,7 @@ const async = require('async'); const { errors, s3middleware } = require('arsenal'); -const { decodeVersionId, getVersionIdResHeader } = +const { decodeVersionId, getVersionIdResHeader, getVersionSpecificMetadataOptions } = require('./apiUtils/object/versioning'); const { ObjectLockInfo, checkUserGovernanceBypass, hasGovernanceBypassHeader } = require('./apiUtils/object/objectLockHelpers'); @@ -116,13 +116,7 @@ function objectPutRetention(authInfo, request, log, callback) { /* eslint-disable no-param-reassign */ objectMD.retentionMode = retentionInfo.mode; objectMD.retentionDate = retentionInfo.date; - const params = {}; - if (objectMD.versionId) { - params.versionId = objectMD.versionId; - if (!config.nullVersionCompatMode) { - params.isNull = objectMD.isNull || false; - } - } + const params = getVersionSpecificMetadataOptions(objectMD, config.nullVersionCompatMode); const replicationInfo = getReplicationInfo(objectKey, bucket, true, 0, REPLICATION_ACTION, objectMD); if (replicationInfo) { diff --git a/lib/api/objectPutTagging.js b/lib/api/objectPutTagging.js index c37b11c569..038a43c612 100644 --- a/lib/api/objectPutTagging.js +++ b/lib/api/objectPutTagging.js @@ -1,7 +1,7 @@ const async = require('async'); const { errors, s3middleware } = require('arsenal'); -const { decodeVersionId, getVersionIdResHeader } = +const { decodeVersionId, getVersionIdResHeader, getVersionSpecificMetadataOptions } = require('./apiUtils/object/versioning'); const { metadataValidateBucketAndObj } = require('../metadata/metadataUtils'); @@ -81,13 +81,7 @@ function objectPutTagging(authInfo, request, log, callback) { (bucket, tags, objectMD, next) => { // eslint-disable-next-line no-param-reassign objectMD.tags = tags; - const params = {}; - if (objectMD.versionId) { - params.versionId = objectMD.versionId; - if (!config.nullVersionCompatMode) { - params.isNull = objectMD.isNull || false; - } - } + const params = getVersionSpecificMetadataOptions(objectMD, config.nullVersionCompatMode); const replicationInfo = getReplicationInfo(objectKey, bucket, true, 0, REPLICATION_ACTION, objectMD); if (replicationInfo) { diff --git a/tests/functional/aws-node-sdk/test/versioning/legacyNullVersionCompat.js b/tests/functional/aws-node-sdk/test/versioning/legacyNullVersionCompat.js new file mode 100644 index 0000000000..ce77ac50fe --- /dev/null +++ b/tests/functional/aws-node-sdk/test/versioning/legacyNullVersionCompat.js @@ -0,0 +1,156 @@ +const assert = require('assert'); +const async = require('async'); + +const BucketUtility = require('../../lib/utility/bucket-util'); + +const { + removeAllVersions, + versioningEnabled, +} = require('../../lib/utility/versioning-util.js'); + +// This series of tests can only be enabled on an environment that has +// two Cloudserver instances, with one of them in null version +// compatibility mode. This is why they have to be explicitly enabled, +// which is done in a particular Integration test suite. This test +// suite makes the most sense in Integration because it tests the +// combination of Cloudserver requests to bucketd and the behavior of +// bucketd based on those requests. + +const describeSkipIfNotExplicitlyEnabled = + process.env.ENABLE_LEGACY_NULL_VERSION_COMPAT_TESTS ? describe : describe.skip; + +describeSkipIfNotExplicitlyEnabled('legacy null version compatibility tests', () => { + const bucketUtilCompat = new BucketUtility('default', { + endpoint: 'http://127.0.0.1:8001', + }); + const s3Compat = bucketUtilCompat.s3; + const bucketUtil = new BucketUtility('default', {}); + const s3 = bucketUtil.s3; + const bucket = `legacy-null-version-compat-${Date.now()}`; + + // In this series of tests, we first create a non-current null + // version in legacy format (with "nullVersionId" field in the + // master and no "isNull2" metadata attribute), by using the + // Cloudserver endpoint that is configured with null version + // compatibility mode enabled. + beforeEach(done => async.series([ + next => s3Compat.createBucket({ + Bucket: bucket, + }, next), + next => s3Compat.putObject({ + Bucket: bucket, + Key: 'obj', + Body: 'nullbody', + }, next), + next => s3Compat.putBucketVersioning({ + Bucket: bucket, + VersioningConfiguration: versioningEnabled, + }, next), + next => s3Compat.putObject({ + Bucket: bucket, + Key: 'obj', + Body: 'versionedbody', + }, next), + ], done)); + + afterEach(done => { + removeAllVersions({ Bucket: bucket }, err => { + if (err) { + return done(err); + } + return s3Compat.deleteBucket({ Bucket: bucket }, done); + }); + }); + + it('updating ACL of legacy null version with non-compat cloudserver', done => { + async.series([ + next => s3.putObjectAcl({ + Bucket: bucket, + Key: 'obj', + VersionId: 'null', + ACL: 'public-read', + }, next), + next => s3.getObjectAcl({ + Bucket: bucket, + Key: 'obj', + VersionId: 'null', + }, (err, acl) => { + assert.ifError(err); + // check that we fetched the updated null version + assert.strictEqual(acl.Grants.length, 2); + next(); + }), + next => s3.deleteObject({ + Bucket: bucket, + Key: 'obj', + VersionId: 'null', + }, next), + next => s3.listObjectVersions({ + Bucket: bucket, + }, (err, listing) => { + assert.ifError(err); + // check that the null version has been correctly deleted + assert(listing.Versions.every(version => version.VersionId !== 'null')); + next(); + }), + ], done); + }); + + it('updating tags of legacy null version with non-compat cloudserver', done => { + const tagSet = [ + { + Key: 'newtag', + Value: 'newtagvalue', + }, + ]; + async.series([ + next => s3.putObjectTagging({ + Bucket: bucket, + Key: 'obj', + VersionId: 'null', + Tagging: { + TagSet: tagSet, + }, + }, next), + next => s3.getObjectTagging({ + Bucket: bucket, + Key: 'obj', + VersionId: 'null', + }, (err, tagging) => { + assert.ifError(err); + assert.deepStrictEqual(tagging.TagSet, tagSet); + next(); + }), + next => s3.deleteObjectTagging({ + Bucket: bucket, + Key: 'obj', + VersionId: 'null', + }, err => { + assert.ifError(err); + next(); + }), + next => s3.getObjectTagging({ + Bucket: bucket, + Key: 'obj', + VersionId: 'null', + }, (err, tagging) => { + assert.ifError(err); + assert.deepStrictEqual(tagging.TagSet, []); + next(); + }), + next => s3.deleteObject({ + Bucket: bucket, + Key: 'obj', + VersionId: 'null', + }, next), + next => s3.listObjectVersions({ + Bucket: bucket, + }, (err, listing) => { + assert.ifError(err); + // check that the null version has been correctly deleted + assert(listing.Versions.every(version => version.VersionId !== 'null')); + next(); + }), + ], done); + }); +}); diff --git a/tests/unit/api/apiUtils/versioning.js b/tests/unit/api/apiUtils/versioning.js index fca9e06c72..230c13b767 100644 --- a/tests/unit/api/apiUtils/versioning.js +++ b/tests/unit/api/apiUtils/versioning.js @@ -5,6 +5,7 @@ const { config } = require('../../../../lib/Config'); const INF_VID = versioning.VersionID.getInfVid(config.replicationGroupId); const { processVersioningState, getMasterState, + getVersionSpecificMetadataOptions, preprocessingVersioningDelete } = require('../../../../lib/api/apiUtils/object/versioning'); @@ -527,6 +528,68 @@ describe('versioning helpers', () => { })))); }); + describe('getVersionSpecificMetadataOptions', () => { + [ + { + description: 'object put before versioning was first enabled', + objMD: {}, + expectedRes: {}, + expectedResCompat: {}, + }, + { + description: 'non-null object version', + objMD: { + versionId: 'v1', + }, + expectedRes: { + versionId: 'v1', + isNull: false, + }, + expectedResCompat: { + versionId: 'v1', + }, + }, + { + description: 'legacy null object version', + objMD: { + versionId: 'vnull', + isNull: true, + }, + expectedRes: { + versionId: 'vnull', + }, + expectedResCompat: { + versionId: 'vnull', + }, + }, + { + description: 'null object version in null key', + objMD: { + versionId: 'vnull', + isNull: true, + isNull2: true, + }, + expectedRes: { + versionId: 'vnull', + isNull: true, + }, + expectedResCompat: { + versionId: 'vnull', + isNull: true, + }, + }, + ].forEach(testCase => + [false, true].forEach(nullVersionCompatMode => + it(`${testCase.description}${nullVersionCompatMode ? ' (null compat)' : ''}`, + () => { + const options = getVersionSpecificMetadataOptions( + testCase.objMD, nullVersionCompatMode); + const expectedResAttr = nullVersionCompatMode ? + 'expectedResCompat' : 'expectedRes'; + assert.deepStrictEqual(options, testCase[expectedResAttr]); + }))); + }); + describe('preprocessingVersioningDelete', () => { [ { From 15144e4adf5f3850086e10e59b76b3a66a1992d8 Mon Sep 17 00:00:00 2001 From: Jonathan Gramain Date: Wed, 11 Oct 2023 11:03:02 -0700 Subject: [PATCH 2/2] CLDSRV-458 bump cloudserver version --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index ab391695c3..08ef3ae106 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "s3", - "version": "7.70.27", + "version": "7.70.28", "description": "S3 connector", "main": "index.js", "engines": {