Unify elasticsearch config handling

app/models/application_setting.rb

@@ -273,6 +273,16 @@ class ApplicationSetting < ActiveRecord::Base
     read_attribute(:elasticsearch_url).split(',').map(&:strip)
   end
 
+  def elasticsearch_config
+    {
+      url:                   elasticsearch_url,
+      aws:                   elasticsearch_aws,
+      aws_access_key:        elasticsearch_aws_access_key,
+      aws_secret_access_key: elasticsearch_aws_secret_access_key,
+      aws_region:            elasticsearch_aws_region,
+    }
+  end
+
   def home_page_url_column_exist
     ActiveRecord::Base.connection.column_exists?(:application_settings, :home_page_url)
   end

bin/elastic_repo_indexer

@@ -19,27 +19,20 @@ FROM_SHA = ENV['FROM_SHA']
 TO_SHA = ENV['TO_SHA']
 RAILS_ENV = ENV['RAILS_ENV']
 
-LOGGER.info("Has been scheduled for project #{REPO_PATH} with SHA range #{FROM_SHA}:#{TO_SHA}")
+# Symbols get stringified when passed through JSON
+elastic = {}
+JSON.parse(ENV['ELASTIC_CONNECTION_INFO']).each { |k, v| elastic[k.to_sym] = v }
+ELASTIC_CONFIG = elastic
 
-elastic_connection_info = JSON.parse ENV['ELASTIC_CONNECTION_INFO']
-ELASTIC_URL = elastic_connection_info['url']
-ELASTIC_AWS = elastic_connection_info['aws']
-ELASTIC_AWS_ACCESS_KEY = elastic_connection_info['aws_access_key']
-ELASTIC_AWS_SECRET_ACCESS_KEY = elastic_connection_info['aws_secret_access_key']
-ELASTIC_AWS_REGION = elastic_connection_info['aws_region']
+LOGGER.info("Has been scheduled for project #{REPO_PATH} with SHA range #{FROM_SHA}:#{TO_SHA}")
 
 class Repository
   include Elasticsearch::Git::Repository
-  include Gitlab::Elastic
 
   index_name ['gitlab', RAILS_ENV].compact.join('-')
 
   def initialize
-    if ELASTIC_AWS
-      self.__elasticsearch__.client = AWSClient(ELASTIC_URL, ELASTIC_REGION, ELASTIC_AWS_ACCESS_KEY, ELASTIC_AWS_SECRET_ACCESS_KEY).client
-    else
-      self.__elasticsearch__.client = BaseClient.new(ELASTIC_URL).client
-    end
+    self.__elasticsearch__.client = ::Gitlab::Elastic::Client.build(ELASTIC_CONFIG)
   end
 
   def client_for_indexing

config/initializers/elastic_client_setup.rb

@@ -7,28 +7,18 @@ module Elasticsearch
     module Client
       module ClassMethods
         include Gitlab::CurrentSettings
-        include Gitlab::Elastic
+
         def client(client = nil)
           if @client.nil? || es_configuration_changed?
-            @es_url = current_application_settings.elasticsearch_url
-            @es_aws = current_application_settings.elasticsearch_aws
-
-            if @es_aws
-              # AWS specific handling
-              @es_region = current_application_settings.elasticsearch_aws_region
-              @es_access_key = current_application_settings.elasticsearch_aws_access_key
-              @es_secret_access_key = current_application_settings.elasticsearch_aws_secret_access_key
-              @client = AWSClient.new(@es_url, @es_region, @es_access_key, @es_secret_access_key).client
-            else
-              @client = BaseClient.new(@es_url).client
-            end
+            @es_config = current_application_settings.elasticsearch_config
+            @client = ::Gitlab::Elastic::Client.build(@es_config)
           end
+
           @client
         end
 
         def es_configuration_changed?
-          @es_url != current_application_settings.elasticsearch_url ||
-            @es_aws != current_application_settings.elasticsearch_aws
+          @es_config != current_application_settings.elasticsearch_config
         end
       end
     end

lib/gitlab/elastic/client.rb

@@ -4,37 +4,21 @@ require 'faraday_middleware/aws_signers_v4'
 
 module Gitlab
   module Elastic
-    class BaseClient
-      attr_accessor :client
+    module Client
+      # Takes a hash as returned by `ApplicationSetting#elasticsearch_config`,
+      # and configures itself based on those parameters
+      def self.build(config)
+        base_config = { urls: config[:url] }
 
-      def initialize(urls)
-        @urls = urls
-        @client = Elasticsearch::Client.new urls: @urls
-      end
-    end
-  end
-end
+        if config[:aws]
+          creds = Aws::Credentials.new(config[:aws_access_key], config[:aws_secret_access_key])
+          region = config[:aws_region]
 
-module Gitlab
-  module Elastic
-    class AWSClient < BaseClient
-      def initialize(urls, region, access_key = nil, secret_access_key = nil)
-        @urls = urls
-        @region = region
-        @access_key = access_key
-        @secret_access_key = secret_access_key
-
-        if @access_key.nil? || @secret_access_key.nil?
-          @credentials = Aws::Credentials.new()
+          ::Elasticsearch::Client.new(base_config) do |fmid|
+            fmid.request(:aws_signers_v4, credentials: creds, service_name: 'es', region: region)
+          end
         else
-          @credentials = Aws::Credentials.new(@access_key, @secret_access_key)
-        end
-
-        @client = Elasticsearch::Client.new urls: @urls do |fmid|
-          fmid.request :aws_signers_v4,
-            credentials: @credentials,
-            service_name: 'es',
-            region: @region
+          ::Elasticsearch::Client.new(base_config)
         end
       end
     end

lib/gitlab/elastic/indexer.rb

@@ -13,18 +13,10 @@ module Gitlab
       def initialize(project)
         @project = project
 
-        connection_info = {
-          url: current_application_settings.elasticsearch_url,
-          aws: current_application_settings.elasticsearch_aws,
-          aws_access_key: current_application_settings.elasticsearch_aws_access_key,
-          aws_secret_access_key: current_application_settings.elasticsearch_aws_secret_access_key,
-          aws_region: current_application_settings.elasticsearch_aws_region
-        }.to_json
-
         # We accept any form of settings, including string and array
         # This is why JSON is needed
         @vars = {
-          'ELASTIC_CONNECTION_INFO' => connection_info,
+          'ELASTIC_CONNECTION_INFO' => current_application_settings.elasticsearch_config.to_json,
           'RAILS_ENV'               => Rails.env
         }
       end

lib/tasks/gitlab/check.rake

@@ -1072,16 +1072,7 @@ namespace :gitlab do
   end
 
   def check_elasticsearch
-    if ApplicationSetting.current.elasticsearch_aws
-      client = Gitlab::Elastic::AWSClient.new(
-        ApplicationSetting.current.elasticsearch_url,
-        ApplicationSetting.current.elasticsearch_aws_region,
-        ApplicationSetting.current.elasticsearch_aws_access_key,
-        ApplicationSetting.current.elasticsearch_aws_secret_access_key
-      ).client
-    else
-      client = Gitlab::Elastic::BaseClient.new(ApplicationSetting.current.elasticsearch_url).client
-    end
+    client = Gitlab::Elastic::Client.build(ApplicationSetting.current.elasticsearch_config)
 
     print "Elasticsearch version 5.1.x? ... "
 

spec/lib/gitlab/elastic/indexer_spec.rb

@@ -5,7 +5,7 @@ describe Gitlab::Elastic::Indexer do
 
   before do
     stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'true')
-    stub_application_setting(es_url: ['http://localhost:9200', 'http://localhost:9201'])
+    stub_application_setting(elasticsearch_url: ['http://localhost:9200'])
   end
 
   let(:project)  { create(:project) }
@@ -16,12 +16,6 @@ describe Gitlab::Elastic::Indexer do
   let(:popen_success) { [[''], 0] }
   let(:popen_failure) { [['error'], 1] }
 
-  let(:elastic_connection_info) do
-    {
-      url: current_application_settings.elasticsearch_url
-    }
-  end
-
   context 'empty project' do
     let(:project) { create(:empty_project) }
 
@@ -57,7 +51,7 @@ describe Gitlab::Elastic::Indexer do
         ],
         nil,
         hash_including(
-          'ELASTIC_CONNECTION_INFO' => elastic_connection_info.to_json,
+          'ELASTIC_CONNECTION_INFO' => current_application_settings.elasticsearch_config.to_json,
           'RAILS_ENV'               => Rails.env,
           'FROM_SHA'                => from_sha,
           'TO_SHA'                  => to_sha

spec/models/application_setting_spec.rb

@@ -294,4 +294,44 @@ describe ApplicationSetting, models: true do
       expect(setting.repository_size_limit).to eql(8.exabytes - 1)
     end
   end
+
+  describe '#elasticsearch_url' do
+    it 'presents a single URL as a one-element array' do
+      setting.elasticsearch_url = 'http://example.com'
+
+      expect(setting.elasticsearch_url).to eq(%w[http://example.com])
+    end
+
+    it 'presents multiple URLs as a many-element array' do
+      setting.elasticsearch_url = 'http://example.com,https://invalid.invalid:9200'
+
+      expect(setting.elasticsearch_url).to eq(%w[http://example.com https://invalid.invalid:9200])
+    end
+
+    it 'strips whitespace from around URLs' do
+      setting.elasticsearch_url = ' http://example.com, https://invalid.invalid:9200 '
+
+      expect(setting.elasticsearch_url).to eq(%w[http://example.com https://invalid.invalid:9200])
+    end
+  end
+
+  describe '#elasticsearch_config' do
+    it 'places all elasticsearch configuration values into a hash' do
+      setting.update!(
+        elasticsearch_url: 'http://example.com:9200',
+        elasticsearch_aws: false,
+        elasticsearch_aws_region:     'test-region',
+        elasticsearch_aws_access_key: 'test-access-key',
+        elasticsearch_aws_secret_access_key: 'test-secret-access-key'
+      )
+
+      expect(setting.elasticsearch_config).to eq(
+        url: ['http://example.com:9200'],
+        aws: false,
+        aws_region:     'test-region',
+        aws_access_key: 'test-access-key',
+        aws_secret_access_key: 'test-secret-access-key'
+      )
+    end
+  end
 end

spec/workers/elastic_indexer_worker_spec.rb

@@ -5,13 +5,12 @@ describe ElasticIndexerWorker, elastic: true do
   subject { described_class.new }
 
   before do
-    Elasticsearch::Model.client = Elasticsearch::Client.new(
-      url: current_application_settings.elasticsearch_url
-    )
+    stub_application_setting(elasticsearch_indexing: true)
 
-    Gitlab::Elastic::Helper.create_empty_index
+    Elasticsearch::Model.client =
+      Gitlab::Elastic::Client.build(current_application_settings.elasticsearch_config)
 
-    stub_application_setting(elasticsearch_indexing: true)
+    Gitlab::Elastic::Helper.create_empty_index
   end
 
   after do