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

don't try to reach STS if role_arn is not specified #4681

Closed
wants to merge 2 commits into from

Conversation

MichaelCosby
Copy link

@MichaelCosby MichaelCosby commented Sep 11, 2018

When telegraf is running inside a VPC, STS (AWS Simple Token Service) may not be reachable. This makes the cloudwatch output plugin fail before it even tries other methods of getting its authentication tokens.

Since the documentation already states that STS is only tried if role_arn is specified, this patch skips STS entirely if the role_arn is blank.

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

Since the change I'm making here simply makes the existing README.md true, there was no need to modify it.

I didn't add a unit test; I don't know enough about go yet to know how to mock out the sts module and the existing tests don't have a good example to copy. Assuming that's required for this change, I can learn that...

We should only try to reach AWS Simple Token Service is the role_arn was specified. This solves the situation where telegraf is running inside a VPC and doesn't have access to STS but is instead using IAM credentials or another auth mechanism, but auth fails because STS is unreachable
@danielnelson
Copy link
Contributor

This code is only used to test if the authentication works, and isn't critical even with other authentication methods. I think perhaps we should just remove it altogether and rely on errors when we actually try to write. @adamchainz What do you think?

@danielnelson danielnelson added fix pr to fix corresponding bug area/aws AWS plugins including cloudwatch, ecs, kinesis labels Sep 11, 2018
@adamchainz
Copy link
Contributor

Since the documentation already states that STS is only tried if role_arn is specified, this patch skips STS entirely if the role_arn is blank.

That's not quite true. It's saying the credentials are explicitly taken from STS if role_arn is provided.

The check using STS is separate, and something I added because I believed STS is always reachable. Some VPC's being configured to make AWS services unreachable breaks that assumption.

I agree with @danielnelson , removing the check entirely is probably the only sensible course of action. It would still leave my original issue #3319, resolved, just it's a shame configuration can't be validated at startup.

@danielnelson
Copy link
Contributor

I opened a PR that removes the check altogether #4695, I don't think it is worth having the check only in the rare case that the role_arn is set.

Copy link
Contributor

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @danielnelson , #4695 makes more sense

@MichaelCosby
Copy link
Author

That sounds good. I'll go ahead and drop this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/aws AWS plugins including cloudwatch, ecs, kinesis fix pr to fix corresponding bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants