Merge pull request #2189 from alan-turing-institute/show_invalid_config

Show invalid config

data_safe_haven/commands/config.py

@@ -1,21 +1,31 @@
 """Command group and entrypoints for managing DSH configuration"""
 
+from logging import Logger
 from pathlib import Path
 from typing import Annotated, Optional
 
 import typer
 
 from data_safe_haven import console
-from data_safe_haven.config import ContextManager, DSHPulumiConfig, SHMConfig, SREConfig
+from data_safe_haven.config import (
+    ContextManager,
+    DSHPulumiConfig,
+    SHMConfig,
+    SREConfig,
+    sre_config_name,
+)
 from data_safe_haven.exceptions import (
+    DataSafeHavenAzureError,
     DataSafeHavenAzureStorageError,
     DataSafeHavenConfigError,
     DataSafeHavenError,
     DataSafeHavenPulumiError,
+    DataSafeHavenTypeError,
 )
 from data_safe_haven.external.api.azure_sdk import AzureSdk
 from data_safe_haven.infrastructure import SREProjectManager
 from data_safe_haven.logging import get_logger
+from data_safe_haven.serialisers import ContextBase
 
 config_command_group = typer.Typer()
 
@@ -38,14 +48,17 @@ def show_shm(
             "or `dsh context switch` to select one."
         )
         raise typer.Exit(1) from exc
+
     try:
         config = SHMConfig.from_remote(context)
     except DataSafeHavenError as exc:
         logger.critical(
             "SHM must be deployed before its configuration can be displayed."
         )
         raise typer.Exit(1) from exc
+
     config_yaml = config.to_yaml()
+
     if file:
         with open(file, "w") as outfile:
             outfile.write(config_yaml)
@@ -66,7 +79,9 @@ def available() -> None:
             "or `dsh context switch` to select one."
         )
         raise typer.Exit(1) from exc
+
     azure_sdk = AzureSdk(context.subscription_name)
+
     try:
         blobs = azure_sdk.list_blobs(
             container_name=context.storage_container_name,
@@ -77,9 +92,11 @@ def available() -> None:
     except DataSafeHavenAzureStorageError as exc:
         logger.critical("Ensure SHM is deployed before attempting to use SRE configs.")
         raise typer.Exit(1) from exc
+
     if not blobs:
         logger.info(f"No configurations found for context '{context.name}'.")
         raise typer.Exit(0)
+
     pulumi_config = DSHPulumiConfig.from_remote(context)
     sre_status = {}
     for blob in blobs:
@@ -92,6 +109,7 @@ def available() -> None:
             pulumi_config=pulumi_config,
             create_project=True,
         )
+
         try:
             sre_status[sre_config.name] = (
                 "No output values" not in stack.run_pulumi_command("stack output")
@@ -101,6 +119,7 @@ def available() -> None:
                 f"Failed to run Pulumi command querying stack outputs for SRE '{sre_config.name}'."
             )
             raise typer.Exit(1) from exc
+
     headers = ["SRE Name", "Deployed"]
     rows = [[name, "x" if deployed else ""] for name, deployed in sre_status.items()]
     console.print(f"Available SRE configurations for context '{context.name}':")
@@ -117,6 +136,7 @@ def show(
 ) -> None:
     """Print the SRE configuration for the selected SRE and Data Safe Haven context"""
     logger = get_logger()
+
     try:
         context = ContextManager.from_file().assert_context()
     except DataSafeHavenConfigError as exc:
@@ -125,17 +145,23 @@ def show(
             "or `dsh context switch` to select one."
         )
         raise typer.Exit(1) from exc
+
     try:
         sre_config = SREConfig.from_remote_by_name(context, name)
     except DataSafeHavenAzureStorageError as exc:
         logger.critical("Ensure SHM is deployed before attempting to use SRE configs.")
         raise typer.Exit(1) from exc
-    except DataSafeHavenError as exc:
+    except DataSafeHavenAzureError as exc:
         logger.critical(
             f"No configuration exists for an SRE named '{name}' for the selected context."
         )
         raise typer.Exit(1) from exc
+    except DataSafeHavenTypeError as exc:
+        dump_remote_config(context, name, logger)
+        raise typer.Exit(1) from exc
+
     config_yaml = sre_config.to_yaml()
+
     if file:
         with open(file, "w") as outfile:
             outfile.write(config_yaml)
@@ -160,6 +186,7 @@ def template(
     # Serialisation warnings are therefore suppressed to avoid misleading the users into
     # thinking there is a problem and contaminating the output.
     config_yaml = sre_config.to_yaml(warnings=False)
+
     if file:
         with open(file, "w") as outfile:
             outfile.write(config_yaml)
@@ -169,7 +196,13 @@ def template(
 
 @config_command_group.command()
 def upload(
-    file: Annotated[Path, typer.Argument(help="Path to configuration file")],
+    file: Annotated[Path, typer.Argument(help="Path to configuration file.")],
+    force: Annotated[  # noqa: FBT002
+        bool,
+        typer.Option(
+            help="Skip validation and difference calculation of remote configuration."
+        ),
+    ] = False,
 ) -> None:
     """Upload an SRE configuration to the Data Safe Haven context"""
     context = ContextManager.from_file().assert_context()
@@ -185,24 +218,45 @@ def upload(
     config = SREConfig.from_yaml(config_yaml)
 
     # Present diff to user
-    if SREConfig.remote_exists(context, filename=config.filename):
-        if diff := config.remote_yaml_diff(context, filename=config.filename):
-            for line in "".join(diff).splitlines():
-                logger.info(line)
-            if not console.confirm(
-                (
-                    "Configuration has changed, "
-                    "do you want to overwrite the remote configuration?"
-                ),
-                default_to_yes=False,
-            ):
+    if (not force) and SREConfig.remote_exists(context, filename=config.filename):
+        try:
+            if diff := config.remote_yaml_diff(context, filename=config.filename):
+                for line in "".join(diff).splitlines():
+                    logger.info(line)
+                if not console.confirm(
+                    (
+                        "Configuration has changed, "
+                        "do you want to overwrite the remote configuration?"
+                    ),
+                    default_to_yes=False,
+                ):
+                    raise typer.Exit()
+            else:
+                console.print("No changes, won't upload configuration.")
                 raise typer.Exit()
-        else:
-            console.print("No changes, won't upload configuration.")
-            raise typer.Exit()
+        except DataSafeHavenTypeError as exc:
+            dump_remote_config(context, config.name, logger)
+            console.print(
+                "To overwrite the remote config, use `dsh config upload --force`"
+            )
+            raise typer.Exit(1) from exc
 
     try:
         config.upload(context, filename=config.filename)
     except DataSafeHavenError as exc:
         logger.critical("No infrastructure found for the selected context.")
         raise typer.Exit(1) from exc
+
+
+def dump_remote_config(context: ContextBase, name: str, logger: Logger) -> None:
+    logger.warning(
+        f"Remote configuration for SRE '{name}' is not valid. Dumping remote file."
+    )
+    azure_sdk = AzureSdk(subscription_name=context.subscription_name)
+    config_yaml = azure_sdk.download_blob(
+        sre_config_name(name),
+        context.resource_group_name,
+        context.storage_account_name,
+        context.storage_container_name,
+    )
+    console.print(config_yaml)

data_safe_haven/config/__init__.py

@@ -3,7 +3,7 @@
 from .dsh_pulumi_config import DSHPulumiConfig
 from .dsh_pulumi_project import DSHPulumiProject
 from .shm_config import SHMConfig
-from .sre_config import SREConfig
+from .sre_config import SREConfig, sre_config_name
 
 __all__ = [
     "Context",
@@ -12,4 +12,5 @@
     "DSHPulumiProject",
     "SHMConfig",
     "SREConfig",
+    "sre_config_name",
 ]

data_safe_haven/serialisers/azure_serialisable_model.py

@@ -5,7 +5,7 @@
 from data_safe_haven.exceptions import (
     DataSafeHavenAzureError,
     DataSafeHavenAzureStorageError,
-    DataSafeHavenError,
+    DataSafeHavenTypeError,
 )
 from data_safe_haven.external import AzureSdk
 
@@ -44,9 +44,12 @@ def from_remote(
         except DataSafeHavenAzureStorageError as exc:
             msg = f"Storage account '{context.storage_account_name}' does not exist."
             raise DataSafeHavenAzureStorageError(msg) from exc
-        except DataSafeHavenError as exc:
+        except DataSafeHavenAzureError as exc:
             msg = f"Could not load file '{filename or cls.default_filename}' from Azure storage."
             raise DataSafeHavenAzureError(msg) from exc
+        except DataSafeHavenTypeError as exc:
+            msg = f"'{filename or cls.default_filename}' does not contain a valid {cls.config_type} configuration."
+            raise DataSafeHavenTypeError(msg) from exc
 
     @classmethod
     def from_remote_or_create(

data_safe_haven/serialisers/yaml_serialisable_model.py

@@ -56,7 +56,7 @@ def from_yaml(cls: type[T], settings_yaml: str) -> T:
                 logger.error(
                     f"[red]{'.'.join(map(str, error.get('loc', [])))}: {error.get('input', '')}[/] - {error.get('msg', '')}"
                 )
-            msg = f"Could not load {cls.config_type} configuration."
+            msg = f"{cls.config_type} configuration is invalid."
             raise DataSafeHavenTypeError(msg) from exc
 
     def to_filepath(self, config_file_path: PathType) -> None:

tests/commands/test_config_sre.py

@@ -4,9 +4,10 @@
 from data_safe_haven.config import ContextManager, SREConfig
 from data_safe_haven.config.sre_config import sre_config_name
 from data_safe_haven.exceptions import (
+    DataSafeHavenAzureError,
     DataSafeHavenAzureStorageError,
     DataSafeHavenConfigError,
-    DataSafeHavenError,
+    DataSafeHavenTypeError,
 )
 from data_safe_haven.external import AzureSdk
 
@@ -41,6 +42,25 @@ def test_show_file(self, mocker, runner, sre_config_yaml, tmp_path):
             template_text = f.read()
         assert sre_config_yaml in template_text
 
+    def test_show_invalid_config(self, mocker, runner, context, sre_config_yaml):
+        mocker.patch.object(
+            SREConfig, "from_remote_by_name", side_effect=DataSafeHavenTypeError(" ")
+        )
+        mock_method = mocker.patch.object(
+            AzureSdk, "download_blob", return_value=sre_config_yaml
+        )
+        sre_name = "sandbox"
+        result = runner.invoke(config_command_group, ["show", sre_name])
+
+        assert result.exit_code == 1
+        assert sre_config_yaml in result.stdout
+        mock_method.assert_called_once_with(
+            sre_config_name(sre_name),
+            context.resource_group_name,
+            context.storage_account_name,
+            context.storage_container_name,
+        )
+
     def test_no_context(self, mocker, runner):
         sre_name = "sandbox"
         mocker.patch.object(
@@ -73,7 +93,7 @@ def test_no_storage_account(self, mocker, runner):
     def test_incorrect_sre_name(self, mocker, runner):
         sre_name = "sandbox"
         mocker.patch.object(
-            SREConfig, "from_remote_by_name", side_effect=DataSafeHavenError(" ")
+            SREConfig, "from_remote_by_name", side_effect=DataSafeHavenAzureError(" ")
         )
         result = runner.invoke(config_command_group, ["show", sre_name])
         assert "No configuration exists for an SRE" in result.stdout
@@ -261,5 +281,53 @@ def test_upload_no_file(self, mocker, runner):
     def test_upload_file_does_not_exist(self, mocker, runner):
         mocker.patch.object(Path, "is_file", return_value=False)
         result = runner.invoke(config_command_group, ["upload", "fake_config.yaml"])
+        assert result.exit_code == 1
         assert "Configuration file 'fake_config.yaml' not found." in result.stdout
+
+    def test_upload_invalid_config(
+        self, mocker, runner, context, sre_config_file, sre_config_yaml
+    ):
+        sre_name = "SandBox"
+        sre_filename = sre_config_name(sre_name)
+
+        mock_exists = mocker.patch.object(SREConfig, "remote_exists", return_value=True)
+        mocker.patch.object(
+            SREConfig, "remote_yaml_diff", side_effect=DataSafeHavenTypeError(" ")
+        )
+        mocker.patch.object(AzureSdk, "download_blob", return_value=sre_config_yaml)
+
+        result = runner.invoke(config_command_group, ["upload", str(sre_config_file)])
+
         assert result.exit_code == 1
+
+        mock_exists.assert_called_once_with(context, filename=sre_filename)
+        assert sre_config_yaml in result.stdout
+        assert (
+            "To overwrite the remote config, use `dsh config upload --force`"
+            in result.stdout
+        )
+
+    def test_upload_invalid_config_force(
+        self, mocker, runner, context, sre_config_file, sre_config_yaml
+    ):
+        sre_name = "SandBox"
+        sre_filename = sre_config_name(sre_name)
+
+        mocker.patch.object(
+            SREConfig, "remote_yaml_diff", side_effect=DataSafeHavenTypeError(" ")
+        )
+        mock_upload = mocker.patch.object(AzureSdk, "upload_blob", return_value=None)
+
+        result = runner.invoke(
+            config_command_group, ["upload", "--force", str(sre_config_file)]
+        )
+
+        assert result.exit_code == 0
+
+        mock_upload.assert_called_once_with(
+            sre_config_yaml,
+            sre_filename,
+            context.resource_group_name,
+            context.storage_account_name,
+            context.storage_container_name,
+        )

tests/config/test_context_manager.py

@@ -114,7 +114,7 @@ def test_missing_selected(self, context_yaml):
         )
         with pytest.raises(
             DataSafeHavenTypeError,
-            match="Could not load ContextManager configuration.",
+            match="ContextManager configuration is invalid.",
         ):
             ContextManager.from_yaml(context_yaml)
 
@@ -124,7 +124,7 @@ def test_invalid_selected_input(self, context_yaml):
         )
         with pytest.raises(
             DataSafeHavenTypeError,
-            match="Could not load ContextManager configuration.",
+            match="ContextManager configuration is invalid.",
         ):
             ContextManager.from_yaml(context_yaml)
 

tests/config/test_pulumi.py

@@ -114,9 +114,7 @@ def test_from_yaml_not_dict(self):
 
     def test_from_yaml_validation_error(self):
         not_valid = "projects: -3"
-        with raises(
-            DataSafeHavenTypeError, match="Could not load Pulumi configuration."
-        ):
+        with raises(DataSafeHavenTypeError, match="Pulumi configuration is invalid."):
             DSHPulumiConfig.from_yaml(not_valid)
 
     def test_upload(self, mocker, pulumi_config, context):