From b49086b7343fca69d9bbbd4c8d985cba84c3e4cd Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Thu, 18 Jun 2026 10:39:12 +0530 Subject: [PATCH 01/14] Fix vPC pair physical interface handling --- .../module_utils/manage_vpc_pair/deploy.py | 314 +++++++++++++----- .../nd_vpc_pair/tasks/nd_vpc_pair_delete.yaml | 16 +- .../nd_vpc_pair/tasks/nd_vpc_pair_merge.yaml | 27 +- .../tasks/nd_vpc_pair_override.yaml | 9 +- .../tasks/nd_vpc_pair_replace.yaml | 16 +- 5 files changed, 268 insertions(+), 114 deletions(-) diff --git a/plugins/module_utils/manage_vpc_pair/deploy.py b/plugins/module_utils/manage_vpc_pair/deploy.py index 34fffb3fe..04447ae29 100644 --- a/plugins/module_utils/manage_vpc_pair/deploy.py +++ b/plugins/module_utils/manage_vpc_pair/deploy.py @@ -12,6 +12,13 @@ _raise_vpc_error, get_config_actions, ) +from ansible_collections.cisco.nd.plugins.module_utils.manage_vpc_pair.enums import ( + VpcFieldNames, +) +from ansible_collections.cisco.nd.plugins.module_utils.endpoints.v1.manage.manage_fabrics_switchactions import ( + EpManageFabricsSwitchActionsDeployPost, +) +from ansible_collections.cisco.nd.plugins.module_utils.enums import HttpVerbEnum from ansible_collections.cisco.nd.plugins.module_utils.nd_v2 import ( NDModule as NDModuleV2, NDModuleError, @@ -107,99 +114,140 @@ def _is_non_fatal_config_save_error(error: NDModuleError) -> bool: return any(signature in message for signature in non_fatal_signatures) -def custom_vpc_deploy(nrm: Any, fabric_name: str, result: dict[str, Any]) -> dict[str, Any]: +def _collect_affected_switch_ids(nrm: Any, result: dict[str, Any]) -> list[str]: """ - Custom save/deploy action handler for vPC fabric changes using RestSend. + Collect the serial numbers of switches affected by this run. - - Smart action decision (_needs_deployment) - - Optional Step 1: Save fabric configuration - - Optional Step 2: Deploy fabric with forceShowRun=true - - Proper error handling with NDModuleError - - Results aggregation - - Executes only if there are actual changes or pending operations + Switch-scoped deploy targets only the switches that changed (created, + updated, deleted, or currently not in-sync) instead of the whole fabric. + + Serials are gathered from two complementary sources so that every write + operation is covered, including a fresh create where the query-phase + pending lists are still empty: + + 1. The authoritative per-run diff in ``result`` (``created`` / ``deleted`` + identifiers and ``updated`` entries). Each identifier is a + ``(switch_id, peer_switch_id)`` tuple of switch serials. + 2. The query-phase ``_pending_create`` / ``_pending_delete`` / + ``_not_in_sync_pairs`` contexts, which cover the deploy-only, + no-diff case (pair already exists but is not in-sync on the controller). Args: nrm: NDStateMachine instance - fabric_name: Fabric name to deploy - result: Module result dictionary to check for changes + result: Module result dictionary with class diff information Returns: - Save/deploy result dictionary + Ordered, de-duplicated list of switch serial numbers. + """ + switch_ids: list[str] = [] + seen: set[str] = set() + + def _add(serial: Any) -> None: + if serial and serial not in seen: + seen.add(serial) + switch_ids.append(serial) + + def _add_identifier(identifier: Any) -> None: + # Identifiers are (switch_id, peer_switch_id) serial tuples/lists. + if isinstance(identifier, (list, tuple)): + for serial in identifier: + _add(serial) + + # Source 1: authoritative per-run diff. + if isinstance(result, dict): + for identifier in result.get("created", []) or []: + _add_identifier(identifier) + for identifier in result.get("deleted", []) or []: + _add_identifier(identifier) + for entry in result.get("updated", []) or []: + if isinstance(entry, dict): + _add_identifier(entry.get("identifier")) + else: + _add_identifier(entry) - Raises: - NDModuleError: If deployment fails + # Source 2: query-phase pending / not-in-sync contexts. + for key in ("_pending_create", "_pending_delete", "_not_in_sync_pairs"): + for pair in nrm.module.params.get(key, []) or []: + _add(pair.get(VpcFieldNames.SWITCH_ID)) + _add(pair.get(VpcFieldNames.PEER_SWITCH_ID)) + + return switch_ids + + +def _build_switch_deploy_path(fabric_name: str) -> str: """ - config_actions = get_config_actions(nrm.module) - save_enabled = bool(config_actions.get("save", True)) - deploy_enabled = bool(config_actions.get("deploy", True)) - action_type = config_actions.get("type", "switch") - action_payload = {"type": action_type} + Build the switch-scoped deploy endpoint path for the given fabric. - # Defensive runtime validation (model validation already enforces this). - if deploy_enabled and not save_enabled: - _raise_vpc_error(msg="Invalid config_actions: deploy=true requires save=true") + POST /api/v1/manage/fabrics/{fabricName}/switchActions/deploy + """ + endpoint = EpManageFabricsSwitchActionsDeployPost(fabric_name=fabric_name) + return endpoint.path - if not save_enabled and not deploy_enabled: - return { - "msg": "Config actions disabled (save=false, deploy=false), skipping config save/deploy", - "fabric": fabric_name, - "deployment_needed": False, - "changed": False, - "config_actions": config_actions, - } - # Smart deployment decision (from Common.needs_deployment) - if not _needs_deployment(result, nrm): - return { - "msg": ("No configuration changes, pending operations, or out-of-sync pairs " "detected, skipping config actions"), - "fabric": fabric_name, - "deployment_needed": False, - "changed": False, - "config_actions": config_actions, - } +def _switch_level_deploy(nd_v2: NDModuleV2, fabric_name: str, switch_ids: list[str], results: Any) -> None: + """ + Generate and push configuration for specific switches. - if nrm.module.check_mode: - # check_mode deployment preview - pending_create = nrm.module.params.get("_pending_create", []) - pending_delete = nrm.module.params.get("_pending_delete", []) - not_in_sync_pairs = nrm.module.params.get("_not_in_sync_pairs", []) - planned_actions = [] - if save_enabled: - save_path = FabricUtils.build_config_save_path(fabric_name) - planned_actions.append(f"POST {save_path} payload={action_payload}") - if deploy_enabled: - deploy_path = FabricUtils.build_config_deploy_path(fabric_name, force_show_run=True) - planned_actions.append(f"POST {deploy_path} payload={action_payload}") - if save_enabled and deploy_enabled: - preview_msg = "CHECK MODE: Would save and deploy fabric configuration" - elif save_enabled: - preview_msg = "CHECK MODE: Would save fabric configuration" - else: - preview_msg = "CHECK MODE: Would deploy fabric configuration" + Uses the switch-scoped ``switchActions/deploy`` endpoint, which generates + and deploys configuration only for the supplied switch serials. This avoids + the fabric-level ``actions/configSave`` path that returns HTTP 500 for vPC + pairs configured with a physical peer-link (issue #335). - deployment_info = { - "msg": preview_msg, - "fabric": fabric_name, - "deployment_needed": True, - "changed": True, - "would_save": save_enabled, - "would_deploy": deploy_enabled, - "config_actions": config_actions, - "deployment_decision_factors": { - "diff_has_changes": _has_explicit_diff_changes(result), - "pending_create_operations": len(pending_create), - "pending_delete_operations": len(pending_delete), - "not_in_sync_pairs": len(not_in_sync_pairs), - "actual_changes": result.get("changed", False), - }, - "planned_actions": planned_actions, - } - return deployment_info + Args: + nd_v2: NDModuleV2 RestSend wrapper. + fabric_name: Fabric name. + switch_ids: Switch serial numbers to deploy. + results: Results aggregator. - # Initialize RestSend via NDModuleV2 - nd_v2 = NDModuleV2(nrm.module) + Raises: + VpcPairResourceError: If the deploy call fails. + """ + deploy_path = _build_switch_deploy_path(fabric_name) + payload = {"switchIds": switch_ids} + + try: + nd_v2.request(deploy_path, HttpVerbEnum.POST, payload) + register_action_api_call( + results=results, + request_path=deploy_path, + payload=payload, + return_code=nd_v2.status, + message="Switch deployment successful", + success=True, + changed=True, + ) + except NDModuleError as error: + register_action_api_call( + results=results, + request_path=deploy_path, + payload=payload, + return_code=error.status, + message=error.msg, + success=False, + changed=False, + ) + results.build_final_result() + final_result = dict(results.final_result) + final_msg = final_result.pop("msg", "Switch deployment failed") + _raise_vpc_error(msg=final_msg, **final_result) + + +def _fabric_level_deploy( + nd_v2: NDModuleV2, + nrm: Any, + fabric_name: str, + action_payload: dict[str, Any], + save_enabled: bool, + deploy_enabled: bool, + results: Any, +) -> None: + """ + Fabric-level (global) config save + deploy flow. + + Retained for ``config_actions.type == "global"`` and as a fallback when no + affected switch serials can be resolved for a switch-scoped deploy. + """ fabric_utils = FabricUtils(nd_v2, fabric_name) - results = Results() # Step 1: Save config if save_enabled: @@ -282,6 +330,118 @@ def custom_vpc_deploy(nrm: Any, fabric_name: str, result: dict[str, Any]) -> dic final_msg = final_result.pop("msg", "Fabric deployment failed") _raise_vpc_error(msg=final_msg, **final_result) + +def custom_vpc_deploy(nrm: Any, fabric_name: str, result: dict[str, Any]) -> dict[str, Any]: + """ + Custom save/deploy action handler for vPC fabric changes using RestSend. + + - Smart action decision (_needs_deployment) + - Optional Step 1: Save fabric configuration + - Optional Step 2: Deploy fabric with forceShowRun=true + - Proper error handling with NDModuleError + - Results aggregation + - Executes only if there are actual changes or pending operations + + Args: + nrm: NDStateMachine instance + fabric_name: Fabric name to deploy + result: Module result dictionary to check for changes + + Returns: + Save/deploy result dictionary + + Raises: + NDModuleError: If deployment fails + """ + config_actions = get_config_actions(nrm.module) + save_enabled = bool(config_actions.get("save", True)) + deploy_enabled = bool(config_actions.get("deploy", True)) + action_type = config_actions.get("type", "switch") + action_payload = {"type": action_type} + + # Defensive runtime validation (model validation already enforces this). + if deploy_enabled and not save_enabled: + _raise_vpc_error(msg="Invalid config_actions: deploy=true requires save=true") + + if not save_enabled and not deploy_enabled: + return { + "msg": "Config actions disabled (save=false, deploy=false), skipping config save/deploy", + "fabric": fabric_name, + "deployment_needed": False, + "changed": False, + "config_actions": config_actions, + } + + # Smart deployment decision (from Common.needs_deployment) + if not _needs_deployment(result, nrm): + return { + "msg": ("No configuration changes, pending operations, or out-of-sync pairs " "detected, skipping config actions"), + "fabric": fabric_name, + "deployment_needed": False, + "changed": False, + "config_actions": config_actions, + } + + if nrm.module.check_mode: + # check_mode deployment preview + pending_create = nrm.module.params.get("_pending_create", []) + pending_delete = nrm.module.params.get("_pending_delete", []) + not_in_sync_pairs = nrm.module.params.get("_not_in_sync_pairs", []) + planned_actions = [] + switch_ids = _collect_affected_switch_ids(nrm, result) if action_type == "switch" and deploy_enabled else [] + if switch_ids: + deploy_path = _build_switch_deploy_path(fabric_name) + planned_actions.append(f"POST {deploy_path} payload={{'switchIds': {switch_ids}}}") + preview_msg = "CHECK MODE: Would deploy configuration for affected switches" + else: + if save_enabled: + save_path = FabricUtils.build_config_save_path(fabric_name) + planned_actions.append(f"POST {save_path} payload={action_payload}") + if deploy_enabled: + deploy_path = FabricUtils.build_config_deploy_path(fabric_name, force_show_run=True) + planned_actions.append(f"POST {deploy_path} payload={action_payload}") + if save_enabled and deploy_enabled: + preview_msg = "CHECK MODE: Would save and deploy fabric configuration" + elif save_enabled: + preview_msg = "CHECK MODE: Would save fabric configuration" + else: + preview_msg = "CHECK MODE: Would deploy fabric configuration" + + deployment_info = { + "msg": preview_msg, + "fabric": fabric_name, + "deployment_needed": True, + "changed": True, + "would_save": save_enabled, + "would_deploy": deploy_enabled, + "config_actions": config_actions, + "deployment_decision_factors": { + "diff_has_changes": _has_explicit_diff_changes(result), + "pending_create_operations": len(pending_create), + "pending_delete_operations": len(pending_delete), + "not_in_sync_pairs": len(not_in_sync_pairs), + "actual_changes": result.get("changed", False), + }, + "planned_actions": planned_actions, + } + return deployment_info + + # Initialize RestSend via NDModuleV2 + nd_v2 = NDModuleV2(nrm.module) + results = Results() + + # Switch-scoped deploy (config_actions.type == "switch", the default) targets + # only the affected switches via switchActions/deploy, which generates and + # pushes config in a single call. This avoids the fabric-level actions/configSave + # path that returns HTTP 500 for vPC pairs with a physical peer-link (issue #335). + # The fabric-level (global) flow is retained for type == "global" and as a + # fallback when no affected switch serials can be resolved. + switch_ids = _collect_affected_switch_ids(nrm, result) if action_type == "switch" and deploy_enabled else [] + if switch_ids: + _switch_level_deploy(nd_v2, fabric_name, switch_ids, results) + else: + _fabric_level_deploy(nd_v2, nrm, fabric_name, action_payload, save_enabled, deploy_enabled, results) + # Build final result results.build_final_result() final_result = dict(results.final_result) diff --git a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_delete.yaml b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_delete.yaml index 0ec0a678e..93869e643 100644 --- a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_delete.yaml +++ b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_delete.yaml @@ -184,25 +184,24 @@ - (result.deployment_needed | default(result.deployment.deployment_needed | default(false))) | bool == true tags: delete -- name: DELETE - TC2b - ASSERT - Verify config-save and deploy API traces +- name: DELETE - TC2b - ASSERT - Verify switch-level deploy API traces ansible.builtin.assert: that: - result.deployment.response is defined - - (result.deployment.response | length) >= 2 - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/configSave') + | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') | list | length ) > 0 - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/deploy') + | selectattr('REQUEST_PATH', 'search', '/actions/configSave') | list | length - ) > 0 + ) == 0 tags: delete - name: DELETE - TC2b - GATHER - Verify deploy delete result in ND @@ -431,21 +430,20 @@ - tc6_delete_result.deployment.config_actions.deploy == true - tc6_delete_result.deployment.config_actions.type == "switch" - tc6_delete_result.deployment.response is defined - - (tc6_delete_result.deployment.response | length) >= 2 - > ( tc6_delete_result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/configSave') + | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') | list | length ) > 0 - > ( tc6_delete_result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/deploy') + | selectattr('REQUEST_PATH', 'search', '/actions/configSave') | list | length - ) > 0 + ) == 0 tags: delete # TC7 - check_mode should not apply delete configuration changes diff --git a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_merge.yaml b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_merge.yaml index a4d42045f..a36f9e64f 100644 --- a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_merge.yaml +++ b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_merge.yaml @@ -539,8 +539,8 @@ - > ( (tc6b_deployment_needed and (result.deployment.response is defined) - and ((result.deployment.response | selectattr('REQUEST_PATH', 'search', '/actions/configSave') | list | length) > 0) - and ((result.deployment.response | selectattr('REQUEST_PATH', 'search', '/actions/deploy') | list | length) > 0)) + and ((result.deployment.response | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') | list | length) > 0) + and ((result.deployment.response | selectattr('REQUEST_PATH', 'search', '/actions/configSave') | list | length) == 0)) or ((not tc6b_deployment_needed) and (result.deployment.msg | default('') is search('skipping'))) ) @@ -827,25 +827,24 @@ - (result.deployment_needed | default(result.deployment.deployment_needed | default(false))) | bool == true tags: merge -- name: MERGE - TC10 - ASSERT - Verify config-save and deploy API traces +- name: MERGE - TC10 - ASSERT - Verify switch-level deploy API traces ansible.builtin.assert: that: - result.deployment.response is defined - - (result.deployment.response | length) >= 2 - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/configSave') + | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') | list | length ) > 0 - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/deploy') + | selectattr('REQUEST_PATH', 'search', '/actions/configSave') | list | length - ) > 0 + ) == 0 tags: merge - name: MERGE - TC10 - GATHER - Query gathered pair after deploy flow @@ -1486,17 +1485,17 @@ - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/configSave') + | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') | list | length ) > 0 - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/deploy') + | selectattr('REQUEST_PATH', 'search', '/actions/configSave') | list | length - ) > 0 + ) == 0 tags: merge # TC18 - config_actions save-only path (save=true, deploy=false) @@ -1687,18 +1686,18 @@ - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/configSave') + | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') | list | length ) > 0 - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/deploy') + | selectattr('REQUEST_PATH', 'search', '/actions/configSave') | list | length - ) > 0 - - 'result.deployment.response | to_json is search(''"type": "switch"'')' + ) == 0 + - 'result.deployment.response | to_json is search(''switchIds'')' tags: merge # TC22 - Interaction: deploy=true + config_actions set (config_actions must win) diff --git a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_override.yaml b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_override.yaml index a6578b653..b35b4d80a 100644 --- a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_override.yaml +++ b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_override.yaml @@ -267,25 +267,24 @@ - (result.deployment_needed | default(result.deployment.deployment_needed | default(false))) | bool == true tags: override -- name: OVERRIDE - TC5 - ASSERT - Verify config-save and deploy API traces +- name: OVERRIDE - TC5 - ASSERT - Verify switch-level deploy API traces ansible.builtin.assert: that: - result.deployment.response is defined - - (result.deployment.response | length) >= 2 - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/configSave') + | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') | list | length ) > 0 - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/deploy') + | selectattr('REQUEST_PATH', 'search', '/actions/configSave') | list | length - ) > 0 + ) == 0 tags: override - name: OVERRIDE - TC5 - GATHER - Verify pair exists after deploy flow diff --git a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_replace.yaml b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_replace.yaml index dc5cf59ba..56b3897a1 100644 --- a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_replace.yaml +++ b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_replace.yaml @@ -218,25 +218,24 @@ - (result.deployment_needed | default(result.deployment.deployment_needed | default(false))) | bool == true tags: replace -- name: REPLACE - TC4 - ASSERT - Verify config-save and deploy API traces +- name: REPLACE - TC4 - ASSERT - Verify switch-level deploy API traces ansible.builtin.assert: that: - result.deployment.response is defined - - (result.deployment.response | length) >= 2 - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/configSave') + | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') | list | length ) > 0 - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/deploy') + | selectattr('REQUEST_PATH', 'search', '/actions/configSave') | list | length - ) > 0 + ) == 0 tags: replace - name: REPLACE - TC4 - GATHER - Verify pair exists after deploy flow @@ -410,21 +409,20 @@ - result.deployment.config_actions.deploy == true - result.deployment.config_actions.type == "switch" - result.deployment.response is defined - - (result.deployment.response | length) >= 2 - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/configSave') + | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') | list | length ) > 0 - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/deploy') + | selectattr('REQUEST_PATH', 'search', '/actions/configSave') | list | length - ) > 0 + ) == 0 tags: replace # TC7 - check_mode should not apply replace configuration changes From 0e7087a03224953803a18feb5b8f8a4403a60e7b Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Thu, 2 Jul 2026 19:21:05 +0530 Subject: [PATCH 02/14] Adding UT to deploy --- .../module_utils/manage_vpc_pair/deploy.py | 1 - .../test_manage_vpc_pair_deploy.py | 154 ++++++++++++++++++ 2 files changed, 154 insertions(+), 1 deletion(-) create mode 100644 tests/unit/module_utils/test_manage_vpc_pair_deploy.py diff --git a/plugins/module_utils/manage_vpc_pair/deploy.py b/plugins/module_utils/manage_vpc_pair/deploy.py index 04447ae29..171bdc664 100644 --- a/plugins/module_utils/manage_vpc_pair/deploy.py +++ b/plugins/module_utils/manage_vpc_pair/deploy.py @@ -109,7 +109,6 @@ def _is_non_fatal_config_save_error(error: NDModuleError) -> bool: non_fatal_signatures = ( "vpc fabric peering is not supported", "vpcsanitycheck", - "unexpected error generating vpc configuration", ) return any(signature in message for signature in non_fatal_signatures) diff --git a/tests/unit/module_utils/test_manage_vpc_pair_deploy.py b/tests/unit/module_utils/test_manage_vpc_pair_deploy.py new file mode 100644 index 000000000..de3b21e50 --- /dev/null +++ b/tests/unit/module_utils/test_manage_vpc_pair_deploy.py @@ -0,0 +1,154 @@ +# -*- coding: utf-8 -*- + +# Copyright: (c) 2026, Sivakami S +# GNU General Public License v3.0+ (see LICENSE or https://www.gnu.org/licenses/gpl-3.0.txt) + +from __future__ import absolute_import, annotations, division, print_function + +__metaclass__ = type + +from unittest.mock import patch + +import pytest + +from ansible_collections.cisco.nd.plugins.module_utils.manage_vpc_pair.exceptions import ( + VpcPairResourceError, +) +from ansible_collections.cisco.nd.plugins.module_utils.manage_vpc_pair import deploy + + +class _FakeModule: + def __init__(self, params, check_mode=False): + self.params = params + self.check_mode = check_mode + self.warnings = [] + + def warn(self, msg): + self.warnings.append(msg) + + +class _FakeNrm: + def __init__(self, module): + self.module = module + + +class _FakeNDModuleV2: + """Records every request issued during deploy for assertions.""" + + calls = [] + fail_on_path = None + fail_message = "request failed" + fail_status = 500 + + def __init__(self, module): + self.module = module + self.status = 200 + + def request(self, path, verb, payload): + type(self).calls.append({"path": path, "verb": verb, "payload": payload}) + if type(self).fail_on_path and type(self).fail_on_path in path: + raise deploy.NDModuleError(type(self).fail_message, status=type(self).fail_status) + return {} + + +@pytest.fixture(autouse=True) +def _reset_calls(): + _FakeNDModuleV2.calls = [] + _FakeNDModuleV2.fail_on_path = None + _FakeNDModuleV2.fail_message = "request failed" + _FakeNDModuleV2.fail_status = 500 + yield + _FakeNDModuleV2.calls = [] + _FakeNDModuleV2.fail_on_path = None + _FakeNDModuleV2.fail_message = "request failed" + _FakeNDModuleV2.fail_status = 500 + + +def _build_nrm(config_type="switch", check_mode=False, pending_create=None): + params = { + "fabric_name": "myFabric", + "config_actions": {"save": True, "deploy": True, "type": config_type}, + "_pending_create": pending_create if pending_create is not None else [], + "_pending_delete": [], + "_not_in_sync_pairs": [], + } + return _FakeNrm(_FakeModule(params, check_mode=check_mode)) + + +def test_switch_scoped_deploy_targets_affected_switches(): + """type=switch deploys via switchActions/deploy with both peer serials (issue #335).""" + nrm = _build_nrm(config_type="switch") + # Fresh create: query-phase pending lists are empty; serials come from the + # authoritative per-run diff in result. + result = {"changed": True, "created": [("FOC1", "FOC2")], "deleted": [], "updated": []} + + with patch.object(deploy, "NDModuleV2", _FakeNDModuleV2): + deploy.custom_vpc_deploy(nrm, "myFabric", result) + + assert len(_FakeNDModuleV2.calls) == 1 + call = _FakeNDModuleV2.calls[0] + assert call["path"] == "/api/v1/manage/fabrics/myFabric/switchActions/deploy" + assert call["payload"] == {"switchIds": ["FOC1", "FOC2"]} + # No fabric-level configSave is issued for the switch-scoped path. + assert all("configSave" not in c["path"] for c in _FakeNDModuleV2.calls) + + +def test_switch_scoped_deploy_uses_pending_when_no_diff(): + """Deploy-only no-diff case resolves serials from query-phase pending context.""" + nrm = _build_nrm(config_type="switch") + nrm.module.params["_not_in_sync_pairs"] = [{"switchId": "FOC9", "peerSwitchId": "FOC8"}] + result = {"changed": False, "created": [], "deleted": [], "updated": []} + + with patch.object(deploy, "NDModuleV2", _FakeNDModuleV2): + deploy.custom_vpc_deploy(nrm, "myFabric", result) + + assert len(_FakeNDModuleV2.calls) == 1 + call = _FakeNDModuleV2.calls[0] + assert call["path"] == "/api/v1/manage/fabrics/myFabric/switchActions/deploy" + assert call["payload"] == {"switchIds": ["FOC9", "FOC8"]} + + +def test_global_scoped_deploy_uses_fabric_actions(): + """type=global retains the fabric-level configSave + deploy flow.""" + nrm = _build_nrm(config_type="global") + result = {"changed": True, "created": [("FOC1", "FOC2")], "deleted": [], "updated": []} + + with patch.object(deploy, "NDModuleV2", _FakeNDModuleV2): + deploy.custom_vpc_deploy(nrm, "myFabric", result) + + paths = [c["path"] for c in _FakeNDModuleV2.calls] + assert any("actions/configSave" in p for p in paths) + assert any("actions/deploy" in p for p in paths) + assert all("switchActions/deploy" not in p for p in paths) + + +def test_global_config_save_vpc_generation_error_is_fatal(): + """The issue #335 configSave 500 must not be masked and followed by deploy.""" + nrm = _build_nrm(config_type="global") + result = {"changed": True, "created": [("FOC1", "FOC2")], "deleted": [], "updated": []} + _FakeNDModuleV2.fail_on_path = "actions/configSave" + _FakeNDModuleV2.fail_message = ( + "ND Error 500: [FOC1/NX_1] - Unexpected error generating vPC configuration. " + "Last Status [Peer Link Interface]
Switch [NX_1/FOC1]: Member port Ethernet1/1 " + "is a member of VPC_PAIR, remove Ethernet1/1 from VPC_PAIR first." + ) + + with patch.object(deploy, "NDModuleV2", _FakeNDModuleV2): + with pytest.raises(VpcPairResourceError): + deploy.custom_vpc_deploy(nrm, "myFabric", result) + + paths = [c["path"] for c in _FakeNDModuleV2.calls] + assert any("actions/configSave" in p for p in paths) + assert all("actions/deploy" not in p for p in paths) + assert all("switchActions/deploy" not in p for p in paths) + + +def test_switch_scoped_check_mode_previews_switch_deploy(): + """check_mode preview for type=switch references the switch-scoped endpoint.""" + nrm = _build_nrm(config_type="switch", check_mode=True) + result = {"changed": True, "created": [("FOC1", "FOC2")], "deleted": [], "updated": []} + + info = deploy.custom_vpc_deploy(nrm, "myFabric", result) + + assert info["deployment_needed"] is True + assert any("switchActions/deploy" in action for action in info["planned_actions"]) From 1298125977267dd4262c34f2f81b83190ef8c855 Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Thu, 9 Jul 2026 18:53:38 +0530 Subject: [PATCH 03/14] Block unsupported vPC pair details on VXLAN fabrics --- .../module_utils/manage_vpc_pair/actions.py | 156 ++++++++++++++++-- .../targets/nd_vpc_pair/tasks/base_tasks.yaml | 11 ++ .../nd_vpc_pair/tasks/nd_vpc_pair_merge.yaml | 55 +++++- 3 files changed, 203 insertions(+), 19 deletions(-) diff --git a/plugins/module_utils/manage_vpc_pair/actions.py b/plugins/module_utils/manage_vpc_pair/actions.py index aa74683ac..5b2dde600 100644 --- a/plugins/module_utils/manage_vpc_pair/actions.py +++ b/plugins/module_utils/manage_vpc_pair/actions.py @@ -5,7 +5,9 @@ from __future__ import annotations +import re from typing import Any +from urllib.parse import quote from ansible_collections.cisco.nd.plugins.module_utils.enums import HttpVerbEnum from ansible_collections.cisco.nd.plugins.module_utils.manage_vpc_pair.enums import ( @@ -40,6 +42,122 @@ ) +_BLOCKED_FABRIC_TYPE_TOKENS_FOR_VPC_PAIR_DETAILS = {"vxlanibgp", "vxlanebgp"} + + +def _normalize_fabric_type_token(value: Any) -> str: + """ + Normalize fabric-type strings for stable comparisons. + + Examples: + - "vxlanIbgp" -> "vxlanibgp" + - "VXLAN_EBGP" -> "vxlanebgp" + """ + if not isinstance(value, str): + return "" + return re.sub(r"[^a-z0-9]", "", value.strip().lower()) + + +def _resolve_fabric_type_token(nd_v2: NDModuleV2, fabric_name: str, module: Any) -> str: + """ + Resolve and cache the normalized fabric-type token for current run. + + Best-effort lookup from fabric details endpoint. Returns empty string when + fabric type cannot be determined. + """ + cached = module.params.get("_fabric_type_token") + if isinstance(cached, str) and cached: + return cached + + details_path = f"/api/v1/manage/fabrics/{quote(fabric_name, safe='')}" + try: + details = nd_v2.request(details_path, HttpVerbEnum.GET) + except Exception as exc: + module.warn( + f"Unable to determine fabric type for '{fabric_name}' while validating vpc_pair_details: " + f"{str(exc).splitlines()[0]}" + ) + return "" + + if not isinstance(details, dict): + return "" + + candidates: list[str] = [] + for key in ("fabricType", "fabricTechnology", "type", "category"): + value = details.get(key) + if isinstance(value, str): + candidates.append(value) + + management = details.get("management") + if isinstance(management, dict): + mgmt_type = management.get("type") + if isinstance(mgmt_type, str): + candidates.append(mgmt_type) + + properties = details.get("properties") + if isinstance(properties, dict): + for key in ("fabricType", "fabricTechnology", "type"): + value = properties.get(key) + if isinstance(value, str): + candidates.append(value) + + for candidate in candidates: + token = _normalize_fabric_type_token(candidate) + if token: + module.params["_fabric_type_token"] = token + return token + + return "" + + +def _get_proposed_vpc_pair_details(proposed_config: Any) -> Any: + """Return proposed vpc_pair_details (snake_case or API key), if present.""" + if not isinstance(proposed_config, dict): + return None + + details = proposed_config.get("vpc_pair_details") + if details is None: + details = proposed_config.get(VpcFieldNames.VPC_PAIR_DETAILS) + return details + + +def _validate_vpc_pair_details_fabric_support( + nrm: Any, + nd_v2: NDModuleV2, + fabric_name: str, + allow_lookup: bool = True, +) -> None: + """ + Block vpc_pair_details on fabrics where it is not supported. + + Currently blocked for VXLAN iBGP/eBGP fabrics. Allowed for other fabric + types (for example External/ISN-style deployments) where the controller + supports the additional settings. + """ + proposed_details = _get_proposed_vpc_pair_details(nrm.proposed_config) + if not proposed_details: + return + + token = nrm.module.params.get("_fabric_type_token") + if not isinstance(token, str) or not token: + if not allow_lookup: + return + token = _resolve_fabric_type_token(nd_v2, fabric_name, nrm.module) + + normalized_token = _normalize_fabric_type_token(token) + if normalized_token in _BLOCKED_FABRIC_TYPE_TOKENS_FOR_VPC_PAIR_DETAILS: + _raise_vpc_error( + msg=( + "Invalid nd_manage_vpc_pair input: 'vpc_pair_details' is not supported " + "for iBGP/eBGP VXLAN fabrics. Use only peer switch IDs and " + "'use_virtual_peer_link' for this fabric type." + ), + fabric=fabric_name, + fabric_type=token, + unsupported_field="vpc_pair_details", + ) + + def _build_compare_payloads(nrm: Any) -> tuple[dict[str, Any], dict[str, Any]]: """ Build normalized want/have payloads for idempotence comparisons. @@ -104,9 +222,6 @@ def custom_vpc_create(nrm: Any) -> dict[str, Any] | None: ValueError: If fabric_name or switch_id is not provided AnsibleModule.fail_json: If validation fails """ - if nrm.module.check_mode: - return nrm.proposed_config - fabric_name = nrm.module.params.get("fabric_name") switch_id = nrm.proposed_config.get(VpcFieldNames.SWITCH_ID) peer_switch_id = nrm.proposed_config.get(VpcFieldNames.PEER_SWITCH_ID) @@ -119,6 +234,20 @@ def custom_vpc_create(nrm: Any) -> dict[str, Any] | None: if not peer_switch_id: raise ValueError("peer_switch_id is required but was not provided") + # Initialize RestSend via NDModuleV2. + # Validate details support before create to fail early on unsupported + # fabric types (iBGP/eBGP VXLAN). + nd_v2 = NDModuleV2(nrm.module) + _validate_vpc_pair_details_fabric_support( + nrm=nrm, + nd_v2=nd_v2, + fabric_name=fabric_name, + allow_lookup=not nrm.module.check_mode, + ) + + if nrm.module.check_mode: + return nrm.proposed_config + # Validation Step 1: both switches must exist in discovered fabric inventory. _validate_switches_exist_in_fabric( nrm=nrm, @@ -141,8 +270,6 @@ def custom_vpc_create(nrm: Any) -> dict[str, Any] | None: nrm.module.warn(f"VPC pair {nrm.current_identifier} already exists in desired state - skipping create") return nrm.existing_config - # Initialize RestSend via NDModuleV2 - nd_v2 = NDModuleV2(nrm.module) use_virtual_peer_link = nrm.proposed_config.get(VpcFieldNames.USE_VIRTUAL_PEER_LINK, False) # Validate pairing support using dedicated endpoint. @@ -238,9 +365,6 @@ def custom_vpc_update(nrm: Any) -> dict[str, Any] | None: Raises: ValueError: If fabric_name or switch_id is not provided """ - if nrm.module.check_mode: - return nrm.proposed_config - fabric_name = nrm.module.params.get("fabric_name") switch_id = nrm.proposed_config.get(VpcFieldNames.SWITCH_ID) peer_switch_id = nrm.proposed_config.get(VpcFieldNames.PEER_SWITCH_ID) @@ -253,6 +377,20 @@ def custom_vpc_update(nrm: Any) -> dict[str, Any] | None: if not peer_switch_id: raise ValueError("peer_switch_id is required but was not provided") + # Initialize RestSend via NDModuleV2. + # Validate details support before update to fail early on unsupported + # fabric types (iBGP/eBGP VXLAN). + nd_v2 = NDModuleV2(nrm.module) + _validate_vpc_pair_details_fabric_support( + nrm=nrm, + nd_v2=nd_v2, + fabric_name=fabric_name, + allow_lookup=not nrm.module.check_mode, + ) + + if nrm.module.check_mode: + return nrm.proposed_config + # Validation Step 1: both switches must exist in discovered fabric inventory. _validate_switches_exist_in_fabric( nrm=nrm, @@ -278,8 +416,6 @@ def custom_vpc_update(nrm: Any) -> dict[str, Any] | None: nrm.module.warn(f"VPC pair {nrm.current_identifier} is already in desired state - skipping update") return nrm.existing_config - # Initialize RestSend via NDModuleV2 - nd_v2 = NDModuleV2(nrm.module) use_virtual_peer_link = nrm.proposed_config.get(VpcFieldNames.USE_VIRTUAL_PEER_LINK, False) # Validate fabric peering support if virtual peer link is requested. diff --git a/tests/integration/targets/nd_vpc_pair/tasks/base_tasks.yaml b/tests/integration/targets/nd_vpc_pair/tasks/base_tasks.yaml index 4959854b2..5aea48883 100644 --- a/tests/integration/targets/nd_vpc_pair/tasks/base_tasks.yaml +++ b/tests/integration/targets/nd_vpc_pair/tasks/base_tasks.yaml @@ -25,6 +25,17 @@ test_switch1: "{{ switch1_serial }}" test_switch2: "{{ switch2_serial }}" test_fabric_type: "{{ fabric_type | default('LANClassic') }}" + test_fabric_type_normalized: "{{ (fabric_type | default('LANClassic') | string | lower) }}" + vpc_pair_details_disallowed_fabric: >- + {{ + (fabric_type | default('LANClassic') | string | lower) + in ['vxlanfabric', 'vxlanibgp', 'vxlanebgp'] + }} + vpc_pair_details_expected_supported: >- + {{ + (fabric_type | default('LANClassic') | string | lower) + in ['lanclassic', 'external', 'isn'] + }} deploy_local: true delegate_to: localhost diff --git a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_merge.yaml b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_merge.yaml index a36f9e64f..0316e5310 100644 --- a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_merge.yaml +++ b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_merge.yaml @@ -624,7 +624,7 @@ use_virtual_peer_link: false register: result when: - - test_fabric_type == "LANClassic" + - vpc_pair_details_expected_supported | default(false) | bool - supports_vpc_fabric_peering | default(true) | bool tags: merge @@ -634,7 +634,7 @@ - result.failed == false - result.changed == true when: - - test_fabric_type == "LANClassic" + - vpc_pair_details_expected_supported | default(false) | bool - supports_vpc_fabric_peering | default(true) | bool tags: merge @@ -644,7 +644,7 @@ method: get register: tc7b_vpc_pair_direct when: - - test_fabric_type == "LANClassic" + - vpc_pair_details_expected_supported | default(false) | bool - supports_vpc_fabric_peering | default(true) | bool tags: merge @@ -658,7 +658,7 @@ mode: "exists" register: tc7b_validation when: - - test_fabric_type == "LANClassic" + - vpc_pair_details_expected_supported | default(false) | bool - supports_vpc_fabric_peering | default(true) | bool tags: merge @@ -669,7 +669,7 @@ - tc7b_vpc_pair_direct.current is mapping - tc7b_validation.failed == false when: - - test_fabric_type == "LANClassic" + - vpc_pair_details_expected_supported | default(false) | bool - supports_vpc_fabric_peering | default(true) | bool tags: merge @@ -1776,7 +1776,7 @@ vpc_pair_details: "{{ vpc_pair_details_extended }}" register: result when: - - test_fabric_type == "LANClassic" + - vpc_pair_details_expected_supported | default(false) | bool - supports_vpc_fabric_peering | default(true) | bool tags: merge @@ -1786,7 +1786,7 @@ method: get register: tc23_vpc_pair_direct when: - - test_fabric_type == "LANClassic" + - vpc_pair_details_expected_supported | default(false) | bool - supports_vpc_fabric_peering | default(true) | bool tags: merge @@ -1802,7 +1802,7 @@ validate_vpc_pair_details: true register: tc23_validation when: - - test_fabric_type == "LANClassic" + - vpc_pair_details_expected_supported | default(false) | bool - supports_vpc_fabric_peering | default(true) | bool tags: merge @@ -1815,10 +1815,47 @@ - tc23_vpc_pair_direct.current is mapping - tc23_validation.failed == false when: - - test_fabric_type == "LANClassic" + - vpc_pair_details_expected_supported | default(false) | bool - supports_vpc_fabric_peering | default(true) | bool tags: merge +# TC23b - Reject vpc_pair_details on iBGP/eBGP-type VXLAN fabrics +- name: MERGE - TC23b - MERGE - Reject vpc_pair_details for disallowed fabric types + cisco.nd.nd_manage_vpc_pair: + <<: *nd_info + fabric_name: "{{ test_fabric }}" + state: merged + config_actions: + save: false + deploy: false + type: switch + config: + - peer1_switch_id: "{{ test_switch1 }}" + peer2_switch_id: "{{ test_switch2 }}" + use_virtual_peer_link: false + vpc_pair_details: + type: default + domain_id: 1 + keep_alive_vrf: management + switch_keep_alive_local_ip: "192.0.2.11" + peer_switch_keep_alive_local_ip: "192.0.2.12" + register: tc23b_result + failed_when: false + when: + - vpc_pair_details_disallowed_fabric | default(false) | bool + tags: merge + +- name: MERGE - TC23b - ASSERT - Verify vpc_pair_details is blocked on disallowed fabrics + ansible.builtin.assert: + that: + - tc23b_result.failed == true + - tc23b_result.msg is defined + - tc23b_result.msg is search('vpc_pair_details') + - tc23b_result.msg is search('not supported') + when: + - vpc_pair_details_disallowed_fabric | default(false) | bool + tags: merge + ############################################## ## CLEAN-UP ## ############################################## From c9aacd938f2f0f74a20a664d2d6afa0f8cd57290 Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Thu, 9 Jul 2026 19:05:24 +0530 Subject: [PATCH 04/14] Revert "Fix vPC pair physical interface handling" This reverts commit b49086b7343fca69d9bbbd4c8d985cba84c3e4cd. --- .../module_utils/manage_vpc_pair/deploy.py | 315 +++++------------- .../nd_vpc_pair/tasks/nd_vpc_pair_delete.yaml | 16 +- .../nd_vpc_pair/tasks/nd_vpc_pair_merge.yaml | 27 +- .../tasks/nd_vpc_pair_override.yaml | 9 +- .../tasks/nd_vpc_pair_replace.yaml | 16 +- .../test_manage_vpc_pair_deploy.py | 154 --------- 6 files changed, 115 insertions(+), 422 deletions(-) delete mode 100644 tests/unit/module_utils/test_manage_vpc_pair_deploy.py diff --git a/plugins/module_utils/manage_vpc_pair/deploy.py b/plugins/module_utils/manage_vpc_pair/deploy.py index 171bdc664..34fffb3fe 100644 --- a/plugins/module_utils/manage_vpc_pair/deploy.py +++ b/plugins/module_utils/manage_vpc_pair/deploy.py @@ -12,13 +12,6 @@ _raise_vpc_error, get_config_actions, ) -from ansible_collections.cisco.nd.plugins.module_utils.manage_vpc_pair.enums import ( - VpcFieldNames, -) -from ansible_collections.cisco.nd.plugins.module_utils.endpoints.v1.manage.manage_fabrics_switchactions import ( - EpManageFabricsSwitchActionsDeployPost, -) -from ansible_collections.cisco.nd.plugins.module_utils.enums import HttpVerbEnum from ansible_collections.cisco.nd.plugins.module_utils.nd_v2 import ( NDModule as NDModuleV2, NDModuleError, @@ -109,144 +102,104 @@ def _is_non_fatal_config_save_error(error: NDModuleError) -> bool: non_fatal_signatures = ( "vpc fabric peering is not supported", "vpcsanitycheck", + "unexpected error generating vpc configuration", ) return any(signature in message for signature in non_fatal_signatures) -def _collect_affected_switch_ids(nrm: Any, result: dict[str, Any]) -> list[str]: +def custom_vpc_deploy(nrm: Any, fabric_name: str, result: dict[str, Any]) -> dict[str, Any]: """ - Collect the serial numbers of switches affected by this run. - - Switch-scoped deploy targets only the switches that changed (created, - updated, deleted, or currently not in-sync) instead of the whole fabric. - - Serials are gathered from two complementary sources so that every write - operation is covered, including a fresh create where the query-phase - pending lists are still empty: + Custom save/deploy action handler for vPC fabric changes using RestSend. - 1. The authoritative per-run diff in ``result`` (``created`` / ``deleted`` - identifiers and ``updated`` entries). Each identifier is a - ``(switch_id, peer_switch_id)`` tuple of switch serials. - 2. The query-phase ``_pending_create`` / ``_pending_delete`` / - ``_not_in_sync_pairs`` contexts, which cover the deploy-only, - no-diff case (pair already exists but is not in-sync on the controller). + - Smart action decision (_needs_deployment) + - Optional Step 1: Save fabric configuration + - Optional Step 2: Deploy fabric with forceShowRun=true + - Proper error handling with NDModuleError + - Results aggregation + - Executes only if there are actual changes or pending operations Args: nrm: NDStateMachine instance - result: Module result dictionary with class diff information + fabric_name: Fabric name to deploy + result: Module result dictionary to check for changes Returns: - Ordered, de-duplicated list of switch serial numbers. - """ - switch_ids: list[str] = [] - seen: set[str] = set() - - def _add(serial: Any) -> None: - if serial and serial not in seen: - seen.add(serial) - switch_ids.append(serial) - - def _add_identifier(identifier: Any) -> None: - # Identifiers are (switch_id, peer_switch_id) serial tuples/lists. - if isinstance(identifier, (list, tuple)): - for serial in identifier: - _add(serial) - - # Source 1: authoritative per-run diff. - if isinstance(result, dict): - for identifier in result.get("created", []) or []: - _add_identifier(identifier) - for identifier in result.get("deleted", []) or []: - _add_identifier(identifier) - for entry in result.get("updated", []) or []: - if isinstance(entry, dict): - _add_identifier(entry.get("identifier")) - else: - _add_identifier(entry) - - # Source 2: query-phase pending / not-in-sync contexts. - for key in ("_pending_create", "_pending_delete", "_not_in_sync_pairs"): - for pair in nrm.module.params.get(key, []) or []: - _add(pair.get(VpcFieldNames.SWITCH_ID)) - _add(pair.get(VpcFieldNames.PEER_SWITCH_ID)) - - return switch_ids - - -def _build_switch_deploy_path(fabric_name: str) -> str: - """ - Build the switch-scoped deploy endpoint path for the given fabric. + Save/deploy result dictionary - POST /api/v1/manage/fabrics/{fabricName}/switchActions/deploy + Raises: + NDModuleError: If deployment fails """ - endpoint = EpManageFabricsSwitchActionsDeployPost(fabric_name=fabric_name) - return endpoint.path + config_actions = get_config_actions(nrm.module) + save_enabled = bool(config_actions.get("save", True)) + deploy_enabled = bool(config_actions.get("deploy", True)) + action_type = config_actions.get("type", "switch") + action_payload = {"type": action_type} + # Defensive runtime validation (model validation already enforces this). + if deploy_enabled and not save_enabled: + _raise_vpc_error(msg="Invalid config_actions: deploy=true requires save=true") -def _switch_level_deploy(nd_v2: NDModuleV2, fabric_name: str, switch_ids: list[str], results: Any) -> None: - """ - Generate and push configuration for specific switches. + if not save_enabled and not deploy_enabled: + return { + "msg": "Config actions disabled (save=false, deploy=false), skipping config save/deploy", + "fabric": fabric_name, + "deployment_needed": False, + "changed": False, + "config_actions": config_actions, + } - Uses the switch-scoped ``switchActions/deploy`` endpoint, which generates - and deploys configuration only for the supplied switch serials. This avoids - the fabric-level ``actions/configSave`` path that returns HTTP 500 for vPC - pairs configured with a physical peer-link (issue #335). + # Smart deployment decision (from Common.needs_deployment) + if not _needs_deployment(result, nrm): + return { + "msg": ("No configuration changes, pending operations, or out-of-sync pairs " "detected, skipping config actions"), + "fabric": fabric_name, + "deployment_needed": False, + "changed": False, + "config_actions": config_actions, + } - Args: - nd_v2: NDModuleV2 RestSend wrapper. - fabric_name: Fabric name. - switch_ids: Switch serial numbers to deploy. - results: Results aggregator. + if nrm.module.check_mode: + # check_mode deployment preview + pending_create = nrm.module.params.get("_pending_create", []) + pending_delete = nrm.module.params.get("_pending_delete", []) + not_in_sync_pairs = nrm.module.params.get("_not_in_sync_pairs", []) + planned_actions = [] + if save_enabled: + save_path = FabricUtils.build_config_save_path(fabric_name) + planned_actions.append(f"POST {save_path} payload={action_payload}") + if deploy_enabled: + deploy_path = FabricUtils.build_config_deploy_path(fabric_name, force_show_run=True) + planned_actions.append(f"POST {deploy_path} payload={action_payload}") + if save_enabled and deploy_enabled: + preview_msg = "CHECK MODE: Would save and deploy fabric configuration" + elif save_enabled: + preview_msg = "CHECK MODE: Would save fabric configuration" + else: + preview_msg = "CHECK MODE: Would deploy fabric configuration" - Raises: - VpcPairResourceError: If the deploy call fails. - """ - deploy_path = _build_switch_deploy_path(fabric_name) - payload = {"switchIds": switch_ids} - - try: - nd_v2.request(deploy_path, HttpVerbEnum.POST, payload) - register_action_api_call( - results=results, - request_path=deploy_path, - payload=payload, - return_code=nd_v2.status, - message="Switch deployment successful", - success=True, - changed=True, - ) - except NDModuleError as error: - register_action_api_call( - results=results, - request_path=deploy_path, - payload=payload, - return_code=error.status, - message=error.msg, - success=False, - changed=False, - ) - results.build_final_result() - final_result = dict(results.final_result) - final_msg = final_result.pop("msg", "Switch deployment failed") - _raise_vpc_error(msg=final_msg, **final_result) - - -def _fabric_level_deploy( - nd_v2: NDModuleV2, - nrm: Any, - fabric_name: str, - action_payload: dict[str, Any], - save_enabled: bool, - deploy_enabled: bool, - results: Any, -) -> None: - """ - Fabric-level (global) config save + deploy flow. + deployment_info = { + "msg": preview_msg, + "fabric": fabric_name, + "deployment_needed": True, + "changed": True, + "would_save": save_enabled, + "would_deploy": deploy_enabled, + "config_actions": config_actions, + "deployment_decision_factors": { + "diff_has_changes": _has_explicit_diff_changes(result), + "pending_create_operations": len(pending_create), + "pending_delete_operations": len(pending_delete), + "not_in_sync_pairs": len(not_in_sync_pairs), + "actual_changes": result.get("changed", False), + }, + "planned_actions": planned_actions, + } + return deployment_info - Retained for ``config_actions.type == "global"`` and as a fallback when no - affected switch serials can be resolved for a switch-scoped deploy. - """ + # Initialize RestSend via NDModuleV2 + nd_v2 = NDModuleV2(nrm.module) fabric_utils = FabricUtils(nd_v2, fabric_name) + results = Results() # Step 1: Save config if save_enabled: @@ -329,118 +282,6 @@ def _fabric_level_deploy( final_msg = final_result.pop("msg", "Fabric deployment failed") _raise_vpc_error(msg=final_msg, **final_result) - -def custom_vpc_deploy(nrm: Any, fabric_name: str, result: dict[str, Any]) -> dict[str, Any]: - """ - Custom save/deploy action handler for vPC fabric changes using RestSend. - - - Smart action decision (_needs_deployment) - - Optional Step 1: Save fabric configuration - - Optional Step 2: Deploy fabric with forceShowRun=true - - Proper error handling with NDModuleError - - Results aggregation - - Executes only if there are actual changes or pending operations - - Args: - nrm: NDStateMachine instance - fabric_name: Fabric name to deploy - result: Module result dictionary to check for changes - - Returns: - Save/deploy result dictionary - - Raises: - NDModuleError: If deployment fails - """ - config_actions = get_config_actions(nrm.module) - save_enabled = bool(config_actions.get("save", True)) - deploy_enabled = bool(config_actions.get("deploy", True)) - action_type = config_actions.get("type", "switch") - action_payload = {"type": action_type} - - # Defensive runtime validation (model validation already enforces this). - if deploy_enabled and not save_enabled: - _raise_vpc_error(msg="Invalid config_actions: deploy=true requires save=true") - - if not save_enabled and not deploy_enabled: - return { - "msg": "Config actions disabled (save=false, deploy=false), skipping config save/deploy", - "fabric": fabric_name, - "deployment_needed": False, - "changed": False, - "config_actions": config_actions, - } - - # Smart deployment decision (from Common.needs_deployment) - if not _needs_deployment(result, nrm): - return { - "msg": ("No configuration changes, pending operations, or out-of-sync pairs " "detected, skipping config actions"), - "fabric": fabric_name, - "deployment_needed": False, - "changed": False, - "config_actions": config_actions, - } - - if nrm.module.check_mode: - # check_mode deployment preview - pending_create = nrm.module.params.get("_pending_create", []) - pending_delete = nrm.module.params.get("_pending_delete", []) - not_in_sync_pairs = nrm.module.params.get("_not_in_sync_pairs", []) - planned_actions = [] - switch_ids = _collect_affected_switch_ids(nrm, result) if action_type == "switch" and deploy_enabled else [] - if switch_ids: - deploy_path = _build_switch_deploy_path(fabric_name) - planned_actions.append(f"POST {deploy_path} payload={{'switchIds': {switch_ids}}}") - preview_msg = "CHECK MODE: Would deploy configuration for affected switches" - else: - if save_enabled: - save_path = FabricUtils.build_config_save_path(fabric_name) - planned_actions.append(f"POST {save_path} payload={action_payload}") - if deploy_enabled: - deploy_path = FabricUtils.build_config_deploy_path(fabric_name, force_show_run=True) - planned_actions.append(f"POST {deploy_path} payload={action_payload}") - if save_enabled and deploy_enabled: - preview_msg = "CHECK MODE: Would save and deploy fabric configuration" - elif save_enabled: - preview_msg = "CHECK MODE: Would save fabric configuration" - else: - preview_msg = "CHECK MODE: Would deploy fabric configuration" - - deployment_info = { - "msg": preview_msg, - "fabric": fabric_name, - "deployment_needed": True, - "changed": True, - "would_save": save_enabled, - "would_deploy": deploy_enabled, - "config_actions": config_actions, - "deployment_decision_factors": { - "diff_has_changes": _has_explicit_diff_changes(result), - "pending_create_operations": len(pending_create), - "pending_delete_operations": len(pending_delete), - "not_in_sync_pairs": len(not_in_sync_pairs), - "actual_changes": result.get("changed", False), - }, - "planned_actions": planned_actions, - } - return deployment_info - - # Initialize RestSend via NDModuleV2 - nd_v2 = NDModuleV2(nrm.module) - results = Results() - - # Switch-scoped deploy (config_actions.type == "switch", the default) targets - # only the affected switches via switchActions/deploy, which generates and - # pushes config in a single call. This avoids the fabric-level actions/configSave - # path that returns HTTP 500 for vPC pairs with a physical peer-link (issue #335). - # The fabric-level (global) flow is retained for type == "global" and as a - # fallback when no affected switch serials can be resolved. - switch_ids = _collect_affected_switch_ids(nrm, result) if action_type == "switch" and deploy_enabled else [] - if switch_ids: - _switch_level_deploy(nd_v2, fabric_name, switch_ids, results) - else: - _fabric_level_deploy(nd_v2, nrm, fabric_name, action_payload, save_enabled, deploy_enabled, results) - # Build final result results.build_final_result() final_result = dict(results.final_result) diff --git a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_delete.yaml b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_delete.yaml index 93869e643..0ec0a678e 100644 --- a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_delete.yaml +++ b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_delete.yaml @@ -184,24 +184,25 @@ - (result.deployment_needed | default(result.deployment.deployment_needed | default(false))) | bool == true tags: delete -- name: DELETE - TC2b - ASSERT - Verify switch-level deploy API traces +- name: DELETE - TC2b - ASSERT - Verify config-save and deploy API traces ansible.builtin.assert: that: - result.deployment.response is defined + - (result.deployment.response | length) >= 2 - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') + | selectattr('REQUEST_PATH', 'search', '/actions/configSave') | list | length ) > 0 - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/configSave') + | selectattr('REQUEST_PATH', 'search', '/actions/deploy') | list | length - ) == 0 + ) > 0 tags: delete - name: DELETE - TC2b - GATHER - Verify deploy delete result in ND @@ -430,20 +431,21 @@ - tc6_delete_result.deployment.config_actions.deploy == true - tc6_delete_result.deployment.config_actions.type == "switch" - tc6_delete_result.deployment.response is defined + - (tc6_delete_result.deployment.response | length) >= 2 - > ( tc6_delete_result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') + | selectattr('REQUEST_PATH', 'search', '/actions/configSave') | list | length ) > 0 - > ( tc6_delete_result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/configSave') + | selectattr('REQUEST_PATH', 'search', '/actions/deploy') | list | length - ) == 0 + ) > 0 tags: delete # TC7 - check_mode should not apply delete configuration changes diff --git a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_merge.yaml b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_merge.yaml index 0316e5310..554357056 100644 --- a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_merge.yaml +++ b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_merge.yaml @@ -539,8 +539,8 @@ - > ( (tc6b_deployment_needed and (result.deployment.response is defined) - and ((result.deployment.response | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') | list | length) > 0) - and ((result.deployment.response | selectattr('REQUEST_PATH', 'search', '/actions/configSave') | list | length) == 0)) + and ((result.deployment.response | selectattr('REQUEST_PATH', 'search', '/actions/configSave') | list | length) > 0) + and ((result.deployment.response | selectattr('REQUEST_PATH', 'search', '/actions/deploy') | list | length) > 0)) or ((not tc6b_deployment_needed) and (result.deployment.msg | default('') is search('skipping'))) ) @@ -827,24 +827,25 @@ - (result.deployment_needed | default(result.deployment.deployment_needed | default(false))) | bool == true tags: merge -- name: MERGE - TC10 - ASSERT - Verify switch-level deploy API traces +- name: MERGE - TC10 - ASSERT - Verify config-save and deploy API traces ansible.builtin.assert: that: - result.deployment.response is defined + - (result.deployment.response | length) >= 2 - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') + | selectattr('REQUEST_PATH', 'search', '/actions/configSave') | list | length ) > 0 - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/configSave') + | selectattr('REQUEST_PATH', 'search', '/actions/deploy') | list | length - ) == 0 + ) > 0 tags: merge - name: MERGE - TC10 - GATHER - Query gathered pair after deploy flow @@ -1485,17 +1486,17 @@ - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') + | selectattr('REQUEST_PATH', 'search', '/actions/configSave') | list | length ) > 0 - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/configSave') + | selectattr('REQUEST_PATH', 'search', '/actions/deploy') | list | length - ) == 0 + ) > 0 tags: merge # TC18 - config_actions save-only path (save=true, deploy=false) @@ -1686,18 +1687,18 @@ - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') + | selectattr('REQUEST_PATH', 'search', '/actions/configSave') | list | length ) > 0 - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/configSave') + | selectattr('REQUEST_PATH', 'search', '/actions/deploy') | list | length - ) == 0 - - 'result.deployment.response | to_json is search(''switchIds'')' + ) > 0 + - 'result.deployment.response | to_json is search(''"type": "switch"'')' tags: merge # TC22 - Interaction: deploy=true + config_actions set (config_actions must win) diff --git a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_override.yaml b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_override.yaml index b35b4d80a..a6578b653 100644 --- a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_override.yaml +++ b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_override.yaml @@ -267,24 +267,25 @@ - (result.deployment_needed | default(result.deployment.deployment_needed | default(false))) | bool == true tags: override -- name: OVERRIDE - TC5 - ASSERT - Verify switch-level deploy API traces +- name: OVERRIDE - TC5 - ASSERT - Verify config-save and deploy API traces ansible.builtin.assert: that: - result.deployment.response is defined + - (result.deployment.response | length) >= 2 - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') + | selectattr('REQUEST_PATH', 'search', '/actions/configSave') | list | length ) > 0 - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/configSave') + | selectattr('REQUEST_PATH', 'search', '/actions/deploy') | list | length - ) == 0 + ) > 0 tags: override - name: OVERRIDE - TC5 - GATHER - Verify pair exists after deploy flow diff --git a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_replace.yaml b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_replace.yaml index 56b3897a1..dc5cf59ba 100644 --- a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_replace.yaml +++ b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_replace.yaml @@ -218,24 +218,25 @@ - (result.deployment_needed | default(result.deployment.deployment_needed | default(false))) | bool == true tags: replace -- name: REPLACE - TC4 - ASSERT - Verify switch-level deploy API traces +- name: REPLACE - TC4 - ASSERT - Verify config-save and deploy API traces ansible.builtin.assert: that: - result.deployment.response is defined + - (result.deployment.response | length) >= 2 - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') + | selectattr('REQUEST_PATH', 'search', '/actions/configSave') | list | length ) > 0 - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/configSave') + | selectattr('REQUEST_PATH', 'search', '/actions/deploy') | list | length - ) == 0 + ) > 0 tags: replace - name: REPLACE - TC4 - GATHER - Verify pair exists after deploy flow @@ -409,20 +410,21 @@ - result.deployment.config_actions.deploy == true - result.deployment.config_actions.type == "switch" - result.deployment.response is defined + - (result.deployment.response | length) >= 2 - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') + | selectattr('REQUEST_PATH', 'search', '/actions/configSave') | list | length ) > 0 - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/configSave') + | selectattr('REQUEST_PATH', 'search', '/actions/deploy') | list | length - ) == 0 + ) > 0 tags: replace # TC7 - check_mode should not apply replace configuration changes diff --git a/tests/unit/module_utils/test_manage_vpc_pair_deploy.py b/tests/unit/module_utils/test_manage_vpc_pair_deploy.py deleted file mode 100644 index de3b21e50..000000000 --- a/tests/unit/module_utils/test_manage_vpc_pair_deploy.py +++ /dev/null @@ -1,154 +0,0 @@ -# -*- coding: utf-8 -*- - -# Copyright: (c) 2026, Sivakami S -# GNU General Public License v3.0+ (see LICENSE or https://www.gnu.org/licenses/gpl-3.0.txt) - -from __future__ import absolute_import, annotations, division, print_function - -__metaclass__ = type - -from unittest.mock import patch - -import pytest - -from ansible_collections.cisco.nd.plugins.module_utils.manage_vpc_pair.exceptions import ( - VpcPairResourceError, -) -from ansible_collections.cisco.nd.plugins.module_utils.manage_vpc_pair import deploy - - -class _FakeModule: - def __init__(self, params, check_mode=False): - self.params = params - self.check_mode = check_mode - self.warnings = [] - - def warn(self, msg): - self.warnings.append(msg) - - -class _FakeNrm: - def __init__(self, module): - self.module = module - - -class _FakeNDModuleV2: - """Records every request issued during deploy for assertions.""" - - calls = [] - fail_on_path = None - fail_message = "request failed" - fail_status = 500 - - def __init__(self, module): - self.module = module - self.status = 200 - - def request(self, path, verb, payload): - type(self).calls.append({"path": path, "verb": verb, "payload": payload}) - if type(self).fail_on_path and type(self).fail_on_path in path: - raise deploy.NDModuleError(type(self).fail_message, status=type(self).fail_status) - return {} - - -@pytest.fixture(autouse=True) -def _reset_calls(): - _FakeNDModuleV2.calls = [] - _FakeNDModuleV2.fail_on_path = None - _FakeNDModuleV2.fail_message = "request failed" - _FakeNDModuleV2.fail_status = 500 - yield - _FakeNDModuleV2.calls = [] - _FakeNDModuleV2.fail_on_path = None - _FakeNDModuleV2.fail_message = "request failed" - _FakeNDModuleV2.fail_status = 500 - - -def _build_nrm(config_type="switch", check_mode=False, pending_create=None): - params = { - "fabric_name": "myFabric", - "config_actions": {"save": True, "deploy": True, "type": config_type}, - "_pending_create": pending_create if pending_create is not None else [], - "_pending_delete": [], - "_not_in_sync_pairs": [], - } - return _FakeNrm(_FakeModule(params, check_mode=check_mode)) - - -def test_switch_scoped_deploy_targets_affected_switches(): - """type=switch deploys via switchActions/deploy with both peer serials (issue #335).""" - nrm = _build_nrm(config_type="switch") - # Fresh create: query-phase pending lists are empty; serials come from the - # authoritative per-run diff in result. - result = {"changed": True, "created": [("FOC1", "FOC2")], "deleted": [], "updated": []} - - with patch.object(deploy, "NDModuleV2", _FakeNDModuleV2): - deploy.custom_vpc_deploy(nrm, "myFabric", result) - - assert len(_FakeNDModuleV2.calls) == 1 - call = _FakeNDModuleV2.calls[0] - assert call["path"] == "/api/v1/manage/fabrics/myFabric/switchActions/deploy" - assert call["payload"] == {"switchIds": ["FOC1", "FOC2"]} - # No fabric-level configSave is issued for the switch-scoped path. - assert all("configSave" not in c["path"] for c in _FakeNDModuleV2.calls) - - -def test_switch_scoped_deploy_uses_pending_when_no_diff(): - """Deploy-only no-diff case resolves serials from query-phase pending context.""" - nrm = _build_nrm(config_type="switch") - nrm.module.params["_not_in_sync_pairs"] = [{"switchId": "FOC9", "peerSwitchId": "FOC8"}] - result = {"changed": False, "created": [], "deleted": [], "updated": []} - - with patch.object(deploy, "NDModuleV2", _FakeNDModuleV2): - deploy.custom_vpc_deploy(nrm, "myFabric", result) - - assert len(_FakeNDModuleV2.calls) == 1 - call = _FakeNDModuleV2.calls[0] - assert call["path"] == "/api/v1/manage/fabrics/myFabric/switchActions/deploy" - assert call["payload"] == {"switchIds": ["FOC9", "FOC8"]} - - -def test_global_scoped_deploy_uses_fabric_actions(): - """type=global retains the fabric-level configSave + deploy flow.""" - nrm = _build_nrm(config_type="global") - result = {"changed": True, "created": [("FOC1", "FOC2")], "deleted": [], "updated": []} - - with patch.object(deploy, "NDModuleV2", _FakeNDModuleV2): - deploy.custom_vpc_deploy(nrm, "myFabric", result) - - paths = [c["path"] for c in _FakeNDModuleV2.calls] - assert any("actions/configSave" in p for p in paths) - assert any("actions/deploy" in p for p in paths) - assert all("switchActions/deploy" not in p for p in paths) - - -def test_global_config_save_vpc_generation_error_is_fatal(): - """The issue #335 configSave 500 must not be masked and followed by deploy.""" - nrm = _build_nrm(config_type="global") - result = {"changed": True, "created": [("FOC1", "FOC2")], "deleted": [], "updated": []} - _FakeNDModuleV2.fail_on_path = "actions/configSave" - _FakeNDModuleV2.fail_message = ( - "ND Error 500: [FOC1/NX_1] - Unexpected error generating vPC configuration. " - "Last Status [Peer Link Interface]
Switch [NX_1/FOC1]: Member port Ethernet1/1 " - "is a member of VPC_PAIR, remove Ethernet1/1 from VPC_PAIR first." - ) - - with patch.object(deploy, "NDModuleV2", _FakeNDModuleV2): - with pytest.raises(VpcPairResourceError): - deploy.custom_vpc_deploy(nrm, "myFabric", result) - - paths = [c["path"] for c in _FakeNDModuleV2.calls] - assert any("actions/configSave" in p for p in paths) - assert all("actions/deploy" not in p for p in paths) - assert all("switchActions/deploy" not in p for p in paths) - - -def test_switch_scoped_check_mode_previews_switch_deploy(): - """check_mode preview for type=switch references the switch-scoped endpoint.""" - nrm = _build_nrm(config_type="switch", check_mode=True) - result = {"changed": True, "created": [("FOC1", "FOC2")], "deleted": [], "updated": []} - - info = deploy.custom_vpc_deploy(nrm, "myFabric", result) - - assert info["deployment_needed"] is True - assert any("switchActions/deploy" in action for action in info["planned_actions"]) From fc58940243b78628e511351fdb0fed775ef9cb8a Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Mon, 13 Jul 2026 14:29:51 +0530 Subject: [PATCH 05/14] Fixed pep8 sanity failure --- plugins/module_utils/manage_vpc_pair/actions.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/plugins/module_utils/manage_vpc_pair/actions.py b/plugins/module_utils/manage_vpc_pair/actions.py index 5b2dde600..9509070c2 100644 --- a/plugins/module_utils/manage_vpc_pair/actions.py +++ b/plugins/module_utils/manage_vpc_pair/actions.py @@ -41,7 +41,6 @@ NDModuleError, ) - _BLOCKED_FABRIC_TYPE_TOKENS_FOR_VPC_PAIR_DETAILS = {"vxlanibgp", "vxlanebgp"} @@ -73,10 +72,7 @@ def _resolve_fabric_type_token(nd_v2: NDModuleV2, fabric_name: str, module: Any) try: details = nd_v2.request(details_path, HttpVerbEnum.GET) except Exception as exc: - module.warn( - f"Unable to determine fabric type for '{fabric_name}' while validating vpc_pair_details: " - f"{str(exc).splitlines()[0]}" - ) + module.warn(f"Unable to determine fabric type for '{fabric_name}' while validating vpc_pair_details: " f"{str(exc).splitlines()[0]}") return "" if not isinstance(details, dict): From 12878264b6723d2993fdc0932cdc9440bc062e34 Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Wed, 15 Jul 2026 10:53:38 +0530 Subject: [PATCH 06/14] Addressing review comments --- .../module_utils/manage_vpc_pair/actions.py | 38 +++++++++++++++---- 1 file changed, 31 insertions(+), 7 deletions(-) diff --git a/plugins/module_utils/manage_vpc_pair/actions.py b/plugins/module_utils/manage_vpc_pair/actions.py index 9509070c2..901b49f21 100644 --- a/plugins/module_utils/manage_vpc_pair/actions.py +++ b/plugins/module_utils/manage_vpc_pair/actions.py @@ -57,12 +57,28 @@ def _normalize_fabric_type_token(value: Any) -> str: return re.sub(r"[^a-z0-9]", "", value.strip().lower()) +def _first_line(value: Any) -> str: + """Return first non-empty line from value converted to string.""" + text = str(value).strip() + if not text: + return "unknown error" + lines = text.splitlines() + return lines[0] if lines else "unknown error" + + def _resolve_fabric_type_token(nd_v2: NDModuleV2, fabric_name: str, module: Any) -> str: """ Resolve and cache the normalized fabric-type token for current run. - Best-effort lookup from fabric details endpoint. Returns empty string when - fabric type cannot be determined. + The lookup is fail-open: when type cannot be determined, return empty string + and skip the VXLAN iBGP/eBGP vpc_pair_details block to avoid false + negatives caused by transient fabric-details lookup failures. + + Candidate precedence is deterministic and favors top-level fabric type + fields over nested properties: + 1. details.fabricType / fabricTechnology / type / category + 2. details.management.type + 3. details.properties.fabricType / fabricTechnology / type """ cached = module.params.get("_fabric_type_token") if isinstance(cached, str) and cached: @@ -72,7 +88,10 @@ def _resolve_fabric_type_token(nd_v2: NDModuleV2, fabric_name: str, module: Any) try: details = nd_v2.request(details_path, HttpVerbEnum.GET) except Exception as exc: - module.warn(f"Unable to determine fabric type for '{fabric_name}' while validating vpc_pair_details: " f"{str(exc).splitlines()[0]}") + module.warn( + f"Unable to determine fabric type for '{fabric_name}' while validating " + f"vpc_pair_details: {_first_line(exc)}" + ) return "" if not isinstance(details, dict): @@ -106,14 +125,16 @@ def _resolve_fabric_type_token(nd_v2: NDModuleV2, fabric_name: str, module: Any) return "" -def _get_proposed_vpc_pair_details(proposed_config: Any) -> Any: - """Return proposed vpc_pair_details (snake_case or API key), if present.""" +def _get_proposed_vpc_pair_details(proposed_config: Any) -> dict[str, Any] | None: + """Return non-empty proposed vpc_pair_details (snake_case or API key).""" if not isinstance(proposed_config, dict): return None details = proposed_config.get("vpc_pair_details") if details is None: details = proposed_config.get(VpcFieldNames.VPC_PAIR_DETAILS) + if not isinstance(details, dict) or not details: + return None return details @@ -131,7 +152,7 @@ def _validate_vpc_pair_details_fabric_support( supports the additional settings. """ proposed_details = _get_proposed_vpc_pair_details(nrm.proposed_config) - if not proposed_details: + if proposed_details is None: return token = nrm.module.params.get("_fabric_type_token") @@ -291,7 +312,10 @@ def custom_vpc_create(nrm: Any) -> dict[str, Any] | None: except VpcPairResourceError: raise except Exception as support_error: - nrm.module.warn(f"Pairing support check failed for switch {switch_id}: " f"{str(support_error).splitlines()[0]}. Continuing with create operation.") + nrm.module.warn( + f"Pairing support check failed for switch {switch_id}: " + f"{_first_line(support_error)}. Continuing with create operation." + ) # Validate fabric peering support if virtual peer link is requested. _validate_fabric_peering_support( From 63ed907832884df1dc9d556ab8169c03f41bcba7 Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Wed, 15 Jul 2026 11:18:07 +0530 Subject: [PATCH 07/14] Resolving pep8 sanity issue --- plugins/module_utils/manage_vpc_pair/actions.py | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/plugins/module_utils/manage_vpc_pair/actions.py b/plugins/module_utils/manage_vpc_pair/actions.py index 901b49f21..b01b2d0ba 100644 --- a/plugins/module_utils/manage_vpc_pair/actions.py +++ b/plugins/module_utils/manage_vpc_pair/actions.py @@ -88,10 +88,7 @@ def _resolve_fabric_type_token(nd_v2: NDModuleV2, fabric_name: str, module: Any) try: details = nd_v2.request(details_path, HttpVerbEnum.GET) except Exception as exc: - module.warn( - f"Unable to determine fabric type for '{fabric_name}' while validating " - f"vpc_pair_details: {_first_line(exc)}" - ) + module.warn(f"Unable to determine fabric type for '{fabric_name}' while validating " f"vpc_pair_details: {_first_line(exc)}") return "" if not isinstance(details, dict): @@ -312,10 +309,7 @@ def custom_vpc_create(nrm: Any) -> dict[str, Any] | None: except VpcPairResourceError: raise except Exception as support_error: - nrm.module.warn( - f"Pairing support check failed for switch {switch_id}: " - f"{_first_line(support_error)}. Continuing with create operation." - ) + nrm.module.warn(f"Pairing support check failed for switch {switch_id}: " f"{_first_line(support_error)}. Continuing with create operation.") # Validate fabric peering support if virtual peer link is requested. _validate_fabric_peering_support( From 2c055e1ce7332bd00df9700a71b4c2f4b2357645 Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Wed, 15 Jul 2026 15:31:01 +0530 Subject: [PATCH 08/14] Simplified fabric type resolution for vpc_pair_details validation --- .../module_utils/manage_vpc_pair/actions.py | 39 ++++++------------- 1 file changed, 12 insertions(+), 27 deletions(-) diff --git a/plugins/module_utils/manage_vpc_pair/actions.py b/plugins/module_utils/manage_vpc_pair/actions.py index b01b2d0ba..4bfed932f 100644 --- a/plugins/module_utils/manage_vpc_pair/actions.py +++ b/plugins/module_utils/manage_vpc_pair/actions.py @@ -70,15 +70,13 @@ def _resolve_fabric_type_token(nd_v2: NDModuleV2, fabric_name: str, module: Any) """ Resolve and cache the normalized fabric-type token for current run. - The lookup is fail-open: when type cannot be determined, return empty string - and skip the VXLAN iBGP/eBGP vpc_pair_details block to avoid false - negatives caused by transient fabric-details lookup failures. - - Candidate precedence is deterministic and favors top-level fabric type - fields over nested properties: - 1. details.fabricType / fabricTechnology / type / category - 2. details.management.type - 3. details.properties.fabricType / fabricTechnology / type + The lookup is fail-open: when type cannot be determined, return empty string + and skip the VXLAN iBGP/eBGP vpc_pair_details block to avoid false + negatives caused by transient lookup failures. + + Candidate precedence is deterministic and based on observed response shapes: + 1. /api/v1/manage/fabrics/ -> management.type + 2. /api/v1/manage/fabrics//switches -> fabricType """ cached = module.params.get("_fabric_type_token") if isinstance(cached, str) and cached: @@ -94,27 +92,14 @@ def _resolve_fabric_type_token(nd_v2: NDModuleV2, fabric_name: str, module: Any) if not isinstance(details, dict): return "" - candidates: list[str] = [] - for key in ("fabricType", "fabricTechnology", "type", "category"): - value = details.get(key) - if isinstance(value, str): - candidates.append(value) + token = _normalize_fabric_type_token(details.get("fabricType")) + if token: + module.params["_fabric_type_token"] = token + return token management = details.get("management") if isinstance(management, dict): - mgmt_type = management.get("type") - if isinstance(mgmt_type, str): - candidates.append(mgmt_type) - - properties = details.get("properties") - if isinstance(properties, dict): - for key in ("fabricType", "fabricTechnology", "type"): - value = properties.get(key) - if isinstance(value, str): - candidates.append(value) - - for candidate in candidates: - token = _normalize_fabric_type_token(candidate) + token = _normalize_fabric_type_token(management.get("type")) if token: module.params["_fabric_type_token"] = token return token From 5565554686c729cd281f8a3e78025aa9967bd7de Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Wed, 15 Jul 2026 23:39:26 +0530 Subject: [PATCH 09/14] Removed fabric-type normaisation and fallback mappings --- .../module_utils/manage_vpc_pair/actions.py | 61 +++--- .../test_manage_vpc_pair_actions.py | 197 ++++++++++++++++++ 2 files changed, 221 insertions(+), 37 deletions(-) create mode 100644 tests/unit/module_utils/test_manage_vpc_pair_actions.py diff --git a/plugins/module_utils/manage_vpc_pair/actions.py b/plugins/module_utils/manage_vpc_pair/actions.py index 4bfed932f..a0e4a7d0f 100644 --- a/plugins/module_utils/manage_vpc_pair/actions.py +++ b/plugins/module_utils/manage_vpc_pair/actions.py @@ -5,7 +5,6 @@ from __future__ import annotations -import re from typing import Any from urllib.parse import quote @@ -36,25 +35,18 @@ _build_vpc_pair_payload, _get_api_field_value, ) +from ansible_collections.cisco.nd.plugins.module_utils.models.manage_fabric.enums import ( + FabricTypeEnum, +) from ansible_collections.cisco.nd.plugins.module_utils.nd_v2 import ( NDModule as NDModuleV2, NDModuleError, ) -_BLOCKED_FABRIC_TYPE_TOKENS_FOR_VPC_PAIR_DETAILS = {"vxlanibgp", "vxlanebgp"} - - -def _normalize_fabric_type_token(value: Any) -> str: - """ - Normalize fabric-type strings for stable comparisons. - - Examples: - - "vxlanIbgp" -> "vxlanibgp" - - "VXLAN_EBGP" -> "vxlanebgp" - """ - if not isinstance(value, str): - return "" - return re.sub(r"[^a-z0-9]", "", value.strip().lower()) +_BLOCKED_FABRIC_TYPES_FOR_VPC_PAIR_DETAILS = { + FabricTypeEnum.VXLAN_IBGP.value, + FabricTypeEnum.VXLAN_EBGP.value, +} def _first_line(value: Any) -> str: @@ -66,19 +58,20 @@ def _first_line(value: Any) -> str: return lines[0] if lines else "unknown error" -def _resolve_fabric_type_token(nd_v2: NDModuleV2, fabric_name: str, module: Any) -> str: +def _resolve_fabric_type(nd_v2: NDModuleV2, fabric_name: str, module: Any) -> str: """ - Resolve and cache the normalized fabric-type token for current run. + Resolve and cache the canonical fabric type for the current run. - The lookup is fail-open: when type cannot be determined, return empty string + Nexus Dashboard returns the fabric type in ``management.type``. The lookup + is fail-open: when the type cannot be determined, return an empty string and skip the VXLAN iBGP/eBGP vpc_pair_details block to avoid false negatives caused by transient lookup failures. - Candidate precedence is deterministic and based on observed response shapes: - 1. /api/v1/manage/fabrics/ -> management.type - 2. /api/v1/manage/fabrics//switches -> fabricType + Candidate precedence is deterministic and based on observed response shapes: + 1. /api/v1/manage/fabrics/ -> management.type + 2. /api/v1/manage/fabrics//switches -> fabricType """ - cached = module.params.get("_fabric_type_token") + cached = module.params.get("_fabric_type") if isinstance(cached, str) and cached: return cached @@ -92,17 +85,12 @@ def _resolve_fabric_type_token(nd_v2: NDModuleV2, fabric_name: str, module: Any) if not isinstance(details, dict): return "" - token = _normalize_fabric_type_token(details.get("fabricType")) - if token: - module.params["_fabric_type_token"] = token - return token - management = details.get("management") if isinstance(management, dict): - token = _normalize_fabric_type_token(management.get("type")) - if token: - module.params["_fabric_type_token"] = token - return token + fabric_type = management.get("type") + if isinstance(fabric_type, str) and fabric_type: + module.params["_fabric_type"] = fabric_type + return fabric_type return "" @@ -137,14 +125,13 @@ def _validate_vpc_pair_details_fabric_support( if proposed_details is None: return - token = nrm.module.params.get("_fabric_type_token") - if not isinstance(token, str) or not token: + fabric_type = nrm.module.params.get("_fabric_type") + if not isinstance(fabric_type, str) or not fabric_type: if not allow_lookup: return - token = _resolve_fabric_type_token(nd_v2, fabric_name, nrm.module) + fabric_type = _resolve_fabric_type(nd_v2, fabric_name, nrm.module) - normalized_token = _normalize_fabric_type_token(token) - if normalized_token in _BLOCKED_FABRIC_TYPE_TOKENS_FOR_VPC_PAIR_DETAILS: + if fabric_type in _BLOCKED_FABRIC_TYPES_FOR_VPC_PAIR_DETAILS: _raise_vpc_error( msg=( "Invalid nd_manage_vpc_pair input: 'vpc_pair_details' is not supported " @@ -152,7 +139,7 @@ def _validate_vpc_pair_details_fabric_support( "'use_virtual_peer_link' for this fabric type." ), fabric=fabric_name, - fabric_type=token, + fabric_type=fabric_type, unsupported_field="vpc_pair_details", ) diff --git a/tests/unit/module_utils/test_manage_vpc_pair_actions.py b/tests/unit/module_utils/test_manage_vpc_pair_actions.py new file mode 100644 index 000000000..84a0abf61 --- /dev/null +++ b/tests/unit/module_utils/test_manage_vpc_pair_actions.py @@ -0,0 +1,197 @@ +# -*- coding: utf-8 -*- + +# Copyright: (c) 2026, Sivakami S +# GNU General Public License v3.0+ (see LICENSE or https://www.gnu.org/licenses/gpl-3.0.txt) + +from __future__ import annotations + +from unittest.mock import patch + +import pytest + +from ansible_collections.cisco.nd.plugins.module_utils.manage_vpc_pair import actions +from ansible_collections.cisco.nd.plugins.module_utils.manage_vpc_pair.exceptions import ( + VpcPairResourceError, +) +from ansible_collections.cisco.nd.plugins.module_utils.manage_vpc_pair.enums import ( + VpcFieldNames, +) +from ansible_collections.cisco.nd.plugins.module_utils.models.manage_fabric.enums import ( + FabricTypeEnum, +) + + +class _FakeModule: + def __init__(self, params, check_mode=False): + self.params = params + self.check_mode = check_mode + self.warnings = [] + + def warn(self, msg): + self.warnings.append(msg) + + +class _FakeNrm: + def __init__(self, params, proposed_config, existing_config=None): + self.module = _FakeModule(params) + self.proposed_config = proposed_config + self.existing_config = existing_config or {} + self.current_identifier = ( + proposed_config.get(VpcFieldNames.SWITCH_ID), + proposed_config.get(VpcFieldNames.PEER_SWITCH_ID), + ) + + +class _FakeNDModuleV2: + def __init__(self, module): + self.module = module + + def request(self, path, verb, payload=None): + if path == "/api/v1/manage/fabrics/fab1": + mgmt_type = self.module.params.get("_test_management_type_response", "") + if mgmt_type: + return {"management": {"type": mgmt_type}} + return {} + if path == "/api/v1/manage/fabrics/fab1/switches": + fabric_type = self.module.params.get("_test_switch_fabric_type", "") + if fabric_type: + return [{"fabricType": fabric_type}] + return [] + return {} + + +@pytest.fixture +def _base_params(): + return { + "fabric_name": "fab1", + "_have": [], + "_fabric_type": "", + } + + +@pytest.fixture +def _details_config(): + return { + VpcFieldNames.SWITCH_ID: "SN01", + VpcFieldNames.PEER_SWITCH_ID: "SN02", + VpcFieldNames.USE_VIRTUAL_PEER_LINK: False, + "vpc_pair_details": { + "type": "default", + "domain_id": 1, + "keep_alive_vrf": "management", + }, + } + + +def test_manage_vpc_pair_actions_00010_block_vpc_pair_details_on_ibgp_create(_base_params, _details_config): + nrm = _FakeNrm( + {**_base_params, "_test_management_type_response": "vxlanIbgp"}, + _details_config, + ) + + with patch.object(actions, "NDModuleV2", _FakeNDModuleV2): + with pytest.raises(VpcPairResourceError) as exc: + actions.custom_vpc_create(nrm) + + assert "vpc_pair_details" in exc.value.msg + assert "iBGP/eBGP VXLAN fabrics" in exc.value.msg + + +def test_manage_vpc_pair_actions_00020_block_vpc_pair_details_on_ebgp_update(_base_params, _details_config): + nrm = _FakeNrm( + {**_base_params, "_test_management_type_response": "vxlanEbgp"}, + _details_config, + existing_config={ + VpcFieldNames.SWITCH_ID: "SN01", + VpcFieldNames.PEER_SWITCH_ID: "SN02", + VpcFieldNames.USE_VIRTUAL_PEER_LINK: False, + }, + ) + + with patch.object(actions, "NDModuleV2", _FakeNDModuleV2): + with pytest.raises(VpcPairResourceError) as exc: + actions.custom_vpc_update(nrm) + + assert "vpc_pair_details" in exc.value.msg + assert "iBGP/eBGP VXLAN fabrics" in exc.value.msg + + +def test_manage_vpc_pair_actions_00030_allow_vpc_pair_details_on_external(_base_params, _details_config): + nrm = _FakeNrm( + { + **_base_params, + "_fabric_type": FabricTypeEnum.EXTERNAL_CONNECTIVITY.value, + }, + _details_config, + ) + + with patch.object(actions, "NDModuleV2", _FakeNDModuleV2): + with patch.object(actions, "_validate_switches_exist_in_fabric", return_value=None): + with patch.object(actions, "_get_pairing_support_details", return_value=None): + with patch.object(actions, "_validate_fabric_peering_support", return_value=None): + with patch.object(actions, "_build_vpc_pair_payload", return_value={"ok": True}): + response = actions.custom_vpc_create(nrm) + + assert response == {} + + +def test_manage_vpc_pair_actions_00040_allow_vpc_pair_details_when_fabric_type_unknown(_base_params, _details_config): + nrm = _FakeNrm(_base_params, _details_config) + + with patch.object(actions, "NDModuleV2", _FakeNDModuleV2): + with patch.object(actions, "_validate_switches_exist_in_fabric", return_value=None): + with patch.object(actions, "_get_pairing_support_details", return_value=None): + with patch.object(actions, "_validate_fabric_peering_support", return_value=None): + with patch.object(actions, "_build_vpc_pair_payload", return_value={"ok": True}): + response = actions.custom_vpc_create(nrm) + + assert response == {} + + +def test_manage_vpc_pair_actions_00050_get_proposed_vpc_pair_details_normalizes_empty_values(): + assert actions._get_proposed_vpc_pair_details({"vpc_pair_details": {}}) is None + assert actions._get_proposed_vpc_pair_details({"vpc_pair_details": []}) is None + assert actions._get_proposed_vpc_pair_details({"vpc_pair_details": "x"}) is None + assert actions._get_proposed_vpc_pair_details({"vpc_pair_details": {"domain_id": 10}}) == {"domain_id": 10} + + +def test_manage_vpc_pair_actions_00060_resolve_fabric_type_handles_empty_exception_message(): + class _RaisingNDModuleV2: + def request(self, path, verb, payload=None): + raise Exception() + + module = _FakeModule({"_fabric_type": ""}) + fabric_type = actions._resolve_fabric_type(_RaisingNDModuleV2(), "fab1", module) + + assert fabric_type == "" + assert len(module.warnings) == 1 + assert "unknown error" in module.warnings[0] + + +def test_manage_vpc_pair_actions_00070_resolve_fabric_type_uses_management_type(): + class _PrecedenceNDModuleV2: + def request(self, path, verb, payload=None): + if path == "/api/v1/manage/fabrics/fab1/switches": + return [{"fabricType": "vxlanEbgp"}] + return { + "fabricType": "VXLAN", + "management": {"type": FabricTypeEnum.VXLAN_EBGP.value}, + } + + module = _FakeModule({"_fabric_type": ""}) + fabric_type = actions._resolve_fabric_type(_PrecedenceNDModuleV2(), "fab1", module) + + assert fabric_type == FabricTypeEnum.VXLAN_EBGP.value + assert module.params.get("_fabric_type") == FabricTypeEnum.VXLAN_EBGP.value + + +def test_manage_vpc_pair_actions_00080_resolve_fabric_type_does_not_infer_from_other_fields(): + class _NoManagementTypeNDModuleV2: + def request(self, path, verb, payload=None): + return {"fabricType": "VXLAN_EBGP"} + + module = _FakeModule({"_fabric_type": ""}) + fabric_type = actions._resolve_fabric_type(_NoManagementTypeNDModuleV2(), "fab1", module) + + assert fabric_type == "" + assert module.params.get("_fabric_type") == "" From 1ab7755896c7968ae9566bc85d70dc0e588f4a39 Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Wed, 15 Jul 2026 23:47:54 +0530 Subject: [PATCH 10/14] 4.3 compatible state aware validation that threads context to avoid warning --- .../models/manage_vpc_pair/vpc_pair_model.py | 7 ++++-- .../test_manage_vpc_pair_model.py | 22 +++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/plugins/module_utils/models/manage_vpc_pair/vpc_pair_model.py b/plugins/module_utils/models/manage_vpc_pair/vpc_pair_model.py index b62b377e6..5e4222818 100644 --- a/plugins/module_utils/models/manage_vpc_pair/vpc_pair_model.py +++ b/plugins/module_utils/models/manage_vpc_pair/vpc_pair_model.py @@ -159,7 +159,7 @@ def to_config(self, **kwargs: Any) -> dict[str, Any]: return self.model_dump(by_alias=False, exclude_none=True, **kwargs) @classmethod - def from_config(cls, ansible_config: dict[str, Any]) -> "VpcPairModel": + def from_config(cls, ansible_config: dict[str, Any], **kwargs) -> "VpcPairModel": """ Construct VpcPairModel from playbook config dict. @@ -167,12 +167,15 @@ def from_config(cls, ansible_config: dict[str, Any]) -> "VpcPairModel": Args: ansible_config: Dict from playbook config item + **kwargs: Additional keyword arguments forwarded to ``model_validate`` + (for example the ``context`` threaded by the state machine for + state-aware validation). Unused context is ignored by pydantic. Returns: Validated VpcPairModel instance. """ data = normalize_vpc_pair_aliases(ansible_config) - return cls.model_validate(data, by_alias=True, by_name=True) + return cls.model_validate(data, by_alias=True, by_name=True, **kwargs) def merge(self, other: "VpcPairModel") -> "VpcPairModel": """ diff --git a/tests/unit/module_utils/test_manage_vpc_pair_model.py b/tests/unit/module_utils/test_manage_vpc_pair_model.py index c752cf600..97c05dc9c 100644 --- a/tests/unit/module_utils/test_manage_vpc_pair_model.py +++ b/tests/unit/module_utils/test_manage_vpc_pair_model.py @@ -193,3 +193,25 @@ def test_manage_vpc_pair_model_00090() -> None: assert model.verify.enabled is True assert model.verify.retries == 5 assert model.verify.timeout == 10 + + +def test_manage_vpc_pair_model_00100() -> None: + """Verify from_config accepts the ``context`` threaded by the state machine. + + Regression: the shared state machine passes ``context={"state": ...}`` into + model construction so models can apply state-aware validation. from_config + must accept and forward it rather than raising + ``VpcPairModel.from_config() got an unexpected keyword argument 'context'``. + """ + with does_not_raise(): + model = VpcPairModel.from_config( + { + "switch_id": "SN01", + "peer_switch_id": "SN02", + "use_virtual_peer_link": True, + }, + context={"state": "merged"}, + ) + assert model.switch_id == "SN01" + assert model.peer_switch_id == "SN02" + assert model.use_virtual_peer_link is True From b664ba54475222a1750bde734825e59d28284f98 Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Thu, 16 Jul 2026 23:58:48 +0530 Subject: [PATCH 11/14] Resolving the review comments --- .../module_utils/manage_vpc_pair/actions.py | 104 +++--- .../module_utils/manage_vpc_pair/resources.py | 1 + .../manage_vpc_pair/runtime_endpoints.py | 20 ++ .../test_manage_vpc_pair_actions.py | 299 ++++++++++++++---- 4 files changed, 324 insertions(+), 100 deletions(-) diff --git a/plugins/module_utils/manage_vpc_pair/actions.py b/plugins/module_utils/manage_vpc_pair/actions.py index a0e4a7d0f..dd359cab4 100644 --- a/plugins/module_utils/manage_vpc_pair/actions.py +++ b/plugins/module_utils/manage_vpc_pair/actions.py @@ -6,7 +6,6 @@ from __future__ import annotations from typing import Any -from urllib.parse import quote from ansible_collections.cisco.nd.plugins.module_utils.enums import HttpVerbEnum from ansible_collections.cisco.nd.plugins.module_utils.manage_vpc_pair.enums import ( @@ -49,50 +48,75 @@ } -def _first_line(value: Any) -> str: - """Return first non-empty line from value converted to string.""" - text = str(value).strip() - if not text: - return "unknown error" - lines = text.splitlines() - return lines[0] if lines else "unknown error" +def _first_exception_line(error: Exception) -> str: + """Return the first message line for an exception.""" + if not isinstance(error, Exception): + raise TypeError("error must be an exception") + text = str(error).strip() + return text.splitlines()[0] if text else type(error).__name__ -def _resolve_fabric_type(nd_v2: NDModuleV2, fabric_name: str, module: Any) -> str: +def _resolve_fabric_type(nd_v2: NDModuleV2, fabric_name: str) -> str: """ - Resolve and cache the canonical fabric type for the current run. + Resolve the canonical fabric type from the authoritative fabric endpoint. - Nexus Dashboard returns the fabric type in ``management.type``. The lookup - is fail-open: when the type cannot be determined, return an empty string - and skip the VXLAN iBGP/eBGP vpc_pair_details block to avoid false - negatives caused by transient lookup failures. - - Candidate precedence is deterministic and based on observed response shapes: - 1. /api/v1/manage/fabrics/ -> management.type - 2. /api/v1/manage/fabrics//switches -> fabricType + Nexus Dashboard returns the fabric type in ``management.type`` from + ``GET /api/v1/manage/fabrics/``. Failure to determine the type is + fatal because callers use it to reject unsupported ``vpc_pair_details`` + before attempting a write. """ - cached = module.params.get("_fabric_type") - if isinstance(cached, str) and cached: - return cached - - details_path = f"/api/v1/manage/fabrics/{quote(fabric_name, safe='')}" + details_path: str | None = None try: + details_path = VpcPairEndpoints.fabric_details(fabric_name) details = nd_v2.request(details_path, HttpVerbEnum.GET) - except Exception as exc: - module.warn(f"Unable to determine fabric type for '{fabric_name}' while validating " f"vpc_pair_details: {_first_line(exc)}") - return "" + except NDModuleError as error: + _raise_vpc_error( + msg=(f"Unable to determine fabric type for '{fabric_name}' while validating " f"vpc_pair_details: {_first_exception_line(error)}"), + fabric=fabric_name, + path=details_path, + status=error.status, + exception_type=type(error).__name__, + ) + except (TypeError, ValueError) as error: + _raise_vpc_error( + msg=(f"Unable to determine fabric type for '{fabric_name}' while validating " f"vpc_pair_details: {_first_exception_line(error)}"), + fabric=fabric_name, + path=details_path, + exception_type=type(error).__name__, + ) if not isinstance(details, dict): - return "" + _raise_vpc_error( + msg=( + f"Unable to determine fabric type for '{fabric_name}' while validating " + f"vpc_pair_details: expected a dictionary response, got {type(details).__name__}" + ), + fabric=fabric_name, + path=details_path, + response_type=type(details).__name__, + ) management = details.get("management") - if isinstance(management, dict): - fabric_type = management.get("type") - if isinstance(fabric_type, str) and fabric_type: - module.params["_fabric_type"] = fabric_type - return fabric_type + if not isinstance(management, dict): + _raise_vpc_error( + msg=(f"Unable to determine fabric type for '{fabric_name}' while validating " "vpc_pair_details: response does not contain 'management.type'"), + fabric=fabric_name, + path=details_path, + missing_field="management.type", + ) - return "" + fabric_type = management.get("type") + if not isinstance(fabric_type, str) or not fabric_type.strip(): + _raise_vpc_error( + msg=( + f"Unable to determine fabric type for '{fabric_name}' while validating " + "vpc_pair_details: response does not contain a non-empty 'management.type'" + ), + fabric=fabric_name, + path=details_path, + missing_field="management.type", + ) + return fabric_type.strip() def _get_proposed_vpc_pair_details(proposed_config: Any) -> dict[str, Any] | None: @@ -112,7 +136,6 @@ def _validate_vpc_pair_details_fabric_support( nrm: Any, nd_v2: NDModuleV2, fabric_name: str, - allow_lookup: bool = True, ) -> None: """ Block vpc_pair_details on fabrics where it is not supported. @@ -125,11 +148,10 @@ def _validate_vpc_pair_details_fabric_support( if proposed_details is None: return - fabric_type = nrm.module.params.get("_fabric_type") - if not isinstance(fabric_type, str) or not fabric_type: - if not allow_lookup: - return - fabric_type = _resolve_fabric_type(nd_v2, fabric_name, nrm.module) + fabric_type = nrm.fabric_type + if fabric_type is None: + fabric_type = _resolve_fabric_type(nd_v2, fabric_name) + nrm.fabric_type = fabric_type if fabric_type in _BLOCKED_FABRIC_TYPES_FOR_VPC_PAIR_DETAILS: _raise_vpc_error( @@ -228,7 +250,6 @@ def custom_vpc_create(nrm: Any) -> dict[str, Any] | None: nrm=nrm, nd_v2=nd_v2, fabric_name=fabric_name, - allow_lookup=not nrm.module.check_mode, ) if nrm.module.check_mode: @@ -281,7 +302,7 @@ def custom_vpc_create(nrm: Any) -> dict[str, Any] | None: except VpcPairResourceError: raise except Exception as support_error: - nrm.module.warn(f"Pairing support check failed for switch {switch_id}: " f"{_first_line(support_error)}. Continuing with create operation.") + nrm.module.warn(f"Pairing support check failed for switch {switch_id}: " f"{_first_exception_line(support_error)}. Continuing with create operation.") # Validate fabric peering support if virtual peer link is requested. _validate_fabric_peering_support( @@ -371,7 +392,6 @@ def custom_vpc_update(nrm: Any) -> dict[str, Any] | None: nrm=nrm, nd_v2=nd_v2, fabric_name=fabric_name, - allow_lookup=not nrm.module.check_mode, ) if nrm.module.check_mode: diff --git a/plugins/module_utils/manage_vpc_pair/resources.py b/plugins/module_utils/manage_vpc_pair/resources.py index f30e8cab6..5cb10a367 100644 --- a/plugins/module_utils/manage_vpc_pair/resources.py +++ b/plugins/module_utils/manage_vpc_pair/resources.py @@ -59,6 +59,7 @@ def __init__(self, module: AnsibleModule) -> None: Args: module: AnsibleModule instance with validated params """ + self.fabric_type: str | None = None super().__init__(module=module, model_orchestrator=VpcPairOrchestrator) self.model_orchestrator.bind_state_machine(self) diff --git a/plugins/module_utils/manage_vpc_pair/runtime_endpoints.py b/plugins/module_utils/manage_vpc_pair/runtime_endpoints.py index 94afd2c95..791127c08 100644 --- a/plugins/module_utils/manage_vpc_pair/runtime_endpoints.py +++ b/plugins/module_utils/manage_vpc_pair/runtime_endpoints.py @@ -5,6 +5,8 @@ from __future__ import annotations +from urllib.parse import quote + from ansible_collections.cisco.nd.plugins.module_utils.endpoints.query_params import ( CompositeQueryParams, EndpointQueryParams, @@ -15,6 +17,9 @@ from ansible_collections.cisco.nd.plugins.module_utils.manage_vpc_pair.enums import ( ComponentTypeSupportEnum, ) +from ansible_collections.cisco.nd.plugins.module_utils.endpoints.v1.manage.manage_fabrics import ( + EpManageFabricsGet, +) from ansible_collections.cisco.nd.plugins.module_utils.endpoints.v1.manage.manage_fabrics_switches_vpc_pair import ( EpVpcPairGet, EpVpcPairPut, @@ -58,6 +63,7 @@ class VpcPairEndpoints: Centralized endpoint builders for vPC pair runtime operations. Runtime helper -> API path: + - fabric_details -> /api/v1/manage/fabrics/{fabricName} - vpc_pairs_list/vpc_pair_base -> /api/v1/manage/fabrics/{fabricName}/vpcPairs - switch_vpc_pair/vpc_pair_put -> /api/v1/manage/fabrics/{fabricName}/switches/{switchId}/vpcPair - switch_vpc_support -> /api/v1/manage/fabrics/{fabricName}/switches/{switchId}/vpcPairSupport @@ -86,6 +92,20 @@ def _append_query(path: str, *query_groups: EndpointQueryParams) -> str: query_string = composite_params.to_query_string(url_encode=False) return f"{path}?{query_string}" if query_string else path + @staticmethod + def fabric_details(fabric_name: str) -> str: + """ + Build the path for querying fabric details. + + Args: + fabric_name: Fabric name + + Returns: + Path: /api/v1/manage/fabrics/{fabricName} + """ + endpoint = EpManageFabricsGet(fabric_name=quote(fabric_name, safe="")) + return endpoint.path + @staticmethod def vpc_pair_base(fabric_name: str) -> str: """ diff --git a/tests/unit/module_utils/test_manage_vpc_pair_actions.py b/tests/unit/module_utils/test_manage_vpc_pair_actions.py index 84a0abf61..791b8edca 100644 --- a/tests/unit/module_utils/test_manage_vpc_pair_actions.py +++ b/tests/unit/module_utils/test_manage_vpc_pair_actions.py @@ -10,15 +10,16 @@ import pytest from ansible_collections.cisco.nd.plugins.module_utils.manage_vpc_pair import actions -from ansible_collections.cisco.nd.plugins.module_utils.manage_vpc_pair.exceptions import ( - VpcPairResourceError, -) from ansible_collections.cisco.nd.plugins.module_utils.manage_vpc_pair.enums import ( VpcFieldNames, ) +from ansible_collections.cisco.nd.plugins.module_utils.manage_vpc_pair.exceptions import ( + VpcPairResourceError, +) from ansible_collections.cisco.nd.plugins.module_utils.models.manage_fabric.enums import ( FabricTypeEnum, ) +from ansible_collections.cisco.nd.plugins.module_utils.nd_v2 import NDModuleError class _FakeModule: @@ -32,10 +33,18 @@ def warn(self, msg): class _FakeNrm: - def __init__(self, params, proposed_config, existing_config=None): - self.module = _FakeModule(params) + def __init__( + self, + params, + proposed_config, + existing_config=None, + check_mode=False, + fabric_type=None, + ): + self.module = _FakeModule(params, check_mode=check_mode) self.proposed_config = proposed_config self.existing_config = existing_config or {} + self.fabric_type = fabric_type self.current_identifier = ( proposed_config.get(VpcFieldNames.SWITCH_ID), proposed_config.get(VpcFieldNames.PEER_SWITCH_ID), @@ -47,16 +56,17 @@ def __init__(self, module): self.module = module def request(self, path, verb, payload=None): + self.module.params["_test_request_count"] = self.module.params.get("_test_request_count", 0) + 1 + request_exception = self.module.params.get("_test_request_exception") + if request_exception is not None: + raise request_exception if path == "/api/v1/manage/fabrics/fab1": - mgmt_type = self.module.params.get("_test_management_type_response", "") - if mgmt_type: - return {"management": {"type": mgmt_type}} + if "_test_fabric_details_response" in self.module.params: + return self.module.params["_test_fabric_details_response"] + management_type = self.module.params.get("_test_management_type_response", "") + if management_type: + return {"management": {"type": management_type}} return {} - if path == "/api/v1/manage/fabrics/fab1/switches": - fabric_type = self.module.params.get("_test_switch_fabric_type", "") - if fabric_type: - return [{"fabricType": fabric_type}] - return [] return {} @@ -65,7 +75,6 @@ def _base_params(): return { "fabric_name": "fab1", "_have": [], - "_fabric_type": "", } @@ -83,9 +92,21 @@ def _details_config(): } +def _run_supported_create(nrm): + with patch.object(actions, "NDModuleV2", _FakeNDModuleV2): + with patch.object(actions, "_validate_switches_exist_in_fabric", return_value=None): + with patch.object(actions, "_get_pairing_support_details", return_value=None): + with patch.object(actions, "_validate_fabric_peering_support", return_value=None): + with patch.object(actions, "_build_vpc_pair_payload", return_value={"ok": True}): + return actions.custom_vpc_create(nrm) + + def test_manage_vpc_pair_actions_00010_block_vpc_pair_details_on_ibgp_create(_base_params, _details_config): nrm = _FakeNrm( - {**_base_params, "_test_management_type_response": "vxlanIbgp"}, + { + **_base_params, + "_test_management_type_response": FabricTypeEnum.VXLAN_IBGP.value, + }, _details_config, ) @@ -99,7 +120,10 @@ def test_manage_vpc_pair_actions_00010_block_vpc_pair_details_on_ibgp_create(_ba def test_manage_vpc_pair_actions_00020_block_vpc_pair_details_on_ebgp_update(_base_params, _details_config): nrm = _FakeNrm( - {**_base_params, "_test_management_type_response": "vxlanEbgp"}, + { + **_base_params, + "_test_management_type_response": FabricTypeEnum.VXLAN_EBGP.value, + }, _details_config, existing_config={ VpcFieldNames.SWITCH_ID: "SN01", @@ -116,36 +140,28 @@ def test_manage_vpc_pair_actions_00020_block_vpc_pair_details_on_ebgp_update(_ba assert "iBGP/eBGP VXLAN fabrics" in exc.value.msg -def test_manage_vpc_pair_actions_00030_allow_vpc_pair_details_on_external(_base_params, _details_config): +def test_manage_vpc_pair_actions_00030_allow_cached_external_fabric_type(_base_params, _details_config): nrm = _FakeNrm( - { - **_base_params, - "_fabric_type": FabricTypeEnum.EXTERNAL_CONNECTIVITY.value, - }, + _base_params, _details_config, + fabric_type=FabricTypeEnum.EXTERNAL_CONNECTIVITY.value, ) - with patch.object(actions, "NDModuleV2", _FakeNDModuleV2): - with patch.object(actions, "_validate_switches_exist_in_fabric", return_value=None): - with patch.object(actions, "_get_pairing_support_details", return_value=None): - with patch.object(actions, "_validate_fabric_peering_support", return_value=None): - with patch.object(actions, "_build_vpc_pair_payload", return_value={"ok": True}): - response = actions.custom_vpc_create(nrm) + response = _run_supported_create(nrm) assert response == {} -def test_manage_vpc_pair_actions_00040_allow_vpc_pair_details_when_fabric_type_unknown(_base_params, _details_config): - nrm = _FakeNrm(_base_params, _details_config) +def test_manage_vpc_pair_actions_00040_allow_future_fabric_type(_base_params, _details_config): + nrm = _FakeNrm( + {**_base_params, "_test_management_type_response": "futureFabricType"}, + _details_config, + ) - with patch.object(actions, "NDModuleV2", _FakeNDModuleV2): - with patch.object(actions, "_validate_switches_exist_in_fabric", return_value=None): - with patch.object(actions, "_get_pairing_support_details", return_value=None): - with patch.object(actions, "_validate_fabric_peering_support", return_value=None): - with patch.object(actions, "_build_vpc_pair_payload", return_value={"ok": True}): - response = actions.custom_vpc_create(nrm) + response = _run_supported_create(nrm) assert response == {} + assert nrm.fabric_type == "futureFabricType" def test_manage_vpc_pair_actions_00050_get_proposed_vpc_pair_details_normalizes_empty_values(): @@ -155,43 +171,210 @@ def test_manage_vpc_pair_actions_00050_get_proposed_vpc_pair_details_normalizes_ assert actions._get_proposed_vpc_pair_details({"vpc_pair_details": {"domain_id": 10}}) == {"domain_id": 10} -def test_manage_vpc_pair_actions_00060_resolve_fabric_type_handles_empty_exception_message(): - class _RaisingNDModuleV2: - def request(self, path, verb, payload=None): - raise Exception() +def test_manage_vpc_pair_actions_00060_first_exception_line_uses_first_message_line(): + assert actions._first_exception_line(ValueError("first line\nsecond line")) == "first line" + + +def test_manage_vpc_pair_actions_00070_first_exception_line_uses_exception_class_for_empty_message(): + assert actions._first_exception_line(ValueError()) == "ValueError" + - module = _FakeModule({"_fabric_type": ""}) - fabric_type = actions._resolve_fabric_type(_RaisingNDModuleV2(), "fab1", module) +def test_manage_vpc_pair_actions_00080_first_exception_line_rejects_non_exception(): + with pytest.raises(TypeError, match="error must be an exception"): + actions._first_exception_line(None) - assert fabric_type == "" - assert len(module.warnings) == 1 - assert "unknown error" in module.warnings[0] +def test_manage_vpc_pair_actions_00090_resolve_fabric_type_uses_management_type_and_encoded_endpoint(): + class _FabricDetailsNDModuleV2: + def __init__(self): + self.paths = [] -def test_manage_vpc_pair_actions_00070_resolve_fabric_type_uses_management_type(): - class _PrecedenceNDModuleV2: def request(self, path, verb, payload=None): - if path == "/api/v1/manage/fabrics/fab1/switches": - return [{"fabricType": "vxlanEbgp"}] + self.paths.append(path) return { "fabricType": "VXLAN", "management": {"type": FabricTypeEnum.VXLAN_EBGP.value}, } - module = _FakeModule({"_fabric_type": ""}) - fabric_type = actions._resolve_fabric_type(_PrecedenceNDModuleV2(), "fab1", module) + nd_v2 = _FabricDetailsNDModuleV2() + + fabric_type = actions._resolve_fabric_type(nd_v2, "fab/name") assert fabric_type == FabricTypeEnum.VXLAN_EBGP.value - assert module.params.get("_fabric_type") == FabricTypeEnum.VXLAN_EBGP.value + assert nd_v2.paths == ["/api/v1/manage/fabrics/fab%2Fname"] -def test_manage_vpc_pair_actions_00080_resolve_fabric_type_does_not_infer_from_other_fields(): - class _NoManagementTypeNDModuleV2: +@pytest.mark.parametrize( + ("request_error", "expected_status"), + [ + (NDModuleError(msg="controller failed", status=503), 503), + (ValueError("invalid request settings"), None), + (TypeError("invalid request type"), None), + ], +) +def test_manage_vpc_pair_actions_00100_resolve_fabric_type_wraps_expected_request_errors(request_error, expected_status): + class _RaisingNDModuleV2: def request(self, path, verb, payload=None): - return {"fabricType": "VXLAN_EBGP"} + raise request_error + + with pytest.raises(VpcPairResourceError) as exc: + actions._resolve_fabric_type(_RaisingNDModuleV2(), "fab1") + + assert "Unable to determine fabric type" in exc.value.msg + assert exc.value.details["exception_type"] == type(request_error).__name__ + if expected_status is not None: + assert exc.value.details["status"] == expected_status + else: + assert "status" not in exc.value.details + + +def test_manage_vpc_pair_actions_00105_resolve_fabric_type_wraps_endpoint_validation_error(): + class _NeverCalledNDModuleV2: + def request(self, path, verb, payload=None): + raise AssertionError("request should not be called") + + with pytest.raises(VpcPairResourceError) as exc: + actions._resolve_fabric_type(_NeverCalledNDModuleV2(), "x" * 65) + + assert "Unable to determine fabric type" in exc.value.msg + assert exc.value.details["path"] is None + assert exc.value.details["exception_type"] == "ValidationError" + + +def test_manage_vpc_pair_actions_00110_resolve_fabric_type_does_not_hide_unexpected_error(): + class _RaisingNDModuleV2: + def request(self, path, verb, payload=None): + raise RuntimeError("programming error") + + with pytest.raises(RuntimeError, match="programming error"): + actions._resolve_fabric_type(_RaisingNDModuleV2(), "fab1") + + +@pytest.mark.parametrize( + "response", + [ + [], + {}, + {"management": []}, + {"management": {}}, + {"management": {"type": None}}, + {"management": {"type": " "}}, + {"fabricType": "VXLAN_EBGP"}, + ], +) +def test_manage_vpc_pair_actions_00120_resolve_fabric_type_rejects_malformed_response( + response, +): + class _MalformedResponseNDModuleV2: + def request(self, path, verb, payload=None): + return response + + with pytest.raises(VpcPairResourceError) as exc: + actions._resolve_fabric_type(_MalformedResponseNDModuleV2(), "fab1") - module = _FakeModule({"_fabric_type": ""}) - fabric_type = actions._resolve_fabric_type(_NoManagementTypeNDModuleV2(), "fab1", module) + assert "Unable to determine fabric type" in exc.value.msg + + +def test_manage_vpc_pair_actions_00130_cache_reuses_successful_fabric_type(_base_params, _details_config): + class _CountingNDModuleV2: + def __init__(self): + self.request_count = 0 + + def request(self, path, verb, payload=None): + self.request_count += 1 + return {"management": {"type": FabricTypeEnum.EXTERNAL_CONNECTIVITY.value}} + + nrm = _FakeNrm(_base_params, _details_config) + nd_v2 = _CountingNDModuleV2() + + actions._validate_vpc_pair_details_fabric_support(nrm, nd_v2, "fab1") + actions._validate_vpc_pair_details_fabric_support(nrm, nd_v2, "fab1") + + assert nd_v2.request_count == 1 + assert nrm.fabric_type == FabricTypeEnum.EXTERNAL_CONNECTIVITY.value + + +def test_manage_vpc_pair_actions_00135_failed_resolution_does_not_populate_cache(_base_params, _details_config): + class _MalformedResponseNDModuleV2: + def request(self, path, verb, payload=None): + return {} + + nrm = _FakeNrm(_base_params, _details_config) + + with pytest.raises(VpcPairResourceError): + actions._validate_vpc_pair_details_fabric_support(nrm, _MalformedResponseNDModuleV2(), "fab1") + + assert nrm.fabric_type is None + + +@pytest.mark.parametrize( + "proposed_config", + [ + {}, + {"vpc_pair_details": {}}, + {VpcFieldNames.VPC_PAIR_DETAILS: []}, + ], +) +def test_manage_vpc_pair_actions_00140_no_details_skips_fabric_lookup(_base_params, proposed_config): + class _NeverCalledNDModuleV2: + def request(self, path, verb, payload=None): + raise AssertionError("fabric lookup should not be called") + + nrm = _FakeNrm(_base_params, proposed_config) + + actions._validate_vpc_pair_details_fabric_support(nrm, _NeverCalledNDModuleV2(), "fab1") + + assert nrm.fabric_type is None + + +def test_manage_vpc_pair_actions_00150_check_mode_create_rejects_ibgp_details(_base_params, _details_config): + nrm = _FakeNrm( + { + **_base_params, + "_test_management_type_response": FabricTypeEnum.VXLAN_IBGP.value, + }, + _details_config, + check_mode=True, + ) + + with patch.object(actions, "NDModuleV2", _FakeNDModuleV2): + with pytest.raises(VpcPairResourceError): + actions.custom_vpc_create(nrm) + + assert nrm.module.params["_test_request_count"] == 1 + + +def test_manage_vpc_pair_actions_00160_check_mode_update_rejects_ebgp_details(_base_params, _details_config): + nrm = _FakeNrm( + { + **_base_params, + "_test_management_type_response": FabricTypeEnum.VXLAN_EBGP.value, + }, + _details_config, + existing_config={VpcFieldNames.SWITCH_ID: "SN01"}, + check_mode=True, + ) + + with patch.object(actions, "NDModuleV2", _FakeNDModuleV2): + with pytest.raises(VpcPairResourceError): + actions.custom_vpc_update(nrm) + + assert nrm.module.params["_test_request_count"] == 1 + + +def test_manage_vpc_pair_actions_00170_check_mode_allows_supported_details_without_write(_base_params, _details_config): + nrm = _FakeNrm( + { + **_base_params, + "_test_management_type_response": FabricTypeEnum.EXTERNAL_CONNECTIVITY.value, + }, + _details_config, + check_mode=True, + ) + + with patch.object(actions, "NDModuleV2", _FakeNDModuleV2): + response = actions.custom_vpc_create(nrm) - assert fabric_type == "" - assert module.params.get("_fabric_type") == "" + assert response == _details_config + assert nrm.fabric_type == FabricTypeEnum.EXTERNAL_CONNECTIVITY.value + assert nrm.module.params["_test_request_count"] == 1 From 6f0d2d8ad7e3ba42d9db0fa6b9f3d04f40f2920a Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Wed, 22 Jul 2026 13:22:07 +0530 Subject: [PATCH 12/14] Fixed vpc pair details handling on iBGP/eBGP VXLAN fabrics, preflight strip inherited details on blocked fabrics, gate detail-bearing integration cases and TC23b assertions --- .../module_utils/manage_vpc_pair/actions.py | 107 +++++-- .../module_utils/manage_vpc_pair/resources.py | 15 + plugins/modules/nd_manage_vpc_pair.py | 38 +++ .../targets/nd_vpc_pair/tasks/base_tasks.yaml | 2 +- .../nd_vpc_pair/tasks/nd_vpc_pair_merge.yaml | 99 ++++++- .../test_manage_vpc_pair_actions.py | 279 ++++++++++-------- 6 files changed, 386 insertions(+), 154 deletions(-) diff --git a/plugins/module_utils/manage_vpc_pair/actions.py b/plugins/module_utils/manage_vpc_pair/actions.py index dd359cab4..f7c555c14 100644 --- a/plugins/module_utils/manage_vpc_pair/actions.py +++ b/plugins/module_utils/manage_vpc_pair/actions.py @@ -119,6 +119,21 @@ def _resolve_fabric_type(nd_v2: NDModuleV2, fabric_name: str) -> str: return fabric_type.strip() +def _ensure_fabric_type(nrm: Any, nd_v2: NDModuleV2, fabric_name: str) -> str: + """ + Resolve the fabric type once and cache it on the state machine. + + The resolved value is stored on ``nrm.fabric_type`` so repeated validation + and payload-sanitization passes across the proposed items in a single run + trigger at most one controller lookup. + """ + fabric_type = nrm.fabric_type + if fabric_type is None: + fabric_type = _resolve_fabric_type(nd_v2, fabric_name) + nrm.fabric_type = fabric_type + return fabric_type + + def _get_proposed_vpc_pair_details(proposed_config: Any) -> dict[str, Any] | None: """Return non-empty proposed vpc_pair_details (snake_case or API key).""" if not isinstance(proposed_config, dict): @@ -132,26 +147,41 @@ def _get_proposed_vpc_pair_details(proposed_config: Any) -> dict[str, Any] | Non return details -def _validate_vpc_pair_details_fabric_support( - nrm: Any, - nd_v2: NDModuleV2, - fabric_name: str, -) -> None: +def _get_explicit_proposed_details(proposed_item: Any) -> dict[str, Any] | None: """ - Block vpc_pair_details on fabrics where it is not supported. + Return non-empty vpc_pair_details only when the user explicitly supplied it. - Currently blocked for VXLAN iBGP/eBGP fabrics. Allowed for other fabric - types (for example External/ISN-style deployments) where the controller - supports the additional settings. + ``proposed_item`` is the raw user-intent model built directly from the + playbook config, so ``model_dump(exclude_unset=True)`` reflects exactly the + fields the user set; defaults and merge-inherited values are excluded. A + plain dict is also accepted for convenience. """ - proposed_details = _get_proposed_vpc_pair_details(nrm.proposed_config) + if hasattr(proposed_item, "model_dump"): + explicit_config = proposed_item.model_dump(by_alias=True, exclude_none=True, exclude_unset=True) + else: + explicit_config = proposed_item + return _get_proposed_vpc_pair_details(explicit_config) + + +def validate_proposed_details_support(nrm: Any, proposed_item: Any) -> None: + """ + Reject vpc_pair_details on blocked fabrics using only raw user intent. + + This preflight check runs before the state machine computes diffs, so an + unsupported field is rejected even when the requested value happens to match + existing controller state; an idempotent request must not silently accept a + prohibited field. Only fields the user explicitly supplied are considered, so + inherited/merged details never cause a false rejection. The fabric type is + resolved and cached lazily and only when explicit details are present, so the + common id-only path performs no extra controller lookup. + """ + proposed_details = _get_explicit_proposed_details(proposed_item) if proposed_details is None: return - fabric_type = nrm.fabric_type - if fabric_type is None: - fabric_type = _resolve_fabric_type(nd_v2, fabric_name) - nrm.fabric_type = fabric_type + fabric_name = nrm.module.params.get("fabric_name") + nd_v2 = NDModuleV2(nrm.module) + fabric_type = _ensure_fabric_type(nrm, nd_v2, fabric_name) if fabric_type in _BLOCKED_FABRIC_TYPES_FOR_VPC_PAIR_DETAILS: _raise_vpc_error( @@ -166,6 +196,41 @@ def _validate_vpc_pair_details_fabric_support( ) +def strip_inherited_details_for_blocked_fabric(nrm: Any, proposed_item: Any) -> None: + """ + Drop merge-inherited vpc_pair_details from the outgoing payload on blocked fabrics. + + When a user omits vpc_pair_details on an update, ``merge()`` re-adds the + vpcPairDetails carried by existing controller state into the reconciled + payload. On iBGP/eBGP VXLAN fabrics that inherited field must never be sent + back to Nexus Dashboard. Explicitly supplied details are left untouched here + because they are already validated by :func:`validate_proposed_details_support` + and remain permitted on External/ISN/LANClassic fabrics. The fabric lookup is + performed only when the payload actually carries details. + """ + if not isinstance(nrm.proposed_config, dict): + return + + payload_details = nrm.proposed_config.get(VpcFieldNames.VPC_PAIR_DETAILS) + if payload_details is None: + payload_details = nrm.proposed_config.get("vpc_pair_details") + if not payload_details: + return + + if _get_explicit_proposed_details(proposed_item) is not None: + # User explicitly supplied details; preflight already validated them and + # supported fabrics must keep them. + return + + fabric_name = nrm.module.params.get("fabric_name") + nd_v2 = NDModuleV2(nrm.module) + fabric_type = _ensure_fabric_type(nrm, nd_v2, fabric_name) + + if fabric_type in _BLOCKED_FABRIC_TYPES_FOR_VPC_PAIR_DETAILS: + nrm.proposed_config.pop(VpcFieldNames.VPC_PAIR_DETAILS, None) + nrm.proposed_config.pop("vpc_pair_details", None) + + def _build_compare_payloads(nrm: Any) -> tuple[dict[str, Any], dict[str, Any]]: """ Build normalized want/have payloads for idempotence comparisons. @@ -243,14 +308,7 @@ def custom_vpc_create(nrm: Any) -> dict[str, Any] | None: raise ValueError("peer_switch_id is required but was not provided") # Initialize RestSend via NDModuleV2. - # Validate details support before create to fail early on unsupported - # fabric types (iBGP/eBGP VXLAN). nd_v2 = NDModuleV2(nrm.module) - _validate_vpc_pair_details_fabric_support( - nrm=nrm, - nd_v2=nd_v2, - fabric_name=fabric_name, - ) if nrm.module.check_mode: return nrm.proposed_config @@ -385,14 +443,7 @@ def custom_vpc_update(nrm: Any) -> dict[str, Any] | None: raise ValueError("peer_switch_id is required but was not provided") # Initialize RestSend via NDModuleV2. - # Validate details support before update to fail early on unsupported - # fabric types (iBGP/eBGP VXLAN). nd_v2 = NDModuleV2(nrm.module) - _validate_vpc_pair_details_fabric_support( - nrm=nrm, - nd_v2=nd_v2, - fabric_name=fabric_name, - ) if nrm.module.check_mode: return nrm.proposed_config diff --git a/plugins/module_utils/manage_vpc_pair/resources.py b/plugins/module_utils/manage_vpc_pair/resources.py index 5cb10a367..563d4d786 100644 --- a/plugins/module_utils/manage_vpc_pair/resources.py +++ b/plugins/module_utils/manage_vpc_pair/resources.py @@ -27,6 +27,10 @@ get_verify_iterations, get_verify_settings, ) +from ansible_collections.cisco.nd.plugins.module_utils.manage_vpc_pair.actions import ( + strip_inherited_details_for_blocked_fabric, + validate_proposed_details_support, +) from ansible_collections.cisco.nd.plugins.module_utils.utils import issubset """ @@ -342,6 +346,12 @@ def _manage_create_update_state(self, state: str, unwanted_keys: list[Any]) -> N try: self.current_identifier = identifier + # Preflight: validate vpc_pair_details against fabric support + # using only the fields the user explicitly supplied, before the + # diff/no_diff decision so idempotent requests (values already + # matching controller state) are validated too. + validate_proposed_details_support(self, proposed_item) + existing_item = self.existing.get(identifier) self.existing_config = existing_item.model_dump(by_alias=True, exclude_none=True) if existing_item else {} @@ -377,6 +387,11 @@ def _manage_create_update_state(self, state: str, unwanted_keys: list[Any]) -> N self.proposed_config = final_item.to_payload() + # On blocked iBGP/eBGP VXLAN fabrics, drop any vpc_pair_details + # inherited from existing controller state via merge() so an + # unsupported field is never sent back to Nexus Dashboard. + strip_inherited_details_for_blocked_fabric(self, proposed_item) + if diff_status == "changed": response = self.model_orchestrator.update(final_item) operation_status = "updated" diff --git a/plugins/modules/nd_manage_vpc_pair.py b/plugins/modules/nd_manage_vpc_pair.py index a6b97b956..ecf40984f 100644 --- a/plugins/modules/nd_manage_vpc_pair.py +++ b/plugins/modules/nd_manage_vpc_pair.py @@ -115,6 +115,13 @@ vpc_pair_details: description: - Optional vPC pair template details (default/custom template fields). + - This option is not supported on iBGP/eBGP VXLAN fabrics (C(vxlanIbgp) and + C(vxlanEbgp)) and is rejected during validation for those fabric types. On + iBGP/eBGP VXLAN fabrics, supply only the peer switch IDs with + O(config.use_virtual_peer_link=false); Nexus Dashboard discovers the directly + connected physical peer-link interfaces automatically. + - Supported on fabric types that accept explicit physical peer-link details, such + as LANClassic, External Connectivity (C(externalConnectivity)), and ISN fabrics. type: dict extends_documentation_fragment: - cisco.nd.modules @@ -194,6 +201,37 @@ - peer1_switch_id: "FDO23040Q85" peer2_switch_id: "FDO23040Q86" +# Create a vPC pair on an iBGP/eBGP VXLAN fabric (physical peer-link). +# vpc_pair_details is not supported on vxlanIbgp/vxlanEbgp fabrics: omit it and +# set use_virtual_peer_link: false so Nexus Dashboard discovers the directly +# connected peer interfaces automatically. +- name: Create vPC pair on an iBGP/eBGP VXLAN fabric + cisco.nd.nd_manage_vpc_pair: + fabric_name: myVxlanFabric + state: merged + config: + - peer1_switch_id: "FDO23040Q85" + peer2_switch_id: "FDO23040Q86" + use_virtual_peer_link: false + +# Create a vPC pair on an External Connectivity fabric with explicit +# vpc_pair_details. Explicit physical peer-link details are accepted on +# LANClassic, External Connectivity, and ISN fabrics. +- name: Create vPC pair on an External Connectivity fabric with vpc_pair_details + cisco.nd.nd_manage_vpc_pair: + fabric_name: myExternalFabric + state: merged + config: + - peer1_switch_id: "FDO23040Q85" + peer2_switch_id: "FDO23040Q86" + use_virtual_peer_link: false + vpc_pair_details: + type: default + domain_id: 110 + keep_alive_vrf: management + switch_keep_alive_local_ip: "192.0.2.11" + peer_switch_keep_alive_local_ip: "192.0.2.12" + # Native Ansible check_mode behavior - name: Check mode vPC pair creation cisco.nd.nd_manage_vpc_pair: diff --git a/tests/integration/targets/nd_vpc_pair/tasks/base_tasks.yaml b/tests/integration/targets/nd_vpc_pair/tasks/base_tasks.yaml index 5aea48883..94b1f3214 100644 --- a/tests/integration/targets/nd_vpc_pair/tasks/base_tasks.yaml +++ b/tests/integration/targets/nd_vpc_pair/tasks/base_tasks.yaml @@ -34,7 +34,7 @@ vpc_pair_details_expected_supported: >- {{ (fabric_type | default('LANClassic') | string | lower) - in ['lanclassic', 'external', 'isn'] + in ['lanclassic', 'external', 'externalconnectivity', 'isn'] }} deploy_local: true delegate_to: localhost diff --git a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_merge.yaml b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_merge.yaml index 554357056..d26937dbf 100644 --- a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_merge.yaml +++ b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_merge.yaml @@ -98,6 +98,8 @@ type: switch config: "{{ nd_vpc_pair_merge_full_conf }}" register: result + when: + - vpc_pair_details_expected_supported | default(false) | bool tags: merge - name: MERGE - TC1 - ASSERT - Check if changed flag is true @@ -105,6 +107,8 @@ that: - result.failed == false - result.changed == true + when: + - vpc_pair_details_expected_supported | default(false) | bool tags: merge - name: MERGE - TC1 - ASSERT - Verify full config payload includes vpc_pair_details @@ -116,6 +120,8 @@ - result.logs | to_json is search('"switchKeepAliveLocalIp"') - result.logs | to_json is search('"peerSwitchKeepAliveLocalIp"') - result.logs | to_json is search('"keepAliveVrf"') + when: + - vpc_pair_details_expected_supported | default(false) | bool tags: merge - name: MERGE - TC1 - GATHER - Get vPC pair state in ND @@ -131,6 +137,8 @@ - peer1_switch_id: "{{ test_switch1 }}" peer2_switch_id: "{{ test_switch2 }}" register: verify_result + when: + - vpc_pair_details_expected_supported | default(false) | bool tags: merge - name: MERGE - TC1 - VALIDATE - Verify vPC pair state in ND @@ -139,17 +147,23 @@ expected_data: "{{ nd_vpc_pair_merge_full_conf }}" changed: "{{ result.changed }}" register: validation + when: + - vpc_pair_details_expected_supported | default(false) | bool tags: merge - name: MERGE - TC1 - ASSERT - Validation passed ansible.builtin.assert: that: - validation.failed == false + when: + - vpc_pair_details_expected_supported | default(false) | bool tags: merge - name: MERGE - TC1 - conf - Idempotence cisco.nd.nd_manage_vpc_pair: *conf_full register: result + when: + - vpc_pair_details_expected_supported | default(false) | bool tags: merge - name: MERGE - TC1 - ASSERT - Check if changed flag is false @@ -166,6 +180,38 @@ and (result.class_diff.updated | default([]) | length) > 0 and ('vpcPairDetails' in (result.class_diff.updated[0].changed_properties | default([]))) ) + when: + - vpc_pair_details_expected_supported | default(false) | bool + tags: merge + +# TC1 (VXLAN) - iBGP/eBGP VXLAN fabrics reject vpc_pair_details; ND auto-discovers +# the physical peer-link when use_virtual_peer_link is false. Seed an ID-only pair +# so the ID-only lifecycle cases below have state to operate on. +- name: MERGE - TC1 - VXLAN - Create vPC pair with ID-only configuration + cisco.nd.nd_manage_vpc_pair: + <<: *nd_info + fabric_name: "{{ test_fabric }}" + state: merged + config_actions: + save: false + deploy: false + type: switch + config: + - peer1_switch_id: "{{ test_switch1 }}" + peer2_switch_id: "{{ test_switch2 }}" + use_virtual_peer_link: false + register: tc1_vxlan_result + when: + - vpc_pair_details_disallowed_fabric | default(false) | bool + tags: merge + +- name: MERGE - TC1 - VXLAN - ASSERT - ID-only create succeeds on disallowed fabrics + ansible.builtin.assert: + that: + - tc1_vxlan_result.failed == false + - tc1_vxlan_result.changed == true + when: + - vpc_pair_details_disallowed_fabric | default(false) | bool tags: merge # TC2 - Modify existing vPC pair configuration @@ -567,6 +613,9 @@ peer_switch_keep_alive_local_ip: "192.0.2.12" keep_alive_vrf: management register: result + when: + - vpc_pair_details_expected_supported | default(false) | bool + - supports_vpc_fabric_peering | default(true) | bool tags: merge - name: MERGE - TC7 - ASSERT - Verify default vpc_pair_details path @@ -580,6 +629,9 @@ - result.logs | to_json is search('"switchKeepAliveLocalIp"') - result.logs | to_json is search('"peerSwitchKeepAliveLocalIp"') - result.logs | to_json is search('"keepAliveVrf"') + when: + - vpc_pair_details_expected_supported | default(false) | bool + - supports_vpc_fabric_peering | default(true) | bool tags: merge - name: MERGE - TC7 - API - Query direct vpcPair state for switch1 @@ -587,6 +639,9 @@ path: "/api/v1/manage/fabrics/{{ test_fabric }}/switches/{{ test_switch1 }}/vpcPair" method: get register: tc7_vpc_pair_direct + when: + - vpc_pair_details_expected_supported | default(false) | bool + - supports_vpc_fabric_peering | default(true) | bool tags: merge - name: MERGE - TC7 - VALIDATE - Verify pair state after default details update @@ -598,6 +653,9 @@ use_virtual_peer_link: true mode: "exists" register: tc7_validation + when: + - vpc_pair_details_expected_supported | default(false) | bool + - supports_vpc_fabric_peering | default(true) | bool tags: merge - name: MERGE - TC7 - ASSERT - Validation passed @@ -606,6 +664,9 @@ - tc7_vpc_pair_direct.failed == false - tc7_vpc_pair_direct.current is mapping - tc7_validation.failed == false + when: + - vpc_pair_details_expected_supported | default(false) | bool + - supports_vpc_fabric_peering | default(true) | bool tags: merge # TC7b - Merge without vpc_pair_details should preserve existing details @@ -695,6 +756,8 @@ customConfig: "vpc domain 20" register: result failed_when: false + when: + - vpc_pair_details_expected_supported | default(false) | bool tags: merge - name: MERGE - TC8 - ASSERT - Verify custom details update outcome @@ -704,11 +767,15 @@ (result.changed == true and result.failed == false) or (result.msg is defined and result.msg is search("templateDO")) + when: + - vpc_pair_details_expected_supported | default(false) | bool tags: merge - name: MERGE - TC8 - SETUP - Track custom-template availability ansible.builtin.set_fact: tc8_template_unavailable: "{{ result.msg is defined and result.msg is search('templateDO') }}" + when: + - vpc_pair_details_expected_supported | default(false) | bool tags: merge - name: MERGE - TC8 - ASSERT - Verify custom vpc_pair_details path @@ -720,7 +787,9 @@ - result.logs | to_json is search('"vpcPairDetails"') - result.logs | to_json is search('"templateName"') - result.logs | to_json is search('"templateConfig"') - when: result.changed == true + when: + - vpc_pair_details_expected_supported | default(false) | bool + - result.changed == true tags: merge - name: MERGE - TC8 - API - Query direct vpcPair state for switch1 @@ -728,7 +797,9 @@ path: "/api/v1/manage/fabrics/{{ test_fabric }}/switches/{{ test_switch1 }}/vpcPair" method: get register: tc8_vpc_pair_direct - when: result.changed == true + when: + - vpc_pair_details_expected_supported | default(false) | bool + - result.changed == true tags: merge - name: MERGE - TC8 - VALIDATE - Verify pair state after custom details update @@ -740,7 +811,9 @@ use_virtual_peer_link: true mode: "exists" register: tc8_validation - when: result.changed == true + when: + - vpc_pair_details_expected_supported | default(false) | bool + - result.changed == true tags: merge - name: MERGE - TC8 - ASSERT - Validation passed @@ -749,7 +822,9 @@ - tc8_vpc_pair_direct.failed == false - tc8_vpc_pair_direct.current is mapping - tc8_validation.failed == false - when: result.changed == true + when: + - vpc_pair_details_expected_supported | default(false) | bool + - result.changed == true tags: merge # TC9 - Test invalid configurations @@ -1155,6 +1230,8 @@ config: "{{ nd_vpc_pair_merge_full_conf }}" check_mode: true register: result + when: + - vpc_pair_details_expected_supported | default(false) | bool tags: merge - name: MERGE - TC12 - ASSERT - Verify check_mode invocation succeeded @@ -1163,6 +1240,8 @@ - result.failed == false - result.changed == true - result.deployment is not defined + when: + - vpc_pair_details_expected_supported | default(false) | bool tags: merge # TC13 - check_mode + deploy preview should report deployment plan without applying @@ -1178,6 +1257,8 @@ config: "{{ nd_vpc_pair_merge_full_conf }}" check_mode: true register: result + when: + - vpc_pair_details_expected_supported | default(false) | bool tags: merge - name: MERGE - TC13 - ASSERT - Verify check_mode deployment preview @@ -1188,6 +1269,8 @@ - result.deployment.would_deploy is defined - result.deployment.would_deploy | bool == true - (result.deployment_needed | default(result.deployment.deployment_needed | default(false))) | bool == true + when: + - vpc_pair_details_expected_supported | default(false) | bool tags: merge - name: MERGE - TC13 - GATHER - Verify check_mode flows did not create vPC pair @@ -1203,6 +1286,8 @@ - peer1_switch_id: "{{ test_switch1 }}" peer2_switch_id: "{{ test_switch2 }}" register: verify_result + when: + - vpc_pair_details_expected_supported | default(false) | bool tags: merge - name: MERGE - TC13 - VALIDATE - Confirm no persistent changes from check_mode flows @@ -1211,12 +1296,16 @@ expected_data: [] mode: "count_only" register: validation + when: + - vpc_pair_details_expected_supported | default(false) | bool tags: merge - name: MERGE - TC13 - ASSERT - Validation passed ansible.builtin.assert: that: - validation.failed == false + when: + - vpc_pair_details_expected_supported | default(false) | bool tags: merge # TC14 - Validate vpcPairSupport enforcement path (isPairingAllowed == false) @@ -1841,7 +1930,7 @@ switch_keep_alive_local_ip: "192.0.2.11" peer_switch_keep_alive_local_ip: "192.0.2.12" register: tc23b_result - failed_when: false + ignore_errors: true when: - vpc_pair_details_disallowed_fabric | default(false) | bool tags: merge diff --git a/tests/unit/module_utils/test_manage_vpc_pair_actions.py b/tests/unit/module_utils/test_manage_vpc_pair_actions.py index 791b8edca..4e5437187 100644 --- a/tests/unit/module_utils/test_manage_vpc_pair_actions.py +++ b/tests/unit/module_utils/test_manage_vpc_pair_actions.py @@ -19,6 +19,9 @@ from ansible_collections.cisco.nd.plugins.module_utils.models.manage_fabric.enums import ( FabricTypeEnum, ) +from ansible_collections.cisco.nd.plugins.module_utils.models.manage_vpc_pair.vpc_pair_model import ( + VpcPairModel, +) from ansible_collections.cisco.nd.plugins.module_utils.nd_v2 import NDModuleError @@ -78,89 +81,18 @@ def _base_params(): } -@pytest.fixture -def _details_config(): - return { - VpcFieldNames.SWITCH_ID: "SN01", - VpcFieldNames.PEER_SWITCH_ID: "SN02", - VpcFieldNames.USE_VIRTUAL_PEER_LINK: False, - "vpc_pair_details": { - "type": "default", - "domain_id": 1, - "keep_alive_vrf": "management", - }, - } - - -def _run_supported_create(nrm): - with patch.object(actions, "NDModuleV2", _FakeNDModuleV2): - with patch.object(actions, "_validate_switches_exist_in_fabric", return_value=None): - with patch.object(actions, "_get_pairing_support_details", return_value=None): - with patch.object(actions, "_validate_fabric_peering_support", return_value=None): - with patch.object(actions, "_build_vpc_pair_payload", return_value={"ok": True}): - return actions.custom_vpc_create(nrm) - - -def test_manage_vpc_pair_actions_00010_block_vpc_pair_details_on_ibgp_create(_base_params, _details_config): - nrm = _FakeNrm( - { - **_base_params, - "_test_management_type_response": FabricTypeEnum.VXLAN_IBGP.value, - }, - _details_config, - ) - - with patch.object(actions, "NDModuleV2", _FakeNDModuleV2): - with pytest.raises(VpcPairResourceError) as exc: - actions.custom_vpc_create(nrm) - - assert "vpc_pair_details" in exc.value.msg - assert "iBGP/eBGP VXLAN fabrics" in exc.value.msg - - -def test_manage_vpc_pair_actions_00020_block_vpc_pair_details_on_ebgp_update(_base_params, _details_config): - nrm = _FakeNrm( - { - **_base_params, - "_test_management_type_response": FabricTypeEnum.VXLAN_EBGP.value, - }, - _details_config, - existing_config={ - VpcFieldNames.SWITCH_ID: "SN01", - VpcFieldNames.PEER_SWITCH_ID: "SN02", - VpcFieldNames.USE_VIRTUAL_PEER_LINK: False, - }, - ) - - with patch.object(actions, "NDModuleV2", _FakeNDModuleV2): - with pytest.raises(VpcPairResourceError) as exc: - actions.custom_vpc_update(nrm) - - assert "vpc_pair_details" in exc.value.msg - assert "iBGP/eBGP VXLAN fabrics" in exc.value.msg - - -def test_manage_vpc_pair_actions_00030_allow_cached_external_fabric_type(_base_params, _details_config): - nrm = _FakeNrm( - _base_params, - _details_config, - fabric_type=FabricTypeEnum.EXTERNAL_CONNECTIVITY.value, - ) - - response = _run_supported_create(nrm) - - assert response == {} - - -def test_manage_vpc_pair_actions_00040_allow_future_fabric_type(_base_params, _details_config): +def test_manage_vpc_pair_actions_00040_preflight_allows_unknown_fabric_type(_base_params): + # Only the explicit blocked set (iBGP/eBGP VXLAN) is rejected; any other + # fabric type (including an unknown/future value) accepts explicit details. + proposed_item = _details_model() nrm = _FakeNrm( {**_base_params, "_test_management_type_response": "futureFabricType"}, - _details_config, + _inherited_payload(), ) - response = _run_supported_create(nrm) + with patch.object(actions, "NDModuleV2", _FakeNDModuleV2): + actions.validate_proposed_details_support(nrm, proposed_item) - assert response == {} assert nrm.fabric_type == "futureFabricType" @@ -275,7 +207,7 @@ def request(self, path, verb, payload=None): assert "Unable to determine fabric type" in exc.value.msg -def test_manage_vpc_pair_actions_00130_cache_reuses_successful_fabric_type(_base_params, _details_config): +def test_manage_vpc_pair_actions_00130_cache_reuses_successful_fabric_type(_base_params): class _CountingNDModuleV2: def __init__(self): self.request_count = 0 @@ -284,97 +216,204 @@ def request(self, path, verb, payload=None): self.request_count += 1 return {"management": {"type": FabricTypeEnum.EXTERNAL_CONNECTIVITY.value}} - nrm = _FakeNrm(_base_params, _details_config) + nrm = _FakeNrm(_base_params, {}) nd_v2 = _CountingNDModuleV2() - actions._validate_vpc_pair_details_fabric_support(nrm, nd_v2, "fab1") - actions._validate_vpc_pair_details_fabric_support(nrm, nd_v2, "fab1") + actions._ensure_fabric_type(nrm, nd_v2, "fab1") + actions._ensure_fabric_type(nrm, nd_v2, "fab1") assert nd_v2.request_count == 1 assert nrm.fabric_type == FabricTypeEnum.EXTERNAL_CONNECTIVITY.value -def test_manage_vpc_pair_actions_00135_failed_resolution_does_not_populate_cache(_base_params, _details_config): +def test_manage_vpc_pair_actions_00135_failed_resolution_does_not_populate_cache(_base_params): class _MalformedResponseNDModuleV2: def request(self, path, verb, payload=None): return {} - nrm = _FakeNrm(_base_params, _details_config) + nrm = _FakeNrm(_base_params, {}) with pytest.raises(VpcPairResourceError): - actions._validate_vpc_pair_details_fabric_support(nrm, _MalformedResponseNDModuleV2(), "fab1") + actions._ensure_fabric_type(nrm, _MalformedResponseNDModuleV2(), "fab1") assert nrm.fabric_type is None @pytest.mark.parametrize( - "proposed_config", + "proposed_item", [ {}, {"vpc_pair_details": {}}, {VpcFieldNames.VPC_PAIR_DETAILS: []}, ], ) -def test_manage_vpc_pair_actions_00140_no_details_skips_fabric_lookup(_base_params, proposed_config): +def test_manage_vpc_pair_actions_00140_no_details_skips_fabric_lookup(_base_params, proposed_item): class _NeverCalledNDModuleV2: + def __init__(self, module=None): + pass + def request(self, path, verb, payload=None): raise AssertionError("fabric lookup should not be called") - nrm = _FakeNrm(_base_params, proposed_config) + nrm = _FakeNrm(_base_params, {}) - actions._validate_vpc_pair_details_fabric_support(nrm, _NeverCalledNDModuleV2(), "fab1") + with patch.object(actions, "NDModuleV2", _NeverCalledNDModuleV2): + actions.validate_proposed_details_support(nrm, proposed_item) assert nrm.fabric_type is None -def test_manage_vpc_pair_actions_00150_check_mode_create_rejects_ibgp_details(_base_params, _details_config): - nrm = _FakeNrm( +# --------------------------------------------------------------------------- +# Fix for GitHub review comment (actions.py:147): fabric-support validation must +# examine the raw user intent, not the reconciled/merged state. These tests +# exercise the two helpers wired into the state machine +# (``_manage_create_update_state``): +# * ``validate_proposed_details_support`` runs BEFORE the diff/no_diff decision +# so an unsupported field is rejected even when it matches existing state +# (an idempotent request must not silently accept a prohibited field), and it +# only considers fields the user explicitly supplied. +# * ``strip_inherited_details_for_blocked_fabric`` removes merge-inherited +# vpcPairDetails from the outgoing payload on blocked fabrics so the field is +# never sent back to Nexus Dashboard, while leaving explicit and +# External/ISN/LANClassic details intact. +# --------------------------------------------------------------------------- + +_DETAILS_INPUT = {"type": "default", "domain_id": 1} + + +def _details_model(): + """Raw user model that explicitly supplies vpc_pair_details.""" + return VpcPairModel.from_config( { - **_base_params, - "_test_management_type_response": FabricTypeEnum.VXLAN_IBGP.value, - }, - _details_config, - check_mode=True, + "switch_id": "SN01", + "peer_switch_id": "SN02", + "use_virtual_peer_link": False, + "vpc_pair_details": dict(_DETAILS_INPUT), + } + ) + + +def _no_details_model(): + """Raw user model that omits vpc_pair_details (id-only intent).""" + return VpcPairModel.from_config( + { + "switch_id": "SN01", + "peer_switch_id": "SN02", + "use_virtual_peer_link": False, + } + ) + + +def _inherited_payload(): + """Reconciled payload that carries vpcPairDetails (e.g. inherited via merge).""" + return { + VpcFieldNames.SWITCH_ID: "SN01", + VpcFieldNames.PEER_SWITCH_ID: "SN02", + VpcFieldNames.USE_VIRTUAL_PEER_LINK: False, + VpcFieldNames.VPC_PAIR_DETAILS: dict(_DETAILS_INPUT), + } + + +@pytest.mark.parametrize( + "blocked_type", + [FabricTypeEnum.VXLAN_IBGP.value, FabricTypeEnum.VXLAN_EBGP.value], +) +@pytest.mark.parametrize("check_mode", [False, True]) +def test_manage_vpc_pair_actions_00180_preflight_rejects_explicit_details_even_when_idempotent(_base_params, check_mode, blocked_type): + # Mike case 1 (+ check mode): explicit unsupported details are rejected from + # the raw user intent, regardless of whether the requested value already + # matches controller state. The preflight never consults existing_config, so + # the state machine's no_diff early-return cannot silently accept the field. + # Parametrized over both blocked fabric types (iBGP/eBGP VXLAN) and check + # mode, replacing the retired handler-level block tests. + proposed_item = _details_model() + nrm = _FakeNrm( + {**_base_params, "_test_management_type_response": blocked_type}, + _inherited_payload(), + existing_config=_inherited_payload(), + check_mode=check_mode, ) with patch.object(actions, "NDModuleV2", _FakeNDModuleV2): - with pytest.raises(VpcPairResourceError): - actions.custom_vpc_create(nrm) + with pytest.raises(VpcPairResourceError) as exc: + actions.validate_proposed_details_support(nrm, proposed_item) - assert nrm.module.params["_test_request_count"] == 1 + assert "vpc_pair_details" in exc.value.msg + assert "iBGP/eBGP VXLAN fabrics" in exc.value.msg -def test_manage_vpc_pair_actions_00160_check_mode_update_rejects_ebgp_details(_base_params, _details_config): +def test_manage_vpc_pair_actions_00190_preflight_ignores_omitted_details_without_fabric_lookup(_base_params): + # Mike case 2 (preflight half): when the user omits details, merge-inherited + # details must not be treated as user intent, and no controller lookup runs. + proposed_item = _no_details_model() nrm = _FakeNrm( - { - **_base_params, - "_test_management_type_response": FabricTypeEnum.VXLAN_EBGP.value, - }, - _details_config, - existing_config={VpcFieldNames.SWITCH_ID: "SN01"}, - check_mode=True, + {**_base_params, "_test_management_type_response": FabricTypeEnum.VXLAN_IBGP.value}, + _inherited_payload(), ) with patch.object(actions, "NDModuleV2", _FakeNDModuleV2): - with pytest.raises(VpcPairResourceError): - actions.custom_vpc_update(nrm) + actions.validate_proposed_details_support(nrm, proposed_item) - assert nrm.module.params["_test_request_count"] == 1 + assert nrm.fabric_type is None + assert "_test_request_count" not in nrm.module.params -def test_manage_vpc_pair_actions_00170_check_mode_allows_supported_details_without_write(_base_params, _details_config): +def test_manage_vpc_pair_actions_00200_preflight_allows_explicit_details_on_external(_base_params): + # Mike point 4: External continues to accept explicitly supplied details. + proposed_item = _details_model() nrm = _FakeNrm( - { - **_base_params, - "_test_management_type_response": FabricTypeEnum.EXTERNAL_CONNECTIVITY.value, - }, - _details_config, - check_mode=True, + {**_base_params, "_test_management_type_response": FabricTypeEnum.EXTERNAL_CONNECTIVITY.value}, + _inherited_payload(), ) with patch.object(actions, "NDModuleV2", _FakeNDModuleV2): - response = actions.custom_vpc_create(nrm) + actions.validate_proposed_details_support(nrm, proposed_item) - assert response == _details_config assert nrm.fabric_type == FabricTypeEnum.EXTERNAL_CONNECTIVITY.value - assert nrm.module.params["_test_request_count"] == 1 + + +def test_manage_vpc_pair_actions_00210_sanitize_strips_inherited_details_on_blocked_fabric(_base_params): + # Mike case 2 + point 3: inherited details are stripped from the outgoing + # payload on blocked fabrics, so the update does NOT send vpcPairDetails. + proposed_item = _no_details_model() + nrm = _FakeNrm( + {**_base_params, "_test_management_type_response": FabricTypeEnum.VXLAN_EBGP.value}, + _inherited_payload(), + ) + + with patch.object(actions, "NDModuleV2", _FakeNDModuleV2): + actions.strip_inherited_details_for_blocked_fabric(nrm, proposed_item) + + assert VpcFieldNames.VPC_PAIR_DETAILS not in nrm.proposed_config + assert "vpc_pair_details" not in nrm.proposed_config + + +def test_manage_vpc_pair_actions_00220_sanitize_preserves_inherited_details_on_external(_base_params): + # Mike point 4: External preserves details even when inherited via merge. + proposed_item = _no_details_model() + nrm = _FakeNrm( + {**_base_params, "_test_management_type_response": FabricTypeEnum.EXTERNAL_CONNECTIVITY.value}, + _inherited_payload(), + ) + + with patch.object(actions, "NDModuleV2", _FakeNDModuleV2): + actions.strip_inherited_details_for_blocked_fabric(nrm, proposed_item) + + assert nrm.proposed_config[VpcFieldNames.VPC_PAIR_DETAILS] == _DETAILS_INPUT + + +def test_manage_vpc_pair_actions_00230_sanitize_keeps_explicit_details_without_fabric_lookup(_base_params): + # Explicit user details are never stripped by the sanitizer and require no + # fabric lookup there (they are governed by the preflight validator instead). + proposed_item = _details_model() + nrm = _FakeNrm( + {**_base_params, "_test_management_type_response": FabricTypeEnum.VXLAN_IBGP.value}, + _inherited_payload(), + ) + + with patch.object(actions, "NDModuleV2", _FakeNDModuleV2): + actions.strip_inherited_details_for_blocked_fabric(nrm, proposed_item) + + assert nrm.proposed_config[VpcFieldNames.VPC_PAIR_DETAILS] == _DETAILS_INPUT + assert nrm.fabric_type is None + assert "_test_request_count" not in nrm.module.params From 32496d1338330f8a08e8736f4ed34dd9a776b392 Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Wed, 22 Jul 2026 13:29:42 +0530 Subject: [PATCH 13/14] Switch and fabric scroped deploy according to the config_actions added with UT --- .../module_utils/manage_vpc_pair/deploy.py | 102 ++++++-- plugins/module_utils/utils.py | 31 +++ plugins/modules/nd_manage_vpc_pair.py | 6 +- .../test_manage_vpc_pair_deploy.py | 243 ++++++++++++++++++ 4 files changed, 365 insertions(+), 17 deletions(-) create mode 100644 tests/unit/module_utils/test_manage_vpc_pair_deploy.py diff --git a/plugins/module_utils/manage_vpc_pair/deploy.py b/plugins/module_utils/manage_vpc_pair/deploy.py index 34fffb3fe..ee2da0d70 100644 --- a/plugins/module_utils/manage_vpc_pair/deploy.py +++ b/plugins/module_utils/manage_vpc_pair/deploy.py @@ -12,6 +12,12 @@ _raise_vpc_error, get_config_actions, ) +from ansible_collections.cisco.nd.plugins.module_utils.manage_vpc_pair.query import ( + _is_switch_config_in_sync, +) +from ansible_collections.cisco.nd.plugins.module_utils.manage_vpc_pair.validation import ( + _validate_fabric_switches, +) from ansible_collections.cisco.nd.plugins.module_utils.nd_v2 import ( NDModule as NDModuleV2, NDModuleError, @@ -107,13 +113,38 @@ def _is_non_fatal_config_save_error(error: NDModuleError) -> bool: return any(signature in message for signature in non_fatal_signatures) +def _get_switches_needing_deploy(nd_v2: Any, fabric_name: str) -> list[str]: + """ + Return serial numbers of fabric switches that require deployment. + + Queries the fabric switch inventory and selects every switch whose + config-sync state is not confirmed in-sync. Any switch that is not + explicitly in-sync (including unknown or missing status) is treated as + needing deployment so a pending change is never silently skipped. This + mirrors the collection's switch-scoped deploy contract used elsewhere for + config_actions.type == "switch". + + Args: + nd_v2: NDModuleV2 instance for RestSend + fabric_name: Fabric name to query + + Returns: + Sorted list of unique switch serial numbers needing deployment + """ + switches = _validate_fabric_switches(nd_v2, fabric_name) + switch_ids = [serial_number for serial_number, switch_data in switches.items() if _is_switch_config_in_sync(switch_data) is not True] + return sorted(set(switch_ids)) + + def custom_vpc_deploy(nrm: Any, fabric_name: str, result: dict[str, Any]) -> dict[str, Any]: """ Custom save/deploy action handler for vPC fabric changes using RestSend. - Smart action decision (_needs_deployment) - Optional Step 1: Save fabric configuration - - Optional Step 2: Deploy fabric with forceShowRun=true + - Optional Step 2: Deploy scope depends on config_actions.type: + * "global": deploy the whole fabric via .../actions/deploy (forceShowRun=true) + * "switch": deploy only affected switches via .../switchActions/deploy - Proper error handling with NDModuleError - Results aggregation - Executes only if there are actual changes or pending operations @@ -168,8 +199,12 @@ def custom_vpc_deploy(nrm: Any, fabric_name: str, result: dict[str, Any]) -> dic save_path = FabricUtils.build_config_save_path(fabric_name) planned_actions.append(f"POST {save_path} payload={action_payload}") if deploy_enabled: - deploy_path = FabricUtils.build_config_deploy_path(fabric_name, force_show_run=True) - planned_actions.append(f"POST {deploy_path} payload={action_payload}") + if action_type == "switch": + deploy_path = FabricUtils.build_switch_deploy_path(fabric_name) + planned_actions.append(f'POST {deploy_path} payload={{"switchIds": [switches needing deployment]}}') + else: + deploy_path = FabricUtils.build_config_deploy_path(fabric_name, force_show_run=True) + planned_actions.append(f"POST {deploy_path} payload={action_payload}") if save_enabled and deploy_enabled: preview_msg = "CHECK MODE: Would save and deploy fabric configuration" elif save_enabled: @@ -250,26 +285,63 @@ def custom_vpc_deploy(nrm: Any, fabric_name: str, result: dict[str, Any]) -> dic _raise_vpc_error(msg=final_msg, **final_result) # Step 2: Deploy + # + # config_actions.type selects the deploy scope: + # - "global": deploy the entire fabric via .../actions/deploy. + # - "switch": deploy only the switches left out-of-sync by the vPC pair + # changes above, via .../switchActions/deploy. if deploy_enabled: - deploy_path = fabric_utils.config_deploy_path(force_show_run=True) + if action_type == "switch": + deploy_path = fabric_utils.switch_deploy_path + else: + deploy_path = fabric_utils.config_deploy_path(force_show_run=True) try: - response = fabric_utils.deploy_config(action_payload, force_show_run=True) - register_action_api_call( - results=results, - request_path=deploy_path, - payload=action_payload, - return_code=response.get("status"), - message="Deployment successful", - success=True, - changed=True, - ) + if action_type == "switch": + switch_ids = _get_switches_needing_deploy(nd_v2, fabric_name) + deploy_payload = {"switchIds": switch_ids} + if switch_ids: + response = fabric_utils.deploy_switches(switch_ids) + register_action_api_call( + results=results, + request_path=deploy_path, + payload=deploy_payload, + return_code=response.get("status"), + message="Switch deployment successful", + success=True, + changed=True, + ) + else: + # Switch scope requested but nothing is out-of-sync; record a + # successful no-op instead of posting an empty switch list. + register_action_api_call( + results=results, + request_path=deploy_path, + payload=deploy_payload, + return_code=None, + message="No switches required switch-scoped deployment", + success=True, + changed=False, + ) + else: + deploy_payload = action_payload + response = fabric_utils.deploy_config(action_payload, force_show_run=True) + register_action_api_call( + results=results, + request_path=deploy_path, + payload=deploy_payload, + return_code=response.get("status"), + message="Deployment successful", + success=True, + changed=True, + ) except NDModuleError as error: + error_payload = {"switchIds": []} if action_type == "switch" else action_payload register_action_api_call( results=results, request_path=deploy_path, - payload=action_payload, + payload=error_payload, return_code=error.status, message=error.msg, success=False, diff --git a/plugins/module_utils/utils.py b/plugins/module_utils/utils.py index b6757ccdc..6e87f765a 100644 --- a/plugins/module_utils/utils.py +++ b/plugins/module_utils/utils.py @@ -13,6 +13,9 @@ from ansible_collections.cisco.nd.plugins.module_utils.endpoints.v1.manage.manage_fabrics_actions_deploy import ( EpFabricDeployPost, ) +from ansible_collections.cisco.nd.plugins.module_utils.endpoints.v1.manage.manage_fabrics_switchactions import ( + EpManageFabricsSwitchActionsDeployPost, +) from ansible_collections.cisco.nd.plugins.module_utils.enums import HttpVerbEnum @@ -183,3 +186,31 @@ def deploy_config(self, payload: dict[str, Any], force_show_run: bool = True) -> "status": self.nd.status, "response_data": response_data, } + + @staticmethod + def build_switch_deploy_path(fabric_name: str) -> str: + """ + Build /switchActions/deploy endpoint path for the given fabric. + """ + endpoint = EpManageFabricsSwitchActionsDeployPost(fabric_name=fabric_name) + return endpoint.path + + @property + def switch_deploy_path(self) -> str: + return self.build_switch_deploy_path(self.fabric_name) + + def deploy_switches(self, switch_ids: list[str]) -> dict[str, Any]: + """ + Call switch-scoped deploy action for specific switches. + + Unlike the fabric-level deploy, this targets only the supplied switch + serial numbers via /switchActions/deploy with a {"switchIds": [...]} body. + """ + path = self.switch_deploy_path + payload = {"switchIds": switch_ids} + response_data = self.nd.request(path, HttpVerbEnum.POST, payload) + return { + "path": path, + "status": self.nd.status, + "response_data": response_data, + } diff --git a/plugins/modules/nd_manage_vpc_pair.py b/plugins/modules/nd_manage_vpc_pair.py index ecf40984f..04f6a7946 100644 --- a/plugins/modules/nd_manage_vpc_pair.py +++ b/plugins/modules/nd_manage_vpc_pair.py @@ -54,8 +54,10 @@ default: true type: description: - - Scope type for save/deploy action payload. - - Valid values are C(switch) and C(global). + - Deploy scope for configuration actions. + - C(switch) deploys only the switches left out-of-sync by the vPC pair changes using the per-switch deploy action. + - C(global) deploys the entire fabric. + - Configuration is saved at the fabric level before deploying for both scopes. type: str choices: [switch, global] default: switch diff --git a/tests/unit/module_utils/test_manage_vpc_pair_deploy.py b/tests/unit/module_utils/test_manage_vpc_pair_deploy.py new file mode 100644 index 000000000..4d95ec245 --- /dev/null +++ b/tests/unit/module_utils/test_manage_vpc_pair_deploy.py @@ -0,0 +1,243 @@ +# -*- coding: utf-8 -*- + +# Copyright: (c) 2026, Sivakami S +# GNU General Public License v3.0+ (see LICENSE or https://www.gnu.org/licenses/gpl-3.0.txt) + +from __future__ import annotations + +from unittest.mock import patch + +import pytest + +from ansible_collections.cisco.nd.plugins.module_utils.manage_vpc_pair import deploy +from ansible_collections.cisco.nd.plugins.module_utils.manage_vpc_pair.exceptions import ( + VpcPairResourceError, +) +from ansible_collections.cisco.nd.plugins.module_utils.nd_v2 import NDModuleError +from ansible_collections.cisco.nd.plugins.module_utils.utils import FabricUtils + +SAVE_PATH = "/api/v1/manage/fabrics/fab1/actions/configSave" +GLOBAL_DEPLOY_PATH = "/api/v1/manage/fabrics/fab1/actions/deploy?forceShowRun=true" +SWITCH_DEPLOY_PATH = "/api/v1/manage/fabrics/fab1/switchActions/deploy" +SWITCHES_PATH = "/api/v1/manage/fabrics/fab1/switches" + + +class _FakeModule: + def __init__(self, params, check_mode=False): + self.params = params + self.check_mode = check_mode + self.warnings = [] + + def warn(self, msg): + self.warnings.append(msg) + + +class _FakeNrm: + def __init__(self, params, check_mode=False): + self.module = _FakeModule(params, check_mode=check_mode) + + +class _FakeNDModuleV2: + """Records (path, verb, payload) calls and routes switches/action requests.""" + + def __init__(self, module, switches_response=None, status=200, switches_exception=None, fail_on=None, fail_exception=None): + self.module = module + self.status = status + self.calls = [] + self._switches_response = switches_response if switches_response is not None else {"switches": []} + self._switches_exception = switches_exception + self._fail_on = fail_on + self._fail_exception = fail_exception + + def request(self, path, verb, payload=None): + self.calls.append((path, verb, payload)) + if path.endswith("/switches"): + if self._switches_exception is not None: + raise self._switches_exception + return self._switches_response + if self._fail_on is not None and self._fail_on in path: + raise self._fail_exception + return {"success": True} + + def paths(self): + return [path for (path, _verb, _payload) in self.calls] + + def payloads_for(self, target_path): + return [payload for (path, _verb, payload) in self.calls if path == target_path] + + +def _boom(*_args, **_kwargs): + raise AssertionError("NDModuleV2 must not be constructed in check_mode") + + +def _make_nrm(action_type, save=True, deploy_flag=True, check_mode=False): + params = { + "fabric_name": "fab1", + "config_actions": {"save": save, "deploy": deploy_flag, "type": action_type}, + } + return _FakeNrm(params, check_mode=check_mode) + + +def _run_deploy(nrm, fake_nd): + with patch.object(deploy, "NDModuleV2", lambda module: fake_nd): + return deploy.custom_vpc_deploy(nrm, "fab1", {"changed": True}) + + +def test_manage_vpc_pair_deploy_00010_global_scope_uses_fabric_deploy(): + # type=global keeps the fabric-scoped configSave + actions/deploy dispatch + # and never queries or deploys individual switches. + nrm = _make_nrm("global") + fake_nd = _FakeNDModuleV2(nrm.module) + + _run_deploy(nrm, fake_nd) + + paths = fake_nd.paths() + assert SAVE_PATH in paths + assert GLOBAL_DEPLOY_PATH in paths + assert SWITCH_DEPLOY_PATH not in paths + assert SWITCHES_PATH not in paths + assert fake_nd.payloads_for(GLOBAL_DEPLOY_PATH) == [{"type": "global"}] + + +def test_manage_vpc_pair_deploy_00020_switch_scope_deploys_only_out_of_sync_switches(): + # type=switch routes deploy to switchActions/deploy with only the switch + # serials that are not confirmed in-sync (out-of-sync + unknown status). + nrm = _make_nrm("switch") + switches_response = { + "switches": [ + {"serialNumber": "FOX222AAA", "configSyncStatus": "In-Sync"}, + {"serialNumber": "FOX111AAA", "configSyncStatus": "Out-of-Sync"}, + {"serialNumber": "FOX333AAA"}, # unknown/missing status -> must deploy + ] + } + fake_nd = _FakeNDModuleV2(nrm.module, switches_response=switches_response) + + _run_deploy(nrm, fake_nd) + + paths = fake_nd.paths() + assert SAVE_PATH in paths # save stays fabric-scoped configSave + assert SWITCHES_PATH in paths # switch inventory queried at deploy time + assert GLOBAL_DEPLOY_PATH not in paths + assert SWITCH_DEPLOY_PATH in paths + assert fake_nd.payloads_for(SWITCH_DEPLOY_PATH) == [{"switchIds": ["FOX111AAA", "FOX333AAA"]}] + + +def test_manage_vpc_pair_deploy_00030_switch_scope_noop_when_all_in_sync(): + # type=switch with every switch in-sync: still saves, queries switches, but + # never posts an empty switchActions/deploy. + nrm = _make_nrm("switch") + switches_response = { + "switches": [ + {"serialNumber": "FOX222AAA", "configSyncStatus": "In-Sync"}, + {"serialNumber": "FOX444AAA", "additionalData": {"configSyncStatus": "in_sync"}}, + ] + } + fake_nd = _FakeNDModuleV2(nrm.module, switches_response=switches_response) + + _run_deploy(nrm, fake_nd) + + paths = fake_nd.paths() + assert SAVE_PATH in paths + assert SWITCHES_PATH in paths + assert SWITCH_DEPLOY_PATH not in paths + + +def test_manage_vpc_pair_deploy_00040_check_mode_switch_scope_previews_switch_endpoint(): + # check_mode must be side-effect free: no NDModuleV2 is constructed and the + # preview names the switch-scoped endpoint. + nrm = _make_nrm("switch", check_mode=True) + + with patch.object(deploy, "NDModuleV2", _boom): + out = deploy.custom_vpc_deploy(nrm, "fab1", {"changed": True}) + + assert out["deployment_needed"] is True + planned = out["planned_actions"] + assert any(SAVE_PATH in action for action in planned) + assert any(SWITCH_DEPLOY_PATH in action for action in planned) + assert any('"switchIds"' in action for action in planned) + assert all(GLOBAL_DEPLOY_PATH not in action for action in planned) + + +def test_manage_vpc_pair_deploy_00050_check_mode_global_scope_previews_fabric_endpoint(): + nrm = _make_nrm("global", check_mode=True) + + with patch.object(deploy, "NDModuleV2", _boom): + out = deploy.custom_vpc_deploy(nrm, "fab1", {"changed": True}) + + planned = out["planned_actions"] + assert any(GLOBAL_DEPLOY_PATH in action for action in planned) + assert all(SWITCH_DEPLOY_PATH not in action for action in planned) + + +def test_manage_vpc_pair_deploy_00060_global_deploy_failure_raises_vpc_error(): + # Save succeeds, the fabric deploy fails -> structured VpcPairResourceError. + nrm = _make_nrm("global") + fake_nd = _FakeNDModuleV2( + nrm.module, + fail_on="/actions/deploy", + fail_exception=NDModuleError(msg="deploy boom", status=500), + ) + + with patch.object(deploy, "NDModuleV2", lambda module: fake_nd): + with pytest.raises(VpcPairResourceError) as exc: + deploy.custom_vpc_deploy(nrm, "fab1", {"changed": True}) + + assert exc.value.msg == "Fabric deployment failed" + assert SAVE_PATH in fake_nd.paths() + assert GLOBAL_DEPLOY_PATH in fake_nd.paths() + + +def test_manage_vpc_pair_deploy_00070_switch_query_failure_raises_vpc_error(): + # A failed switch inventory lookup during switch-scoped deploy surfaces as a + # deploy failure and never posts to switchActions/deploy. + nrm = _make_nrm("switch") + fake_nd = _FakeNDModuleV2( + nrm.module, + switches_exception=NDModuleError(msg="switch query boom", status=500), + ) + + with patch.object(deploy, "NDModuleV2", lambda module: fake_nd): + with pytest.raises(VpcPairResourceError) as exc: + deploy.custom_vpc_deploy(nrm, "fab1", {"changed": True}) + + assert exc.value.msg == "Fabric deployment failed" + assert SWITCHES_PATH in fake_nd.paths() + assert SWITCH_DEPLOY_PATH not in fake_nd.paths() + + +def test_manage_vpc_pair_deploy_00080_fabric_utils_deploy_switches_posts_switch_endpoint(): + fake_nd = _FakeNDModuleV2(_FakeModule({})) + fabric_utils = FabricUtils(fake_nd, "fab1") + + out = fabric_utils.deploy_switches(["FOX111AAA", "FOX222AAA"]) + + assert len(fake_nd.calls) == 1 + path, _verb, payload = fake_nd.calls[0] + assert path == SWITCH_DEPLOY_PATH + assert payload == {"switchIds": ["FOX111AAA", "FOX222AAA"]} + assert out["path"] == SWITCH_DEPLOY_PATH + assert out["status"] == 200 + + +def test_manage_vpc_pair_deploy_00090_switch_deploy_path_builders_match_endpoint(): + assert FabricUtils.build_switch_deploy_path("fab1") == SWITCH_DEPLOY_PATH + fabric_utils = FabricUtils(_FakeNDModuleV2(_FakeModule({})), "fab1") + assert fabric_utils.switch_deploy_path == SWITCH_DEPLOY_PATH + + +def test_manage_vpc_pair_deploy_00100_get_switches_needing_deploy_filters_and_sorts(): + # In-sync excluded; out-of-sync + unknown included; duplicate serials deduped; + # result sorted for deterministic payloads. + switches_response = { + "switches": [ + {"serialNumber": "FOXZZZ", "configSyncStatus": "In-Sync"}, + {"serialNumber": "FOXBBB", "configSyncStatus": "Pending"}, + {"serialNumber": "FOXAAA"}, + {"serialNumber": "FOXAAA", "configSyncStatus": "Out-of-Sync"}, + ] + } + fake_nd = _FakeNDModuleV2(_FakeModule({}), switches_response=switches_response) + + result = deploy._get_switches_needing_deploy(fake_nd, "fab1") + + assert result == ["FOXAAA", "FOXBBB"] From 56866ce3ce579d19c2b375d90760992941ade9d2 Mon Sep 17 00:00:00 2001 From: Sivakami Sivaraman Date: Wed, 22 Jul 2026 18:09:44 +0530 Subject: [PATCH 14/14] Corresponding config_actions type specific deploy type --- .../targets/nd_vpc_pair/tasks/nd_vpc_pair_delete.yaml | 4 ++-- .../targets/nd_vpc_pair/tasks/nd_vpc_pair_merge.yaml | 10 +++++----- .../nd_vpc_pair/tasks/nd_vpc_pair_override.yaml | 2 +- .../targets/nd_vpc_pair/tasks/nd_vpc_pair_replace.yaml | 4 ++-- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_delete.yaml b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_delete.yaml index 0ec0a678e..21edb1396 100644 --- a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_delete.yaml +++ b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_delete.yaml @@ -199,7 +199,7 @@ - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/deploy') + | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') | list | length ) > 0 @@ -442,7 +442,7 @@ - > ( tc6_delete_result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/deploy') + | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') | list | length ) > 0 diff --git a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_merge.yaml b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_merge.yaml index d26937dbf..ee27a2d7f 100644 --- a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_merge.yaml +++ b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_merge.yaml @@ -586,7 +586,7 @@ ( (tc6b_deployment_needed and (result.deployment.response is defined) and ((result.deployment.response | selectattr('REQUEST_PATH', 'search', '/actions/configSave') | list | length) > 0) - and ((result.deployment.response | selectattr('REQUEST_PATH', 'search', '/actions/deploy') | list | length) > 0)) + and ((result.deployment.response | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') | list | length) > 0)) or ((not tc6b_deployment_needed) and (result.deployment.msg | default('') is search('skipping'))) ) @@ -917,7 +917,7 @@ - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/deploy') + | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') | list | length ) > 0 @@ -1582,7 +1582,7 @@ - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/deploy') + | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') | list | length ) > 0 @@ -1640,7 +1640,7 @@ - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/deploy') + | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') | list | length ) == 0 @@ -1783,7 +1783,7 @@ - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/deploy') + | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') | list | length ) > 0 diff --git a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_override.yaml b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_override.yaml index a6578b653..ee31ba972 100644 --- a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_override.yaml +++ b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_override.yaml @@ -282,7 +282,7 @@ - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/deploy') + | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') | list | length ) > 0 diff --git a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_replace.yaml b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_replace.yaml index dc5cf59ba..589c9e36a 100644 --- a/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_replace.yaml +++ b/tests/integration/targets/nd_vpc_pair/tasks/nd_vpc_pair_replace.yaml @@ -233,7 +233,7 @@ - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/deploy') + | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') | list | length ) > 0 @@ -421,7 +421,7 @@ - > ( result.deployment.response - | selectattr('REQUEST_PATH', 'search', '/actions/deploy') + | selectattr('REQUEST_PATH', 'search', '/switchActions/deploy') | list | length ) > 0