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

Allow user to provide a role to assume #27

Closed
wants to merge 8 commits into from

Conversation

@patrobinson
Copy link
Contributor

commented Nov 7, 2016

It's a common use case to read ELB logs. But if those ELB logs are written to an s3 bucket in a different account to where Logstash is running, it's impossible to give the third account permission to read them, because ELB logs are not owned by the account they exist in.

This change allows you to specify a role ARN that can be assumed in order to access resources.

patrobinson added some commits Nov 7, 2016

@patrobinson

This comment has been minimized.

Copy link
Contributor Author

commented Nov 9, 2016

I've signed the CLA, but it's not showing up here

patrobinson added some commits Jan 19, 2017

Fix bug with credentials expiring.
Use the assume role credentials provider instead of passing around keys
@aboutte

This comment has been minimized.

Copy link

commented Jan 20, 2017

@patrobinson nice work switching to AssumeRoleCredentials...that is awesome. Do you know what the next step is to get this merged in?

@patrobinson

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2017

Other than pinging the maintainers (@suyograo and @jordansissel) no

@aboutte

This comment has been minimized.

Copy link

commented May 20, 2017

@suyograo and @jordansissel,

What are the next steps for this PR?

@morganchristiansson

This comment has been minimized.

Copy link

commented Aug 4, 2017

Would be very nice to have! 👍

@jordansissel

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2017

I'm not in a good position to evaluate this PR. I have insufficient experience with AWS IAM, Role delegation, etc.

The only review I can offer is to say these things:

  • User-facing documentation changes will be required for all plugins using this mixin. For every plugin using this mixin, the docs/index.asciidoc needs to have the new settings added.
  • Can you add test coverage? The completeness of testing may ultimately determine if we can merge this. [1]
  • Are the new settings role_arn and role_session_name the most informative names we can pick?
  • For the new settings, I am assuming a user may read these new settings and not have any experience with AWS ARN/IAM. This user may become confused "What does 'role to assume' mean?" It will be helpful to include links to AWS references on what these things are, or at least otherwise include some indicator a human can read to identify "should I care about this setting?"

Someone testing this and providing feedback on usability would be helpful also. "It works" is cool, but are the setting names sufficient? What other docs could be added? Can we reduce the number of new settings? etc

[1] Regarding merging and testing -- Given I (and maybe nobody else on the Logstash dev team) don't have any AWS ARN/IAM experience, and this library is covered transitively (through other plugins) by our commercial support contracts (aka: Someone can page us on the weekend if it breaks), I want to feel confident that it works and stays working -- this means it needs good test coverage.

@davbo

This comment has been minimized.

Copy link

commented Oct 27, 2017

I've looked into this, I think the feature would be a great addition but would be made much better by supporting an external ID. My changes are on a work in progress branch, however creating AssumeRoleCredentials actually makes calls to AWS API's so I need to figure out a way of mocking the API for the purposes of tests.

This commit message explains why support external ID is so important.

ps. I'm away for a couple of weeks now but would be happy if someone picked up my changes, otherwise I'll see about fixing the tests in a couple of weeks.

@seymour1

This comment has been minimized.

Copy link

commented Oct 30, 2017

I attempted to build this branch (the PR, not @davbo's work in progress). Line 58 is the new capability for role assumption, but it's only called if access_key_id and secret_access_key are not provided in the configuration file. This means that users can assume a role or authenticate, but not both. Is there some other method for authentication other than the configuration file?

@frezbo

This comment has been minimized.

Copy link

commented Nov 18, 2017

@suyograo and @jordansissel any plans to get this merged, we do a lot of other hacks for assume role and this PR solves a lot of dirty hacks, of its missing something I'm happy to contribute

@patrobinson

This comment has been minimized.

Copy link
Contributor Author

commented Nov 18, 2017

I don't have the bandwidth nor desire to get this over the line. It's not something I use any more and the slow progress of it just re-iterates why I don't want to use Logstash anymore.

Someone else is free to pick up this code and run with it.

@patrobinson patrobinson reopened this Feb 1, 2018

@patrobinson

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2018

@jordansissel I've improved the documentation and added integration tests for the assume_role component, which tests a path this plugin had issues with previously (rotating expired keys).

For every plugin using this mixin, the docs/index.asciidoc needs to have the new settings added.

I appreciate that and that's the nature of the mixin though and not something I can control.

@patrobinson patrobinson referenced this pull request Feb 2, 2018

Merged

Support logstash 5 #2

@patrobinson patrobinson closed this Feb 2, 2018

@patrobinson patrobinson reopened this Feb 2, 2018

@jordansissel

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2018

the slow progress of it

I understand your frustration. I still don't personally have much I can offer in evaluating this PR. I can test it, but I have no idea whether or not it lines up with what the median AWS user would expect, for example.

I can take a look at the code/tests, but fundamentally, someone with AWS IAM experience should be also evaluating this before it merges, and that person, at least for today, is not me.

@jordansissel

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2018

@frezbo @davbo @morganchristiansson @aboutte @ozacas @lukewaite -- You've all expressed interest in this improvement. Are you able to test this and let us know your experience with this patch? Such feedback would aid in moving this PR along in the review process.

@frezbo

This comment has been minimized.

Copy link

commented Feb 2, 2018

@jordansissel Awesome, I could test this when I get some time, but last I checked I saw another change that needs to be pulled in to this PR from this #27 (comment), let me see if I can get some spare time to get those changes, I am not working on anything related to logstash now, so If time permits I could take a look. Meanwhile @davbo or @morganchristiansson or @aboutte or @lukewaite could take a look, I feel the need of importance of external ID, thanls @jordansissel for bringing this up. Would be grateful if you can build a gem and send us the link, as I will have to build from scratch.

@patrobinson

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2018

FWIW, we've used this plugin in production successfully on and off for the past 12 months.

The Travis failures seem unrelated to my PR? The commit that caused the failure just introduced a new test dependency "timecop"

bundler: failed to load command: rspec (/home/travis/build/logstash-plugins/logstash-mixin-aws/vendor/bundle/jruby/2.3.0/bin/rspec)
Gem::LoadError: You have already activated jar-dependencies 0.3.10, but your Gemfile requires jar-dependencies 0.3.12. Since jar-dependencies is a default gem, you can either remove your dependency on it or try updating to a newer version of bundler that supports jar-dependencies as a default gem.
@morganchristiansson

This comment has been minimized.

Copy link

commented Feb 27, 2018

The code in this PR works great. I've just deployed it to access an encrypted s3 bucket in a different account.

Further the changes don't really affect existing features or codepaths. So as others in this discussion confirm. LGTM!

PS. Unrelated to this issue but related to my issues - this PR is ready to merge but nobody is available to review and release it. logstash-plugins/logstash-codec-cloudtrail#18

@frezbo

This comment has been minimized.

Copy link

commented Feb 27, 2018

ping @jordansissel
thanks for verifying @morganchristiansson

@morganchristiansson

This comment has been minimized.

Copy link

commented Mar 12, 2018

There's merge conflicts preventing this from being merged. Can someone fix or do we need a new branch to merge from? @patrobinson

@m6a-UdS

This comment has been minimized.

Copy link

commented Mar 16, 2018

Following this thread with great interest This feature will become more and more needed as AWS usage matures to working with roles instead of static secrets. I am blocked in a similar situation, I will see if I can test with this branch on my side.

@morganchristiansson

This comment has been minimized.

Copy link

commented Mar 16, 2018

I hotfixed using this in my Dockerfile @m6a-UdS

# hotfix from https://github.com/logstash-plugins/logstash-mixin-aws/pull/27
COPY v1.rb vendor/bundle/jruby/2.3.0/gems/logstash-mixin-aws-4.2.3/lib/logstash/plugin_mixins/aws_config/v1.rb
COPY v2.rb vendor/bundle/jruby/2.3.0/gems/logstash-mixin-aws-4.2.3/lib/logstash/plugin_mixins/aws_config/v2.rb
COPY generic.rb vendor/bundle/jruby/2.3.0/gems/logstash-mixin-aws-4.2.3/lib/logstash/plugin_mixins/aws_config/generic.rb
@m6a-UdS

This comment has been minimized.

Copy link

commented Mar 16, 2018

The hotfix works! :-)

@morganchristiansson

This comment has been minimized.

Copy link

commented Mar 17, 2018

@jsvd jsvd self-assigned this Mar 17, 2018

@jsvd jsvd added the enhancement label Mar 17, 2018

@patrobinson

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2018

I've fixed the merge conflict now

@jsvd

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2018

rebased, squashed and merged in b98805e, thank you all for the code, testing and patience.

The mixin gem will be released soon together with documentation on the elastic.co site

@jsvd jsvd closed this Mar 29, 2018

@morganchristiansson

This comment has been minimized.

Copy link

commented Apr 1, 2018

4.3.0 was released march 29th. https://rubygems.org/gems/logstash-mixin-aws/versions/4.3.0 when @jsvd commented. Thanks all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.