AWS credentials chain
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?
-
Changelog entry added, if necessary -
Documentation created/updated -
API support added - Tests
-
Added for this feature/bug - [?] All builds are passing - failing on unrelated things
-
-
Conform by the merge request performance guides -
Conform by the style guides -
Branch has no merge conflicts with master
(if it does - rebase it please) -
Squashed related commits together
What are the relevant issue numbers?
Closes #2169 (closed)
Merge request reports
Activity
@nick.thomas FYI
mentioned in merge request gitlab-elasticsearch-indexer!6 (merged)
- Resolved by Matt Gresko
- Resolved by Matt Gresko
- Resolved by Nick Thomas
- Resolved by Matt Gresko
- Resolved by Matt Gresko
- Resolved by Matt Gresko
- Resolved by Matt Gresko
- Resolved by Matt Gresko
- Resolved by Matt Gresko
- Resolved by Matt Gresko
- Resolved by Matt Gresko
- Resolved by Matt Gresko
- Resolved by Matt Gresko
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.
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.
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 :)
@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.
@mgresko we use https://gitlab.com/gitlab-org/omnibus-gitlab to generate packages - it has a "build your own package" section here: https://gitlab.com/gitlab-org/omnibus-gitlab/blob/master/doc/build/README.md
@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.
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 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:- https://github.com/aws/aws-sdk-ruby/blob/master/aws-sdk-core/lib/aws-sdk-core/instance_profile_credentials.rb
- https://github.com/aws/aws-sdk-ruby/blob/master/aws-sdk-core/lib/aws-sdk-core/refreshing_credentials.rb
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 trybundle update aws-sdk
, and committing the changes toGemfile.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.added 1 commit
- 3214e693 - update faraday, fix how credentials are used so refresh is triggered
- Resolved by Matt Gresko
- Resolved by Matt Gresko
@mgresko also, can we rebase this against current master, or merge master into it, so that it's more up-to-date? This reduces the chance of us accidentally merging something that fails against the latest static analysis checks, etc.
Edited by Nick Thomas
- Resolved by Matt Gresko
- Resolved by Matt Gresko
- Resolved by Nick Thomas
- Resolved by Matt Gresko
- Resolved by Matt Gresko
added 655 commits
-
3214e693...40588ddd - 654 commits from branch
gitlab-org:master
- fdee4732 - AWS credentials chain
-
3214e693...40588ddd - 654 commits from branch
Great!
@smcgivern what do you think? It should go in concert with gitlab-elasticsearch-indexer!6 (merged)
/cc @vsizov
changed milestone to %9.4
- Resolved by Matt Gresko
- Resolved by Matt Gresko
- Resolved by Matt Gresko
- Resolved by Matt Gresko
- Resolved by Matt Gresko
@mgresko thanks, this is great! Just some minor comments.
assigned to @mgresko
Thanks for reviewing @smcgivern let me know if anything else needs to be adjusted.
assigned to @smcgivern
- Resolved by Matt Gresko
- Resolved by Matt Gresko
- Resolved by Matt Gresko
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
added 218 commits
-
e5e646af...fe6a0fbe - 217 commits from branch
gitlab-org:master
- 880c457e - AWS credentials chain
-
e5e646af...fe6a0fbe - 217 commits from branch
added 218 commits
-
e5e646af...fe6a0fbe - 217 commits from branch
gitlab-org:master
- 880c457e - AWS credentials chain
-
e5e646af...fe6a0fbe - 217 commits from branch
assigned to @smcgivern
@mgresko thanks, great work! @nick.thomas how do we coordinate this with gitlab-elasticsearch-indexer!6 (merged)?
@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?
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
mentioned in issue omnibus-gitlab#2458 (closed)
@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
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)
@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@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
.@mgresko you also need to update your fork's project CI settings to increase the build timeout from 60 minutes to 90 or so.
@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!
mentioned in commit 77531aa1
added Community contribution label
mentioned in issue #4334 (closed)
added devopsplan label
added Enterprise Edition label
mentioned in issue #3655
mentioned in issue #197306 (closed)
removed Plan [DEPRECATED] label