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

Fix panic in AWS ECS detector if container ARN is not valid #3583

Merged
merged 7 commits into from Nov 24, 2023

Conversation

DarthSim
Copy link
Contributor

This PR fixes the panic that may occur here: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/main/detectors/aws/ecs/ecs.go#L116

If containerMetadata.ContainerARN doesn't include : (that is our client case), strings.LastIndex(containerMetadata.ContainerARN, ":") will return -1. This leads to containerMetadata.ContainerARN[:-1], and index out of range panic.

Here's what changes in this PR do:

  • Try to extract baseArn from taskMetadata.TaskARN', 'containerMetadata.ContainerARN', and 'taskMetadata.Cluster
  • If baseArn is successfully extracted, and some of the ARNs aren't full, fix these ARNs

@DarthSim DarthSim requested review from a team and Aneurysm9 as code owners March 14, 2023 17:31
CHANGELOG.md Outdated Show resolved Hide resolved
detectors/aws/ecs/ecs.go Outdated Show resolved Hide resolved
detectors/aws/ecs/ecs.go Show resolved Hide resolved
detectors/aws/ecs/ecs.go Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 20, 2023

Codecov Report

Merging #3583 (30decb6) into main (ab49dc8) will decrease coverage by 0.2%.
The diff coverage is 0.0%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3583     +/-   ##
=======================================
- Coverage   80.9%   80.8%   -0.2%     
=======================================
  Files        150     150             
  Lines      10361   10374     +13     
=======================================
  Hits        8388    8388             
- Misses      1831    1844     +13     
  Partials     142     142             
Files Coverage Δ
detectors/aws/ecs/ecs.go 37.7% <0.0%> (-3.8%) ⬇️

@DarthSim
Copy link
Contributor Author

Hey everyone!
Sorry for the long delay. I updated the PR:

  • Added an entry about the task ARN to the changelog.
  • Added test for fixing the task ARN.
  • Applied suggested changes.

@pellared pellared dismissed their stale review May 11, 2023 06:30

dismissing my review as changes were applied and I have not time to review it now

@DarthSim
Copy link
Contributor Author

DarthSim commented Oct 5, 2023

Hey @pellared @Aneurysm9!

Is there any chance for this PR to be merged? Some of our users face the issue fixed by this PR, so I have to use a fork of this package and update it every time I update the project dependencies; this is pretty annoying.

@pellared pellared merged commit 932d191 into open-telemetry:main Nov 24, 2023
21 of 22 checks passed
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

3 participants