bucket notifications - validate notifications conf on change (gh issue 8649) (#8667)

Signed-off-by: Amit Prinz Setter <[email protected]>

Author: alphaprinz

src/cmd/manage_nsfs.js

@@ -203,7 +203,7 @@ async function get_bucket_status(data) {
  * @param {Object} data
  * @returns { Promise<{ code: typeof ManageCLIResponse.BucketUpdated, detail: Object }>} 
  */
-async function update_bucket(data, user_input) {
+async function update_bucket(data) {
     const cur_name = data.name;
     const new_name = data.new_name;
     const name_update = is_name_update(data);
@@ -212,13 +212,6 @@ async function update_bucket(data, user_input) {
 
     let parsed_bucket_data;
 
-    if (user_input.notifications) {
-        //notifications are tested before they can be updated
-        const test_notif_err = await notifications_util.test_notifications(data, config_fs.connect_dir_path);
-        if (test_notif_err) {
-            throw_cli_error(ManageCLIError.InvalidArgument, "Failed to update notifications", test_notif_err);
-        }
-    }
     if (name_update) {
         parsed_bucket_data = await config_fs.create_bucket_config_file({ ...data, name: new_name });
         await config_fs.delete_bucket_config_file(cur_name);
@@ -279,14 +272,15 @@ async function delete_bucket(data, force) {
 async function bucket_management(action, user_input) {
     const data = action === ACTIONS.LIST ? undefined : await fetch_bucket_data(action, user_input);
     await manage_nsfs_validations.validate_bucket_args(config_fs, data, action);
+    await manage_nsfs_validations.validate_bucket_notifications(config_fs, user_input.notifications);
 
     let response = {};
     if (action === ACTIONS.ADD) {
         response = await add_bucket(data);
     } else if (action === ACTIONS.STATUS) {
         response = await get_bucket_status(data);
     } else if (action === ACTIONS.UPDATE) {
-        response = await update_bucket(data, user_input);
+        response = await update_bucket(data);
     } else if (action === ACTIONS.DELETE) {
         const force = get_boolean_or_string_value(user_input.force);
         response = await delete_bucket(data, force);

src/endpoint/s3/ops/s3_put_bucket_notification.js

@@ -1,6 +1,9 @@
 /* Copyright (C) 2016 NooBaa */
 'use strict';
 
+const S3Error = require('../s3_errors').S3Error;
+const notif_util = require('../../../util/notifications_util');
+
 /**
  * http://docs.aws.amazon.com/AmazonS3/latest/API/RESTBucketPUTnotification.html
  */
@@ -22,6 +25,19 @@ async function put_bucket_notification(req) {
         topic_configuration = [];
     }
 
+    //test new notifications.
+    //if test notification fails, fail the put op
+    const err = await notif_util.test_notifications(topic_configuration,
+        req.object_sdk.nsfs_config_root);
+    if (err) {
+        throw new S3Error({
+            code: 'InvalidArgument',
+            message: JSON.stringify(err.message),
+            http_code: 400,
+            detail: err.toString()
+        });
+    }
+
     const reply = await req.object_sdk.put_bucket_notification({
         bucket_name: req.params.bucket,
         notifications: topic_configuration

src/manage_nsfs/manage_nsfs_validations.js

@@ -15,6 +15,7 @@ const { TYPES, ACTIONS, VALID_OPTIONS, OPTION_TYPE, FROM_FILE, BOOLEAN_STRING_VA
     GLACIER_ACTIONS, LIST_UNSETABLE_OPTIONS, ANONYMOUS, DIAGNOSE_ACTIONS, UPGRADE_ACTIONS } = require('../manage_nsfs/manage_nsfs_constants');
 const { check_root_account_owns_user } = require('../nc/nc_utils');
 const { validate_username } = require('../util/validation_utils');
+const notifications_util = require('../util/notifications_util');
 
 /////////////////////////////
 //// GENERAL VALIDATIONS ////
@@ -447,6 +448,24 @@ async function validate_bucket_args(config_fs, data, action) {
     }
 }
 
+/**
+ * When setting notifications, we are supposed to send a test notification.
+ * If this test fails, we fail the user's request.
+ * @param {object} config_fs
+ * @param {object} notifications bucket's notification conf
+ * @returns {Promise} Error encountered during notification test, if any
+ */
+async function validate_bucket_notifications(config_fs, notifications) {
+    if (!notifications) {
+        return;
+    }
+    //test_notification returns an error if notification fails for the given config
+    const test_notif_err = await notifications_util.test_notifications(notifications, config_fs.config_root);
+    if (test_notif_err) {
+        throw_cli_error(ManageCLIError.InvalidArgument, "Failed to update notifications", test_notif_err);
+    }
+}
+
 /////////////////////////////
 //// ACCOUNT VALIDATIONS ////
 /////////////////////////////
@@ -682,6 +701,7 @@ function validate_connection_args(user_input, action) {
 // EXPORTS
 exports.validate_input_types = validate_input_types;
 exports.validate_bucket_args = validate_bucket_args;
+exports.validate_bucket_notifications = validate_bucket_notifications;
 exports.validate_account_args = validate_account_args;
 exports._validate_access_keys = _validate_access_keys;
 exports.validate_root_accounts_manager_update = validate_root_accounts_manager_update;

src/util/notifications_util.js

@@ -15,6 +15,7 @@ const nb_native = require('../util/nb_native');
 const http_utils = require('../util/http_utils');
 const nc_mkm = require('../manage_nsfs/nc_master_key_manager').get_instance();
 const NoobaaEvent = require('../manage_nsfs/manage_nsfs_events_utils').NoobaaEvent;
+const {ConfigFS} = require('../sdk/config_fs');
 
 const OP_TO_EVENT = Object.freeze({
     put_object: { name: 'ObjectCreated' },
@@ -338,23 +339,51 @@ function get_connection(connect) {
     }
 }
 
+/**
+ * Try to send a test notification for each of the notification configuration.
+ * If there's a failure, return it.
+ * @param {*} notifs notificatoin config to test
+ * @param {*} nc_config_dir path to NC conf dir (if system is NC)
+ * @returns {Promise} Error while testing, if any.
+ */
 
-async function test_notifications(bucket, connect_files_dir) {
-    if (!bucket.notifications) {
+async function test_notifications(notifs, nc_config_dir) {
+    //notifs can be empty in case we're removing the notification from the bucket
+    if (!notifs) {
         return;
     }
+    let connect_files_dir = DEFAULT_CONNECT_FILES_DIR;
+    if (nc_config_dir) {
+        connect_files_dir = new ConfigFS(nc_config_dir).connections_dir_path;
+    }
     const notificator = new Notificator({connect_files_dir});
-    for (const notif of bucket.notifications) {
-        const connect = await notificator.parse_connect_file(notif.connect, true);
-        dbg.log1("testing notif", notif);
+    for (const notif of notifs) {
+        let connect;
+        let connection;
+        let failure = false;
+        let notif_failure;
         try {
-            const connection = get_connection(connect);
+            connect = await notificator.parse_connect_file(notif.topic[0]);
+            connection = get_connection(connect);
             await connection.connect();
-            await connection.promise_notify({notif: "test notification"}, async err => err);
-            connection.destroy();
+            await connection.promise_notify({notif: "test notification"}, async (notif_cb, err_cb) => {
+                failure = true;
+                notif_failure = err_cb;
+            });
+            if (failure) {
+                if (notif_failure) {
+                    throw notif_failure;
+                }
+                //no error was thrown during notify, throw generic error
+                throw new Error();
+            }
         } catch (err) {
-            dbg.error("Connection failed for", connect);
+            dbg.error("Connection failed for", notif, ", connect =", connect, ", err = ", err);
             return err;
+        } finally {
+            if (connection) {
+                connection.destroy();
+            }
         }
     }
 }