diff --git a/napalm_proxmox/vm_provision_mixin.py b/napalm_proxmox/vm_provision_mixin.py index 783e80c..c7ce580 100644 --- a/napalm_proxmox/vm_provision_mixin.py +++ b/napalm_proxmox/vm_provision_mixin.py @@ -2,7 +2,7 @@ from __future__ import annotations -import io +import base64 import logging import time import yaml @@ -130,6 +130,25 @@ class ProxmoxVMProvisionMixin: "No storage with content='images' found. Configure a storage for VM disks." ) + def _get_storage_path(self, storage: str) -> str: + """Resolve a storage's filesystem path on the node. + + Needed to write Cloud-Init snippets directly: Proxmox's + /storage/{s}/upload API only accepts content in {iso, vztmpl, + import} — "snippets" is rejected outright, so snippets must be + written straight to the filesystem instead. Only dir-backed storages + (dir, nfs, cifs, cephfs) expose "path"; those are also the only + storage types Proxmox itself allows content='snippets' on. + """ + config = self._api.storage(storage).get() + path = config.get("path") + if not path: + raise ValueError( + f"Storage '{storage}' has no filesystem path (content='snippets' " + "requires a dir/nfs/cifs/cephfs-backed storage)" + ) + return path + def get_image_storages(self) -> List[StorageTargetDict]: """List node-available storage pools suitable for a new VM's root disk.""" targets: List[StorageTargetDict] = [] @@ -336,20 +355,19 @@ class ProxmoxVMProvisionMixin: ) filename = f"{vmid}-user-data.yaml" - _logger.debug(f"Uploading Cloud-Init snippet {filename} to {snippet_storage}") + _logger.debug(f"Writing Cloud-Init snippet {filename} to {snippet_storage}") - # Upload to snippet storage. Proxmox's upload endpoint expects the - # "filename" parameter to BE the file (multipart), not a name - # string with separate content — proxmoxer only builds a - # multipart request when the value is an io.IOBase instance, - # otherwise it silently sends everything as a plain - # form-urlencoded POST, which real Proxmox rejects by dropping - # the connection (RemoteDisconnected, no HTTP response at all). - file_obj = io.BytesIO(user_data_yaml.encode("utf-8")) - file_obj.name = filename - self._node_api().storage(snippet_storage).upload.post( - content="snippets", - filename=file_obj, + # Proxmox's /storage/{s}/upload API only accepts content in + # {iso, vztmpl, import} — "snippets" is rejected outright + # ("does not have a value in the enumeration"). Snippets can only + # be written directly to the filesystem, so resolve the storage's + # backing path and write the file over SSH instead. + storage_path = self._get_storage_path(snippet_storage) + encoded = base64.b64encode(user_data_yaml.encode("utf-8")).decode("ascii") + self._run_node_command( + f"mkdir -p {storage_path}/snippets && " + f"echo {encoded} | base64 -d > {storage_path}/snippets/{filename}", + timeout=30, ) # Step 7: Configure Cloud-Init references and SSH keys diff --git a/tests/test_vm_provision_mixin.py b/tests/test_vm_provision_mixin.py index 7c3fc4a..a9d08d1 100644 --- a/tests/test_vm_provision_mixin.py +++ b/tests/test_vm_provision_mixin.py @@ -2,7 +2,7 @@ from __future__ import annotations -import io +import base64 import pytest from unittest.mock import MagicMock, patch @@ -76,6 +76,7 @@ def test_create_vm_from_cloud_init_single_nic(): # Mock API hierarchy mock_api = MagicMock() mock_api.cluster.nextid.get.return_value = 101 + mock_api.storage.return_value.get.return_value = {"path": "/var/lib/vz"} mock_node = MagicMock() mock_node.storage.get.return_value = [ @@ -103,11 +104,6 @@ def test_create_vm_from_cloud_init_single_nic(): mock_task.status.get.return_value = {"status": "stopped", "exitstatus": "OK"} mock_node.tasks.return_value = mock_task - # Mock storage upload - mock_storage = MagicMock() - mock_storage.upload.post.return_value = {"filename": "snippets:snippets/101-user-data.yaml"} - mock_node.storage.return_value = mock_storage - with patch("time.sleep"): result = mixin.create_vm_from_cloud_init( name="test-vm", @@ -132,14 +128,15 @@ def test_create_vm_from_cloud_init_single_nic(): assert "tag=10" in net_call_args[1]["net0"] assert "vmbr0" in net_call_args[1]["net0"] - # Regression: the snippet must be uploaded as an actual file (io.IOBase), - # not a plain filename string with a separate "data" field — proxmoxer - # only builds a real multipart request for io.IOBase values, and real - # Proxmox drops the connection outright for anything else (see - # test_create_vm_uploads_snippet_as_file_object for the dedicated check). - upload_kwargs = mock_storage.upload.post.call_args[1] - assert "data" not in upload_kwargs - assert isinstance(upload_kwargs["filename"], io.IOBase) + # Regression: the snippet must be written directly to the filesystem via + # SSH, not uploaded via the /storage/upload API — real Proxmox rejects + # content='snippets' on that endpoint outright (see + # test_create_vm_writes_snippet_via_ssh_with_correct_content for the + # dedicated check). + write_cmd = next( + c[0][0] for c in mixin._run_node_command.call_args_list if "snippets/101-user-data.yaml" in c[0][0] + ) + assert "/var/lib/vz/snippets" in write_cmd def test_create_vm_from_cloud_init_dual_nic_trunk(): @@ -150,6 +147,7 @@ def test_create_vm_from_cloud_init_dual_nic_trunk(): # Mock API hierarchy mock_api = MagicMock() mock_api.cluster.nextid.get.return_value = 102 + mock_api.storage.return_value.get.return_value = {"path": "/var/lib/vz"} mock_node = MagicMock() mock_node.storage.get.return_value = [ @@ -177,11 +175,6 @@ def test_create_vm_from_cloud_init_dual_nic_trunk(): mock_task.status.get.return_value = {"status": "stopped", "exitstatus": "OK"} mock_node.tasks.return_value = mock_task - # Mock storage upload - mock_storage = MagicMock() - mock_storage.upload.post.return_value = {"filename": "snippets:snippets/102-user-data.yaml"} - mock_node.storage.return_value = mock_storage - with patch("time.sleep"): result = mixin.create_vm_from_cloud_init( name="wireshark-sat-1", @@ -265,6 +258,7 @@ def test_create_vm_with_disk_resize(): # Mock API hierarchy mock_api = MagicMock() mock_api.cluster.nextid.get.return_value = 103 + mock_api.storage.return_value.get.return_value = {"path": "/var/lib/vz"} mock_node = MagicMock() mock_node.storage.get.return_value = [ @@ -293,11 +287,6 @@ def test_create_vm_with_disk_resize(): mock_task.status.get.return_value = {"status": "stopped", "exitstatus": "OK"} mock_node.tasks.return_value = mock_task - # Mock storage upload - mock_storage = MagicMock() - mock_storage.upload.post.return_value = {"filename": "snippets:snippets/103-user-data.yaml"} - mock_node.storage.return_value = mock_storage - with patch("time.sleep"): result = mixin.create_vm_from_cloud_init( name="big-vm", @@ -722,6 +711,42 @@ def test_find_default_image_storage_excludes_storage_restricted_to_other_nodes() assert storage == "local-zfs" +# --------------------------------------------------------------------------- +# _get_storage_path +# --------------------------------------------------------------------------- + + +def test_get_storage_path_returns_path_from_cluster_config(): + mixin = ProxmoxVMProvisionMixin() + mixin._api = MagicMock() + mixin._api.storage.return_value.get.return_value = { + "storage": "local", + "type": "dir", + "path": "/var/lib/vz", + } + + path = mixin._get_storage_path("local") + + assert path == "/var/lib/vz" + mixin._api.storage.assert_called_with("local") + + +def test_get_storage_path_raises_when_storage_has_no_path(): + """A storage type without a filesystem path (e.g. lvmthin, zfspool) can't + back content='snippets' at all — Proxmox itself only allows that content + type on dir/nfs/cifs/cephfs storages, so this should never actually be + reached for a real snippet storage, but must fail clearly if it is.""" + mixin = ProxmoxVMProvisionMixin() + mixin._api = MagicMock() + mixin._api.storage.return_value.get.return_value = { + "storage": "local-lvm", + "type": "lvmthin", + } + + with pytest.raises(ValueError, match="path"): + mixin._get_storage_path("local-lvm") + + def test_create_vm_from_cloud_init_with_real_world_storage_shape(): """Regression: real Proxmox servers omit "enabled" for never-toggled storages (observed live: local-lvm/local-zfs/fast-zfs all had content=images but no @@ -733,6 +758,7 @@ def test_create_vm_from_cloud_init_with_real_world_storage_shape(): mock_api = MagicMock() mock_api.cluster.nextid.get.return_value = 104 + mock_api.storage.return_value.get.return_value = {"path": "/var/lib/vz"} mock_node = MagicMock() mock_node.storage.get.return_value = [ @@ -760,10 +786,6 @@ def test_create_vm_from_cloud_init_with_real_world_storage_shape(): mock_task.status.get.return_value = {"status": "stopped", "exitstatus": "OK"} mock_node.tasks.return_value = mock_task - mock_storage = MagicMock() - mock_storage.upload.post.return_value = {"filename": "snippets:snippets/104-user-data.yaml"} - mock_node.storage.return_value = mock_storage - with patch("time.sleep"): result = mixin.create_vm_from_cloud_init( name="real-shape-vm", @@ -786,6 +808,7 @@ def test_create_vm_from_cloud_init_explicit_storage_skips_auto_detect(): mock_api = MagicMock() mock_api.cluster.nextid.get.return_value = 105 + mock_api.storage.return_value.get.return_value = {"path": "/var/lib/vz"} mock_node = MagicMock() mock_node.storage.get.return_value = [ @@ -813,10 +836,6 @@ def test_create_vm_from_cloud_init_explicit_storage_skips_auto_detect(): mock_task.status.get.return_value = {"status": "stopped", "exitstatus": "OK"} mock_node.tasks.return_value = mock_task - mock_storage = MagicMock() - mock_storage.upload.post.return_value = {"filename": "snippets:snippets/105-user-data.yaml"} - mock_node.storage.return_value = mock_storage - with patch("time.sleep"): result = mixin.create_vm_from_cloud_init( name="explicit-storage-vm", @@ -901,21 +920,22 @@ def test_get_image_storages_excludes_storage_restricted_to_other_nodes(): # --------------------------------------------------------------------------- -def test_create_vm_uploads_snippet_as_file_object_with_correct_content(): - """Regression: real Proxmox's /storage/{s}/upload endpoint expects the - "filename" parameter to be the file itself (multipart). proxmoxer only - builds a multipart request when the value is an io.IOBase instance — - passing a plain string (with a separate, nonexistent "data" field, as the - old code did) makes proxmoxer send a normal form-urlencoded POST instead, - which real Proxmox responds to by closing the connection outright - (observed live: requests.exceptions.ConnectionError / - RemoteDisconnected('Remote end closed connection without response'), - after the VM shell and disk import had already succeeded).""" +def test_create_vm_writes_snippet_via_ssh_with_correct_content(): + """Regression: real Proxmox's /storage/{s}/upload endpoint rejects + content='snippets' outright ("value 'snippets' does not have a value in + the enumeration 'iso, vztmpl, import'") — that endpoint only handles + ISOs, container templates, and imports. Snippets can only be written + directly to the storage's filesystem path, so this must go through SSH + (_run_node_command), not the upload API. + (observed live: 400 Bad Request from Proxmox, right after the earlier + multipart-upload fix had already gotten past a prior RemoteDisconnected + bug at the same step — two distinct real-world failures at this line).""" mixin = ProxmoxVMProvisionMixin() mixin._node_name = "pve1" mock_api = MagicMock() mock_api.cluster.nextid.get.return_value = 106 + mock_api.storage.return_value.get.return_value = {"path": "/mnt/pve/snippet-nfs"} mock_node = MagicMock() mock_node.storage.get.return_value = [ @@ -941,28 +961,29 @@ def test_create_vm_uploads_snippet_as_file_object_with_correct_content(): mock_task.status.get.return_value = {"status": "stopped", "exitstatus": "OK"} mock_node.tasks.return_value = mock_task - mock_storage = MagicMock() - mock_storage.upload.post.return_value = {"filename": "local:snippets/106-user-data.yaml"} - mock_node.storage.return_value = mock_storage - with patch("time.sleep"): mixin.create_vm_from_cloud_init( - name="upload-shape-vm", + name="write-shape-vm", image_url="https://cloud.debian.org/images/cloud/bookworm/latest/debian-12-genericcloud-amd64.qcow2", cpu=2, memory=2048, nics=[{"bridge": "vmbr0"}], - cloud_init_config={"hostname": "upload-shape-vm", "chpasswd": {"expire": False}}, + cloud_init_config={"hostname": "write-shape-vm", "chpasswd": {"expire": False}}, ) - upload_call = mock_storage.upload.post.call_args - assert upload_call[1]["content"] == "snippets" - assert "data" not in upload_call[1] + # No call to the upload API at all — snippets aren't a valid content + # type there. + mock_node.storage.assert_not_called() - file_obj = upload_call[1]["filename"] - assert isinstance(file_obj, io.IOBase) - assert file_obj.name == "106-user-data.yaml" - file_obj.seek(0) - content = file_obj.read().decode("utf-8") + write_cmd = next( + c[0][0] + for c in mixin._run_node_command.call_args_list + if "snippets/106-user-data.yaml" in c[0][0] + ) + assert "mkdir -p /mnt/pve/snippet-nfs/snippets" in write_cmd + assert "/mnt/pve/snippet-nfs/snippets/106-user-data.yaml" in write_cmd + + encoded = write_cmd.split("echo ", 1)[1].split(" | base64 -d")[0] + content = base64.b64decode(encoded).decode("utf-8") assert content.startswith("#cloud-config\n") - assert "hostname: upload-shape-vm" in content + assert "hostname: write-shape-vm" in content