Explicitly set group for repositories_storages and improve manage-storage-directories tests

changelogs/unreleased/4589.yml

+---
+title: Explicitly set group for repositories_storages and improve manage-storage-directories tests
+merge_request: 4589
+author: Ben Bodenmiller (@bbodenmiller)
+type: other

doc/settings/configuration.md

@@ -513,7 +513,7 @@ Enabling this setting will prevent the creation of the following directories:
 | Default location                                       | Permissions | Ownership        | Purpose |
 |--------------------------------------------------------|-------------|------------------|---------|
 | `/var/opt/gitlab/git-data`                             | `0700`        | `git`            | Holds repositories directory |
-| `/var/opt/gitlab/git-data/repositories`                | `2770`        | `git`            | Holds Git repositories |
+| `/var/opt/gitlab/git-data/repositories`                | `2770`        | `git:git`        | Holds Git repositories |
 | `/var/opt/gitlab/gitlab-rails/shared`                  | `0751`        | `git:gitlab-www` | Holds large object directories |
 | `/var/opt/gitlab/gitlab-rails/shared/artifacts`        | `0700`        | `git`            | Holds CI artifacts |
 | `/var/opt/gitlab/gitlab-rails/shared/external-diffs`   | `0700`        | `git`            | Holds external merge request diffs |
@@ -521,8 +521,8 @@ Enabling this setting will prevent the creation of the following directories:
 | `/var/opt/gitlab/gitlab-rails/shared/packages`         | `0700`        | `git`            | Holds package repository |
 | `/var/opt/gitlab/gitlab-rails/shared/dependency_proxy` | `0700`        | `git`            | Holds dependency proxy |
 | `/var/opt/gitlab/gitlab-rails/shared/terraform_state`  | `0700`        | `git`            | Holds terraform state |
-| `/var/opt/gitlab/gitlab-rails/uploads`                 | `0700`        | `git`            | Holds user attachments |
 | `/var/opt/gitlab/gitlab-rails/shared/pages`            | `0750`        | `git:gitlab-www` | Holds user pages |
+| `/var/opt/gitlab/gitlab-rails/uploads`                 | `0700`        | `git`            | Holds user attachments |
 | `/var/opt/gitlab/gitlab-ci/builds`                     | `0700`        | `git`            | Holds CI build logs |
 | `/var/opt/gitlab/.ssh`                                 | `0700`        | `git:git`        | Holds authorized keys |

files/gitlab-cookbooks/gitlab/recipes/gitlab-rails.rb

@@ -56,6 +56,7 @@ repositories_storages = node['gitlab']['gitlab-rails']['repositories_storages']
 repositories_storages.each do |_name, repositories_storage|
   storage_directory repositories_storage['path'] do
     owner gitlab_user
+    group gitlab_group
     mode "2770"
   end
 end

spec/chef/recipes/gitlab-rails_spec.rb

@@ -53,13 +53,27 @@ RSpec.describe 'gitlab::gitlab-rails' do
       RSpec::Mocks.with_temporary_scope do
         stub_gitlab_rb(gitlab_rails: { shared_path: '/tmp/shared',
                                        uploads_directory: '/tmp/uploads',
-                                       builds_directory: '/tmp/builds' },
+                                       uploads_storage_path: '/tmp/uploads_storage' },
+                       gitlab_ci: { builds_directory: '/tmp/builds' },
+                       git_data_dirs: {
+                         "some_storage" => {
+                           "path" => "/tmp/git-data"
+                         }
+                       },
                        manage_storage_directories: { enable: false })
       end
       ChefSpec::SoloRunner.new.converge('gitlab::default')
     end
+    it 'does not create the git-data directory' do
+      expect(chef_run).not_to run_ruby_block('directory resource: /tmp/git-data')
+    end
+
+    it 'does not create the repositories directory' do
+      expect(chef_run).not_to run_ruby_block('directory resource: /tmp/git-data/repositories')
+    end
+
     it 'does not create the shared directory' do
       expect(chef_run).not_to run_ruby_block('directory resource: /tmp/shared')
     end
@@ -88,16 +102,20 @@ RSpec.describe 'gitlab::gitlab-rails' do
       expect(chef_run).not_to run_ruby_block('directory resource: /tmp/shared/terraform_state')
     end
-    it 'does not create the uploads storage directory' do
+    it 'does not create the GitLab pages directory' do
+      expect(chef_run).not_to run_ruby_block('directory resource: /tmp/shared/pages')
+    end
+
+    it 'does not create the uploads directory' do
       expect(chef_run).not_to run_ruby_block('directory resource: /tmp/uploads')
     end
     it 'does not create the ci builds directory' do
-      expect(chef_run).not_to run_ruby_block('directory resource: /tmp/builds')
+      expect(chef_run).not_to run_ruby_block('directory resource: /tmp/uploads_storage')
     end
-    it 'does not create the GitLab pages directory' do
-      expect(chef_run).not_to run_ruby_block('directory resource: /tmp/shared/pages')
+    it 'does not create the uploads storage directory' do
+      expect(chef_run).not_to run_ruby_block('directory resource: /tmp/uploads_storage')
     end
   end
@@ -107,14 +125,27 @@ RSpec.describe 'gitlab::gitlab-rails' do
         stub_gitlab_rb(gitlab_rails: { shared_path: '/tmp/shared',
                                        uploads_directory: '/tmp/uploads',
                                        uploads_storage_path: '/tmp/uploads_storage' },
-                       gitlab_ci: { builds_directory: '/tmp/builds' })
+                       gitlab_ci: { builds_directory: '/tmp/builds' },
+                       git_data_dirs: {
+                         "some_storage" => {
+                           "path" => "/tmp/git-data"
+                         }
+                       })
       end
       ChefSpec::SoloRunner.converge('gitlab::default')
     end
+    it 'creates the git-data directory' do
+      expect(chef_run).to create_storage_directory('/tmp/git-data').with(owner: 'git', mode: '0700')
+    end
+
+    it 'creates the repositories directory' do
+      expect(chef_run).to create_storage_directory('/tmp/git-data/repositories').with(owner: 'git', group: 'git', mode: '2770')
+    end
+
     it 'creates the shared directory' do
-      expect(chef_run).to create_storage_directory('/tmp/shared').with(owner: 'git', mode: '0751')
+      expect(chef_run).to create_storage_directory('/tmp/shared').with(owner: 'git', group: 'gitlab-www', mode: '0751')
     end
     it 'creates the artifacts directory' do
@@ -141,16 +172,8 @@ RSpec.describe 'gitlab::gitlab-rails' do
       expect(chef_run).to create_storage_directory('/tmp/shared/terraform_state').with(owner: 'git', mode: '0700')
     end
-    it 'creates the uploads directory' do
-      expect(chef_run).to create_storage_directory('/tmp/uploads').with(owner: 'git', mode: '0700')
-    end
-
-    it 'creates the ci builds directory' do
-      expect(chef_run).to create_storage_directory('/tmp/builds').with(owner: 'git', mode: '0700')
-    end
-
     it 'creates the GitLab pages directory' do
-      expect(chef_run).to create_storage_directory('/tmp/shared/pages').with(owner: 'git', mode: '0750')
+      expect(chef_run).to create_storage_directory('/tmp/shared/pages').with(owner: 'git', group: 'gitlab-www', mode: '0750')
     end
     it 'creates the shared tmp directory' do
@@ -161,6 +184,14 @@ RSpec.describe 'gitlab::gitlab-rails' do
       expect(chef_run).to create_storage_directory('/tmp/shared/cache').with(owner: 'git', mode: '0700')
     end
+    it 'creates the uploads directory' do
+      expect(chef_run).to create_storage_directory('/tmp/uploads').with(owner: 'git', mode: '0700')
+    end
+
+    it 'creates the ci builds directory' do
+      expect(chef_run).to create_storage_directory('/tmp/builds').with(owner: 'git', mode: '0700')
+    end
+
     it 'creates the uploads storage directory' do
       expect(chef_run).to create_storage_directory('/tmp/uploads_storage').with(owner: 'git', mode: '0700')
     end
@@ -802,7 +833,7 @@ RSpec.describe 'gitlab::gitlab-rails' do
         stub_gitlab_rb(
           git_data_dirs: {
             "second_storage" => {
-              "path" => "tmp/storage"
+              "path" => "/tmp/storage"
             }
           }
         )
@@ -811,7 +842,7 @@ RSpec.describe 'gitlab::gitlab-rails' do
           hash_including(
             'repositories_storages' => {
               'second_storage' => {
-                'path' => 'tmp/storage/repositories',
+                'path' => '/tmp/storage/repositories',
                 'gitaly_address' => 'unix:/var/opt/gitlab/gitaly/gitaly.socket'
               }
             }

spec/chef/recipes/gitlab-shell_spec.rb

@@ -90,7 +90,7 @@ RSpec.describe 'gitlab::gitlab-shell' do
     before { stub_gitlab_rb(user: { home: '/tmp/user' }) }
     it 'creates the ssh dir in the user\'s home directory' do
-      expect(chef_run).to create_storage_directory('/tmp/user/.ssh').with(owner: 'git', mode: '0700')
+      expect(chef_run).to create_storage_directory('/tmp/user/.ssh').with(owner: 'git', group: 'git', mode: '0700')
     end
     it 'creates the config file with the auth_file within user\'s ssh directory' do
@@ -106,11 +106,11 @@ RSpec.describe 'gitlab::gitlab-shell' do
     before { stub_gitlab_rb(user: { home: '/tmp/user' }, gitlab_shell: { auth_file: '/tmp/ssh/authorized_keys' }) }
     it 'creates the ssh dir in the user\'s home directory' do
-      expect(chef_run).to create_storage_directory('/tmp/user/.ssh').with(owner: 'git', mode: '0700')
+      expect(chef_run).to create_storage_directory('/tmp/user/.ssh').with(owner: 'git', group: 'git', mode: '0700')
     end
     it 'creates the auth_file\'s parent directory' do
-      expect(chef_run).to create_storage_directory('/tmp/ssh').with(owner: 'git', mode: '0700')
+      expect(chef_run).to create_storage_directory('/tmp/ssh').with(owner: 'git', group: 'git', mode: '0700')
     end
     it 'creates the config file with the auth_file at the specified location' do
@@ -122,6 +122,14 @@ RSpec.describe 'gitlab::gitlab-shell' do
     end
   end
+  context 'when manage-storage-directories is disabled' do
+    before { stub_gitlab_rb(user: { home: '/tmp/user' }, manage_storage_directories: { enable: false }) }
+
+    it 'doesn\'t create the ssh dir in the user\'s home directory' do
+      expect(chef_run).not_to run_ruby_block('directory resource: /tmp/user/.ssh')
+    end
+  end
+
   context 'with custom settings' do
     before do
       stub_gitlab_rb(