diff options
author | Ahmad Hassan <ahmad.hassan612@gmail.com> | 2018-12-19 13:36:06 +0300 |
---|---|---|
committer | Ahmad Hassan <ahmad.hassan612@gmail.com> | 2018-12-19 16:19:43 +0300 |
commit | 32c4f70aa585f58619c32d6c8e9db44191d82bfb (patch) | |
tree | 43e1e18847e16539e9470ac91bb02b3aeb98e5fe | |
parent | c1ed498f3ba0a241e064cdd56074924407203417 (diff) |
Followups on review
-rw-r--r-- | changelogs/unreleased/support-gitaly-tls.yml | 2 | ||||
-rw-r--r-- | doc/administration/gitaly/index.md | 8 | ||||
-rw-r--r-- | lib/gitlab/gitaly_client.rb | 30 | ||||
-rw-r--r-- | spec/lib/gitlab/gitaly_client_spec.rb | 45 |
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) |