fix: role_arn support in AWSCloudWatch and AWSEmf exporters

[yaten2302]

Description

This PR attempts to fix re-add the support of role_arn in AWSCloudWatch and AWSEmf exporters.
The support was present in aws-go-sdk-v1, but broke while moving to v2.

Link to tracking issue

Fixes #42115

[axw]

I'm afraid I don't have enough expertise in the AWS SDK to know if this is right. A few things that may help:

• documenting how you have tested this
• adding unit tests
• linking the old code related to assume role, that was working with aws-sdk-go

[yaten2302]

@axw , addressing your comments:

1. I haven't tested this component, because, while I was going through the codebase for v1 and comparing the aws go sdk v1 and v2 changes in the docs, I found that there were some internal changes made as a result of which we would now require to mention about the RoleARN explicitly (in v2).
2. I'll add the unit tests. This would require refactoring of the GetAWSConfig.
3. Previously (till version 0.123), this GetAWSConfig func was named as GetAWSConfigSession. And if we check the 148th line of this func, we've cn.newAWSSession(logger, cfg.RoleARN, cfg.ExternalID, awsRegion). In this, we're passing the cfg.RoleARN. But, this support was removed in v2. So, now we need to explicitly mention that in our func. This has also been implemented in AWSKinesisExporter (aws-go-sdk-v2). If we check from lines - 65-73.

[axw]

This has also been implemented in AWSKinesisExporter (aws-go-sdk-v2). If we check from lines - 65-73.

Thanks @yaten2302. Based on that, assuming it works for Kinesis, I suppose the code change is fine. If nothing else, a little bit of manual testing would be good.

[yaten2302]

@axw , I was trying to refactor the GetAWSConfig() func, so for this I was checking the docs and all.
So, should I go ahead with the adding the unit tests or manual testing would serve the purpose?

[axw]

@yaten2302 in my opinion, a manual test is the minimum requirement. Adding a unit test would be great too, but given the simplicity and precedent for this code I personally wouldn't block on it.

Put another way: I'd like to see evidence of manual testing. Your choice on whether you add a unit test, unless someone else feels strongly.

[yaten2302]

@axw , if I've understood this correctly, then, manual testing is a must for this PR, so that it doesn't breaks in future, right?
I also haven't used this component or AWSCloudWatch. So, maybe we can ask those folks who actually depend this component.
But, is manual testing (on real logs/data on AWS cloud-watch or emf), really possible locally?

WDYT?

[axw]

if I've understood this correctly, then, manual testing is a must for this PR, so that it doesn't breaks in future, right?

Manual testing is just to make sure the change does what you intended. That code can unintentionally removed or broken, so it doesn't prevent future breakage.

Automated tests will help prevent future breakage. Ideally you should have some functional tests that would fail if the role_arn support breaks.

I also haven't used this component or AWSCloudWatch. So, maybe we can ask those folks who actually depend this component.

Maybe @pavolloffay?

But, is manual testing (on real logs/data on AWS cloud-watch or emf), really possible locally?

I don't see why not. If it's not possible, or not practical, you could also run the collector in an EC2 instance.

[pavolloffay]

If anyone wants to test this I have built a docker image pavolloffay/otelcol-contrib:fix-aws-rolearn2

[yaten2302]

Hi @pavolloffay , for this comment, I'm refactoring the GetAWSConfig func and then we'll be able to unit test that code. We can pass an stsClient into the func and nil if role_arn isn't mentioned. And according to that, we can unit test this code.

Have I got your point correctly?

[yaten2302]

Also, thanks for the docker img. For manually testing this, can I deploy this on localstack and test it out if it's working or not?

[pavolloffay]

Our QE test the cloudwatch logs exporter with this fix and it works. Thanks for putting this together!

[andrzej-stencel]

Thanks @yaten2302 for the contribution, much appreciated. Also thank you everyone involved in testing this manually.

We should have a unit test for this if possible.

Other than that, can we get the eyes of some AWS subject matter experts on this before merging? @mxiamxia @Aneurysm9 can you take a look as codeowners?

[yaten2302]

Hi @andrzej-stencel @pavolloffay , thanks for the reviews, I'll push the unit tests for this change soon 👍

[yaten2302]

I want to ask here that the awsxray_test.go tests are failing in the CI, but locally (on my device), everything is working fine.

[pavolloffay]

It seems they pass here as well no?

[yaten2302]

In CI, it's failing. But on locally it's working fine.
Is there some other behaviour on your side?

[pavolloffay]

Could someone merge this PR?