[code sync] Merge code from sonic-net/sonic-utilities:202605 to 202606

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):

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 ("'<port>' 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']