Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CannotPullContainerError/ASM error message enhancements #4181

Open
wants to merge 8 commits into
base: feature/project-telescope
Choose a base branch
from

Conversation

nineonine
Copy link

@nineonine nineonine commented May 16, 2024

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 image repo url or non-existent repos.
  • enhances error message for failed ASM get secret API calls, specifically tht ResourceNotFoundException
  • adds corresponding test coverage
  • refactors asm_test.go implementation to use gomock

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
  • Unit tests added in asm_test.go
  • another test in docker_task_engine_test.go to capture augmented message.
  • updated asmsecret_test.go to account for removed prefix and updated message

New tests cover the changes:
yes

Description for the changelog

Augment CannotPullContainerError/ ASM error messages with extra context to aid in troubleshooting

Licensing

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

@nineonine nineonine requested a review from a team as a code owner May 16, 2024 05:02
@nineonine nineonine force-pushed the cannot-pull-container-err-msg branch from 5dd5027 to 65a2859 Compare May 16, 2024 16:37
Improve CannotPullContainerError message
@nineonine nineonine force-pushed the cannot-pull-container-err-msg branch 2 times, most recently from 0a68d36 to ffcd9bf Compare May 16, 2024 22:05
@nineonine nineonine force-pushed the cannot-pull-container-err-msg branch from ffcd9bf to dc51281 Compare May 16, 2024 22:41
@nineonine nineonine force-pushed the cannot-pull-container-err-msg branch from 470ee7c to ec5361e Compare May 18, 2024 01:16
@nineonine nineonine force-pushed the cannot-pull-container-err-msg branch from ec5361e to 5d7418b Compare May 18, 2024 01:27
ecs-agent/api/errors/error_messages.go Outdated Show resolved Hide resolved
ecs-agent/api/errors/error_messages.go Outdated Show resolved Hide resolved
ecs-agent/api/errors/error_messages.go Outdated Show resolved Hide resolved
ecs-agent/api/errors/error_messages.go Outdated Show resolved Hide resolved
ecs-agent/api/errors/error_messages.go Outdated Show resolved Hide resolved
ecs-agent/api/errors/error_messages.go Outdated Show resolved Hide resolved
agent/engine/docker_task_engine_test.go Outdated Show resolved Hide resolved
agent/engine/docker_task_engine_test.go Show resolved Hide resolved
agent/asm/asm_test.go Outdated Show resolved Hide resolved
agent/asm/asm.go Outdated Show resolved Hide resolved
Alex Dudarenko added 2 commits May 30, 2024 23:40
* CannotPullContainerError: cleanup, renaming
* ASM error: do not use added errormessages code - match on AWS SDK
  error directly, drop tracing prefix, fix/add more unit tests
agent/asm/asm.go Outdated Show resolved Hide resolved
@nineonine nineonine changed the title Improve CannotPullContainerError message CannotPullContainerError/ASM error message enhancements May 31, 2024
@amogh09
Copy link
Contributor

amogh09 commented May 31, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants