storage: when enabled, raise NRE when from POST v2/add_boot_partition

When the request to v2/add_boot_partition can not be processed by the
server because it is not valid, raise a FilesystemValueError exception.

Depending on configuration, it will be treated as a normal ValueError or
as a NonReportableError.

Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>

subiquity/cmd/server.py

@@ -23,6 +23,7 @@ import sys
 
 import attr
 
+from subiquity.server.controllers.filesystem import set_user_error_reportable
 from subiquitycore.log import setup_logger
 
 from .common import LOGDIR, setup_environment
@@ -137,6 +138,16 @@ def make_server_args_parser():
     )
 
     parser.add_argument("--storage-version", action="store", type=int)
+    parser.add_argument(
+        "--no-report-storage-user-errors",
+        action="store_true",
+        help="""\
+When False (the default), exceptions raised by /storage/v2/ POST handlers when
+the client sends bad information will cause the creation of a crash report and
+a HTTP 500 error to be returned.
+When True, no crash report will be created and a HTTP 422 error will be
+returned. """,
+    )
     parser.add_argument("--use-os-prober", action="store_true", default=False)
     parser.add_argument(
         "--postinst-hooks-dir", default="/etc/subiquity/postinst.d", type=pathlib.Path
@@ -158,6 +169,7 @@ def main():
         opts.storage_version = int(
             opts.kernel_cmdline.get("subiquity-storage-version", 1)
         )
+    set_user_error_reportable(not opts.no_report_storage_user_errors)
     logdir = LOGDIR
     if opts.dry_run:
         if opts.dry_run_config:

subiquity/server/controllers/filesystem.py

@@ -22,7 +22,7 @@ import os
 import pathlib
 import subprocess
 import time
-from typing import Any, Callable, Dict, List, Optional, Union
+from typing import Any, Callable, Dict, List, Optional, Type, Union
 
 import attr
 import pyudev
@@ -79,6 +79,7 @@ from subiquity.server.autoinstall import AutoinstallError
 from subiquity.server.controller import SubiquityController
 from subiquity.server.controllers.source import SEARCH_DRIVERS_AUTOINSTALL_DEFAULT
 from subiquity.server.mounter import Mounter
+from subiquity.server.nonreportable import NonReportableException
 from subiquity.server.snapdapi import (
     StorageEncryptionSupport,
     StorageSafety,
@@ -122,6 +123,23 @@ class NoSnapdSystemsOnSource(Exception):
     pass
 
 
+class NonReportableSVE(NonReportableException):
+    """Non reportable storage value error"""
+
+
+class ReportableSVE(ValueError):
+    """Reportable storage value error"""
+
+
+# Depending on config, we will let the SVE fail on the server side
+StorageValueError: Type[NonReportableSVE | ReportableSVE] = ReportableSVE
+
+
+def set_user_error_reportable(reportable: bool) -> None:
+    global StorageValueError
+    StorageValueError = ReportableSVE if reportable else NonReportableSVE
+
+
 @attr.s(auto_attribs=True)
 class CapabilityInfo:
     allowed: List[GuidedCapability] = attr.Factory(list)
@@ -1193,9 +1211,9 @@ class FilesystemController(SubiquityController, FilesystemManipulator):
         self.locked_probe_data = True
         disk = self.model._one(id=disk_id)
         if boot.is_boot_device(disk):
-            raise ValueError("device already has bootloader partition")
+            raise StorageValueError("device already has bootloader partition")
         if DeviceAction.TOGGLE_BOOT not in DeviceAction.supported(disk):
-            raise ValueError("disk does not support boot partiton")
+            raise StorageValueError("disk does not support boot partiton")
         self.add_boot_disk(disk)
         return await self.v2_GET()
 

subiquity/server/controllers/tests/test_filesystem.py

@@ -60,6 +60,7 @@ from subiquity.server.autoinstall import AutoinstallError
 from subiquity.server.controllers.filesystem import (
     DRY_RUN_RESET_SIZE,
     FilesystemController,
+    StorageValueError,
     VariationInfo,
 )
 from subiquity.server.dryrun import DRConfig
@@ -220,7 +221,9 @@ class TestSubiquityControllerFilesystem(IsolatedAsyncioTestCase):
     async def test_v2_add_boot_partition_POST_existing_bootloader(self):
         self.fsc.locked_probe_data = False
         with mock.patch.object(self.fsc, "add_boot_disk") as add_boot_disk:
-            with self.assertRaisesRegex(ValueError, r"already\ has\ bootloader"):
+            with self.assertRaises(
+                StorageValueError, msg="device already has bootloader"
+            ):
                 await self.fsc.v2_add_boot_partition_POST("dev-sda")
         self.assertTrue(self.fsc.locked_probe_data)
         add_boot_disk.assert_not_called()
@@ -230,7 +233,7 @@ class TestSubiquityControllerFilesystem(IsolatedAsyncioTestCase):
     async def test_v2_add_boot_partition_POST_not_supported(self):
         self.fsc.locked_probe_data = False
         with mock.patch.object(self.fsc, "add_boot_disk") as add_boot_disk:
-            with self.assertRaisesRegex(ValueError, r"does\ not\ support\ boot"):
+            with self.assertRaises(StorageValueError, msg="disk does not support boot"):
                 await self.fsc.v2_add_boot_partition_POST("dev-sda")
         self.assertTrue(self.fsc.locked_probe_data)
         add_boot_disk.assert_not_called()

subiquity/tests/api/test_api.py

@@ -2272,3 +2272,37 @@ class TestMountDetection(TestAPI):
             self.assertTrue(disk1["has_in_use_partition"])
             disk1p2 = disk1["partitions"][1]
             self.assertTrue(disk1p2["is_in_use"])
+
+
+class TestFilesystemUserErrors(TestAPI):
+    @timeout()
+    async def test_add_boot_partition__with_error_report(self):
+        cfg = "examples/machines/simple.json"
+        extra = ["--storage-version", "2"]
+        async with start_server(cfg, extra_args=extra) as inst:
+            await inst.post("/storage/v2/add_boot_partition", disk_id="disk-sda")
+            try:
+                await inst.post("/storage/v2/add_boot_partition", disk_id="disk-sda")
+            except ClientResponseError as cre:
+                self.assertEqual(500, cre.status)
+                self.assertIn("x-error-report", cre.headers)
+                self.assertEqual(
+                    "device already has bootloader partition",
+                    json.loads(cre.headers["x-error-msg"]),
+                )
+
+    @timeout()
+    async def test_add_boot_partition__no_error_report(self):
+        cfg = "examples/machines/simple.json"
+        extra = ["--storage-version", "2", "--no-report-storage-user-error"]
+        async with start_server(cfg, extra_args=extra) as inst:
+            await inst.post("/storage/v2/add_boot_partition", disk_id="disk-sda")
+            try:
+                await inst.post("/storage/v2/add_boot_partition", disk_id="disk-sda")
+            except ClientResponseError as cre:
+                self.assertEqual(422, cre.status)
+                self.assertNotIn("x-error-report", cre.headers)
+                self.assertEqual(
+                    "device already has bootloader partition",
+                    json.loads(cre.headers["x-error-msg"]),
+                )