Merge branch 'sh-fix-unix-redis-with-password' into 'master'

redis: Fix password auth with UNIX domain sockets

See merge request https://gitlab.com/gitlab-org/omnibus-gitlab/-/merge_requests/7573

Merged-by: Stan Hu <stanhu@gmail.com>
Reviewed-by: Gabriel Mazetto <gabriel@gitlab.com>
Reviewed-by: Ian Baum <ibaum@gitlab.com>

doc/settings/memory_constrained_envs.md

@@ -145,14 +145,14 @@ We observed 100-400MB of memory usage reduction configuring Puma this way.
 ## Optimize Sidekiq
 
 Sidekiq is a background processing daemon. When configured with GitLab by default
-it runs with a high concurrency mode of `50`. This does impact how much memory it can
+it runs with a concurrency mode of `20`. This does impact how much memory it can
 allocate at a given time. It is advised to configure it to use a significantly
 smaller value of `5` or `10` (preferred).
 
 In `/etc/gitlab/gitlab.rb`:
 
 ```ruby
-sidekiq['max_concurrency'] = 10
+sidekiq['concurrency'] = 10
 ```
 
 ## Optimize Gitaly
@@ -249,7 +249,7 @@ and disable the Prometheus Metrics feature:
    ```ruby
    puma['worker_processes'] = 0
 
-   sidekiq['max_concurrency'] = 10
+   sidekiq['concurrency'] = 10
 
    prometheus_monitoring['enable'] = false
 

doc/settings/rpi.md

@@ -55,7 +55,7 @@ some running processes:
    ```ruby
    # Reduce the number of running workers to the minimum in order to reduce memory usage
    puma['worker_processes'] = 2
-   sidekiq['max_concurrency'] = 9
+   sidekiq['concurrency'] = 9
    # Turn off monitoring to reduce idle cpu and disk usage
    prometheus_monitoring['enable'] = false
    ```

files/gitlab-config-template/gitlab.rb.template

@@ -1280,9 +1280,7 @@ external_url 'GENERATED_EXTERNAL_URL'
 # sidekiq['log_format'] = "json"
 # sidekiq['shutdown_timeout'] = 4
 # sidekiq['interval'] = nil
-# sidekiq['concurrency'] = nil
-# sidekiq['max_concurrency'] = 20
-# sidekiq['min_concurrency'] = nil
+# sidekiq['concurrency'] = 20
 
 ##! GitLab allows route a job to a particular queue determined by an array of ##! routing rules.
 ##! Each routing rule is a tuple of queue selector query and corresponding queue. By default,

files/gitlab-cookbooks/gitlab/attributes/default.rb

@@ -728,9 +728,7 @@ default['gitlab']['sidekiq']['health_checks_listen_port'] = 8092
 
 # Cluster specific settings
 default['gitlab']['sidekiq']['interval'] = nil
-default['gitlab']['sidekiq']['concurrency'] = nil
-default['gitlab']['sidekiq']['max_concurrency'] = 20
-default['gitlab']['sidekiq']['min_concurrency'] = nil
+default['gitlab']['sidekiq']['concurrency'] = 20
 default['gitlab']['sidekiq']['queue_groups'] = ['*']
 default['gitlab']['sidekiq']['consul_service_name'] = 'sidekiq'
 default['gitlab']['sidekiq']['consul_service_meta'] = nil

files/gitlab-cookbooks/gitlab/libraries/redis_helper.rb

@@ -33,13 +33,17 @@ class RedisHelper
 
     redis_socket = gitlab_rails['redis_socket']
     redis_socket = false if RedisHelper::Checks.is_gitlab_rails_redis_tcp?
+    params = redis_params(support_sentinel_groupname: support_sentinel_groupname)
 
     if redis_socket && !RedisHelper::Checks.has_sentinels?
-      uri = URI('unix:/')
+      uri = URI("unix://")
       uri.path = redis_socket
-    else
-      params = redis_params(support_sentinel_groupname: support_sentinel_groupname)
 
+      if params[2]
+        password = encode_redis_password(params[2])
+        uri.userinfo = ":#{password}"
+      end
+    else
       uri = build_redis_url(
         ssl: gitlab_rails['redis_ssl'],
         host: params[0],
@@ -97,6 +101,10 @@ class RedisHelper
     uri
   end
 
+  def encode_redis_password(password)
+    URI::Generic::DEFAULT_PARSER.escape(password)
+  end
+
   def redis_sentinel_urls(sentinels_key)
     gitlab_rails = @node['gitlab']['gitlab_rails']
 

files/gitlab-cookbooks/gitlab/templates/default/sv-sidekiq-run.erb

@@ -22,12 +22,6 @@ exec chpst -e /opt/gitlab/etc/gitlab-rails/env -P \
       <% if node['gitlab']['sidekiq']['concurrency'] %>
       -c <%= node['gitlab']['sidekiq']['concurrency'] %> \
       <% end %>
-      <% if node['gitlab']['sidekiq']['max_concurrency'] %>
-      -m <%= node['gitlab']['sidekiq']['max_concurrency'] %> \
-      <% end %>
-      <% if node['gitlab']['sidekiq']['min_concurrency'] %>
-      --min-concurrency <%= node['gitlab']['sidekiq']['min_concurrency'] %> \
-      <% end %>
       <% if node['gitlab']['sidekiq']['shutdown_timeout'] %>
       --timeout <%= node['gitlab']['sidekiq']['shutdown_timeout'] %> \
       <% end %>

files/gitlab-cookbooks/package/libraries/deprecations.rb

@@ -346,14 +346,14 @@ module Gitlab
           },
           {
             config_keys: %w(gitlab sidekiq min_concurrency),
-            deprecation: '16.9',
-            removal: '17.0',
+            deprecation: '16.9', # Remove message issue: https://gitlab.com/gitlab-org/omnibus-gitlab/-/issues/8491
+            removal: '17.0', # Removal issue: https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/3422
             note: "Starting with GitLab 17.0, `sidekiq['min_concurrency']` will be removed. Please follow https://docs.gitlab.com/ee/administration/sidekiq/extra_sidekiq_processes.html#manage-thread-counts-explicitly to use `sidekiq['concurrency']` instead."
           },
           {
             config_keys: %w(gitlab sidekiq max_concurrency),
-            deprecation: '16.9',
-            removal: '17.0',
+            deprecation: '16.9', # Remove message issue: https://gitlab.com/gitlab-org/omnibus-gitlab/-/issues/8491
+            removal: '17.0', # Removal issue: https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/3422
             note: "Starting with GitLab 17.0, `sidekiq['max_concurrency']` will be removed. Please follow https://docs.gitlab.com/ee/administration/sidekiq/extra_sidekiq_processes.html#manage-thread-counts-explicitly to use `sidekiq['concurrency']` instead."
           },
           {

files/gitlab-cookbooks/package/libraries/helpers/new_redis_helper.rb

@@ -28,5 +28,14 @@ module NewRedisHelper
         )
       end
     end
+
+    # RFC 3986 says that the userinfo value should be percent-encoded:
+    # https://datatracker.ietf.org/doc/html/rfc3986#section-3.2.1.
+    # Note that CGI.escape and URI.encode_www_form_component encodes
+    # a space as "+" instead of "%20". While this appears to be handled with
+    # the Ruby client, the Go client doesn't work with "+".
+    def encode_redis_password(password)
+      URI::Generic::DEFAULT_PARSER.escape(password)
+    end
   end
 end

files/gitlab-cookbooks/package/libraries/helpers/new_redis_helper/base.rb

@@ -37,12 +37,17 @@ module NewRedisHelper
                  redis_socket
                end
 
-      if socket && !has_sentinels?
-        uri = URI('unix:/')
-        uri.path = socket
-      else
-        params = redis_credentials
+      params = redis_credentials
 
+      if socket && !has_sentinels?
+        uri = URI("unix://")
+        uri.path = socket
+
+        if params[:password]
+          password = NewRedisHelper.encode_redis_password(params[:password])
+          uri.userinfo = ":#{password}"
+        end
+      else
         uri = NewRedisHelper.build_redis_url(
           ssl: redis_ssl,
           host: params[:host],

spec/chef/cookbooks/gitlab/libraries/redis_helper_spec.rb

@@ -49,7 +49,7 @@ RSpec.describe RedisHelper do
   context '#redis_url' do
     context 'with default configuration' do
       it 'returns a unix socket' do
-        expect(subject.redis_url.to_s).to eq('unix:/var/opt/gitlab/redis/redis.socket')
+        expect(subject.redis_url.to_s).to eq('unix:///var/opt/gitlab/redis/redis.socket')
       end
     end
 

spec/chef/cookbooks/gitlab/recipes/gitlab-rails_spec.rb

@@ -210,20 +210,24 @@ RSpec.describe 'gitlab::gitlab-rails' do
 
   context 'with redis settings' do
     let(:config_file) { '/var/opt/gitlab/gitlab-rails/etc/resque.yml' }
+    let(:resque_yml_template) { chef_run.template('/var/opt/gitlab/gitlab-rails/etc/resque.yml') }
+    let(:resque_yml_file_content) { ChefSpec::Renderer.new(chef_run, resque_yml_template).content }
+    let(:resque_yml) { YAML.safe_load(resque_yml_file_content, aliases: true, symbolize_names: true) }
+
     let(:chef_run) { ChefSpec::SoloRunner.new(step_into: %w(templatesymlink)).converge('gitlab::default') }
 
     context 'and default configuration' do
       it 'creates the config file with the required redis settings' do
         expect(chef_run).to create_templatesymlink('Create a resque.yml and create a symlink to Rails root').with_variables(
           hash_including(
-            redis_url: URI('unix:/var/opt/gitlab/redis/redis.socket'),
+            redis_url: URI('unix:///var/opt/gitlab/redis/redis.socket'),
             redis_sentinels: [],
             redis_enable_client: true
           )
         )
 
         expect(chef_run).to render_file(config_file).with_content { |content|
-          expect(content).to match(%r(url: unix:/var/opt/gitlab/redis/redis.socket$))
+          expect(content).to match(%r(url: unix:///var/opt/gitlab/redis/redis.socket$))
           expect(content).not_to match(/id:/)
         }
       end
@@ -231,14 +235,14 @@ RSpec.describe 'gitlab::gitlab-rails' do
       it 'creates cable.yml with the same settings' do
         expect(chef_run).to create_templatesymlink('Create a cable.yml and create a symlink to Rails root').with_variables(
           hash_including(
-            redis_url: URI('unix:/var/opt/gitlab/redis/redis.socket'),
+            redis_url: URI('unix:///var/opt/gitlab/redis/redis.socket'),
             redis_sentinels: [],
             redis_enable_client: true
           )
         )
 
         expect(chef_run).to render_file('/var/opt/gitlab/gitlab-rails/etc/cable.yml').with_content { |content|
-          expect(content).to match(%r(url: unix:/var/opt/gitlab/redis/redis.socket$))
+          expect(content).to match(%r(url: unix:///var/opt/gitlab/redis/redis.socket$))
         }
       end
 
@@ -301,10 +305,6 @@ RSpec.describe 'gitlab::gitlab-rails' do
     end
 
     context 'with TLS settings' do
-      let(:resque_yml_template) { chef_run.template('/var/opt/gitlab/gitlab-rails/etc/resque.yml') }
-      let(:resque_yml_file_content) { ChefSpec::Renderer.new(chef_run, resque_yml_template).content }
-      let(:resque_yml) { YAML.safe_load(resque_yml_file_content, aliases: true, symbolize_names: true) }
-
       before do
         stub_gitlab_rb(
           gitlab_rails: {
@@ -370,6 +370,38 @@ RSpec.describe 'gitlab::gitlab-rails' do
       end
     end
 
+    context 'with a password and UNIX socket' do
+      let(:cable_yml_template) { chef_run.template('/var/opt/gitlab/gitlab-rails/etc/cable.yml') }
+      let(:cable_yml_file_content) { ChefSpec::Renderer.new(chef_run, cable_yml_template).content }
+      let(:cable_yml) { YAML.safe_load(cable_yml_file_content, aliases: true, symbolize_names: true) }
+
+      before do
+        stub_gitlab_rb(
+          gitlab_rails: {
+            redis_password: 'my pass',
+          }
+        )
+      end
+
+      it 'renders resque.yml with password' do
+        expected_output = {
+          url: "unix://:my%20pass@/var/opt/gitlab/redis/redis.socket",
+          secret_file: "/var/opt/gitlab/gitlab-rails/shared/encrypted_settings/redis.yml.enc"
+        }
+
+        expect(resque_yml[:production]).to eq(expected_output)
+      end
+
+      it 'creates cable.yml with password' do
+        expected_output = {
+          adapter: 'redis',
+          url: "unix://:my%20pass@/var/opt/gitlab/redis/redis.socket",
+        }
+
+        expect(cable_yml[:production]).to eq(expected_output)
+      end
+    end
+
     context 'with multiple instances' do
       context 'with action cable' do
         before do

spec/chef/cookbooks/gitlab/recipes/gitlab-workhorse_spec.rb

@@ -504,7 +504,7 @@ RSpec.describe 'gitlab::gitlab-workhorse' do
 
   context 'with default values for redis' do
     it 'should generate config file' do
-      content_url = 'URL = "unix:/var/opt/gitlab/redis/redis.socket"'
+      content_url = 'URL = "unix:///var/opt/gitlab/redis/redis.socket"'
       expect(chef_run).to render_file(config_file).with_content(content_url)
       expect(chef_run).not_to render_file(config_file).with_content(/Sentinel/)
     end
@@ -543,7 +543,7 @@ RSpec.describe 'gitlab::gitlab-workhorse' do
     end
 
     it 'should generate config file with the specified values' do
-      content_url = 'URL = "unix:/home/random/path.socket"'
+      content_url = 'URL = "unix://:examplepassword@/home/random/path.socket"'
       content_password = 'Password = "examplepassword"'
       expect(chef_run).to render_file("/var/opt/gitlab/gitlab-workhorse/config.toml").with_content(content_url)
       expect(chef_run).to render_file("/var/opt/gitlab/gitlab-workhorse/config.toml").with_content(content_password)
@@ -653,7 +653,7 @@ RSpec.describe 'gitlab::gitlab-workhorse' do
       end
 
       it 'should generate config file with the specified values' do
-        content_url = 'URL = "unix:/home/random/path.socket"'
+        content_url = 'URL = "unix://:some%20pass@/home/random/path.socket"'
         content_password = 'Password = "some pass"'
         expect(chef_run).to render_file("/var/opt/gitlab/gitlab-workhorse/config.toml").with_content(content_url)
         expect(chef_run).to render_file("/var/opt/gitlab/gitlab-workhorse/config.toml").with_content(content_password)

spec/chef/cookbooks/gitlab/recipes/sidekiq_spec.rb

@@ -26,7 +26,7 @@ RSpec.describe 'gitlab::sidekiq' do
           expect(content).to match(/rubyopt=\"-W:no-experimental\"/)
           expect(content).to include(%(RUBYOPT="${rubyopt}"))
           expect(content).to match(%r{bin/sidekiq-cluster})
-          expect(content).to match(/-m 20/) # max_concurrency
+          expect(content).to match(/-c 20/) # concurrency
           expect(content).to match(/--timeout 25/) # shutdown timeout
           expect(content).to match(/\*/) # all queues
         }
@@ -64,11 +64,19 @@ RSpec.describe 'gitlab::sidekiq' do
       before do
         stub_gitlab_rb(
           sidekiq: {
-            log_group: 'fugee'
+            log_group: 'fugee',
+            concurrency: 42
           }
         )
       end
       it_behaves_like 'enabled logged service', 'sidekiq', true, { log_directory_owner: 'git', log_group: 'fugee' }
+
+      it 'correctly renders out the sidekiq service file' do
+        expect(chef_run).to render_file("/opt/gitlab/sv/sidekiq/run")
+          .with_content { |content|
+            expect(content).to match(/-c 42/) # concurrency
+          }
+      end
     end
   end
 

spec/chef/cookbooks/package/libraries/new_redis_helper/gitlab_workhorse_spec.rb

@@ -16,7 +16,7 @@ RSpec.describe NewRedisHelper::GitlabWorkhorse do
     context 'by default' do
       it 'returns information about the default Redis instance' do
         expect(subject.redis_params).to eq(
-          url: 'unix:/var/opt/gitlab/redis/redis.socket',
+          url: 'unix:///var/opt/gitlab/redis/redis.socket',
           password: nil,
           sentinels: [],
           sentinelMaster: 'gitlab-redis',
@@ -26,6 +26,26 @@ RSpec.describe NewRedisHelper::GitlabWorkhorse do
     end
 
     context 'with user specified values' do
+      context 'when password set for UNIX socket' do
+        before do
+          stub_gitlab_rb(
+            gitlab_rails: {
+              redis_password: 'redis-password'
+            }
+          )
+        end
+
+        it 'returns a UNIX socket URL with password' do
+          expect(subject.redis_params).to eq(
+            url: 'unix://:redis-password@/var/opt/gitlab/redis/redis.socket',
+            password: 'redis-password',
+            sentinels: [],
+            sentinelMaster: 'gitlab-redis',
+            sentinelPassword: nil
+          )
+        end
+      end
+
       context 'when settings specified via gitlab_rails' do
         before do
           stub_gitlab_rb(