From d04c01a06867e8d88fa5d37eb2ccb58f1a9d40b4 Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Fri, 26 Apr 2024 23:57:38 +0200 Subject: [PATCH] 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 --- subiquity/cmd/server.py | 12 +++++++ subiquity/server/controllers/filesystem.py | 24 +++++++++++-- .../controllers/tests/test_filesystem.py | 7 ++-- subiquity/tests/api/test_api.py | 34 +++++++++++++++++++ 4 files changed, 72 insertions(+), 5 deletions(-) diff --git a/subiquity/cmd/server.py b/subiquity/cmd/server.py index d3aaca66..104022ac 100644 --- a/subiquity/cmd/server.py +++ b/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: diff --git a/subiquity/server/controllers/filesystem.py b/subiquity/server/controllers/filesystem.py index f210267c..04fb7fe9 100644 --- a/subiquity/server/controllers/filesystem.py +++ b/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() diff --git a/subiquity/server/controllers/tests/test_filesystem.py b/subiquity/server/controllers/tests/test_filesystem.py index cc1522fc..22c4c8fe 100644 --- a/subiquity/server/controllers/tests/test_filesystem.py +++ b/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() diff --git a/subiquity/tests/api/test_api.py b/subiquity/tests/api/test_api.py index 9468e112..d5177043 100644 --- a/subiquity/tests/api/test_api.py +++ b/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"]), + )