CannotPullContainerError/ASM error message enhancements

[nineonine]

Summary

This change does a few things:

• introduces new abstraction to parse known Docker errors and augment error message with extra useful context. The first error that we handle now is CannotPullContainerError, spefically the missing ECR pull permissions, broken or non-existent image repo url .
• adds corresponding test coverage
• refactors asm_test.go implementation to use gomock
• enhances error message for failed ASM get secret API calls, specifically the ResourceNotFoundException
• Unit tests added in asm_test.go
• updated asmsecret_test.go to account for removed prefix and updated message

Implementation details

• new module was added with all the encapsulated logic - error_messages.go
• in engine.pullAndUpdateContainerReference we use new functionality to attempt to update error message when image pull failed due CannotPullContainerError

Testing

• Unit tests added in error_messages_test.go
• another test in docker_task_engine_test.go to capture augmented message.

New tests cover the changes:
yes

Manual testing steps and describe-tasks outputs

1. Invalid registry - use non existent registry URL.

"containers": [
    {
        ... 
        "name": "foo",
        "image": "some/nonsense:latest",
        "lastStatus": "STOPPED",
        "reason": "CannotPullContainerError: The task can’t pull the image. Check whether the image exists. Error response from daemon: pull access denied for some/nonsense, repository does not exist or may require 'docker login': denied: requested access to the resource ",
        "networkInterfaces": [ ... ],
        "healthStatus": "UNKNOWN",
        "cpu": "0",
        ...
    }

2. Missing permissions - remove ecr:BatchGetImage from policy attached to task execution role

"containers": [
    {
        ...
        "name": "main_container",
        "image": "123123123123.dkr.ecr.us-east-1.amazonaws.com/test_image:latest",
        "lastStatus": "STOPPED",
        "reason": "CannotPullContainerError: The task can’t pull the image. Check that the role has the permissions to pull images from the registry. Error response from daemon: pull access denied for 123123123123.dkr.ecr.us-east-1.amazonaws.com/test_image, repository does not exist or may require 'docker login': denied: User: arn:aws:sts::123123123123:assumed-role/MissinECRPull_ECSTaskExecutionRoleRole/60bece4b8db140d18dad0acd964c0889 is not authorized to perform: ecr:BatchGetImage on resource: arn:aws:ecr:us-east-1:123123123123:repository/test_image because no identity-based policy allows the ecr:BatchGetImage action",
        "healthStatus": "UNKNOWN",
        "cpu": "0",
        ...
    }
],

3. Missing permissions - remove ecr:BatchGetImage from policy attached to task ec2 instance role

"containers": [
    {
        ...
        "name": "main_container",
        "image": "123123123123.dkr.ecr.us-east-1.amazonaws.com/test_image:latest",
        "lastStatus": "STOPPED",
        "reason": "CannotPullContainerError: The task can’t pull the image. Check that the role has the permissions to pull images from the registry. Error response from daemon: pull access denied for 123123123123.dkr.ecr.us-east-1.amazonaws.com/test_image, repository does not exist or may require 'docker login': denied: User: arn:aws:sts::123123123123:assumed-role/ecsInstanceRole/i-0123456789fff is not authorized to perform: ecr:BatchGetImage on resource: arn:aws:ecr:us-east-1:123123123123:repository/test_image because no identity-based policy allows the ecr:BatchGetImage action",
        "healthStatus": "UNKNOWN",
        "cpu": "0",
        ...
    }
],

4. CannotPull - network issue. Delete SG outbound rule (pulling from ECR).

"containers": [
    {
        ...
        "name": "main_container",
        "image": "123123123123.dkr.ecr.us-east-1.amazonaws.com/test_image:latest",
        "lastStatus": "STOPPED",
        "reason": "CannotPullECRContainerError: The task can’t pull the image. Check your network configuration. RequestError: send request failed\ncaused by: Post \"https://api.ecr.us-east-1.amazonaws.com/\": net/http: request canceled while waiting for connection (Client.Timeout exceeded while awaiting headers)",
        "healthStatus": "UNKNOWN",
        "cpu": "0"
        ...
    }
],

5. CannotPull - network issue. Delete SG outbound rule (pulling from private registry).

"containers": [
    {
        ...
        "name": "main_container",
        "image": "123123123123.dkr.ecr.us-east-1.amazonaws.com/test_image:latest",
        "lastStatus": "STOPPED",
        "reason": "CannotPullContainerError: The task can’t pull the image. Check your network configuration. Error response from daemon: Get \"https://registry-1.docker.io/v2/\": net/http: request canceled while waiting for connection (Client.Timeout exceeded while awaiting headers)",
        "healthStatus": "UNKNOWN",
        "cpu": "0"
        ...
    }
],

6. Missing secret - pass non-existent secret via taskDef.container.secrets

{
    "tasks": [
          {
             "stopCode": "TaskFailedToStart",
            "stoppedAt": "2024-06-03T19:03:47.816000-07:00",
            "stoppedReason": "ResourceNotFoundException: The task can't retrieve the secret with ARN 'arn:aws:secretsmanager:us-east-1:123123123123:secret:foo-bar-baz' from AWS Secrets Manager. Check whether the secret exists in the specified Region: ResourceNotFoundException: Secrets Manager can't find the specified secret.",
            "stoppingAt": "2024-06-03T19:03:34.828000-07:00",
            "tags": [],
          }
     ]
}

Description for the changelog

Enhancement: update CannotPullContainerError error messages

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[amogh09]

Thanks for this change! Let's please add details about any manual tests performed to validate this changes. The new error messages seen in describe-tasks response in particular would be very nice to have in the PR description.

The base branch was changed.

[sparrc]

it seems like this is the only place where AugmentNamedErrMsg is used? Is there some plan to add more errors to this?

[nineonine]

At this time, there are no plans to continue updating other messages using AugmentNamedErrMsg. This function was implemented and applied to specifically for the scenarios we are trying to address. If future requirements necessitate similar enhancements for additional error messages, we can consider extending its usage then.

[sparrc]

why is this a map? I don't see it's keys being used anywhere?

[nineonine]

You are right, not used right now. I did it just for symmetry with formatters and (arguably) slightly better readability of code. We can change it if you feel strongly. Let me know what you think. Thanks.

[sparrc]

I'm not sure I fully understand the point of this interface. Why not just have a function in the errormessages package that can take in a named error, augment the message if it can (using the available parser functions), and then return it? Why the need to create an interface and add this function to particular named errors?

[nineonine]

Excellent suggestion, Thanks. I considered this approach too. Once slight complication is that we would need to somehow rebuild NamedError and return it. This creates dependencies on all modules that would need to be imported to bring error into scope.
The decision to create an interface was made to improve modularity. By doing this, we avoid the need to import all the specific error types into the errormessages package. This design allows each error type to implement the interface independently, keeping the errormessages package clean and free from dependencies. I felt this approach makes the codebase more maintainable and flexible, allowing new error types to be augmented without modifying the main parsing driver - AugmentNamedErrMsg and AugmentMessage function.
If you think it is an overkill - we can certainly change it, I am fine doing it either way.