diff --git a/lib/api/apiUtils/object/versioning.js b/lib/api/apiUtils/object/versioning.js index 1ece12c652..a9018c0a60 100644 --- a/lib/api/apiUtils/object/versioning.js +++ b/lib/api/apiUtils/object/versioning.js @@ -1,4 +1,5 @@ const { errorInstances, versioning } = require('arsenal'); +const { ExternalNullVersionId } = versioning.VersioningConstants; const async = require('async'); const metadata = require('../../../metadata/wrapper'); @@ -18,7 +19,7 @@ const nonVersionedObjId = versionIdUtils.getInfVid(config.replicationGroupId); * fails due to improper format, otherwise undefined or the decoded version id */ function decodeVID(versionId) { - if (versionId === 'null') { + if (versionId === ExternalNullVersionId) { return versionId; } diff --git a/lib/routes/routeBackbeat.js b/lib/routes/routeBackbeat.js index 75ad86f14c..4ee18a4026 100644 --- a/lib/routes/routeBackbeat.js +++ b/lib/routes/routeBackbeat.js @@ -432,38 +432,26 @@ function putData(request, response, bucketInfo, objMd, log, callback) { return callback(errorInstances.BadRequest.customizeDescription(errMessage)); } - const incomingVersionIdEncoded = request.headers['x-scal-version-id']; - if (incomingVersionIdEncoded !== undefined) { - const incomingVersionIdDecoded = - incomingVersionIdEncoded !== 'null' ? decode(incomingVersionIdEncoded) : 'null'; - if (incomingVersionIdDecoded instanceof Error) { - log.error('crr putData: failed to decode x-scal-version-id header', { - method: 'putData', - error: incomingVersionIdDecoded.message, - }); - return callback( - errorInstances.BadRequest.customizeDescription('bad request: invalid x-scal-version-id header'), - ); - } - if (objMd && objMd.versionId === incomingVersionIdDecoded) { - // Data already at destination for this version; return 409 with the existing - // microVersionId so backbeat can decide if putMetadata is still needed. - log.debug('crr putData: version already at destination', { - method: 'putData', - bucketName: request.bucketName, - objectKey: request.objectKey, - hasMicroVersionId: !!objMd.microVersionId, - }); - request.resume(); - return _respondWithHeaderCrrConflict( - response, - log, - callback, - VersionIdCollisionException.name, - 'version id already at destination', - objMd.microVersionId, - ); - } + const incomingVersionIdEncoded = request.query?.versionId; + if (incomingVersionIdEncoded !== undefined && objMd) { + // objMd is the specific version requested via the versionId query param. + // Its existence means the data is already at the destination. Return 409 with the + // existing microVersionId so backbeat can decide if putMetadata is still needed. + log.debug('crr putData: version already at destination', { + method: 'putData', + bucketName: request.bucketName, + objectKey: request.objectKey, + hasMicroVersionId: !!objMd.microVersionId, + }); + request.resume(); + return _respondWithHeaderCrrConflict( + response, + log, + callback, + VersionIdCollisionException.name, + 'version id already at destination', + objMd.microVersionId, + ); } writeContinue(request, response); diff --git a/package.json b/package.json index 9e7f93e688..013ad37f59 100644 --- a/package.json +++ b/package.json @@ -64,7 +64,7 @@ "vaultclient": "scality/vaultclient#8.5.8", "werelogs": "scality/werelogs#semver:^8.2.4", "ws": "^8.18.0", - "@scality/cloudserverclient": "1.0.9", + "@scality/cloudserverclient": "1.0.12", "xml2js": "^0.6.2" }, "devDependencies": { diff --git a/tests/functional/backbeat/putData.js b/tests/functional/backbeat/putData.js index b4cb871860..b267243b30 100644 --- a/tests/functional/backbeat/putData.js +++ b/tests/functional/backbeat/putData.js @@ -14,6 +14,7 @@ const { BackbeatRoutesClient, PutDataCommand, VersionIdCollisionException } = re const { generateVersionId, encode: encodeVersionId } = versioning.VersionID; const TEST_BUCKET = `bucket-putdata-${uuidv4().split('-')[0]}`; +const TEST_BUCKET_UNVERSIONED = `bucket-putdata-unver-${uuidv4().split('-')[0]}`; const OBJECT_BODY = 'imAboutToBeCascadedWitNoParachuteInMyBack'; const OBJECT_MD5_HEX = createHash('md5').update(OBJECT_BODY).digest('hex'); const CANONICAL_ID = '79a59df900b949e55d96a1e698fbacedfd6e09d98eacf8f8d5218e7cd47ef2be'; @@ -55,6 +56,8 @@ before(async () => { VersioningConfiguration: { Status: 'Enabled' }, }), ); + + await s3.send(new CreateBucketCommand({ Bucket: TEST_BUCKET_UNVERSIONED })); }); describe('putData : VersionId collision detection', () => { @@ -106,17 +109,63 @@ describe('putData : VersionId collision detection', () => { const output = await putData(key, { versionId: differentVersionId }); assert.ok(output.Location, 'should return a Location when data is written normally'); }); + + it('should throw VersionIdCollisionException when a non-current version already exists', async () => { + const key = 'putdata-non-current-collision'; + + const v1Result = await s3.send( + new PutObjectCommand({ + Bucket: TEST_BUCKET, + Key: key, + Body: Buffer.from(OBJECT_BODY), + ContentType: 'text/plain', + }), + ); + const v1VersionId = v1Result.VersionId; + assert.ok(v1VersionId, 'first PutObject should return a VersionId'); + + const v2Result = await s3.send( + new PutObjectCommand({ + Bucket: TEST_BUCKET, + Key: key, + Body: Buffer.from(OBJECT_BODY), + ContentType: 'text/plain', + }), + ); + assert.ok(v2Result.VersionId, 'second PutObject should return a VersionId'); + + // putData on v1 (non-current) must detect the collision, not just compare against master + try { + await putData(key, { versionId: v1VersionId }); + assert.fail('expected VersionIdCollisionException'); + } catch (err) { + assert.ok( + err instanceof VersionIdCollisionException, + `expected VersionIdCollisionException, got ${err.constructor.name}`, + ); + assert.strictEqual(err.microVersionId, '', 'microVersionId should be empty for original write state'); + } + }); }); describe('putData : null-version objects (ExternalNullVersionId)', () => { - // Null-version objects created before versioning was enabled use Arsenal constant ExternalNullVersionId = 'null' - // getEncodedVersionId() returns 'null' as-is (no base62 encoding), and objMd.versionId is - // undefined in metadata : collision detection is not possible, so putData must write normally. - it('should write normally when VersionId is "null" (ExternalNullVersionId)', async () => { + it('should write normally when VersionId is "null" and destination has no null version', async () => { + // Versioned bucket: existing object has a real versionId, not a null version. + // Fetching with ExternalNullVersionId returns no objMd => no collision => write normally. + const key = 'putdata-null-version-no-collision'; + await s3.send( + new PutObjectCommand({ + Bucket: TEST_BUCKET, + Key: key, + Body: Buffer.from(OBJECT_BODY), + ContentType: 'text/plain', + }), + ); + const output = await backbeatClient.send( new PutDataCommand({ Bucket: TEST_BUCKET, - Key: 'putdata-null-version', + Key: key, ContentMD5: OBJECT_MD5_HEX, CanonicalID: CANONICAL_ID, VersioningRequired: true, @@ -126,10 +175,44 @@ describe('putData : null-version objects (ExternalNullVersionId)', () => { ); assert.ok(output.Location, 'putData with null-version versionId should write normally'); }); + + it('should throw VersionIdCollisionException when a null version already exists at destination', async () => { + // Unversioned bucket: objects have no versionId in metadata (they are null versions). + // putData with ExternalNullVersionId must detect the collision. + const key = 'putdata-null-version-collision'; + await s3.send( + new PutObjectCommand({ + Bucket: TEST_BUCKET_UNVERSIONED, + Key: key, + Body: Buffer.from(OBJECT_BODY), + ContentType: 'text/plain', + }), + ); + + try { + await backbeatClient.send( + new PutDataCommand({ + Bucket: TEST_BUCKET_UNVERSIONED, + Key: key, + ContentMD5: OBJECT_MD5_HEX, + CanonicalID: CANONICAL_ID, + VersionId: ExternalNullVersionId, + Body: Buffer.from(OBJECT_BODY), + }), + ); + assert.fail('expected VersionIdCollisionException'); + } catch (err) { + assert.ok( + err instanceof VersionIdCollisionException, + `expected VersionIdCollisionException, got ${err.constructor.name}`, + ); + assert.strictEqual(err.microVersionId, '', 'microVersionId should be empty for null-version collision'); + } + }); }); -describe('putData : baseline (no cascade headers)', () => { - it('should succeed normally when putData has no VersionId header', async () => { +describe('putData : baseline', () => { + it('should succeed normally when putData has no VersionId query param', async () => { const key = 'putdata-baseline-no-version-id'; const output = await putData(key); assert.ok(output.Location, 'putData without VersionId should return a Location'); diff --git a/tests/unit/routes/routeBackbeat.js b/tests/unit/routes/routeBackbeat.js index 091bed2e26..5b8edc614f 100644 --- a/tests/unit/routes/routeBackbeat.js +++ b/tests/unit/routes/routeBackbeat.js @@ -229,10 +229,9 @@ describe('routeBackbeat', () => { 'content-md5': '1234', 'content-length': '0', 'x-scal-versioning-required': 'true', - 'x-scal-version-id': encodedVersionId, }); mockRequest.method = 'PUT'; - mockRequest.url = '/_/backbeat/data/bucket0/key0'; + mockRequest.url = `/_/backbeat/data/bucket0/key0?versionId=${encodedVersionId}`; mockRequest.destroy = () => {}; metadataUtils.standardMetadataValidateBucketAndObj.callsFake((params, denies, log, callback) => { diff --git a/yarn.lock b/yarn.lock index 8c5bdb9967..2475da4c59 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3210,10 +3210,10 @@ resolved "https://registry.yarnpkg.com/@rtsao/scc/-/scc-1.1.0.tgz#927dd2fae9bc3361403ac2c7a00c32ddce9ad7e8" integrity sha512-zt6OdqaDoOnJ1ZYsCYGt9YmWzDXl4vQdKTyJev62gFhRGKdx7mcT54V9KIjg+d2wi9EXsPvAPKe7i7WjfVWB8g== -"@scality/cloudserverclient@1.0.9": - version "1.0.9" - resolved "https://registry.yarnpkg.com/@scality/cloudserverclient/-/cloudserverclient-1.0.9.tgz#0a333e39436e1f1e74279c3b5a11645be1fffbc1" - integrity sha512-ZGqH4G535opDAEP2PxdYDFG7kjZ/eBrzP3ZmO49goFVjRhyDCZ1gT0Pask3/wZcyysZ2Au1qylorZSLPiZmB2A== +"@scality/cloudserverclient@1.0.12": + version "1.0.12" + resolved "https://registry.yarnpkg.com/@scality/cloudserverclient/-/cloudserverclient-1.0.12.tgz#cbef36fb65b9cb9f7235d6d7dec291a535cd8dd2" + integrity sha512-A85ki+dQSdBt7C9pGxTA2NXRbg74azZ2LV/dBWf2A5soQFYXabDAnKQl/4+hnar8OAbIZ8XFoJjqqQzRZ4mGhg== dependencies: "@aws-sdk/client-s3" "^3.1009.0" "@aws-sdk/middleware-expect-continue" "^3.972.8"