Use separate http settings config object

We want to fall back to the legacy config if we find any empty values in
the http_settings. This is to guard against a partially empty config
value getting passed from gitaly, which has empty fields in the http
settings. This could happen as we transition from using the legacy shell
config yml to the gitaly config toml.

5 files changed, 74 insertions, 22 deletions

diff --git a/changelogs/unreleased/jc-fetch-http-settings-separately.yml b/changelogs/unreleased/jc-fetch-http-settings-separately.yml
new file mode 100644
index 000000000..9029c7bfb
--- /dev/null
+++ b/changelogs/unreleased/jc-fetch-http-settings-separately.yml
@@ -0,0 +1,5 @@
+---
+title: Use separate http settings config object
+merge_request: 2104
+author:
+type: changed
diff --git a/ruby/gitlab-shell/lib/gitlab_config.rb b/ruby/gitlab-shell/lib/gitlab_config.rb
index 9020ff7f9..b874c42e2 100644
--- a/ruby/gitlab-shell/lib/gitlab_config.rb
+++ b/ruby/gitlab-shell/lib/gitlab_config.rb
@@ -16,8 +16,62 @@ class GitlabConfig
     fetch_from_config('gitlab_url', fetch_from_legacy_config('gitlab_url',"http://localhost:8080").sub(%r{/*$}, ''))
   end
 
+  class HTTPSettings
+    DEFAULT_TIMEOUT = 300
+
+    attr_reader :settings
+    attr_reader :legacy_settings
+
+    def initialize(settings, legacy_settings = {})
+      @settings = settings || {}
+      @legacy_settings = legacy_settings || {}
+    end
+
+    def user
+      fetch_from_settings('user')
+    end
+
+    def password
+      fetch_from_settings('password')
+    end
+
+    def read_timeout
+      read_timeout = fetch_from_settings('read_timeout').to_i
+
+      return read_timeout unless read_timeout == 0
+
+      DEFAULT_TIMEOUT
+    end
+
+    def ca_file
+      fetch_from_settings('ca_file')
+    end
+
+    def ca_path
+      fetch_from_settings('ca_path')
+    end
+
+    def self_signed_cert
+      fetch_from_settings('self_signed_cert')
+    end
+
+    private
+
+    def fetch_from_settings(key)
+      value = settings[key]
+
+      return value if [true,false].include? value
+
+      return legacy_settings[key] if value.nil? || value.empty?
+
+      value
+    end
+  end
+
   def http_settings
-    fetch_from_config('http_settings', fetch_from_legacy_config('http_settings', {}))
+    @http_settings ||= GitlabConfig::HTTPSettings.new(
+                        fetch_from_config('http_settings', {}),
+                        fetch_from_legacy_config('http_settings', {}))
   end
 
   def log_file
diff --git a/ruby/gitlab-shell/lib/http_helper.rb b/ruby/gitlab-shell/lib/http_helper.rb
index c6a4bb845..deea8863a 100644
--- a/ruby/gitlab-shell/lib/http_helper.rb
+++ b/ruby/gitlab-shell/lib/http_helper.rb
@@ -3,7 +3,6 @@ require_relative 'gitlab_logger'
 require_relative 'gitlab_net/errors'
 
 module HTTPHelper
-  READ_TIMEOUT = 300
   CONTENT_TYPE_JSON = 'application/json'.freeze
 
   protected
@@ -32,7 +31,7 @@ module HTTPHelper
     if uri.is_a?(URI::HTTPS)
       http.use_ssl = true
       http.cert_store = cert_store
-      http.verify_mode = OpenSSL::SSL::VERIFY_NONE if config.http_settings['self_signed_cert']
+      http.verify_mode = OpenSSL::SSL::VERIFY_NONE if config.http_settings.self_signed_cert
     end
 
     http
@@ -42,8 +41,8 @@ module HTTPHelper
     request_klass = method == :get ? Net::HTTP::Get : Net::HTTP::Post
     request = request_klass.new(uri.request_uri, headers)
 
-    user = config.http_settings['user']
-    password = config.http_settings['password']
+    user = config.http_settings.user
+    password = config.http_settings.password
     request.basic_auth(user, password) if user && password
 
     if options[:json]
@@ -105,10 +104,10 @@ module HTTPHelper
       store = OpenSSL::X509::Store.new
       store.set_default_paths
 
-      ca_file = config.http_settings['ca_file']
+      ca_file = config.http_settings.ca_file
       store.add_file(ca_file) if ca_file
 
-      ca_path = config.http_settings['ca_path']
+      ca_path = config.http_settings.ca_path
       store.add_path(ca_path) if ca_path
 
       store
@@ -120,6 +119,6 @@ module HTTPHelper
   end
 
   def read_timeout
-    config.http_settings['read_timeout'] || READ_TIMEOUT
+    config.http_settings.read_timeout
   end
 end
diff --git a/ruby/gitlab-shell/spec/gitlab_config_spec.rb b/ruby/gitlab-shell/spec/gitlab_config_spec.rb
index b0f12381c..9ceefbfa8 100644
--- a/ruby/gitlab-shell/spec/gitlab_config_spec.rb
+++ b/ruby/gitlab-shell/spec/gitlab_config_spec.rb
@@ -82,7 +82,7 @@ describe GitlabConfig do
 
     describe '#http_settings' do
       it 'returns the correct http_settings' do
-        expect(config.http_settings).to eq(config_data[:http_settings].transform_keys(&:to_s))
+        expect(config.http_settings.settings).to eq(config_data[:http_settings].transform_keys(&:to_s))
       end
     end
 
diff --git a/ruby/gitlab-shell/spec/gitlab_net_spec.rb b/ruby/gitlab-shell/spec/gitlab_net_spec.rb
index f8161e222..30771c087 100644
--- a/ruby/gitlab-shell/spec/gitlab_net_spec.rb
+++ b/ruby/gitlab-shell/spec/gitlab_net_spec.rb
@@ -264,7 +264,7 @@ describe GitlabNet, vcr: true do
 
     before do
       allow(gitlab_net).to receive :cert_store
-      allow(gitlab_net.send(:config)).to receive(:http_settings) { {'self_signed_cert' => true} }
+      allow(gitlab_net.send(:config)).to receive(:http_settings) { GitlabConfig::HTTPSettings.new({'self_signed_cert' => true}) }
     end
 
     it { expect(subject.verify_mode).to eq(OpenSSL::SSL::VERIFY_NONE) }
@@ -284,8 +284,7 @@ describe GitlabNet, vcr: true do
         subject { gitlab_net.send :http_request_for, :get, url }
 
         before do
-          allow(gitlab_net.send(:config).http_settings).to receive(:[]).with('user') { user }
-          allow(gitlab_net.send(:config).http_settings).to receive(:[]).with('password') { password }
+          allow(gitlab_net.send(:config)).to receive(:http_settings) { GitlabConfig::HTTPSettings.new({'user' => user, 'password' => password}) }
           expect(Net::HTTP::Get).to receive(:new).with('/', {}).and_return(get)
           expect(get).to receive(:basic_auth).with(user, password).once
           expect(get).to receive(:set_form_data).with(hash_including(secret_token: secret)).once
@@ -298,8 +297,7 @@ describe GitlabNet, vcr: true do
         subject { gitlab_net.send :http_request_for, :get, url, params: params, headers: headers }
 
         before do
-          allow(gitlab_net.send(:config).http_settings).to receive(:[]).with('user') { user }
-          allow(gitlab_net.send(:config).http_settings).to receive(:[]).with('password') { password }
+          allow(gitlab_net.send(:config)).to receive(:http_settings) { GitlabConfig::HTTPSettings.new({'user' => user, 'password' => password}) }
           expect(Net::HTTP::Get).to receive(:new).with('/', headers).and_return(get)
           expect(get).to receive(:basic_auth).with(user, password).once
           expect(get).to receive(:set_form_data).with({ 'key1' => 'value1', secret_token: secret }).once
@@ -312,8 +310,7 @@ describe GitlabNet, vcr: true do
         subject { gitlab_net.send :http_request_for, :get, url, headers: headers }
 
         before do
-          allow(gitlab_net.send(:config).http_settings).to receive(:[]).with('user') { user }
-          allow(gitlab_net.send(:config).http_settings).to receive(:[]).with('password') { password }
+          allow(gitlab_net.send(:config)).to receive(:http_settings) { GitlabConfig::HTTPSettings.new({'user' => user, 'password' => password}) }
           expect(Net::HTTP::Get).to receive(:new).with('/', headers).and_return(get)
           expect(get).to receive(:basic_auth).with(user, password).once
           expect(get).to receive(:set_form_data).with(hash_including(secret_token: secret)).once
@@ -327,8 +324,7 @@ describe GitlabNet, vcr: true do
           subject { gitlab_net.send :http_request_for, :get, url, options: options }
 
           before do
-            allow(gitlab_net.send(:config).http_settings).to receive(:[]).with('user') { user }
-            allow(gitlab_net.send(:config).http_settings).to receive(:[]).with('password') { password }
+            allow(gitlab_net.send(:config)).to receive(:http_settings) { GitlabConfig::HTTPSettings.new({'user' => user, 'password' => password}) }
             expect(Net::HTTP::Get).to receive(:new).with('/', {}).and_return(get)
             expect(get).to receive(:basic_auth).with(user, password).once
             expect(get).to receive(:body=).with({ 'key2' => 'value2', secret_token: secret }.to_json).once
@@ -368,15 +364,13 @@ describe GitlabNet, vcr: true do
     end
 
     it "calls add_file with http_settings['ca_file']" do
-      allow(gitlab_net.send(:config).http_settings).to receive(:[]).with('ca_file') { 'test_file' }
-      allow(gitlab_net.send(:config).http_settings).to receive(:[]).with('ca_path') { nil }
+      allow(gitlab_net.send(:config)).to receive(:http_settings) { GitlabConfig::HTTPSettings.new({'ca_file' => 'test_file', 'ca_path' => nil}) }
       expect(store).to receive(:add_file).with('test_file')
       expect(store).not_to receive(:add_path)
     end
 
     it "calls add_path with http_settings['ca_path']" do
-      allow(gitlab_net.send(:config).http_settings).to receive(:[]).with('ca_file') { nil }
-      allow(gitlab_net.send(:config).http_settings).to receive(:[]).with('ca_path') { 'test_path' }
+      allow(gitlab_net.send(:config)).to receive(:http_settings) { GitlabConfig::HTTPSettings.new({'ca_file' => nil, 'ca_path' => 'test_path'}) }
       expect(store).not_to receive(:add_file)
       expect(store).to receive(:add_path).with('test_path')
     end