🐛 Retrieving service key/version from running service to use regex instead of split (ITISFoundation#2738)

* use regex instead of split

Author: sanderegg

services/director/src/simcore_service_director/producer.py

@@ -638,6 +638,12 @@ async def _get_dependant_repos(
     return dependent_repositories
 
 
+_TAG_REGEX = re.compile(r"^\d+\.\d+\.\d+$")
+_SERVICE_KEY_REGEX = re.compile(
+    r"^(simcore/services/(comp|dynamic|frontend)(/[\w/-]+)+):(\d+\.\d+\.\d+).*$"
+)
+
+
 async def _find_service_tag(
     list_of_images: Dict, service_key: str, service_tag: str
 ) -> str:
@@ -646,8 +652,7 @@ async def _find_service_tag(
             service_name=service_key, service_tag=service_tag
         )
     # filter incorrect chars
-    regex = re.compile(r"^\d+\.\d+\.\d+$")
-    filtered_tags_list = filter(regex.search, list_of_images[service_key])
+    filtered_tags_list = filter(_TAG_REGEX.search, list_of_images[service_key])
     # sort them now
     available_tags_list = sorted(filtered_tags_list, key=StrictVersion)
     # not tags available... probably an undefined service there...
@@ -813,11 +818,18 @@ async def _get_service_key_version_from_docker_service(
     service_full_name = str(service["Spec"]["TaskTemplate"]["ContainerSpec"]["Image"])
     if not service_full_name.startswith(config.REGISTRY_PATH):
         raise exceptions.DirectorException(
-            msg="Invalid service {}".format(service_full_name)
+            msg=f"Invalid service '{service_full_name}', it is missing {config.REGISTRY_PATH}"
         )
 
     service_full_name = service_full_name[len(config.REGISTRY_PATH) :].strip("/")
-    return service_full_name.split(":")[0], service_full_name.split(":")[1]
+    service_re_match = _SERVICE_KEY_REGEX.match(service_full_name)
+    if not service_re_match:
+        raise exceptions.DirectorException(
+            msg=f"Invalid service '{service_full_name}', it does not follow pattern '{_SERVICE_KEY_REGEX.pattern}'"
+        )
+    service_key = service_re_match.group(1)
+    service_tag = service_re_match.group(4)
+    return service_key, service_tag
 
 
 async def _get_service_basepath_from_docker_service(service: Dict) -> str:

services/director/tests/test_producer.py

@@ -7,6 +7,7 @@
 import asyncio
 import time
 import uuid
+from dataclasses import dataclass
 
 import docker
 import pytest
@@ -77,7 +78,7 @@ async def push_start_services(number_comp, number_dyn, dependant=False):
             start_time = time.perf_counter()
             max_time = 2 * 60
             while node_details["service_state"] != "running":
-                asyncio.sleep(2)
+                await asyncio.sleep(2)
                 if (time.perf_counter() - start_time) > max_time:
                     assert True, "waiting too long to start service"
                 node_details = await producer.get_service_details(
@@ -254,3 +255,76 @@ async def test_dependent_services_have_common_network(run_services):
             list_of_services[0].attrs["Spec"]["Networks"][0]["Target"]
             == list_of_services[1].attrs["Spec"]["Networks"][0]["Target"]
         )
+
+
+@dataclass
+class FakeDockerService:
+    service_str: str
+    expected_key: str
+    expected_tag: str
+
+
+@pytest.mark.parametrize(
+    "fake_service",
+    [
+        FakeDockerService(
+            "/simcore/services/dynamic/some/sub/folder/my_service-key:123.456.3214",
+            "simcore/services/dynamic/some/sub/folder/my_service-key",
+            "123.456.3214",
+        ),
+        FakeDockerService(
+            "/simcore/services/dynamic/some/sub/folder/my_service-key:123.456.3214@sha256:2aef165ab4f30fbb109e88959271d8b57489790ea13a77d27c02d8adb8feb20f",
+            "simcore/services/dynamic/some/sub/folder/my_service-key",
+            "123.456.3214",
+        ),
+    ],
+)
+async def test_get_service_key_version_from_docker_service(
+    fake_service: FakeDockerService,
+):
+    docker_service_partial_inspect = {
+        "Spec": {
+            "TaskTemplate": {
+                "ContainerSpec": {
+                    "Image": f"{config.REGISTRY_PATH}{fake_service.service_str}"
+                }
+            }
+        }
+    }
+    (
+        service_key,
+        service_tag,
+    ) = await producer._get_service_key_version_from_docker_service(
+        docker_service_partial_inspect
+    )
+    assert service_key == fake_service.expected_key
+    assert service_tag == fake_service.expected_tag
+
+
+@pytest.mark.parametrize(
+    "fake_service_str",
+    [
+        "postgres:10.11@sha256:2aef165ab4f30fbb109e88959271d8b57489790ea13a77d27c02d8adb8feb20f",
+        "/simcore/postgres:10.11@sha256:2aef165ab4f30fbb109e88959271d8b57489790ea13a77d27c02d8adb8feb20f",
+        "itisfoundation/postgres:10.11@sha256:2aef165ab4f30fbb109e88959271d8b57489790ea13a77d27c02d8adb8feb20f",
+        "/simcore/services/stuff/postgres:10.11",
+        "/simcore/services/dynamic/postgres:10.11",
+        "/simcore/services/dynamic/postgres:10",
+    ],
+)
+async def test_get_service_key_version_from_docker_service_except_invalid_keys(
+    fake_service_str: str,
+):
+    docker_service_partial_inspect = {
+        "Spec": {
+            "TaskTemplate": {
+                "ContainerSpec": {
+                    "Image": f"{config.REGISTRY_PATH if fake_service_str.startswith('/') else ''}{fake_service_str}"
+                }
+            }
+        }
+    }
+    with pytest.raises(exceptions.DirectorException):
+        await producer._get_service_key_version_from_docker_service(
+            docker_service_partial_inspect
+        )