From 8d7f28482fad48ad372d06c6977ba0bbb383eb2e Mon Sep 17 00:00:00 2001 From: catborise Date: Mon, 26 Oct 2026 17:11:11 +0300 Subject: [PATCH] fix(api): VM API answers as the VM pages do - A VM delete reads delete_disk and delete_nvram as booleans from a JSON or form body. The view read them as HTML checkboxes from the form body only: JSON {"delete_disk": true} kept the disks while reporting success, and a form "delete_disk=false" deleted them. Refused options stop nothing; the API schema names both options. - Power on and power cycle of a template answer 409 instead of reporting a start that did not happen. - The VM details leave out the host's ISO images for non-superusers, as the VM page does; reading them also refreshed every pool of the host. - Flavors take values of at least 1, as the web form does, and show their id for later changes; a migration needs neither live nor offline; a VM create on an unknown compute answers 404 instead of 500. --- instances/api/serializers.py | 24 +++++++++++- instances/api/viewsets.py | 7 +++- instances/jobs.py | 3 +- instances/tests/test_api_parity.py | 62 ++++++++++++++++++++++++++++++ instances/tests/test_destroy.py | 20 ++++++++++ instances/views.py | 9 ++++- 6 files changed, 119 insertions(+), 6 deletions(-) create mode 100644 instances/tests/test_api_parity.py diff --git a/instances/api/serializers.py b/instances/api/serializers.py index 28b85a08..00c2f0e5 100644 --- a/instances/api/serializers.py +++ b/instances/api/serializers.py @@ -10,6 +10,15 @@ class InstanceSerializer(serializers.ModelSerializer): class InstanceDetailsSerializer(serializers.ModelSerializer): + def get_fields(self): + fields = super().get_fields() + # the host's ISO images are a superuser's, as on the VM page; reading + # them refreshes every pool of the host + request = self.context.get("request") + if not (request and request.user.is_superuser): + del fields["media_iso"] + return fields + class Meta: model = Instance fields = [ @@ -49,7 +58,17 @@ class InstanceDetailsSerializer(serializers.ModelSerializer): class FlavorSerializer(serializers.ModelSerializer): class Meta: model = Flavor - fields = ["label", "memory", "vcpu", "disk"] + fields = ["id", "label", "memory", "vcpu", "disk"] + # as the web form (instances.forms.FlavorForm): MiB, vCPUs and GiB, each at least 1 + extra_kwargs = {"memory": {"min_value": 1}, "vcpu": {"min_value": 1}, "disk": {"min_value": 1}} + + +class DestroyOptionsSerializer(serializers.Serializer): + """What a VM's delete takes besides the VM: from the web form's checkboxes + or an API body, JSON or form-encoded ("false" is false).""" + + delete_disk = serializers.BooleanField(default=False) + delete_nvram = serializers.BooleanField(default=False) class CreateInstanceSerializer(serializers.ModelSerializer): @@ -92,7 +111,8 @@ class CreateInstanceSerializer(serializers.ModelSerializer): class MigrateSerializer(serializers.ModelSerializer): instance = Instance.objects.all().prefetch_related("userinstance_set") - live = serializers.BooleanField(initial=True) + live = serializers.BooleanField(default=True) + offline = serializers.BooleanField(default=False) # left out, also from a form-encoded request: auto converge on autoconverge = serializers.BooleanField(default=True) compress = serializers.BooleanField(default=False) diff --git a/instances/api/viewsets.py b/instances/api/viewsets.py index 967d3343..acdcf24a 100644 --- a/instances/api/viewsets.py +++ b/instances/api/viewsets.py @@ -26,6 +26,7 @@ from webvirtcloud.serializers import StatusSerializer from .serializers import ( CreateInstanceSerializer, + DestroyOptionsSerializer, FlavorSerializer, InstanceDetailsSerializer, InstanceSerializer, @@ -119,7 +120,7 @@ class InstanceViewSet(viewsets.ViewSet): return Response(serializer.data) - @extend_schema(responses=StatusSerializer) + @extend_schema(request=DestroyOptionsSerializer, responses=StatusSerializer) def destroy(self, request, pk=None, compute_pk=None): queryset = get_instance(request.user, pk, perm_type="delete") if compute_pk is not None and str(queryset.compute_id) != str(compute_pk): @@ -132,6 +133,8 @@ class InstanceViewSet(viewsets.ViewSet): queryset = get_instance(request.user, pk, perm_type="power") if compute_pk is not None and str(queryset.compute_id) != str(compute_pk): raise Http404() + if queryset.is_template: + return Response({"status": "Templates cannot be started"}, status=status.HTTP_409_CONFLICT) return instance_action_response(poweron, request, pk, "poweron command send") @extend_schema(request=None, responses=StatusSerializer) @@ -148,6 +151,8 @@ class InstanceViewSet(viewsets.ViewSet): queryset = get_instance(request.user, pk, perm_type="power") if compute_pk is not None and str(queryset.compute_id) != str(compute_pk): raise Http404() + if queryset.is_template: + return Response({"status": "Templates cannot be started"}, status=status.HTTP_409_CONFLICT) return instance_action_response(powercycle, request, pk, "powercycle command send") @extend_schema(request=None, responses=StatusSerializer) diff --git a/instances/jobs.py b/instances/jobs.py index 565eda74..a6ac5090 100644 --- a/instances/jobs.py +++ b/instances/jobs.py @@ -5,6 +5,7 @@ import re import time from django.core.exceptions import PermissionDenied +from django.shortcuts import get_object_or_404 from django.template.loader import render_to_string from django.utils.translation import gettext_noop from django.utils.translation import gettext_noop as _ @@ -648,7 +649,7 @@ class CreateInstance(Kind): if not user.is_superuser: raise PermissionDenied data = params - c = Compute.objects.get(pk=data["compute"]) + c = get_object_or_404(Compute, pk=data["compute"]) conn = wvmCreate(c.hostname, c.login, c.password, c.type) cdrom_buses = [bus for bus in conn.get_disk_bus_types(data["arch"], data["machine"]) if bus in CDROM_BUSES] if data["name"] in conn.get_instances(): diff --git a/instances/tests/test_api_parity.py b/instances/tests/test_api_parity.py new file mode 100644 index 00000000..51cc2a81 --- /dev/null +++ b/instances/tests/test_api_parity.py @@ -0,0 +1,62 @@ +"""The VM API answers as its web pages do: a template is not started, the +host's ISO images are a superuser's, flavors and migrations take what the +web forms take.""" + +from unittest.mock import patch + +from accounts.models import UserInstance +from computes.models import Compute +from django.contrib.auth import get_user_model +from django.test import TestCase +from instances.api.serializers import InstanceDetailsSerializer, MigrateSerializer +from instances.models import Flavor, Instance + + +class ApiParityTests(TestCase): + def setUp(self): + self.user = get_user_model().objects.create_user("api-owner", password="pw") + self.admin = get_user_model().objects.create_superuser("api-admin", "api@example.com", "pw") + self.compute = Compute.objects.create(name="api-host", hostname="192.0.2.30", login="u", password="p", type=1) + self.vm = Instance.objects.create(compute=self.compute, name="api-vm", uuid="31313131-0000-0000-0000-000000000000", + is_template=True) + UserInstance.objects.create(user=self.user, instance=self.vm) + self.url = f"/api/v1/computes/{self.compute.id}/instances/{self.vm.id}/" + + def test_a_template_is_not_started(self): + self.client.force_login(self.user) + with patch("instances.models.wvmInstance") as wvm: + for action in ("poweron", "powercycle"): + with self.subTest(action=action): + self.assertEqual(self.client.post(self.url + f"{action}/").status_code, 409) + wvm.return_value.start.assert_not_called() + + def test_the_hosts_iso_images_are_a_superusers(self): + class Request: + def __init__(self, user): + self.user = user + + for user, shown in ((self.user, False), (self.admin, True)): + fields = InstanceDetailsSerializer(self.vm, context={"request": Request(user)}).fields + self.assertEqual("media_iso" in fields, shown, user) + + def test_a_flavor_is_at_least_one_of_each_and_has_its_id(self): + self.client.force_login(self.admin) + response = self.client.post("/api/v1/flavor/", {"label": "zero", "memory": 0, "vcpu": -1, "disk": 0}, + content_type="application/json") + self.assertEqual(set(response.json()), {"memory", "vcpu", "disk"}) + flavor = Flavor.objects.create(label="one", memory=1024, vcpu=1, disk=10) + self.assertEqual(self.client.get(f"/api/v1/flavor/{flavor.id}/").json()["id"], flavor.id) + + def test_a_migration_needs_neither_live_nor_offline(self): + other = Compute.objects.create(name="api-host-2", hostname="192.0.2.31", login="u", password="p", type=1) + data = {"instance": self.vm.id, "target_compute": other.id, "submission_key": "k" * 12} + serializer = MigrateSerializer(data=data) + self.assertTrue(serializer.is_valid(), serializer.errors) + self.assertEqual((serializer.validated_data["live"], serializer.validated_data["offline"]), (True, False)) + + def test_a_vm_on_an_unknown_compute_is_not_found(self): + from django.http import Http404 + from instances.jobs import CreateInstance + + with self.assertRaises(Http404): + CreateInstance().validate(self.admin, {"compute": 999999}) diff --git a/instances/tests/test_destroy.py b/instances/tests/test_destroy.py index cc90627b..4dd7948e 100644 --- a/instances/tests/test_destroy.py +++ b/instances/tests/test_destroy.py @@ -44,6 +44,26 @@ class DestroyOrderTestCase(TestCase): self.assertTrue(flags & wanted) self.assertFalse(flags & unwanted) + def test_the_api_takes_its_options_as_booleans(self): + url = f"/api/v1/computes/{self.instance.compute_id}/instances/{self.instance.id}/" + for body, content_type, deleted in ( + ({"delete_disk": True}, "application/json", True), + ("delete_disk=false", "application/x-www-form-urlencoded", False), + ): + with self.subTest(body=body): + Instance.objects.get_or_create( + compute=self.instance.compute, name="destroy-vm", uuid="11111111-2222-3333-4444-555555555555" + ) + self.instance = Instance.objects.get(uuid="11111111-2222-3333-4444-555555555555") + with patch("instances.models.wvmInstance") as wvm: + proxy = wvm.return_value + proxy.instance.isActive.return_value = False + proxy.split_disk_paths_by_use.return_value = (["/pool/own.qcow2"], []) + url = f"/api/v1/computes/{self.instance.compute_id}/instances/{self.instance.id}/" + response = self.client.delete(url, body, content_type=content_type) + self.assertEqual(response.status_code, 200) + self.assertEqual(proxy.get_volume_by_path.called, deleted) + def test_disks_are_deleted_after_the_undefine(self): proxy = self.destroy() proxy.get_volume_by_path.assert_called_once_with("/pool/own.qcow2") diff --git a/instances/views.py b/instances/views.py index a7788644..149cc93d 100755 --- a/instances/views.py +++ b/instances/views.py @@ -55,6 +55,7 @@ from vrtManager.util import randomPasswd from instances.models import Instance from . import addresses, report, utils +from .api.serializers import DestroyOptionsSerializer from .forms import ConsoleForm, FlavorForm, NewVMForm from .models import Flavor from .states import STATES, UNKNOWN, state_label @@ -764,6 +765,10 @@ def force_off(request, pk): def destroy(request, pk): if request.method in ["POST", "DELETE"]: instance = get_instance(request.user, pk, perm_type="delete") + # an API request is a DRF request whose body is in .data; refused + # options stop nothing + options = DestroyOptionsSerializer(data=getattr(request, "data", request.POST)) + options.is_valid(raise_exception=True) with libvirt_instance_lock(instance): with audit_vm(request, instance, "delete"): proxy = instance.proxy @@ -773,14 +778,14 @@ def destroy(request, pk): proxy.force_shutdown() to_delete, shared = [], [] - if request.POST.get("delete_disk", ""): + if options.validated_data["delete_disk"]: to_delete, shared = proxy.split_disk_paths_by_use() # Undefine before deleting any disk: if it fails, nothing is lost. # Snapshot metadata goes with the domain; internal snapshot data # stays in the disk images that are kept. flags = VIR_DOMAIN_UNDEFINE_MANAGED_SAVE | VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA - if request.POST.get("delete_nvram", ""): + if options.validated_data["delete_nvram"]: flags |= VIR_DOMAIN_UNDEFINE_NVRAM else: flags |= VIR_DOMAIN_UNDEFINE_KEEP_NVRAM