Merge branch 'use-redis-rb-client' into 'master'

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

Previously the post-receive hook fired redis-cli, but if the argument list was too long the hook would silently fail. Instead of shelling out to redis-cli, we use a Ruby client to send the same message.

Closes gitlab-org/gitlab-ce#17329


See merge request !59

.rubocop.yml

@@ -967,3 +967,4 @@ AllCops:
     - 'lib/email_validator.rb'
     - 'lib/gitlab/upgrader.rb'
     - 'lib/gitlab/seeder.rb'
+    - 'lib/vendor/**/*'

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'

Gemfile.lock

@@ -81,3 +81,6 @@ DEPENDENCIES
   rubocop (= 0.28.0)
   vcr
   webmock
+
+BUNDLED WITH
+   1.11.2

Makefile

+REDIS_RB_VERSION=v3.3.0
+REDIS_RB_VENDOR_DIR=lib/vendor/redis
+PWD=`pwd`
+
+all:
+
+update-redis:
+	rm -rf $(REDIS_RB_VENDOR_DIR)
+	git clone -b $(REDIS_RB_VERSION) https://github.com/redis/redis-rb.git $(REDIS_RB_VENDOR_DIR)
+	rm -rf $(REDIS_RB_VENDOR_DIR)/.git
+
+.PHONY=update-redis

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

lib/gitlab_config.rb

+$:.unshift(File.expand_path(File.join(File.dirname(__FILE__), 'vendor/redis/lib')))
 require 'yaml'
 
 class GitlabConfig
@@ -54,26 +55,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

lib/gitlab_net.rb

 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

lib/gitlab_post_receive.rb

@@ -74,11 +74,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

lib/vendor/redis/lib/redis/connection.rb

+require "redis/connection/registry"
+
+# If a connection driver was required before this file, the array
+# Redis::Connection.drivers will contain one or more classes. The last driver
+# in this array will be used as default driver. If this array is empty, we load
+# the plain Ruby driver as our default. Another driver can be required at a
+# later point in time, causing it to be the last element of the #drivers array
+# and therefore be chosen by default.
+require "redis/connection/ruby" if Redis::Connection.drivers.empty?
\ No newline at end of file

lib/vendor/redis/lib/redis/connection/command_helper.rb

+class Redis
+  module Connection
+    module CommandHelper
+
+      COMMAND_DELIMITER = "\r\n"
+
+      def build_command(args)
+        command = [nil]
+
+        args.each do |i|
+          if i.is_a? Array
+            i.each do |j|
+              j = j.to_s
+              command << "$#{j.bytesize}"
+              command << j
+            end
+          else
+            i = i.to_s
+            command << "$#{i.bytesize}"
+            command << i
+          end
+        end
+
+        command[0] = "*#{(command.length - 1) / 2}"
+
+        # Trailing delimiter
+        command << ""
+        command.join(COMMAND_DELIMITER)
+      end
+
+    protected
+
+      if defined?(Encoding::default_external)
+        def encode(string)
+          string.force_encoding(Encoding::default_external)
+        end
+      else
+        def encode(string)
+          string
+        end
+      end
+    end
+  end
+end

lib/vendor/redis/lib/redis/connection/hiredis.rb

+require "redis/connection/registry"
+require "redis/errors"
+require "hiredis/connection"
+require "timeout"
+
+class Redis
+  module Connection
+    class Hiredis
+
+      def self.connect(config)
+        connection = ::Hiredis::Connection.new
+        connect_timeout = (config.fetch(:connect_timeout, 0) * 1_000_000).to_i
+
+        if config[:scheme] == "unix"
+          connection.connect_unix(config[:path], connect_timeout)
+        elsif config[:scheme] == "rediss" || config[:ssl]
+          raise NotImplementedError, "SSL not supported by hiredis driver"
+        else
+          connection.connect(config[:host], config[:port], connect_timeout)
+        end
+
+        instance = new(connection)
+        instance.timeout = config[:read_timeout]
+        instance
+      rescue Errno::ETIMEDOUT
+        raise TimeoutError
+      end
+
+      def initialize(connection)
+        @connection = connection
+      end
+
+      def connected?
+        @connection && @connection.connected?
+      end
+
+      def timeout=(timeout)
+        # Hiredis works with microsecond timeouts
+        @connection.timeout = Integer(timeout * 1_000_000)
+      end
+
+      def disconnect
+        @connection.disconnect
+        @connection = nil
+      end
+
+      def write(command)
+        @connection.write(command.flatten(1))
+      rescue Errno::EAGAIN
+        raise TimeoutError
+      end
+
+      def read
+        reply = @connection.read
+        reply = CommandError.new(reply.message) if reply.is_a?(RuntimeError)
+        reply
+      rescue Errno::EAGAIN
+        raise TimeoutError
+      rescue RuntimeError => err
+        raise ProtocolError.new(err.message)
+      end
+    end
+  end
+end
+
+Redis::Connection.drivers << Redis::Connection::Hiredis

lib/vendor/redis/lib/redis/connection/registry.rb

+class Redis
+  module Connection
+
+    # Store a list of loaded connection drivers in the Connection module.
+    # Redis::Client uses the last required driver by default, and will be aware
+    # of the loaded connection drivers if the user chooses to override the
+    # default connection driver.
+    def self.drivers
+      @drivers ||= []
+    end
+  end
+end

lib/vendor/redis/lib/redis/connection/synchrony.rb

+require "redis/connection/command_helper"
+require "redis/connection/registry"
+require "redis/errors"
+require "em-synchrony"
+require "hiredis/reader"
+
+class Redis
+  module Connection
+    class RedisClient < EventMachine::Connection
+      include EventMachine::Deferrable
+
+      attr_accessor :timeout
+
+      def post_init
+        @req = nil
+        @connected = false
+        @reader = ::Hiredis::Reader.new
+      end
+
+      def connection_completed
+        @connected = true
+        succeed
+      end
+
+      def connected?
+        @connected
+      end
+
+      def receive_data(data)
+        @reader.feed(data)
+
+        loop do
+          begin
+            reply = @reader.gets
+          rescue RuntimeError => err
+            @req.fail [:error, ProtocolError.new(err.message)]
+            break
+          end
+
+          break if reply == false
+
+          reply = CommandError.new(reply.message) if reply.is_a?(RuntimeError)
+          @req.succeed [:reply, reply]
+        end
+      end
+
+      def read
+        @req = EventMachine::DefaultDeferrable.new
+        if @timeout > 0
+          @req.timeout(@timeout, :timeout)
+        end
+        EventMachine::Synchrony.sync @req
+      end
+
+      def send(data)
+        callback { send_data data }
+      end
+
+      def unbind
+        @connected = false
+        if @req
+          @req.fail [:error, Errno::ECONNRESET]
+          @req = nil
+        else
+          fail
+        end
+      end
+    end
+
+    class Synchrony
+      include Redis::Connection::CommandHelper
+
+      def self.connect(config)
+        if config[:scheme] == "unix"
+          conn = EventMachine.connect_unix_domain(config[:path], RedisClient)
+        elsif config[:scheme] == "rediss" || config[:ssl]
+          raise NotImplementedError, "SSL not supported by synchrony driver"
+        else
+          conn = EventMachine.connect(config[:host], config[:port], RedisClient) do |c|
+            c.pending_connect_timeout = [config[:connect_timeout], 0.1].max
+          end
+        end
+
+        fiber = Fiber.current
+        conn.callback { fiber.resume }
+        conn.errback { fiber.resume :refused }
+
+        raise Errno::ECONNREFUSED if Fiber.yield == :refused
+
+        instance = new(conn)
+        instance.timeout = config[:read_timeout]
+        instance
+      end
+
+      def initialize(connection)
+        @connection = connection
+      end
+
+      def connected?
+        @connection && @connection.connected?
+      end
+
+      def timeout=(timeout)
+        @connection.timeout = timeout
+      end
+
+      def disconnect
+        @connection.close_connection
+        @connection = nil
+      end
+
+      def write(command)
+        @connection.send(build_command(command))
+      end
+
+      def read
+        type, payload = @connection.read
+
+        if type == :reply
+          payload
+        elsif type == :error
+          raise payload
+        elsif type == :timeout
+          raise TimeoutError
+        else
+          raise "Unknown type #{type.inspect}"
+        end
+      end
+    end
+  end
+end
+
+Redis::Connection.drivers << Redis::Connection::Synchrony

lib/vendor/redis/lib/redis/version.rb

+class Redis
+  VERSION = "3.3.0"
+end