Merge pull request #1699 from alan-turing-institute/sre_name

Improve handling of SRE names

data_safe_haven/commands/admin_register_users.py

@@ -3,7 +3,6 @@
 from data_safe_haven.config import Config, ContextSettings
 from data_safe_haven.exceptions import DataSafeHavenError
 from data_safe_haven.external import GraphApi
-from data_safe_haven.functions import alphanumeric
 from data_safe_haven.utility import LoggingSingleton
 
 
@@ -17,7 +16,7 @@ def admin_register_users(
 
     shm_name = context.shm_name
     # Use a JSON-safe SRE name
-    sre_name = alphanumeric(sre).lower()
+    sre_name = config.sanitise_sre_name(sre)
 
     try:
         # Check that SRE option has been provided

data_safe_haven/commands/admin_unregister_users.py

@@ -3,7 +3,6 @@
 from data_safe_haven.config import Config, ContextSettings
 from data_safe_haven.exceptions import DataSafeHavenError
 from data_safe_haven.external import GraphApi
-from data_safe_haven.functions import alphanumeric
 from data_safe_haven.utility import LoggingSingleton
 
 
@@ -16,8 +15,7 @@ def admin_unregister_users(
     config = Config.from_remote(context)
 
     shm_name = context.shm_name
-    # Use a JSON-safe SRE name
-    sre_name = alphanumeric(sre).lower()
+    sre_name = config.sanitise_sre_name(sre)
 
     try:
         # Check that SRE option has been provided

data_safe_haven/commands/deploy.py

@@ -6,7 +6,7 @@
 from data_safe_haven.config import Config, ContextSettings
 from data_safe_haven.exceptions import DataSafeHavenError
 from data_safe_haven.external import GraphApi
-from data_safe_haven.functions import alphanumeric, bcrypt_salt, password
+from data_safe_haven.functions import bcrypt_salt, password
 from data_safe_haven.infrastructure import SHMStackManager, SREStackManager
 from data_safe_haven.provisioning import SHMProvisioningManager, SREProvisioningManager
 
@@ -101,10 +101,9 @@ def sre(
     context = ContextSettings.from_file().assert_context()
     config = Config.from_remote(context)
 
-    try:
-        # Use a JSON-safe SRE name
-        sre_name = alphanumeric(name).lower()
+    sre_name = config.sanitise_sre_name(name)
 
+    try:
         # Load GraphAPI as this may require user-interaction that is not possible as
         # part of a Pulumi declarative command
         graph_api = GraphApi(

data_safe_haven/commands/teardown.py

@@ -9,7 +9,6 @@
     DataSafeHavenInputError,
 )
 from data_safe_haven.external import GraphApi
-from data_safe_haven.functions import alphanumeric
 from data_safe_haven.infrastructure import SHMStackManager, SREStackManager
 
 teardown_command_group = typer.Typer()
@@ -51,7 +50,7 @@ def sre(
     context = ContextSettings.from_file().assert_context()
     config = Config.from_remote(context)
 
-    sre_name = alphanumeric(name).lower()
+    sre_name = config.sanitise_sre_name(name)
     try:
         # Load GraphAPI as this may require user-interaction that is not possible as
         # part of a Pulumi declarative command
@@ -72,9 +71,8 @@ def sre(
             msg = f"Unable to teardown Pulumi infrastructure.\n{exc}"
             raise DataSafeHavenInputError(msg) from exc
 
-        # Remove information from config file
+        # Remove stack from config file
         config.remove_stack(stack.stack_name)
-        config.remove_sre(sre_name)
 
         # Upload config to blob storage
         config.upload()

data_safe_haven/config/config.py

@@ -24,6 +24,7 @@
 )
 from data_safe_haven.external import AzureApi
 from data_safe_haven.functions import (
+    alphanumeric,
     b64decode,
     b64encode,
 )
@@ -285,18 +286,17 @@ def is_complete(self, *, require_sres: bool) -> bool:
             return False
         return True
 
+    @staticmethod
+    def sanitise_sre_name(name: str) -> str:
+        return alphanumeric(name).lower()
+
     def sre(self, name: str) -> ConfigSectionSRE:
-        """Return the config entry for this SRE creating it if it does not exist"""
+        """Return the config entry for this SRE, raising an exception if it does not exist"""
         if name not in self.sres.keys():
-            highest_index = max(0 + sre.index for sre in self.sres.values())
-            self.sres[name] = ConfigSectionSRE(index=highest_index + 1)
+            msg = f"SRE {name} does not exist"
+            raise DataSafeHavenConfigError(msg)
         return self.sres[name]
 
-    def remove_sre(self, name: str) -> None:
-        """Remove SRE config section by name"""
-        if name in self.sres.keys():
-            del self.sres[name]
-
     def add_stack(self, name: str, path: Path) -> None:
         """Add a Pulumi stack file to config"""
         if self.pulumi:

tests_/config/test_config.py

@@ -11,7 +11,10 @@
     ConfigSectionTags,
     ConfigSubsectionRemoteDesktopOpts,
 )
-from data_safe_haven.exceptions import DataSafeHavenParameterError
+from data_safe_haven.exceptions import (
+    DataSafeHavenConfigError,
+    DataSafeHavenParameterError,
+)
 from data_safe_haven.external import AzureApi
 from data_safe_haven.utility.enums import DatabaseSystem, SoftwarePackageCategory
 from data_safe_haven.version import __version__
@@ -258,26 +261,23 @@ def test_is_complete_no_sres(self, config_no_sres, require_sres, expected):
     def test_is_complete_sres(self, config_sres, require_sres):
         assert config_sres.is_complete(require_sres=require_sres)
 
+    @pytest.mark.parametrize(
+        "value,expected",
+        [("Test SRE", "testsre"), ("%*aBc", "abc"), ("MY_SRE", "mysre")],
+    )
+    def test_sanitise_sre_name(self, value, expected):
+        assert Config.sanitise_sre_name(value) == expected
+
     def test_sre(self, config_sres):
         sre1, sre2 = config_sres.sre("sre1"), config_sres.sre("sre2")
         assert sre1.index == 0
         assert sre2.index == 1
         assert sre1 != sre2
 
-    def test_sre_create(self, config_sres):
-        sre1 = config_sres.sre("sre1")
-        sre3 = config_sres.sre("sre3")
-        assert isinstance(sre3, ConfigSectionSRE)
-        assert sre3.index == 2
-        assert sre3 != sre1
-        assert len(config_sres.sres) == 3
-
-    def test_remove_sre(self, config_sres):
-        assert len(config_sres.sres) == 2
-        config_sres.remove_sre("sre1")
-        assert len(config_sres.sres) == 1
-        assert "sre2" in config_sres.sres.keys()
-        assert "sre1" not in config_sres.sres.keys()
+    def test_sre_invalid(self, config_sres):
+        with pytest.raises(DataSafeHavenConfigError) as exc:
+            config_sres.sre("sre3")
+            assert "SRE sre3 does not exist" in exc
 
     def test_template(self, context):
         config = Config.template(context)