Redact ECR layer URLs from container pull errors

[prateekchaudhry]

Summary

Redact error messages with URLs containing references to string starport-layer-bucket (ECR buckets). ECR buckets named as prod-region-starport-layer-bucket are used to fetch image layers from.

This redaction is done because container runtime's request may sometimes contain security tokens when accessing ECR buckets for pulling layers. When these requests error out, the URLs with secrets may get bubbled up to Agent logs and ECS logs through Reason field in Submit Task State Change calls. So we redact the otherwise hidden URLs for security.

Implementation details

Replace regex \"?https[^\s]+starport-layer-bucket[^\s]+ with a string message containing image reference

Testing

• Unit tests are added and verified with
• Manually verified this doesn't affect some common Container Pull error messages like

DockerGoClient: failed to pull image <acc id>.dkr.ecr.us-west-2.amazonaws.com/benchmark-app:latest: [CannotPullContainerError] Error response from daemon: pull access denied for <acc id>.dkr.ecr.us-west-2.amazonaws.com/benchmark-app, repository does not exist or may require 'docker login': denied: User: arn:aws:sts::<acc id>:assumed-role/ecsInstanceRole/<inst id> is not authorized to perform....

New tests cover the changes:
Yes

Description for the changelog

Redact ECR layer URLs from container pull errors

Licensing

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

[fierlion]

we're being very prescriptive in this redact function (ie this applies only to ECR). This is fine for now. I can see this being applicable to more cases in future.

Also nit maybe we call this not str, but image so it's clear that the error returns the image in place of the redacted URL?

[mythri-garaga]

synced up offline with @prateekchaudhry, for now we are restricting redacting only ECR's URLs containing starport-layer-bucket bucket name and cannot make this regex expression more generic as there is no evidence of other URLs containing security token and might need them for debugging purpose.

[prateekchaudhry]

That is right, being prescriptive is the intention - we can make it more generic if we have to in future.

Ack, I'll replace str with overrideStr to keep the function usage generic and more clear, if that is fine.