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 (990a9b1) to head (2448fa5).
⚠️ Report is 3 commits behind head on development/9.0.
✅ All tests successful. No failed tests found.

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 1 file with indirect coverage changes

@@                 Coverage Diff                 @@
##           development/9.0    #5966      +/-   ##
===================================================
+ Coverage            83.37%   83.47%   +0.09%     
===================================================
  Files                  189      191       +2     
  Lines                12198    12271      +73     
===================================================
+ Hits                 10170    10243      +73     
  Misses                2028     2028

Flag	Coverage Δ
ceph-backend-test	`66.13% <95.89%> (+0.17%)`	⬆️
file-ft-tests	`66.98% <95.89%> (+0.17%)`	⬆️
kmip-ft-tests	`26.95% <21.91%> (-0.04%)`	⬇️
mongo-v0-ft-tests	`68.28% <95.89%> (+0.14%)`	⬆️
mongo-v1-ft-tests	`68.29% <95.89%> (+0.16%)`	⬆️
multiple-backend	`35.60% <21.91%> (-0.09%)`	⬇️
quota-tests	`32.16% <21.91%> (-0.92%)`	⬇️
quota-tests-inflights	`34.11% <21.91%> (-0.08%)`	⬇️
unit	`67.70% <98.63%> (+0.23%)`	⬆️
utapi-v2-tests	`33.28% <21.91%> (-0.07%)`	⬇️

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

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 added 2 commits October 8, 2025 18:25

CLDSRV-754: add GetBucketLogging and PutBucketLogging endpoints

4f9c2ed

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

7fd5895

leif-scality force-pushed the improvement/CLDSRV-754-bucket-put-get-logging branch from 3402063 to d7f1f64 Compare October 8, 2025 16:25

tmp add arsenal brach to allow CI

10f4dcd

leif-scality force-pushed the improvement/CLDSRV-754-bucket-put-get-logging branch from 83f49e7 to 10f4dcd Compare October 8, 2025 16:35

leif-scality added 4 commits October 8, 2025 18:38

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

a0146d6

fixup! CLDSRV-754: add GetBucketLogging and PutBucketLogging endpoints

0a189be

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

f6cf49c

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

2448fa5

CLDSRV754: add GetBucketLogging and PutBucketLogging endpoints #5966

Are you sure you want to change the base?

CLDSRV754: add GetBucketLogging and PutBucketLogging endpoints #5966

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!

Uh oh!

leif-scality commented Oct 6, 2025 •

edited

Loading

codecov bot commented Oct 6, 2025 •

edited

Loading