Merge pull request 'fix: resolve the node the connection landed on, not the first cluster member' (#2) from fix/cluster-node-resolution into master

Reviewed-on: #2

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
+        members = [
-        try:
+            s for s in (self._api.cluster.status.get() or []) if s.get("type") == "node"
-            nodes = self._api.nodes.get()
+        ]
-            # Match by hostname or IP
+        short_host = self.hostname.split(".")[0]
-            for n in nodes:
+        for pick in (
-                n_node = n.get("node", "")
+            lambda s: s.get("local"),
-                if n_node:
+            lambda s: s.get("ip") == self.hostname,
-                    # First match: the node exists in the cluster
+            lambda s: s.get("name") in (self.hostname, short_host),
-                    self._node_name = n_node
+        ):
-                    return self._node_name
+            match = next((s for s in members if pick(s)), None)
-        except Exception:
+            if match:
-            pass
+                self._node_name = match["name"]
+                return self._node_name
 
-        # Fallback: use the configured hostname as node name
+        names = [n.get("node") for n in (self._api.nodes.get() or []) if n.get("node")]
-        # (may not match the PVE node name — SSH-based methods will fail,
+        if len(names) == 1:
-        # but API methods that target a specific node name require correct
+            self._node_name = names[0]
-        # resolution)
+            return self._node_name
-        self._node_name = self.hostname
+
-        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."""

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
 

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