Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLDSRV-598: Add overhreadField to metadata-only operations #5718

Closed
wants to merge 2 commits into from
Closed

CLDSRV-598: Add overhreadField to metadata-only operations #5718

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f5c1240
CLDSRV-598: Add overhreadField to md-only ops
dvasilas Dec 18, 2024
1681dac
CLDSRV-598: Bump version to 7.70.59
dvasilas Dec 18, 2024
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
2 changes: 2 additions & 0 deletions lib/api/objectPutACL.js
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

As a result, the entries in the metadata op log for those operations do not include overhead information, and scuba interprets them as object puts, resulting in wrong metric updates.

We may want to also review the metadata operations done in backbeat or route backbeat here, just to make sure they doesn't lead to wrong behavior in scuba

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, don't we have the same issue with metadata-only deletes, that also alter the metadata?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We may want to also review the metadata operations done in backbeat or route backbeat here, just to make sure they doesn't lead to wrong behavior in scuba.

I created https://scality.atlassian.net/browse/S3C-9618 for that.

Also, don't we have the same issue with metadata-only deletes, that also alter the metadata?

Good point !
I see objectDeleteTagging that should be fixed. Is there another one that I am missing ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see anything else, but maybe @Kerkesni or @francoisferrand will have some

Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const { decodeVersionId, getVersionIdResHeader, getVersionSpecificMetadataOption
const { standardMetadataValidateBucketAndObj } = require('../metadata/metadataUtils');
const monitoring = require('../utilities/metrics');
const { config } = require('../Config');
const { overheadField } = require('../../constants');

/*
Format of xml request:
Expand Down Expand Up @@ -282,6 +283,7 @@ function objectPutACL(authInfo, request, log, cb) {
function addAclsToObjMD(bucket, objectMD, ACLParams, next) {
// Add acl's to object metadata
const params = getVersionSpecificMetadataOptions(objectMD, config.nullVersionCompatMode);
params.overheadField = overheadField;
acl.addObjectACL(bucket, objectKey, objectMD,
ACLParams, params, log, err => next(err, bucket, objectMD));
},
Expand Down
2 changes: 2 additions & 0 deletions lib/api/objectPutLegalHold.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const metadata = require('../metadata/wrapper');
const { standardMetadataValidateBucketAndObj } = require('../metadata/metadataUtils');
const { pushMetric } = require('../utapi/utilities');
const { config } = require('../Config');
const { overheadField } = require('../../constants');

const { parseLegalHoldXml } = s3middleware.objectLegalHold;

Expand Down Expand Up @@ -87,6 +88,7 @@ function objectPutLegalHold(authInfo, request, log, callback) {
// eslint-disable-next-line no-param-reassign
objectMD.legalHold = legalHold;
const params = getVersionSpecificMetadataOptions(objectMD, config.nullVersionCompatMode);
params.overheadField = overheadField;
const replicationInfo = getReplicationInfo(objectKey, bucket, true,
0, REPLICATION_ACTION, objectMD);
if (replicationInfo) {
Expand Down
2 changes: 2 additions & 0 deletions lib/api/objectPutRetention.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const getReplicationInfo = require('./apiUtils/object/getReplicationInfo');
const collectCorsHeaders = require('../utilities/collectCorsHeaders');
const metadata = require('../metadata/wrapper');
const { config } = require('../Config');
const { overheadField } = require('../../constants');

const { parseRetentionXml } = s3middleware.retention;
const REPLICATION_ACTION = 'PUT_RETENTION';
Expand Down Expand Up @@ -125,6 +126,7 @@ function objectPutRetention(authInfo, request, log, callback) {
objectMD.retentionMode = retentionInfo.mode;
objectMD.retentionDate = retentionInfo.date;
const params = getVersionSpecificMetadataOptions(objectMD, config.nullVersionCompatMode);
params.overheadField = overheadField;
const replicationInfo = getReplicationInfo(objectKey, bucket, true,
0, REPLICATION_ACTION, objectMD);
if (replicationInfo) {
Expand Down
3 changes: 3 additions & 0 deletions lib/api/objectPutTagging.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ const metadata = require('../metadata/wrapper');
const { data } = require('../data/wrapper');
const { parseTagXml } = s3middleware.tagging;
const { config } = require('../Config');
const { overheadField } = require('../../constants');

const REPLICATION_ACTION = 'PUT_TAGGING';

/**
Expand Down Expand Up @@ -81,6 +83,7 @@ function objectPutTagging(authInfo, request, log, callback) {
// eslint-disable-next-line no-param-reassign
objectMD.tags = tags;
const params = getVersionSpecificMetadataOptions(objectMD, config.nullVersionCompatMode);
params.overheadField = overheadField;
const replicationInfo = getReplicationInfo(objectKey, bucket, true,
0, REPLICATION_ACTION, objectMD);
if (replicationInfo) {
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "s3",
"version": "7.70.58",
"version": "7.70.59",
"description": "S3 connector",
"main": "index.js",
"engines": {
Expand Down
52 changes: 52 additions & 0 deletions tests/unit/api/objectPutACL.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const assert = require('assert');
const async = require('async');
const sinon = require('sinon');

const { errors } = require('arsenal');
const AuthInfo = require('arsenal').auth.AuthInfo;
Expand Down Expand Up @@ -480,6 +481,57 @@ describe('putObjectACL API', () => {
});
});
});

describe('overheadField', () => {
before(done => {
cleanup();
sinon.spy(metadata, 'putObjectMD');
return done();
});

after(() => {
metadata.putObjectMD.restore();
cleanup();
});

it('should pass overheadField', done => {
const testObjACLRequest = {
bucketName,
namespace,
objectKey: objectName,
headers: {
'x-amz-grant-full-control':
'emailaddress="[email protected]"' +
',emailaddress="[email protected]"',
'x-amz-grant-read': `uri=${constants.logId}`,
'x-amz-grant-read-acp': `id=${ownerID}`,
'x-amz-grant-write-acp': `id=${anotherID}`,
},
url: `/${bucketName}/${objectName}?acl`,
query: { acl: '' },
actionImplicitDenies: false,
};
bucketPut(authInfo, testPutBucketRequest, log, () => {
objectPut(authInfo, testPutObjectRequest, undefined, log,
(err) => {
assert.ifError(err);
objectPutACL(authInfo, testObjACLRequest, log, err => {
assert.ifError(err);
sinon.assert.calledWith(
metadata.putObjectMD,
sinon.match.string,
objectName,
sinon.match.any,
sinon.match({ overheadField: sinon.match.array }),
sinon.match.any,
sinon.match.any
);
done();
});
});
});
});
});
});

describe('with bucket policy', () => {
Expand Down
30 changes: 30 additions & 0 deletions tests/unit/api/objectPutLegalHold.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
const assert = require('assert');
const sinon = require('sinon');

const { bucketPut } = require('../../../lib/api/bucketPut');
const objectPut = require('../../../lib/api/objectPut');
Expand Down Expand Up @@ -98,5 +99,34 @@ describe('putObjectLegalHold API', () => {
});
});
});

describe('overheadField', () => {
before(done => {
cleanup();
sinon.spy(metadata, 'putObjectMD');
return done();
});

after(() => {
metadata.putObjectMD.restore();
cleanup();
});

it('should pass overheadField', done => {
objectPutLegalHold(authInfo, putLegalHoldReq('OFF'), log, err => {
assert.ifError(err);
sinon.assert.calledWith(
metadata.putObjectMD,
sinon.match.string,
objectName,
sinon.match.any,
sinon.match({ overheadField: sinon.match.array }),
sinon.match.any,
sinon.match.any
);
done();
});
});
});
});
});
30 changes: 30 additions & 0 deletions tests/unit/api/objectPutRetention.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const assert = require('assert');
const moment = require('moment');
const sinon = require('sinon');

const { errors } = require('arsenal');
const { bucketPut } = require('../../../lib/api/bucketPut');
Expand Down Expand Up @@ -223,5 +224,34 @@ describe('putObjectRetention API', () => {
});
});
});

describe('overheadField', () => {
before(done => {
cleanup();
sinon.spy(metadata, 'putObjectMD');
return done();
});

after(() => {
metadata.putObjectMD.restore();
cleanup();
});

it('should pass overheadField', done => {
objectPutRetention(authInfo, putObjRetRequestGovernance, log, err => {
assert.ifError(err);
sinon.assert.calledWith(
metadata.putObjectMD,
sinon.match.string,
objectName,
sinon.match.any,
sinon.match({ overheadField: sinon.match.array }),
sinon.match.any,
sinon.match.any
);
done();
});
});
});
});
});
33 changes: 33 additions & 0 deletions tests/unit/api/objectPutTagging.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
const assert = require('assert');
const sinon = require('sinon');

const { bucketPut } = require('../../../lib/api/bucketPut');
const objectPut = require('../../../lib/api/objectPut');
Expand Down Expand Up @@ -91,6 +92,38 @@ describe('putObjectTagging API', () => {
});
});
});

describe('overheadField', () => {
before(done => {
cleanup();
sinon.spy(metadata, 'putObjectMD');
return done();
});

after(() => {
metadata.putObjectMD.restore();
cleanup();
});

it('should pass overheadField', done => {
const taggingUtil = new TaggingConfigTester();
const testObjectPutTaggingRequest = taggingUtil
.createObjectTaggingRequest('PUT', bucketName, objectName);
objectPutTagging(authInfo, testObjectPutTaggingRequest, log, err => {
assert.ifError(err);
sinon.assert.calledWith(
metadata.putObjectMD,
sinon.match.string,
objectName,
sinon.match.any,
sinon.match({ overheadField: sinon.match.array }),
sinon.match.any,
sinon.match.any
);
done();
});
});
});
});

describe('PUT object tagging :: helper validation functions ', () => {
Expand Down
Loading