Skip to content
Snippets Groups Projects

AWS credentials chain

Merged Matt Gresko requested to merge mgresko/gitlab-ee:aws_credentials_chain into master
All threads resolved!

What does this MR do?

Adds support for EC2 instance profile credentials when using AWS hosted elasticsearch clusters

Are there points in the code the reviewer needs to double check?

Why was this MR needed?

Could only use static IAM credentials to access AWS hosted elasticsearch cluster

Screenshots (if relevant)

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

Closes #2169 (closed)

Edited by Coung Ngo

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Nick Thomas
  • Thanks @mgresko, a few comments.

    I think it's possible - and perhaps even desirable - to do all the credentials resolution here and so not need the Go MR at all! Happy to hear your thoughts on that approach.

  • Author Contributor

    Thanks for the review @nick.thomas as mentioned inline instance credentials are auto rotated by AWS so rails and indexer need to handle expiring of credentials individually or we will end up with a situation where the indexer is trying to index with expired creds.

  • How long are the credentials expected to last for? The indexer is short-lived, so that shouldn't be a problem?

  • Author Contributor

    Looking on an instance it appears to be around 6 hours and I believe there is a 5 minute window after new credentials are issued that the old still work. So if it can guaranteed that all indexing requests would finish in <5min that is relatively safe. However, I am betting somewhere someone has a huge repository that will cause the indexing to take longer than 5minutes :)

  • Matt Gresko added 1 commit

    added 1 commit

    Compare with previous version

  • Author Contributor

    @nick.thomas is there a method for me to build this into an RPM (like it is distributed from gitlab)? Was looking for an easy way to load it into one of our test servers to run for a while and make sure works properly.

  • Matt Gresko added 1 commit

    added 1 commit

    Compare with previous version

  • Author Contributor

    @nick.thomas thanks for the info. I was able to create an omnibus package and been running this and the updated go elasticsearch indexer code all day without issue. Credentials for the ec2 profile have been rotated and was handled as expected.

  • Author Contributor

    I stand corrected... credentials are not cleanly refreshing, they are cached for the life of the unicorn process.

    Completed 500 Internal Server Error in 98ms (ActiveRecord: 15.8ms | Elasticsearch: 43.5ms)
    
    ActionView::Template::Error ([403] {"message":"The security token included in the request is expired"}):
        20:           = link_to search_filter_path(scope: 'merge_requests') do
        21:             Merge requests
        22:             %span.badge
        23:               = @search_results.merge_requests_count
        24:       - if project_search_tabs?(:milestones)
        25:         %li{ class: active_when(@scope == 'milestones') }
        26:           = link_to search_filter_path(scope: 'milestones') do
      lib/gitlab/elastic/search_results.rb:59:in `merge_requests_count'
      app/views/search/_category.html.haml:23:in `block in _app_views_search__category_html_haml___4557493664271148860_69833467902720'
      app/views/search/_category.html.haml:20:in `_app_views_search__category_html_haml___4557493664271148860_69833467902720'
      app/views/search/show.html.haml:6:in `_app_views_search_show_html_haml___772796762768427179_69833454204780'
      lib/gitlab/i18n.rb:34:in `with_locale'
      lib/gitlab/i18n.rb:40:in `with_user_locale'
      app/controllers/application_controller.rb:291:in `set_locale'
      lib/gitlab/middleware/multipart.rb:93:in `call'
      lib/gitlab/request_profiler/middleware.rb:14:in `call'
      lib/gitlab/middleware/go.rb:16:in `call'
      lib/gitlab/etag_caching/middleware.rb:10:in `call'
      lib/gitlab/middleware/readonly_geo.rb:30:in `call'
      lib/gitlab/request_context.rb:18:in `call'
  • Nick Thomas resolved all discussions

    resolved all discussions

  • Author Contributor

    @nick.thomas do you have any guidance on where to try and catch this and reinitialize the client? I am not familiar with the layers of rails and not seeing a clear path.

  • @mgresko interesting. Here's how a unicorn process ends up with an Elasticsearch::Client - https://gitlab.com/gitlab-org/gitlab-ee/blob/master/config/initializers/elastic_client_setup.rb

    We're performing requests against a single long-lived client instance, but we passed it an Aws::InstanceProfileCredentials - which is meant to dynamically refresh:

    So it should be refreshing automatically when the credentials have < 5 minutes to live, even with a long-lived instance.

    we're using a fairly old version of the aws-sdk gem: 2.7.8, when the latest is 2.9.32. Perhaps try bundle update aws-sdk, and committing the changes to Gemfile.lock ?

    This feels like the sort of thing we could write a test for - we can use travel_to to change the current system time to be within expiry, and look for the client trying to refresh.

  • Matt Gresko added 1 commit

    added 1 commit

    Compare with previous version

  • Author Contributor

    Thanks! I will play with this.

  • Matt Gresko added 1 commit

    added 1 commit

    • 3214e693 - update faraday, fix how credentials are used so refresh is triggered

    Compare with previous version

  • Matt Gresko
  • Nick Thomas resolved all discussions

    resolved all discussions

  • Matt Gresko added 655 commits

    added 655 commits

    Compare with previous version

  • Author Contributor

    Been running this on an environment in aws and lived through a credential rotation without issue. Going to let it go and verify again tomorrow and will update here.

  • Author Contributor

    Still looks good, credentials refreshing properly

  • Nick Thomas resolved all discussions

    resolved all discussions

  • Great!

    @smcgivern what do you think? It should go in concert with gitlab-elasticsearch-indexer!6 (merged)

  • Nick Thomas changed milestone to %9.4

    changed milestone to %9.4

  • @mgresko thanks, this is great! Just some minor comments.

  • assigned to @mgresko

  • Matt Gresko added 1 commit

    added 1 commit

    Compare with previous version

  • Author Contributor

    Thanks for reviewing @smcgivern let me know if anything else needs to be adjusted.

  • assigned to @smcgivern

  • Thanks @mgresko, I just had a couple more nit-picks on the specs. RuboCop is still failing - you can run it locally with bundle exec rubocop.

    The other failures are either timeouts (https://gitlab.com/mgresko/gitlab-ee/builds/18225209, https://gitlab.com/mgresko/gitlab-ee/builds/18225216), or should be fixed in latest master.

  • assigned to @mgresko

  • Matt Gresko added 1 commit

    added 1 commit

    • e5e646af - remove creds responses from top level

    Compare with previous version

  • Author Contributor

    oops

  • Author Contributor

    added fixes, rebased with latest master

  • Matt Gresko added 218 commits

    added 218 commits

    Compare with previous version

  • Matt Gresko added 218 commits

    added 218 commits

    Compare with previous version

  • Matt Gresko added 1 commit

    added 1 commit

    Compare with previous version

  • Matt Gresko resolved all discussions

    resolved all discussions

  • @mgresko thanks, great work! @nick.thomas how do we coordinate this with gitlab-elasticsearch-indexer!6 (merged)?

  • Sean McGivern approved this merge request

    approved this merge request

  • @smcgivern we just need to merge them at the same time.

    I'm a little worried about omnibus-gitlab though. I've just remembered that right now, it will always build against the current master of gitlab-elasticsearch-indexer: omnibus-gitlab!1455 (comment 27720815)

    @marin do we need to introduce versioning for gitlab-elasticsearch-indexer into omnibus before we can merge this?

  • Yes, definitely. We build against master so tagging would be useful.

  • OK, thanks @marin. I'll open an issue against omnibus-gitlab and add a v0.1.0 tag for current master on gitlab-elasticsearch-indexer. We can make gitlab-elasticsearch-indexer!6 (merged) into v0.2.0

  • @smcgivern WDYT to merging this MR now, and gitlab-elasticsearch-indexer!6 (merged) when #2458 is done?

    If we do, and omnibus-gitlab#2458 (closed) is not resolved in time for %9.4, then the optional, experimental indexer won't support the new feature (instance profile credentials).

    The Ruby indexer will work, so the worst-case scenario would be advising users of the new feature to switch off the experimental indexer.

    Edited by Nick Thomas
  • @nick.thomas I'm OK with that.

    @mgresko does that work for you? Also, once this is merged, can we close https://gitlab.com/gitlab-org/gitlab-ee/issues/2169? If so, I'll update the description so it auto-closes :thumbsup:

  • Author Contributor

    Works for me. We can certainly use the ruby indexer until whatever build logic gets worked out for the experimental indexer. That being said, not sure it hurts to merge the experimental indexer. It does not break anything existing only adds. Either way.

    We can auto close #2169 (closed) :thumbsup:

  • Sean McGivern changed the description

    changed the description

  • @mgresko could you retry the failed builds, please? (Or share your fork with gitlab-org, then I can do it.) I don't believe any are related - most are timeouts - but I'd like to make certain :slight_smile:

  • Author Contributor

    @smcgivern I retried the build but it still times out. I also added you on the fork... I couldn't figure out how to share with gitlab-org.

  • Author Contributor

    FYI going to be away from computer for 3 days

  • @mgresko you also need to update your fork's project CI settings to increase the build timeout from 60 minutes to 90 or so.

  • Author Contributor

    @nick.thomas added you to fork, increased timeout to 120 and hit retry. Have to go offline for real now. :) Thanks!

  • Thanks @mgresko, finally green! :sweat_smile:

  • Sean McGivern mentioned in commit 77531aa1

    mentioned in commit 77531aa1

  • merged

  • Author Contributor

    :thumbsup: thanks for all the help

  • mentioned in issue #4334 (closed)

  • Sean McGivern mentioned in issue #3655

    mentioned in issue #3655

  • mentioned in issue #197306 (closed)

  • Coung Ngo changed the description

    changed the description

  • Please register or sign in to reply
    Loading