sonic-utilities: Update config reload() to verify formatting of an input file (sonic-net#2529)

sonic-utilities: bugfix-9499

Update config/main.py to verify if a config input file is properly formatted before writing it into confib_db
Update tests/config_test.py to include test cases for invalid input file
Add tests/config_reload_input/config_db_invalid.json as invalid input file used in tests/config_test.py
Signed-off-by: [email protected]

What I did
Include a check to validate if all input files are properly formatted before trying to write them to config_db to resolve bug 9499 in sonic-buildimage.

How I did it
Include a check to validate if all input files are properly formatted before trying to write them to config_db.

How to verify it
Run tests/config_test.py.
With the change added in main.py, run 'config reload' with an inproperly formatted input file.

Previous command output (if the output of a command-line utility has changed)
crystalnet@ibr02:~$ sudo config reload conf_db.json
Clear current config and reload config in config_db from the file(s) conf_db.json ? [y/N]: y
Disabling container monitoring ...
Stopping SONiC target ...
Running command: /usr/local/bin/sonic-cfggen -j /etc/sonic/init_cfg.json -j conf_db.json --write-to-db
Traceback (most recent call last):
File "/usr/local/bin/sonic-cfggen", line 452, in
main()
File "/usr/local/bin/sonic-cfggen", line 322, in main
_process_json(args, data)
File "/usr/local/bin/sonic-cfggen", line 236, in _process_json
deep_update(data, FormatConverter.to_deserialized(json.load(stream)))
File "/usr/lib/python3.9/json/init.py", line 293, in load
return loads(fp.read(),
File "/usr/lib/python3.9/json/init.py", line 346, in loads
return _default_decoder.decode(s)
File "/usr/lib/python3.9/json/decoder.py", line 337, in decode
obj, end = self.raw_decode(s, idx=_w(s, 0).end())
File "/usr/lib/python3.9/json/decoder.py", line 353, in raw_decode
obj, end = self.scan_once(s, idx)
json.decoder.JSONDecodeError: Expecting ',' delimiter: line 713 column 5 (char 21870)

*Then the terminal becomes unusable until the device is reloaded.

New command output (if the output of a command-line utility has changed)
crystalnet@ibr02:~$ sudo config reload conf_db.json
Clear current config and reload config in config_db from the file(s) conf_db.json ? [y/N]: y
Bad format: json file broken. Expecting ',' delimiter: line 713 column 5 (char 21870)

Author: cchoate54

config/main.py

@@ -1189,6 +1189,17 @@ def load_backend_acl(cfg_db, device_type):
         if os.path.isfile(BACKEND_ACL_FILE):
             clicommon.run_command("acl-loader update incremental {}".format(BACKEND_ACL_FILE), display_cmd=True)
 
+def validate_config_file(file):
+    """
+    A validator to check config files for syntax errors
+    """
+    try:
+        # Load golden config json
+        read_json_file(file)
+    except Exception as e:
+        click.secho("Bad format: json file '{}' broken.\n{}".format(file, str(e)),
+                    fg='magenta')
+        sys.exit(1)
 
 # This is our main entrypoint - the main 'config' command
 @click.group(cls=clicommon.AbbreviationGroup, context_settings=CONTEXT_SETTINGS)
@@ -1567,10 +1578,8 @@ def reload(db, filename, yes, load_sysinfo, no_service_restart, force, file_form
             click.echo("Input {} config file(s) separated by comma for multiple files ".format(num_cfg_file))
             return
 
-    #Stop services before config push
-    if not no_service_restart:
-        log.log_info("'reload' stopping services...")
-        _stop_services()
+    # Create a dictionary to store each cfg_file, namespace, and a bool representing if a the file exists
+    cfg_file_dict = {}
 
     # In Single ASIC platforms we have single DB service. In multi-ASIC platforms we have a global DB
     # service running in the host + DB services running in each ASIC namespace created per ASIC.
@@ -1595,9 +1604,27 @@ def reload(db, filename, yes, load_sysinfo, no_service_restart, force, file_form
             else:
                 file = DEFAULT_CONFIG_YANG_FILE
 
-
-        # Check the file exists before proceeding.
+        # Check if the file exists before proceeding
+        # Instead of exiting, skip the current namespace and check the next one
         if not os.path.exists(file):
+            cfg_file_dict[inst] = [file, namespace, False]
+            continue
+        cfg_file_dict[inst] = [file, namespace, True]
+
+        # Check the file is properly formatted before proceeding.
+        validate_config_file(file) 
+            
+    #Validate INIT_CFG_FILE if it exits
+    if os.path.isfile(INIT_CFG_FILE):
+        validate_config_file(INIT_CFG_FILE)
+
+    #Stop services before config push
+    if not no_service_restart:
+        log.log_info("'reload' stopping services...")
+        _stop_services()
+
+    for file, namespace, file_exists in cfg_file_dict.values():
+        if not file_exists:
             click.echo("The config file {} doesn't exist".format(file))
             continue
 

tests/config_reload_input/config_db_invalid.json

@@ -0,0 +1,62 @@
+{
+    "DEVICE_METADATA": {
+        "localhost": {
+            "docker_routing_config_mode": "split",
+            "hostname": "sonic",
+            "hwsku": "Seastone-DX010-25-50",
+            "mac": "00:e0:ec:89:6e:48",
+            "platform": "x86_64-cel_seastone-r0",
+            "type": "ToRRouter"
+        }
+    }
+    "VLAN_MEMBER": {
+        "Vlan1000|Ethernet0": {
+            "tagging_mode": "untagged",
+        },
+        "Vlan1000|Ethernet4": {
+            "tagging_mode": "untagged"
+        },
+        "Vlan1000|Ethernet8": {
+            "tagging_mode": "untagged"
+        }
+    },
+    "VLAN": {
+        "Vlan1000": {
+            "vlanid": "1000",
+            "dhcp_servers": [
+                "192.0.0.1",
+                "192.0.0.2",
+                "192.0.0.3",
+                "192.0.0.4"
+            ]
+        }
+    },
+    "PORT": {
+        "Ethernet0": {
+            "alias": "Eth1",
+            "lanes": "65, 66, 67, 68",
+            "description": "Ethernet0 100G link",
+            "speed": "100000"
+        },
+        "Ethernet4": {
+            "admin_status": "up",
+            "alias": "fortyGigE0/4",
+            "description": "Servers0:eth0",
+            "index": "1",
+            "lanes": "29,30,31,32",
+            "mtu": "9100",
+            "pfc_asym": "off",
+            "speed": "40000"
+        },
+        "Ethernet8": {
+            "admin_status": "up",
+            "alias": "fortyGigE0/8",
+            "description": "Servers1:eth0",
+            "index": "2",
+            "lanes": "33,34,35,36",
+            "mtu": "9100",
+            "pfc_asym": "off",
+            "speed": "40000"
+        }
+    }
+}

tests/config_test.py

@@ -4,6 +4,7 @@
 import traceback
 import json
 import jsonpatch
+import shutil
 import sys
 import unittest
 import ipaddress
@@ -88,6 +89,12 @@
 Reloading Monit configuration ...
 """
 
+RELOAD_CONFIG_DB_OUTPUT_INVALID_MSG = """\
+Bad format: json file"""
+
+RELOAD_CONFIG_DB_OUTPUT_INVALID_ERROR = """\
+Expecting ',' delimiter: line 12 column 5 (char 321)"""
+
 RELOAD_YANG_CFG_OUTPUT = """\
 Stopping SONiC target ...
 Running command: /usr/local/bin/sonic-cfggen  -Y /tmp/config.json  --write-to-db
@@ -104,6 +111,15 @@
 Reloading Monit configuration ...
 """
 
+RELOAD_MASIC_CONFIG_DB_OUTPUT_FILE_NOT_EXIST = """\
+Stopping SONiC target ...
+Running command: /usr/local/bin/sonic-cfggen  -j /tmp/config.json  --write-to-db
+The config file non_exist.json doesn't exist
+Running command: /usr/local/bin/sonic-cfggen  -j /tmp/config.json  -n asic1  --write-to-db
+Restarting SONiC target ...
+Reloading Monit configuration ...
+"""
+
 reload_config_with_sys_info_command_output="""\
 Running command: /usr/local/bin/sonic-cfggen -H -k Seastone-DX010-25-50 --write-to-db"""
 
@@ -195,6 +211,7 @@ def mock_run_command_side_effect_gnmi(*args, **kwargs):
 
 class TestConfigReload(object):
     dummy_cfg_file = os.path.join(os.sep, "tmp", "config.json")
+    dummy_cfg_file_contents = os.path.join(mock_db_path, "config_db.json")
 
     @classmethod
     def setup_class(cls):
@@ -206,7 +223,8 @@ def setup_class(cls):
 
         import config.main
         importlib.reload(config.main)
-        open(cls.dummy_cfg_file, 'w').close()
+        shutil.copyfile(cls.dummy_cfg_file_contents, cls.dummy_cfg_file)
+        open(cls.dummy_cfg_file, 'r').close()
 
     def test_config_reload(self, get_cmd_module, setup_single_broadcom_asic):
         with mock.patch("utilities_common.cli.run_command", mock.MagicMock(side_effect=mock_run_command_side_effect)) as mock_run_command:
@@ -479,14 +497,17 @@ def teardown_class(cls):
 
 class TestReloadConfig(object):
     dummy_cfg_file = os.path.join(os.sep, "tmp", "config.json")
+    dummy_cfg_file_contents = os.path.join(mock_db_path, "config_db.json")
+    dummy_cfg_file_invalid = os.path.join(mock_db_path, "config_db_invalid.json")
 
     @classmethod
     def setup_class(cls):
         os.environ['UTILITIES_UNIT_TESTING'] = "1"
         print("SETUP")
         import config.main
         importlib.reload(config.main)
-        open(cls.dummy_cfg_file, 'w').close()
+        shutil.copyfile(cls.dummy_cfg_file_contents, cls.dummy_cfg_file)
+        open(cls.dummy_cfg_file, 'r').close()
 
     def test_reload_config(self, get_cmd_module, setup_single_broadcom_asic):
         with mock.patch(
@@ -507,6 +528,27 @@ def test_reload_config(self, get_cmd_module, setup_single_broadcom_asic):
             assert "\n".join([l.rstrip() for l in result.output.split('\n')]) \
                 == RELOAD_CONFIG_DB_OUTPUT
 
+    def test_reload_config_invalid_config_file(self, get_cmd_module, setup_single_broadcom_asic):
+        with mock.patch(
+                "utilities_common.cli.run_command",
+                mock.MagicMock(side_effect=mock_run_command_side_effect)
+        ) as mock_run_command:
+            (config, show) = get_cmd_module
+            runner = CliRunner()
+
+            result = runner.invoke(
+                config.config.commands["reload"],
+                [self.dummy_cfg_file_invalid, '-y', '-f'])
+
+            print(result.exit_code)
+            print(result.output)
+            traceback.print_tb(result.exc_info[2])
+            assert result.exit_code == 1
+
+            output = "\n".join([l.rstrip() for l in result.output.split('\n')])
+            assert RELOAD_CONFIG_DB_OUTPUT_INVALID_MSG in output
+            assert RELOAD_CONFIG_DB_OUTPUT_INVALID_ERROR in output
+
     def test_config_reload_disabled_service(self, get_cmd_module, setup_single_broadcom_asic):
         with mock.patch(
                "utilities_common.cli.run_command",
@@ -549,6 +591,54 @@ def test_reload_config_masic(self, get_cmd_module, setup_multi_broadcom_masic):
             assert "\n".join([l.rstrip() for l in result.output.split('\n')]) \
                 == RELOAD_MASIC_CONFIG_DB_OUTPUT
 
+    def test_reload_config_masic_invalid(self, get_cmd_module, setup_multi_broadcom_masic):
+        with mock.patch(
+                "utilities_common.cli.run_command",
+                mock.MagicMock(side_effect=mock_run_command_side_effect)
+        ) as mock_run_command:
+            (config, show) = get_cmd_module
+            runner = CliRunner()
+            # 3 config files: 1 for host and 2 for asic
+            cfg_files = "{},{},{}".format(
+                            self.dummy_cfg_file,
+                            self.dummy_cfg_file_invalid,
+                            self.dummy_cfg_file)
+            result = runner.invoke(
+                config.config.commands["reload"],
+                [cfg_files, '-y', '-f'])
+
+            print(result.exit_code)
+            print(result.output)
+            traceback.print_tb(result.exc_info[2])
+            assert result.exit_code == 1
+
+            output = "\n".join([l.rstrip() for l in result.output.split('\n')])
+            assert RELOAD_CONFIG_DB_OUTPUT_INVALID_MSG in output
+            assert RELOAD_CONFIG_DB_OUTPUT_INVALID_ERROR in output
+
+    def test_reload_config_masic_non_exist_file(self, get_cmd_module, setup_multi_broadcom_masic):
+        with mock.patch(
+                "utilities_common.cli.run_command",
+                mock.MagicMock(side_effect=mock_run_command_side_effect)
+        ) as mock_run_command:
+            (config, show) = get_cmd_module
+            runner = CliRunner()
+            # 3 config files: 1 for host and 2 for asic
+            cfg_files = "{},{},{}".format(
+                            self.dummy_cfg_file,
+                            "non_exist.json",
+                            self.dummy_cfg_file)
+            result = runner.invoke(
+                config.config.commands["reload"],
+                [cfg_files, '-y', '-f'])
+
+            print(result.exit_code)
+            print(result.output)
+            traceback.print_tb(result.exc_info[2])
+            assert result.exit_code == 0
+            assert "\n".join([l.rstrip() for l in result.output.split('\n')]) \
+                == RELOAD_MASIC_CONFIG_DB_OUTPUT_FILE_NOT_EXIST
+
     def test_reload_yang_config(self, get_cmd_module,
                                         setup_single_broadcom_asic):
         with mock.patch(
@@ -568,6 +658,27 @@ def test_reload_yang_config(self, get_cmd_module,
             assert "\n".join([l.rstrip() for l in result.output.split('\n')]) \
                 == RELOAD_YANG_CFG_OUTPUT
 
+    def test_reload_yang_config_invalid(self, get_cmd_module,
+                                        setup_single_broadcom_asic):
+        with mock.patch(
+                "utilities_common.cli.run_command",
+                mock.MagicMock(side_effect=mock_run_command_side_effect)
+        ) as mock_run_command:
+            (config, show) = get_cmd_module
+            runner = CliRunner()
+
+            result = runner.invoke(config.config.commands["reload"],
+                                    [self.dummy_cfg_file_invalid, '-y', '-f', '-t', 'config_yang'])
+
+            print(result.exit_code)
+            print(result.output)
+            traceback.print_tb(result.exc_info[2])
+            assert result.exit_code == 1
+
+            output = "\n".join([l.rstrip() for l in result.output.split('\n')])
+            assert RELOAD_CONFIG_DB_OUTPUT_INVALID_MSG in output
+            assert RELOAD_CONFIG_DB_OUTPUT_INVALID_ERROR in output
+
     @classmethod
     def teardown_class(cls):
         os.environ['UTILITIES_UNIT_TESTING'] = "0"