-
Notifications
You must be signed in to change notification settings - Fork 27
Fix manual_underlay_allocation with TOR switches #866
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from 3 commits
0c8f210
ffae49c
2341c8c
ddefdd5
de89ef1
829b369
0e1049e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -48,6 +48,11 @@ def match(cls, data_model): | |||||||
| switches = cls.safeget(data_model, ["vxlan", "topology", "switches"]) | ||||||||
| for switch in switches: | ||||||||
| switch_name = switch.get("name") | ||||||||
| switch_role = switch.get("role", "").lower() | ||||||||
|
|
||||||||
| if switch_role == "tor": | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Low: The source documentation still instructs users to configure values the PR rejectsIssue The implementation introduces a ToR exception to manual underlay allocation, but the authoritative model documentation still says every non-spine needs routing and VTEP loopback addresses and broadly describes Evidence
Practical example A user following the current manual-allocation documentation could reasonably configure this ToR vPC pair: vxlan:
underlay:
general:
manual_underlay_allocation: true
underlay_routing_loopback_id: 0
underlay_vtep_loopback_id: 1
topology:
switches:
- name: dc1-tor1
role: tor
interfaces:
- name: Loopback0
mode: fabric_loopback
ipv4_address: 10.0.0.31
- name: Loopback1
mode: fabric_loopback
ipv4_address: 10.1.0.31
- name: dc1-tor2
role: tor
interfaces:
- name: Loopback0
mode: fabric_loopback
ipv4_address: 10.0.0.32
- name: Loopback1
mode: fabric_loopback
ipv4_address: 10.1.0.32
vpc_peers:
- peer1: dc1-tor1
peer2: dc1-tor2
fabric_peering: false
vtep_vip: 10.1.0.30This follows the documented interpretation: routing and VTEP loopback IPs are required for every role except To pass the new validation, the user must remove the ToR underlay loopback addresses and Existing PR overlap No matching existing PR comment found. Impact Users following the current source documentation can add ToR loopbacks or a ToR VIP and receive the new Rule 208 errors. Conversely, users cannot discover from the manual-allocation guide which ToR values must be omitted. Suggested fix Update the model repository's manual-allocation and vPC/ToR documentation in the coordinated change, including iBGP/eBGP applicability, ToR loopback/VIP/fabric-link rules, and a minimal ToR example. Add a changelog/release note if that is the project's release convention. |
||||||||
| cls.validate_tor_no_underlay(switch, switch_name, underlay_routing_loopback_id, underlay_vtep_loopback_id) | ||||||||
| continue | ||||||||
|
|
||||||||
| if "interfaces" not in switch: | ||||||||
| cls.results.append(f"Missing interfaces in vxlan.topology.switches.{switch_name}") | ||||||||
|
|
@@ -67,18 +72,68 @@ def match(cls, data_model): | |||||||
| vtep_loopback_name = f"loopback{underlay_vtep_loopback_id}" | ||||||||
| vtep_loopback_found = cls.check_interface_with_ipv4(interfaces, vtep_loopback_name) | ||||||||
|
|
||||||||
| switch_role = switch.get("role", "").lower() | ||||||||
|
|
||||||||
| if not vtep_loopback_found and switch_role != "spine": | ||||||||
| if not vtep_loopback_found and switch_role not in ["spine"]: | ||||||||
| cls.results.append( | ||||||||
| f"Switch '{switch_name}' is missing a configured interface '{vtep_loopback_name}' with an IPv4 address." | ||||||||
| ) | ||||||||
|
|
||||||||
| # Check if vtep_ip exist in vpc_peers | ||||||||
| cls.validate_vpc_peers_and_vtep_vip(data_model) | ||||||||
|
|
||||||||
| # Check if TOR switches have IPv4 on fabric links | ||||||||
| cls.validate_tor_no_fabric_link_ipv4(data_model) | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Medium: ToR restrictions do not run for eBGP VXLAN fabricsIssue The new ToR semantic checks were added only to the iBGP Rule 208 path. The modified underlay resource template is active for both VXLAN_EVPN and eBGP_VXLAN, so an eBGP model can still render prohibited ToR fabric-link allocations. Evidence
Existing PR overlap No matching existing PR comment was found. The approval reports VXLAN_EVPN testing only and does not cover eBGP routing. Impact Equivalent ToR models behave differently depending on fabric type. On eBGP_VXLAN, underlay allocations can reach NDFC for a ToR that this PR says should not participate in the VXLAN underlay. Suggested fix Move the fabric-independent manual-allocation and ToR checks into |
||||||||
|
|
||||||||
| return cls.results | ||||||||
|
|
||||||||
| @classmethod | ||||||||
| def validate_tor_no_underlay(cls, switch, switch_name, routing_lo_id, vtep_lo_id): | ||||||||
| """ | ||||||||
| Validates that TOR switches do not have underlay IP configuration. | ||||||||
| TOR switches do not participate in the VXLAN underlay. | ||||||||
| """ | ||||||||
| interfaces = switch.get("interfaces", []) | ||||||||
| for interface in interfaces: | ||||||||
| intf_name = interface.get("name", "").lower() | ||||||||
| if intf_name in (f"loopback{routing_lo_id}", f"lo{routing_lo_id}", | ||||||||
| f"loopback{vtep_lo_id}", f"lo{vtep_lo_id}"): | ||||||||
| if interface.get("ipv4_address"): | ||||||||
| cls.results.append( | ||||||||
| f"TOR switch '{switch_name}': underlay loopback '{interface.get('name')}' with IPv4 " | ||||||||
| "should not be defined (TOR switches do not participate in VXLAN underlay)." | ||||||||
| ) | ||||||||
|
|
||||||||
| @classmethod | ||||||||
| def validate_tor_no_fabric_link_ipv4(cls, data_model): | ||||||||
| """ | ||||||||
| Validates that fabric links involving TOR switches do not have IPv4 underlay configuration. | ||||||||
| """ | ||||||||
| check = cls.data_model_key_check(data_model, ["vxlan", "topology", "fabric_links"]) | ||||||||
| if 'fabric_links' not in check['keys_data']: | ||||||||
| return | ||||||||
|
|
||||||||
| switches = cls.safeget(data_model, ["vxlan", "topology", "switches"]) or [] | ||||||||
| tor_names = {sw.get("name", "") for sw in switches if sw.get("role", "").lower() == "tor"} | ||||||||
|
|
||||||||
| if not tor_names: | ||||||||
| return | ||||||||
|
|
||||||||
| fabric_links = cls.safeget(data_model, ["vxlan", "topology", "fabric_links"]) | ||||||||
| for link in fabric_links: | ||||||||
| src = link.get("source_device", "") | ||||||||
| dst = link.get("dest_device", "") | ||||||||
| ipv4_config = link.get("ipv4", {}) | ||||||||
|
|
||||||||
| if not ipv4_config: | ||||||||
| continue | ||||||||
|
|
||||||||
| if src in tor_names or dst in tor_names: | ||||||||
| tor_device = src if src in tor_names else dst | ||||||||
| if ipv4_config.get("subnet") or ipv4_config.get("source_ipv4") or ipv4_config.get("dest_ipv4"): | ||||||||
| cls.results.append( | ||||||||
| f"Fabric link '{src}' → '{dst}': IPv4 underlay configuration should not be defined " | ||||||||
| f"for TOR switch '{tor_device}' (TOR switches do not participate in VXLAN underlay)." | ||||||||
| ) | ||||||||
|
|
||||||||
| @classmethod | ||||||||
| def validate_vpc_peers_and_vtep_vip(cls, data_model): | ||||||||
| """ | ||||||||
|
|
@@ -89,14 +144,26 @@ def validate_vpc_peers_and_vtep_vip(cls, data_model): | |||||||
| # If switches key is missing, no need to proceed | ||||||||
| return cls.results | ||||||||
| vpc_peers = cls.safeget(data_model, ["vxlan", "topology", "vpc_peers"]) | ||||||||
| switches = cls.safeget(data_model, ["vxlan", "topology", "switches"]) or [] | ||||||||
| switch_role_map = {sw.get("name", ""): sw.get("role", "").lower() for sw in switches} | ||||||||
| vtep_vip_list = set() | ||||||||
| vpc_peers_list = [] | ||||||||
|
|
||||||||
| for peer in vpc_peers: | ||||||||
| peer_name = f"{peer.get('peer1')}-{peer.get('peer2')}" | ||||||||
| vpc_peers_list.append(peer_name) | ||||||||
|
|
||||||||
| peer1_role = switch_role_map.get(peer.get("peer1", ""), "") | ||||||||
| peer2_role = switch_role_map.get(peer.get("peer2", ""), "") | ||||||||
| if peer1_role == "tor" or peer2_role == "tor": | ||||||||
| if peer.get("vtep_vip"): | ||||||||
| cls.results.append( | ||||||||
| f"vPC peer '{peer_name}': vtep_vip should not be defined for TOR switches " | ||||||||
| "(TOR switches do not participate in VXLAN underlay)." | ||||||||
| ) | ||||||||
| continue | ||||||||
|
|
||||||||
| vtep_vip = peer.get("vtep_vip", False) | ||||||||
| # Check if vtep_vip is defined | ||||||||
| if not vtep_vip: | ||||||||
| cls.results.append(f"vPC peer '{peer_name}' is missing a defined vtep_vip address.") | ||||||||
| continue | ||||||||
|
|
@@ -114,7 +181,47 @@ def validate_vpc_peers_and_vtep_vip(cls, data_model): | |||||||
| # Check IP address under vxlan.topology.fabric_link only if | ||||||||
| # fabric numbering is P2P or fabric peering is false (Use Fabric Peer-Link) | ||||||||
| if peer.get("fabric_peering") is False or interface_numbering["fabric_interface_numbering"] == "p2p": | ||||||||
| cls.validate_fabric_links(data_model, vpc_peers_list) | ||||||||
| if interface_numbering["fabric_interface_numbering"] == "p2p": | ||||||||
| cls.validate_fabric_links(data_model, vpc_peers_list) | ||||||||
| else: | ||||||||
| cls.validate_vpc_peer_fabric_link(data_model, peer) | ||||||||
|
|
||||||||
| @classmethod | ||||||||
| def validate_vpc_peer_fabric_link(cls, data_model, peer): | ||||||||
| """ | ||||||||
| Validates that a specific vPC peer with fabric_peering=false has a fabric link with IPv4 config. | ||||||||
| Only checks the link between the two peers, not all switches. | ||||||||
| """ | ||||||||
| peer1 = peer.get("peer1") | ||||||||
| peer2 = peer.get("peer2") | ||||||||
|
|
||||||||
| check = cls.data_model_key_check(data_model, ["vxlan", "topology", "fabric_links"]) | ||||||||
| if 'fabric_links' not in check['keys_data']: | ||||||||
| cls.results.append( | ||||||||
| f"Fabric link between '{peer1}' and '{peer2}' is missing (required for vPC with fabric_peering=false)." | ||||||||
| ) | ||||||||
| return | ||||||||
|
|
||||||||
| fabric_links = cls.safeget(data_model, ["vxlan", "topology", "fabric_links"]) | ||||||||
| peer_link = None | ||||||||
| for link in fabric_links: | ||||||||
| src = link.get("source_device", "") | ||||||||
| dst = link.get("dest_device", "") | ||||||||
| if (src == peer1 and dst == peer2) or (src == peer2 and dst == peer1): | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Medium: The new peer-link validator rejects a valid later matchIssue
Evidence
Practical example A vPC pair can have two modeled links between the same switches: vxlan:
topology:
vpc_peers:
- peer1: dc1-leaf1
peer2: dc1-leaf2
fabric_peering: false
fabric_links:
# Valid unnumbered physical link
- source_device: dc1-leaf1
source_interface: Ethernet1/49
dest_device: dc1-leaf2
dest_interface: Ethernet1/49
# Numbered backup link required for underlay peering
- source_device: dc1-leaf1
source_interface: Vlan3600
dest_device: dc1-leaf2
dest_interface: Vlan3600
ipv4:
subnet: 10.5.0.0/31
source_ipv4: 10.5.0.0
dest_ipv4: 10.5.0.1The loop examines only the device names and stops at the first match: for link in fabric_links:
if link connects dc1-leaf1 and dc1-leaf2:
peer_link = link
breakIt therefore selects The complete
Existing PR overlap No matching existing PR comment found. Impact Validation becomes dependent on list order and can reject a topology that contains a complete configured vPC backup link. Reordering the same two links can change the result without changing intent. Suggested fix Collect all endpoint matches and succeed when any matching link has |
||||||||
| peer_link = link | ||||||||
| break | ||||||||
|
|
||||||||
| if not peer_link: | ||||||||
| cls.results.append( | ||||||||
| f"Fabric link between '{peer1}' and '{peer2}' is missing (required for vPC with fabric_peering=false)." | ||||||||
| ) | ||||||||
| return | ||||||||
|
|
||||||||
| ipv4_config = peer_link.get("ipv4", {}) | ||||||||
| if not ipv4_config or not ipv4_config.get("subnet") or not ipv4_config.get("source_ipv4") or not ipv4_config.get("dest_ipv4"): | ||||||||
| cls.results.append( | ||||||||
| f"Fabric link between '{peer1}' and '{peer2}' is missing IPv4 configuration (required for vPC with fabric_peering=false)." | ||||||||
| ) | ||||||||
|
|
||||||||
| @classmethod | ||||||||
| def validate_fabric_links(cls, data_model, vpc_peers_list): | ||||||||
|
|
@@ -154,7 +261,7 @@ def validate_fabric_links(cls, data_model, vpc_peers_list): | |||||||
| # and P2P configured | ||||||||
|
|
||||||||
| switches = cls.safeget(data_model, ["vxlan", "topology", "switches"]) | ||||||||
| switch_names = [switch.get("name") for switch in switches if switch.get("role", "").lower() not in ["spine", "border_gateway_spine"]] | ||||||||
| switch_names = [switch.get("name") for switch in switches if switch.get("role", "").lower() not in ["spine", "border_gateway_spine", "tor"]] | ||||||||
|
|
||||||||
| # If switch in switch_names not in regex fabric_links_list: | ||||||||
| # Switch doesn't have fabric link configured | ||||||||
|
|
@@ -184,10 +291,15 @@ def validate_fabric_links(cls, data_model, vpc_peers_list): | |||||||
|
|
||||||||
| # Check if vpc_peers is on fabric_link | ||||||||
| vpc_peers = cls.safeget(data_model, ["vxlan", "topology", "vpc_peers"]) | ||||||||
| switch_role_map = {sw.get("name", ""): sw.get("role", "").lower() for sw in switches} | ||||||||
| for peer in vpc_peers: | ||||||||
| peer1 = f"{peer.get('peer1')}-{peer.get('peer2')}" | ||||||||
| peer2 = f"{peer.get('peer2')}-{peer.get('peer1')}" | ||||||||
| fabric_peering = peer.get("fabric_peering", False) | ||||||||
|
|
||||||||
| if switch_role_map.get(peer.get("peer1", ""), "") == "tor" or switch_role_map.get(peer.get("peer2", ""), "") == "tor": | ||||||||
| continue | ||||||||
|
|
||||||||
| # Skip fabric link validation when fabric_peering is true | ||||||||
| # because vPC uses virtual peer-link instead of physical fabric links | ||||||||
| if fabric_peering is False: | ||||||||
|
|
||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Medium: Existing ToR pool allocations are omitted but never released
Issue
The changed template stops rendering routing-loopback, VTEP-loopback, and vPC VIP resource-manager entries for ToR switches, but neither the create nor remove pipeline releases entries which were allocated by an earlier run. This is a state-transition defect: the desired file becomes correct while NDFC retains the old allocation.
Evidence
ndfc_underlay_ip_address.j2:28, and the new peer-role gate similarly omits the vPC VIP entry at line 47.underlay_ip_addressis structurally diffed byentity_name. A focused old-versus-new fixture containing one former ToR allocation producedupdated: 0,removed: 1,equal: 0.cisco.dcnm.dcnm_resource_managerwithstate: merged(resources/create_resources.yml, VXLAN_EVPN lines 96-108 and eBGP_VXLAN lines 286-298). In a selective run,ResourceManager._resolve_create_data()consumes onlydiff.updated; it never consumesdiff.removed. In a full run, the remote audit compares only desired entries, so an omitted controller entry is not selected either.underlay_ip_addressstep inresources/remove_resources.yml. At both the declared floorcisco.dcnm3.12.1 (6311b66ff02b8ed593cbbfed4abdd502607dd6d2) and current inspected ref939c75bb667498631e9047ff9f62f424f00b98c2, thedcnm_resource_managercontract saysmergedleaves unspecified resources untouched anddeletedreleases resources explicitly supplied inconfig.Practical example
Before this change, assume manual allocation created these controller resources for ToR
TOR1:After upgrading, the user removes those ToR loopbacks from the model to comply with the new validation. The desired file correctly omits them, and the structural result is effectively:
The selective create path has nothing in
diff.updated, and no remove step consumes these two entries. A forced full run still usesstate: merged, so omission does not release them. NDFC therefore continues to reserve10.10.0.21and10.20.0.21forTOR1; a later attempt to reuse either address can fail even though neither appears in the current NaC model.Existing PR overlap
No matching existing PR comment found. The visible approval tested creation from a happy-path ToR topology; it did not test migration from a previously rendered ToR allocation or the last-item-removed transition.
Impact
An environment that previously ran manual allocation with ToR loopbacks or a ToR vPC VIP can continue to reserve those IPs after upgrading to this fix. The stale reservations can exhaust pools, block reuse, or make controller state disagree with the saved NaC artifact. Both
diff_run=trueand a full reconciliation leave them untouched.Suggested fix
Add an underlay-resource removal step that sends
underlay_ip_address.diff.removedtocisco.dcnm.dcnm_resource_managerwithstate: deleted, with appropriate opt-in/safety semantics if resource release is intentionally guarded. Test one removed ToR loopback, one removed ToR vPC VIP, mixed removed-and-updated entries, the last allocation removed, a full run, a selective run, and a no-change rerun.