Merge pull request #8919 from alphaprinz/notif_backports

liranmauda · web-flow · commit 2a2c184e905f · 2025-03-30T11:31:19.000+03:00
[Backport into 5.18] Notif backports
diff --git a/src/api/object_api.js b/src/api/object_api.js
@@ -1166,9 +1166,23 @@ module.exports = {
                     deleted_objects: {
                         type: 'array',
                         items: {
-                            $ref: '#/definitions/object_info'
+                            oneOf: [
+                                {
+                                    $ref: '#/definitions/object_info',
+                                },
+                                {
+                                    type: 'object',
+                                    properties: {
+                                        err_code: {
+                                            type: 'string',
+                                            enum: ['AccessDenied', 'InternalError']
+                                        },
+                                        err_message: { type: 'string' }
+                                    }
+                                }
+                            ]
                         }
-                    }
+                    },
                 }
             },
             auth: { system: 'admin' }
diff --git a/src/endpoint/s3/ops/s3_post_object_uploadId.js b/src/endpoint/s3/ops/s3_post_object_uploadId.js
@@ -43,6 +43,8 @@ async function post_object_uploadId(req, res) {
         res.setHeader('x-amz-version-id', reply.version_id);
     }
 
+    res.size_for_notif = reply.size;
+
     return {
         CompleteMultipartUploadResult: {
             Bucket: req.params.bucket,
diff --git a/src/endpoint/s3/s3_bucket_logging.js b/src/endpoint/s3/s3_bucket_logging.js
@@ -8,7 +8,7 @@ const { Buffer } = require('node:buffer');
 const config = require('../../../config');
 const {compose_notification_req, check_notif_relevant, check_free_space_if_needed} = require('../../util/notifications_util');
 
-async function send_bucket_op_logs(req, res) {
+async function send_bucket_op_logs(req, res, reply) {
     if (req.params && req.params.bucket &&
         !(req.op_name === 'put_bucket' ||
           req.op_name === 'delete_bucket' ||
@@ -36,8 +36,10 @@ async function send_bucket_op_logs(req, res) {
 
         if (req.notification_logger && bucket_info.notifications) {
             for (const notif_conf of bucket_info.notifications) {
-                if (check_notif_relevant(notif_conf, req)) {
-                    const notif = compose_notification_req(req, res, bucket_info, notif_conf);
+                //write the notification log only if request is successful (ie res.statusCode < 300)
+                //and event is actually relevant to the notification conf
+                if (res.statusCode < 300 && check_notif_relevant(notif_conf, req)) {
+                    const notif = compose_notification_req(req, res, bucket_info, notif_conf, reply);
                     dbg.log1("logging notif ", notif_conf, ", notif = ", notif);
                     writes_aggregate.push({
                         file: req.notification_logger,
diff --git a/src/endpoint/s3/s3_rest.js b/src/endpoint/s3/s3_rest.js
@@ -175,7 +175,7 @@ async function handle_request(req, res) {
     http_utils.send_reply(req, res, reply, options);
     collect_bucket_usage(op, req, res);
     try {
-        await s3_logging.send_bucket_op_logs(req, res); // logging again with result
+        await s3_logging.send_bucket_op_logs(req, res, reply); // logging again with result
     } catch (err) {
         dbg.error(`Could not log bucket operation (after operation ${req.op_name}):`, err);
     }
diff --git a/src/server/bg_services/lifecycle.js b/src/server/bg_services/lifecycle.js
@@ -68,13 +68,13 @@ async function handle_bucket_rule(system, rule, j, bucket) {
         })
     });
 
-    //dbg.log0("LIFECYCLE PROCESSING res =", res);
-
     if (res.deleted_objects) {
 
         const writes = [];
 
         for (const deleted_obj of res.deleted_objects) {
+            //if deletion has failed, don't send a notification
+            if (deleted_obj.err_code) continue;
             for (const notif of bucket.notifications) {
                 if (check_notif_relevant(notif, {
                     op_name: 'lifecycle_delete',
diff --git a/src/server/object_services/object_server.js b/src/server/object_services/object_server.js
@@ -964,7 +964,7 @@ async function delete_multiple_objects_by_filter(req) {
 
     const { objects } = await MDStore.instance().find_objects(query);
 
-    await delete_multiple_objects(_.assign(req, {
+    const delete_results = await delete_multiple_objects(_.assign(req, {
         rpc_params: {
             bucket: req.bucket.name,
             objects: _.map(objects, obj => ({
@@ -978,8 +978,19 @@ async function delete_multiple_objects_by_filter(req) {
     if (reply_objects) {
         //reply needs to include deleted objects
         //(this is used for LifecycleExpiratoin event notifications)
-        //so map the md into (api friendly) object info
-        reply.deleted_objects = _.map(objects, get_object_info);
+        //so map the md into (api friendly) object info,
+        //or incude the error if deletion failed
+        reply.deleted_objects = [];
+        for (let i = 0; i < objects.length; ++i) {
+             if (delete_results[i].err_code) {
+                reply.deleted_objects[i] = {
+                    err_code: delete_results[i].err_code,
+                    err_message: delete_results[i].err_message
+                };
+             } else {
+                reply.deleted_objects[i] = get_object_info(objects[i]);
+             }
+        }
     }
 
     return reply;
diff --git a/src/test/unit_tests/test_notifications.js b/src/test/unit_tests/test_notifications.js
@@ -52,6 +52,8 @@ let http_server = null;
 let server_done = false;
 let expected_bucket;
 let expected_event_name;
+let expected_key;
+let expected_eTag;
 
 // eslint-disable-next-line max-lines-per-function
 mocha.describe('notifications', function() {
@@ -91,9 +93,10 @@ mocha.describe('notifications', function() {
                 if (notif !== "test notification") {
                     assert.strictEqual(notif.Records[0].s3.bucket.name, expected_bucket, 'wrong bucket name in notification');
                     assert.strictEqual(notif.Records[0].eventName, expected_event_name, 'wrong event name in notification');
-
+                    assert.strictEqual(notif.Records[0].s3.object.key, expected_key, 'wrong key in notification');
+                    const expected_eTag_trimmed = expected_eTag && expected_eTag.substring(1, expected_eTag.length - 1);
+                    assert.strictEqual(notif.Records[0].s3.object.eTag, expected_eTag_trimmed, 'wrong eTag in notification');
                 }
-
                 res.writeHead(200, {'Content-Type': 'text/plain'});
                 res.end();
                 server_done = true;
@@ -120,13 +123,18 @@ mocha.describe('notifications', function() {
         });
 
         mocha.it('simple notif put', async () => {
-            await s3.putObject({
+            const res = await s3.putObject({
                 Bucket: bucket,
                 Key: 'f1',
                 Body: 'this is the body',
             });
 
-            await notify_await_result({bucket_name: bucket, event_name: 'ObjectCreated:Put'});
+            await notify_await_result({
+                bucket_name: bucket,
+                event_name: 'ObjectCreated:Put',
+                key: "f1",
+                etag: res.ETag
+            });
         });
 
 
@@ -136,7 +144,12 @@ mocha.describe('notifications', function() {
                 Key: 'f1',
             });
 
-            await notify_await_result({bucket_name: bucket, event_name: 'ObjectRemoved:Delete'});
+            await notify_await_result({
+                bucket_name: bucket,
+                event_name: 'ObjectRemoved:Delete',
+                key: "f1",
+                etag: undefined
+            });
         });
 
 
@@ -155,13 +168,18 @@ mocha.describe('notifications', function() {
 
             assert.strictEqual(set.$metadata.httpStatusCode, 200);
 
-            await s3.putObject({
+            const res = await s3.putObject({
                 Bucket: bucket,
                 Key: 'f2',
                 Body: 'this is the body',
             });
 
-            await notify_await_result({bucket_name: bucket, event_name: 'ObjectCreated:Put'});
+            await notify_await_result({
+                bucket_name: bucket,
+                event_name: 'ObjectCreated:Put',
+                key: "f2",
+                etag: res.ETag
+            });
 
             await s3.deleteObject({
                 Bucket: bucket,
@@ -171,16 +189,53 @@ mocha.describe('notifications', function() {
             //there shouldn't be a notification for the delete, wait 2 seconds to validate this
             await notify_await_result({timeout: 2000});
         });
+
+        mocha.it('multipart', async () => {
+            const res_create = await s3.createMultipartUpload({
+                Bucket: bucket,
+                Key: 'mp1'
+            });
+
+            const res_upload = await s3.uploadPart({
+                Bucket: bucket,
+                Key: 'mp1',
+                UploadId: res_create.UploadId,
+                PartNumber: 1,
+                Body: 'this is the body'
+            });
+
+            const res_complete = await s3.completeMultipartUpload({
+                Bucket: bucket,
+                Key: 'mp1',
+                UploadId: res_create.UploadId,
+                MultipartUpload: {
+                    Parts: [{
+                        ETag: res_upload.ETag,
+                        PartNumber: 1
+                    }]
+                }
+            });
+
+            await notify_await_result({
+                bucket_name: bucket,
+                event_name: 'ObjectCreated:CompleteMultipartUpload',
+                key: "mp1",
+                etag: res_complete.ETag
+            });
+        });
+
     });
 
 });
 
 const step_wait = 100;
-async function notify_await_result({bucket_name, event_name, timeout}) {
+async function notify_await_result({bucket_name, event_name, etag, key, timeout = undefined}) {
 
     //remember expected result here so server could compare it to actual result later
     expected_bucket = bucket_name;
     expected_event_name = event_name;
+    expected_eTag = etag;
+    expected_key = key;
     server_done = false;
 
     //busy-sync wait for server
diff --git a/src/util/notifications_util.js b/src/util/notifications_util.js
@@ -418,8 +418,9 @@ function compose_notification_base(notif_conf, bucket, req) {
 }
 
 //see https://docs.aws.amazon.com/AmazonS3/latest/userguide/notification-content-structure.html
-function compose_notification_req(req, res, bucket, notif_conf) {
-    let eTag = res.getHeader('ETag');
+function compose_notification_req(req, res, bucket, notif_conf, reply) {
+    //most s3 ops put etag in header. CompleteMultipartUploadResult is an exception.
+    let eTag = res.getHeader('ETag') || reply?.CompleteMultipartUploadResult?.ETag;
     //eslint-disable-next-line
     if (eTag && eTag.startsWith('\"') && eTag.endsWith('\"')) {
         eTag = eTag.substring(1, eTag.length - 1);

Original file line number	Diff line number	Diff line change
`@@ -43,6 +43,8 @@ async function post_object_uploadId(req, res) {`
`43`	`43`	`res.setHeader('x-amz-version-id', reply.version_id);`
`44`	`44`	`}`
`45`	`45`
	`46`	`+ res.size_for_notif = reply.size;`
	`47`	`+`
`46`	`48`	`return {`
`47`	`49`	`CompleteMultipartUploadResult: {`
`48`	`50`	`Bucket: req.params.bucket,`
Original file line number	Diff line number	Diff line change
`@@ -175,7 +175,7 @@ async function handle_request(req, res) {`
`175`	`175`	`http_utils.send_reply(req, res, reply, options);`
`176`	`176`	`collect_bucket_usage(op, req, res);`
`177`	`177`	`try {`
`178`		`- await s3_logging.send_bucket_op_logs(req, res); // logging again with result`
	`178`	`+ await s3_logging.send_bucket_op_logs(req, res, reply); // logging again with result`
`179`	`179`	`} catch (err) {`
`180`	`180`	dbg.error(`Could not log bucket operation (after operation ${req.op_name}):`, err);
`181`	`181`	`}`