diff --git a/lib/api/apiUtils/bucket/bucketCreation.js b/lib/api/apiUtils/bucket/bucketCreation.js index ca43c09a82..43d125475a 100644 --- a/lib/api/apiUtils/bucket/bucketCreation.js +++ b/lib/api/apiUtils/bucket/bucketCreation.js @@ -18,7 +18,6 @@ const oldUsersBucket = constants.oldUsersBucket; const zenkoSeparator = constants.zenkoSeparator; const userBucketOwner = 'admin'; - function addToUsersBucket(canonicalID, bucketName, bucketMD, log, cb) { // BACKWARD: Simplify once do not have to deal with old // usersbucket name and old splitter @@ -28,8 +27,7 @@ function addToUsersBucket(canonicalID, bucketName, bucketMD, log, cb) { if (err && !err.is.NoSuchBucket && !err.is.BucketAlreadyExists) { return cb(err); } - const splitter = usersBucketAttrs ? - constants.splitter : constants.oldSplitter; + const splitter = usersBucketAttrs ? constants.splitter : constants.oldSplitter; let key = createKeyForUserBucket(canonicalID, splitter, bucketName); const omVal = { creationDate: new Date().toJSON(), @@ -38,46 +36,44 @@ function addToUsersBucket(canonicalID, bucketName, bucketMD, log, cb) { // If the new format usersbucket does not exist, try to put the // key in the old usersBucket using the old splitter. // Otherwise put the key in the new format usersBucket - const usersBucketBeingCalled = usersBucketAttrs ? - usersBucket : oldUsersBucket; - return metadata.putObjectMD(usersBucketBeingCalled, key, - omVal, {}, log, err => { - if (err?.is?.NoSuchBucket) { - // There must be no usersBucket so createBucket - // one using the new format - log.trace('users bucket does not exist, ' + - 'creating users bucket'); - key = `${canonicalID}${constants.splitter}` + - `${bucketName}`; - const creationDate = new Date().toJSON(); - const freshBucket = new BucketInfo(usersBucket, - userBucketOwner, userBucketOwner, creationDate, - BucketInfo.currentModelVersion()); - return metadata.createBucket(usersBucket, - freshBucket, log, err => { - // Note: In the event that two - // users' requests try to create the - // usersBucket at the same time, - // this will prevent one of the users - // from getting a BucketAlreadyExists - // error with respect - // to the usersBucket. - // TODO: move to `.is` once BKTCLT-9 is done and bumped in Cloudserver - if (err && !err.BucketAlreadyExists) { - log.error('error from metadata', { - error: err, - }); - return cb(err); - } - log.trace('Users bucket created'); - // Finally put the key in the new format - // usersBucket - return metadata.putObjectMD(usersBucket, - key, omVal, {}, log, cb); + const usersBucketBeingCalled = usersBucketAttrs ? usersBucket : oldUsersBucket; + return metadata.putObjectMD(usersBucketBeingCalled, key, omVal, {}, log, err => { + if (err?.is?.NoSuchBucket) { + // There must be no usersBucket so createBucket + // one using the new format + log.trace('users bucket does not exist, ' + 'creating users bucket'); + key = `${canonicalID}${constants.splitter}` + `${bucketName}`; + const creationDate = new Date().toJSON(); + const freshBucket = new BucketInfo( + usersBucket, + userBucketOwner, + userBucketOwner, + creationDate, + BucketInfo.currentModelVersion(), + ); + return metadata.createBucket(usersBucket, freshBucket, log, err => { + // Note: In the event that two + // users' requests try to create the + // usersBucket at the same time, + // this will prevent one of the users + // from getting a BucketAlreadyExists + // error with respect + // to the usersBucket. + // TODO: move to `.is` once BKTCLT-9 is done and bumped in Cloudserver + if (err && !err.BucketAlreadyExists) { + log.error('error from metadata', { + error: err, }); - } - return cb(err); - }); + return cb(err); + } + log.trace('Users bucket created'); + // Finally put the key in the new format + // usersBucket + return metadata.putObjectMD(usersBucket, key, omVal, {}, log, cb); + }); + } + return cb(err); + }); }); } @@ -92,6 +88,12 @@ function removeTransientOrDeletedLabel(bucket, log, callback) { function freshStartCreateBucket(bucket, canonicalID, log, callback) { const bucketName = bucket.getName(); metadata.createBucket(bucketName, bucket, log, err => { + if (err?.is?.BucketAlreadyExists) { + // The concurrent creator owns users-bucket registration and + // cleanup of the transient flag. + log.trace('bucket already exists in metadata'); + return callback(); + } if (err) { log.debug('error from metadata', { error: err }); return callback(err); @@ -139,16 +141,15 @@ function cleanUpBucket(bucketMD, canonicalID, log, callback) { * @callback called with (err, sseInfo: object) */ function bucketLevelServerSideEncryption(bucket, headers, log, cb) { - kms.bucketLevelEncryption( - bucket, headers, log, (err, sseInfo) => { - if (err) { - log.debug('error getting bucket encryption info', { - error: err, - }); - return cb(err); - } - return cb(null, sseInfo); - }); + kms.bucketLevelEncryption(bucket, headers, log, (err, sseInfo) => { + if (err) { + log.debug('error getting bucket encryption info', { + error: err, + }); + return cb(err); + } + return cb(null, sseInfo); + }); } /** @@ -163,28 +164,44 @@ function bucketLevelServerSideEncryption(bucket, headers, log, cb) { * @param {function} cb - callback to bucketPut * @return {undefined} */ -function createBucket(authInfo, bucketName, headers, - locationConstraint, log, cb) { +function createBucket(authInfo, bucketName, headers, locationConstraint, log, cb) { // Prefer using undefined instead of null for unused properties so they are not present in metadata payload log.trace('Creating bucket'); assert.strictEqual(typeof bucketName, 'string'); const canonicalID = authInfo.getCanonicalID(); - const ownerDisplayName = - authInfo.getAccountDisplayName(); + const ownerDisplayName = authInfo.getAccountDisplayName(); const creationDate = new Date().toJSON(); const isNFSEnabled = headers['x-scal-nfs-enabled'] === 'true' || undefined; const headerObjectLock = headers['x-amz-bucket-object-lock-enabled']; - const objectLockEnabled - = headerObjectLock && headerObjectLock.toLowerCase() === 'true'; - const bucket = new BucketInfo(bucketName, canonicalID, ownerDisplayName, - creationDate, BucketInfo.currentModelVersion(), undefined, undefined, undefined, undefined, - undefined, undefined, undefined, undefined, undefined, undefined, undefined, undefined, undefined, isNFSEnabled, - undefined, undefined, objectLockEnabled); + const objectLockEnabled = headerObjectLock && headerObjectLock.toLowerCase() === 'true'; + const bucket = new BucketInfo( + bucketName, + canonicalID, + ownerDisplayName, + creationDate, + BucketInfo.currentModelVersion(), + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + isNFSEnabled, + undefined, + undefined, + objectLockEnabled, + ); let locationConstraintVal = null; if (locationConstraint) { - const [locationConstraintStr, ingestion] = - locationConstraint.split(zenkoSeparator); + const [locationConstraintStr, ingestion] = locationConstraint.split(zenkoSeparator); if (locationConstraintStr) { locationConstraintVal = locationConstraintStr; bucket.setLocationConstraint(locationConstraintStr); @@ -210,88 +227,91 @@ function createBucket(authInfo, bucketName, headers, acl: bucket.acl, log, }; - async.parallel({ - prepareNewBucketMD: function prepareNewBucketMD(callback) { - acl.parseAclFromHeaders(parseAclParams, (err, parsedACL) => { - if (err) { - log.debug('error parsing acl from headers', { - error: err, - }); - return callback(err); - } - bucket.setFullAcl(parsedACL); - return callback(null, bucket); - }); - }, - getAnyExistingBucketInfo: function getAnyExistingBucketInfo(callback) { - metadata.getBucket(bucketName, log, (err, data) => { - // TODO: move to `.is` once BKTCLT-9 is done and bumped in Cloudserver - if (err && err.NoSuchBucket) { - return callback(null, 'NoBucketYet'); - } - if (err) { - return callback(err); - } - return callback(null, data); - }); + async.parallel( + { + prepareNewBucketMD: function prepareNewBucketMD(callback) { + acl.parseAclFromHeaders(parseAclParams, (err, parsedACL) => { + if (err) { + log.debug('error parsing acl from headers', { + error: err, + }); + return callback(err); + } + bucket.setFullAcl(parsedACL); + return callback(null, bucket); + }); + }, + getAnyExistingBucketInfo: function getAnyExistingBucketInfo(callback) { + metadata.getBucket(bucketName, log, (err, data) => { + // TODO: move to `.is` once BKTCLT-9 is done and bumped in Cloudserver + if (err && err.NoSuchBucket) { + return callback(null, 'NoBucketYet'); + } + if (err) { + return callback(err); + } + return callback(null, data); + }); + }, }, - }, - // Function to run upon finishing both parallel requests - (err, results) => { - if (err) { - return cb(err); - } - const existingBucketMD = results.getAnyExistingBucketInfo; - if (existingBucketMD instanceof BucketInfo && - existingBucketMD.getOwner() !== canonicalID && - !isServiceAccount(canonicalID)) { - // return existingBucketMD to collect cors headers - return cb(errors.BucketAlreadyExists, existingBucketMD); - } - const newBucketMD = results.prepareNewBucketMD; - if (existingBucketMD === 'NoBucketYet') { - const bucketSseConfig = parseBucketEncryptionHeaders(headers); + // Function to run upon finishing both parallel requests + (err, results) => { + if (err) { + return cb(err); + } + const existingBucketMD = results.getAnyExistingBucketInfo; + if ( + existingBucketMD instanceof BucketInfo && + existingBucketMD.getOwner() !== canonicalID && + !isServiceAccount(canonicalID) + ) { + // return existingBucketMD to collect cors headers + return cb(errors.BucketAlreadyExists, existingBucketMD); + } + const newBucketMD = results.prepareNewBucketMD; + if (existingBucketMD === 'NoBucketYet') { + const bucketSseConfig = parseBucketEncryptionHeaders(headers); - // Apply global SSE configuration when global encryption is enabled - // and no SSE settings were specified during bucket creation. - // Bucket-specific SSE headers override the default encryption. - const sseConfig = config.globalEncryptionEnabled && !bucketSseConfig.algorithm - ? { - algorithm: 'AES256', - mandatory: true, - } : bucketSseConfig; + // Apply global SSE configuration when global encryption is enabled + // and no SSE settings were specified during bucket creation. + // Bucket-specific SSE headers override the default encryption. + const sseConfig = + config.globalEncryptionEnabled && !bucketSseConfig.algorithm + ? { + algorithm: 'AES256', + mandatory: true, + } + : bucketSseConfig; - return bucketLevelServerSideEncryption( - bucket, sseConfig, log, - (err, sseInfo) => { + return bucketLevelServerSideEncryption(bucket, sseConfig, log, (err, sseInfo) => { if (err) { return cb(err); } newBucketMD.setServerSideEncryption(sseInfo); - log.trace( - 'new bucket without flags; adding transient label'); + log.trace('new bucket without flags; adding transient label'); newBucketMD.addTransientFlag(); - return freshStartCreateBucket(newBucketMD, canonicalID, - log, cb); + return freshStartCreateBucket(newBucketMD, canonicalID, log, cb); }); - } - if (existingBucketMD.hasTransientFlag() || - existingBucketMD.hasDeletedFlag()) { - log.trace('bucket has transient flag or deleted flag. cleaning up'); - return cleanUpBucket(newBucketMD, canonicalID, log, cb); - } - // If bucket already exists in non-transient and non-deleted - // state and owned by requester, then return BucketAlreadyOwnedByYou - // error unless old AWS behavior (us-east-1) - // Existing locationConstraint must have legacyAwsBehavior === true - // New locationConstraint should have legacyAwsBehavior === true - if (isLegacyAWSBehavior(locationConstraintVal) && - isLegacyAWSBehavior(existingBucketMD.getLocationConstraint())) { - log.trace('returning 200 instead of 409 to mirror us-east-1'); - return cb(null, existingBucketMD); - } - return cb(errors.BucketAlreadyOwnedByYou, existingBucketMD); - }); + } + if (existingBucketMD.hasTransientFlag() || existingBucketMD.hasDeletedFlag()) { + log.trace('bucket has transient flag or deleted flag. cleaning up'); + return cleanUpBucket(newBucketMD, canonicalID, log, cb); + } + // If bucket already exists in non-transient and non-deleted + // state and owned by requester, then return BucketAlreadyOwnedByYou + // error unless old AWS behavior (us-east-1) + // Existing locationConstraint must have legacyAwsBehavior === true + // New locationConstraint should have legacyAwsBehavior === true + if ( + isLegacyAWSBehavior(locationConstraintVal) && + isLegacyAWSBehavior(existingBucketMD.getLocationConstraint()) + ) { + log.trace('returning 200 instead of 409 to mirror us-east-1'); + return cb(null, existingBucketMD); + } + return cb(errors.BucketAlreadyOwnedByYou, existingBucketMD); + }, + ); } module.exports = { diff --git a/lib/services.js b/lib/services.js index 5d07c79753..2eefec878a 100644 --- a/lib/services.js +++ b/lib/services.js @@ -974,6 +974,10 @@ const services = { // bucket. This is the desired behavior since this should be // a hidden bucket. return metadata.createBucket(MPUBucketName, mpuBucket, log, err => { + if (err?.is?.BucketAlreadyExists) { + log.trace('mpu bucket already exists in metadata'); + return cb(null, mpuBucket); + } if (err) { log.error('error from metadata', { error: err }); return cb(err); diff --git a/package.json b/package.json index 0db3eff54b..2611e1e544 100644 --- a/package.json +++ b/package.json @@ -35,7 +35,7 @@ "@opentelemetry/instrumentation-ioredis": "~0.64.0", "@opentelemetry/instrumentation-mongodb": "~0.69.0", "@smithy/node-http-handler": "^3.0.0", - "arsenal": "git+https://github.com/scality/arsenal#8.5.6", + "arsenal": "git+https://github.com/scality/arsenal#8.5.12", "async": "2.6.4", "aws-crt": "^1.24.0", "bucketclient": "scality/bucketclient#8.2.7", diff --git a/tests/unit/bucket/bucketCreation.js b/tests/unit/bucket/bucketCreation.js index 6ae385d17c..e110b3d880 100644 --- a/tests/unit/bucket/bucketCreation.js +++ b/tests/unit/bucket/bucketCreation.js @@ -1,8 +1,10 @@ const assert = require('assert'); +const sinon = require('sinon'); +const { errors } = require('arsenal'); +const metadata = require('../../../lib/metadata/wrapper'); const { cleanup, DummyRequestLogger } = require('../helpers'); -const { createBucket } = - require('../../../lib/api/apiUtils/bucket/bucketCreation'); +const { createBucket } = require('../../../lib/api/apiUtils/bucket/bucketCreation'); const { makeAuthInfo } = require('../helpers'); const bucketName = 'creationbucket'; @@ -15,34 +17,30 @@ const specialBehaviorLocationConstraint = 'us-east-1'; describe('bucket creation', () => { it('should create a bucket', done => { - createBucket(authInfo, bucketName, headers, - normalBehaviorLocationConstraint, log, err => { - assert.ifError(err); - done(); - }); + createBucket(authInfo, bucketName, headers, normalBehaviorLocationConstraint, log, err => { + assert.ifError(err); + done(); + }); }); describe('when you already created the bucket in us-east-1', () => { beforeEach(done => { cleanup(); - createBucket(authInfo, bucketName, headers, - specialBehaviorLocationConstraint, log, err => { - assert.ifError(err); - done(); - }); + createBucket(authInfo, bucketName, headers, specialBehaviorLocationConstraint, log, err => { + assert.ifError(err); + done(); + }); }); it('should return 200 if try to recreate in us-east-1', done => { - createBucket(authInfo, bucketName, headers, - specialBehaviorLocationConstraint, log, err => { + createBucket(authInfo, bucketName, headers, specialBehaviorLocationConstraint, log, err => { assert.ifError(err); done(); }); }); it('should return 409 if try to recreate in non-us-east-1', done => { - createBucket(authInfo, bucketName, headers, - normalBehaviorLocationConstraint, log, err => { + createBucket(authInfo, bucketName, headers, normalBehaviorLocationConstraint, log, err => { assert.strictEqual(err.is.BucketAlreadyOwnedByYou, true); done(); }); @@ -52,32 +50,69 @@ describe('bucket creation', () => { describe('when you already created the bucket in non-us-east-1', () => { beforeEach(done => { cleanup(); - createBucket(authInfo, bucketName, headers, - normalBehaviorLocationConstraint, log, err => { - assert.ifError(err); - done(); - }); + createBucket(authInfo, bucketName, headers, normalBehaviorLocationConstraint, log, err => { + assert.ifError(err); + done(); + }); }); it('should return 409 if try to recreate in us-east-1', done => { - createBucket(authInfo, bucketName, headers, - specialBehaviorLocationConstraint, log, err => { + createBucket(authInfo, bucketName, headers, specialBehaviorLocationConstraint, log, err => { assert.strictEqual(err.is.BucketAlreadyOwnedByYou, true); done(); }); }); }); }); + +describe('bucket creation when createBucket races', () => { + const raceBucketName = 'race-creation-bucket'; + let sandbox; + + beforeEach(() => { + cleanup(); + sandbox = sinon.createSandbox(); + }); + + afterEach(() => { + sandbox.restore(); + }); + + it('should complete creation when metadata returns BucketAlreadyExists', done => { + sandbox.stub(metadata, 'getBucket').callsFake((name, log, cb) => { + if (name === raceBucketName) { + return cb(errors.NoSuchBucket); + } + return cb(errors.NoSuchBucket); + }); + sandbox.stub(metadata, 'createBucket').callsFake((name, bucket, log, cb) => { + if (name === raceBucketName) { + return cb(errors.BucketAlreadyExists); + } + return cb(null); + }); + sandbox.stub(metadata, 'putObjectMD').yields(null); + sandbox.stub(metadata, 'updateBucket').yields(null); + + createBucket(authInfo, raceBucketName, headers, normalBehaviorLocationConstraint, log, err => { + assert.ifError(err); + sandbox.assert.calledOnce(metadata.createBucket); + sandbox.assert.notCalled(metadata.putObjectMD); + sandbox.assert.notCalled(metadata.updateBucket); + done(); + }); + }); +}); + describe('bucket creation with object lock', () => { it('should return 200 when creating a bucket with object lock', done => { const bucketName = 'test-bucket-with-objectlock'; const headers = { 'x-amz-bucket-object-lock-enabled': 'true', }; - createBucket(authInfo, bucketName, headers, - normalBehaviorLocationConstraint, log, err => { - assert.ifError(err); - done(); - }); + createBucket(authInfo, bucketName, headers, normalBehaviorLocationConstraint, log, err => { + assert.ifError(err); + done(); + }); }); }); diff --git a/tests/unit/lib/services.spec.js b/tests/unit/lib/services.spec.js index 3249be1c62..5f341d887d 100644 --- a/tests/unit/lib/services.spec.js +++ b/tests/unit/lib/services.spec.js @@ -1,6 +1,7 @@ const assert = require('assert'); const sinon = require('sinon'); -const { versioning } = require('arsenal'); +const { versioning, errors } = require('arsenal'); +const BucketInfo = require('arsenal').models.BucketInfo; const services = require('../../../lib/services'); const metadata = require('../../../lib/metadata/wrapper'); @@ -418,4 +419,55 @@ describe('services', () => { }); }); }); + + describe('getMPUBucket', () => { + const destinationBucket = new BucketInfo('dest-bucket', 'owner', 'ownerDisplay', new Date().toJSON()); + let getBucketStub; + let createBucketStub; + + beforeEach(() => { + getBucketStub = sinon.stub(metadata, 'getBucket'); + createBucketStub = sinon.stub(metadata, 'createBucket'); + }); + + it('should return local MPU bucket when createBucket races', done => { + const mpuBucketName = `${constants.mpuBucketPrefix}${bucketName}`; + getBucketStub.yields(errors.NoSuchBucket); + createBucketStub.yields(errors.BucketAlreadyExists); + + services.getMPUBucket(destinationBucket, bucketName, log, (err, bucket) => { + assert.ifError(err); + assert.strictEqual(bucket.getName(), mpuBucketName); + assert.strictEqual(bucket.getOwner(), destinationBucket.getOwner()); + sinon.assert.calledOnce(getBucketStub); + sinon.assert.calledOnce(createBucketStub); + done(); + }); + }); + + it('should create MPU bucket when it does not exist', done => { + getBucketStub.yields(errors.NoSuchBucket); + createBucketStub.yields(null); + + services.getMPUBucket(destinationBucket, bucketName, log, (err, bucket) => { + assert.ifError(err); + assert.strictEqual(bucket.getName(), `${constants.mpuBucketPrefix}${bucketName}`); + sinon.assert.calledOnce(createBucketStub); + done(); + }); + }); + + it('should return error when createBucket fails after NoSuchBucket', done => { + const testError = errors.InternalError; + getBucketStub.yields(errors.NoSuchBucket); + createBucketStub.yields(testError); + + services.getMPUBucket(destinationBucket, bucketName, log, err => { + assert.strictEqual(err, testError); + sinon.assert.calledOnce(getBucketStub); + sinon.assert.calledOnce(createBucketStub); + done(); + }); + }); + }); }); diff --git a/yarn.lock b/yarn.lock index 586c43f42b..844e71e1b8 100644 --- a/yarn.lock +++ b/yarn.lock @@ -19,7 +19,7 @@ "@aws-sdk/types" "^3.222.0" tslib "^1.11.1" -"@aws-crypto/crc32@5.2.0", "@aws-crypto/crc32@^5.2.0": +"@aws-crypto/crc32@5.2.0": version "5.2.0" resolved "https://registry.yarnpkg.com/@aws-crypto/crc32/-/crc32-5.2.0.tgz#cfcc22570949c98c6689cfcbd2d693d36cdae2e1" integrity sha512-nLbCWqQNgUiwwtFsen1AdzAtvuLRsQS8rYgMuxCrdKf9kOssamGLuPwyTY9wyYblNr9+1XM8v6zoDTPPSIeANg== @@ -28,7 +28,7 @@ "@aws-sdk/types" "^3.222.0" tslib "^2.6.2" -"@aws-crypto/crc32c@5.2.0", "@aws-crypto/crc32c@^5.2.0": +"@aws-crypto/crc32c@5.2.0": version "5.2.0" resolved "https://registry.yarnpkg.com/@aws-crypto/crc32c/-/crc32c-5.2.0.tgz#4e34aab7f419307821509a98b9b08e84e0c1917e" integrity sha512-+iWb8qaHLYKrNvGRbiYRHSdKRWhto5XlZUEBwDjYNf+ly5SVYG6zEoYIdxvf5R3zyeP16w4PLBn3rH1xc74Rag== @@ -6596,9 +6596,9 @@ arraybuffer.prototype.slice@^1.0.4: optionalDependencies: ioctl "^2.0.2" -"arsenal@git+https://github.com/scality/arsenal#8.5.6": - version "8.5.6" - resolved "git+https://github.com/scality/arsenal#0db557930c7d13204167188a7503e6e00d154df5" +"arsenal@git+https://github.com/scality/arsenal#8.5.12": + version "8.5.12" + resolved "git+https://github.com/scality/arsenal#fd1354baedadbff1ff6ba6c61c04df1795aca7c7" dependencies: "@aws-sdk/client-kms" "^3.975.0" "@aws-sdk/client-s3" "^3.975.0"