Followups on review

4 files changed, 40 insertions, 45 deletions

diff --git a/changelogs/unreleased/support-gitaly-tls.yml b/changelogs/unreleased/support-gitaly-tls.yml
index c564f5b7ca0..2a15500d6da 100644
--- a/changelogs/unreleased/support-gitaly-tls.yml
+++ b/changelogs/unreleased/support-gitaly-tls.yml
@@ -2,4 +2,4 @@
 title: Support tls communication in gitaly
 merge_request: 22602
 author:
-type: changed
+type: added
diff --git a/doc/administration/gitaly/index.md b/doc/administration/gitaly/index.md
index 444e2955d90..bcb6a11cd85 100644
--- a/doc/administration/gitaly/index.md
+++ b/doc/administration/gitaly/index.md
@@ -228,8 +228,8 @@ Omnibus installations:
 ```ruby
 # /etc/gitlab/gitlab.rb
 git_data_dirs({
-  'default' => { 'path' => '/mnt/gitlab/default', 'gitaly_address' => 'tls://gitaly.internal:8075' },
-  'storage1' => { 'path' => '/mnt/gitlab/storage1', 'gitaly_address' => 'tls://gitaly.internal:8075' },
+  'default' => { 'path' => '/mnt/gitlab/default', 'gitaly_address' => 'tls://gitaly.internal:9999' },
+  'storage1' => { 'path' => '/mnt/gitlab/storage1', 'gitaly_address' => 'tls://gitaly.internal:9999' },
 })
 
 gitlab_rails['gitaly_token'] = 'abc123secret'
@@ -244,10 +244,10 @@ gitlab:
     storages:
       default:
         path: /mnt/gitlab/default/repositories
-        gitaly_address: tls://gitaly.internal:8075
+        gitaly_address: tls://gitaly.internal:9999
       storage1:
         path: /mnt/gitlab/storage1/repositories
-        gitaly_address: tls://gitaly.internal:8075
+        gitaly_address: tls://gitaly.internal:9999
 
   gitaly:
     token: 'abc123secret'
diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb
index 2f34c984e15..d54d40c08fb 100644
--- a/lib/gitlab/gitaly_client.rb
+++ b/lib/gitlab/gitaly_client.rb
@@ -26,7 +26,7 @@ module Gitlab
       end
     end
 
-    PEM_REXP = /[-]+BEGIN CERTIFICATE[-]+.+?[-]+END CERTIFICATE[-]+/m
+    PEM_REGEX = /\-+BEGIN CERTIFICATE\-+.+?\-+END CERTIFICATE\-+/m
     SERVER_VERSION_FILE = 'GITALY_SERVER_VERSION'
     MAXIMUM_GITALY_CALLS = 35
     CLIENT_NAME = (Sidekiq.server? ? 'gitlab-sidekiq' : 'gitlab-web').freeze
@@ -57,29 +57,27 @@ module Gitlab
       end
     end
 
-    def self.certs
+    def self.stub_certs
       return @certs if @certs
 
       cert_paths = Dir["#{OpenSSL::X509::DEFAULT_CERT_DIR}/*"]
       cert_paths << OpenSSL::X509::DEFAULT_CERT_FILE if File.exist? OpenSSL::X509::DEFAULT_CERT_FILE
 
-      @certs = []
-      cert_paths.each do |cert_file|
-        begin
-          File.read(cert_file).scan(PEM_REXP).each do |cert|
-            pem = OpenSSL::X509::Certificate.new(cert).to_pem
-            @certs << pem
+      @certs = cert_paths.flat_map do |cert_file|
+        File.read(cert_file).scan(PEM_REGEX).map do |cert|
+          begin
+            OpenSSL::X509::Certificate.new(cert).to_pem
+          rescue OpenSSL::OpenSSLError => e
+            Rails.logger.error "Could not load certificate #{cert_file} #{e}"
+            nil
           end
-        rescue StandardError => e
-          Rails.logger.error "Could not load certificate #{e}"
-        end
-      end
-      @certs = @certs.uniq.join "\n"
+        end.compact
+      end.uniq.join("\n")
     end
 
     def self.stub_creds(storage)
       if URI(address(storage)).scheme == 'tls'
-        GRPC::Core::ChannelCredentials.new certs
+        GRPC::Core::ChannelCredentials.new stub_certs
       else
         :this_channel_is_insecure
       end
@@ -94,9 +92,7 @@ module Gitlab
     end
 
     def self.stub_address(storage)
-      addr = address(storage)
-      addr = addr.sub(%r{^tcp://|^tls://}, '') if %w(tcp tls).include? URI(addr).scheme
-      addr
+      address(storage).sub(%r{^tcp://|^tls://}, '')
     end
 
     def self.clear_stubs!
diff --git a/spec/lib/gitlab/gitaly_client_spec.rb b/spec/lib/gitlab/gitaly_client_spec.rb
index 36c9e9a72e9..2501e855697 100644
--- a/spec/lib/gitlab/gitaly_client_spec.rb
+++ b/spec/lib/gitlab/gitaly_client_spec.rb
@@ -3,6 +3,12 @@ require 'spec_helper'
 # We stub Gitaly in `spec/support/gitaly.rb` for other tests. We don't want
 # those stubs while testing the GitalyClient itself.
 describe Gitlab::GitalyClient do
+  def stub_repos_storages(address)
+    allow(Gitlab.config.repositories).to receive(:storages).and_return({
+      'default' => { 'gitaly_address' => address }
+    })
+  end
+
   describe '.stub_class' do
     it 'returns the gRPC health check stub' do
       expect(described_class.stub_class(:health_check)).to eq(::Grpc::Health::V1::Health::Stub)
@@ -15,12 +21,8 @@ describe Gitlab::GitalyClient do
 
   describe '.stub_address' do
     it 'returns the same result after being called multiple times' do
-      address = 'localhost:9876'
-      prefixed_address = "tcp://#{address}"
-
-      allow(Gitlab.config.repositories).to receive(:storages).and_return({
-        'default' => { 'gitaly_address' => prefixed_address }
-      })
+      address = 'tcp://localhost:9876'
+      stub_repos_storages address
 
       2.times do
         expect(described_class.stub_address('default')).to eq('localhost:9876')
@@ -29,19 +31,24 @@ describe Gitlab::GitalyClient do
   end
 
   describe '.stub_creds' do
+    it 'returns :this_channel_is_insecure if unix' do
+      address = 'unix:/tmp/gitaly.sock'
+      stub_repos_storages address
+
+      expect(described_class.stub_creds('default')).to eq(:this_channel_is_insecure)
+    end
+
     it 'returns :this_channel_is_insecure if tcp' do
       address = 'tcp://localhost:9876'
-      allow(Gitlab.config.repositories).to receive(:storages).and_return({
-        'default' => { 'gitaly_address' => address }
-      })
+      stub_repos_storages address
+
       expect(described_class.stub_creds('default')).to eq(:this_channel_is_insecure)
     end
 
     it 'returns Credentials object if tls' do
       address = 'tls://localhost:9876'
-      allow(Gitlab.config.repositories).to receive(:storages).and_return({
-        'default' => { 'gitaly_address' => address }
-      })
+      stub_repos_storages address
+
       expect(described_class.stub_creds('default')).to be_a(GRPC::Core::ChannelCredentials)
     end
   end
@@ -55,9 +62,7 @@ describe Gitlab::GitalyClient do
     context 'when passed a UNIX socket address' do
       it 'passes the address as-is to GRPC' do
         address = 'unix:/tmp/gitaly.sock'
-        allow(Gitlab.config.repositories).to receive(:storages).and_return({
-          'default' => { 'gitaly_address' => address }
-        })
+        stub_repos_storages address
 
         expect(Gitaly::CommitService::Stub).to receive(:new).with(address, any_args)
 
@@ -69,10 +74,7 @@ describe Gitlab::GitalyClient do
       it 'strips tls:// prefix before passing it to GRPC::Core::Channel initializer' do
         address = 'localhost:9876'
         prefixed_address = "tls://#{address}"
-
-        allow(Gitlab.config.repositories).to receive(:storages).and_return({
-          'default' => { 'gitaly_address' => prefixed_address }
-        })
+        stub_repos_storages prefixed_address
 
         expect(Gitaly::CommitService::Stub).to receive(:new).with(address, any_args)
 
@@ -84,10 +86,7 @@ describe Gitlab::GitalyClient do
       it 'strips tcp:// prefix before passing it to GRPC::Core::Channel initializer' do
         address = 'localhost:9876'
         prefixed_address = "tcp://#{address}"
-
-        allow(Gitlab.config.repositories).to receive(:storages).and_return({
-          'default' => { 'gitaly_address' => prefixed_address }
-        })
+        stub_repos_storages prefixed_address
 
         expect(Gitaly::CommitService::Stub).to receive(:new).with(address, any_args)