CLDSRV754: add GetBucketLogging and PutBucketLogging endpoints #5966

leif-scality · 2025-10-06T17:24:59Z

Add GetBucketLogging and PutBucketLogging endpoints

The package.json changes will be reverted before change

bert-e · 2025-10-06T17:25:03Z

Hello leif-scality,

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.

Available options

name	description	privileged	authored
`/after_pull_request`	Wait for the given pull request id to be merged before continuing with the current one.
`/bypass_author_approval`	Bypass the pull request author's approval	⭐
`/bypass_build_status`	Bypass the build and test status	⭐
`/bypass_commit_size`	Bypass the check on the size of the changeset `TBA`	⭐
`/bypass_incompatible_branch`	Bypass the check on the source branch prefix	⭐
`/bypass_jira_check`	Bypass the Jira issue check	⭐
`/bypass_peer_approval`	Bypass the pull request peers' approval	⭐
`/bypass_leader_approval`	Bypass the pull request leaders' approval	⭐
`/approve`	Instruct Bert-E that the author has approved the pull request.		✍️
`/create_pull_requests`	Allow the creation of integration pull requests.
`/create_integration_branches`	Allow the creation of integration branches.
`/no_octopus`	Prevent Wall-E from doing any octopus merge and use multiple consecutive merge instead
`/unanimity`	Change review acceptance criteria from `one reviewer at least` to `all reviewers`
`/wait`	Instruct Bert-E not to run until further notice.

Available commands

name	description	privileged
`/help`	Print Bert-E's manual in the pull request.
`/status`	Print Bert-E's current status in the pull request `TBA`
`/clear`	Remove all comments from Bert-E from the history `TBA`
`/retry`	Re-start a fresh build `TBA`
`/build`	Re-start a fresh build `TBA`
`/force_reset`	Delete integration branches & pull requests, and restart merge process from the beginning.
`/reset`	Try to remove integration branches unless there are commits on them which do not appear on the source branch.

Status report is not available.

bert-e · 2025-10-06T17:25:15Z

Incorrect fix version

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

None

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

9.1.5

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

codecov · 2025-10-06T17:29:56Z

Codecov Report

❌ Patch coverage is 98.63014% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 83.47%. Comparing base (a432e79) to head (b08e0a2).

Files with missing lines	Patch %	Lines
lib/api/bucketPutLogging.js	97.67%	1 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
lib/api/api.js	`89.79% <100.00%> (+0.10%)`	⬆️
lib/api/bucketGetLogging.js	`100.00% <100.00%> (ø)`
lib/api/bucketPutLogging.js	`97.67% <97.67%> (ø)`

... and 2 files with indirect coverage changes

@@                 Coverage Diff                 @@
##           development/9.0    #5966      +/-   ##
===================================================
+ Coverage            83.40%   83.47%   +0.06%     
===================================================
  Files                  189      191       +2     
  Lines                12198    12271      +73     
===================================================
+ Hits                 10174    10243      +69     
- Misses                2024     2028       +4

Flag	Coverage Δ
unit	`67.79% <98.63%> (+0.23%)`	⬆️
utapi-v2-tests	`33.28% <21.91%> (-0.11%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

bert-e · 2025-10-06T18:51:48Z

Incorrect fix version

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

None

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

9.0.31
9.1.5

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

jonathan-gramain · 2025-10-06T23:51:30Z

lib/api/bucketPutLogging.js

+                    return callback(err);
+                }
+
+                return callback(null);


Suggested change

return callback(null);

return callback();

jonathan-gramain · 2025-10-06T23:53:15Z

tests/unit/api/bucketGetLogging.js

+        const request = createGetLoggingRequest(bucketName);
+
+        bucketGetLogging(authInfo, request, log, (err, xml) => {
+            assert.strictEqual(err, null);


Suggested change

assert.strictEqual(err, null);

assert.ifError(err);

replaced the strict equal with ifError in both test files

jonathan-gramain · 2025-10-06T23:54:59Z

tests/unit/api/bucketGetLogging.js

+        const putRequest = createLoggingRequest(bucketName, loggingXML);
+
+        // First enable logging
+        bucketPutLogging(authInfo, putRequest, log, err => {


Optional: you may want to use async.series or async.waterfall to reduce nesting levels

changed to waterfall to handle logging and metrics in only one place

jonathan-gramain · 2025-10-06T23:57:05Z

tests/unit/api/bucketGetLogging.js

+        });
+    });
+
+    it('should handle multiple get requests consistently', done => {


I don't know if this test is really useful, but no problem to keep it if you prefer.

jonathan-gramain · 2025-10-06T23:58:58Z

tests/unit/api/bucketPutLogging.js

+        bucketPutLogging(authInfo, request, log, err => {
+            assert(err);
+            // Missing body should trigger XML parsing error
+            assert(err.is.MalformedXML || err.code === 'MalformedXML');


Is there a reason to support both error types?

no, removed the string check

dvasilas

Looks good.

We should also add monitoring to these APIs:

cloudserver/lib/api/bucketPutACL.js

Line 303 in e2ace24

monitoring.promMetrics('PUT', bucketName, '200', 'bucketPutACL');

leif-scality · 2025-10-07T14:33:51Z

Looks good.

We should also add monitoring to these APIs:

cloudserver/lib/api/bucketPutACL.js

Line 303 in e2ace24

monitoring.promMetrics('PUT', bucketName, '200', 'bucketPutACL');

added metrics

dvasilas · 2025-10-08T09:11:03Z

I think you need to remove logging from unsupportedQueries in constants because currently the API returns NotImplemented:

cloudserver/constants.js

Line 120 in e2ace24

'logging',

dvasilas · 2025-10-08T09:30:43Z

lib/api/bucketPutLogging.js

+        request,
+    };
+
+    return waterfall([


Should we check if the target bucket exists? Do you know if AWS returns an error if it does not exist?

An error occurred (InvalidTargetBucketForLogging) when calling the PutBucketLogging operation: The target bucket for logging does not exist

We can do the same

added a check and functional tests

BourgoisMickael · 2025-10-13T17:08:30Z

lib/api/api.js

+    bucketPutLogging,
+    bucketGetLogging,


follow alphabetical order here

I don't see them moved.
I see the alphabetical order is not strictly enforced, but at lost move thise next to the other bucket actions

BourgoisMickael · 2025-10-13T17:08:45Z

lib/api/api.js

+const bucketPutLogging = require('./bucketPutLogging');
+const bucketGetLogging = require('./bucketGetLogging');



same, follow alphabetical order

lib/api/bucketPutLogging.js

tests/functional/aws-node-sdk/test/bucket/putBucketLogging.js

BourgoisMickael · 2025-10-14T17:22:09Z

tests/functional/aws-node-sdk/test/bucket/putBucketLogging.js

+                    Bucket: bucketName,
+                    BucketLoggingStatus: validLoggingConfig,
+                }, err => {
+                    assert.strictEqual(err, null,


Use ifError if possible

Suggested change

assert.strictEqual(err, null,

assert.ifError(err)

BourgoisMickael · 2025-10-14T17:25:35Z

tests/functional/aws-node-sdk/test/bucket/putBucketLogging.js

+                _testPutBucketLoggingError(s3, validLoggingConfigWithGrants, 501, 'NotImplemented', done);
+            });
+
+            it('should disable logging with empty BucketLoggingStatus', done => {


It can be difficult to read with many nested callbacks.

I'd prefer if possible now to use async await instead of done callback in tests.

You can use await s3.putBucketLogging({...}).promise().

We started using that on new test files

tcarmet approved these changes Oct 6, 2025

View reviewed changes

leif-scality changed the base branch from development/9.1 to development/9.0 October 6, 2025 18:51

jonathan-gramain approved these changes Oct 6, 2025

View reviewed changes

dvasilas approved these changes Oct 7, 2025

View reviewed changes

leif-scality force-pushed the improvement/CLDSRV-754-bucket-put-get-logging branch from 856c74c to 3402063 Compare October 7, 2025 17:43

dvasilas reviewed Oct 8, 2025

View reviewed changes

leif-scality force-pushed the improvement/CLDSRV-754-bucket-put-get-logging branch 4 times, most recently from 5993699 to 951dd9c Compare October 13, 2025 08:20

leif-scality added 2 commits October 13, 2025 19:02

CLDSRV-754: add GetBucketLogging and PutBucketLogging endpoints

1b5fb3a

CLDSRV-754: add s3-node-sdk functional tests

c89ef85

leif-scality force-pushed the improvement/CLDSRV-754-bucket-put-get-logging branch from 951dd9c to 28ee88f Compare October 13, 2025 17:02

BourgoisMickael reviewed Oct 13, 2025

View reviewed changes

lib/api/bucketPutLogging.js Outdated Show resolved Hide resolved

BourgoisMickael reviewed Oct 13, 2025

View reviewed changes

tests/functional/aws-node-sdk/test/bucket/putBucketLogging.js Outdated Show resolved Hide resolved

BourgoisMickael reviewed Oct 13, 2025

View reviewed changes

tests/functional/aws-node-sdk/test/bucket/putBucketLogging.js Outdated Show resolved Hide resolved

leif-scality added 3 commits October 14, 2025 18:00

CLDSRV-754: bump arsenal version to 8.2.33

cd8d153

fixup! CLDSRV-754: add GetBucketLogging and PutBucketLogging endpoints

1f1fde4

fixup! CLDSRV-754: add s3-node-sdk functional tests

b08e0a2

leif-scality force-pushed the improvement/CLDSRV-754-bucket-put-get-logging branch from 029a87c to b08e0a2 Compare October 14, 2025 16:39

BourgoisMickael reviewed Oct 14, 2025

View reviewed changes

		const bucketPutLogging = require('./bucketPutLogging');
		const bucketGetLogging = require('./bucketGetLogging');

CLDSRV754: add GetBucketLogging and PutBucketLogging endpoints #5966

Are you sure you want to change the base?

CLDSRV754: add GetBucketLogging and PutBucketLogging endpoints #5966

Uh oh!

Conversation

leif-scality commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bert-e commented Oct 6, 2025

Hello leif-scality,

Uh oh!

bert-e commented Oct 6, 2025

Incorrect fix version

Uh oh!

codecov bot commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bert-e commented Oct 6, 2025

Incorrect fix version

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!

dvasilas left a comment

Choose a reason for hiding this comment

Uh oh!

leif-scality commented Oct 7, 2025

Uh oh!

dvasilas commented Oct 8, 2025

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!

Uh oh!

Uh oh!

Uh oh!

BourgoisMickael Oct 14, 2025 • 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!

BourgoisMickael Oct 14, 2025 • 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!

Reviewers

Assignees

Labels

leif-scality commented Oct 6, 2025 •

edited

Loading

codecov bot commented Oct 6, 2025 •

edited

Loading

BourgoisMickael Oct 14, 2025 •

edited

Loading

BourgoisMickael Oct 14, 2025 •

edited

Loading