diff --git a/config/main.py b/config/main.py index 44f633bf44..36f89b26ca 100644 --- a/config/main.py +++ b/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 diff --git a/tests/config_reload_input/config_db_invalid.json b/tests/config_reload_input/config_db_invalid.json new file mode 100644 index 0000000000..cb394023d8 --- /dev/null +++ b/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" + } + } +} diff --git a/tests/config_test.py b/tests/config_test.py index 4ebc14cd14..18ac823b82 100644 --- a/tests/config_test.py +++ b/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,25 +83,40 @@ 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 ... """ @@ -107,9 +124,9 @@ 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,6 +455,8 @@ 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): @@ -443,7 +464,8 @@ def setup_class(cls): 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):