DUPLO-38290 fix: add comprehensive property mapping for ECS task definition (#191)

* DUPLO-38290 fix: comprehensive mapping for task definition and container definition models based on AWS models

* DUPLO-38290 chore: rename volume sanitizer helper

* DUPLO-38290 chore: clean up unused snippet

* DUPLO-38290 chore: update changelog

* DUPLO-38290 test: add dedicated test for internal taskdef mapping helper

* DUPLO-38290 chore: refine changes

* DUPLO-38290 test: more coverage for mapping logic

* DUPLO-38345 fix: decouple find_def method from ECS services

* DUPLO-38345 chore: update docs to better reflect usage intent for run_task command

* DUPLO-38345 revert: DUPLO-35823

* DUPLO-38345 chore: update changelog

---------

Co-authored-by: Kelly Ferrone <[email protected]>

Author: Matheusbafutto

CHANGELOG.md

@@ -10,6 +10,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 
 - updating image for an ecs task definition ignored existing defined volumes when mapping to new payload
+- updating image for an ECS service failed to comprehensively map existing payload properties to new task definition version
+- reverts DUPLO-35823 and adds clarifications to duploctl ecs commands making intended usage clearer
 
 ### Added
 

src/duplo_resource/ecs_service.py

@@ -94,22 +94,20 @@ def list_task_def_family(self) -> dict:
   @Command()
   def find_def(self,
                name: args.NAME):
-    """Find a ECS task definition by name.
+    """Find the latest version of an ECS task definition by family name.
 
     Args:
-      name: The name of the ECS task definition to find.
+      name: The family name of the ECS task definition to find.
 
     Returns:
-      task_definition: The ECS task definition object.
+      task_definition: The latest version of that ECS task definition in the family.
 
     Raises:
       DuploError: If the ECS task definition could not be found.
     """
     name = self.prefixed_name(name)
-    svcFam = self.find_service_family(name)
-    family = svcFam["TaskDefFamily"]
-    tdf = self.find_task_def_family(family)
-    arn = tdf["VersionArns"][-1]
+    task_definition_family = self.find_task_def_family(name)
+    arn = task_definition_family["VersionArns"][-1]
     return self.find_def_by_arn(arn)
 
   @Command()
@@ -199,14 +197,16 @@ def update_image(self,
                    container_image: args.CONTAINER_IMAGE = None) -> dict:
     """Update Image
     
-    Update the image for an ECS task definition and service container if one exists.
+    Creates a new task definition version cloning the latest existing version in the family except for image arguments
+
+    If task family is used by an ECS service, method also updates the service to use that newly created definition version
 
     Usage: Basic CLI Use
       ```sh
-        duploctl ecs update_image <service-name> <service-image>
+        duploctl ecs update_image <task-definition-family-name> <new-image>
       ```
       ```sh
-        duploctl ecs update_image <service-name> --container-image <container-name> <container-image>
+        duploctl ecs update_image <task-definition-family-name> --container-image <container-name> <new-container-image>
       ```
 
     Example: Update image and wait
@@ -217,15 +217,15 @@ def update_image(self,
       ```
 
     Args:
-      name: The name of the ECS service to update.
+      name: The name of the ECS task definition family to update.
       image: The new image to use for the container.
       container-image: A list of key-value pairs to set as container image.
 
     Returns:
       dict: A dictionary containing a message about the update status.
 
     Raises:
-      DuploError: If the ECS service could not be updated.
+      DuploError: If the ECS task definition family could not be updated.
     """
     name = self.prefixed_name(name)
     tdf = self.find_def(name)
@@ -255,12 +255,22 @@ def update_image(self,
     }
 
   def __ecs_task_def_body(self, task_def):
-    containers = [
-      self.__ecs_container_update_body(c)
-      for c in task_def.get("ContainerDefinitions", [])
-    ]
+    def sanitize_container_definition(containerDefinition):
+        if containerDefinition.get("Cpu") == 0:
+          del containerDefinition["Cpu"]
+        if containerDefinition.get("Memory") == 0:
+          del containerDefinition["Memory"]
+        if containerDefinition.get("MemoryReservation") == 0:
+          del containerDefinition["MemoryReservation"]
+        if containerDefinition.get("StartTimeout") == 0:
+          del containerDefinition["StartTimeout"]
+        if containerDefinition.get("StopTimeout") == 0:
+          del containerDefinition["StopTimeout"]
+        return containerDefinition
+    
+    containers = list(map(sanitize_container_definition, task_def.get("ContainerDefinitions", [])))
 
-    def sanitize_efs_port_if_needed(v):
+    def sanitize_volume(v):
       if "EfsVolumeConfiguration" in v and "TransitEncryptionPort" in v["EfsVolumeConfiguration"] and v["EfsVolumeConfiguration"]["TransitEncryptionPort"] == 0:
         del v["EfsVolumeConfiguration"]["TransitEncryptionPort"]
       return v
@@ -272,44 +282,28 @@ def sanitize_efs_port_if_needed(v):
       "ContainerDefinitions": containers,
       "RuntimePlatform": task_def.get("RuntimePlatform", {}),
       "RequiresCompatibilities": task_def.get("RequiresCompatibilities", []),
-      "Volumes": list(map(sanitize_efs_port_if_needed, task_def.get("Volumes", []))),
+      "Volumes": list(map(sanitize_volume, task_def.get("Volumes", []))),
     }
     if task_def.get("Cpu") not in (None, 0):
       result["Cpu"] = task_def["Cpu"]
     if task_def.get("Memory") not in (None, 0):
       result["Memory"] = task_def["Memory"]
     if task_def.get("MemoryReservation") not in (None, 0):
       result["MemoryReservation"] = task_def["MemoryReservation"]
-    return result
-
+    if "EphemeralStorage" in task_def:
+      result["EphemeralStorage"] = task_def["EphemeralStorage"]
+    if "ExecutionRoleArn" in task_def:
+      result["ExecutionRoleArn"] = task_def["ExecutionRoleArn"]
+    if "IpcMode" in task_def:
+      result["IpcMode"] = task_def["IpcMode"]
+    if "PlacementConstraints" in task_def:
+      result["PlacementConstraints"] = task_def["PlacementConstraints"]
+    if "ProxyConfiguration" in task_def:
+      result["ProxyConfiguration"] = task_def["ProxyConfiguration"]
+    if "TaskRoleArn" in task_def:
+      result["TaskRoleArn"] = task_def["TaskRoleArn"]
 
-  def __ecs_container_update_body(self, container_def):
-    update_body = {
-        "Essential": container_def.get("Essential"),
-        "Image": container_def.get("Image") ,
-        "Name": container_def.get("Name") ,
-        "PortMappings": container_def.get("PortMappings", []) ,
-        "Environment": container_def.get("Environment", {}) ,
-        "Command": container_def.get("Command", {}) ,
-        "Secrets": container_def.get("Secrets", {}) ,
-        "EnvironmentFiles": container_def.get("EnvironmentFiles", {})
-    }
-
-    # Add LogConfiguration only if it exists in container_def
-    if "LogConfiguration" in container_def:
-        update_body["LogConfiguration"] = container_def["LogConfiguration"]
-    # Add FirelensConfiguration only if it exists in container_def
-    if "FirelensConfiguration" in container_def:
-        update_body["FirelensConfiguration"] = container_def["FirelensConfiguration"]
-    if container_def.get("Cpu") not in (None, 0):
-        update_body["Cpu"] = container_def["Cpu"]
-    if container_def.get("Memory") not in (None, 0):
-        update_body["Memory"] = container_def["Memory"]
-    if container_def.get("MemoryReservation") not in (None, 0):
-        update_body["MemoryReservation"] = container_def["MemoryReservation"]
-    if container_def.get("RepositoryCredentials") not in (None, 0):
-        update_body["RepositoryCredentials"] = container_def["RepositoryCredentials"]
-    return update_body
+    return result
 
   @Command()
   def list_tasks(self,
@@ -338,13 +332,13 @@ def list_tasks(self,
   def run_task(self,
                name: args.NAME,
                replicas: args.REPLICAS) -> dict:
-    """Run a task for an ECS service."
+    """Run a task from an ECS task definition family's latest definition version."
 
     Execute a task based on some definition.
 
     Usage: Basic CLI Use
       ```sh
-      duploctl ecs run_task <service-name> <replicas>
+      duploctl ecs run_task <task-definition-family-name> <replicas>
       ```
 
     Example: Wait for task to complete
@@ -355,7 +349,7 @@ def run_task(self,
       ```
 
     Args:
-      name: The name of the ECS service to run the task for.
+      name: The name of the ECS task definition family the task will be spawned from.
       replicas: The number of replicas to run.
 
     Returns:

src/tests/test_ecs_service.py

@@ -1,6 +1,7 @@
 import pytest
 from unittest.mock import ANY, MagicMock
 from duplo_resource.ecs_service import DuploEcsService
+from duplocloud.client import DuploClient
 from duplocloud.errors import DuploError
 from tests.test_utils import execute_test, assert_response
 
@@ -46,20 +47,7 @@ def test_update_image_without_container(mocker):
     # Mock task definition with single container
     mock_task_def = {
         "ContainerDefinitions": [
-            {"Name": "app", "Image": "old-image:1"}
-        ],
-        "Volumes": [
-            {
-                "ConfiguredAtLaunch": False,
-                "EfsVolumeConfiguration": {
-                    "FileSystemId": "<my-efs-id>",
-                    "RootDirectory": "/",
-                    "TransitEncryption": {
-                        "Value": "ENABLED"
-                    }
-                },
-                "Name": "my-volume"
-            }
+            { "Name": "app", "Image": "old-image:1" }
         ]
     }
     # Mock service family
@@ -108,26 +96,124 @@ def test_update_image_no_service(mocker):
     assert_response(result, "No Service Configured, only the definition is updated.")
 
 @pytest.mark.unit
-def test_find_def_uses_taskdef_family_from_service(mocker):
+def test_taskdef_mapping_comprehensive_enough(mocker):
+    mock_client = mocker.MagicMock()
+    service = DuploEcsService(mock_client)
+
+    existing_taskdef = {
+        "Family": "my-family",
+        "ContainerDefinitions": [
+            {
+                "Name": "app",
+                "Image": "my-image:1",
+                "CredentialSpecs" : [],
+                "Environment" : [
+                    {
+                        "Name" : "ENV_VAR1",
+                        "Value" : "VALUE1"
+                    },
+                    {
+                        "Name" : "ENV_VAR2",
+                        "Value" : "VALUE2"
+                    },
+                ],
+                "EnvironmentFiles" : [
+                    {
+                    "Type" : {
+                        "Value" : "s3"
+                    },
+                    "Value" : "arn:aws:s3:::my-example-bucket/sample.env"
+                    }
+                ],
+                "LogConfiguration" : {
+                    "LogDriver" : {
+                        "Value" : "dummy-driver"
+                    },
+                    "Options" : {
+                        "OptionKey1": "OptionValue1",
+                        "OptionKey2": "OptionValue2"
+                    },
+                    "SecretOptions" : []
+                },
+                "stopTimeout" : 120,
+                "MountPoints" : [
+                    {
+                        "ContainerPath" : "/mnt/my-volume",
+                        "ReadOnly" : "false",
+                        "SourceVolume" : "my-volume"
+                    }
+                ]
+            }
+        ],
+        "Volumes": [
+            {
+                "ConfiguredAtLaunch": False,
+                "EfsVolumeConfiguration": {
+                    "FileSystemId": "<my-efs-id>",
+                    "RootDirectory": "/",
+                    "TransitEncryption": {
+                        "Value": "ENABLED"
+                    }
+                },
+                "Name": "my-volume"
+            }
+        ]
+    }
+
+    # Even though this is a private method and should not be part of a behavior driven test suite
+    # Issues with this method has caused enough problems to justify this extra coverage
+    result = execute_test(service._DuploEcsService__ecs_task_def_body, existing_taskdef)
+
+    assert result["Volumes"] == existing_taskdef["Volumes"]
+    assert result["Family"] == existing_taskdef["Family"]
+    assert result["ContainerDefinitions"] == existing_taskdef["ContainerDefinitions"]
+
+@pytest.mark.unit
+def test_taskdef_mapping_properly_sanitizes_properties(mocker):
     mock_client = mocker.MagicMock()
     service = DuploEcsService(mock_client)
+    existing_taskdef = {
+        "Family": "my-family",
+        "ContainerDefinitions": [
+            {
+                "Name": "app",
+                "Image": "my-image:1",
+                "Cpu": 0,
+                "Memory": 0,
+                "MemoryReservation": 0,
+                "StartTimeout": 0,
+                "StopTimeout": 0,
+                "MountPoints" : [
+                    {
+                        "ContainerPath" : "/mnt/my-volume",
+                        "ReadOnly" : "false",
+                        "SourceVolume" : "my-volume"
+                    }
+                ]
+            }
+        ],
+        "Volumes": [
+            {
+                "ConfiguredAtLaunch": False,
+                "EfsVolumeConfiguration": {
+                    "FileSystemId": "<my-efs-id>",
+                    "RootDirectory": "/",
+                    "TransitEncryption": {
+                        "Value": "ENABLED"
+                    },
+                    "TransitEncryptionPort": 0
+                },
+                "Name": "my-volume"
+            }
+        ]
+    }
 
-    # Arrange
-    mocker.patch.object(service, 'prefixed_name', return_value='prefixed-svc')
-    mocker.patch.object(service, 'find_service_family', return_value={
-        'TaskDefFamily': 'actual-family'
-    })
-    mock_find_tdf = mocker.patch.object(service, 'find_task_def_family', return_value={
-        'VersionArns': ['arn:tdf:1', 'arn:tdf:2']
-    })
-    mock_find_by_arn = mocker.patch.object(service, 'find_def_by_arn', return_value={
-        'TaskDefinitionArn': 'arn:tdf:2'
-    })
+    result = execute_test(service._DuploEcsService__ecs_task_def_body, existing_taskdef)
 
-    # Act
-    result = execute_test(service.find_def, 'svc-name')
+    assert "Cpu" not in result["ContainerDefinitions"][0]
+    assert "Memory" not in result["ContainerDefinitions"][0]
+    assert "MemoryReservation" not in result["ContainerDefinitions"][0]
+    assert "StartTimeout" not in result["ContainerDefinitions"][0]
+    assert "StopTimeout" not in result["ContainerDefinitions"][0]
 
-    # Assert
-    mock_find_tdf.assert_called_once_with('actual-family')
-    mock_find_by_arn.assert_called_once_with('arn:tdf:2')
-    assert result == {'TaskDefinitionArn': 'arn:tdf:2'}
+    assert "TransitEncryptionPort" not in result["Volumes"][0]["EfsVolumeConfiguration"]