Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions lib/api/apiUtils/object/bumpMicroVersionId.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
const { versioning } = require('arsenal');
const { config } = require('../../../Config');

/**
* Bump objectMD.microVersionId. microVersionId is a generic
* metadata-revision marker, not a CRR-specific field, but cascaded CRR
* is its only consumer today - so we gate on replicationInfo to avoid
* inflating storage on objects that wouldn't use it. The gate can be
* widened later if another consumer needs it on every object.
* Pass `force = true` to bump unconditionally.
*
* @param {object} objectMD - object MD POJO or `md.getValue()`
* @param {boolean} [force] - bump even without replicationInfo
* @return {undefined}
*/
function bumpMicroVersionId(objectMD, force) {
if (!force && !objectMD?.replicationInfo) {
return;
}

const { instanceId, replicationGroupId } = config;

// eslint-disable-next-line no-param-reassign
objectMD.microVersionId = versioning.VersionID.generateVersionId(instanceId, replicationGroupId);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't there a method ObjectMD.updateMicroVersionId() in arsenal ? should it not be used instead?

(if we can't because of POJO, then I wonder if that function was really useful....)

}

module.exports = bumpMicroVersionId;
6 changes: 6 additions & 0 deletions lib/api/objectDeleteTagging.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const monitoring = require('../utilities/monitoringHandler');
const collectCorsHeaders = require('../utilities/collectCorsHeaders');
const metadata = require('../metadata/wrapper');
const getReplicationInfo = require('./apiUtils/object/getReplicationInfo');
const bumpMicroVersionId = require('./apiUtils/object/bumpMicroVersionId');
const { data } = require('../data/wrapper');
const { config } = require('../Config');
const REPLICATION_ACTION = 'DELETE_TAGGING';
Expand Down Expand Up @@ -76,13 +77,18 @@ function objectDeleteTagging(authInfo, request, log, callback) {
// eslint-disable-next-line no-param-reassign
objectMD.tags = {};
const params = getVersionSpecificMetadataOptions(objectMD, config.nullVersionCompatMode);
if (objectMD.replicationInfo?.isReplica) {
// eslint-disable-next-line no-param-reassign
objectMD.replicationInfo.isReplica = false;
}
const replicationInfo = getReplicationInfo(config,
objectKey, bucket, true, 0, REPLICATION_ACTION, objectMD);
if (replicationInfo) {
// eslint-disable-next-line no-param-reassign
objectMD.replicationInfo = Object.assign({},
objectMD.replicationInfo, replicationInfo);
}
bumpMicroVersionId(objectMD);
Comment on lines +80 to +91
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (objectMD.replicationInfo?.isReplica) {
// eslint-disable-next-line no-param-reassign
objectMD.replicationInfo.isReplica = false;
}
const replicationInfo = getReplicationInfo(config,
objectKey, bucket, true, 0, REPLICATION_ACTION, objectMD);
if (replicationInfo) {
// eslint-disable-next-line no-param-reassign
objectMD.replicationInfo = Object.assign({},
objectMD.replicationInfo, replicationInfo);
}
bumpMicroVersionId(objectMD);
const replicationInfo = getReplicationInfo(config, objectKey, bucket, true, 0, REPLICATION_ACTION, objectMD);
if (replicationInfo) {
// eslint-disable-next-line no-param-reassign
objectMD.replicationInfo = Object.assign({},
objectMD.replicationInfo, replicationInfo);
if (objectMD.replicationInfo?.isReplica) {
// eslint-disable-next-line no-param-reassign
objectMD.replicationInfo.isReplica = false;
}
bumpMicroVersionId(objectMD);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(same in all files...)

// eslint-disable-next-line no-param-reassign
objectMD.originOp = 's3:ObjectTagging:Delete';
metadata.putObjectMD(bucket.getName(), objectKey, objectMD, params,
Expand Down
6 changes: 6 additions & 0 deletions lib/api/objectPutLegalHold.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const collectCorsHeaders = require('../utilities/collectCorsHeaders');
const { decodeVersionId, getVersionIdResHeader, getVersionSpecificMetadataOptions } =
require('./apiUtils/object/versioning');
const getReplicationInfo = require('./apiUtils/object/getReplicationInfo');
const bumpMicroVersionId = require('./apiUtils/object/bumpMicroVersionId');
const metadata = require('../metadata/wrapper');
const { standardMetadataValidateBucketAndObj } = require('../metadata/metadataUtils');
const { pushMetric } = require('../utapi/utilities');
Expand Down Expand Up @@ -87,13 +88,18 @@ function objectPutLegalHold(authInfo, request, log, callback) {
// eslint-disable-next-line no-param-reassign
objectMD.legalHold = legalHold;
const params = getVersionSpecificMetadataOptions(objectMD, config.nullVersionCompatMode);
if (objectMD.replicationInfo?.isReplica) {
// eslint-disable-next-line no-param-reassign
objectMD.replicationInfo.isReplica = false;
}
Comment on lines +91 to +94
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this not be handled in getReplicationInfo ? (I think this function is not really a getter, but really "builds" the new replication info...)

const replicationInfo = getReplicationInfo(config,
objectKey, bucket, true, 0, REPLICATION_ACTION, objectMD);
if (replicationInfo) {
// eslint-disable-next-line no-param-reassign
objectMD.replicationInfo = Object.assign({},
objectMD.replicationInfo, replicationInfo);
}
bumpMicroVersionId(objectMD);
// eslint-disable-next-line no-param-reassign
objectMD.originOp = 's3:ObjectLegalHold:Put';
metadata.putObjectMD(bucket.getName(), objectKey, objectMD, params,
Expand Down
5 changes: 5 additions & 0 deletions lib/api/objectPutRetention.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const { ObjectLockInfo, hasGovernanceBypassHeader } =
const { standardMetadataValidateBucketAndObj } = require('../metadata/metadataUtils');
const { pushMetric } = require('../utapi/utilities');
const getReplicationInfo = require('./apiUtils/object/getReplicationInfo');
const bumpMicroVersionId = require('./apiUtils/object/bumpMicroVersionId');
const collectCorsHeaders = require('../utilities/collectCorsHeaders');
const metadata = require('../metadata/wrapper');
const { config } = require('../Config');
Expand Down Expand Up @@ -112,12 +113,16 @@ function objectPutRetention(authInfo, request, log, callback) {
objectMD.retentionMode = retentionInfo.mode;
objectMD.retentionDate = retentionInfo.date;
const params = getVersionSpecificMetadataOptions(objectMD, config.nullVersionCompatMode);
if (objectMD.replicationInfo?.isReplica) {
objectMD.replicationInfo.isReplica = false;
}
const replicationInfo = getReplicationInfo(config,
objectKey, bucket, true, 0, REPLICATION_ACTION, objectMD);
if (replicationInfo) {
objectMD.replicationInfo = Object.assign({},
objectMD.replicationInfo, replicationInfo);
}
bumpMicroVersionId(objectMD);
objectMD.originOp = 's3:ObjectRetention:Put';
/* eslint-enable no-param-reassign */
metadata.putObjectMD(bucket.getName(), objectKey, objectMD, params,
Expand Down
6 changes: 6 additions & 0 deletions lib/api/objectPutTagging.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const { standardMetadataValidateBucketAndObj } = require('../metadata/metadataUt
const { pushMetric } = require('../utapi/utilities');
const monitoring = require('../utilities/monitoringHandler');
const getReplicationInfo = require('./apiUtils/object/getReplicationInfo');
const bumpMicroVersionId = require('./apiUtils/object/bumpMicroVersionId');
const collectCorsHeaders = require('../utilities/collectCorsHeaders');
const metadata = require('../metadata/wrapper');
const { data } = require('../data/wrapper');
Expand Down Expand Up @@ -80,13 +81,18 @@ function objectPutTagging(authInfo, request, log, callback) {
// eslint-disable-next-line no-param-reassign
objectMD.tags = tags;
const params = getVersionSpecificMetadataOptions(objectMD, config.nullVersionCompatMode);
if (objectMD.replicationInfo?.isReplica) {
// eslint-disable-next-line no-param-reassign
objectMD.replicationInfo.isReplica = false;
}
const replicationInfo = getReplicationInfo(config,
objectKey, bucket, true, 0, REPLICATION_ACTION, objectMD);
if (replicationInfo) {
// eslint-disable-next-line no-param-reassign
objectMD.replicationInfo = Object.assign({},
objectMD.replicationInfo, replicationInfo);
}
bumpMicroVersionId(objectMD);
// eslint-disable-next-line no-param-reassign
objectMD.originOp = 's3:ObjectTagging:Put';
metadata.putObjectMD(bucket.getName(), objectKey, objectMD, params,
Expand Down
7 changes: 7 additions & 0 deletions lib/metadata/acl.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
const { errors } = require('arsenal');

const getReplicationInfo = require('../api/apiUtils/object/getReplicationInfo');
const bumpMicroVersionId = require('../api/apiUtils/object/bumpMicroVersionId');
const aclUtils = require('../utilities/aclUtils');
const constants = require('../../constants');
const metadata = require('../metadata/wrapper');
Expand Down Expand Up @@ -44,6 +45,10 @@ const acl = {
objectMD.acl = addACLParams;
objectMD.originOp = 's3:ObjectAcl:Put';

if (objectMD.replicationInfo?.isReplica) {
objectMD.replicationInfo.isReplica = false;
}

// Use storageType to determine if replication update is needed, as it is set only for
// "cloud" locations. This ensures that we reset replication when CRR is used, but not
// when multi-backend replication (i.e. Zenko) is used.
Expand All @@ -57,6 +62,8 @@ const acl = {
};
}

bumpMicroVersionId(objectMD);

return metadata.putObjectMD(bucket.getName(), objectKey, objectMD, params, log, cb);
}
return cb();
Expand Down
5 changes: 2 additions & 3 deletions lib/services.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const metadata = require('./metadata/wrapper');
const { setObjectLockInformation }
= require('./api/apiUtils/object/objectLockHelpers');
const removeAWSChunked = require('./api/apiUtils/object/removeAWSChunked');
const bumpMicroVersionId = require('./api/apiUtils/object/bumpMicroVersionId');
const { parseTagFromQuery } = s3middleware.tagging;

const usersBucket = constants.usersBucket;
Expand Down Expand Up @@ -190,9 +191,7 @@ const services = {
options.replayId = uploadId;
}
// update microVersionId when overwriting metadata.
if (updateMicroVersionId) {
md.updateMicroVersionId();
}
bumpMicroVersionId(md.getValue(), updateMicroVersionId);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • is this not redundant with the other calls in APIs ?
  • calling it here we ALWAYS generate a new microVersionId when creating a new object: which is not needed since it comes with a new versionId... (except for non-versioned case, but no replication there: so the function does nothing)

// update restore
if (archive) {
md.setAmzStorageClass(amzStorageClass);
Expand Down
8 changes: 5 additions & 3 deletions lib/utilities/collectResponseHeaders.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,11 @@ function collectResponseHeaders(objectMD, corsHeaders, versioningCfg,
responseMetaHeaders['x-amz-object-lock-legal-hold']
= objectMD.legalHold ? 'ON' : 'OFF';
}
if (objectMD.replicationInfo && objectMD.replicationInfo.status) {
responseMetaHeaders['x-amz-replication-status'] =
objectMD.replicationInfo.status;
if (objectMD.replicationInfo) {
const { isReplica, status } = objectMD.replicationInfo;
if (isReplica || status) {
responseMetaHeaders['x-amz-replication-status'] = isReplica ? 'REPLICA' : status;
}
}
if (Array.isArray(objectMD?.replicationInfo?.backends)) {
objectMD.replicationInfo.backends.forEach(backend => {
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
"@azure/storage-blob": "^12.28.0",
"@hapi/joi": "^17.1.1",
"@smithy/node-http-handler": "^3.0.0",
"arsenal": "git+https://github.com/scality/Arsenal#8.4.2",
"arsenal": "git+https://github.com/scality/Arsenal#improvement/ARSN-578/micro-version-id",
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arsenal is pinned to a branch (improvement/ARSN-578/micro-version-id) instead of a tag. Git-based deps should be pinned to a tag for reproducible builds.

— Claude Code

"async": "2.6.4",
"bucketclient": "scality/bucketclient#8.2.7",
"bufferutil": "^4.0.8",
Expand Down
31 changes: 31 additions & 0 deletions tests/unit/api/apiUtils/object/bumpMicroVersionId.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
const assert = require('assert');

const bumpMicroVersionId = require('../../../../../lib/api/apiUtils/object/bumpMicroVersionId');

describe('bumpMicroVersionId', () => {
it('sets a fresh microVersionId when replicationInfo is present', () => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test names using it() should start with "should".

Suggested change
it('sets a fresh microVersionId when replicationInfo is present', () => {
it('should set a fresh microVersionId when replicationInfo is present', () => {

— Claude Code

const objectMD = { replicationInfo: {} };
bumpMicroVersionId(objectMD);
assert(objectMD.microVersionId, 'expected microVersionId to be set');
});

it('produces a different value on each call', () => {
const objectMD = { replicationInfo: {} };
bumpMicroVersionId(objectMD);
const first = objectMD.microVersionId;
bumpMicroVersionId(objectMD);
assert.notStrictEqual(objectMD.microVersionId, first);
});

it('does nothing when replicationInfo is absent', () => {
const objectMD = {};
bumpMicroVersionId(objectMD);
assert.strictEqual(objectMD.microVersionId, undefined);
});

it('bumps unconditionally when force is true', () => {
const objectMD = {};
bumpMicroVersionId(objectMD, true);
assert(objectMD.microVersionId, 'expected microVersionId to be set when force=true');
});
});
53 changes: 53 additions & 0 deletions tests/unit/api/objectReplicationMD.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
const assert = require('assert');
const async = require('async');
const crypto = require('crypto');
const { promisify } = require('util');

const BucketInfo = require('arsenal').models.BucketInfo;

Expand Down Expand Up @@ -718,3 +719,55 @@ describe('Replication object MD without bucket replication config', () => {
});
});
});

describe('microVersionId is bumped on every object metadata write', () => {
const getMD = key => metadata.keyMaps.get(bucketName).get(key);
const objectPutAsync = promisify(objectPut);
const objectPutTaggingAsync = promisify(objectPutTagging);

beforeEach(() => {
cleanup();
createBucket();
});

afterEach(() => cleanup());

it('sets microVersionId on objectPut', async () => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test names using it() should start with "should".

Suggested change
it('sets microVersionId on objectPut', async () => {
it('should set microVersionId on objectPut', async () => {

— Claude Code

await objectPutAsync(authInfo, getObjectPutReq(keyA, true), undefined, log);
assert(getMD(keyA).microVersionId, 'expected microVersionId to be set');
});

it('bumps microVersionId on subsequent objectPutTagging', async () => {
await objectPutAsync(authInfo, getObjectPutReq(keyA, true), undefined, log);
const before = getMD(keyA).microVersionId;
await objectPutTaggingAsync(authInfo, taggingPutReq, log);
const after = getMD(keyA).microVersionId;
assert(after && after !== before, 'expected microVersionId to change after tagging');
});
});

describe('isReplica is cleared on direct user writes overwriting a replica', () => {
const getMD = key => metadata.keyMaps.get(bucketName).get(key);
const objectPutAsync = promisify(objectPut);
const objectPutTaggingAsync = promisify(objectPutTagging);

beforeEach(() => {
cleanup();
createBucket();
});

afterEach(() => cleanup());

it('clears isReplica when prior MD has it true', async () => {
await objectPutAsync(authInfo, getObjectPutReq(keyA, true), undefined, log);
getMD(keyA).replicationInfo.isReplica = true;
await objectPutTaggingAsync(authInfo, taggingPutReq, log);
assert.strictEqual(getMD(keyA).replicationInfo.isReplica, false);
});

it('leaves isReplica untouched when prior MD does not have it', async () => {
await objectPutAsync(authInfo, getObjectPutReq(keyA, true), undefined, log);
await objectPutTaggingAsync(authInfo, taggingPutReq, log);
assert.strictEqual(getMD(keyA).replicationInfo.isReplica, undefined);
});
});
16 changes: 16 additions & 0 deletions tests/unit/utils/collectResponseHeaders.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,22 @@ describe('Middleware: Collect Response Headers', () => {
assert.deepStrictEqual(headers['x-amz-replication-status'], 'REPLICA');
});

it('should set REPLICA header from isReplica even when status is PENDING', () => {
const objectMD = {
replicationInfo: { status: 'PENDING', isReplica: true },
};
const headers = collectResponseHeaders(objectMD);
assert.deepStrictEqual(headers['x-amz-replication-status'], 'REPLICA');
});

it('should use replicationInfo.status when isReplica is false', () => {
const objectMD = {
replicationInfo: { status: 'PENDING', isReplica: false },
};
const headers = collectResponseHeaders(objectMD);
assert.deepStrictEqual(headers['x-amz-replication-status'], 'PENDING');
});

it('should set the replication status of each site', () => {
const objectMD = {
replicationInfo: {
Expand Down
4 changes: 2 additions & 2 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -6143,9 +6143,9 @@ arraybuffer.prototype.slice@^1.0.4:
optionalDependencies:
ioctl "^2.0.2"

"arsenal@git+https://github.com/scality/Arsenal#8.4.2":
"arsenal@git+https://github.com/scality/Arsenal#improvement/ARSN-578/micro-version-id":
version "8.4.2"
resolved "git+https://github.com/scality/Arsenal#2312744c5e6bdd4d0cc9d60bf4cdcf10f32461e6"
resolved "git+https://github.com/scality/Arsenal#7be6414628ae9c3cf4ee4fe1b91990363bf0fa28"
dependencies:
"@aws-sdk/client-kms" "^3.975.0"
"@aws-sdk/client-s3" "^3.975.0"
Expand Down
Loading