Merge branch 'nfs_safe_directories' into 'master'

Creating root_squash safe storage directories

related: https://gitlab.com/gitlab-org/omnibus-gitlab/issues/1324
Fixes: https://gitlab.com/gitlab-org/omnibus-gitlab/issues/1251
related: https://gitlab.com/gitlab-org/omnibus-gitlab/issues/1305

@jacobvosmaer-gitlab 

See merge request !899

CHANGELOG.md

@@ -3,6 +3,9 @@
 The latest version of this file can be found at the master branch of the
 omnibus-gitlab repository.
 
+8.12.0
+- Add support for using NFS root_squash for storage directories
+
 8.11.1
  - Fixed a regression where the default container registry and mattermost nginx proxy headers were not being set
 

doc/README.md

@@ -77,6 +77,7 @@ Omnibus is a way to package different services and tools required to run GitLab,
 - ['pg_dump: aborting because of server version mismatch'](settings/database.md#using-a-non-packaged-postgresql-database-management-server)
 - ['Errno::ENOMEM: Cannot allocate memory' during backup or upgrade](common_installation_problems/README.md#errnoenomem-cannot-allocate-memory-during-backup-or-upgrade)
 - [NGINX error: 'could not build server_names_hash'](common_installation_problems/README.md#nginx-error-could-not-build-server_names_hash-you-should-increase-server_names_hash_bucket_size)
+- [Reconfigure fails due to "'root' cannot chown" with NFS root_squash](common_installation_problems/README.md#reconfigure-fails-due-to-root-cannot-chown-with-nfs-root_squash)
 
 ## Omnibus GitLab developer documentation
 

doc/common_installation_problems/README.md

@@ -501,6 +501,26 @@ nginx['server_names_hash_bucket_size'] = 128
 
 Run `sudo gitlab-ctl reconfigure` for the change to take effect.
 
+### Reconfigure fails due to "'root' cannot chown" with NFS root_squash
+
+```
+$ gitlab-ctl reconfigure
+    ================================================================================
+    Error executing action `run` on resource 'ruby_block[directory resource: /gitlab-data/git-data]'
+    ================================================================================
+
+    Errno::EPERM
+    ------------
+    'root' cannot chown /gitlab-data/git-data. If using NFS mounts you will need to re-export them in 'no_root_squash' mode and try again.
+    Operation not permitted @ chown_internal - /gitlab-data/git-data
+```
+
+This can happen if you have directories mounted using NFS and configured in `root_squash`
+mode. Reconfigure is not able to properly set the ownership of your directories. You
+will need to switch to using `no_root_squash` in your NFS exports on the NFS server, or
+[disable storage directory management](doc/settings/configuration.md#disable-storage-directories-management)
+ and manage the permissions yourself.
+
 [CAcert.org]: http://www.cacert.org/
 [certificate link shell script]: https://gitlab.com/snippets/6285
 [script source]: https://www.madboa.com/geek/openssl/#verify-new

doc/settings/configuration.md

@@ -305,8 +305,21 @@ Some of these directories will hold large amount of data so in certain setups,
 these directories will most likely be mounted on a NFS (or some other) share.
 
 Some types of mounts won't allow automatic creation of directories by root user
- (default user for initial setup), eg. NFS with `no_root_squash` enabled on the
-share.
+ (default user for initial setup), eg. NFS with `root_squash` enabled on the
+share. To work around this the omnibus-gitlab package will attempt to create
+these directories using the directory's owner user.
+
+If you have the `/etc/gitlab` directory mounted, you can turn off management of
+that directory.
+
+In `/etc/gitlab/gitlab.rb` set:
+
+```ruby
+manage_storage_directories['manage_etc'] = false
+```
+
+If you are mounting all GitLab's storage directories, each on a seperate mount,
+you should completely disable the management of storage directories.
 
 In order to disable management of these directories,
 in `/etc/gitlab/gitlab.rb` set:

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

@@ -667,6 +667,7 @@ external_url 'GENERATED_EXTERNAL_URL'
 ## Set only if the select directories are created manually
 ## See: http://doc.gitlab.com/omnibus/settings/configuration.html#disable-storage-directories-management
 # manage_storage_directories['enable'] = false
+# manage_storage_directories['manage_etc'] = false
 
 #######
 # Git #

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

@@ -29,6 +29,7 @@ default['gitlab']['manage-accounts']['enable'] = true
 
 # Create directories with correct permissions and ownership required by the pkg
 default['gitlab']['manage-storage-directories']['enable'] = true
+default['gitlab']['manage-storage-directories']['manage_etc'] = true
 
 ####
 # The Git User that services run as
@@ -45,7 +46,6 @@ default['gitlab']['user']['home'] = "/var/opt/gitlab"
 default['gitlab']['user']['git_user_name'] = "GitLab"
 default['gitlab']['user']['git_user_email'] = "gitlab@#{node['fqdn']}"
 
-
 ####
 # GitLab Rails app
 ####

files/gitlab-cookbooks/gitlab/definitions/storage_directory.rb

@@ -0,0 +1,39 @@
+#
+# Copyright:: Copyright (c) 2016 GitLab Inc
+# License:: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# Manage the storage directory as the target owner user instead of the running
+# user in order to work with root_squash directories on NFS mounts. It will
+# fallback to using root if the target owner user doesn't have enough access
+define :storage_directory, path: nil, owner: 'root', group: nil, mode: nil do
+  next unless node['gitlab']['manage-storage-directories']['enable']
+
+  params[:path] ||= params[:name]
+  storage_helper = StorageDirectoryHelper.new(params[:owner], params[:group], params[:mode])
+
+  ruby_block "directory resource: #{params[:path]}" do
+    block do
+      # Ensure the directory exists
+      storage_helper.ensure_directory_exists(params[:path])
+
+      # Ensure the permissions are set
+      storage_helper.ensure_permissions_set(params[:path])
+
+      # Error out if we have not achieved the target permissions
+      storage_helper.validate!(params[:path])
+    end
+    not_if { storage_helper.validate(params[:path]) }
+  end
+end

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

@@ -0,0 +1,108 @@
+#
+# Copyright:: Copyright (c) 2016 GitLab Inc
+# License:: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+require_relative 'helper'
+
+class StorageDirectoryHelper
+  include ShellOutHelper
+
+  def initialize(owner, group, mode)
+    @target_owner = owner
+    @target_group = group
+    @target_mode = mode
+  end
+
+  def writable?(path)
+    do_shell_out("test -w #{path}", @target_owner).exitstatus == 0
+  end
+
+  def run_command(cmd, use_euid: false, throw_error: true)
+    run_shell = Mixlib::ShellOut.new(cmd, user: (@target_owner if use_euid),  group: (@target_group if use_euid))
+    run_shell.run_command
+    run_shell.error! if throw_error
+    run_shell
+  end
+
+  def ensure_directory_exists(path)
+    # Ensure the directory exists, create using the euid if the parent directory
+    # is writable by the target_owner
+    run_command("mkdir -p #{path}", use_euid: writable?(File.expand_path('..', path)))
+  end
+
+  def ensure_permissions_set(path)
+    # If the owner doesn't match the expected owner, we need to chown.
+    # Manual user intervention will be required if it fails. (enabling no_root_squash)
+    run_chown(path) if @target_owner != get_owner(path)
+
+    # Set the correct mode on the directory, run using the euid if target_owner
+    # has write access, otherwise use root
+    run_command("chmod #{@target_mode} #{path}", use_euid: writable?(path)) if @target_mode
+
+    # Set the group on the directory, run using the euid if target_owner has
+    # write access, otherwise use root
+    run_command("chgrp #{@target_group} #{path}", use_euid: writable?(path)) if @target_group
+  end
+
+  def get_owner(path)
+    # Use stat to return the owner. The root user may not have execute permissions
+    # to the directory, but the target_owner will in the success case, so always
+    # use the euid to run the command
+    run_command("stat --printf='%U' #{path}", use_euid: true).stdout
+  end
+
+  def run_chown(path)
+    # Chown will not work if it's in a root_squash directory, so the only workarounds
+    # will be for the admin to manually chown on the nfs server, or use
+    # 'no_root_squash' mode in their exports and re-run reconfigure
+    FileUtils.chown(@target_owner, @target_group, path)
+  rescue Errno::EPERM => e
+    raise(
+      e,
+      "'root' cannot chown #{path}. If using NFS mounts you will need to "\
+      "re-export them in 'no_root_squash' mode and try again.\n#{e}",
+      e.backtrace
+    )
+  end
+
+  def validate!(path)
+    # Test that directory is in expected state and error if not.
+    validate(path, throw_error: true)
+  end
+
+  def validate(path, throw_error: false)
+    # Test that directory is in expected state. The root user may not have
+    # execute permissions to the directory, but the target_owner will in the
+    # success case, so always use the euid to run the command
+    run_command(test_stat_cmd(path), use_euid: true, throw_error: throw_error).exitstatus == 0
+  end
+
+  def test_stat_cmd(path)
+    format_string = '%F %U'
+    expect_string = "directory #{@target_owner}"
+
+    if @target_group
+      format_string << ':%G'
+      expect_string << ":#{@target_group}"
+    end
+
+    if @target_mode
+      format_string << ' %04a'
+      expect_string << " #{@target_mode}"
+    end
+
+    "test \"$(stat --printf='#{format_string}' #{path})\" = '#{expect_string}'"
+  end
+end

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

@@ -23,19 +23,19 @@ require 'openssl'
 install_dir = node['package']['install-dir']
 ENV['PATH'] = "#{install_dir}/bin:#{install_dir}/embedded/bin:#{ENV['PATH']}"
 
-directory "/etc/gitlab" do
-  owner "root"
-  group "root"
-  mode "0775"
-  action :nothing
-end.run_action(:create)
-
 Gitlab[:node] = node
 if File.exists?("/etc/gitlab/gitlab.rb")
   Gitlab.from_file("/etc/gitlab/gitlab.rb")
 end
 node.consume_attributes(Gitlab.generate_config(node['fqdn']))
 
+directory "/etc/gitlab" do
+  owner "root"
+  group "root"
+  mode "0775"
+  only_if { node['gitlab']['manage-storage-directories']['manage_etc'] }
+end.run_action(:create)
+
 if File.exists?("/var/opt/gitlab/bootstrapped")
 	node.default['gitlab']['bootstrap']['enable'] = false
 end

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

@@ -48,37 +48,32 @@ directory File.dirname(gitlab_rails_log_dir) do
   recursive true
 end
 
-if node['gitlab']['manage-storage-directories']['enable']
-  # We create shared_path with 751 allowing other users to enter into the directories
-  # It's needed, because by default the shared_path is used to store pages which are served by gitlab-www:gitlab-www
-  directory node['gitlab']['gitlab-rails']['shared_path'] do
-    owner gitlab_user
-    group account_helper.web_server_group
-    mode '0751'
-    recursive true
-  end
+# We create shared_path with 751 allowing other users to enter into the directories
+# It's needed, because by default the shared_path is used to store pages which are served by gitlab-www:gitlab-www
+storage_directory node['gitlab']['gitlab-rails']['shared_path'] do
+  owner gitlab_user
+  group account_helper.web_server_group
+  mode '0751'
+end
 
-  [
-    node['gitlab']['gitlab-rails']['artifacts_path'],
-    node['gitlab']['gitlab-rails']['lfs_storage_path'],
-    gitlab_rails_public_uploads_dir,
-    gitlab_ci_builds_dir
-  ].compact.each do |dir_name|
-    directory dir_name do
-      owner gitlab_user
-      mode '0700'
-      recursive true
-    end
-  end
-
-  directory node['gitlab']['gitlab-rails']['pages_path'] do
+[
+  node['gitlab']['gitlab-rails']['artifacts_path'],
+  node['gitlab']['gitlab-rails']['lfs_storage_path'],
+  gitlab_rails_public_uploads_dir,
+  gitlab_ci_builds_dir
+].compact.each do |dir_name|
+  storage_directory dir_name do
     owner gitlab_user
-    group account_helper.web_server_group
-    mode '0750'
-    recursive true
+    mode '0700'
   end
 end
 
+storage_directory node['gitlab']['gitlab-rails']['pages_path'] do
+  owner gitlab_user
+  group account_helper.web_server_group
+  mode '0750'
+end
+
 [
   gitlab_rails_etc_dir,
   gitlab_rails_static_etc_dir,

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

@@ -29,20 +29,17 @@ log_directory = node['gitlab']['gitlab-shell']['log_directory']
 hooks_directory = node['gitlab']['gitlab-rails']['gitlab_shell_hooks_path']
 gitlab_shell_keys_check = File.join(gitlab_shell_dir, 'bin/gitlab-keys')
 
-if node['gitlab']['manage-storage-directories']['enable']
-  git_data_directories.each do |_name, git_data_directory|
-    directory git_data_directory do
-      owner git_user
-      mode "0700"
-      recursive true
-    end
+git_data_directories.each do |_name, git_data_directory|
+  storage_directory git_data_directory do
+    owner git_user
+    mode "0700"
   end
-  repositories_storages.each do |_name, repositories_storage|
-    directory repositories_storage do
-      owner git_user
-      mode "2770"
-      recursive true
-    end
+end
+
+repositories_storages.each do |_name, repositories_storage|
+  storage_directory repositories_storage do
+    owner git_user
+    mode "2770"
   end
 end
 
@@ -50,11 +47,10 @@ end
   ssh_dir,
   File.dirname(authorized_keys)
 ].uniq.each do |dir|
-  directory dir do
+  storage_directory dir do
     owner git_user
     group git_group
     mode "0700"
-    recursive true
   end
 end
 

spec/chef/recipes/default_spec.rb

@@ -0,0 +1,50 @@
+require 'chef_helper'
+
+describe 'gitlab::default' do
+  let(:chef_run) { ChefSpec::SoloRunner.converge('gitlab::default') }
+
+  before do
+    allow(Gitlab).to receive(:[]).and_call_original
+  end
+
+  it 'creates the user config directory' do
+    expect(chef_run).to create_directory('/etc/gitlab').with(
+      user: 'root',
+      group: 'root',
+      mode: '0775'
+    )
+  end
+
+  it 'creates the var opt data config directory' do
+    expect(chef_run).to create_directory('Create /var/opt/gitlab').with(
+      path: '/var/opt/gitlab',
+      user: 'root',
+      group: 'root',
+      mode: '0755'
+    )
+  end
+
+  it 'creates the system gitconfig directory and file' do
+    stub_gitlab_rb(omnibus_gitconfig: { system: { receive: ["fsckObjects = true"], pack: ["threads = 2"] } })
+
+    expect(chef_run).to create_directory('/opt/gitlab/embedded/etc').with(
+      user: 'root',
+      group: 'root',
+      mode: '0755'
+    )
+
+    expect(chef_run).to create_template('/opt/gitlab/embedded/etc/gitconfig').with(
+      source: 'gitconfig-system.erb',
+      variables: { gitconfig:  { "receive" => ["fsckObjects = true"], "pack" => ["threads = 2"] } },
+      mode: 0755
+    )
+  end
+
+  context 'when manage_etc directory management is disabled' do
+    before { stub_gitlab_rb(manage_storage_directories: { enable: true, manage_etc: false } ) }
+
+    it 'does not create the user config directory' do
+      expect(chef_run).to_not create_directory('/etc/gitlab')
+    end
+  end
+end

spec/chef/recipes/gitlab-rails_spec.rb

@@ -0,0 +1,77 @@
+require 'chef_helper'
+
+describe 'gitlab::gitlab-rails' do
+  let(:chef_run) { ChefSpec::SoloRunner.converge('gitlab::default') }
+
+  before do
+    allow(Gitlab).to receive(:[]).and_call_original
+
+    # Prevent chef converge from reloading the helper library, which would override our helper stub
+    allow(Kernel).to receive(:load).and_call_original
+    allow(Kernel).to receive(:load).with(%r{gitlab/libraries/storage_directory_helper}).and_return(true)
+  end
+
+  context 'when manage-storage-directories is disabled' do
+    before do
+      stub_gitlab_rb(gitlab_rails: { shared_path: '/tmp/shared' }, manage_storage_directories: { enable: false })
+    end
+
+    it 'does not create the shared directory' do
+      expect(chef_run).to_not run_ruby_block('directory resource: /tmp/shared')
+    end
+
+    it 'does not create the artifacts directory' do
+      expect(chef_run).to_not run_ruby_block('directory resource: /tmp/shared/artifacts')
+    end
+
+    it 'does not create the lfs storage directory' do
+      expect(chef_run).to_not run_ruby_block('directory resource: /tmp/shared/lfs-objects')
+    end
+
+    it 'does not create the uploads storage directory' do
+      stub_gitlab_rb(gitlab_rails: { uploads_directory: '/tmp/uploads' })
+      expect(chef_run).to_not run_ruby_block('directory resource: /tmp/uploads')
+    end
+
+    it 'does not create the ci builds directory' do
+      stub_gitlab_rb(gitlab_ci: { builds_directory: '/tmp/builds' })
+      expect(chef_run).to_not run_ruby_block('directory resource: /tmp/builds')
+    end
+
+    it 'does not create the GitLab pages directory' do
+      expect(chef_run).to_not run_ruby_block('directory resource: /tmp/shared/pages')
+    end
+  end
+
+  context 'when manage-storage-directories is enabled' do
+    before do
+      stub_gitlab_rb(gitlab_rails: { shared_path: '/tmp/shared' } )
+    end
+
+    it 'creates the shared directory' do
+      expect(chef_run).to run_ruby_block('directory resource: /tmp/shared')
+    end
+
+    it 'creates the artifacts directory' do
+      expect(chef_run).to run_ruby_block('directory resource: /tmp/shared/artifacts')
+    end
+
+    it 'creates the lfs storage directory' do
+      expect(chef_run).to run_ruby_block('directory resource: /tmp/shared/lfs-objects')
+    end
+
+    it 'creates the uploads directory' do
+      stub_gitlab_rb(gitlab_rails: { uploads_directory: '/tmp/uploads' })
+      expect(chef_run).to run_ruby_block('directory resource: /tmp/uploads')
+    end
+
+    it 'creates the ci builds directory' do
+      stub_gitlab_rb(gitlab_ci: { builds_directory: '/tmp/builds' })
+      expect(chef_run).to run_ruby_block('directory resource: /tmp/builds')
+    end
+
+    it 'creates the GitLab pages directory' do
+      expect(chef_run).to run_ruby_block('directory resource: /tmp/shared/pages')
+    end
+  end
+end

spec/chef/recipes/gitlab-shell_spec.rb

@@ -3,7 +3,13 @@ require 'chef_helper'
 describe 'gitlab::gitlab-shell' do
   let(:chef_run) { ChefSpec::SoloRunner.converge('gitlab::default') }
 
-  before { allow(Gitlab).to receive(:[]).and_call_original }
+  before do
+    allow(Gitlab).to receive(:[]).and_call_original
+
+    # Prevent chef converge from reloading the storage helper library, which would override our helper stub
+    allow(Kernel).to receive(:load).and_call_original
+    allow(Kernel).to receive(:load).with(%r{gitlab/libraries/storage_directory_helper}).and_return(true)
+  end
 
   it 'calls into check permissions to create and validate the authorized_keys' do
     expect(chef_run).to run_execute('/opt/gitlab/embedded/service/gitlab-shell/bin/gitlab-keys check-permissions')
@@ -29,11 +35,7 @@ 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_directory('/tmp/user/.ssh').with(
-        user: 'git',
-        group: 'git',
-        mode: '0700'
-      )
+      expect(chef_run).to run_ruby_block('directory resource: /tmp/user/.ssh')
     end
 
     it 'creates the config file with the auth_file within user\'s ssh directory' do
@@ -46,19 +48,11 @@ 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_directory('/tmp/user/.ssh').with(
-        user: 'git',
-        group: 'git',
-        mode: '0700'
-      )
+      expect(chef_run).to run_ruby_block('directory resource: /tmp/user/.ssh')
     end
 
-    it 'creates the auth_file\'s parent directory with the correct permissions' do
-      expect(chef_run).to create_directory('/tmp/ssh').with(
-        user: 'git',
-        group: 'git',
-        mode: '0700'
-      )
+    it 'creates the auth_file\'s parent directory' do
+      expect(chef_run).to run_ruby_block('directory resource: /tmp/ssh')
     end
 
     it 'creates the config file with the auth_file at the specified location' do
@@ -67,6 +61,26 @@ describe 'gitlab::gitlab-shell' do
     end
   end
 
+  context 'when git_data_dir is moved' do
+    before { stub_gitlab_rb({git_data_dir: '/tmp/user/git-data'}) }
+
+    it 'creates the git data directories' do
+      expect(chef_run).to run_ruby_block('directory resource: /tmp/user/git-data')
+    end
+
+    it 'creates the git storage directories' do
+      expect(chef_run).to run_ruby_block('directory resource: /tmp/user/git-data/repositories')
+    end
+
+    it 'creates the ssh dir in the user\'s home directory' do
+      expect(chef_run).to run_ruby_block('directory resource: /var/opt/gitlab/.ssh')
+    end
+
+    it 'creates the auth_file\'s parent directory' do
+      stub_gitlab_rb(gitlab_shell: { auth_file: '/tmp/ssh/authorized_keys' })
+      expect(chef_run).to run_ruby_block('directory resource: /tmp/ssh')
+    end
+  end
   context 'with redis settings' do
     context 'and default configuration' do
       it 'creates the config file with the required redis settings' do

spec/chef_helper.rb

@@ -36,6 +36,7 @@ RSpec.configure do |config|
     stub_command(%r{\(test -f /var/opt/gitlab/gitlab-rails/upgrade-status/db-migrate-\h+-\) && \(cat /var/opt/gitlab/gitlab-rails/upgrade-status/db-migrate-\h+- | grep -Fx 0\)}).and_return('')
     stub_command("getenforce | grep Disabled").and_return(true)
     stub_command("semodule -l | grep '^#gitlab-7.2.0-ssh-keygen\\s'").and_return(true)
+    stub_command(%r{test "\$\(stat \-\-printf='[^']*' /[^\)]*\)" = 'directory }).and_return(false)
     allow_any_instance_of(Chef::Recipe).to receive(:system).with('/sbin/init --version | grep upstart')
     allow_any_instance_of(Chef::Recipe).to receive(:system).with('systemctl | grep "\-\.mount"')
   end