NC | Online Upgrade CLI | Validate expected_version on upgrade start

1. Add the function of validate_expected_version that would check that we have the flag of expected_version and it is from the supported versions we allow. Also, add specific errors to the CLI (MissingExpectedVersionFlag and InvalidArgumentType) so that eventually we would not report it as UpgradeFailed.
2. Move functions that are related to versions to versions_utils, and added a helper function is_valid_sematic_version to check the structure of the string.
3. Throw an error instead of the message "config_dir_version on system.json and config_fs.config_dir_version match, nothing to upgrade" and change the tests accordingly.
4. Add the skip_verification to BOOLEAN_STRING_OPTIONS and use it to in validate_expected_version.

Signed-off-by: shirady <[email protected]>

Author: shirady

src/manage_nsfs/manage_nsfs_cli_errors.js

@@ -472,6 +472,12 @@ ManageCLIError.InvalidUpgradeAction = Object.freeze({
     http_code: 400,
 });
 
+ManageCLIError.MissingExpectedVersionFlag = Object.freeze({
+    code: 'MissingExpectedVersionFlag',
+    message: 'Expected version is mandatory, please use the --expected_version flag',
+    http_code: 400,
+});
+
 ManageCLIError.UpgradeFailed = Object.freeze({
     code: 'UpgradeFailed',
     message: 'Upgrade request failed',

src/manage_nsfs/manage_nsfs_constants.js

@@ -174,7 +174,8 @@ const OPTION_TYPE = {
 
 const BOOLEAN_STRING_VALUES = ['true', 'false'];
 const BOOLEAN_STRING_OPTIONS = new Set(['allow_bucket_creation', 'regenerate', 'wide', 'show_secrets', 'force',
-    'force_md5_etag', 'iam_operate_on_root_account', 'all_account_details', 'all_bucket_details', 'anonymous', 'disable_service_validation', 'disable_runtime_validation', 'short_status']);
+    'force_md5_etag', 'iam_operate_on_root_account', 'all_account_details', 'all_bucket_details', 'anonymous',
+    'disable_service_validation', 'disable_runtime_validation', 'short_status', 'skip_verification']);
 
 // CLI UNSET VALUES
 const CLI_EMPTY_STRING = '';

src/manage_nsfs/manage_nsfs_validations.js

@@ -1,6 +1,7 @@
 /* Copyright (C) 2024 NooBaa */
 'use strict';
 
+const pkg = require('../../package.json');
 const config = require('../../config');
 const dbg = require('../util/debug_module')(__filename);
 const net = require('net');
@@ -16,6 +17,7 @@ const { TYPES, ACTIONS, VALID_OPTIONS, OPTION_TYPE, FROM_FILE, BOOLEAN_STRING_VA
 const { check_root_account_owns_user } = require('../nc/nc_utils');
 const { validate_username } = require('../util/validation_utils');
 const notifications_util = require('../util/notifications_util');
+const version_utils = require('../util/versions_utils');
 
 /////////////////////////////
 //// GENERAL VALIDATIONS ////
@@ -738,6 +740,43 @@ function validate_connection_args(user_input, action) {
     }
 }
 
+////////////////////////////
+//// UPGRADE VALIDATION ////
+///////////////////////////
+
+/**
+ * validate_expected_version checks that expected_version is valid
+ * if it's not valid it throws an error
+ * Note: the cases where we used ManageCLIError errors will 
+ *        not be reported as UpgradeFailed and without adding an event
+ * @param {string} expected_version
+ * @param {boolean} [skip_verification]
+ */
+function validate_expected_version(expected_version, skip_verification = false) {
+    if (!expected_version) {
+        throw_cli_error(ManageCLIError.MissingExpectedVersionFlag);
+    }
+    if (!version_utils.is_valid_semantic_version(expected_version)) {
+        const detail = 'expected_version must have sematic version structure (major.minor.patch)';
+        throw_cli_error(ManageCLIError.InvalidArgumentType, detail);
+    }
+    if (!skip_verification && !version_match_to_current_version(expected_version)) {
+        throw new Error(`expected_version must match the package version ${pkg.version}`);
+    }
+}
+
+/**
+ * version_match_to_current_version checks that the version
+ * is the same as the one that was defined in the pkg
+ * @param {string} expected_version
+ * @returns {boolean}
+ */
+function version_match_to_current_version(expected_version) {
+    const package_version = pkg.version;
+    const diff_from_package_version = version_utils.version_compare(expected_version, package_version);
+    return diff_from_package_version === 0;
+}
+
 
 // EXPORTS
 exports.validate_input_types = validate_input_types;
@@ -751,3 +790,4 @@ exports.validate_whitelist_ips = validate_whitelist_ips;
 exports.validate_flags_combination = validate_flags_combination;
 exports.validate_connection_args = validate_connection_args;
 exports.validate_no_extra_args = validate_no_extra_args;
+exports.validate_expected_version = validate_expected_version;

src/manage_nsfs/upgrade.js

@@ -7,8 +7,9 @@ const { ManageCLIError } = require('./manage_nsfs_cli_errors');
 const { UPGRADE_ACTIONS } = require('./manage_nsfs_constants');
 const { NCUpgradeManager } = require('../upgrade/nc_upgrade_manager');
 const { ManageCLIResponse } = require('../manage_nsfs/manage_nsfs_cli_responses');
-const { throw_cli_error, write_stdout_response } = require('./manage_nsfs_cli_utils');
+const { throw_cli_error, write_stdout_response, get_boolean_or_string_value } = require('./manage_nsfs_cli_utils');
 const { NoobaaEvent } = require('./manage_nsfs_events_utils');
+const { validate_expected_version } = require('./manage_nsfs_validations');
 
 /**
  * manage_upgrade_operations handles cli upgrade operations
@@ -41,23 +42,29 @@ async function manage_upgrade_operations(action, user_input, config_fs) {
  */
 async function start_config_dir_upgrade(user_input, config_fs) {
     try {
-        const skip_verification = user_input.skip_verification;
+        const skip_verification = get_boolean_or_string_value(user_input.skip_verification);
         const expected_version = user_input.expected_version;
         const expected_hosts = user_input.expected_hosts && user_input.expected_hosts.split(',').filter(host => !_.isEmpty(host));
         const custom_upgrade_scripts_dir = user_input.custom_upgrade_scripts_dir;
 
         new NoobaaEvent(NoobaaEvent.CONFIG_DIR_UPGRADE_STARTED).create_event(undefined, { expected_version, expected_hosts }, undefined);
 
-        if (!expected_version) throw new Error('expected_version flag is required');
+        validate_expected_version(expected_version, skip_verification);
         if (!expected_hosts) dbg.warn('expected_hosts flag is empty, config dir upgrade will be performed without hosts version verification');
 
         const nc_upgrade_manager = new NCUpgradeManager(config_fs, { custom_upgrade_scripts_dir });
         const upgrade_res = await nc_upgrade_manager.upgrade_config_dir(expected_version, expected_hosts, { skip_verification });
         if (!upgrade_res) throw new Error('Upgrade config directory failed', { cause: upgrade_res });
         write_stdout_response(ManageCLIResponse.UpgradeSuccessful, upgrade_res, { expected_version, expected_hosts });
     } catch (err) {
-        dbg.error('could not upgrade config directory successfully - error', err);
-        throw_cli_error({ ...ManageCLIError.UpgradeFailed, cause: err }, undefined, { error: err.stack || err });
+        if (err instanceof ManageCLIError) {
+            // those are errors that the admin made in the CLI, the upgrade didn't start
+            // therefore, we won't report it as UpgradeFailed and without adding an event
+            throw err;
+        } else {
+            dbg.error('could not upgrade config directory successfully - error', err);
+            throw_cli_error({ ...ManageCLIError.UpgradeFailed, cause: err }, undefined, { error: err.stack || err });
+        }
     }
 }
 

src/sdk/config_fs.js

@@ -16,7 +16,7 @@ const native_fs_utils = require('../util/native_fs_utils');
 const { RpcError } = require('../rpc');
 const nc_mkm = require('../manage_nsfs/nc_master_key_manager').get_instance();
 const nsfs_schema_utils = require('../manage_nsfs/nsfs_schema_utils');
-const { version_compare } = require('../upgrade/upgrade_utils');
+const { version_compare } = require('../util/versions_utils');
 const { anonymous_access_key } = require('./object_sdk');
 
 /** @typedef {import('fs').Dirent} Dirent */

src/test/unit_tests/jest_tests/test_cli_upgrade.test.js

@@ -266,8 +266,39 @@ describe('noobaa cli - upgrade', () => {
         await fs_utils.replace_file(config_fs.system_json_path, JSON.stringify(old_rpm_expected_system_json));
         const res = await exec_manage_cli(TYPES.UPGRADE, UPGRADE_ACTIONS.START, { config_root, expected_hosts }, true);
         const parsed_res = JSON.parse(res.stdout);
-        expect(parsed_res.error.message).toBe(ManageCLIError.UpgradeFailed.message);
-        expect(parsed_res.error.cause).toContain('expected_version flag is required');
+        expect(parsed_res.error.message).toBe(ManageCLIError.MissingExpectedVersionFlag.message);
+    });
+
+    it('upgrade start - should fail on expected_version with wrong type (number instead of string)', async () => {
+        await fs_utils.replace_file(config_fs.system_json_path, JSON.stringify(old_rpm_expected_system_json));
+        const res = await exec_manage_cli(TYPES.UPGRADE, UPGRADE_ACTIONS.START, { config_root, expected_version: 5, expected_hosts }, true);
+        const parsed_res = JSON.parse(res.stdout);
+        expect(parsed_res.error.code).toBe(ManageCLIError.InvalidArgumentType.code);
+        expect(parsed_res.error.detail).toContain('type of flag expected_version should be string');
+    });
+
+    it('upgrade start - should fail on expected_version is a string not in the format on sematic version', async () => {
+        await fs_utils.replace_file(config_fs.system_json_path, JSON.stringify(old_rpm_expected_system_json));
+        const res = await exec_manage_cli(TYPES.UPGRADE, UPGRADE_ACTIONS.START, { config_root, expected_version: 'invalid-semversion', expected_hosts }, true);
+        const parsed_res = JSON.parse(res.stdout);
+        expect(parsed_res.error.code).toBe(ManageCLIError.InvalidArgumentType.code);
+        expect(parsed_res.error.detail).toContain('expected_version must have sematic version structure');
+    });
+
+    it('upgrade start - should fail on expected_version that is later that package version', async () => {
+        await fs_utils.replace_file(config_fs.system_json_path, JSON.stringify(old_rpm_expected_system_json));
+        const res = await exec_manage_cli(TYPES.UPGRADE, UPGRADE_ACTIONS.START, { config_root, expected_version: '6.18.0', expected_hosts }, true);
+        const parsed_res = JSON.parse(res.stdout);
+        expect(parsed_res.error.code).toBe(ManageCLIError.UpgradeFailed.code);
+        expect(parsed_res.error.cause).toContain('expected_version must match the package version');
+    });
+
+    it('upgrade start - should fail on expected_version that is older that package version', async () => {
+        await fs_utils.replace_file(config_fs.system_json_path, JSON.stringify(old_rpm_expected_system_json));
+        const res = await exec_manage_cli(TYPES.UPGRADE, UPGRADE_ACTIONS.START, { config_root, expected_version: '5.16.0', expected_hosts }, true);
+        const parsed_res = JSON.parse(res.stdout);
+        expect(parsed_res.error.code).toBe(ManageCLIError.UpgradeFailed.code);
+        expect(parsed_res.error.cause).toContain('expected_version must match the package version');
     });
 
     it('upgrade start - should succeed although missing expected hosts', async () => {
@@ -306,7 +337,7 @@ describe('noobaa cli - upgrade', () => {
         const res = await exec_manage_cli(TYPES.UPGRADE, UPGRADE_ACTIONS.START, { config_root, expected_version, expected_hosts }, true);
         const parsed_res = JSON.parse(res.stdout);
         expect(parsed_res.error.message).toBe(ManageCLIError.UpgradeFailed.message);
-        expect(parsed_res.error.cause).toContain(`config dir upgrade can not be started - the host's package version=${pkg.version} does not match the user's expected version=${expected_version}`);
+        expect(parsed_res.error.cause).toContain(`expected_version must match the package version`);
     });
 
     it('upgrade start - should fail on old rpm version', async () => {
@@ -332,18 +363,17 @@ describe('noobaa cli - upgrade', () => {
         expect(system_data_after_upgrade.config_directory).toBeUndefined();
     });
 
-    it('upgrade start - should succeed - same version, nothing to upgrade - config directory property exists', async () => {
+    it('upgrade start - should fail - same version, nothing to upgrade - config directory property exists', async () => {
         await fs_utils.replace_file(config_fs.system_json_path, JSON.stringify(old_expected_system_json2));
         const system_data_before_upgrade = await config_fs.get_system_config_file();
         const options = { config_root, expected_version: pkg.version, expected_hosts };
         const res = await exec_manage_cli(TYPES.UPGRADE, UPGRADE_ACTIONS.START, options, true);
-        const parsed_res = JSON.parse(res);
-        expect(parsed_res.response.code).toBe(ManageCLIResponse.UpgradeSuccessful.code);
-        expect(parsed_res.response.reply.message).toBe('config_dir_version on system.json and config_fs.config_dir_version match, nothing to upgrade');
+        const parsed_res = JSON.parse(res.stdout);
+        expect(parsed_res.error.message).toBe(ManageCLIError.UpgradeFailed.message);
+        expect(parsed_res.error.cause).toContain(`config_dir_version on system.json and config_fs.config_dir_version match, nothing to upgrade`);
         const system_data_after_upgrade = await config_fs.get_system_config_file();
         // check that in the hostname section nothing changed
         expect(system_data_before_upgrade[hostname]).toStrictEqual(system_data_after_upgrade[hostname]);
-        expect(system_data_after_upgrade.config_directory).toStrictEqual(old_expected_system_json2.config_directory);
     });
 
     it('upgrade start - should succeed - no old config directory', async () => {

src/test/unit_tests/jest_tests/test_nc_upgrade_manager.test.js

@@ -785,9 +785,9 @@ describe('nc upgrade manager - upgrade config directory', () => {
             const expected_version = pkg.version;
             const hosts_list = [hostname];
             await config_fs.create_system_config_file(JSON.stringify(system_data));
-            const expected_msg = { "message": "config_dir_version on system.json and config_fs.config_dir_version match, nothing to upgrade" };
-            const res = await nc_upgrade_manager.upgrade_config_dir(expected_version, hosts_list);
-            expect(res).toStrictEqual(expected_msg);
+            const expected_err_msg = 'config_dir_version on system.json and config_fs.config_dir_version match, nothing to upgrade';
+            await expect(nc_upgrade_manager.upgrade_config_dir(expected_version, hosts_list))
+                .rejects.toThrow(expected_err_msg);
         });
 
         it('upgrade_config_dir - config_dir_version in system.json is newer than the hosts current config_dir_version', async () => {

src/upgrade/nc_upgrade_manager.js

@@ -7,7 +7,8 @@ const util = require('util');
 const pkg = require('../../package.json');
 const dbg = require('../util/debug_module')(__filename);
 const { CONFIG_DIR_PHASES } = require('../sdk/config_fs');
-const { should_upgrade, run_upgrade_scripts, version_compare } = require('./upgrade_utils');
+const { should_upgrade, run_upgrade_scripts } = require('./upgrade_utils');
+const { version_compare } = require('../util/versions_utils');
 
 const hostname = os.hostname();
 // prior to 5.18.0 - there is no config dir version, the config dir version to be used on the first upgrade is 0.0.0 (5.17.0 -> 5.18.0)
@@ -108,7 +109,11 @@ class NCUpgradeManager {
         const package_to_version = this.package_version;
         const this_upgrade_versions = { config_dir_from_version, config_dir_to_version, package_from_version, package_to_version };
 
-        if (!should_upgrade(config_dir_from_version, config_dir_to_version)) return { message: 'config_dir_version on system.json and config_fs.config_dir_version match, nothing to upgrade' };
+        if (!should_upgrade(config_dir_from_version, config_dir_to_version)) {
+            const err_message = 'config_dir_version on system.json and config_fs.config_dir_version match, nothing to upgrade';
+            dbg.error(`upgrade_config_dir: ${err_message}`);
+            throw new Error(err_message);
+        }
 
         if (!skip_verification) await this._verify_config_dir_upgrade(system_data, expected_version, hosts_list);
 

src/upgrade/upgrade_utils.js

@@ -5,34 +5,7 @@ const fs = require('fs');
 const _ = require('lodash');
 const path = require('path');
 const dbg = require('../util/debug_module')(__filename);
-
-/**
- * @param {string} ver
- */
-function parse_ver(ver) {
-    const stripped_ver = ver.split('-')[0];
-    return stripped_ver.split('.').map(i => Number.parseInt(i, 10));
-}
-
-
-/**
- * version_compare compares 2 versions. returns positive if ver1 is larger, negative if ver2, 0 if equal
- * @param {string} ver1
- * @param {string} ver2
- */
-function version_compare(ver1, ver2) {
-    const ver1_arr = parse_ver(ver1);
-    const ver2_arr = parse_ver(ver2);
-    const max_length = Math.max(ver1_arr.length, ver2_arr.length);
-    for (let i = 0; i < max_length; ++i) {
-        const comp1 = ver1_arr[i] || 0;
-        const comp2 = ver2_arr[i] || 0;
-        const diff = comp1 - comp2;
-        // if version component is not the same, return the difference
-        if (diff) return diff;
-    }
-    return 0;
-}
+const { version_compare } = require('../util/versions_utils');
 
 /**
  * @param {string} current_version
@@ -137,5 +110,4 @@ async function run_upgrade_scripts(this_upgrade, upgrade_scripts_dir, options) {
 
 exports.should_upgrade = should_upgrade;
 exports.load_required_scripts = load_required_scripts;
-exports.version_compare = version_compare;
 exports.run_upgrade_scripts = run_upgrade_scripts;

src/util/versions_utils.js

@@ -0,0 +1,47 @@
+/* Copyright (C) 2025 NooBaa */
+"use strict";
+
+/**
+ * @param {string} ver
+ */
+function parse_ver(ver) {
+    const stripped_ver = ver.split('-')[0];
+    return stripped_ver.split('.').map(i => Number.parseInt(i, 10));
+}
+
+/**
+ * version_compare compares 2 versions. returns positive if ver1 is larger, negative if ver2, 0 if equal
+ * @param {string} ver1
+ * @param {string} ver2
+ */
+function version_compare(ver1, ver2) {
+    const ver1_arr = parse_ver(ver1);
+    const ver2_arr = parse_ver(ver2);
+    const max_length = Math.max(ver1_arr.length, ver2_arr.length);
+    for (let i = 0; i < max_length; ++i) {
+        const comp1 = ver1_arr[i] || 0;
+        const comp2 = ver2_arr[i] || 0;
+        const diff = comp1 - comp2;
+        // if version component is not the same, return the difference
+        if (diff) return diff;
+    }
+    return 0;
+}
+
+/**
+ * is_valid_semantic_version checks that the version string is a valid sematic version
+ *   1. strips the additional "-<something>" that might be added, for example: -alpha, -beta etc
+ *   2. checks that the version has the sematic version is from the structure of version: major.minor.patch
+ * @param {string} version
+ * @returns {boolean}
+ */
+function is_valid_semantic_version(version) {
+    const stripped_ver = version.split('-')[0];
+    // sematic version is from the structure of version: major.minor.patch
+    const semantic_version_regex = /^\d+\.\d+.\.\d+$/;
+    return semantic_version_regex.test(stripped_ver);
+}
+
+exports.version_compare = version_compare;
+exports.is_valid_semantic_version = is_valid_semantic_version;
+