From abce85d6efeb9dfd9bac0c6c83f91d61b3f4496a Mon Sep 17 00:00:00 2001 From: Christian Manivong Date: Tue, 29 Sep 2026 10:37:01 +0200 Subject: [PATCH] fix: resolve the node the connection landed on, not the first cluster member _resolve_node() took the first entry of GET /nodes. In a cluster that lists every member, so a node polled without an explicit `node` driver argument talked to whichever member came first: pve-dual reported pve-02's name and VMs, and netOrk's VM sync moved pve-02's VM devices over to it. Resolve through GET /cluster/status instead: the entry marked local, then a match by IP or (short) name, then the sole node of a standalone host, and otherwise raise rather than guess. The lookup no longer swallows API errors either. A TLS verification failure used to leave the IP as the node name, so open() succeeded and every getter failed quietly while the poll reported success with empty data. It now surfaces as a ConnectionException from open(). Refs NetOrk/netork#417, NetOrk/netork#418 --- napalm_proxmox/driver.py | 52 ++++++++++++++++++---------- tests/conftest.py | 2 ++ tests/test_connection.py | 75 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 110 insertions(+), 19 deletions(-) diff --git a/napalm_proxmox/driver.py b/napalm_proxmox/driver.py index d419716..81c164a 100644 --- a/napalm_proxmox/driver.py +++ b/napalm_proxmox/driver.py @@ -183,30 +183,44 @@ class ProxmoxDriver( ) from exc def _resolve_node(self) -> str: - """Resolve the node name from the hostname or optional_args.""" + """Resolve the PVE node this connection talks to. + + In a cluster, ``GET /nodes`` lists every member, so its first entry is + just some node, not necessarily the one at ``self.hostname``. That used + to be taken blindly, and a device then reported another node's name and + VMs. ``GET /cluster/status`` marks the node the session landed on with + ``local: 1``; failing that, the node is matched by IP or name. + + API errors propagate so that ``open()`` fails with the real cause (e.g. + a TLS verification error) rather than succeeding on a guessed name. + """ if self._node: self._node_name = self._node return self._node_name - # Try the node's hostname via API cluster/resources - try: - nodes = self._api.nodes.get() - # Match by hostname or IP - for n in nodes: - n_node = n.get("node", "") - if n_node: - # First match: the node exists in the cluster - self._node_name = n_node - return self._node_name - except Exception: - pass + members = [ + s for s in (self._api.cluster.status.get() or []) if s.get("type") == "node" + ] + short_host = self.hostname.split(".")[0] + for pick in ( + lambda s: s.get("local"), + lambda s: s.get("ip") == self.hostname, + lambda s: s.get("name") in (self.hostname, short_host), + ): + match = next((s for s in members if pick(s)), None) + if match: + self._node_name = match["name"] + return self._node_name - # Fallback: use the configured hostname as node name - # (may not match the PVE node name — SSH-based methods will fail, - # but API methods that target a specific node name require correct - # resolution) - self._node_name = self.hostname - return self._node_name + names = [n.get("node") for n in (self._api.nodes.get() or []) if n.get("node")] + if len(names) == 1: + self._node_name = names[0] + return self._node_name + + raise ConnectionException( + f"Cannot tell which PVE node {self.hostname} is among {names}; " + f"set the 'node' driver argument" + ) def close(self) -> None: """Close the connection.""" diff --git a/tests/conftest.py b/tests/conftest.py index 1bd20a0..3c9b320 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -138,6 +138,7 @@ def _build_mock_api( pve_users=None, exec_return="", sensors=None, + cluster_status=None, ): """Build a MagicMock ProxmoxAPI with pre-configured return values.""" api = MagicMock() @@ -165,6 +166,7 @@ def _build_mock_api( # SDN cluster = MagicMock() api.cluster = cluster + cluster.status.get.return_value = cluster_status or [] cluster.sdn.zones.get.return_value = sdn_zones or SDN_ZONES cluster.sdn.vnets.get.return_value = sdn_vnets or SDN_VNETS diff --git a/tests/test_connection.py b/tests/test_connection.py index e17fc43..7687c3f 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -76,3 +76,78 @@ class TestIsAlive: def test_not_alive_when_api_fails(self, driver): driver._api.version.get.side_effect = Exception("timeout") assert driver.is_alive() == {"is_alive": False} + + +# A four-node cluster as GET /cluster/status reports it. The node the API +# session landed on carries local=1 — here the third one, so taking the first +# entry of /nodes (the old behaviour) picks the wrong host. +CLUSTER_NODES = [ + {"type": "node", "name": "pve-01", "ip": "172.22.8.101", "local": 0}, + {"type": "node", "name": "pve-02", "ip": "172.22.8.102", "local": 0}, + {"type": "node", "name": "pve-dual", "ip": "172.22.8.120", "local": 1}, + {"type": "node", "name": "pve-garden", "ip": "172.22.8.5", "local": 0}, +] +CLUSTER_STATUS = [{"type": "cluster", "name": "home", "nodes": 4}, *CLUSTER_NODES] +CLUSTER_NODES_LIST = [{"node": n["name"], "status": "online"} for n in CLUSTER_NODES] + + +def _open_with(hostname, **api_kwargs): + drv = ProxmoxDriver(hostname, "root", "secret") + mock_api = _build_mock_api(**api_kwargs) + with patch("napalm_proxmox.driver.ProxmoxAPI", return_value=mock_api): + drv.open() + return drv + + +class TestClusterNodeResolution: + def test_local_node_wins_over_first_listed(self): + drv = _open_with( + "172.22.8.120", nodes=CLUSTER_NODES_LIST, cluster_status=CLUSTER_STATUS + ) + assert drv._node_name == "pve-dual" + + def test_matches_by_ip_without_local_flag(self): + status = [{**n, "local": 0} for n in CLUSTER_STATUS if n["type"] == "node"] + drv = _open_with("172.22.8.102", nodes=CLUSTER_NODES_LIST, cluster_status=status) + assert drv._node_name == "pve-02" + + def test_matches_by_name_without_local_flag(self): + status = [{**n, "local": 0} for n in CLUSTER_STATUS if n["type"] == "node"] + drv = _open_with("pve-garden", nodes=CLUSTER_NODES_LIST, cluster_status=status) + assert drv._node_name == "pve-garden" + + def test_matches_short_name_of_fqdn(self): + status = [{**n, "local": 0} for n in CLUSTER_STATUS if n["type"] == "node"] + drv = _open_with( + "pve-01.mgmt.example.com", nodes=CLUSTER_NODES_LIST, cluster_status=status + ) + assert drv._node_name == "pve-01" + + def test_single_node_without_cluster_status(self): + drv = _open_with("10.0.0.9", nodes=[{"node": "solo", "status": "online"}]) + assert drv._node_name == "solo" + + def test_ambiguous_cluster_refuses_to_guess(self): + status = [{**n, "local": 0} for n in CLUSTER_STATUS if n["type"] == "node"] + with pytest.raises(ConnectionException, match="node"): + _open_with("10.9.9.9", nodes=CLUSTER_NODES_LIST, cluster_status=status) + + def test_api_error_fails_open_instead_of_guessing(self): + # A TLS failure used to be swallowed here: the IP became the node + # name, open() succeeded and every later call failed quietly. + drv = ProxmoxDriver("172.22.8.120", "root", "secret") + mock_api = _build_mock_api() + mock_api.cluster.status.get.side_effect = Exception("CERTIFICATE_VERIFY_FAILED") + mock_api.nodes.get.side_effect = Exception("CERTIFICATE_VERIFY_FAILED") + with patch("napalm_proxmox.driver.ProxmoxAPI", return_value=mock_api): + with pytest.raises(ConnectionException, match="CERTIFICATE_VERIFY_FAILED"): + drv.open() + + def test_explicit_node_skips_lookup(self): + drv = ProxmoxDriver("172.22.8.120", "root", "secret", optional_args={"node": "pve-dual"}) + mock_api = _build_mock_api() + with patch("napalm_proxmox.driver.ProxmoxAPI", return_value=mock_api): + drv.open() + assert drv._node_name == "pve-dual" + mock_api.cluster.status.get.assert_not_called() + mock_api.nodes.get.assert_not_called() -- 2.54.0