diff --git a/generic_config_updater/patch_sorter.py b/generic_config_updater/patch_sorter.py index 33a89cfbe..5d7dacdee 100644 --- a/generic_config_updater/patch_sorter.py +++ b/generic_config_updater/patch_sorter.py @@ -748,6 +748,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) -> Tuple[bool, Optional[str]]: # Note: group is not used by this validator @@ -822,7 +823,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 @@ -963,6 +985,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) -> Tuple[bool, Optional[str]]: reload_config = True @@ -979,7 +1002,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 @@ -1030,7 +1054,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): @@ -1100,11 +1125,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/tests/generic_config_updater/patch_sorter_test.py b/tests/generic_config_updater/patch_sorter_test.py index 2cc6b94f1..501b38ca2 100644 --- a/tests/generic_config_updater/patch_sorter_test.py +++ b/tests/generic_config_updater/patch_sorter_test.py @@ -1246,6 +1246,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)[0]) + 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)[0]) + 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 @@ -2061,6 +2133,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)[0]) + # 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)[0]) + 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']