CLDSRV-374 putMetadata API route is not updating null version properly by nicolas2bert · Pull Request #5167 · scality/cloudserver

nicolas2bert · 2023-05-17T19:39:42Z

Instead of using the provided "null" value, the metadata "null version id" is used when updating the metadata of a null version.

Note: In S3C, the use of the "putMetadata" call is restricted to versioned buckets.

Issue: CLDSRV-374

nicolas2bert · 2023-05-17T20:25:47Z

ping

bert-e · 2023-05-17T20:25:50Z

Hello nicolas2bert,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Status report is not available.

bert-e · 2023-05-17T20:25:56Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-374 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

7.10.28
7.70.21
8.5.21
8.6.10
8.7.21

Please check the Fix Version/s of CLDSRV-374, or the target
branch of this pull request.

nicolas2bert · 2023-05-17T20:27:26Z

ping

bert-e · 2023-05-17T20:27:37Z

Conflict

A conflict has been raised during the creation of
integration branch w/8.5/bugfix/CLDSRV-374/put-metadata-null with contents from w/7.70/bugfix/CLDSRV-374/put-metadata-null
and development/8.5.

I have not created the integration branch.

Here are the steps to resolve this conflict:

 $ git fetch
 $ git checkout -B w/8.5/bugfix/CLDSRV-374/put-metadata-null origin/development/8.5
 $ git merge origin/w/7.70/bugfix/CLDSRV-374/put-metadata-null
 $ # <intense conflict resolution>
 $ git commit
 $ git push -u origin w/8.5/bugfix/CLDSRV-374/put-metadata-null

bert-e · 2023-05-17T20:57:50Z

Integration data created

I have created the integration data for the additional destination branches.

this pull request will merge bugfix/CLDSRV-374/put-metadata-null into
development/7.10
w/7.70/bugfix/CLDSRV-374/put-metadata-null will be merged into development/7.70
w/8.5/bugfix/CLDSRV-374/put-metadata-null will be merged into development/8.5
w/8.6/bugfix/CLDSRV-374/put-metadata-null will be merged into development/8.6
w/8.7/bugfix/CLDSRV-374/put-metadata-null will be merged into development/8.7

The following branches will NOT be impacted:

development/7.4

You can set option create_pull_requests if you need me to create
integration pull requests in addition to integration branches, with:

@bert-e create_pull_requests

bert-e · 2023-05-17T20:57:53Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

jonathan-gramain · 2023-05-17T23:12:19Z

lib/routes/routeBackbeat.js

        // versionId
        const options = {
-            versioning: bucketInfo.isVersioningEnabled(),
+            versioning,


It could be cleaner to

prepare the final versioning and versionId fields to pass in options

build the options object with those two fields

I did not change the existing logic much to have the change easily forwarded into development/8.x branches which have a slightly different logic.

jonathan-gramain · 2023-05-17T23:17:02Z

lib/routes/routeBackbeat.js

+            if (versioning && !nullVersionId) {
+                // If the null version does not have a version id, it is a current null version.
+                // To update the metadata of a current version, versioning is set to false.
+                options.versioning = false;


Beware of this, as option fields are converted to strings when passed to Metadata (through the query string), there's a good chance that we interpret versioning: "false" as being true in Metadata, I'd rather recommend to not pass versioning at all if it is false. Metadata should have better parsing of its options on the other hand but I don't think it's done properly now.

bert-e · 2023-05-18T12:56:34Z

History mismatch

Merge commit #048a10b191caa788ca526a2690d2a75177e0ad1a on the integration branch
w/7.70/bugfix/CLDSRV-374/put-metadata-null is merging a branch which is neither the current
branch bugfix/CLDSRV-374/put-metadata-null nor the development branch
development/7.70.

It is likely due to a rebase of the branch bugfix/CLDSRV-374/put-metadata-null and the
merge is not possible until all related w/* branches are deleted or updated.

Please use the reset command to have me reinitialize these branches.

jonathan-gramain · 2023-05-18T17:03:45Z

lib/routes/routeBackbeat.js

+        // NOTE: When 'versioning' is set to true and no 'versionId' is specified,
+        // it results in the creation of a "new" version, which also updates the master.


Possibly this comment could fit better above if (versioning) {

jonathan-gramain · 2023-05-18T17:07:46Z

tests/functional/raw-node/test/routes/routeBackbeat.js

+                    .then(() => done());
+            });
+
+            it('should update metadata of a current null version', done => {


Would you mind adding some similar tests on versioning-suspended buckets:

when there is an existing null version

when there is no existing null version

and make sure we only delete the data when a prior null version exists

In S3C, the use of the "putMetadata" call is restricted to versioned buckets.
I will add tests for Zenko/Artesca branches that support PUT metadata for non-versioned buckets.

jonathan-gramain · 2023-05-18T17:15:45Z

lib/routes/routeBackbeat.js

+                    // - if a version ID is provided in the query string (updating MD of a specific version) OR
+                    // - if no version ID provided and versioning is not enabled (updating MD of the master key),
+                    // data locations are allowed to be removed as they will be overwritten.
+                    (versionId || !versioning) &&


I have a doubt that we may remove valid data in the following case, not completely sure, to be checked:

versioning-suspended bucket

no existing null version

I am thinking that if objMd is the metadata of the current master (a real version), we may remove the master's data and create a new null version, making the old master unreadable.

If there is no existing null version, the object MD (objMd) of the null version will be undefined, resulting in the condition being false.

lib/routes/routeBackbeat.js

bert-e · 2023-05-24T17:15:39Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-374 contains:

7.10.28
7.70.21
8.5.21
8.6.10
8.7.21

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

8.7.21

Please check the Fix Version/s of CLDSRV-374, or the target
branch of this pull request.

nicolas2bert · 2023-05-24T17:18:12Z

While it's not the preferred scenario, due to the urgent need to implement this fix in Artesca, we will initially integrate it into the development/8.7 branch and subsequently backport it to the development/7.x branches.

bert-e · 2023-05-24T17:30:13Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

jonathan-gramain · 2023-05-25T07:56:17Z

tests/multipleBackend/routes/routeBackbeat.js

            .then(() => done());
    });

+    describe.only('null version', () => {


We eliminated the '.skip' modifier in the 'describe' section of the parent.
Initially, all tests were being skipped.
However, I unskipped them and only executed the null test for the time being.

To address the remaining routeBackbeat tests, I created a follow-up task CLDSRV-394, which is mentioned here: https://github.com/scality/cloudserver/pull/5167/files#diff-e346bc0177b28612b0892a923d8c1d33429b4d7a3c8bd955aadf6e3ccb701dcdR1281.

It may cause confusion to have a committed .only test, usually we follow the pattern of skipping tests until they pass, I think we should follow this scheme here too, so skipping the rest of the tests in this suite and removing .only of the only test we'd like to activate may be more appropriate.

jonathan-gramain · 2023-05-25T08:08:14Z

tests/multipleBackend/routes/routeBackbeat.js

+        it('should create a new null version if versioning suspended and no version', done => {
+            let objMD;
+            return async.series([
+                next => s3.putBucketVersioning({ Bucket: bucket, VersioningConfiguration: { Status: 'Enabled' } },


You don't need to enable versioning before suspending it, you can just put a versioning-suspended status on a nonversioned bucket (same for the other tests).

Indeed, I simply prefer having it Enabled prior to Suspending it as it feels more instinctive to me.
However, as enabling it beforehand is not obligatory, I will eliminate that step.

jonathan-gramain · 2023-05-25T08:09:16Z

tests/multipleBackend/routes/routeBackbeat.js

+                    next),
+                next => s3.putBucketVersioning({ Bucket: bucket, VersioningConfiguration: { Status: 'Suspended' } },
+                    next),
+                next => s3.putObject({ Bucket: bucket, Key: keyName, Body: new Buffer(testData) }, next),


Is it useful to PUT and DELETE an object prior to using the backbeat PUT?

For the test to be closer to reality and to be able "GET" the null version metadata that I will later "PUT".

jonathan-gramain · 2023-05-25T08:13:22Z

tests/multipleBackend/routes/routeBackbeat.js

+                    next),
+                next => s3.putBucketVersioning({ Bucket: bucket, VersioningConfiguration: { Status: 'Suspended' } },
+                    next),
+                next => s3.putObject({ Bucket: bucket, Key: keyName, Body: new Buffer(testData) }, next),


There's probably no need to PUT an object first before DELETE, as a DELETE would normally put a delete marker with a null version.

Same reason as earlier.
Also, we create the object to GET its metadata.
In my opinion, this appears to be a more realistic scenario.

jonathan-gramain · 2023-05-25T08:19:55Z

Thanks for the extensive test coverage @nicolas2bert

Instead of using the provided "null" value, the metadata "null version id" is now used when updating the metadata of a null version.

lib/routes/routeBackbeat.js

nicolas2bert · 2023-05-25T16:56:23Z

@bert-e approve

bert-e · 2023-05-25T16:56:36Z

In the queue

The changeset has received all authorizations and has been added to the
relevant queue(s). The queue(s) will be merged in the target development
branch(es) as soon as builds have passed.

The changeset will be merged in:

✔️ development/8.7

The following branches will NOT be impacted:

development/7.10
development/7.4
development/7.70
development/8.5
development/8.6

There is no action required on your side. You will be notified here once
the changeset has been merged. In the unlikely event that the changeset
fails permanently on the queue, a member of the admin team will
contact you to help resolve the matter.

IMPORTANT

Please do not attempt to modify this pull request.

Any commit you add on the source branch will trigger a new cycle after the
current queue is merged.
Any commit you add on one of the integration branches will be lost.

If you need this pull request to be removed from the queue, please contact a
member of the admin team now.

The following options are set: approve

bert-e · 2023-05-25T16:56:48Z

I have successfully merged the changeset of this pull request
into targetted development branches:

✔️ development/8.7

The following branches have NOT changed:

development/7.10
development/7.4
development/7.70
development/8.5
development/8.6

Please check the status of the associated issue CLDSRV-374.

Goodbye nicolas2bert.

Revert 7e405ff, as it prevents creating new versions in versioned buckets when not specifying versionId. The proper fix was introduced later in [2] [1] #4655 [2] #5167 Issue: CLDSRV-394

jonathan-gramain reviewed May 17, 2023

View reviewed changes

nicolas2bert force-pushed the bugfix/CLDSRV-374/put-metadata-null branch from 048a10b to 56ffbb2 Compare May 18, 2023 12:56

nicolas2bert force-pushed the bugfix/CLDSRV-374/put-metadata-null branch 2 times, most recently from 18e0f2b to 1df4eb9 Compare May 18, 2023 15:59

jonathan-gramain reviewed May 18, 2023

View reviewed changes

nicolas2bert force-pushed the bugfix/CLDSRV-374/put-metadata-null branch from 1df4eb9 to f2e9446 Compare May 22, 2023 11:46

welansari reviewed May 23, 2023

View reviewed changes

lib/routes/routeBackbeat.js Show resolved Hide resolved

nicolas2bert force-pushed the bugfix/CLDSRV-374/put-metadata-null branch from f2e9446 to 16024d6 Compare May 24, 2023 17:04

nicolas2bert changed the base branch from development/7.10 to development/8.7 May 24, 2023 17:15

nicolas2bert force-pushed the bugfix/CLDSRV-374/put-metadata-null branch 2 times, most recently from 0f59a73 to 16434a4 Compare May 24, 2023 22:14

jonathan-gramain reviewed May 25, 2023

View reviewed changes

nicolas2bert force-pushed the bugfix/CLDSRV-374/put-metadata-null branch from 16434a4 to 9205084 Compare May 25, 2023 12:45

CLDSRV-374 putMetadata API route is not updating null version properly

50cb6a2

Instead of using the provided "null" value, the metadata "null version id" is now used when updating the metadata of a null version.

nicolas2bert force-pushed the bugfix/CLDSRV-374/put-metadata-null branch from 9205084 to 50cb6a2 Compare May 25, 2023 13:40

jonathan-gramain approved these changes May 25, 2023

View reviewed changes

welansari reviewed May 25, 2023

View reviewed changes

lib/routes/routeBackbeat.js Show resolved Hide resolved

welansari approved these changes May 25, 2023

View reviewed changes

bert-e merged commit 50cb6a2 into development/8.7 May 25, 2023

bert-e deleted the bugfix/CLDSRV-374/put-metadata-null branch May 25, 2023 16:56

		// NOTE: When 'versioning' is set to true and no 'versionId' is specified,
		// it results in the creation of a "new" version, which also updates the master.

Conversation

nicolas2bert commented May 17, 2023 • edited by francoisferrand Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas2bert commented May 17, 2023

Uh oh!

bert-e commented May 17, 2023

Hello nicolas2bert,

Uh oh!

bert-e commented May 17, 2023

Incorrect fix version

Uh oh!

nicolas2bert commented May 17, 2023

Uh oh!

bert-e commented May 17, 2023

Conflict

Uh oh!

bert-e commented May 17, 2023

Integration data created

Uh oh!

bert-e commented May 17, 2023

Waiting for approval

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas2bert May 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bert-e commented May 18, 2023

History mismatch

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bert-e commented May 24, 2023

Incorrect fix version

Uh oh!

nicolas2bert commented May 24, 2023

Uh oh!

bert-e commented May 24, 2023

Waiting for approval

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathan-gramain commented May 25, 2023

Uh oh!

Uh oh!

nicolas2bert commented May 25, 2023

Uh oh!

bert-e commented May 25, 2023

In the queue

nicolas2bert commented May 17, 2023 •

edited by francoisferrand

Loading

nicolas2bert May 18, 2023 •

edited

Loading