Merge pull request #8774 from romayalon/romy-cli-fix-unset-issue

NC | CLI | Fix for unsettable flags issues

Author: romayalon

docs/NooBaaNonContainerized/CI&Tests.md

@@ -87,7 +87,7 @@ Run `NC mocha tests` with root permissions -
 
 #### NC mocha tests 
 The following is a list of `NC mocha test` files -   
-1. `test_nc_nsfs_cli.js` - Tests NooBaa CLI.  
+1. `test_nc_cli.js` - Tests NooBaa CLI.  
 2. `test_nc_health` - Tests NooBaa Health CLI.  
 3. `test_nsfs_glacier_backend.js` - Tests NooBaa Glacier Backend.  
 4. `test_nc_with_a_couple_of_forks.js` - Tests the `bucket_namespace_cache` when running with a couple of forks. Please notice that it uses `nc_coretest` with setup that includes a couple of forks.
@@ -99,7 +99,7 @@ The following is a list of `NC jest tests` files -
 2. `test_nc_master_keys.test.js` - Tests NC master key manager (store type = file).  
 3. `test_nc_master_keys_exec.test.js` - Tests NC master key manager (store type = executable).  
 4. `test_nc_nsfs_bucket_cli.test.js` - Tests NooBaa CLI bucket commands.  
-5. `test_nc_nsfs_account_cli.test.js` - Tests NooBaa CLI account commands.  
+5. `test_nc_account_cli.test.js` - Tests NooBaa CLI account commands.  
 6. `test_nc_nsfs_anonymous_cli.test.js` - Tests NooBaa CLI anonymous account commands.  
 7. `test_nc_nsfs_config_schema_validation.test.js` - Tests NC config.json schema validation.  
 8. `test_nc_nsfs_bucket_schema_validation.test.js` - Tests NC bucket schema validation.  

docs/NooBaaNonContainerized/NooBaaCLI.md

@@ -168,11 +168,11 @@ noobaa-cli account update --name <account_name> [--new_name][--uid][--gid][--use
 
 - `supplemental_groups`
     - Type: String
-    - Description: Specifies additional FS groups (GID) a user can be a part of. Allows access to directories/files having one or more of the provided groups. A String of GIDs separated by commas. unset with ''
+    - Description: Specifies additional FS groups (GID) a user can be a part of. Allows access to directories/files having one or more of the provided groups. A String of GIDs separated by commas. Unset with ''.
 
 - `new_buckets_path` 
     - Type: String
-    - Description: Specifies a file system directory to be used for creating underlying directories that represent buckets created by an account using the S3 API.
+    - Description: Specifies a file system directory to be used for creating underlying directories that represent buckets created by an account using the S3 API. Unset with ''.
 
 - `regenerate`
     - Type: Boolean
@@ -189,15 +189,15 @@ noobaa-cli account update --name <account_name> [--new_name][--uid][--gid][--use
 - `fs_backend`
     - Type: String
     - Enum: none | GPFS | CEPH_FS | NFSv4
-    - Description: Specifies the file system of new_buckets_path (default config.NSFS_NC_STORAGE_BACKEND).
+    - Description: Specifies the file system of new_buckets_path (default config.NSFS_NC_STORAGE_BACKEND). Unset with ''.
 
 - `allow_bucket_creation`
     - Type: Boolean
     - Description: Specifies if the account to explicitly allow or block bucket creation.
 
 - `force_md5_etag`
     - Type: Boolean
-    - Description: Set the account to force md5 ETag calculation.
+    - Description: Set the account to force md5 ETag calculation. Unset with ''.
 
 - `anonymous`
     - Type: Boolean
@@ -365,16 +365,16 @@ noobaa-cli bucket update --name <bucket_name> [--new_name] [--owner]
 
 - `bucket_policy`
     - Type: String
-    - Description: Set the bucket policy, type is a string of valid JSON policy.
+    - Description: Set the bucket policy, type is a string of valid JSON policy. Unset with ''.
 
 - `fs_backend`
     - Type: String
     - Enum: none | GPFS | CEPH_FS | NFSv4
-    - Description: Specifies the file system of the bucket (default config.NSFS_NC_STORAGE_BACKEND), unset with ''.
+    - Description: Specifies the file system of the bucket (default config.NSFS_NC_STORAGE_BACKEND), Unset with ''.
 
 - `force_md5_etag`
     - Type: Boolean
-    - Description: Set the bucket to force md5 ETag calculation.
+    - Description: Set the bucket to force md5 ETag calculation. Unset with ''.
 
 
 ### Bucket Status
@@ -441,7 +441,7 @@ noobaa-cli whitelist --ips <ips>
 #### Flags -
 - `ips` (Required)
     - Type: String
-    - Description: Specifies the white list of server IPs for S3 access. Example - '["127.0.0.1", "192.0.10.0", "3002:0bd6:0000:0000:0000:ee00:0033:6778"]'
+    - Description: Specifies the white list of server IPs for S3 access. Example - '["127.0.0.1", "192.0.10.0", "3002:0bd6:0000:0000:0000:ee00:0033:6778"]'. Unset with '[]'.
 
 
 ## Managing Glacier

src/manage_nsfs/manage_nsfs_cli_errors.js

@@ -92,6 +92,12 @@ ManageCLIError.InvalidArgumentType = Object.freeze({
     http_code: 400,
 });
 
+ManageCLIError.UnsetArgumentIsInvalid = Object.freeze({
+    code: 'UnsetArgumentIsInvalid',
+    message: 'Argument can not be unset or it was unset incorrectly, its value must be set or unset correctly or check there are no leading spaces',
+    http_code: 400,
+});
+
 ManageCLIError.InvalidType = Object.freeze({
     code: 'InvalidType',
     message: 'Invalid type, available types are account, bucket, logging, whitelist, upgrade, notification or connection.',

src/manage_nsfs/manage_nsfs_constants.js

@@ -164,8 +164,21 @@ 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']);
 
-//options that can be unset using ''
-const LIST_UNSETABLE_OPTIONS = ['fs_backend', 's3_policy', 'force_md5_etag'];
+// CLI UNSET VALUES
+const CLI_EMPTY_STRING = '';
+const CLI_EMPTY_STRING_ARRAY = '[]';
+
+const CLI_EMPTY_VALUES = new Set([CLI_EMPTY_STRING, CLI_EMPTY_STRING_ARRAY]);
+
+// options that can be unset using '' / '[]'
+const UNSETTABLE_OPTIONS_OBJ = Object.freeze({
+    'fs_backend': CLI_EMPTY_STRING,
+    'bucket_policy': CLI_EMPTY_STRING,
+    'force_md5_etag': CLI_EMPTY_STRING,
+    'supplemental_groups': CLI_EMPTY_STRING,
+    'new_buckets_path': CLI_EMPTY_STRING,
+    'ips': CLI_EMPTY_STRING_ARRAY,
+});
 
 const LIST_ACCOUNT_FILTERS = ['uid', 'gid', 'user', 'name', 'access_key'];
 const LIST_BUCKET_FILTERS = ['name'];
@@ -181,7 +194,10 @@ exports.OPTION_TYPE = OPTION_TYPE;
 exports.FROM_FILE = FROM_FILE;
 exports.BOOLEAN_STRING_VALUES = BOOLEAN_STRING_VALUES;
 exports.BOOLEAN_STRING_OPTIONS = BOOLEAN_STRING_OPTIONS;
-exports.LIST_UNSETABLE_OPTIONS = LIST_UNSETABLE_OPTIONS;
+exports.UNSETTABLE_OPTIONS_OBJ = UNSETTABLE_OPTIONS_OBJ;
+exports.CLI_EMPTY_VALUES = CLI_EMPTY_VALUES;
+exports.CLI_EMPTY_STRING = CLI_EMPTY_STRING;
+exports.CLI_EMPTY_STRING_ARRAY = CLI_EMPTY_STRING_ARRAY;
 
 exports.LIST_ACCOUNT_FILTERS = LIST_ACCOUNT_FILTERS;
 exports.LIST_BUCKET_FILTERS = LIST_BUCKET_FILTERS;

src/manage_nsfs/manage_nsfs_help_utils.js

@@ -98,7 +98,7 @@ Usage:
 
 Flags:
 
-    --ips <string>                                          Set configuring for allowed IP addresses; format: '["127.0.0.1", "192.0.10.0", "3002:0bd6:0000:0000:0000:ee00:0033:6778"]'
+    --ips <string>                                          Set configuring for allowed IP addresses; format: '["127.0.0.1", "192.0.10.0", "3002:0bd6:0000:0000:0000:ee00:0033:6778"]', unset using '[]'
 
 `;
 
@@ -161,12 +161,12 @@ Flags:
     --uid <number>                                        (optional)        Update the User Identifier (UID)
     --gid <number>                                        (optional)        Update the Group Identifier (GID)
     --supplemental_groups <string>                        (optional)        Update the list of supplemental groups (List of GID) seperated by comma(,) example: 211,202,23 - it will override existing list (unset with '')
-    --new_buckets_path <string>                           (optional)        Update the filesystem's root path where each subdirectory is a bucket
+    --new_buckets_path <string>                           (optional)        Update the filesystem's root path where each subdirectory is a bucket (unset with '')
     --user <string>                                       (optional)        Update the OS user name (instead of uid and gid)
     --regenerate                                          (optional)        Update automatically generated access key and secret key
     --access_key <string>                                 (optional)        Update the access key
     --secret_key <string>                                 (optional)        Update the secret key
-    --fs_backend <none | GPFS | CEPH_FS | NFSv4>          (optional)        Update the filesystem type of new_buckets_path (default config.NSFS_NC_STORAGE_BACKEND)
+    --fs_backend <none | GPFS | CEPH_FS | NFSv4>          (optional)        Update the filesystem type of new_buckets_path (default config.NSFS_NC_STORAGE_BACKEND), (unset with '')
     --allow_bucket_creation <true | false>                (optional)        Update the account to explicitly allow or block bucket creation
     --force_md5_etag <true | false>                       (optional)        Update the account to force md5 etag calculation (unset with '') (will override default config.NSFS_NC_STORAGE_BACKEND)
     --iam_operate_on_root_account <true | false>          (optional)        Update the account to create root accounts instead of IAM users in IAM API requests.

src/manage_nsfs/manage_nsfs_validations.js

@@ -12,7 +12,7 @@ const bucket_policy_utils = require('../endpoint/s3/s3_bucket_policy_utils');
 const { throw_cli_error, get_options_from_file, get_boolean_or_string_value, get_bucket_owner_account_by_id,
     is_name_update, is_access_key_update } = require('../manage_nsfs/manage_nsfs_cli_utils');
 const { TYPES, ACTIONS, VALID_OPTIONS, OPTION_TYPE, FROM_FILE, BOOLEAN_STRING_VALUES, BOOLEAN_STRING_OPTIONS,
-    GLACIER_ACTIONS, LIST_UNSETABLE_OPTIONS, ANONYMOUS, DIAGNOSE_ACTIONS, UPGRADE_ACTIONS } = require('../manage_nsfs/manage_nsfs_constants');
+    GLACIER_ACTIONS, UNSETTABLE_OPTIONS_OBJ, CLI_EMPTY_VALUES, 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');
@@ -158,16 +158,19 @@ function validate_no_extra_options(type, action, input_options, is_options_from_
 }
 
 /**
- * validate_options_type_by_value check the type of the value that match what we expect.
+ * validate_options_type_by_value checks the type of the value that match what we expect.
+ * another check is for unset value (specified by ''/'[]') - it'll be allowed only for flags specified in UNSETTABLE_OPTIONS_OBJ
  * @param {object} input_options_with_data object with flag (key) and value
  */
 function validate_options_type_by_value(input_options_with_data) {
     for (const [option, value] of Object.entries(input_options_with_data)) {
         const type_of_option = OPTION_TYPE[option];
         const type_of_value = typeof value;
+        const is_empty_cli_value = CLI_EMPTY_VALUES.has(value);
+        const is_unsettable_option_match_value = UNSETTABLE_OPTIONS_OBJ[option] === value;
         if (type_of_value !== type_of_option) {
-            // special case for unset value (specified by '').
-            if (LIST_UNSETABLE_OPTIONS.includes(option) && value === '') {
+            // if unset is allowed but the type is not string, we allow it
+            if (is_empty_cli_value && is_unsettable_option_match_value) {
                 continue;
             }
             // special case for names, although the type is string we want to allow numbers as well
@@ -192,6 +195,12 @@ function validate_options_type_by_value(input_options_with_data) {
             const details = `type of flag ${option} should be ${type_of_option} (and the received value is ${value})`;
             throw_cli_error(ManageCLIError.InvalidArgumentType, details);
         }
+        // special case for unset value (specified by '' or '[]').
+        if (is_empty_cli_value && !is_unsettable_option_match_value) {
+            let details = `flag value of ${option} is '${value}' but this option can't be unset via '${value}'.`;
+            if (UNSETTABLE_OPTIONS_OBJ[option] !== undefined) details += ` Please use '${UNSETTABLE_OPTIONS_OBJ[option]}' instead.`;
+            throw_cli_error(ManageCLIError.UnsetArgumentIsInvalid, details);
+        }
     }
 }
 

src/test/system_tests/test_utils.js

@@ -22,6 +22,9 @@ const IS_GPFS = !_.isUndefined(GPFS_ROOT_PATH);
 const TMP_PATH = get_tmp_path();
 const TEST_TIMEOUT = 60 * 1000;
 
+// NC CLI Constants
+const CLI_UNSET_EMPTY_STRING = "''";
+
 /**
  * TMP_PATH is a path to the tmp path based on the process platform
  * in contrast to linux, /tmp/ path on mac is a symlink to /private/tmp/
@@ -814,3 +817,4 @@ exports.update_system_json = update_system_json;
 exports.fail_test_if_default_config_dir_exists = fail_test_if_default_config_dir_exists;
 exports.create_config_dir = create_config_dir;
 exports.clean_config_dir = clean_config_dir;
+exports.CLI_UNSET_EMPTY_STRING = CLI_UNSET_EMPTY_STRING;

src/test/unit_tests/jest_tests/test_nc_nsfs_account_cli.test.js renamed to src/test/unit_tests/jest_tests/test_nc_account_cli.test.js

@@ -12,7 +12,7 @@ const path = require('path');
 const os_util = require('../../../util/os_utils');
 const fs_utils = require('../../../util/fs_utils');
 const { ConfigFS } = require('../../../sdk/config_fs');
-const { set_path_permissions_and_owner, create_fs_user_by_platform,
+const { set_path_permissions_and_owner, create_fs_user_by_platform, CLI_UNSET_EMPTY_STRING,
     delete_fs_user_by_platform, TMP_PATH, set_nc_config_dir_in_config } = require('../../system_tests/test_utils');
 const { TYPES, ACTIONS, ANONYMOUS } = require('../../../manage_nsfs/manage_nsfs_constants');
 const ManageCLIError = require('../../../manage_nsfs/manage_nsfs_cli_errors').ManageCLIError;
@@ -801,8 +801,7 @@ describe('manage nsfs cli account flow', () => {
             it('cli update account unset new_buckets_path', async () => {
                 const { name } = defaults;
                 //in exec_manage_cli an empty string is passed as no input. so operation fails on invalid type. use the string '' instead 
-                const empty_string = '\'\'';
-                const account_options = { config_root, name, new_buckets_path: empty_string};
+                const account_options = { config_root, name, new_buckets_path: CLI_UNSET_EMPTY_STRING};
                 const action = ACTIONS.UPDATE;
                 await exec_manage_cli(type, action, account_options);
                 let new_account_details = await config_fs.get_account_by_name(name, config_fs_account_options);
@@ -859,8 +858,7 @@ describe('manage nsfs cli account flow', () => {
                 expect(new_account_details.force_md5_etag).toBe(true);
 
                 // unset force_md5_etag
-                const empty_string = '\'\'';
-                account_options.force_md5_etag = empty_string;
+                account_options.force_md5_etag = CLI_UNSET_EMPTY_STRING;
                 await exec_manage_cli(type, action, account_options);
                 new_account_details = await config_fs.get_account_by_name(name, config_fs_account_options);
                 expect(new_account_details.force_md5_etag).toBeUndefined();
@@ -890,13 +888,21 @@ describe('manage nsfs cli account flow', () => {
                 expect(new_account_details.iam_operate_on_root_account).toBe(true);
 
                 // unset iam_operate_on_root_account (is not allowed)
-                const empty_string = '\'\'';
-                account_options.iam_operate_on_root_account = empty_string;
+                account_options.iam_operate_on_root_account = CLI_UNSET_EMPTY_STRING;
                 const res = await exec_manage_cli(type, action, account_options);
                 expect(JSON.parse(res.stdout).error.code).toBe(
                     ManageCLIError.InvalidArgumentType.code);
             });
 
+            it(`should fail - cli update account unset flag access_key with ''`, async function() {
+                // unset access_key (is not allowed)
+                const action = ACTIONS.UPDATE;
+                const { name } = defaults;
+                const account_options = { config_root, name, access_key: CLI_UNSET_EMPTY_STRING };
+                const res = await exec_manage_cli(type, action, account_options);
+                expect(JSON.parse(res.stdout).error.code).toBe(ManageCLIError.UnsetArgumentIsInvalid.code);
+            });
+
             it('cli update account iam_operate_on_root_account true when account owns a bucket', async function() {
                 // cli create bucket
                 const bucket_name = 'my-bucket';

src/test/unit_tests/jest_tests/test_nc_nsfs_bucket_cli.test.js

@@ -9,7 +9,7 @@ const path = require('path');
 const os_util = require('../../../util/os_utils');
 const fs_utils = require('../../../util/fs_utils');
 const { ConfigFS } = require('../../../sdk/config_fs');
-const { set_path_permissions_and_owner, TMP_PATH, generate_s3_policy,
+const { set_path_permissions_and_owner, TMP_PATH, generate_s3_policy, CLI_UNSET_EMPTY_STRING,
     set_nc_config_dir_in_config } = require('../../system_tests/test_utils');
 const { ACTIONS, TYPES } = require('../../../manage_nsfs/manage_nsfs_constants');
 const { get_process_fs_context, is_path_exists, get_bucket_tmpdir_full_path } = require('../../../util/native_fs_utils');
@@ -505,8 +505,7 @@ describe('manage nsfs cli bucket flow', () => {
             expect(bucket_config.force_md5_etag).toBe(true);
 
             // unset force_md5_etag
-            const empty_string = '\'\'';
-            bucket_options.force_md5_etag = empty_string;
+            bucket_options.force_md5_etag = CLI_UNSET_EMPTY_STRING;
             await exec_manage_cli(TYPES.BUCKET, action, bucket_options);
             bucket_config = await config_fs.get_bucket_by_name(bucket_defaults.name);
             expect(bucket_config.force_md5_etag).toBeUndefined();

src/test/unit_tests/nc_index.js

@@ -9,7 +9,7 @@ require('./test_namespace_fs');
 require('./test_ns_list_objects');
 require('./test_namespace_fs_mpu');
 require('./test_nb_native_fs');
-require('./test_nc_nsfs_cli');
+require('./test_nc_cli');
 require('./test_nc_health');
 require('./test_nsfs_access');
 require('./test_nsfs_integration');