diff --git a/plugins/module_utils/models/base.py b/plugins/module_utils/models/base.py index 4b9fefedb..24f6ff838 100644 --- a/plugins/module_utils/models/base.py +++ b/plugins/module_utils/models/base.py @@ -221,23 +221,42 @@ def to_diff_dict(self, **kwargs) -> Dict[str, Any]: **kwargs, ) - def get_diff(self, other: "NDBaseModel", exclude_unset: bool = False) -> bool: + def get_diff(self, other: "NDBaseModel", exclude_unset: bool = False, allow_superset: bool = False) -> bool: """Diff comparison. + Subclass contract: + Any subclass that overrides ``get_diff`` MUST accept both the + ``exclude_unset`` and ``allow_superset`` keyword arguments. + ``NDConfigCollection.get_diff_config`` always forwards them to the + concrete model, and Python dispatches to the subclass override + rather than to this base method. An override may ignore + ``allow_superset`` when its comparison does not need superset + semantics (see ``MaintenanceModeModel``), but it must still accept + the keyword. A non-conforming override raises ``TypeError`` at diff + time; that failure is intentional and must not be masked by catching + ``TypeError`` or inspecting the method signature at runtime. + Args: other: The model to compare against. exclude_unset: When True, only compare fields explicitly set in ``other`` (via Pydantic's ``exclude_unset``). This prevents default values from triggering false diffs during merge - operations. This is the merge-path comparison, so a subset - match is additionally cross-checked with ``merge_would_change`` - to catch merge side effects the one-way subset test cannot see - (e.g. mutually exclusive counterpart fields that the merge - would clear). + operations. + allow_superset: When True, list elements are matched + one-directionally so that an existing item with extra fields + (e.g. ``deploy``) does not trigger a spurious diff when the + proposed item omits those fields. This is independent of + ``exclude_unset``: the former controls which of ``other``'s + fields are compared, while this controls how list elements are + matched. + + This is also the merge-path comparison, so a subset match is + additionally cross-checked with ``merge_would_change`` to catch merge + side effects the one-way subset test cannot see. """ self_data = self.to_diff_dict() other_data = other.to_diff_dict(exclude_unset=exclude_unset) - is_subset = issubset(other_data, self_data) + is_subset = issubset(other_data, self_data, allow_superset=allow_superset) if is_subset and exclude_unset and self.merge_would_change(other): return False return is_subset diff --git a/plugins/module_utils/models/maintenance_mode/maintenance_mode.py b/plugins/module_utils/models/maintenance_mode/maintenance_mode.py index 129e0b080..27bb4c68f 100644 --- a/plugins/module_utils/models/maintenance_mode/maintenance_mode.py +++ b/plugins/module_utils/models/maintenance_mode/maintenance_mode.py @@ -118,7 +118,7 @@ def _require_switches_when_mode_set(self) -> "MaintenanceModeModel": # --- Custom Diff (per-switch mode comparison) --- - def get_diff(self, other: "NDBaseModel", exclude_unset: bool = False) -> bool: + def get_diff(self, other: "NDBaseModel", exclude_unset: bool = False, allow_superset: bool = False) -> bool: """ # Summary @@ -127,6 +127,8 @@ def get_diff(self, other: "NDBaseModel", exclude_unset: bool = False) -> bool: `self` is the snapshot (existing); `other` is the proposed config. Default `NDBaseModel.get_diff` does a generic subset check that does not understand the per-switch mode comparison we need. + ``exclude_unset`` and ``allow_superset`` are accepted for NDConfigCollection compatibility; + this custom per-switch comparison intentionally ignores both. ## Raises diff --git a/plugins/module_utils/models/manage_prefix_list/manage_prefix_list.py b/plugins/module_utils/models/manage_prefix_list/manage_prefix_list.py index 4a9658a33..7944b380d 100644 --- a/plugins/module_utils/models/manage_prefix_list/manage_prefix_list.py +++ b/plugins/module_utils/models/manage_prefix_list/manage_prefix_list.py @@ -67,6 +67,7 @@ IpVersionEnum, PrefixListActionEnum, ) +from ansible_collections.cisco.nd.plugins.module_utils.utils import issubset # Allowed characters for prefix list / tenant names (from OpenAPI pattern) _NAME_RE = re.compile(r"^[a-zA-Z0-9~_-]+$") @@ -357,7 +358,7 @@ def get_identifier_value(self) -> tuple[str, str | None, str]: """Return the tenant-aware composite identifier.""" return (str(self.ip_version), self.tenant_name, self.name) - def get_diff(self, other: NDBaseModel, exclude_unset: bool = False) -> bool: + def get_diff(self, other: NDBaseModel, exclude_unset: bool = False, allow_superset: bool = False) -> bool: """ Compare prefix-list config, treating omitted description as empty in replace-style states. @@ -366,11 +367,32 @@ def get_diff(self, other: NDBaseModel, exclude_unset: bool = False) -> bool: ``overridden`` it passes ``exclude_unset=False``; in that path an omitted description means the desired value is empty/absent, so a stale controller description must trigger an update. + + ``allow_superset`` is part of the ``NDBaseModel.get_diff`` subclass + contract and must be forwarded. Authoritative entry-list changes that + superset matching would otherwise hide are detected by + ``merge_would_change`` below. """ if not exclude_unset and isinstance(other, PrefixListModel): if other.description is None and self.description not in (None, ""): return False - return super().get_diff(other, exclude_unset=exclude_unset) + return super().get_diff(other, exclude_unset=exclude_unset, allow_superset=allow_superset) + + def merge_would_change(self, other: NDBaseModel) -> bool: + """ + Detect authoritative prefix-list entry changes during merged comparison. + + Entry models are serialized with defaults included so an omitted + ``action`` remains equivalent to its documented ``permit`` default. + List matching remains order-independent but requires the same entries + and fields in both directions. + """ + if isinstance(other, PrefixListModel) and "entries" in other.model_fields_set: + current_entries = [entry.to_diff_dict() for entry in self.entries or []] + proposed_entries = [entry.to_diff_dict() for entry in other.entries or []] + if not issubset(proposed_entries, current_entries, allow_superset=False): + return True + return super().merge_would_change(other) @classmethod def validate_config_for_state(cls, config: list[dict[str, Any]], state: str) -> None: diff --git a/plugins/module_utils/nd_config_collection.py b/plugins/module_utils/nd_config_collection.py index 33a3c944f..641aa63cc 100644 --- a/plugins/module_utils/nd_config_collection.py +++ b/plugins/module_utils/nd_config_collection.py @@ -2,7 +2,7 @@ # GNU General Public License v3.0+ (see LICENSE or https://www.gnu.org/licenses/gpl-3.0.txt) -from __future__ import absolute_import, division, print_function +from __future__ import absolute_import, annotations, division, print_function from copy import deepcopy from typing import Any, Dict, List, Literal, Optional @@ -150,7 +150,7 @@ def delete_many(self, keys: List[IdentifierKey]) -> List[IdentifierKey]: # Diff Operations - def get_diff_config(self, new_item: NDBaseModel, exclude_unset: bool = False) -> Literal["new", "no_diff", "changed"]: + def get_diff_config(self, new_item: NDBaseModel, exclude_unset: bool = False, allow_superset: bool = False) -> Literal["new", "no_diff", "changed"]: """ Compare single item against collection. @@ -159,6 +159,14 @@ def get_diff_config(self, new_item: NDBaseModel, exclude_unset: bool = False) -> exclude_unset: When True, only compare fields explicitly set in ``new_item``. Useful for merge operations where unspecified fields should not trigger a diff. + allow_superset: When True, list elements are matched + one-directionally so an existing item carrying extra list + elements (or extra dict keys) is not flagged as changed. + + Both ``exclude_unset`` and ``allow_superset`` are forwarded to the + concrete model's ``get_diff``. Any model that overrides ``get_diff`` + must accept these keywords; see the subclass contract on + ``NDBaseModel.get_diff``. """ try: key = self._extract_key(new_item) @@ -170,7 +178,11 @@ def get_diff_config(self, new_item: NDBaseModel, exclude_unset: bool = False) -> if existing is None: return "new" - is_subset = existing.get_diff(new_item, exclude_unset=exclude_unset) + # ``get_diff`` is dispatched to the concrete model, so every override + # must accept ``exclude_unset`` and ``allow_superset`` (see the subclass + # contract on ``NDBaseModel.get_diff``). A non-conforming override raises + # TypeError here by design; do not catch it or inspect the signature. + is_subset = existing.get_diff(new_item, exclude_unset=exclude_unset, allow_superset=allow_superset) return "no_diff" if is_subset else "changed" @@ -240,11 +252,16 @@ def to_payload_list(self, **kwargs) -> List[Dict[str, Any]]: return [item.to_payload(**kwargs) for item in self._items] @staticmethod - def from_ansible_config(data: List[Dict], model_class: type[NDBaseModel], **kwargs) -> "NDConfigCollection": + def from_ansible_config(data: list[dict] | None, model_class: type[NDBaseModel], **kwargs) -> "NDConfigCollection": """ Create collection from Ansible config. + + ``data`` may be ``None`` for callers that intentionally treat absent + config as an empty collection. Callers whose state semantics distinguish + omitted/null config from an explicit empty list must validate that + before calling this helper. """ - items = [model_class.from_config(item_data, **kwargs) for item_data in data] + items = [model_class.from_config(item_data, **kwargs) for item_data in (data or [])] return NDConfigCollection(model_class=model_class, items=items) @staticmethod diff --git a/plugins/module_utils/nd_state_machine.py b/plugins/module_utils/nd_state_machine.py index e9adfc59e..2f585f7c5 100644 --- a/plugins/module_utils/nd_state_machine.py +++ b/plugins/module_utils/nd_state_machine.py @@ -25,6 +25,21 @@ class NDStateMachine: Generic State Machine for Nexus Dashboard (Bulk Support). """ + WRITE_STATES_REQUIRING_CONFIG = frozenset({"merged", "replaced", "overridden"}) + + @classmethod + def validate_config_presence(cls, state: str, config: Any) -> None: + """ + Reject omitted or null config before production wrappers normalize it. + + An explicit empty list remains valid because it can represent an + intentional empty desired set. + """ + if state in cls.WRITE_STATES_REQUIRING_CONFIG and config is None: + raise NDStateMachineError( + f"config must be provided and cannot be null for state '{state}'. " "Use config: [] only when intentionally managing an explicit empty set." + ) + def __init__(self, module: AnsibleModule, model_orchestrator: type[NDBaseOrchestrator] | NDBaseOrchestrator): """ Initialize the ND State Machine. @@ -58,6 +73,8 @@ def __init__(self, module: AnsibleModule, model_orchestrator: type[NDBaseOrchest self.model_class = self.model_orchestrator.model_class self.state = self.module.params["state"] + raw_config = self.module.params.get("config") + self.validate_config_presence(self.state, raw_config) # Cached flags self.check_mode = self.module.check_mode @@ -79,7 +96,9 @@ def __init__(self, module: AnsibleModule, model_orchestrator: type[NDBaseOrchest # state-aware validation (e.g. require certain fields for write states while accepting # identifier-only items for ``deleted``). Models that do not read the context ignore it. self.proposed = NDConfigCollection.from_ansible_config( - data=self.module.params.get("config", []), model_class=self.model_class, context={"state": self.state} + data=raw_config, + model_class=self.model_class, + context={"state": self.state}, ) self.output.assign(after=self.existing, before=self.before, proposed=self.proposed) @@ -140,17 +159,27 @@ def _execute_operation( *args: Any, error_msg_prefix: str = "Operation failed", **kwargs: Any, - ) -> ResponseType | None: - """Execute an API operation with standardized error handling.""" + ) -> bool: + """Execute an API operation with standardized error handling. + + Returns True if the operation completed without raising. In check mode + the API call is skipped but still reported as completed (True) so that + the previewed 'after' state can be built. Returns False only when the + operation raised and the error was suppressed via ``ignore_errors``. + + The orchestrator's return value is deliberately not used as the success + signal: some orchestrators defer the actual API call (queue-and-deploy) + and legitimately return None on success. + """ try: if not self.check_mode: - return operation(*args, **kwargs) - return None + operation(*args, **kwargs) + return True except Exception as e: error_msg = f"{error_msg_prefix}: {e}" if not self.ignore_errors: raise NDStateMachineError(error_msg) from e - return None + return False def _manage_create_update_state(self) -> None: """ @@ -167,9 +196,11 @@ def _manage_create_update_state(self) -> None: # Determine diff status # For merged state, only compare fields explicitly provided by # the user so that Pydantic default values do not trigger false - # diffs or overwrite existing configuration. + # diffs or overwrite existing configuration. Merged also matches + # list elements one-directionally (allow_superset) so an + # existing item with extra list entries is not seen as changed. exclude_unset = self.state == "merged" - diff_status = self.existing.get_diff_config(proposed_item, exclude_unset=exclude_unset) + diff_status = self.existing.get_diff_config(proposed_item, exclude_unset=exclude_unset, allow_superset=exclude_unset) # No changes needed if diff_status == "no_diff": @@ -204,20 +235,29 @@ def _manage_create_update_state(self) -> None: # The policy-required-on-create guard (issue #350) runs in manage_state, before the capability # preflight and before this method mutates self.existing (PR #362 review). - # Execute updates (always individual) + # Execute updates (always individual). An operation that does not fail + # is counted as sent; check mode skips the API call but returns True. + successfully_sent: list[NDBaseModel] = [] for item in items_to_update: - self._execute_operation(self.model_orchestrator.update, item, error_msg_prefix=f"Failed to update {item.get_identifier_value()}") + if self._execute_operation(self.model_orchestrator.update, item, error_msg_prefix=f"Failed to update {item.get_identifier_value()}"): + successfully_sent.append(item) # Execute creates (bulk or individual) if items_to_create: if self.supports_bulk_create: - self._execute_operation(self.model_orchestrator.create_bulk, items_to_create, error_msg_prefix="Failed to create in bulk") + if self._execute_operation(self.model_orchestrator.create_bulk, items_to_create, error_msg_prefix="Failed to create in bulk"): + successfully_sent.extend(items_to_create) else: for item in items_to_create: - self._execute_operation(self.model_orchestrator.create, item, error_msg_prefix=f"Failed to create {item.get_identifier_value()}") - - # Mark as sent only after successful API operations - successfully_sent = items_to_update + items_to_create + if self._execute_operation(self.model_orchestrator.create, item, error_msg_prefix=f"Failed to create {item.get_identifier_value()}"): + successfully_sent.append(item) + + # Mark successfully-processed items as sent. This stays populated in + # check mode (PR #225) so downstream config-save/deploy consumers that + # gate on len(sent) > 0 can still preview what a real run would send; + # execute_config_actions() is itself check-mode-safe (it simulates + # rather than sends). Per-item gating keeps items whose operation raised + # under ignore_errors out of 'sent'. if successfully_sent: self.sent.add_many(successfully_sent) @@ -244,16 +284,28 @@ def _delete_items(self, items: list[NDBaseModel]) -> None: if not items: return - # Execute deletes (bulk or individual) + # Execute deletes (bulk or individual). An item is counted as deleted + # when the operation does not fail; check mode skips the API call but + # returns True so the deletion is still previewed. Items whose delete + # failed under ignore_errors are left in 'existing' so the reported + # 'after' state stays accurate. + deleted: list[NDBaseModel] = [] if self.supports_bulk_delete: - self._execute_operation(self.model_orchestrator.delete_bulk, items, error_msg_prefix="Failed to delete in bulk") + if self._execute_operation(self.model_orchestrator.delete_bulk, items, error_msg_prefix="Failed to delete in bulk"): + deleted.extend(items) else: for item in items: - self._execute_operation(self.model_orchestrator.delete, item, error_msg_prefix=f"Failed to delete {item.get_identifier_value()}") + if self._execute_operation(self.model_orchestrator.delete, item, error_msg_prefix=f"Failed to delete {item.get_identifier_value()}"): + deleted.append(item) + + # Batch remove from collection (single index rebuild). + self.existing.delete_many([item.get_identifier_value() for item in deleted]) - # Batch remove from collection (single index rebuild) - keys_to_delete = [item.get_identifier_value() for item in items] - self.existing.delete_many(keys_to_delete) + # Mark successfully-deleted items as sent. Stays populated in check mode + # (PR #225) so downstream config-save/deploy previews are not skipped; + # per-item gating keeps failed deletes (under ignore_errors) out of 'sent'. + if deleted: + self.sent.add_many(deleted) # Log deletion self.output.assign(after=self.existing) diff --git a/plugins/module_utils/orchestrators/network_workflow_coordinator.py b/plugins/module_utils/orchestrators/network_workflow_coordinator.py index d02fa372e..108477283 100644 --- a/plugins/module_utils/orchestrators/network_workflow_coordinator.py +++ b/plugins/module_utils/orchestrators/network_workflow_coordinator.py @@ -109,6 +109,10 @@ def run(self) -> dict[str, Any]: Returns a result dict suitable for module.exit_json(**result). """ module_args: dict = dict(self.module.params) + NDStateMachine.validate_config_presence( + module_args.get("state", "merged"), + module_args.get("config"), + ) if self.strategy is None: self.strategy = self._resolve_strategy(module_args) self._trace( diff --git a/plugins/module_utils/utils.py b/plugins/module_utils/utils.py index f1ef26c08..a806d94c3 100644 --- a/plugins/module_utils/utils.py +++ b/plugins/module_utils/utils.py @@ -41,25 +41,93 @@ def sanitize_dict(dict_to_sanitize, keys=None, values=None, recursive=True, remo return result -def issubset(subset: Any, superset: Any) -> bool: - """Check if subset is contained in superset.""" +def _has_perfect_matching(adjacency: list[list[int]]) -> bool: + """Return True if every subset item can be matched to a distinct candidate. + + ``adjacency[i]`` holds the indices of the candidates that subset item ``i`` + can match. This solves the maximum bipartite matching problem with Kuhn's + augmenting-path algorithm so that a less-specific item never greedily + consumes a candidate that a more-specific item needs. + """ + # candidate index -> subset item index it is currently assigned to + match_to_item: dict[int, int] = {} + + def _try_assign(item_index: int, visited: set[int]) -> bool: + for candidate_index in adjacency[item_index]: + if candidate_index in visited: + continue + visited.add(candidate_index) + assigned_item = match_to_item.get(candidate_index) + # Candidate is free, or its current owner can be reassigned elsewhere. + if assigned_item is None or _try_assign(assigned_item, visited): + match_to_item[candidate_index] = item_index + return True + return False + + for item_index in range(len(adjacency)): + if not _try_assign(item_index, set()): + return False + return True + + +def issubset(subset: Any, superset: Any, allow_superset: bool = False) -> bool: + """Check if subset is contained in superset. + + For dicts, only the non-``None`` keys of ``subset`` are compared; keys whose + value is ``None`` are ignored, and keys present only in ``superset`` are + allowed. For lists, every ``subset`` element must pair with a distinct + ``superset`` element (matching is order-independent). By default the two + lists must be the same length; when ``allow_superset`` is True the + ``subset`` list may be shorter so that extra existing elements are + tolerated (``len(subset) <= len(superset)``). + + Args: + subset: The value to check. + superset: The value to check against. + allow_superset: When True, list matching is one-directional: an element + in ``subset`` is considered matched when it is a subset of a + candidate in ``superset``, even if the candidate has additional + keys, and ``superset`` may contain extra elements that ``subset`` + does not (``len(subset) <= len(superset)``). When False (default) + matching is bidirectional and the lengths must be equal. For lists + of dicts the default is equivalent to equality *after* ``None`` + -valued keys are dropped from both sides (it is not strict ``==`` + equality, because such keys are ignored). + """ if type(subset) is not type(superset): return False if not isinstance(subset, dict): if isinstance(subset, list): - if len(subset) != len(superset): + # Under allow_superset the proposed list only needs to map into the + # existing one, so extra existing elements are tolerated + # (len(subset) <= len(superset)). Otherwise matching is + # bidirectional and the lengths must be identical. + if allow_superset: + if len(subset) > len(superset): + return False + elif len(subset) != len(superset): return False - remaining = list(superset) + # Build the bipartite adjacency: for each subset item, which + # candidates it can match. A full matching is then required so a + # less-specific item cannot greedily consume a candidate that a + # more-specific item needs (relevant under allow_superset=True). + adjacency: list[list[int]] = [] for item in subset: - for index, candidate in enumerate(remaining): - if issubset(item, candidate) and issubset(candidate, item): - del remaining[index] - break - else: + matches = [] + for index, candidate in enumerate(superset): + if allow_superset: + match = issubset(item, candidate, allow_superset=True) + else: + match = issubset(item, candidate) and issubset(candidate, item) + if match: + matches.append(index) + if not matches: return False - return True + adjacency.append(matches) + + return _has_perfect_matching(adjacency) return subset == superset for key, value in subset.items(): @@ -69,7 +137,7 @@ def issubset(subset: Any, superset: Any) -> bool: if key not in superset: return False - if not issubset(value, superset[key]): + if not issubset(value, superset[key], allow_superset=allow_superset): return False return True diff --git a/plugins/modules/nd_manage_networks.py b/plugins/modules/nd_manage_networks.py index a8dee3231..5feadb86c 100644 --- a/plugins/modules/nd_manage_networks.py +++ b/plugins/modules/nd_manage_networks.py @@ -30,9 +30,10 @@ config: description: - List of Network definitions to manage. + - Required and must not be null for states V(merged), V(replaced), and V(overridden). + - With state V(overridden), an explicit empty list removes all managed Networks in scope. type: list elements: dict - default: [] suboptions: network_name: description: Name of the Network. @@ -586,7 +587,6 @@ def main(): type="list", elements="dict", required=False, - default=[], options=network_parent_argument_spec(), ), ) diff --git a/plugins/modules/nd_manage_vpc_pair.py b/plugins/modules/nd_manage_vpc_pair.py index 04f6a7946..72fbd8cd0 100644 --- a/plugins/modules/nd_manage_vpc_pair.py +++ b/plugins/modules/nd_manage_vpc_pair.py @@ -92,6 +92,8 @@ config: description: - List of vPC pair configuration dictionaries. + - Required and must not be null for states V(merged), V(replaced), and V(overridden). + - With state V(overridden), an explicit empty list removes all managed vPC pairs in scope. type: list elements: dict suboptions: @@ -393,7 +395,13 @@ from ansible_collections.cisco.nd.plugins.module_utils.common.pydantic_compat import ( ValidationError, ) +from ansible_collections.cisco.nd.plugins.module_utils.common.exceptions import ( + NDStateMachineError, +) from ansible_collections.cisco.nd.plugins.module_utils.nd import nd_argument_spec +from ansible_collections.cisco.nd.plugins.module_utils.nd_state_machine import ( + NDStateMachine, +) # Service layer imports from ansible_collections.cisco.nd.plugins.module_utils.manage_vpc_pair.resources import ( @@ -500,6 +508,11 @@ def main() -> None: # State-specific parameter validations state = module_config.state + try: + NDStateMachine.validate_config_presence(state, module_config.config) + except NDStateMachineError as e: + module.fail_json(msg=str(e)) + config_actions = get_config_actions(module) verify_settings = get_verify_settings(module) raw_module_args = _get_raw_module_args() @@ -547,7 +560,7 @@ def main() -> None: module.warn("Parameter 'force' only applies to state 'deleted'. " f"Ignoring force for state '{state}'.") # Normalize config keys for runtime/state-machine model handling. - normalized_config = [item.to_runtime_config() for item in (module_config.config or [])] + normalized_config = [] if module_config.config is None else [item.to_runtime_config() for item in module_config.config] module.params["config"] = normalized_config diff --git a/tests/unit/module_utils/models/test_maintenance_mode_model.py b/tests/unit/module_utils/models/test_maintenance_mode_model.py index 7f822f903..6f02e96d9 100644 --- a/tests/unit/module_utils/models/test_maintenance_mode_model.py +++ b/tests/unit/module_utils/models/test_maintenance_mode_model.py @@ -234,6 +234,32 @@ def test_maintenance_mode_model_00070() -> None: assert snapshot.get_diff(proposed) is True +def test_maintenance_mode_model_00075() -> None: + """ + # Summary + + Verify the custom `get_diff` accepts NDConfigCollection's shared diff + keyword arguments. + + ## Classes and Methods + + - MaintenanceModeModel.get_diff + """ + snapshot = MaintenanceModeModel.from_response( + { + "switches": [{"switch_ip": "192.168.12.131"}], + "switch_modes": {"192.168.12.131": "maintenance"}, + } + ) + proposed = MaintenanceModeModel.from_config( + { + "mode": "maintenance", + "switches": [{"switch_ip": "192.168.12.131"}], + } + ) + assert snapshot.get_diff(proposed, exclude_unset=True, allow_superset=True) is True + + def test_maintenance_mode_model_00080() -> None: """ # Summary diff --git a/tests/unit/module_utils/models/test_manage_prefix_list.py b/tests/unit/module_utils/models/test_manage_prefix_list.py index 8fbf2b0a8..3126e4517 100644 --- a/tests/unit/module_utils/models/test_manage_prefix_list.py +++ b/tests/unit/module_utils/models/test_manage_prefix_list.py @@ -23,6 +23,7 @@ PrefixListEntryModel, PrefixListModel, ) +from ansible_collections.cisco.nd.plugins.module_utils.nd_config_collection import NDConfigCollection from ansible_collections.cisco.nd.tests.unit.module_utils.common_utils import does_not_raise SAMPLE_IPV4_CONFIG = { @@ -773,3 +774,101 @@ def test_manage_prefix_list_00210() -> None: bad_config["name"] = "P" * 107 with pytest.raises(ValidationError, match="combined tenant-qualified"): PrefixListModel.from_config(bad_config) + + +# ============================================================================= +# Test: NDConfigCollection contract (regression) +# ============================================================================= + + +def test_manage_prefix_list_00220() -> None: + """ + # Summary + + Reconcile the real ``PrefixListModel`` through ``NDConfigCollection`` to lock + in the shared ``get_diff`` signature contract. + + ``NDConfigCollection.get_diff_config`` always forwards ``allow_superset`` to + the concrete model's ``get_diff``. Before ``PrefixListModel.get_diff`` adopted + the shared signature this raised + ``TypeError: get_diff() got an unexpected keyword argument 'allow_superset'`` + whenever an existing prefix list was compared, breaking idempotency and + replace/override checks. This drives the real production model (not a fake + override) through the collection so the incompatibility cannot regress. + + ## Classes and Methods + + - PrefixListModel.get_diff + - NDConfigCollection.get_diff_config + """ + existing = PrefixListModel.from_response(copy.deepcopy(SAMPLE_IPV4_API_RESPONSE)) + collection = NDConfigCollection(model_class=PrefixListModel, items=[existing]) + + # Idempotency check on an existing prefix list must not raise and is "no_diff". + idempotent = PrefixListModel.from_config( + { + "ip_version": "ipv4", + "tenant_name": "TENANT1", + "name": "PL-IPV4-BORDERS", + "entries": copy.deepcopy(SAMPLE_IPV4_CONFIG["entries"]), + } + ) + assert collection.get_diff_config(idempotent, exclude_unset=True, allow_superset=True) == "no_diff" + + # Prefix-list entries are authoritative even for merged. The shared state + # machine requests allow_superset=True for merged resources, but this model + # must still detect an existing entry omitted from the desired list. + missing_existing_entry = PrefixListModel.from_config( + { + "ip_version": "ipv4", + "tenant_name": "TENANT1", + "name": "PL-IPV4-BORDERS", + "entries": copy.deepcopy(SAMPLE_IPV4_CONFIG["entries"][:1]), + } + ) + assert collection.get_diff_config(missing_existing_entry, exclude_unset=True, allow_superset=True) == "changed" + + # Default-expanded equality remains idempotent: omitting action means the + # documented default "permit", not a request to change an existing permit. + default_action_config = copy.deepcopy(SAMPLE_IPV4_CONFIG) + default_action_config["entries"][0].pop("action") + default_action = PrefixListModel.from_config(default_action_config) + assert collection.get_diff_config(default_action, exclude_unset=True, allow_superset=True) == "no_diff" + + # Entry order is not semantic; sequence numbers identify the entries. + reordered_config = copy.deepcopy(SAMPLE_IPV4_CONFIG) + reordered_config["entries"].reverse() + reordered = PrefixListModel.from_config(reordered_config) + assert collection.get_diff_config(reordered, exclude_unset=True, allow_superset=True) == "no_diff" + + # Conversely, omitting a controller-side length constraint from the + # authoritative desired entry requests its removal. + constrained_response = copy.deepcopy(SAMPLE_IPV4_API_RESPONSE) + constrained_response["entries"][0]["exactLength"] = 24 + constrained_existing = PrefixListModel.from_response(constrained_response) + constrained_collection = NDConfigCollection(model_class=PrefixListModel, items=[constrained_existing]) + clear_constraint = PrefixListModel.from_config(copy.deepcopy(SAMPLE_IPV4_CONFIG)) + assert constrained_collection.get_diff_config(clear_constraint, exclude_unset=True, allow_superset=True) == "changed" + + # Replace-style comparison of the same prefix list with a stale controller + # description (config omits description) reaches the override and is "changed". + stale_description = PrefixListModel.from_config( + { + "ip_version": "ipv4", + "tenant_name": "TENANT1", + "name": "PL-IPV4-BORDERS", + "entries": copy.deepcopy(SAMPLE_IPV4_CONFIG["entries"]), + } + ) + assert collection.get_diff_config(stale_description, exclude_unset=False, allow_superset=True) == "changed" + + # A different composite key short-circuits to "new" before get_diff runs. + different_name = PrefixListModel.from_config( + { + "ip_version": "ipv4", + "tenant_name": "TENANT1", + "name": "PL-IPV4-OTHER", + "entries": copy.deepcopy(SAMPLE_IPV4_CONFIG["entries"]), + } + ) + assert collection.get_diff_config(different_name, allow_superset=True) == "new" diff --git a/tests/unit/module_utils/orchestrators/test_networks.py b/tests/unit/module_utils/orchestrators/test_networks.py index 17215984b..8ac8de70a 100644 --- a/tests/unit/module_utils/orchestrators/test_networks.py +++ b/tests/unit/module_utils/orchestrators/test_networks.py @@ -8,6 +8,7 @@ from unittest.mock import patch +from ansible_collections.cisco.nd.plugins.module_utils.common.exceptions import NDStateMachineError from ansible_collections.cisco.nd.plugins.module_utils.enums import HttpVerbEnum, OperationType from ansible_collections.cisco.nd.plugins.module_utils.models.manage_networks.config_models import ( NetworkConfigModel, @@ -57,6 +58,9 @@ def fail_json(self, **kwargs): raise AssertionError(kwargs) +_MISSING_CONFIG = object() + + class _ParentStrategy: config_model_cls = NetworkParentConfigModel fabric_data = {"members": [{"fabricName": "child1"}]} @@ -127,6 +131,50 @@ def resolve(self): assert coordinator.workflow_trace[0]["event"] == "fabric_resolver_start" +@pytest.mark.parametrize("check_mode", [False, True]) +@pytest.mark.parametrize( + "config", + [ + pytest.param(_MISSING_CONFIG, id="omitted"), + pytest.param(None, id="null"), + ], +) +def test_network_workflow_rejects_missing_config_before_resolution(config, check_mode): + """Production Network workflows must not normalize missing config to [].""" + params = {"fabric_name": "fab1", "state": "overridden"} + if config is not _MISSING_CONFIG: + params["config"] = config + module = _Module(params) + module.check_mode = check_mode + coordinator = NetworkWorkflowCoordinator(module=module) + + with patch.object(coordinator, "_resolve_strategy") as resolve_strategy: + with pytest.raises(NDStateMachineError, match=r"config must be provided and cannot be null"): + coordinator.run() + + resolve_strategy.assert_not_called() + + +@pytest.mark.parametrize("check_mode", [False, True]) +def test_network_workflow_accepts_explicit_empty_config(check_mode): + """An explicit empty desired set remains valid through the Network wrapper.""" + module = _Module({"fabric_name": "fab1", "state": "overridden", "config": []}) + module.check_mode = check_mode + strategy = StandaloneNetworkStrategy( + fabric_name="fab1", + fabric_data={"managementType": "vxlanIbgp"}, + ) + coordinator = NetworkWorkflowCoordinator(module=module, strategy=strategy) + expected = {"changed": check_mode} + + with patch.object(coordinator, "_handle_standalone_workflow", return_value=expected) as handler: + result = coordinator.run() + + handler.assert_called_once() + assert handler.call_args.args[0]["config"] == [] + assert result["changed"] is check_mode + + def _mcfg_parent_orchestrator(): strategy = MulticlusterParentNetworkStrategy( fabric_name="MCFG_FAB", diff --git a/tests/unit/module_utils/test_nd_config_collection.py b/tests/unit/module_utils/test_nd_config_collection.py new file mode 100644 index 000000000..355e913a0 --- /dev/null +++ b/tests/unit/module_utils/test_nd_config_collection.py @@ -0,0 +1,129 @@ +# -*- coding: utf-8 -*- + +# Copyright: (c) 2026, Cisco Systems, Inc. + +# GNU General Public License v3.0+ (see LICENSE or https://www.gnu.org/licenses/gpl-3.0.txt) + +""" +Unit tests for ``NDConfigCollection.get_diff_config`` dispatch behavior. + +``get_diff_config`` forwards ``exclude_unset`` and ``allow_superset`` to the +concrete model's ``get_diff``. Because Python dispatches ``get_diff`` to the +subclass override (not to ``NDBaseModel.get_diff``), every override must accept +both keywords. These tests exercise that contract *through* ``NDConfigCollection`` +rather than by calling ``get_diff`` directly, so a subclass that predates the +shared signature is caught here: + +- a conforming override receives ``allow_superset`` unchanged, and its return + value flows back out as ``no_diff`` / ``changed``. +- ``allow_superset`` defaults to ``False`` when the caller omits it. +- a non-conforming override (old ``get_diff(self, other, exclude_unset=False)`` + signature) surfaces the incompatibility as a loud ``TypeError`` -- the failure + is intentional and is not hidden by the collection. +""" + +# pylint: disable=protected-access + +from __future__ import absolute_import, annotations, division, print_function + +# pylint: disable=invalid-name +__metaclass__ = type +# pylint: enable=invalid-name + +from typing import ClassVar, List, Literal, Optional + +import pytest +from ansible_collections.cisco.nd.plugins.module_utils.models.base import NDBaseModel +from ansible_collections.cisco.nd.plugins.module_utils.nd_config_collection import NDConfigCollection + +# ============================================================================= +# Test doubles +# ============================================================================= + + +class _ConformingModel(NDBaseModel): + """Override that conforms to the shared ``get_diff`` contract. + + It accepts ``allow_superset`` and lets the forwarded value drive the result, + so a test can prove the keyword was dispatched through ``NDConfigCollection``: + ``True`` -> "no diff" (subset), ``False`` -> "changed". + """ + + identifiers: ClassVar[Optional[List[str]]] = ["name"] + identifier_strategy: ClassVar[Literal["single", "composite", "hierarchical", "singleton"]] = "single" + + name: str + value: Optional[str] = None + + def get_diff(self, other: "NDBaseModel", exclude_unset: bool = False, allow_superset: bool = False) -> bool: + return bool(allow_superset) + + +class _NonConformingModel(NDBaseModel): + """Override predating the contract: it omits ``allow_superset`` by design. + + Mirrors the ``get_diff`` overrides on in-flight PRs #286/#312 that have not + yet adopted the shared signature. Forwarding ``allow_superset`` to it must + raise ``TypeError`` rather than be silently swallowed. + """ + + identifiers: ClassVar[Optional[List[str]]] = ["name"] + identifier_strategy: ClassVar[Literal["single", "composite", "hierarchical", "singleton"]] = "single" + + name: str + value: Optional[str] = None + + def get_diff(self, other: "NDBaseModel", exclude_unset: bool = False) -> bool: # noqa: type checked at runtime + return True + + +# ============================================================================= +# Tests +# ============================================================================= + + +def test_get_diff_config_forwards_allow_superset_to_override(): + """``allow_superset`` is dispatched to the concrete override unchanged. + + The conforming override returns the forwarded value, so ``True`` collapses to + "no_diff" and ``False`` to "changed" -- both proving the keyword reached the + subclass through ``NDConfigCollection.get_diff_config``. + """ + existing = _ConformingModel(name="a") + collection = NDConfigCollection(model_class=_ConformingModel, items=[existing]) + proposed = _ConformingModel(name="a") + + assert collection.get_diff_config(proposed, exclude_unset=True, allow_superset=True) == "no_diff" + assert collection.get_diff_config(proposed, exclude_unset=True, allow_superset=False) == "changed" + + +def test_get_diff_config_defaults_allow_superset_to_false(): + """Omitting ``allow_superset`` forwards ``False`` to the override.""" + existing = _ConformingModel(name="a") + collection = NDConfigCollection(model_class=_ConformingModel, items=[existing]) + proposed = _ConformingModel(name="a") + + assert collection.get_diff_config(proposed) == "changed" + + +def test_get_diff_config_returns_new_when_item_absent(): + """An item with no existing match short-circuits to "new" before ``get_diff``.""" + collection = NDConfigCollection(model_class=_ConformingModel, items=[_ConformingModel(name="a")]) + proposed = _ConformingModel(name="b") + + assert collection.get_diff_config(proposed, allow_superset=True) == "new" + + +def test_get_diff_config_nonconforming_override_raises_typeerror(): + """A non-conforming override surfaces the incompatibility loudly. + + ``get_diff_config`` forwards ``allow_superset``; the old-signature override + rejects it and the ``TypeError`` propagates. The collection must not hide the + mismatch by catching ``TypeError`` or inspecting the signature. + """ + existing = _NonConformingModel(name="a") + collection = NDConfigCollection(model_class=_NonConformingModel, items=[existing]) + proposed = _NonConformingModel(name="a") + + with pytest.raises(TypeError, match="allow_superset"): + collection.get_diff_config(proposed, exclude_unset=True, allow_superset=True) diff --git a/tests/unit/module_utils/test_nd_state_machine_operations.py b/tests/unit/module_utils/test_nd_state_machine_operations.py new file mode 100644 index 000000000..dc0d4ab7b --- /dev/null +++ b/tests/unit/module_utils/test_nd_state_machine_operations.py @@ -0,0 +1,586 @@ +# -*- coding: utf-8 -*- + +# Copyright: (c) 2026, Cisco Systems, Inc. + +# GNU General Public License v3.0+ (see LICENSE or https://www.gnu.org/licenses/gpl-3.0.txt) + +""" +Unit tests for ``NDStateMachine`` operation wiring. + +These cover the state-machine behavior that the pure ``issubset``/``get_diff`` +tests in ``test_utils.py`` cannot reach: + +- ``_execute_operation`` returns a boolean success signal and skips the API call + in check mode. +- creates/updates/deletes are added to ``sent`` after a successful operation + (per-item gating); ``sent`` stays populated in check mode so downstream + config-save/deploy previews are not skipped (PR #225). +- under ``ignore_errors`` a failed delete leaves the item in ``existing`` and out + of ``sent``; without it the failure is raised as ``NDStateMachineError``. +- bulk-delete failure keeps every item, while bulk-delete success removes only + the targeted items. +- ``NDStateMachine`` rejects omitted/null config for mutating write states so + ``state: overridden`` cannot accidentally delete everything. + +A tiny real ``NDBaseModel`` subclass drives the genuine diff/merge logic, while a +duck-typed fake orchestrator records the CRUD calls it receives and can simulate +failures. The heavy ``NDStateMachine.__init__`` (RestSend/Sender/endpoints) is +bypassed with ``object.__new__`` so the tests stay focused on the state logic. +""" + +# pylint: disable=protected-access + +from __future__ import absolute_import, annotations, division, print_function + +# pylint: disable=invalid-name +__metaclass__ = type +# pylint: enable=invalid-name + +from typing import Any, ClassVar, List, Literal, Optional + +import pytest +from ansible_collections.cisco.nd.plugins.module_utils.common.exceptions import NDStateMachineError +from ansible_collections.cisco.nd.plugins.module_utils.models.base import NDBaseModel +from ansible_collections.cisco.nd.plugins.module_utils.nd_config_collection import NDConfigCollection +from ansible_collections.cisco.nd.plugins.module_utils.nd_state_machine import NDStateMachine +from ansible_collections.cisco.nd.plugins.module_utils.orchestrators.base import NDBaseOrchestrator + +# ============================================================================= +# Test doubles +# ============================================================================= + + +class _FakeModel(NDBaseModel): + """Minimal single-identifier model so the real diff/merge logic runs.""" + + identifiers: ClassVar[Optional[List[str]]] = ["name"] + identifier_strategy: ClassVar[Literal["single", "composite", "hierarchical", "singleton"]] = "single" + + name: str + value: Optional[str] = None + + +class _FakeOutput: + """No-op stand-in for ``NDOutput``; the state machine only calls ``assign``.""" + + def assign(self, **kwargs: Any) -> None: + return None + + +class _FakeOrchestrator: + """Duck-typed orchestrator that records CRUD calls and can simulate failures. + + ``fail_ops`` is a set of operation names ("create", "update", "delete", + "create_bulk", "delete_bulk"); a listed operation records the attempt and + then raises, mirroring an orchestrator whose API call failed. + """ + + def __init__(self, supports_bulk_create: bool = False, supports_bulk_delete: bool = False, fail_ops: Optional[set] = None) -> None: + self.model_class = _FakeModel + self.supports_bulk_create = supports_bulk_create + self.supports_bulk_delete = supports_bulk_delete + self.results = None + self.fail_ops = set(fail_ops or set()) + self.calls: dict = { + "create": [], + "update": [], + "delete": [], + "create_bulk": [], + "delete_bulk": [], + "preflight_create": [], + "preflight": [], + } + + def preflight_create(self, model_instances) -> None: + self.calls["preflight_create"].append(list(model_instances)) + + def preflight(self, model_instances) -> None: + self.calls["preflight"].append(list(model_instances)) + + def query_all(self, model_instance=None, **kwargs): + return [] + + def create(self, model_instance, **kwargs): + self.calls["create"].append(model_instance) + if "create" in self.fail_ops: + raise Exception("create failed") + return {} + + def create_bulk(self, model_instances, **kwargs): + self.calls["create_bulk"].append(list(model_instances)) + if "create_bulk" in self.fail_ops: + raise Exception("create_bulk failed") + return {} + + def update(self, model_instance, **kwargs): + self.calls["update"].append(model_instance) + if "update" in self.fail_ops: + raise Exception("update failed") + return {} + + def delete(self, model_instance, **kwargs): + self.calls["delete"].append(model_instance) + if "delete" in self.fail_ops: + raise Exception("delete failed") + return None + + def delete_bulk(self, model_instances, **kwargs): + self.calls["delete_bulk"].append(list(model_instances)) + if "delete_bulk" in self.fail_ops: + raise Exception("delete_bulk failed") + return None + + +_MISSING = object() + + +class _FakeModule: + """Minimal module object for exercising ``NDStateMachine.__init__``.""" + + def __init__(self, state: str = "merged", config: Any = _MISSING, check_mode: bool = False, ignore_errors: bool = False) -> None: + self.check_mode = check_mode + self.params: dict[str, Any] = { + "state": state, + "output_level": "normal", + "ignore_errors": ignore_errors, + } + if config is not _MISSING: + self.params["config"] = config + + +class _InitFakeOrchestrator(NDBaseOrchestrator): + """Minimal real orchestrator subclass for ``NDStateMachine.__init__`` tests.""" + + model_class: ClassVar[type[NDBaseModel]] = _FakeModel + create_endpoint: ClassVar[Any] = None + update_endpoint: ClassVar[Any] = None + delete_endpoint: ClassVar[Any] = None + query_one_endpoint: ClassVar[Any] = None + query_all_endpoint: ClassVar[Any] = None + + def query_all(self, model_instance=None, **kwargs): + return [] + + def create(self, model_instance, **kwargs): + return {} + + def update(self, model_instance, **kwargs): + return {} + + def delete(self, model_instance, **kwargs): + return None + + +def _model(name: str, value: Optional[str] = None) -> _FakeModel: + return _FakeModel(name=name, value=value) + + +def _make_state_machine( + state: str = "merged", + check_mode: bool = False, + ignore_errors: bool = False, + orchestrator: Optional[_FakeOrchestrator] = None, + existing: Optional[List[_FakeModel]] = None, + proposed: Optional[List[_FakeModel]] = None, +) -> NDStateMachine: + """Build an ``NDStateMachine`` wired to the fakes, bypassing ``__init__``.""" + if orchestrator is None: + orchestrator = _FakeOrchestrator() + + sm = object.__new__(NDStateMachine) + sm.state = state + sm.check_mode = check_mode + sm.ignore_errors = ignore_errors + sm.model_class = _FakeModel + sm.model_orchestrator = orchestrator + sm.supports_bulk_create = orchestrator.supports_bulk_create + sm.supports_bulk_delete = orchestrator.supports_bulk_delete + sm.output = _FakeOutput() + sm.before = NDConfigCollection(model_class=_FakeModel, items=list(existing or [])) + sm.existing = sm.before.copy() + sm.proposed = NDConfigCollection(model_class=_FakeModel, items=list(proposed or [])) + sm.sent = NDConfigCollection(model_class=_FakeModel) + return sm + + +def _names(items) -> List[str]: + return sorted(item.name for item in items) + + +# ============================================================================= +# manage_state dispatch +# ============================================================================= + + +def test_manage_state_invalid_state_raises(): + """An unknown state is rejected rather than silently ignored.""" + sm = _make_state_machine(state="bogus") + with pytest.raises(NDStateMachineError, match="Invalid state"): + sm.manage_state() + + +# ============================================================================= +# check mode: API is skipped but items are still marked sent (deploy preview) +# ============================================================================= + + +def test_check_mode_create_skips_api_but_marks_sent(): + """Check-mode create previews the new item and marks it sent (no API call).""" + orch = _FakeOrchestrator() + sm = _make_state_machine(state="merged", check_mode=True, orchestrator=orch, proposed=[_model("a", "x")]) + + sm.manage_state() + + assert orch.calls["create"] == [] # no API call in check mode + assert _names(sm.sent) == ["a"] # sent stays populated so deploy preview is not skipped + assert sm.existing.get("a") is not None # previewed 'after' still reflects it + + +def test_check_mode_update_skips_api_but_marks_sent(): + """Check-mode update previews the change and marks it sent (no API call).""" + orch = _FakeOrchestrator() + sm = _make_state_machine(state="replaced", check_mode=True, orchestrator=orch, existing=[_model("a", "x")], proposed=[_model("a", "y")]) + + sm.manage_state() + + assert orch.calls["update"] == [] + assert _names(sm.sent) == ["a"] + assert sm.existing.get("a").value == "y" # previewed change + + +def test_check_mode_delete_skips_api_but_marks_sent(): + """Check-mode delete previews the removal and marks it sent (no API call).""" + orch = _FakeOrchestrator() + sm = _make_state_machine(state="deleted", check_mode=True, orchestrator=orch, existing=[_model("a", "x")], proposed=[_model("a")]) + + sm.manage_state() + + assert orch.calls["delete"] == [] + assert _names(sm.sent) == ["a"] + assert len(sm.existing) == 0 # previewed removal + + +# ============================================================================= +# normal mode: successful operations are marked sent +# ============================================================================= + + +def test_create_individual_marks_sent(): + """A successful individual create is pushed and recorded in ``sent``.""" + orch = _FakeOrchestrator(supports_bulk_create=False) + sm = _make_state_machine(state="merged", orchestrator=orch, proposed=[_model("a", "x")]) + + sm.manage_state() + + assert _names(orch.calls["create"]) == ["a"] + assert orch.calls["create_bulk"] == [] + assert _names(sm.sent) == ["a"] + assert sm.existing.get("a") is not None + + +def test_create_bulk_marks_sent(): + """When bulk create is supported, creates go through ``create_bulk``.""" + orch = _FakeOrchestrator(supports_bulk_create=True) + sm = _make_state_machine(state="merged", orchestrator=orch, proposed=[_model("a", "x"), _model("b", "y")]) + + sm.manage_state() + + assert orch.calls["create"] == [] + assert len(orch.calls["create_bulk"]) == 1 + assert _names(orch.calls["create_bulk"][0]) == ["a", "b"] + assert _names(sm.sent) == ["a", "b"] + + +def test_update_marks_sent(): + """A successful update is pushed, recorded in ``sent``, and reflected in 'after'.""" + orch = _FakeOrchestrator() + sm = _make_state_machine(state="replaced", orchestrator=orch, existing=[_model("a", "x")], proposed=[_model("a", "y")]) + + sm.manage_state() + + assert _names(orch.calls["update"]) == ["a"] + assert _names(sm.sent) == ["a"] + assert sm.existing.get("a").value == "y" + + +def test_delete_individual_marks_sent(): + """A successful individual delete removes the item and records it in ``sent``.""" + orch = _FakeOrchestrator(supports_bulk_delete=False) + sm = _make_state_machine(state="deleted", orchestrator=orch, existing=[_model("a", "x")], proposed=[_model("a")]) + + sm.manage_state() + + assert _names(orch.calls["delete"]) == ["a"] + assert len(sm.existing) == 0 + assert _names(sm.sent) == ["a"] + + +def test_delete_bulk_marks_sent(): + """When bulk delete is supported, deletes go through ``delete_bulk``.""" + orch = _FakeOrchestrator(supports_bulk_delete=True) + sm = _make_state_machine(state="deleted", orchestrator=orch, existing=[_model("a", "x"), _model("b", "y")], proposed=[_model("a"), _model("b")]) + + sm.manage_state() + + assert orch.calls["delete"] == [] + assert len(orch.calls["delete_bulk"]) == 1 + assert _names(orch.calls["delete_bulk"][0]) == ["a", "b"] + assert _names(sm.sent) == ["a", "b"] + assert len(sm.existing) == 0 + + +# ============================================================================= +# merged diff handling +# ============================================================================= + + +def test_merged_no_diff_is_noop(): + """An item already matching its desired config triggers no operation.""" + orch = _FakeOrchestrator() + sm = _make_state_machine(state="merged", orchestrator=orch, existing=[_model("a", "x")], proposed=[_model("a", "x")]) + + sm.manage_state() + + assert orch.calls["create"] == [] + assert orch.calls["update"] == [] + assert len(sm.sent) == 0 + assert sm.existing.get("a").value == "x" + + +def test_merged_merges_changed_fields(): + """Merged state merges explicitly set fields and updates the item.""" + orch = _FakeOrchestrator() + sm = _make_state_machine(state="merged", orchestrator=orch, existing=[_model("a", "x")], proposed=[_model("a", "y")]) + + sm.manage_state() + + assert _names(orch.calls["update"]) == ["a"] + assert sm.existing.get("a").value == "y" + assert _names(sm.sent) == ["a"] + + +# ============================================================================= +# ignore_errors handling +# ============================================================================= + + +def test_delete_ignored_error_keeps_item_and_skips_sent(): + """An ignored delete failure leaves the item in 'after' and out of ``sent``.""" + orch = _FakeOrchestrator(supports_bulk_delete=False, fail_ops={"delete"}) + sm = _make_state_machine(state="deleted", ignore_errors=True, orchestrator=orch, existing=[_model("a", "x")], proposed=[_model("a")]) + + sm.manage_state() + + assert _names(orch.calls["delete"]) == ["a"] # the delete was attempted + assert sm.existing.get("a") is not None # but the item is kept + assert len(sm.sent) == 0 # and not marked sent + + +def test_delete_non_ignored_error_raises(): + """Without ignore_errors a failed delete surfaces as NDStateMachineError.""" + orch = _FakeOrchestrator(supports_bulk_delete=False, fail_ops={"delete"}) + sm = _make_state_machine(state="deleted", ignore_errors=False, orchestrator=orch, existing=[_model("a", "x")], proposed=[_model("a")]) + + with pytest.raises(NDStateMachineError, match="Failed to delete"): + sm.manage_state() + + +def test_bulk_delete_ignored_error_keeps_all_items(): + """An ignored bulk-delete failure keeps every targeted item.""" + orch = _FakeOrchestrator(supports_bulk_delete=True, fail_ops={"delete_bulk"}) + sm = _make_state_machine( + state="deleted", + ignore_errors=True, + orchestrator=orch, + existing=[_model("a", "x"), _model("b", "y")], + proposed=[_model("a"), _model("b")], + ) + + sm.manage_state() + + assert len(orch.calls["delete_bulk"]) == 1 # bulk delete attempted once + assert _names(sm.existing) == ["a", "b"] # nothing removed + assert len(sm.sent) == 0 # nothing marked sent + + +def test_bulk_delete_non_ignored_error_raises(): + """Without ignore_errors a failed bulk delete surfaces as NDStateMachineError.""" + orch = _FakeOrchestrator(supports_bulk_delete=True, fail_ops={"delete_bulk"}) + sm = _make_state_machine(state="deleted", ignore_errors=False, orchestrator=orch, existing=[_model("a", "x")], proposed=[_model("a")]) + + with pytest.raises(NDStateMachineError, match="Failed to delete in bulk"): + sm.manage_state() + + +def test_bulk_delete_success_removes_only_targeted(): + """A successful bulk delete removes only the proposed items.""" + orch = _FakeOrchestrator(supports_bulk_delete=True) + sm = _make_state_machine( + state="deleted", + orchestrator=orch, + existing=[_model("a", "x"), _model("b", "y"), _model("c", "z")], + proposed=[_model("a"), _model("b")], + ) + + sm.manage_state() + + assert _names(sm.existing) == ["c"] # only the untargeted item remains + assert _names(sm.sent) == ["a", "b"] + + +# ============================================================================= +# overridden deletions +# ============================================================================= + + +def test_overridden_deletes_non_proposed_items(): + """Overridden removes existing items absent from the proposed config.""" + orch = _FakeOrchestrator(supports_bulk_delete=False) + sm = _make_state_machine( + state="overridden", + orchestrator=orch, + existing=[_model("a", "x"), _model("b", "y"), _model("c", "z")], + proposed=[_model("a", "x")], + ) + + sm.manage_state() + + assert _names(orch.calls["delete"]) == ["b", "c"] # only non-proposed deleted + assert _names(sm.existing) == ["a"] + assert _names(sm.sent) == ["b", "c"] + + +@pytest.mark.parametrize("check_mode", [False, True]) +def test_overridden_explicit_empty_config_deletes_all_existing(check_mode): + """An explicit empty overridden config means delete every existing item.""" + orch = _FakeOrchestrator(supports_bulk_delete=False) + sm = _make_state_machine( + state="overridden", + check_mode=check_mode, + orchestrator=orch, + existing=[_model("a", "x"), _model("b", "y")], + proposed=[], + ) + + sm.manage_state() + + assert len(sm.existing) == 0 + if check_mode: + assert orch.calls["delete"] == [] # no API call in check mode + assert _names(sm.sent) == ["a", "b"] # but the deletes are previewed as sent + else: + assert _names(orch.calls["delete"]) == ["a", "b"] + assert _names(sm.sent) == ["a", "b"] + + +# ============================================================================= +# _execute_operation contract +# ============================================================================= + + +def test_execute_operation_check_mode_skips_call(): + """In check mode the API callable is not invoked but success is reported.""" + sm = _make_state_machine(check_mode=True) + calls: List[tuple] = [] + + result = sm._execute_operation(lambda *a, **k: calls.append(a), "payload") + + assert result is True + assert calls == [] # callable skipped + + +def test_execute_operation_success_returns_true(): + """Outside check mode a successful callable runs and reports success.""" + sm = _make_state_machine(check_mode=False) + calls: List[tuple] = [] + + result = sm._execute_operation(lambda *a, **k: calls.append(a), "payload") + + assert result is True + assert calls == [("payload",)] + + +def test_execute_operation_ignored_error_returns_false(): + """An ignored failure is swallowed and reported as not-sent (False).""" + sm = _make_state_machine(check_mode=False, ignore_errors=True) + + def _boom(*args, **kwargs): + raise Exception("boom") + + assert sm._execute_operation(_boom, "payload") is False + + +def test_execute_operation_non_ignored_error_raises(): + """A non-ignored failure is wrapped in NDStateMachineError.""" + sm = _make_state_machine(check_mode=False, ignore_errors=False) + + def _boom(*args, **kwargs): + raise Exception("boom") + + with pytest.raises(NDStateMachineError, match="Operation failed"): + sm._execute_operation(_boom, "payload") + + +# ============================================================================= +# config missing/null/empty handling +# ============================================================================= + + +@pytest.mark.parametrize("state", ["merged", "replaced", "overridden"]) +@pytest.mark.parametrize( + "config", + [ + pytest.param(_MISSING, id="missing"), + pytest.param(None, id="null"), + ], +) +def test_state_machine_rejects_missing_or_null_config_for_write_states(state, config): + """Write states require explicit config so null cannot become destructive.""" + module = _FakeModule(state=state, config=config) + + with pytest.raises(NDStateMachineError, match=r"config must be provided and cannot be null"): + NDStateMachine(module=module, model_orchestrator=_InitFakeOrchestrator) + + +@pytest.mark.parametrize("state", ["merged", "replaced", "overridden", "deleted"]) +def test_state_machine_accepts_explicit_empty_config(state): + """Explicit ``config: []`` remains distinct from omitted/null config.""" + module = _FakeModule(state=state, config=[]) + sm = NDStateMachine(module=module, model_orchestrator=_InitFakeOrchestrator) + + assert len(sm.proposed) == 0 + + +@pytest.mark.parametrize( + "config", + [ + pytest.param(_MISSING, id="missing"), + pytest.param(None, id="null"), + ], +) +def test_deleted_state_tolerates_missing_or_null_config_as_empty(config): + """Missing/null delete config is non-destructive: it targets no proposed items.""" + module = _FakeModule(state="deleted", config=config) + sm = NDStateMachine(module=module, model_orchestrator=_InitFakeOrchestrator) + + assert len(sm.proposed) == 0 + + +# ============================================================================= +# from_ansible_config normalization +# ============================================================================= + + +@pytest.mark.parametrize( + "data, expected_len", + [ + (None, 0), # config omitted / explicitly null must not crash + ([], 0), + ([{"name": "a"}], 1), + ([{"name": "a"}, {"name": "b"}], 2), + ], +) +def test_from_ansible_config_normalizes_data(data, expected_len): + """``from_ansible_config`` tolerates None and builds a collection otherwise.""" + collection = NDConfigCollection.from_ansible_config(data=data, model_class=_FakeModel) + assert len(collection) == expected_len diff --git a/tests/unit/module_utils/test_utils.py b/tests/unit/module_utils/test_utils.py new file mode 100644 index 000000000..8cc8104e6 --- /dev/null +++ b/tests/unit/module_utils/test_utils.py @@ -0,0 +1,349 @@ +# -*- coding: utf-8 -*- + +# Copyright: (c) 2026, Cisco Systems, Inc. + +# GNU General Public License v3.0+ (see LICENSE or https://www.gnu.org/licenses/gpl-3.0.txt) + +""" +Unit tests for utils.py + +Tests the ``issubset`` helper (both the default bidirectional list matching and +the one-directional ``allow_superset=True`` matching) and ``NDBaseModel.get_diff`` +with list-valued fields under ``exclude_unset=True``. +""" + +# pylint: disable=protected-access + +from __future__ import absolute_import, annotations, division, print_function + +# pylint: disable=invalid-name +__metaclass__ = type +# pylint: enable=invalid-name + +from typing import Any, ClassVar, Dict, List, Literal, Optional + +import pytest +from ansible_collections.cisco.nd.plugins.module_utils.common.pydantic_compat import Field +from ansible_collections.cisco.nd.plugins.module_utils.models.base import NDBaseModel +from ansible_collections.cisco.nd.plugins.module_utils.utils import issubset + +# ============================================================================= +# issubset - scalars +# ============================================================================= + + +@pytest.mark.parametrize( + "subset, superset, expected", + [ + (1, 1, True), + (1, 2, False), + ("a", "a", True), + ("a", "b", False), + (True, True, True), + (1, "1", False), # different types + (1.0, 1, False), # different types (float vs int) + ], +) +def test_issubset_scalars(subset, superset, expected): + """Scalar comparison is strict equality and type-sensitive.""" + assert issubset(subset, superset) is expected + + +# ============================================================================= +# issubset - dicts +# ============================================================================= + + +@pytest.mark.parametrize( + "subset, superset, expected", + [ + # Subset of keys present with matching values -> True + ({"a": 1}, {"a": 1, "b": 2}, True), + # Exact match -> True + ({"a": 1, "b": 2}, {"a": 1, "b": 2}, True), + # Missing key in superset -> False + ({"a": 1, "c": 3}, {"a": 1, "b": 2}, False), + # Value mismatch -> False + ({"a": 1}, {"a": 2}, False), + # None values in subset are ignored + ({"a": 1, "b": None}, {"a": 1}, True), + # Nested dict subset + ({"a": {"x": 1}}, {"a": {"x": 1, "y": 2}}, True), + # Nested dict mismatch + ({"a": {"x": 1}}, {"a": {"x": 2}}, False), + ], +) +def test_issubset_dicts(subset, superset, expected): + """Dict comparison checks each non-None key/value of subset.""" + assert issubset(subset, superset) is expected + + +def test_issubset_dicts_none_keys_not_strict_equality(): + """ + Bidirectional dict matching is NOT strict ``==`` equality: ``None``-valued + keys are skipped, so unequal dicts can still match in both directions. + This documents the behavior described in the ``issubset`` docstring. + """ + a = {"a": 1, "b": None} + b = {"a": 1} + + # Not equal as plain dicts ... + assert a != b + # ... yet issubset matches in both directions (b: None is ignored). + assert issubset(a, b) is True + assert issubset(b, a) is True + + +# ============================================================================= +# issubset - lists with allow_superset=False (default, bidirectional) +# +# This is the behavior deliberately added in PR #209: for lists of dicts the +# match is bidirectional, which is equivalent to equality. An element whose +# candidate carries extra keys does NOT match. +# ============================================================================= + + +@pytest.mark.parametrize( + "subset, superset, expected", + [ + # Equal lists -> True + ([1, 2, 3], [1, 2, 3], True), + # Order-independent matching -> True + ([3, 1, 2], [1, 2, 3], True), + # Different length -> False + ([1, 2], [1, 2, 3], False), + # Element missing -> False + ([1, 2, 4], [1, 2, 3], False), + # Lists of dicts, exact element match -> True + ([{"a": 1}], [{"a": 1}], True), + # Lists of dicts, candidate has EXTRA key -> False (bidirectional) + ([{"a": 1}], [{"a": 1, "b": 2}], False), + # Lists of dicts, subset element has extra key -> False + ([{"a": 1, "b": 2}], [{"a": 1}], False), + ], +) +def test_issubset_lists_bidirectional(subset, superset, expected): + """Default list matching is bidirectional (equivalent to equality).""" + assert issubset(subset, superset) is expected + + +# ============================================================================= +# issubset - lists with allow_superset=True (one-directional) +# +# New behavior introduced in this PR: an element in ``subset`` matches a +# candidate in ``superset`` when it is a subset of that candidate, even if the +# candidate has additional keys. The ``subset`` list may also be shorter than +# ``superset`` (extra existing elements are tolerated) as long as every +# proposed element matches a distinct candidate. +# ============================================================================= + + +@pytest.mark.parametrize( + "subset, superset, expected", + [ + # Candidate has EXTRA key -> now matches (one-directional) + ([{"a": 1}], [{"a": 1, "b": 2}], True), + # Multiple elements, each a subset of a distinct candidate -> True + ( + [{"a": 1}, {"c": 3}], + [{"a": 1, "b": 2}, {"c": 3, "d": 4}], + True, + ), + # Subset element with extra key not in candidate -> still False + ([{"a": 1, "z": 9}], [{"a": 1, "b": 2}], False), + # Proposed list shorter than existing: the proposed item matches a + # candidate and the extra existing element is tolerated -> True + ([{"a": 1}], [{"a": 1, "b": 2}, {"c": 3}], True), + # More proposed items than candidates -> no one-to-one match -> False + ([{"a": 1}, {"c": 3}], [{"a": 1, "b": 2}], False), + # Value mismatch -> False + ([{"a": 2}], [{"a": 1, "b": 2}], False), + ], +) +def test_issubset_lists_one_directional(subset, superset, expected): + """``allow_superset=True`` relaxes list matching to one-directional.""" + assert issubset(subset, superset, allow_superset=True) is expected + + +def test_issubset_one_directional_does_not_reuse_candidate(): + """Each candidate is consumed at most once during list matching.""" + # Two identical subset elements require two matching candidates. + subset = [{"a": 1}, {"a": 1}] + superset = [{"a": 1, "b": 2}, {"a": 1, "c": 3}] + assert issubset(subset, superset, allow_superset=True) is True + + # Only one candidate matches -> the second subset element fails. + subset = [{"a": 1}, {"a": 1}] + superset = [{"a": 1, "b": 2}, {"x": 9}] + assert issubset(subset, superset, allow_superset=True) is False + + +def test_issubset_one_directional_allows_shorter_subset(): + """Under allow_superset the proposed list may be shorter than the existing. + + This is the merged-state contract: a user who names only some children must + not flag the parent as changed when the controller already holds additional + children. Every proposed child must still match a distinct existing child. + """ + # One proposed child, two existing children -> matches the first; the extra + # existing child is tolerated. + subset = [{"vrf_name": "TENANT_A"}] + superset = [ + {"vrf_name": "TENANT_A", "vlan_id": 500}, + {"vrf_name": "TENANT_B", "vlan_id": 600}, + ] + assert issubset(subset, superset, allow_superset=True) is True + + # Two proposed children, each matching a distinct existing child that + # carries extra keys, with a third existing child left untouched -> True. + subset = [{"vrf_name": "TENANT_A"}, {"vrf_name": "TENANT_B"}] + superset = [ + {"vrf_name": "TENANT_A", "vlan_id": 500}, + {"vrf_name": "TENANT_B", "vlan_id": 600}, + {"vrf_name": "TENANT_C", "vlan_id": 700}, + ] + assert issubset(subset, superset, allow_superset=True) is True + + # A proposed child that matches none of the existing children -> False. + subset = [{"vrf_name": "TENANT_Z"}] + superset = [ + {"vrf_name": "TENANT_A", "vlan_id": 500}, + {"vrf_name": "TENANT_B", "vlan_id": 600}, + ] + assert issubset(subset, superset, allow_superset=True) is False + + # More proposed children than existing -> one-to-one match impossible. + subset = [{"vrf_name": "TENANT_A"}, {"vrf_name": "TENANT_B"}] + superset = [{"vrf_name": "TENANT_A", "vlan_id": 500}] + assert issubset(subset, superset, allow_superset=True) is False + + +def test_issubset_type_mismatch_returns_false(): + """Mismatched top-level types short-circuit to False.""" + assert issubset({"a": 1}, [1, 2]) is False + assert issubset([1], {"a": 1}) is False + + +# ============================================================================= +# issubset - greedy-matching regression (bipartite matching) +# +# A less-specific subset item must not greedily consume a candidate that a +# more-specific item needs. These cases fail with first-match/greedy logic but +# pass with a proper maximum-matching solution. +# ============================================================================= + + +def test_issubset_one_directional_no_greedy_false_diff(): + """ + The less-specific ``{"a": 1}`` must yield the ``{"a": 1, "b": 2}`` candidate + to the more-specific ``{"a": 1, "b": 2}`` proposed item, matching instead + against ``{"a": 1, "b": 3}``. A valid pairing exists, so no diff. + """ + subset = [{"a": 1}, {"a": 1, "b": 2}] + superset = [{"a": 1, "b": 2}, {"a": 1, "b": 3}] + assert issubset(subset, superset, allow_superset=True) is True + + +def test_issubset_one_directional_no_greedy_reordered(): + """Order-independence: same data, subset elements swapped.""" + subset = [{"a": 1, "b": 2}, {"a": 1}] + superset = [{"a": 1, "b": 3}, {"a": 1, "b": 2}] + assert issubset(subset, superset, allow_superset=True) is True + + +def test_issubset_one_directional_no_valid_pairing_is_false(): + """When no perfect pairing exists, a diff is still correctly reported.""" + # Both proposed items demand b==2, but only one candidate has b==2. + subset = [{"a": 1, "b": 2}, {"a": 1, "b": 2}] + superset = [{"a": 1, "b": 2}, {"a": 1, "b": 3}] + assert issubset(subset, superset, allow_superset=True) is False + + +def test_issubset_bidirectional_no_greedy_false_diff(): + """ + Greedy matching can also misfire in the default bidirectional mode when + duplicate values are present. A perfect matching still exists here. + """ + subset = [{"a": 1}, {"a": 1}] + superset = [{"a": 1}, {"a": 1}] + assert issubset(subset, superset) is True + + +# ============================================================================= +# NDBaseModel.get_diff with list-valued fields under exclude_unset=True +# ============================================================================= + + +class _ListFieldModel(NDBaseModel): + """Minimal model with a list-valued field for diff testing.""" + + identifiers: ClassVar[Optional[List[str]]] = ["name"] + identifier_strategy: ClassVar[Literal["single", "composite", "hierarchical", "singleton"]] = "single" + + name: str = Field(alias="name") + members: Optional[List[Dict[str, Any]]] = Field(default=None, alias="members") + + +def test_get_diff_list_field_subset_with_extra_keys_no_diff(): + """ + With allow_superset=True, an existing element carrying extra keys (e.g. a + controller-populated ``deploy`` flag) does not trigger a spurious diff when + the proposed element omits those keys. + """ + existing = _ListFieldModel(name="vrf1", members=[{"id": 1, "deploy": True}]) + proposed = _ListFieldModel(name="vrf1", members=[{"id": 1}]) + + # allow_superset=True -> one-directional list match -> proposed is a subset. + assert existing.get_diff(proposed, exclude_unset=True, allow_superset=True) is True + + +def test_get_diff_exclude_unset_without_allow_superset_flags_extra_keys(): + """ + exclude_unset and allow_superset are independent: comparing only the + proposed model's set fields (exclude_unset=True) while keeping strict + bidirectional list matching (allow_superset=False) still flags an existing + element that carries extra keys. + """ + existing = _ListFieldModel(name="vrf1", members=[{"id": 1, "deploy": True}]) + proposed = _ListFieldModel(name="vrf1", members=[{"id": 1}]) + + assert existing.get_diff(proposed, exclude_unset=True, allow_superset=False) is False + + +def test_get_diff_allow_superset_without_exclude_unset(): + """ + allow_superset can be requested on its own: with exclude_unset=False the + proposed model's defaults are compared, but list elements are still matched + one-directionally so the extra ``deploy`` key is tolerated. + """ + existing = _ListFieldModel(name="vrf1", members=[{"id": 1, "deploy": True}]) + proposed = _ListFieldModel(name="vrf1", members=[{"id": 1}]) + + assert existing.get_diff(proposed, allow_superset=True) is True + + +def test_get_diff_list_field_extra_keys_triggers_diff_without_exclude_unset(): + """ + Without exclude_unset (default), list matching is bidirectional, so the + extra ``deploy`` key on the existing element produces a diff. + """ + existing = _ListFieldModel(name="vrf1", members=[{"id": 1, "deploy": True}]) + proposed = _ListFieldModel(name="vrf1", members=[{"id": 1}]) + + assert existing.get_diff(proposed) is False + + +def test_get_diff_list_field_value_change_triggers_diff(): + """A genuine value change in a list element is always a diff.""" + existing = _ListFieldModel(name="vrf1", members=[{"id": 1}]) + proposed = _ListFieldModel(name="vrf1", members=[{"id": 2}]) + + assert existing.get_diff(proposed, exclude_unset=True) is False + + +def test_get_diff_unset_list_field_ignored(): + """A list field not set on the proposed model is ignored under exclude_unset.""" + existing = _ListFieldModel(name="vrf1", members=[{"id": 1, "deploy": True}]) + proposed = _ListFieldModel(name="vrf1") # members not set + + assert existing.get_diff(proposed, exclude_unset=True) is True diff --git a/tests/unit/modules/test_nd_manage_networks.py b/tests/unit/modules/test_nd_manage_networks.py index 4c1e7a4ad..29564a4cd 100644 --- a/tests/unit/modules/test_nd_manage_networks.py +++ b/tests/unit/modules/test_nd_manage_networks.py @@ -11,10 +11,102 @@ __metaclass__ = type # pylint: disable=invalid-name +import json from unittest.mock import patch +import pytest +from ansible.module_utils import basic as ansible_basic +from ansible_collections.cisco.nd.plugins.module_utils.rest.sender_nd import ( + Sender, +) from ansible_collections.cisco.nd.plugins.modules import nd_manage_networks +_MISSING_CONFIG = object() + + +class _ModuleFailure(Exception): + """Capture ``AnsibleModule.fail_json`` without terminating pytest.""" + + def __init__(self, module, result): + super().__init__(result.get("msg", "module failed")) + self.module = module + self.result = result + self.controller_calls = [] + + +def _run_composed_network_module(module_args): + """ + Run Ansible parsing, the production workflow, and the real state machine. + + Only the controller transport is replaced. + """ + raw_args = json.dumps({"ANSIBLE_MODULE_ARGS": module_args}).encode() + captured = {} + controller_calls = [] + + def exit_json(module, **kwargs): + captured["module"] = module + captured["result"] = kwargs + + def fail_json(module, **kwargs): + raise _ModuleFailure(module, kwargs) + + def sender_commit(sender): + path = sender.path + verb = sender.verb.value + controller_calls.append((verb, path, sender.payload)) + if path == "/api/v1/manage/fabrics/fab1": + data = {"management": {"type": "vxlanIbgp"}} + elif path == "/api/v1/manage/fabrics?category=fabricGroup&max=10000": + data = {"fabrics": []} + elif path == "/api/v1/manage/fabrics/fab1/networks?offset=0&max=10000": + data = { + "networks": [ + {"networkName": "production"}, + {"networkName": "storage"}, + ], + "metadata": {"counts": {"total": 2, "remaining": 0}}, + } + elif verb == "POST" and path == "/api/v1/manage/fabrics/fab1/networkAttachments/query?offset=0&max=10000&includeAll=true": + data = { + "attachments": [], + "metadata": {"counts": {"total": 0, "remaining": 0}}, + } + elif verb == "POST" and path in { + "/api/v1/manage/fabrics/fab1/networkActions/deploy", + "/api/v1/manage/fabrics/fab1/networkActions/remove", + }: + data = { + "results": [ + {"networkName": "production", "status": "success"}, + {"networkName": "storage", "status": "success"}, + ], + } + else: + raise AssertionError((verb, path, sender.payload)) + sender.response = { + "RETURN_CODE": 200, + "MESSAGE": "OK", + "DATA": data, + "REQUEST_PATH": path, + "METHOD": verb, + } + + # ansible-core 2.19+ requires a serialization profile to decode _ANSIBLE_ARGS. + with patch.object(ansible_basic, "_ANSIBLE_ARGS", raw_args), patch.object(ansible_basic, "_ANSIBLE_PROFILE", "legacy", create=True), patch.object( + nd_manage_networks.AnsibleModule, "exit_json", exit_json + ), patch.object(nd_manage_networks.AnsibleModule, "fail_json", fail_json), patch.object(Sender, "commit", sender_commit): + try: + nd_manage_networks.main() + except _ModuleFailure as exc: + exc.controller_calls = list(controller_calls) + raise + + return { + **captured, + "controller_calls": controller_calls, + } + def test_nd_manage_networks_requires_pydantic_immediately_after_module_creation(): """ @@ -66,3 +158,91 @@ def fake_require_pydantic(module): ] assert events[1][1] is events[2][1] assert events[0][1].kwargs["supports_check_mode"] is True + assert "default" not in events[0][1].kwargs["argument_spec"]["config"] + + +@pytest.mark.parametrize("check_mode", [False, True]) +@pytest.mark.parametrize( + "config", + [ + pytest.param(_MISSING_CONFIG, id="omitted"), + pytest.param(None, id="null"), + ], +) +def test_nd_manage_networks_rejects_missing_overridden_config_before_query(config, check_mode): + """Omitted/null config must fail before production state discovery.""" + module_args = { + "fabric_name": "fab1", + "state": "overridden", + "_ansible_check_mode": check_mode, + "_ansible_verbosity": 0, + } + if config is not _MISSING_CONFIG: + module_args["config"] = config + + with pytest.raises(_ModuleFailure) as exc_info: + _run_composed_network_module(module_args) + + assert exc_info.value.module.params["config"] is None + assert "config must be provided and cannot be null" in exc_info.value.result["msg"] + assert exc_info.value.controller_calls == [] + + +def test_nd_manage_networks_explicit_empty_overridden_previews_existing_deletions(): + """Explicit config: [] previews delete-all through the complete workflow.""" + run = _run_composed_network_module( + { + "fabric_name": "fab1", + "state": "overridden", + "config": [], + "_ansible_check_mode": True, + "_ansible_verbosity": 0, + } + ) + + assert run["module"].params["config"] == [] + assert len(run["result"]["before"]) == 2 + assert run["result"]["after"] == [] + assert run["result"]["changed"] is True + assert run["result"]["check_mode_deploy_payloads"] == [ + {"networkNames": ["production", "storage"]}, + ] + assert len(run["controller_calls"]) == 3 + assert all(verb == "GET" for verb, _path, _payload in run["controller_calls"]) + + +def test_nd_manage_networks_explicit_empty_overridden_deletes_existing_resources(): + """Explicit config: [] performs delete-all through the complete workflow.""" + run = _run_composed_network_module( + { + "fabric_name": "fab1", + "state": "overridden", + "config": [], + "_ansible_check_mode": False, + "_ansible_verbosity": 0, + } + ) + + assert run["module"].params["config"] == [] + assert len(run["result"]["before"]) == 2 + assert run["result"]["after"] == [] + assert run["result"]["changed"] is True + assert "check_mode_deploy_payloads" not in run["result"] + mutation_calls = [ + (verb, path, payload) + for verb, path, payload in run["controller_calls"] + if path + in { + "/api/v1/manage/fabrics/fab1/networkActions/deploy", + "/api/v1/manage/fabrics/fab1/networkActions/remove", + } + ] + assert [(verb, path) for verb, path, _payload in mutation_calls] == [ + ("POST", "/api/v1/manage/fabrics/fab1/networkActions/deploy"), + ("POST", "/api/v1/manage/fabrics/fab1/networkActions/remove"), + ] + for _verb, _path, payload in mutation_calls: + assert set(payload["networkNames"]) == { + "production", + "storage", + } diff --git a/tests/unit/modules/test_nd_manage_vpc_pair.py b/tests/unit/modules/test_nd_manage_vpc_pair.py new file mode 100644 index 000000000..bee7d2b29 --- /dev/null +++ b/tests/unit/modules/test_nd_manage_vpc_pair.py @@ -0,0 +1,214 @@ +# -*- coding: utf-8 -*- + +# Copyright: (c) 2026, Cisco Systems, Inc. +# GNU General Public License v3.0+ (see LICENSE or https://www.gnu.org/licenses/gpl-3.0.txt) + +"""Production-wrapper tests for ``nd_manage_vpc_pair``.""" + +from __future__ import absolute_import, annotations, division, print_function + +import json +from unittest.mock import patch + +import pytest +from ansible.module_utils import basic as ansible_basic + +from ansible_collections.cisco.nd.plugins.module_utils.nd_v2 import ( + NDModule, +) +from ansible_collections.cisco.nd.plugins.modules import nd_manage_vpc_pair + +_MISSING_CONFIG = object() + + +class _ModuleFailure(Exception): + """Capture ``AnsibleModule.fail_json`` without terminating pytest.""" + + def __init__(self, module, result): + super().__init__(result.get("msg", "module failed")) + self.module = module + self.result = result + self.controller_calls = [] + + +def _module_args(config, check_mode): + args = { + "fabric_name": "fab1", + "state": "overridden", + "force": False, + "verify": None, + "config_actions": {"save": False, "deploy": False, "type": "switch"}, + "_ansible_check_mode": check_mode, + "_ansible_verbosity": 0, + } + if config is not _MISSING_CONFIG: + args["config"] = config + return args + + +def _run_composed_vpc_pair_module(module_args): + """Run Ansible parsing, the production service, and the real state machine.""" + raw_args = json.dumps({"ANSIBLE_MODULE_ARGS": module_args}).encode() + captured = {} + controller_calls = [] + deleted_switches = set() + + def exit_json(module, **kwargs): + captured["module"] = module + captured["result"] = kwargs + + def fail_json(module, **kwargs): + raise _ModuleFailure(module, kwargs) + + def nd_request(_client, path, verb, data=None): + method = verb.value + controller_calls.append((method, path, data)) + if path == "/api/v1/manage/fabrics/fab1": + return {"fabricType": "VXLAN"} + if path.endswith("/vpcPairs"): + return { + "vpcPairs": [ + pair + for pair in [ + { + "switchId": "SWA", + "peerSwitchId": "SWB", + "useVirtualPeerLink": False, + }, + { + "switchId": "SWC", + "peerSwitchId": "SWD", + "useVirtualPeerLink": False, + }, + ] + if pair["switchId"] not in deleted_switches + ] + } + if path.endswith("/switches"): + return { + "switches": [ + { + "serialNumber": "SWA", + "vpcConfigured": True, + "vpcData": {"peerSwitchId": "SWB"}, + }, + { + "serialNumber": "SWC", + "vpcConfigured": True, + "vpcData": {"peerSwitchId": "SWD"}, + }, + ] + } + if path.endswith("/switches/SWA/vpcPair"): + if method == "PUT": + deleted_switches.add("SWA") + return {} + if "SWA" in deleted_switches: + return {} + return { + "switchId": "SWA", + "peerSwitchId": "SWB", + "useVirtualPeerLink": False, + } + if path.endswith("/switches/SWC/vpcPair"): + if method == "PUT": + deleted_switches.add("SWC") + return {} + if "SWC" in deleted_switches: + return {} + return { + "switchId": "SWC", + "peerSwitchId": "SWD", + "useVirtualPeerLink": False, + } + if path.endswith("/vpcPairConsistency"): + return {"type2Consistency": True} + if "/vpcPairOverview" in path: + return { + "overlay": { + "networkCount": {"deployed": 0}, + "vrfCount": {"deployed": 0}, + }, + "inventory": { + "syncStatus": {"pending": 0, "outOfSync": 0, "inProgress": 0}, + "vpcInterfaceCount": 0, + }, + } + raise AssertionError((method, path, data)) + + # ansible-core 2.19+ requires a serialization profile to decode _ANSIBLE_ARGS. + with patch.object(ansible_basic, "_ANSIBLE_ARGS", raw_args), patch.object(ansible_basic, "_ANSIBLE_PROFILE", "legacy", create=True), patch.object( + nd_manage_vpc_pair.AnsibleModule, "exit_json", exit_json + ), patch.object(nd_manage_vpc_pair.AnsibleModule, "fail_json", fail_json), patch.object(nd_manage_vpc_pair, "setup_logging"), patch.object( + NDModule, "request", nd_request + ): + try: + nd_manage_vpc_pair.main() + except _ModuleFailure as exc: + exc.controller_calls = list(controller_calls) + raise + + return { + **captured, + "controller_calls": controller_calls, + } + + +@pytest.mark.parametrize("check_mode", [False, True]) +@pytest.mark.parametrize( + "config", + [ + pytest.param(_MISSING_CONFIG, id="omitted"), + pytest.param(None, id="null"), + ], +) +def test_vpc_pair_wrapper_rejects_missing_config_before_service(config, check_mode): + """Missing overridden config must fail before state-machine construction.""" + with pytest.raises(_ModuleFailure) as exc_info: + _run_composed_vpc_pair_module(_module_args(config, check_mode)) + + assert exc_info.value.module.params["config"] is None + assert "config must be provided and cannot be null" in exc_info.value.result["msg"] + assert exc_info.value.controller_calls == [] + + +def test_vpc_pair_wrapper_explicit_empty_overridden_previews_existing_deletions(): + """Explicit config: [] reaches the real state machine safely in check mode.""" + run = _run_composed_vpc_pair_module(_module_args([], check_mode=True)) + + assert run["module"].params["config"] == [] + assert len(run["result"]["before"]) == 2 + assert run["result"]["after"] == [] + assert run["result"]["changed"] is True + assert len(run["result"]["deleted"]) == 2 + assert len(run["controller_calls"]) == 7 + assert all(method == "GET" for method, _path, _data in run["controller_calls"]) + + +def test_vpc_pair_wrapper_explicit_empty_overridden_deletes_existing_pairs(): + """Explicit config: [] performs both unpair operations in normal mode.""" + run = _run_composed_vpc_pair_module(_module_args([], check_mode=False)) + + assert run["module"].params["config"] == [] + assert len(run["result"]["before"]) == 2 + assert run["result"]["after"] == [] + assert run["result"]["current"] == [] + assert run["result"]["changed"] is True + assert len(run["result"]["deleted"]) == 2 + + put_calls = [(path, data) for method, path, data in run["controller_calls"] if method == "PUT"] + assert len(put_calls) == 2 + unpair_calls = dict(put_calls) + assert unpair_calls == { + "/api/v1/manage/fabrics/fab1/switches/SWA/vpcPair": { + "vpcAction": "unPair", + "switchId": "SWA", + "peerSwitchId": "SWB", + }, + "/api/v1/manage/fabrics/fab1/switches/SWC/vpcPair": { + "vpcAction": "unPair", + "switchId": "SWC", + "peerSwitchId": "SWD", + }, + } + assert all(method in {"GET", "PUT"} for method, _path, _data in run["controller_calls"])