From 291aa83137e2091fc05c1daa53cfa27b575541b2 Mon Sep 17 00:00:00 2001 From: Bert Blommers Date: Tue, 18 Jan 2022 19:53:31 -0100 Subject: [PATCH] ECS - LongARN is enabled by default (#4762) --- docs/docs/services/ecs.rst | 2 + moto/ecs/models.py | 24 +++++-- moto/settings.py | 3 +- tests/test_core/test_settings.py | 27 +++++++ tests/test_ecs/test_ecs_account_settings.py | 74 +++++++++---------- tests/test_ecs/test_ecs_boto3.py | 79 +++++++++++++++------ 6 files changed, 144 insertions(+), 65 deletions(-) create mode 100644 tests/test_core/test_settings.py diff --git a/docs/docs/services/ecs.rst b/docs/docs/services/ecs.rst index 230d988aa..513fde20d 100644 --- a/docs/docs/services/ecs.rst +++ b/docs/docs/services/ecs.rst @@ -12,6 +12,8 @@ ecs === +.. autoclass:: moto.ecs.models.EC2ContainerServiceBackend + |start-h3| Example usage |end-h3| .. sourcecode:: python diff --git a/moto/ecs/models.py b/moto/ecs/models.py index d5cfc57fe..15417bedf 100644 --- a/moto/ecs/models.py +++ b/moto/ecs/models.py @@ -716,6 +716,14 @@ class TaskSet(BaseObject): class EC2ContainerServiceBackend(BaseBackend): + """ + ECS resources use the new ARN format by default. + Use the following environment variable to revert back to the old/short ARN format: + `MOTO_ECS_NEW_ARN=false` + + AWS reference: https://aws.amazon.com/blogs/compute/migrating-your-amazon-ecs-deployment-to-the-new-arn-and-resource-id-format-2/ + """ + def __init__(self, region_name): super().__init__() self.account_settings = dict() @@ -1558,9 +1566,15 @@ class EC2ContainerServiceBackend(BaseBackend): @staticmethod def _parse_resource_arn(resource_arn): match = re.match( - "^arn:aws:ecs:(?P[^:]+):(?P[^:]+):(?P[^:]+)/(?P.*)$", + "^arn:aws:ecs:(?P[^:]+):(?P[^:]+):(?P[^:]+)/(?P[^:]+)/(?P.*)$", resource_arn, ) + if not match: + # maybe a short-format ARN + match = re.match( + "^arn:aws:ecs:(?P[^:]+):(?P[^:]+):(?P[^:]+)/(?P.*)$", + resource_arn, + ) if not match: raise JsonRESTError( "InvalidParameterException", "The ARN provided is invalid." @@ -1784,12 +1798,10 @@ class EC2ContainerServiceBackend(BaseBackend): self.account_settings.pop(name, None) def enable_long_arn_for_name(self, name): - if settings.ecs_new_arn_format(): - return True account = self.account_settings.get(name, None) - if account and account.value == "enabled": - return True - return False + if account and account.value == "disabled": + return False + return settings.ecs_new_arn_format() ecs_backends = BackendDict(EC2ContainerServiceBackend, "ecs") diff --git a/moto/settings.py b/moto/settings.py index 961c47d22..ce21ac665 100644 --- a/moto/settings.py +++ b/moto/settings.py @@ -43,7 +43,8 @@ def get_s3_default_key_buffer_size(): def ecs_new_arn_format(): - return os.environ.get("MOTO_ECS_NEW_ARN", "false").lower() == "true" + # True by default - only the value 'false' will return false + return os.environ.get("MOTO_ECS_NEW_ARN", "true").lower() != "false" def allow_unknown_region(): diff --git a/tests/test_core/test_settings.py b/tests/test_core/test_settings.py new file mode 100644 index 000000000..b720e3d86 --- /dev/null +++ b/tests/test_core/test_settings.py @@ -0,0 +1,27 @@ +import os +import mock +import pytest +import sure # noqa # pylint: disable=unused-import + +from moto import settings + + +""" +Sanity checks for interpretation of the MOTO_ECS_NEW_ARN-variable +""" + + +def test_default_is_true(): + settings.ecs_new_arn_format().should.equal(True) + + +@pytest.mark.parametrize("value", ["TrUe", "true", "invalid", "0", "1"]) +def test_anything_but_false_is_true(value): + with mock.patch.dict(os.environ, {"MOTO_ECS_NEW_ARN": value}): + settings.ecs_new_arn_format().should.equal(True) + + +@pytest.mark.parametrize("value", ["False", "false", "faLse"]) +def test_only_false_is_false(value): + with mock.patch.dict(os.environ, {"MOTO_ECS_NEW_ARN": value}): + settings.ecs_new_arn_format().should.equal(False) diff --git a/tests/test_ecs/test_ecs_account_settings.py b/tests/test_ecs/test_ecs_account_settings.py index 76056ffce..537a200e7 100644 --- a/tests/test_ecs/test_ecs_account_settings.py +++ b/tests/test_ecs/test_ecs_account_settings.py @@ -103,7 +103,7 @@ def test_delete_account_setting(): @mock_ecs def test_put_account_setting_changes_service_arn(): client = boto3.client("ecs", region_name="eu-west-1") - client.put_account_setting(name="serviceLongArnFormat", value="enabled") + client.put_account_setting(name="serviceLongArnFormat", value="disabled") _ = client.create_cluster(clusterName="dummy-cluster") _ = client.register_task_definition( @@ -126,7 +126,15 @@ def test_put_account_setting_changes_service_arn(): tags=[{"key": "ResourceOwner", "value": "Dummy"}], ) - # Initial response is long + # Initial response is short (setting serviceLongArnFormat=disabled) + response = client.list_services(cluster="dummy-cluster", launchType="FARGATE") + service_arn = response["serviceArns"][0] + service_arn.should.equal( + "arn:aws:ecs:eu-west-1:{}:service/test-ecs-service".format(ACCOUNT_ID) + ) + + # Second invocation returns long ARN's by default, after deleting the preference + client.delete_account_setting(name="serviceLongArnFormat") response = client.list_services(cluster="dummy-cluster", launchType="FARGATE") service_arn = response["serviceArns"][0] service_arn.should.equal( @@ -135,14 +143,6 @@ def test_put_account_setting_changes_service_arn(): ) ) - # Second invocation returns short ARN's, after deleting the longArn-preference - client.delete_account_setting(name="serviceLongArnFormat") - response = client.list_services(cluster="dummy-cluster", launchType="FARGATE") - service_arn = response["serviceArns"][0] - service_arn.should.equal( - "arn:aws:ecs:eu-west-1:{}:service/test-ecs-service".format(ACCOUNT_ID) - ) - @mock_ec2 @mock_ecs @@ -162,19 +162,7 @@ def test_put_account_setting_changes_containerinstance_arn(): ec2_utils.generate_instance_identity_document(test_instance) ) - # Initial ARN should be short - response = ecs_client.register_container_instance( - cluster=test_cluster_name, instanceIdentityDocument=instance_id_document - ) - full_arn = response["containerInstance"]["containerInstanceArn"] - full_arn.should.match( - f"arn:aws:ecs:us-east-1:{ACCOUNT_ID}:container-instance/[a-z0-9-]+$" - ) - - # Now enable long-format - ecs_client.put_account_setting( - name="containerInstanceLongArnFormat", value="enabled" - ) + # Initial ARN should be long response = ecs_client.register_container_instance( cluster=test_cluster_name, instanceIdentityDocument=instance_id_document ) @@ -183,6 +171,18 @@ def test_put_account_setting_changes_containerinstance_arn(): f"arn:aws:ecs:us-east-1:{ACCOUNT_ID}:container-instance/{test_cluster_name}/[a-z0-9-]+$" ) + # Now disable long-format + ecs_client.put_account_setting( + name="containerInstanceLongArnFormat", value="disabled" + ) + response = ecs_client.register_container_instance( + cluster=test_cluster_name, instanceIdentityDocument=instance_id_document + ) + full_arn = response["containerInstance"]["containerInstanceArn"] + full_arn.should.match( + f"arn:aws:ecs:us-east-1:{ACCOUNT_ID}:container-instance/[a-z0-9-]+$" + ) + @mock_ec2 @mock_ecs @@ -217,20 +217,7 @@ def test_run_task_default_cluster_new_arn_format(): } ], ) - # Initial ARN is short-format - client.put_account_setting(name="taskLongArnFormat", value="disabled") - response = client.run_task( - launchType="FARGATE", - overrides={}, - taskDefinition="test_ecs_task", - count=1, - startedBy="moto", - ) - response["tasks"][0]["taskArn"].should.match( - f"arn:aws:ecs:us-east-1:{ACCOUNT_ID}:task/[a-z0-9-]+$" - ) - - # Enable long-format for the next task + # Initial ARN is long-format client.put_account_setting(name="taskLongArnFormat", value="enabled") response = client.run_task( launchType="FARGATE", @@ -242,3 +229,16 @@ def test_run_task_default_cluster_new_arn_format(): response["tasks"][0]["taskArn"].should.match( f"arn:aws:ecs:us-east-1:{ACCOUNT_ID}:task/{test_cluster_name}/[a-z0-9-]+$" ) + + # Enable short-format for the next task + client.put_account_setting(name="taskLongArnFormat", value="disabled") + response = client.run_task( + launchType="FARGATE", + overrides={}, + taskDefinition="test_ecs_task", + count=1, + startedBy="moto", + ) + response["tasks"][0]["taskArn"].should.match( + f"arn:aws:ecs:us-east-1:{ACCOUNT_ID}:task/[a-z0-9-]+$" + ) diff --git a/tests/test_ecs/test_ecs_boto3.py b/tests/test_ecs/test_ecs_boto3.py index 0de0623c1..ef9aec94a 100644 --- a/tests/test_ecs/test_ecs_boto3.py +++ b/tests/test_ecs/test_ecs_boto3.py @@ -520,7 +520,9 @@ def test_create_service(): response["service"]["pendingCount"].should.equal(0) response["service"]["runningCount"].should.equal(0) response["service"]["serviceArn"].should.equal( - "arn:aws:ecs:us-east-1:{}:service/test_ecs_service".format(ACCOUNT_ID) + "arn:aws:ecs:us-east-1:{}:service/test_ecs_cluster/test_ecs_service".format( + ACCOUNT_ID + ) ) response["service"]["serviceName"].should.equal("test_ecs_service") response["service"]["status"].should.equal("ACTIVE") @@ -610,7 +612,9 @@ def test_create_service_scheduling_strategy(): response["service"]["pendingCount"].should.equal(0) response["service"]["runningCount"].should.equal(0) response["service"]["serviceArn"].should.equal( - "arn:aws:ecs:us-east-1:{}:service/test_ecs_service".format(ACCOUNT_ID) + "arn:aws:ecs:us-east-1:{}:service/test_ecs_cluster/test_ecs_service".format( + ACCOUNT_ID + ) ) response["service"]["serviceName"].should.equal("test_ecs_service") response["service"]["status"].should.equal("ACTIVE") @@ -657,10 +661,14 @@ def test_list_services(): unfiltered_response = client.list_services(cluster="test_ecs_cluster") len(unfiltered_response["serviceArns"]).should.equal(2) unfiltered_response["serviceArns"][0].should.equal( - "arn:aws:ecs:us-east-1:{}:service/test_ecs_service1".format(ACCOUNT_ID) + "arn:aws:ecs:us-east-1:{}:service/test_ecs_cluster/test_ecs_service1".format( + ACCOUNT_ID + ) ) unfiltered_response["serviceArns"][1].should.equal( - "arn:aws:ecs:us-east-1:{}:service/test_ecs_service2".format(ACCOUNT_ID) + "arn:aws:ecs:us-east-1:{}:service/test_ecs_cluster/test_ecs_service2".format( + ACCOUNT_ID + ) ) filtered_response = client.list_services( @@ -668,7 +676,9 @@ def test_list_services(): ) len(filtered_response["serviceArns"]).should.equal(1) filtered_response["serviceArns"][0].should.equal( - "arn:aws:ecs:us-east-1:{}:service/test_ecs_service1".format(ACCOUNT_ID) + "arn:aws:ecs:us-east-1:{}:service/test_ecs_cluster/test_ecs_service1".format( + ACCOUNT_ID + ) ) @@ -715,16 +725,22 @@ def test_describe_services(): cluster="test_ecs_cluster", services=[ "test_ecs_service1", - "arn:aws:ecs:us-east-1:{}:service/test_ecs_service2".format(ACCOUNT_ID), + "arn:aws:ecs:us-east-1:{}:service/test_ecs_cluster/test_ecs_service2".format( + ACCOUNT_ID + ), ], ) len(response["services"]).should.equal(2) response["services"][0]["serviceArn"].should.equal( - "arn:aws:ecs:us-east-1:{}:service/test_ecs_service1".format(ACCOUNT_ID) + "arn:aws:ecs:us-east-1:{}:service/test_ecs_cluster/test_ecs_service1".format( + ACCOUNT_ID + ) ) response["services"][0]["serviceName"].should.equal("test_ecs_service1") response["services"][1]["serviceArn"].should.equal( - "arn:aws:ecs:us-east-1:{}:service/test_ecs_service2".format(ACCOUNT_ID) + "arn:aws:ecs:us-east-1:{}:service/test_ecs_cluster/test_ecs_service2".format( + ACCOUNT_ID + ) ) response["services"][1]["serviceName"].should.equal("test_ecs_service2") @@ -745,7 +761,9 @@ def test_describe_services(): cluster="test_ecs_cluster", services=[ "test_ecs_service1", - "arn:aws:ecs:us-east-1:{}:service/test_ecs_service2".format(ACCOUNT_ID), + "arn:aws:ecs:us-east-1:{}:service/test_ecs_cluster/test_ecs_service2".format( + ACCOUNT_ID + ), ], include=["TAGS"], ) @@ -830,17 +848,23 @@ def test_describe_services_scheduling_strategy(): cluster="test_ecs_cluster", services=[ "test_ecs_service1", - "arn:aws:ecs:us-east-1:{}:service/test_ecs_service2".format(ACCOUNT_ID), + "arn:aws:ecs:us-east-1:{}:service/test_ecs_cluster/test_ecs_service2".format( + ACCOUNT_ID + ), "test_ecs_service3", ], ) len(response["services"]).should.equal(3) response["services"][0]["serviceArn"].should.equal( - "arn:aws:ecs:us-east-1:{}:service/test_ecs_service1".format(ACCOUNT_ID) + "arn:aws:ecs:us-east-1:{}:service/test_ecs_cluster/test_ecs_service1".format( + ACCOUNT_ID + ) ) response["services"][0]["serviceName"].should.equal("test_ecs_service1") response["services"][1]["serviceArn"].should.equal( - "arn:aws:ecs:us-east-1:{}:service/test_ecs_service2".format(ACCOUNT_ID) + "arn:aws:ecs:us-east-1:{}:service/test_ecs_cluster/test_ecs_service2".format( + ACCOUNT_ID + ) ) response["services"][1]["serviceName"].should.equal("test_ecs_service2") @@ -1038,7 +1062,9 @@ def test_delete_service(): response["service"]["pendingCount"].should.equal(0) response["service"]["runningCount"].should.equal(0) response["service"]["serviceArn"].should.equal( - "arn:aws:ecs:us-east-1:{}:service/test_ecs_service".format(ACCOUNT_ID) + "arn:aws:ecs:us-east-1:{}:service/test_ecs_cluster/test_ecs_service".format( + ACCOUNT_ID + ) ) response["service"]["serviceName"].should.equal("test_ecs_service") response["service"]["status"].should.equal("ACTIVE") @@ -1085,7 +1111,9 @@ def test_delete_service_force(): response["service"]["pendingCount"].should.equal(0) response["service"]["runningCount"].should.equal(0) response["service"]["serviceArn"].should.equal( - "arn:aws:ecs:us-east-1:{}:service/test_ecs_service".format(ACCOUNT_ID) + "arn:aws:ecs:us-east-1:{}:service/test_ecs_cluster/test_ecs_service".format( + ACCOUNT_ID + ) ) response["service"]["serviceName"].should.equal("test_ecs_service") response["service"]["status"].should.equal("ACTIVE") @@ -1176,7 +1204,8 @@ def test_register_container_instance(): arn_part[0].should.equal( "arn:aws:ecs:us-east-1:{}:container-instance".format(ACCOUNT_ID) ) - arn_part[1].should.equal(str(UUID(arn_part[1]))) + arn_part[1].should.equal("test_ecs_cluster") + arn_part[2].should.equal(str(UUID(arn_part[2]))) response["containerInstance"]["status"].should.equal("ACTIVE") len(response["containerInstance"]["registeredResources"]).should.equal(4) len(response["containerInstance"]["remainingResources"]).should.equal(4) @@ -1356,7 +1385,7 @@ def test_describe_container_instances(): test_instance_arns.append(response["containerInstance"]["containerInstanceArn"]) - test_instance_ids = list(map((lambda x: x.split("/")[1]), test_instance_arns)) + test_instance_ids = list(map((lambda x: x.split("/")[-1]), test_instance_arns)) response = ecs_client.describe_container_instances( cluster=test_cluster_name, containerInstances=test_instance_ids ) @@ -1424,7 +1453,7 @@ def test_update_container_instances_state(): test_instance_arns.append(response["containerInstance"]["containerInstanceArn"]) - test_instance_ids = list(map((lambda x: x.split("/")[1]), test_instance_arns)) + test_instance_ids = list(map((lambda x: x.split("/")[-1]), test_instance_arns)) response = ecs_client.update_container_instances_state( cluster=test_cluster_name, containerInstances=test_instance_ids, @@ -1641,7 +1670,7 @@ def test_run_task_default_cluster(): len(response["tasks"]).should.equal(2) response["tasks"][0].should.have.key("launchType").equals("FARGATE") response["tasks"][0]["taskArn"].should.match( - "arn:aws:ecs:us-east-1:{}:task/[a-z0-9-]+$".format(ACCOUNT_ID) + "arn:aws:ecs:us-east-1:{}:task/default/[a-z0-9-]+$".format(ACCOUNT_ID) ) response["tasks"][0]["clusterArn"].should.equal( "arn:aws:ecs:us-east-1:{}:cluster/default".format(ACCOUNT_ID) @@ -1790,7 +1819,7 @@ def test_start_task(): "arn:aws:ecs:us-east-1:{}:task-definition/test_ecs_task:1".format(ACCOUNT_ID) ) response["tasks"][0]["containerInstanceArn"].should.equal( - "arn:aws:ecs:us-east-1:{0}:container-instance/{1}".format( + "arn:aws:ecs:us-east-1:{0}:container-instance/test_ecs_cluster/{1}".format( ACCOUNT_ID, container_instance_id ) ) @@ -2681,7 +2710,9 @@ def test_create_service_load_balancing(): response["service"]["pendingCount"].should.equal(0) response["service"]["runningCount"].should.equal(0) response["service"]["serviceArn"].should.equal( - "arn:aws:ecs:us-east-1:{}:service/test_ecs_service".format(ACCOUNT_ID) + "arn:aws:ecs:us-east-1:{}:service/test_ecs_cluster/test_ecs_service".format( + ACCOUNT_ID + ) ) response["service"]["serviceName"].should.equal("test_ecs_service") response["service"]["status"].should.equal("ACTIVE") @@ -2779,8 +2810,14 @@ def test_list_tags_for_resource_ecs_service(): @mock_ecs -def test_ecs_service_tag_resource(): +@pytest.mark.parametrize("long_arn", ["disabled", "enabled"]) +def test_ecs_service_tag_resource(long_arn): + """ + Tagging does some weird ARN parsing - ensure it works with both long and short formats + """ client = boto3.client("ecs", region_name="us-east-1") + client.put_account_setting(name="serviceLongArnFormat", value=long_arn) + _ = client.create_cluster(clusterName="test_ecs_cluster") _ = client.register_task_definition( family="test_ecs_task",