From 7b97ac55ea9e48ed04deba649f248b50ebc15ab7 Mon Sep 17 00:00:00 2001 From: Xincun Li <147451452+xincunli-sonic@users.noreply.github.com> Date: Thu, 16 May 2024 23:16:53 -0700 Subject: [PATCH] Add full configuration validation. (#3316) #### What I did Before apply the json patch, we will precheck and simulate-patch the payload in entire box level. #### How I did it 1. Add Duplication check 2. JSON patch structure validating 3. Simulating patch to full configuration 4. Verifying simulating result match YANG validation. #### How to verify it 1. Single ASIC ``` admin@str2-msn2700-spy-2:~/gcu$ cat empty.json [] admin@str2-msn2700-spy-2:~/gcu$ sudo config apply-patch empty.json Patch Applier: localhost: Patch application starting. Patch Applier: localhost: Patch: [] Patch Applier: localhost getting current config db. Patch Applier: localhost: simulating the target full config after applying the patch. Patch Applier: localhost: validating all JsonPatch operations are permitted on the specified fields Patch Applier: localhost: validating target config does not have empty tables, since they do not show up in ConfigDb. Patch Applier: localhost: sorting patch updates. Patch Applier: The localhost patch was converted into 0 changes. Patch Applier: localhost: applying 0 changes in order. Patch Applier: localhost: verifying patch updates are reflected on ConfigDB. Patch Applier: localhost patch application completed. Patch applied successfully. ``` 2. Multi ASIC ``` stli@str2-7250-2-lc01:~/gcu$ cat empty.json [] stli@str2-7250-2-lc01:~/gcu$ sudo config apply-patch empty.json sonic_yang(6):Note: Below table(s) have no YANG models: DHCP_SERVER, KUBERNETES_MASTER sonic_yang(6):Note: Below table(s) have no YANG models: KUBERNETES_MASTER sonic_yang(6):Note: Below table(s) have no YANG models: KUBERNETES_MASTER Patch Applier: localhost: Patch application starting. Patch Applier: localhost: Patch: [] Patch Applier: localhost getting current config db. Patch Applier: localhost: simulating the target full config after applying the patch. Patch Applier: localhost: validating all JsonPatch operations are permitted on the specified fields Patch Applier: localhost: validating target config does not have empty tables, since they do not show up in ConfigDb. Patch Applier: localhost: sorting patch updates. Patch Applier: The localhost patch was converted into 0 changes. Patch Applier: localhost: applying 0 changes in order. Patch Applier: localhost: verifying patch updates are reflected on ConfigDB. Patch Applier: localhost patch application completed. Patch Applier: asic0: Patch application starting. Patch Applier: asic0: Patch: [] Patch Applier: asic0 getting current config db. Patch Applier: asic0: simulating the target full config after applying the patch. Patch Applier: asic0: validating all JsonPatch operations are permitted on the specified fields Patch Applier: asic0: validating target config does not have empty tables, since they do not show up in ConfigDb. Patch Applier: asic0: sorting patch updates. Patch Applier: The asic0 patch was converted into 0 changes. Patch Applier: asic0: applying 0 changes in order. Patch Applier: asic0: verifying patch updates are reflected on ConfigDB. Patch Applier: asic0 patch application completed. Patch Applier: asic1: Patch application starting. Patch Applier: asic1: Patch: [] Patch Applier: asic1 getting current config db. Patch Applier: asic1: simulating the target full config after applying the patch. Patch Applier: asic1: validating all JsonPatch operations are permitted on the specified fields Patch Applier: asic1: validating target config does not have empty tables, since they do not show up in ConfigDb. Patch Applier: asic1: sorting patch updates. Patch Applier: The asic1 patch was converted into 0 changes. Patch Applier: asic1: applying 0 changes in order. Patch Applier: asic1: verifying patch updates are reflected on ConfigDB. Patch Applier: asic1 patch application completed. Patch applied successfully. ``` --- config/main.py | 62 ++++++++-- generic_config_updater/generic_updater.py | 29 ++--- generic_config_updater/gu_common.py | 1 + tests/config_test.py | 142 ++++++++++++++++++++++ 4 files changed, 206 insertions(+), 28 deletions(-) diff --git a/config/main.py b/config/main.py index 41a9f48121..b64e93949c 100644 --- a/config/main.py +++ b/config/main.py @@ -20,6 +20,7 @@ from jsonpointer import JsonPointerException from collections import OrderedDict from generic_config_updater.generic_updater import GenericUpdater, ConfigFormat, extract_scope +from generic_config_updater.gu_common import HOST_NAMESPACE, GenericConfigUpdaterError from minigraph import parse_device_desc_xml, minigraph_encoder from natsort import natsorted from portconfig import get_child_ports @@ -27,6 +28,7 @@ from sonic_py_common import device_info, multi_asic from sonic_py_common.general import getstatusoutput_noshell from sonic_py_common.interface import get_interface_table_name, get_port_table_name, get_intf_longname +from sonic_yang_cfg_generator import SonicYangCfgDbGenerator from utilities_common import util_base from swsscommon import swsscommon from swsscommon.swsscommon import SonicV2Connector, ConfigDBConnector @@ -1155,18 +1157,23 @@ def validate_gre_type(ctx, _, value): return gre_type_value except ValueError: raise click.UsageError("{} is not a valid GRE type".format(value)) - + # Function to apply patch for a single ASIC. def apply_patch_for_scope(scope_changes, results, config_format, verbose, dry_run, ignore_non_yang_tables, ignore_path): scope, changes = scope_changes # Replace localhost to DEFAULT_NAMESPACE which is db definition of Host - if scope.lower() == "localhost" or scope == "": + if scope.lower() == HOST_NAMESPACE or scope == "": scope = multi_asic.DEFAULT_NAMESPACE - - scope_for_log = scope if scope else "localhost" + + scope_for_log = scope if scope else HOST_NAMESPACE try: # Call apply_patch with the ASIC-specific changes and predefined parameters - GenericUpdater(namespace=scope).apply_patch(jsonpatch.JsonPatch(changes), config_format, verbose, dry_run, ignore_non_yang_tables, ignore_path) + GenericUpdater(namespace=scope).apply_patch(jsonpatch.JsonPatch(changes), + config_format, + verbose, + dry_run, + ignore_non_yang_tables, + ignore_path) results[scope_for_log] = {"success": True, "message": "Success"} log.log_notice(f"'apply-patch' executed successfully for {scope_for_log} by {changes}") except Exception as e: @@ -1174,6 +1181,35 @@ def apply_patch_for_scope(scope_changes, results, config_format, verbose, dry_ru log.log_error(f"'apply-patch' executed failed for {scope_for_log} by {changes} due to {str(e)}") +def validate_patch(patch): + try: + command = ["show", "runningconfiguration", "all"] + proc = subprocess.Popen(command, text=True, stdout=subprocess.PIPE) + all_running_config, returncode = proc.communicate() + if returncode: + log.log_notice(f"Fetch all runningconfiguration failed as output:{all_running_config}") + return False + + # Structure validation and simulate apply patch. + all_target_config = patch.apply(json.loads(all_running_config)) + + # Verify target config by YANG models + target_config = all_target_config.pop(HOST_NAMESPACE) if multi_asic.is_multi_asic() else all_target_config + target_config.pop("bgpraw", None) + if not SonicYangCfgDbGenerator().validate_config_db_json(target_config): + return False + + if multi_asic.is_multi_asic(): + for asic in multi_asic.get_namespace_list(): + target_config = all_target_config.pop(asic) + target_config.pop("bgpraw", None) + if not SonicYangCfgDbGenerator().validate_config_db_json(target_config): + return False + + return True + except Exception as e: + raise GenericConfigUpdaterError(f"Validate json patch: {patch} failed due to:{e}") + # This is our main entrypoint - the main 'config' command @click.group(cls=clicommon.AbbreviationGroup, context_settings=CONTEXT_SETTINGS) @click.pass_context @@ -1381,6 +1417,9 @@ def apply_patch(ctx, patch_file_path, format, dry_run, ignore_non_yang_tables, i patch_as_json = json.loads(text) patch = jsonpatch.JsonPatch(patch_as_json) + if not validate_patch(patch): + raise GenericConfigUpdaterError(f"Failed validating patch:{patch}") + results = {} config_format = ConfigFormat[format.upper()] # Initialize a dictionary to hold changes categorized by scope @@ -1403,7 +1442,8 @@ def apply_patch(ctx, patch_file_path, format, dry_run, ignore_non_yang_tables, i # Empty case to force validate YANG model. if not changes_by_scope: asic_list = [multi_asic.DEFAULT_NAMESPACE] - asic_list.extend(multi_asic.get_namespace_list()) + if multi_asic.is_multi_asic(): + asic_list.extend(multi_asic.get_namespace_list()) for asic in asic_list: changes_by_scope[asic] = [] @@ -1416,7 +1456,7 @@ def apply_patch(ctx, patch_file_path, format, dry_run, ignore_non_yang_tables, i if failures: failure_messages = '\n'.join([f"- {failed_scope}: {results[failed_scope]['message']}" for failed_scope in failures]) - raise Exception(f"Failed to apply patch on the following scopes:\n{failure_messages}") + raise GenericConfigUpdaterError(f"Failed to apply patch on the following scopes:\n{failure_messages}") log.log_notice(f"Patch applied successfully for {patch}.") click.secho("Patch applied successfully.", fg="cyan", underline=True) @@ -1620,9 +1660,9 @@ def reload(db, filename, yes, load_sysinfo, no_service_restart, force, file_form file_input = read_json_file(file) platform = file_input.get("DEVICE_METADATA", {}).\ - get("localhost", {}).get("platform") + get(HOST_NAMESPACE, {}).get("platform") mac = file_input.get("DEVICE_METADATA", {}).\ - get("localhost", {}).get("mac") + get(HOST_NAMESPACE, {}).get("mac") if not platform or not mac: log.log_warning("Input file does't have platform or mac. platform: {}, mac: {}" @@ -1995,8 +2035,8 @@ def override_config_table(db, input_config_db, dry_run): if multi_asic.is_multi_asic() and len(config_input): # Golden Config will use "localhost" to represent host name if ns == DEFAULT_NAMESPACE: - if "localhost" in config_input.keys(): - ns_config_input = config_input["localhost"] + if HOST_NAMESPACE in config_input.keys(): + ns_config_input = config_input[HOST_NAMESPACE] else: click.secho("Wrong config format! 'localhost' not found in host config! cannot override.. abort") sys.exit(1) diff --git a/generic_config_updater/generic_updater.py b/generic_config_updater/generic_updater.py index b75939749c..374ce7670c 100644 --- a/generic_config_updater/generic_updater.py +++ b/generic_config_updater/generic_updater.py @@ -2,7 +2,7 @@ import jsonpointer import os from enum import Enum -from .gu_common import GenericConfigUpdaterError, EmptyTableError, ConfigWrapper, \ +from .gu_common import HOST_NAMESPACE, GenericConfigUpdaterError, EmptyTableError, ConfigWrapper, \ DryRunConfigWrapper, PatchWrapper, genericUpdaterLogging from .patch_sorter import StrictPatchSorter, NonStrictPatchSorter, ConfigSplitter, \ TablesWithoutYangConfigSplitter, IgnorePathsFromYangConfigSplitter @@ -16,21 +16,18 @@ def extract_scope(path): if not path: raise Exception("Wrong patch with empty path.") - try: - pointer = jsonpointer.JsonPointer(path) - parts = pointer.parts - except Exception as e: - raise Exception(f"Error resolving path: '{path}' due to {e}") + pointer = jsonpointer.JsonPointer(path) + parts = pointer.parts if not parts: - raise Exception("Wrong patch with empty path.") + raise GenericConfigUpdaterError("Wrong patch with empty path.") if parts[0].startswith("asic"): if not parts[0][len("asic"):].isnumeric(): - raise Exception(f"Error resolving path: '{path}' due to incorrect ASIC number.") + raise GenericConfigUpdaterError(f"Error resolving path: '{path}' due to incorrect ASIC number.") scope = parts[0] remainder = "/" + "/".join(parts[1:]) - elif parts[0] == "localhost": - scope = "localhost" + elif parts[0] == HOST_NAMESPACE: + scope = HOST_NAMESPACE remainder = "/" + "/".join(parts[1:]) else: scope = "" @@ -38,6 +35,7 @@ def extract_scope(path): return scope, remainder + class ConfigLock: def acquire_lock(self): # TODO: Implement ConfigLock @@ -67,7 +65,7 @@ def __init__(self, self.changeapplier = changeapplier if changeapplier is not None else ChangeApplier(namespace=self.namespace) def apply(self, patch, sort=True): - scope = self.namespace if self.namespace else 'localhost' + scope = self.namespace if self.namespace else HOST_NAMESPACE self.logger.log_notice(f"{scope}: Patch application starting.") self.logger.log_notice(f"{scope}: Patch: {patch}") @@ -84,10 +82,10 @@ def apply(self, patch, sort=True): self.config_wrapper.validate_field_operation(old_config, target_config) # Validate target config does not have empty tables since they do not show up in ConfigDb - self.logger.log_notice(f"{scope}: alidating target config does not have empty tables, " \ - "since they do not show up in ConfigDb.") + self.logger.log_notice(f"""{scope}: validating target config does not have empty tables, + since they do not show up in ConfigDb.""") empty_tables = self.config_wrapper.get_empty_tables(target_config) - if empty_tables: # if there are empty tables + if empty_tables: # if there are empty tables empty_tables_txt = ", ".join(empty_tables) raise EmptyTableError(f"{scope}: given patch is not valid because it will result in empty tables " \ "which is not allowed in ConfigDb. " \ @@ -105,9 +103,6 @@ def apply(self, patch, sort=True): self.logger.log_notice(f"The {scope} patch was converted into {changes_len} " \ f"change{'s' if changes_len != 1 else ''}{':' if changes_len > 0 else '.'}") - for change in changes: - self.logger.log_notice(f" * {change}") - # Apply changes in order self.logger.log_notice(f"{scope}: applying {changes_len} change{'s' if changes_len != 1 else ''} " \ f"in order{':' if changes_len > 0 else '.'}") diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py index 974c540c07..c15334222a 100644 --- a/generic_config_updater/gu_common.py +++ b/generic_config_updater/gu_common.py @@ -16,6 +16,7 @@ SYSLOG_IDENTIFIER = "GenericConfigUpdater" SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__)) GCU_FIELD_OP_CONF_FILE = f"{SCRIPT_DIR}/gcu_field_operation_validators.conf.json" +HOST_NAMESPACE = "localhost" class GenericConfigUpdaterError(Exception): pass diff --git a/tests/config_test.py b/tests/config_test.py index 1054a52a33..1e5f7a9009 100644 --- a/tests/config_test.py +++ b/tests/config_test.py @@ -1,3 +1,4 @@ +import copy import pytest import filecmp import importlib @@ -167,6 +168,39 @@ Reloading Monit configuration ... """ +config_temp = { + "scope": { + "ACL_TABLE": { + "MY_ACL_TABLE": { + "policy_desc": "My ACL", + "ports": ["Ethernet1", "Ethernet2"], + "stage": "ingress", + "type": "L3" + } + }, + "PORT": { + "Ethernet1": { + "alias": "fortyGigE0/0", + "description": "fortyGigE0/0", + "index": "0", + "lanes": "29,30,31,32", + "mtu": "9100", + "pfc_asym": "off", + "speed": "40000" + }, + "Ethernet2": { + "alias": "fortyGigE0/100", + "description": "fortyGigE0/100", + "index": "25", + "lanes": "125,126,127,128", + "mtu": "9100", + "pfc_asym": "off", + "speed": "40000" + } + } + } + } + def mock_run_command_side_effect(*args, **kwargs): command = args[0] if isinstance(command, str): @@ -1023,6 +1057,7 @@ def setUp(self): self.any_checkpoints_list = ["checkpoint1", "checkpoint2", "checkpoint3"] self.any_checkpoints_list_as_text = json.dumps(self.any_checkpoints_list, indent=4) + @patch('config.main.validate_patch', mock.Mock(return_value=True)) def test_apply_patch__no_params__get_required_params_error_msg(self): # Arrange unexpected_exit_code = 0 @@ -1035,6 +1070,7 @@ def test_apply_patch__no_params__get_required_params_error_msg(self): self.assertNotEqual(unexpected_exit_code, result.exit_code) self.assertTrue(expected_output in result.output) + @patch('config.main.validate_patch', mock.Mock(return_value=True)) def test_apply_patch__help__gets_help_msg(self): # Arrange expected_exit_code = 0 @@ -1047,6 +1083,7 @@ def test_apply_patch__help__gets_help_msg(self): self.assertEqual(expected_exit_code, result.exit_code) self.assertTrue(expected_output in result.output) + @patch('config.main.validate_patch', mock.Mock(return_value=True)) def test_apply_patch__only_required_params__default_values_used_for_optional_params(self): # Arrange expected_exit_code = 0 @@ -1065,6 +1102,7 @@ def test_apply_patch__only_required_params__default_values_used_for_optional_par mock_generic_updater.apply_patch.assert_called_once() mock_generic_updater.apply_patch.assert_has_calls([expected_call_with_default_values]) + @patch('config.main.validate_patch', mock.Mock(return_value=True)) def test_apply_patch__all_optional_params_non_default__non_default_values_used(self): # Arrange expected_exit_code = 0 @@ -1094,6 +1132,7 @@ def test_apply_patch__all_optional_params_non_default__non_default_values_used(s mock_generic_updater.apply_patch.assert_called_once() mock_generic_updater.apply_patch.assert_has_calls([expected_call_with_non_default_values]) + @patch('config.main.validate_patch', mock.Mock(return_value=True)) def test_apply_patch__exception_thrown__error_displayed_error_code_returned(self): # Arrange unexpected_exit_code = 0 @@ -1129,6 +1168,7 @@ def test_apply_patch__optional_parameters_passed_correctly(self): ["--ignore-path", "/ANY_TABLE"], mock.call(self.any_patch, ConfigFormat.CONFIGDB, False, False, False, ("/ANY_TABLE",))) + @patch('config.main.validate_patch', mock.Mock(return_value=True)) def validate_apply_patch_optional_parameter(self, param_args, expected_call): # Arrange expected_exit_code = 0 @@ -2679,6 +2719,16 @@ def setUp(self): } ] + test_config = copy.deepcopy(config_temp) + data = test_config.pop("scope") + self.all_config = {} + self.all_config["localhost"] = data + self.all_config["asic0"] = data + self.all_config["asic0"]["bgpraw"] = "" + self.all_config["asic1"] = data + self.all_config["asic1"]["bgpraw"] = "" + + @patch('config.main.validate_patch', mock.Mock(return_value=True)) def test_apply_patch_multiasic(self): # Mock open to simulate file reading with patch('builtins.open', mock_open(read_data=json.dumps(self.patch_content)), create=True) as mocked_open: @@ -2698,6 +2748,7 @@ def test_apply_patch_multiasic(self): # Verify mocked_open was called as expected mocked_open.assert_called_with(self.patch_file_path, 'r') + @patch('config.main.validate_patch', mock.Mock(return_value=True)) def test_apply_patch_dryrun_multiasic(self): # Mock open to simulate file reading with patch('builtins.open', mock_open(read_data=json.dumps(self.patch_content)), create=True) as mocked_open: @@ -2732,6 +2783,97 @@ def test_apply_patch_dryrun_multiasic(self): # Ensure ConfigDBConnector was never instantiated or called mock_config_db_connector.assert_not_called() + @patch('config.main.subprocess.Popen') + @patch('config.main.SonicYangCfgDbGenerator.validate_config_db_json', mock.Mock(return_value=True)) + def test_apply_patch_validate_patch_multiasic(self, mock_subprocess_popen): + mock_instance = MagicMock() + mock_instance.communicate.return_value = (json.dumps(self.all_config), 0) + mock_subprocess_popen.return_value = mock_instance + + # Mock open to simulate file reading + with patch('builtins.open', mock_open(read_data=json.dumps(self.patch_content)), create=True) as mocked_open: + # Mock GenericUpdater to avoid actual patch application + with patch('config.main.GenericUpdater') as mock_generic_updater: + mock_generic_updater.return_value.apply_patch = MagicMock() + + print("Multi ASIC: {}".format(multi_asic.is_multi_asic())) + # Invocation of the command with the CliRunner + result = self.runner.invoke(config.config.commands["apply-patch"], + [self.patch_file_path], + catch_exceptions=True) + + print("Exit Code: {}, output: {}".format(result.exit_code, result.output)) + # Assertions and verifications + self.assertEqual(result.exit_code, 0, "Command should succeed.") + self.assertIn("Patch applied successfully.", result.output) + + # Verify mocked_open was called as expected + mocked_open.assert_called_with(self.patch_file_path, 'r') + + @patch('config.main.subprocess.Popen') + @patch('config.main.SonicYangCfgDbGenerator.validate_config_db_json', mock.Mock(return_value=True)) + def test_apply_patch_validate_patch_with_badpath_multiasic(self, mock_subprocess_popen): + mock_instance = MagicMock() + mock_instance.communicate.return_value = (json.dumps(self.all_config), 0) + mock_subprocess_popen.return_value = mock_instance + + bad_patch = copy.deepcopy(self.patch_content) + bad_patch.append({ + "value": { + "policy_desc": "New ACL Table", + "ports": ["Ethernet3", "Ethernet4"], + "stage": "ingress", + "type": "L3" + } + }) + + # Mock open to simulate file reading + with patch('builtins.open', mock_open(read_data=json.dumps(bad_patch)), create=True) as mocked_open: + # Mock GenericUpdater to avoid actual patch application + with patch('config.main.GenericUpdater') as mock_generic_updater: + mock_generic_updater.return_value.apply_patch = MagicMock() + + print("Multi ASIC: {}".format(multi_asic.is_multi_asic())) + # Invocation of the command with the CliRunner + result = self.runner.invoke(config.config.commands["apply-patch"], + [self.patch_file_path], + catch_exceptions=True) + + print("Exit Code: {}, output: {}".format(result.exit_code, result.output)) + # Assertions and verifications + self.assertNotEqual(result.exit_code, 0, "Command should failed.") + self.assertIn("Failed to apply patch", result.output) + + # Verify mocked_open was called as expected + mocked_open.assert_called_with(self.patch_file_path, 'r') + + @patch('config.main.subprocess.Popen') + @patch('config.main.SonicYangCfgDbGenerator.validate_config_db_json', mock.Mock(return_value=True)) + def test_apply_patch_validate_patch_with_wrong_fetch_config(self, mock_subprocess_popen): + mock_instance = MagicMock() + mock_instance.communicate.return_value = (json.dumps(self.all_config), 2) + mock_subprocess_popen.return_value = mock_instance + + # Mock open to simulate file reading + with patch('builtins.open', mock_open(read_data=json.dumps(self.patch_content)), create=True) as mocked_open: + # Mock GenericUpdater to avoid actual patch application + with patch('config.main.GenericUpdater') as mock_generic_updater: + mock_generic_updater.return_value.apply_patch = MagicMock() + + print("Multi ASIC: {}".format(multi_asic.is_multi_asic())) + # Invocation of the command with the CliRunner + result = self.runner.invoke(config.config.commands["apply-patch"], + [self.patch_file_path], + catch_exceptions=True) + + print("Exit Code: {}, output: {}".format(result.exit_code, result.output)) + # Assertions and verifications + self.assertNotEqual(result.exit_code, 0, "Command should failed.") + self.assertIn("Failed to apply patch", result.output) + + # Verify mocked_open was called as expected + mocked_open.assert_called_with(self.patch_file_path, 'r') + @classmethod def teardown_class(cls): print("TEARDOWN")