-
Notifications
You must be signed in to change notification settings - Fork 331
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
Use sts to construct stream arn #463
Conversation
arn << "arn:" << sts_arn.GetPartition() << ":kinesis:" << region << ":" << result.GetAccount() | ||
<< ":stream/" << stream_name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this stream arn format same for all regions? I thought we had some issues with arn format in some regions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good call out, we once had a problem with pod1, but I think the format is good for all public commercial regions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, we should still test to make sure this works in all partitions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
arn << "arn:" << sts_arn.GetPartition() << ":kinesis:" << region << ":" << result.GetAccount() | ||
<< ":stream/" << stream_name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, we should still test to make sure this works in all partitions.
aws/kinesis/core/pipeline.h
Outdated
return arn_str; | ||
} | ||
|
||
LOG(warning) << "Failed to get StreamARN using STS GetCallerIdentity with exception: " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should throw if this fails.
that will avoid operating in dual-mode, and have all applications start using ARN from this version onwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Description of changes:
Testing
amazon-kinesis-producer-sample
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.