Enable the plugin to assume a role #40
Conversation
|
Based on the other PRs I don't think this is responsible for the broken build. Is that something that is likely to be looked at? |
sihil
force-pushed
the
sihil/assume-role
branch
from
1bf31da to
7c04b37
Compare
|
❗️I've rebased this over #41 to show that it builds - naturally it shouldn't be merged until that has been. |
sihil
force-pushed
the
sihil/assume-role
branch
2 times, most recently
from
934a6ed to
ebfdfc9
Compare
|
Hi @robbavey - I've noticed a number of PRs subsequent to this get reviewed and merged. Do I need to do something to get this picked up by someone? |
|
@sihil Thanks for the reminder - I missed this when I was merging the other PRs. Let me take a look and get back to you |
lib/logstash/inputs/kinesis.rb
Outdated
|
|
||
| # If a role ARN is set then assume the role as a new layer over the credentials already created | ||
| unless @role_arn.nil? | ||
| session_id = "worker" + worker_id |
There was a problem hiding this comment.
Is it possible to add an extra configuration parameter role_session_name for the sake of consistency with other logstash aws plugins (this is in the common aws mixin gem shared by most aws plugins)
There was a problem hiding this comment.
Useful to know. Will look at this today and see what the best approach is.
There was a problem hiding this comment.
I've looked at adopting the mixin for this plugin. Unfortunately the mixin isn't flexible enough to cover all of the options currently in this plugin (e.g. profile can't be specified).
The ideal solution would be to re-work the mixin so these are available everywhere and the credential logic isn't duplicated. However for the time being I'll rework this using the same naming convention and duplicate the logic internally.
sihil
force-pushed
the
sihil/assume-role
branch
from
479e416 to
a07a86c
Compare
sihil
force-pushed
the
sihil/assume-role
branch
from
a07a86c to
56722bf
Compare
|
@robbavey I've addressed your comment. I think the build is failing on master too but I haven't figured out why. I'm also going to deploy my changes to our production environment to confirm they work as expected but can't see why they wouldn't. (Although the more recent KCL requires |
There was a problem hiding this comment.
@sihil The changes look good from this end - I'll be happy to merge this in once you give me the go ahead to do so. The build failure is unrelated - it pertains to a change made on logstash master this morning, and should be fixed now - I'll kick off a rebuild
I'll do a follow-up PR to update the asciidoc, changelog and plugin versions once that has merged, and should be able to get this published shortly.
Thanks for the contribution!
|
Thanks @robbavey! This has been on our prod stack for a few hours now without any issues. Will be glad to no longer be on a fork 😀 |
|
Closing as this has been superseded by #57 |
We have a requirement to get data from a Kinesis stream in a different AWS account.
This PR adds a new
role_arnconfig option that takes an AWS role ARN that will be assumed. This role is assumed from either the default or profile credentials. The newly created credential provider is used only for accessing the kinesis stream. Both dynamo and cloudwatch access by the KCL continue to be done using the underlying credential provider and so these resources are in the home account rather than the account containing the stream.