[DRAFT] sonic-utilities: Validate input files to config reload

config/main.py

@@ -11,6 +11,7 @@
 import subprocess
 import sys
 import time
+import uuid
 import itertools
 import copy
 
@@ -1162,6 +1163,23 @@ def validate_gre_type(ctx, _, value):
     except ValueError:
         raise click.UsageError("{} is not a valid GRE type".format(value))
 
+def validate_config_file(file, remove_tmp_file=False):
+    """
+    A validator to check config files for syntax errors. If an exception is raised,
+    use remove_tmp_file to determine if a temporary file was used to store
+    /dev/stdin contents and should be deleted.
+    """
+    try:
+        # Load golden config json
+        read_json_file(file)
+    except Exception as e:
+        if remove_tmp_file:
+            os.remove(file)
+        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)
 @click.pass_context
@@ -1540,10 +1558,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_notice("'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.
@@ -1570,7 +1586,42 @@ def reload(db, filename, yes, load_sysinfo, no_service_restart, force, file_form
 
 
         # Check 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
+
+        # Check the file is properly formatted before proceeding.
+        if not sys.stdin.isatty():
+            # Pathway to store /dev/stdin contents in a temporary file
+            TMP_FILE = os.path.join('/', "tmp", f"tmp_config_stdin_{str(uuid.uuid4())}.json")
+
+            if os.path.exists(TMP_FILE):
+                click.secho("Unable to validate '{}' contents".format(file),
+                            fg='magenta')
+                sys.exit(1)
+
+            with open(file, 'r' ) as input_file, open( TMP_FILE, 'w') as tmp:
+                for line in input_file:
+                    tmp.write(line)
+
+            validate_config_file(TMP_FILE, remove_tmp_file=True)
+            cfg_file_dict[inst] = [TMP_FILE, namespace, True]  
+        else:
+            validate_config_file(file)
+            cfg_file_dict[inst] = [file, namespace, True]
+
+    #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_notice("'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,8 @@
 import traceback
 import json
 import jsonpatch
+import re
+import shutil
 import sys
 import unittest
 import ipaddress
@@ -81,35 +83,50 @@
 Please note loaded setting will be lost after system reboot. To preserve setting, run `config save`.
 """
 
-RELOAD_CONFIG_DB_OUTPUT = """\
-Stopping SONiC target ...
-Running command: /usr/local/bin/sonic-cfggen  -j /tmp/config.json  --write-to-db
+RELOAD_CONFIG_DB_OUTPUT = \
+"""Stopping SONiC target ...
+Running command: /usr/local/bin/sonic-cfggen  -j (.*?) --write-to-db
 Restarting SONiC target ...
 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
+Running command: /usr/local/bin/sonic-cfggen  -Y (.*?)  --write-to-db
 Restarting SONiC target ...
 Reloading Monit configuration ...
 """
 
-RELOAD_MASIC_CONFIG_DB_OUTPUT = """\
-Stopping SONiC target ...
-Running command: /usr/local/bin/sonic-cfggen  -j /tmp/config.json  --write-to-db
-Running command: /usr/local/bin/sonic-cfggen  -j /tmp/config.json  -n asic0  --write-to-db
-Running command: /usr/local/bin/sonic-cfggen  -j /tmp/config.json  -n asic1  --write-to-db
+RELOAD_MASIC_CONFIG_DB_OUTPUT = \
+"""Stopping SONiC target ...
+Running command: /usr/local/bin/sonic-cfggen  -j (.*?)  --write-to-db
+Running command: /usr/local/bin/sonic-cfggen  -j (.*?)  -n asic0  --write-to-db
+Running command: /usr/local/bin/sonic-cfggen  -j (.*?)  -n asic1  --write-to-db
+Restarting SONiC target ...
+Reloading Monit configuration ...
+"""
+
+RELOAD_MASIC_CONFIG_DB_OUTPUT_FILE_NOT_EXIST = \
+"""Stopping SONiC target ...
+Running command: /usr/local/bin/sonic-cfggen  -j (.*?)  --write-to-db
+The config file non_exist.json doesn't exist
+Running command: /usr/local/bin/sonic-cfggen  -j (.*?)  -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"""
 
-reload_config_with_disabled_service_output="""\
-Stopping SONiC target ...
-Running command: /usr/local/bin/sonic-cfggen  -j /tmp/config.json  --write-to-db
+reload_config_with_disabled_service_output= \
+"""Stopping SONiC target ...
+Running command: /usr/local/bin/sonic-cfggen  -j (.*?)  --write-to-db
 Restarting SONiC target ...
 Reloading Monit configuration ...
 """
@@ -195,6 +212,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 +224,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:
@@ -436,14 +455,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(
@@ -461,8 +483,36 @@ def test_reload_config(self, get_cmd_module, setup_single_broadcom_asic):
             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_CONFIG_DB_OUTPUT
+            output = "\n".join([l.rstrip() for l in result.output.split('\n')])
+            assert re.match(RELOAD_CONFIG_DB_OUTPUT, output)
+
+    def test_validate_config_file_invalid(self):
+        import pytest
+        with pytest.raises(SystemExit) as e:
+            config.validate_config_file("dummy.json")
+        assert e.type == SystemExit
+        assert e.value.code == 1
+
+    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(
@@ -481,7 +531,8 @@ def test_config_reload_disabled_service(self, get_cmd_module, setup_single_broad
 
             assert result.exit_code == 0
 
-            assert "\n".join([l.rstrip() for l in result.output.split('\n')]) == reload_config_with_disabled_service_output
+            output = "\n".join([l.rstrip() for l in result.output.split('\n')])
+            assert re.match(reload_config_with_disabled_service_output, output)
 
     def test_reload_config_masic(self, get_cmd_module, setup_multi_broadcom_masic):
         with mock.patch(
@@ -503,8 +554,56 @@ def test_reload_config_masic(self, get_cmd_module, setup_multi_broadcom_masic):
             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
+            output = "\n".join([l.rstrip() for l in result.output.split('\n')])
+            assert re.match(RELOAD_MASIC_CONFIG_DB_OUTPUT, 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
+            output = "\n".join([l.rstrip() for l in result.output.split('\n')])
+            assert re.match(RELOAD_MASIC_CONFIG_DB_OUTPUT_FILE_NOT_EXIST, output)    
 
     def test_reload_yang_config(self, get_cmd_module,
                                         setup_single_broadcom_asic):
@@ -522,8 +621,29 @@ def test_reload_yang_config(self, get_cmd_module,
             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_YANG_CFG_OUTPUT
+            output = "\n".join([l.rstrip() for l in result.output.split('\n')])
+            assert re.match(RELOAD_YANG_CFG_OUTPUT, 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):