From 45c6aefc35436c2d27e2609e2335547c04f171fa Mon Sep 17 00:00:00 2001 From: Jonathan Gramain Date: Mon, 24 Oct 2022 17:39:53 -0700 Subject: [PATCH 1/2] bugfix: CLDSRV-290 [test] update `PUT /_/backbeat/metadata` tests - modify existing `PUT /_/backbeat/metadata` tests to always pass the `versionId` in the query string, as it should be with the updated API contract - create a new test that does not pass the `versionId` in the query string on an update, and expects a new version to be created (and both versions to be readable to ensure no cleanup occurred) - general tech debt cleanup: update the test `versionId` to be in the new base64 format when encoded by removing the extra info, making it exactly 27 characters long --- .../raw-node/test/routes/routeBackbeat.js | 109 +++++++++++++++++- 1 file changed, 106 insertions(+), 3 deletions(-) diff --git a/tests/functional/raw-node/test/routes/routeBackbeat.js b/tests/functional/raw-node/test/routes/routeBackbeat.js index 5b28ba4f55..02c9034837 100644 --- a/tests/functional/raw-node/test/routes/routeBackbeat.js +++ b/tests/functional/raw-node/test/routes/routeBackbeat.js @@ -50,7 +50,7 @@ const testMd = { }, 'nullVersionId': '99999999999999999999RG001 ', 'isDeleteMarker': false, - 'versionId': '98505119639965999999RG001 9', + 'versionId': '98505119639965999999RG001 ', 'replicationInfo': { status: 'COMPLETED', backends: [{ site: 'zenko', status: 'PENDING' }], @@ -416,8 +416,7 @@ describeSkipIfAWS('backbeat routes', () => { }); }); - it('should remove old object data locations if version is overwritten', - done => { + it('should remove old object data locations if version is overwritten', done => { let oldLocation; const testKeyOldData = `${testKey}-old-data`; async.waterfall([next => { @@ -443,6 +442,9 @@ describeSkipIfAWS('backbeat routes', () => { method: 'PUT', bucket: TEST_BUCKET, objectKey: testKey, resourceType: 'metadata', + queryObj: { + versionId: versionIdUtils.encode(testMd.versionId), + }, authCredentials: backbeatAuthCredentials, requestBody: JSON.stringify(newMd), }, next); @@ -485,6 +487,9 @@ describeSkipIfAWS('backbeat routes', () => { method: 'PUT', bucket: TEST_BUCKET, objectKey: testKey, resourceType: 'metadata', + queryObj: { + versionId: versionIdUtils.encode(testMd.versionId), + }, authCredentials: backbeatAuthCredentials, requestBody: JSON.stringify(newMd), }, next); @@ -516,6 +521,7 @@ describeSkipIfAWS('backbeat routes', () => { done(); }); }); + it('should not remove data locations on replayed metadata PUT', done => { let serializedNewMd; @@ -540,6 +546,9 @@ describeSkipIfAWS('backbeat routes', () => { method: 'PUT', bucket: TEST_BUCKET, objectKey: testKey, resourceType: 'metadata', + queryObj: { + versionId: versionIdUtils.encode(testMd.versionId), + }, authCredentials: backbeatAuthCredentials, requestBody: serializedNewMd, }, (err, response) => { @@ -563,6 +572,97 @@ describeSkipIfAWS('backbeat routes', () => { done(); }); }); + + it('should create a new version when no versionId is passed in query string', done => { + let newVersion; + async.waterfall([next => { + // put object's data locations + makeBackbeatRequest({ + method: 'PUT', bucket: TEST_BUCKET, + objectKey: testKey, + resourceType: 'data', + headers: { + 'content-length': testData.length, + 'content-md5': testDataMd5, + 'x-scal-canonical-id': testArn, + }, + authCredentials: backbeatAuthCredentials, + requestBody: testData }, next); + }, (response, next) => { + assert.strictEqual(response.statusCode, 200); + // put object metadata + const oldMd = Object.assign({}, testMd); + oldMd.location = JSON.parse(response.body); + makeBackbeatRequest({ + method: 'PUT', bucket: TEST_BUCKET, + objectKey: testKey, + resourceType: 'metadata', + queryObj: { + versionId: versionIdUtils.encode(testMd.versionId), + }, + authCredentials: backbeatAuthCredentials, + requestBody: JSON.stringify(oldMd), + }, next); + }, (response, next) => { + assert.strictEqual(response.statusCode, 200); + const parsedResponse = JSON.parse(response.body); + assert.strictEqual(parsedResponse.versionId, testMd.versionId); + // create new data locations + makeBackbeatRequest({ + method: 'PUT', bucket: TEST_BUCKET, + objectKey: testKey, + resourceType: 'data', + headers: { + 'content-length': testData.length, + 'content-md5': testDataMd5, + 'x-scal-canonical-id': testArn, + }, + authCredentials: backbeatAuthCredentials, + requestBody: testData }, next); + }, (response, next) => { + assert.strictEqual(response.statusCode, 200); + // create a new version with the new data locations, + // not passing 'versionId' in the query string + const newMd = Object.assign({}, testMd); + newMd.location = JSON.parse(response.body); + makeBackbeatRequest({ + method: 'PUT', bucket: TEST_BUCKET, + objectKey: testKey, + resourceType: 'metadata', + authCredentials: backbeatAuthCredentials, + requestBody: JSON.stringify(newMd), + }, next); + }, (response, next) => { + assert.strictEqual(response.statusCode, 200); + const parsedResponse = JSON.parse(response.body); + newVersion = parsedResponse.versionId; + assert.notStrictEqual(newVersion, testMd.versionId); + // give some time for the async deletes to complete, + // then check that we can read the latest version + setTimeout(() => s3.getObject({ + Bucket: TEST_BUCKET, + Key: testKey, + }, (err, data) => { + assert.ifError(err); + assert.strictEqual(data.Body.toString(), testData); + next(); + }), 1000); + }, next => { + // check that the previous object version is still readable + s3.getObject({ + Bucket: TEST_BUCKET, + Key: testKey, + VersionId: versionIdUtils.encode(testMd.versionId), + }, (err, data) => { + assert.ifError(err); + assert.strictEqual(data.Body.toString(), testData); + next(); + }); + }], err => { + assert.ifError(err); + done(); + }); + }); }); describe('backbeat authorization checks', () => { [{ method: 'PUT', resourceType: 'metadata' }, @@ -649,6 +749,9 @@ describeSkipIfAWS('backbeat routes', () => { method: 'PUT', bucket: TEST_BUCKET, objectKey: TEST_KEY, resourceType: 'metadata', + queryObj: { + versionId: versionIdUtils.encode(testMd.versionId), + }, authCredentials: backbeatAuthCredentials, requestBody: JSON.stringify(testMd), }, done)); From 7cdb395ee3b236f362b4c926d0ac6dd9331061eb Mon Sep 17 00:00:00 2001 From: Jonathan Gramain Date: Mon, 24 Oct 2022 17:46:49 -0700 Subject: [PATCH 2/2] bugfix: CLDSRV-290 fix `PUT /_/backbeat/metadata` versioning logic Fix the logic by always using the provided `versionId` in the query string as the version to put, instead of relying on the version stored in the metadata. Not passing a `versionId` now amounts to creating a new version. The previous logic was causing a possible confusion when no `versionId` was passed in the query string, that allowed valid data locations to be removed as if it was an overwrite. --- lib/routes/routeBackbeat.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/routes/routeBackbeat.js b/lib/routes/routeBackbeat.js index 362aec4d07..8d4983114d 100644 --- a/lib/routes/routeBackbeat.js +++ b/lib/routes/routeBackbeat.js @@ -472,12 +472,13 @@ function putMetadata(request, response, bucketInfo, objMd, log, callback) { omVal[headerName] = objMd[headerName]; }); } + const versionId = decodeVersionId(request.query); // specify both 'versioning' and 'versionId' to create a "new" // version (updating master as well) but with specified // versionId const options = { versioning: bucketInfo.isVersioningEnabled(), - versionId: omVal.versionId, + versionId, }; // If the object is from a source bucket without versioning (i.e. NFS), // then we want to create a version for the replica object even though @@ -501,6 +502,7 @@ function putMetadata(request, response, bucketInfo, objMd, log, callback) { pushReplicationMetric(objMd, omVal, bucketName, objectKey, log); if (objMd && headers['x-scal-replication-content'] !== 'METADATA' && + versionId && locationKeysHaveChanged(objMd.location, omVal.location)) { log.info('removing old data locations', { method: 'putMetadata',