Merge pull request #8867 from shirady/nc-upgrade-cli

NC | Online Upgrade CLI | Validate `expected_version` on `upgrade start`

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;
+