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