Use Redis Ruby client instead of shelling out to redis-cli

Closes gitlab-org/gitlab-ce#17329

10 files changed, 85 insertions, 79 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 63b04cc..2b41c80 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -3,6 +3,7 @@ v3.0.0
   - Remove create-branch and rm-branch commands (Robert Schilling)
   - Update PostReceive worker so it logs a unique JID in Sidekiq
   - Remove update-head command
+  - Use Redis Ruby client instead of shelling out to redis-cli
 
 v2.7.2
   - Do not prune objects during 'git gc'
diff --git a/Gemfile b/Gemfile
index 7c5ce9d..4fed1a1 100644
--- a/Gemfile
+++ b/Gemfile
@@ -1,5 +1,7 @@
 source "http://rubygems.org"
 
+gem 'redis', '~> 3.3'
+
 group :development, :test do
   gem 'coveralls', require: false
   gem 'rspec', '~> 2.14.0'
diff --git a/Gemfile.lock b/Gemfile.lock
index 9bf7d9f..cd07c9b 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -37,6 +37,7 @@ GEM
       method_source (~> 0.8)
       slop (~> 3.3.1)
     rainbow (2.0.0)
+    redis (3.3.0)
     rest-client (1.7.2)
       mime-types (>= 1.16, < 3.0)
       netrc (~> 0.7)
@@ -77,7 +78,11 @@ DEPENDENCIES
   coveralls
   guard
   guard-rspec
+  redis (~> 3.3)
   rspec (~> 2.14.0)
   rubocop (= 0.28.0)
   vcr
   webmock
+
+BUNDLED WITH
+   1.11.2
diff --git a/bin/check b/bin/check
index b90e15d..7aa1fe4 100755
--- a/bin/check
+++ b/bin/check
@@ -36,8 +36,5 @@ dirs.each do |dir|
   puts "\n"
 end
 
-print "Test redis-cli executable: "
-abort('FAILED') unless system(*config.redis_command, '--version')
-
 print "Send ping to redis server: "
-abort unless system(*config.redis_command, 'ping')
+abort unless GitlabNet.new.redis_client.ping
diff --git a/lib/gitlab_config.rb b/lib/gitlab_config.rb
index 831f0e3..ebf72d6 100644
--- a/lib/gitlab_config.rb
+++ b/lib/gitlab_config.rb
@@ -54,26 +54,4 @@ class GitlabConfig
   def git_annex_enabled?
     @config['git_annex_enabled'] ||= false
   end
-
-  # Build redis command to write update event in gitlab queue
-  def redis_command
-    if redis.empty?
-      # Default to old method of connecting to redis
-      # for users that haven't updated their configuration
-      %W(env -i redis-cli)
-    else
-      redis['database'] ||= 0
-      redis['host'] ||= '127.0.0.1'
-      redis['port'] ||= '6379'
-      if redis.has_key?("socket")
-        %W(#{redis['bin']} -s #{redis['socket']} -n #{redis['database']})
-      else
-        if redis.has_key?("pass")
-          %W(#{redis['bin']} -h #{redis['host']} -p #{redis['port']} -n #{redis['database']} -a #{redis['pass']})
-        else
-          %W(#{redis['bin']} -h #{redis['host']} -p #{redis['port']} -n #{redis['database']})
-        end
-      end
-    end
-  end
 end
diff --git a/lib/gitlab_net.rb b/lib/gitlab_net.rb
index 8b6d33b..8e1fe39 100644
--- a/lib/gitlab_net.rb
+++ b/lib/gitlab_net.rb
@@ -1,6 +1,7 @@
 require 'net/http'
 require 'openssl'
 require 'json'
+require 'redis'
 
 require_relative 'gitlab_config'
 require_relative 'gitlab_logger'
@@ -63,6 +64,24 @@ class GitlabNet
     nil
   end
 
+  def redis_client
+    redis_config = config.redis
+    database = redis_config['database'] || 0
+    params = {
+      host: redis_config['host'] || '127.0.0.1',
+      port: redis_config['port'] || 6379,
+      db: database
+    }
+
+    if redis_config.has_key?("socket")
+      params = { path: redis_config['socket'], db: database }
+    elsif redis_config.has_key?("pass")
+      params[:password] = redis_config['pass']
+    end
+
+    Redis.new(params)
+  end
+
   protected
 
   def config
diff --git a/lib/gitlab_post_receive.rb b/lib/gitlab_post_receive.rb
index 0fff479..8632432 100644
--- a/lib/gitlab_post_receive.rb
+++ b/lib/gitlab_post_receive.rb
@@ -2,6 +2,7 @@ require_relative 'gitlab_init'
 require_relative 'gitlab_net'
 require 'json'
 require 'base64'
+require 'redis'
 require 'securerandom'
 
 class GitlabPostReceive
@@ -74,11 +75,12 @@ class GitlabPostReceive
 
     queue = "#{config.redis_namespace}:queue:post_receive"
     msg = JSON.dump({ 'class' => 'PostReceive', 'args' => [@repo_path, @actor, changes], 'jid' => @jid  })
-    if system(*config.redis_command, 'rpush', queue, msg,
-              err: '/dev/null', out: '/dev/null')
+
+    begin
+      GitlabNet.new.redis_client.rpush(queue, msg)
       return true
-    else
-      puts "GitLab: An unexpected error occurred (redis-cli returned #{$?.exitstatus})."
+    rescue => e
+      puts "GitLab: An unexpected error occurred in writing to Redis: #{e}"
       return false
     end
   end
diff --git a/spec/gitlab_config_spec.rb b/spec/gitlab_config_spec.rb
index 0ce641b..6c1af85 100644
--- a/spec/gitlab_config_spec.rb
+++ b/spec/gitlab_config_spec.rb
@@ -38,7 +38,7 @@ eos
 
     context 'remove trailing slashes' do
       before { config.send(:config)['gitlab_url'] = url + '//' }
-      
+
       it { should eq(url) }
     end
   end
@@ -48,40 +48,4 @@ eos
 
     it("returns false by default") { should eq(false) }
   end
-
-  describe :redis_command do
-    subject { config.redis_command }
-
-    context "with empty redis config" do
-      before do
-        config.stub(:redis) { {} }
-      end
-
-      it { should be_an(Array) }
-      it { should include('redis-cli') }
-    end
-
-    context "with host and port" do
-      before do
-        config.stub(:redis) { {'host' => 'localhost', 'port' => 1123, 'bin' => '/usr/bin/redis-cli'} }
-      end
-
-      it { should be_an(Array) }
-      it { should include(config.redis['host']) }
-      it { should include(config.redis['bin']) }
-      it { should include(config.redis['port'].to_s) }
-    end
-
-    context "with redis socket" do
-      let(:socket) { '/tmp/redis.socket' }
-      before do
-        config.stub(:redis) { {'bin' => '', 'socket' => socket } }
-      end
-
-      it { should be_an(Array) }
-      it { should include(socket) }
-      it { should_not include('-p') }
-      it { should_not include('-h') }
-    end
-  end
 end
diff --git a/spec/gitlab_net_spec.rb b/spec/gitlab_net_spec.rb
index e4dee33..1312554 100644
--- a/spec/gitlab_net_spec.rb
+++ b/spec/gitlab_net_spec.rb
@@ -229,4 +229,44 @@ describe GitlabNet, vcr: true do
       store.should_receive(:add_path).with('test_path')
     end
   end
+
+  describe '#redis_client' do
+    let(:config) { double('config') }
+
+    context "with empty redis config" do
+      it 'returns default parameters' do
+        allow(gitlab_net).to receive(:config).and_return(config)
+        allow(config).to receive(:redis).and_return( {} )
+
+        expect_any_instance_of(Redis).to receive(:initialize).with({ host: '127.0.0.1',
+                                                                     port: 6379,
+                                                                     db: 0 }).and_call_original
+        gitlab_net.redis_client
+      end
+    end
+
+    context "with host and port" do
+      it 'uses the specified host and port' do
+        allow(gitlab_net).to receive(:config).and_return(config)
+        allow(config).to receive(:redis).and_return( { 'host' => 'localhost', 'port' => 1123 } )
+
+        expect_any_instance_of(Redis).to receive(:initialize).with({ host: 'localhost',
+                                                                     port: 1123,
+                                                                     db: 0 }).and_call_original
+        gitlab_net.redis_client
+      end
+    end
+
+    context "with redis socket" do
+      let(:socket) { '/tmp/redis.socket' }
+
+      it 'uses the socket' do
+        allow(gitlab_net).to receive(:config).and_return(config)
+        allow(config).to receive(:redis).and_return( { 'socket' => socket })
+
+        expect_any_instance_of(Redis).to receive(:initialize).with({ path: socket, db: 0 }).and_call_original
+        gitlab_net.redis_client
+      end
+    end
+  end
 end
diff --git a/spec/gitlab_post_receive_spec.rb b/spec/gitlab_post_receive_spec.rb
index 9d7696e..0392bee 100644
--- a/spec/gitlab_post_receive_spec.rb
+++ b/spec/gitlab_post_receive_spec.rb
@@ -1,3 +1,4 @@
+# coding: utf-8
 require 'spec_helper'
 require 'gitlab_post_receive'
 
@@ -11,6 +12,7 @@ describe GitlabPostReceive do
   let(:repo_path)  { File.join(repository_path, repo_name) + ".git" }
   let(:gitlab_post_receive) { GitlabPostReceive.new(repo_path, actor, wrongly_encoded_changes) }
   let(:message) { "test " * 10 + "message " * 10 }
+  let(:redis_client) { double('redis_client') }
 
   before do
     GitlabConfig.any_instance.stub(repos_path: repository_path)
@@ -20,11 +22,11 @@ describe GitlabPostReceive do
   describe "#exec" do
 
     before do
-      GitlabConfig.any_instance.stub(redis_command: %w(env -i redis-cli))
-      allow(gitlab_post_receive).to receive(:system).and_return(true)
+      allow_any_instance_of(GitlabNet).to receive(:redis_client).and_return(redis_client)
     end
 
     it "prints the broadcast message" do
+      expect(redis_client).to receive(:rpush)
       expect(gitlab_post_receive).to receive(:puts).ordered
       expect(gitlab_post_receive).to receive(:puts).with(
         "========================================================================"
@@ -38,7 +40,7 @@ describe GitlabPostReceive do
         "    message message message message message message message message"
       ).ordered
 
-      expect(gitlab_post_receive).to receive(:puts).ordered      
+      expect(gitlab_post_receive).to receive(:puts).ordered
       expect(gitlab_post_receive).to receive(:puts).with(
         "========================================================================"
       ).ordered
@@ -47,12 +49,9 @@ describe GitlabPostReceive do
     end
 
     it "pushes a Sidekiq job onto the queue" do
-      expect(gitlab_post_receive).to receive(:system).with(
-        *[
-          *%w(env -i redis-cli rpush resque:gitlab:queue:post_receive), 
-          %Q/{"class":"PostReceive","args":["#{repo_path}","#{actor}",#{base64_changes.inspect}],"jid":"#{gitlab_post_receive.jid}"}/,
-          { err: "/dev/null", out: "/dev/null" }
-        ]
+      expect(redis_client).to receive(:rpush).with(
+        'resque:gitlab:queue:post_receive',
+         %Q/{"class":"PostReceive","args":["#{repo_path}","#{actor}",#{base64_changes.inspect}],"jid":"#{gitlab_post_receive.jid}"}/
       ).and_return(true)
 
       gitlab_post_receive.exec
@@ -61,7 +60,7 @@ describe GitlabPostReceive do
     context "when the redis command succeeds" do
 
       before do
-        allow(gitlab_post_receive).to receive(:system).and_return(true)
+        allow(redis_client).to receive(:rpush).and_return(true)
       end
 
       it "returns true" do
@@ -72,8 +71,7 @@ describe GitlabPostReceive do
     context "when the redis command fails" do
 
       before do
-        allow(gitlab_post_receive).to receive(:system).and_return(false)
-        allow($?).to receive(:exitstatus).and_return(nil)
+        allow(redis_client).to receive(:rpush).and_raise('Fail')
       end
 
       it "returns false" do