diff --git a/generic_config_updater/generic_updater.py b/generic_config_updater/generic_updater.py index 1d9bce530..671213bc5 100644 --- a/generic_config_updater/generic_updater.py +++ b/generic_config_updater/generic_updater.py @@ -1,4 +1,3 @@ -import fnmatch import json import jsonpatch import jsonpointer @@ -129,31 +128,6 @@ def apply(self, patch, sort=True): which is not allowed in ConfigDb. \ Table{'s' if len(empty_tables) != 1 else ''}: {empty_tables_txt}") # Generate list of changes to apply - - # Skip sorting for tables listed in skip_sort_tables.txt - if sort: - skip_sort_tables_file = os.path.join(os.path.dirname(__file__), "skip_sort_tables.txt") - skip_sort_tables = [] - if os.path.isfile(skip_sort_tables_file): - with open(skip_sort_tables_file) as f: - skip_sort_tables = [line.strip() for line in f if line.strip()] - self.logger.log_notice(f"{scope}: tables to skip sorting: {skip_sort_tables}") - if skip_sort_tables: - all_match = True - for operation in patch: - op_path = operation.get("path", "") - matched = any(fnmatch.fnmatch(op_path, pattern) for pattern in skip_sort_tables) - if matched: - self.logger.log_notice( - f"{scope}: patch path {op_path} matches skip-sort pattern." - ) - else: - all_match = False - break - if all_match: - self.logger.log_notice(f"{scope}: all patch operations match skip-sort patterns, skipping sort.") - sort = False - if sort: self.logger.log_notice(f"{scope}: sorting patch updates.") changes = self.patchsorter.sort(patch) diff --git a/generic_config_updater/patch_sorter.py b/generic_config_updater/patch_sorter.py index 75f20849c..62fc7476d 100644 --- a/generic_config_updater/patch_sorter.py +++ b/generic_config_updater/patch_sorter.py @@ -742,6 +742,7 @@ class RemoveCreateOnlyDependencyMoveValidator: def __init__(self, path_addressing): self.path_addressing = path_addressing self.create_only_filter = CreateOnlyFilter(path_addressing).get_filter() + self.logger = genericUpdaterLogging.get_logger(title="Patch Sorter - RemoveCreateOnly") def validate(self, group: JsonMoveGroup, diff, simulated_config): # Note: group is not used by this validator @@ -816,7 +817,28 @@ def _validate_member(self, tokens, member_name, current_config, target_config, s return False member_path = f"/{table_to_check}/{member_name}" - for ref_path in self.path_addressing.find_ref_paths(member_path, simulated_config, reload_config=reload_config): + try: + ref_paths = self.path_addressing.find_ref_paths( + member_path, simulated_config, reload_config=reload_config) + except (ValueError, KeyError) as e: + # An unresolvable or malformed reference against the simulated intermediate config + # raises here. The motivating case: a create-only field change (e.g. a PORT breakout + # that rewrites lanes) has transiently removed this member from a multi-member leaf-list + # (e.g. ACL_TABLE.ports) while a leafref to it is still present, so the dangling leafref + # fails to resolve (list.index raises ValueError). Other resolution failures in the + # xpath<->configdb-path conversion (schema/key-count mismatches raise ValueError; a table + # with no YANG model raises KeyError) likewise indicate the reference cannot be resolved + # against this intermediate config. In every case the ordering is an invalid intermediate + # move, so reject it and let the sort backtrack rather than aborting the whole sort. + # Scope is deliberately limited to reference-resolution errors: a loadData failure + # (sonic_yang.SonicYangException) is not caught here because FullConfigMoveValidator has + # already loaded this config into the sy singleton, so find_ref_paths skips loadData. + self.logger.log_debug( + f"Rejecting move: reference resolution failed against simulated config " + f"for '{member_path}': {type(e).__name__}: {e}") + return False + + for ref_path in ref_paths: if not self.path_addressing.has_path(current_config, ref_path): return False @@ -957,6 +979,7 @@ class NoDependencyMoveValidator: def __init__(self, path_addressing, config_wrapper): self.path_addressing = path_addressing self.config_wrapper = config_wrapper + self.logger = genericUpdaterLogging.get_logger(title="Patch Sorter - NoDependency") def validate(self, group: JsonMoveGroup, diff, simulated_config): reload_config = True @@ -973,7 +996,8 @@ def __validate_move(self, move, diff, simulated_config, reload_config: bool = Tr if operation_type == OperationType.ADD: # For add operation, we check the simulated config has no dependencies between nodes under the added path - if not self._validate_paths_config([path], simulated_config, reload_config): + if not self._validate_paths_config([path], simulated_config, reload_config, + reject_on_unresolvable_ref=True): return False elif operation_type == OperationType.REMOVE: # For remove operation, we check the current config has no dependencies between nodes under the removed path @@ -1024,7 +1048,8 @@ def _validate_replace(self, move, diff, simulated_config): # so _currently_loaded_hash will match and find_ref_paths skips loadData. # Then validate deleted_paths against current_config (requires a fresh loadData). # This ordering gives 2 loadData calls instead of 3 for REPLACE operations. - if not self._validate_paths_config(added_paths, simulated_config, reload_config=True): + if not self._validate_paths_config(added_paths, simulated_config, reload_config=True, + reject_on_unresolvable_ref=True): return False if not self._validate_paths_config(deleted_paths, diff.current_config, reload_config=True): @@ -1094,11 +1119,31 @@ def _get_list_paths(self, current_list, target_list, tokens): return deleted_paths, added_paths - def _validate_paths_config(self, paths, config, reload_config: bool = True): + def _validate_paths_config(self, paths, config, reload_config: bool = True, + reject_on_unresolvable_ref: bool = False): """ validates all config under paths do not have config and its references + + reject_on_unresolvable_ref: set only when 'config' is the transient simulated intermediate + state. A dangling leafref in that state (e.g. a create-only PORT change that has transiently + removed the port from a leaf-list such as ACL_TABLE.ports while a reference to it lingers) + makes find_ref_paths raise ValueError; a table with no YANG model makes it raise KeyError. + Either way it is an invalid intermediate move, so reject it and let the sort backtrack rather + than aborting. When 'config' is diff.current_config (a valid committed state) the flag stays + False: such an error there is genuine and must surface instead of being silently swallowed. + Scope is limited to reference-resolution errors; a loadData failure + (sonic_yang.SonicYangException) is not caught because the config is already loaded into the sy + singleton by FullConfigMoveValidator, so find_ref_paths skips loadData for it. """ - refs = self.path_addressing.find_ref_paths(paths, config, reload_config=reload_config) + try: + refs = self.path_addressing.find_ref_paths(paths, config, reload_config=reload_config) + except (ValueError, KeyError) as e: + if reject_on_unresolvable_ref: + self.logger.log_debug( + f"Rejecting move: reference resolution failed against simulated config " + f"for {paths}: {type(e).__name__}: {e}") + return False + raise for ref in refs: for path in paths: if ref.startswith(path): diff --git a/generic_config_updater/skip_sort_tables.txt b/generic_config_updater/skip_sort_tables.txt deleted file mode 100644 index 600791bb1..000000000 --- a/generic_config_updater/skip_sort_tables.txt +++ /dev/null @@ -1 +0,0 @@ -/ACL_TABLE/FAIRWATER_DACL_MITIGATION*/ports diff --git a/setup.py b/setup.py index 06380858e..423db53b5 100644 --- a/setup.py +++ b/setup.py @@ -90,7 +90,7 @@ 'sonic_cli_gen', ], package_data={ - 'generic_config_updater': ['gcu_services_validator.conf.json', 'gcu_field_operation_validators.conf.json', 'skip_sort_tables.txt'], + 'generic_config_updater': ['gcu_services_validator.conf.json', 'gcu_field_operation_validators.conf.json'], 'show': ['aliases.ini'], 'sonic_installer': ['aliases.ini'], 'tests': ['acl_input/*', diff --git a/tests/generic_config_updater/generic_updater_test.py b/tests/generic_config_updater/generic_updater_test.py index d41616e73..d86930062 100644 --- a/tests/generic_config_updater/generic_updater_test.py +++ b/tests/generic_config_updater/generic_updater_test.py @@ -1,5 +1,4 @@ import json -import jsonpatch import os import shutil import unittest @@ -46,113 +45,6 @@ def test_apply__no_errors__update_successful(self): patch_applier.patch_wrapper.verify_same_json.assert_has_calls( [call(Files.CONFIG_DB_AFTER_MULTI_PATCH, Files.CONFIG_DB_AFTER_MULTI_PATCH)]) - @patch("builtins.open", create=True) - @patch("os.path.isfile", return_value=True) - def test_apply__all_ops_match_skip_sort__sort_skipped(self, mock_isfile, mock_open): - """All operations match skip_sort_tables.txt patterns, - sort should be skipped.""" - from unittest.mock import mock_open as _mock_open - mock_open.side_effect = _mock_open(read_data="/ACL_TABLE/FAIRWATER_DACL_MITIGATION*/ports\n").side_effect - - deny_patch = jsonpatch.JsonPatch([ - { - "op": "add", - "path": "/ACL_TABLE/FAIRWATER_DACL_MITIGATION_V2/ports", - "value": ["Ethernet0", "Ethernet1"] - } - ]) - patch_applier = self.__create_patch_applier_for_patch(deny_patch) - - patch_applier.apply(deny_patch) - - patch_applier.patchsorter.sort.assert_not_called() - - @patch("builtins.open", create=True) - @patch("os.path.isfile", return_value=True) - def test_apply__no_ops_match_skip_sort__sort_not_skipped(self, mock_isfile, mock_open): - """No operations match skip_sort_tables.txt patterns, - sort should still happen.""" - from unittest.mock import mock_open as _mock_open - mock_open.side_effect = _mock_open(read_data="/ACL_TABLE/FAIRWATER_DACL_MITIGATION*/ports\n").side_effect - - normal_patch = jsonpatch.JsonPatch([ - { - "op": "add", - "path": "/ACL_TABLE/SOME_OTHER_TABLE/ports", - "value": ["Ethernet0"] - } - ]) - changes = [Mock()] - patch_applier = self.__create_patch_applier_for_patch( - normal_patch, changes=changes - ) - - patch_applier.apply(normal_patch) - - patch_applier.patchsorter.sort.assert_called_once() - - @patch("builtins.open", create=True) - @patch("os.path.isfile", return_value=True) - def test_apply__partial_ops_match_skip_sort__sort_not_skipped(self, mock_isfile, mock_open): - """Only some operations match skip_sort_tables.txt patterns, - sort should still happen since not ALL ops match.""" - from unittest.mock import mock_open as _mock_open - mock_open.side_effect = _mock_open(read_data="/ACL_TABLE/FAIRWATER_DACL_MITIGATION*/ports\n").side_effect - - mixed_patch = jsonpatch.JsonPatch([ - { - "op": "add", - "path": "/ACL_TABLE/FAIRWATER_DACL_MITIGATION_V2/ports", - "value": ["Ethernet0"] - }, - { - "op": "add", - "path": "/ACL_TABLE/SOME_OTHER_TABLE/ports", - "value": ["Ethernet1"] - } - ]) - changes = [Mock()] - patch_applier = self.__create_patch_applier_for_patch( - mixed_patch, changes=changes - ) - - patch_applier.apply(mixed_patch) - - patch_applier.patchsorter.sort.assert_called_once() - - def __create_patch_applier_for_patch( - self, patch_obj, changes=None - ): - """Helper to create PatchApplier with mocks for any patch.""" - config_wrapper = Mock() - old_config = {"ACL_TABLE": {}} - new_config = {"ACL_TABLE": { - "DENY_NEW_INGRESS_TABLE": { - "ports": ["Ethernet0", "Ethernet1"] - } - }} - config_wrapper.get_config_db_as_json.side_effect = [ - old_config, new_config - ] - config_wrapper.get_empty_tables.return_value = [] - - patch_wrapper = Mock() - patch_wrapper.simulate_patch.return_value = new_config - patch_wrapper.verify_same_json.return_value = True - - if changes is None: - changes = [Mock()] - patchsorter = Mock() - patchsorter.sort.return_value = changes - - changeapplier = Mock() - changeapplier.apply.return_value = new_config - - return gu.PatchApplier( - patchsorter, changeapplier, - config_wrapper, patch_wrapper - ) - def __create_patch_applier(self, changes=None, valid_patch_does_not_produce_empty_tables=True, diff --git a/tests/generic_config_updater/patch_sorter_test.py b/tests/generic_config_updater/patch_sorter_test.py index bb9e61ba7..bf7574afe 100644 --- a/tests/generic_config_updater/patch_sorter_test.py +++ b/tests/generic_config_updater/patch_sorter_test.py @@ -1222,6 +1222,78 @@ def setUp(self): path_addressing = ps.PathAddressing(config_wrapper) self.validator = ps.NoDependencyMoveValidator(path_addressing, config_wrapper) + def test_validate__unresolvable_ref_in_simulated_config__add_rejected(self): + # A dangling leafref in the transient simulated intermediate config makes find_ref_paths + # raise ValueError. For a simulated-config-facing check (ADD here), the validator must reject + # the move (return False) so the sort backtracks, rather than letting the exception abort the + # whole sort. + current_config = {"PORT": {"Ethernet0": {}}} + target_config = { + "PORT": {"Ethernet0": {}}, + "ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0"]}} + } + diff = ps.Diff(current_config, target_config) + move = JsonMoveGroup(ps.JsonMove(diff, OperationType.ADD, ["ACL_TABLE"], ["ACL_TABLE"])) + simulated_config = move.apply(diff.current_config) + + self.validator.path_addressing.find_ref_paths = Mock( + side_effect=ValueError("'Ethernet0' is not in list")) + + # Must not raise; the move is rejected so the DFS can backtrack. Pin the exact arguments so a + # regression swapping simulated_config <-> diff.current_config in the ADD branch is caught + # (simulated_config is value-distinct from current_config here: it carries the added ACL_TABLE). + self.assertFalse(self.validator.validate(move, diff, simulated_config)) + self.validator.path_addressing.find_ref_paths.assert_called_once_with( + ["/ACL_TABLE"], simulated_config, reload_config=True) + + def test_validate__unresolvable_ref_in_simulated_config__replace_rejected(self): + # REPLACE is the operation type for a PORT breakout (rewriting lanes while an ACL reference + # lingers). _validate_replace validates added_paths against the simulated intermediate config, + # so an unresolvable reference there (KeyError here, e.g. a table with no YANG model) must + # reject the move rather than aborting the sort. + current_config = {"PORT": {"Ethernet0": {}}} + target_config = { + "PORT": {"Ethernet0": {}}, + "ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0"]}} + } + diff = ps.Diff(current_config, target_config) + move = JsonMoveGroup(ps.JsonMove(diff, OperationType.REPLACE, [], [])) + simulated_config = move.apply(diff.current_config) + + self.validator.path_addressing.find_ref_paths = Mock( + side_effect=KeyError("ACL_TABLE")) + + # Must not raise; the move is rejected so the DFS can backtrack. Pin the config argument so a + # regression routing REPLACE added_paths through diff.current_config instead of the simulated + # config is caught (the two configs are value-distinct: simulated carries the added ACL_TABLE). + self.assertFalse(self.validator.validate(move, diff, simulated_config)) + self.validator.path_addressing.find_ref_paths.assert_called_once_with( + ["/ACL_TABLE"], simulated_config, reload_config=True) + + def test_validate__value_error_on_current_config__propagates(self): + # A check against diff.current_config (a valid committed state) is not simulated, so a + # ValueError from find_ref_paths signals a genuine schema error and must propagate rather + # than being silently swallowed as a move rejection. + current_config = { + "PORT": {"Ethernet0": {}}, + "ACL_TABLE": {"EVERFLOW": {"ports": ["Ethernet0"]}} + } + target_config = {"PORT": {"Ethernet0": {}}} + diff = ps.Diff(current_config, target_config) + move = JsonMoveGroup(ps.JsonMove(diff, OperationType.REMOVE, ["ACL_TABLE"])) + simulated_config = move.apply(diff.current_config) + + self.validator.path_addressing.find_ref_paths = Mock( + side_effect=ValueError("Keys in configDb not matching keys in SonicYang")) + + with self.assertRaises(ValueError): + self.validator.validate(move, diff, simulated_config) + # Pin that the REMOVE validation ran against diff.current_config (unguarded), not + # simulated_config: the re-raise must be the guaranteed-current-config path, not a lucky + # raise that a config swap could mask. Configs are value-distinct (current has ACL_TABLE). + self.validator.path_addressing.find_ref_paths.assert_called_once_with( + ["/ACL_TABLE"], diff.current_config, reload_config=True) + def test_validate__add_full_config_has_dependencies__failure(self): # Arrange # CROPPED_CONFIG_DB_AS_JSON has dependencies between PORT and ACL_TABLE @@ -2024,6 +2096,90 @@ def test_validate__lane_replacement_change(self): with self.subTest(name=test_case_name): self._run_single_test(test_cases[test_case_name]) + def test_validate__unresolvable_ref_in_simulated_config__move_rejected(self): + # A create-only PORT field change (breakout rewriting lanes) can transiently + # remove the port from a multi-member leaf-list (e.g. ACL_TABLE.ports) in the simulated + # intermediate config while a leafref to it is still present. Resolving that dangling + # leafref raises ValueError ("'' is not in list"). The validator must treat this as + # an invalid intermediate move (return False) so the sort backtracks, rather than letting + # the exception propagate and abort the whole sort. + current_config = { + "PORT": { + "Ethernet312": {"lanes": "305,306,307,308,309,310,311,312", "admin_status": "up"} + }, + "ACL_TABLE": { + "DATAACL": {"type": "L3", "ports": ["Ethernet312", "Ethernet280"]} + } + } + target_config = { + "PORT": { + "Ethernet312": {"lanes": "305", "admin_status": "up"} + }, + "ACL_TABLE": { + "DATAACL": {"type": "L3", "ports": ["Ethernet280"]} + } + } + # The transient intermediate config the sorter is validating: Ethernet312's create-only + # 'lanes' field has already been rewritten toward the target, but the ACL_TABLE leafref to + # it has not been removed yet, so the reference is momentarily dangling. Resolving it raises + # ValueError. Kept value-distinct from current_config (different lanes) so the argument + # assertion below would catch a regression that passed current_config to find_ref_paths. + simulated_config = { + "PORT": { + "Ethernet312": {"lanes": "305", "admin_status": "up"} + }, + "ACL_TABLE": { + "DATAACL": {"type": "L3", "ports": ["Ethernet312", "Ethernet280"]} + } + } + + move = JsonMoveGroup(Mock()) + diff = ps.Diff(current_config, target_config) + + self.validator.path_addressing.find_ref_paths = Mock( + side_effect=ValueError("'Ethernet312' is not in list")) + + # Must not raise; the move is rejected so the DFS can backtrack. + self.assertFalse(self.validator.validate(move, diff, simulated_config)) + # Pin the assertion to the code path under test: the rejection must come from the guarded + # find_ref_paths call raising, resolved against the simulated (intermediate) config. Asserting + # the exact arguments also catches a regression that swapped simulated_config <-> current_config. + self.validator.path_addressing.find_ref_paths.assert_called_once_with( + "/PORT/Ethernet312", simulated_config, reload_config=True) + + def test_validate__keyerror_in_simulated_config__move_rejected(self): + # find_ref_paths also raises KeyError (not just ValueError) when a referenced path's table + # has no YANG model in the simulated intermediate config. The guard must treat that the same + # way as an unresolvable reference: reject the move so the sort backtracks, not abort it. + current_config = { + "PORT": { + "Ethernet312": {"lanes": "305,306,307,308,309,310,311,312", "admin_status": "up"} + } + } + target_config = { + "PORT": { + "Ethernet312": {"lanes": "305", "admin_status": "up"} + } + } + # Value-distinct from current_config (lanes already rewritten toward the target) so the + # argument assertion below catches a regression that passed current_config instead. + simulated_config = { + "PORT": { + "Ethernet312": {"lanes": "305", "admin_status": "up"} + } + } + + move = JsonMoveGroup(Mock()) + diff = ps.Diff(current_config, target_config) + + self.validator.path_addressing.find_ref_paths = Mock( + side_effect=KeyError("ACL_TABLE")) + + # Must not raise; the move is rejected so the DFS can backtrack. + self.assertFalse(self.validator.validate(move, diff, simulated_config)) + self.validator.path_addressing.find_ref_paths.assert_called_once_with( + "/PORT/Ethernet312", simulated_config, reload_config=True) + def _run_single_test(self, test_case): # Arrange expected = test_case['expected']