diff options
author | Robert Speicher <robert@gitlab.com> | 2016-08-12 21:04:03 +0300 |
---|---|---|
committer | Robert Speicher <robert@gitlab.com> | 2016-08-12 21:04:03 +0300 |
commit | 11eefba891f214eefc1efa334adbcc9e979c0ce3 (patch) | |
tree | 69dc1a835658ba03d05264aee00207e67c178658 | |
parent | 7b63f8289c5e9a811215a3b26ccd5e0e793a9653 (diff) | |
parent | cb8a425ba42e9be23b8712ed29b1db2dcc6bd139 (diff) |
Merge branch 'fix-namespace-deletion' into 'master'
Fix bug where destroying a namespace would not always destroy projects
There is a race condition in DestroyGroupService now that projects are deleted asynchronously:
1. User attempts to delete group
2. DestroyGroupService iterates through all projects and schedules a Sidekiq job to delete each Project
3. DestroyGroupService destroys the Group, leaving all its projects without a namespace
4. Projects::DestroyService runs later but the can?(current_user,
:remove_project) is `false` because the user no longer has permission to
destroy projects with no namespace.
5. This leaves the project in pending_delete state with no namespace/group.
Projects without a namespace or group also adds another problem: it's not possible to destroy the container registry tags, since `container_registry_path_with_namespace` is the wrong value.
The fix is to destroy the group asynchronously and run `execute` directly on Projects::DestroyService.
Closes #17893
See merge request !4341
-rw-r--r-- | CHANGELOG | 1 | ||||
-rw-r--r-- | app/controllers/admin/groups_controller.rb | 4 | ||||
-rw-r--r-- | app/controllers/groups_controller.rb | 4 | ||||
-rw-r--r-- | app/models/namespace.rb | 2 | ||||
-rw-r--r-- | app/services/delete_user_service.rb | 7 | ||||
-rw-r--r-- | app/services/destroy_group_service.rb | 16 | ||||
-rw-r--r-- | app/workers/group_destroy_worker.rb | 17 | ||||
-rw-r--r-- | db/migrate/20140407135544_fix_namespaces.rb | 10 | ||||
-rw-r--r-- | db/migrate/20160805041956_add_deleted_at_to_namespaces.rb | 12 | ||||
-rw-r--r-- | db/schema.rb | 2 | ||||
-rw-r--r-- | spec/controllers/admin/groups_controller_spec.rb | 24 | ||||
-rw-r--r-- | spec/controllers/groups_controller_spec.rb | 29 | ||||
-rw-r--r-- | spec/requests/api/users_spec.rb | 2 | ||||
-rw-r--r-- | spec/services/delete_user_service_spec.rb | 6 | ||||
-rw-r--r-- | spec/services/destroy_group_service_spec.rb | 58 | ||||
-rw-r--r-- | spec/workers/group_destroy_worker_spec.rb | 19 |
16 files changed, 179 insertions, 34 deletions
diff --git a/CHANGELOG b/CHANGELOG index 6fe1720796d..96965a20f69 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -93,6 +93,7 @@ v 8.11.0 (unreleased) - Bump gitlab_git to lazy load compare commits - Reduce number of queries made for merge_requests/:id/diffs - Sensible state specific default sort order for issues and merge requests !5453 (tomb0y) + - Fix bug where destroying a namespace would not always destroy projects - Fix RequestProfiler::Middleware error when code is reloaded in development - Catch what warden might throw when profiling requests to re-throw it - Avoid commit lookup on diff_helper passing existing local variable to the helper method diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb index f3a88a8e6c8..4ce18321649 100644 --- a/app/controllers/admin/groups_controller.rb +++ b/app/controllers/admin/groups_controller.rb @@ -48,9 +48,9 @@ class Admin::GroupsController < Admin::ApplicationController end def destroy - DestroyGroupService.new(@group, current_user).execute + DestroyGroupService.new(@group, current_user).async_execute - redirect_to admin_groups_path, notice: 'Group was successfully deleted.' + redirect_to admin_groups_path, alert: "Group '#{@group.name}' was scheduled for deletion." end private diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 6780a6d4d87..cb82d62616c 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -87,9 +87,9 @@ class GroupsController < Groups::ApplicationController end def destroy - DestroyGroupService.new(@group, current_user).execute + DestroyGroupService.new(@group, current_user).async_execute - redirect_to root_path, alert: "Group '#{@group.name}' was successfully deleted." + redirect_to root_path, alert: "Group '#{@group.name}' was scheduled for deletion." end protected diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 8b52cc824cd..7c29d27ce97 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -1,4 +1,6 @@ class Namespace < ActiveRecord::Base + acts_as_paranoid + include Sortable include Gitlab::ShellAdapter diff --git a/app/services/delete_user_service.rb b/app/services/delete_user_service.rb index 2f237de813c..eaff88d6463 100644 --- a/app/services/delete_user_service.rb +++ b/app/services/delete_user_service.rb @@ -21,6 +21,11 @@ class DeleteUserService ::Projects::DestroyService.new(project, current_user, skip_repo: true).async_execute end - user.destroy + # Destroy the namespace after destroying the user since certain methods may depend on the namespace existing + namespace = user.namespace + user_data = user.destroy + namespace.really_destroy! + + user_data end end diff --git a/app/services/destroy_group_service.rb b/app/services/destroy_group_service.rb index a4ebccb5606..0081364b8aa 100644 --- a/app/services/destroy_group_service.rb +++ b/app/services/destroy_group_service.rb @@ -5,13 +5,23 @@ class DestroyGroupService @group, @current_user = group, user end + def async_execute + group.transaction do + # Soft delete via paranoia gem + group.destroy + job_id = GroupDestroyWorker.perform_async(group.id, current_user.id) + Rails.logger.info("User #{current_user.id} scheduled a deletion of group ID #{group.id} with job ID #{job_id}") + end + end + def execute group.projects.each do |project| + # Execute the destruction of the models immediately to ensure atomic cleanup. # Skip repository removal because we remove directory with namespace - # that contain all this repositories - ::Projects::DestroyService.new(project, current_user, skip_repo: true).async_execute + # that contain all these repositories + ::Projects::DestroyService.new(project, current_user, skip_repo: true).execute end - group.destroy + group.really_destroy! end end diff --git a/app/workers/group_destroy_worker.rb b/app/workers/group_destroy_worker.rb new file mode 100644 index 00000000000..5048746f09b --- /dev/null +++ b/app/workers/group_destroy_worker.rb @@ -0,0 +1,17 @@ +class GroupDestroyWorker + include Sidekiq::Worker + + sidekiq_options queue: :default + + def perform(group_id, user_id) + begin + group = Group.with_deleted.find(group_id) + rescue ActiveRecord::RecordNotFound + return + end + + user = User.find(user_id) + + DestroyGroupService.new(group, user).execute + end +end diff --git a/db/migrate/20140407135544_fix_namespaces.rb b/db/migrate/20140407135544_fix_namespaces.rb index 91374966698..0026ce645a6 100644 --- a/db/migrate/20140407135544_fix_namespaces.rb +++ b/db/migrate/20140407135544_fix_namespaces.rb @@ -1,8 +1,14 @@ # rubocop:disable all class FixNamespaces < ActiveRecord::Migration + DOWNTIME = false + def up - Namespace.where('name <> path and type is null').each do |namespace| - namespace.update_attribute(:name, namespace.path) + namespaces = exec_query('SELECT id, path FROM namespaces WHERE name <> path and type is null') + + namespaces.each do |row| + id = row['id'] + path = row['path'] + exec_query("UPDATE namespaces SET name = '#{path}' WHERE id = #{id}") end end diff --git a/db/migrate/20160805041956_add_deleted_at_to_namespaces.rb b/db/migrate/20160805041956_add_deleted_at_to_namespaces.rb new file mode 100644 index 00000000000..a853de3abfb --- /dev/null +++ b/db/migrate/20160805041956_add_deleted_at_to_namespaces.rb @@ -0,0 +1,12 @@ +class AddDeletedAtToNamespaces < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def change + add_column :namespaces, :deleted_at, :datetime + add_concurrent_index :namespaces, :deleted_at + end +end diff --git a/db/schema.rb b/db/schema.rb index 6c85e1e9dba..1de2cdcf026 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -640,9 +640,11 @@ ActiveRecord::Schema.define(version: 20160810142633) do t.boolean "share_with_group_lock", default: false t.integer "visibility_level", default: 20, null: false t.boolean "request_access_enabled", default: true, null: false + t.datetime "deleted_at" end add_index "namespaces", ["created_at"], name: "index_namespaces_on_created_at", using: :btree + add_index "namespaces", ["deleted_at"], name: "index_namespaces_on_deleted_at", using: :btree add_index "namespaces", ["name"], name: "index_namespaces_on_name", unique: true, using: :btree add_index "namespaces", ["name"], name: "index_namespaces_on_name_trigram", using: :gin, opclasses: {"name"=>"gin_trgm_ops"} add_index "namespaces", ["owner_id"], name: "index_namespaces_on_owner_id", using: :btree diff --git a/spec/controllers/admin/groups_controller_spec.rb b/spec/controllers/admin/groups_controller_spec.rb new file mode 100644 index 00000000000..0239aea47fb --- /dev/null +++ b/spec/controllers/admin/groups_controller_spec.rb @@ -0,0 +1,24 @@ +require 'spec_helper' + +describe Admin::GroupsController do + let(:group) { create(:group) } + let(:project) { create(:project, namespace: group) } + let(:admin) { create(:admin) } + + before do + sign_in(admin) + Sidekiq::Testing.fake! + end + + describe 'DELETE #destroy' do + it 'schedules a group destroy' do + expect { delete :destroy, id: project.group.path }.to change(GroupDestroyWorker.jobs, :size).by(1) + end + + it 'redirects to the admin group path' do + delete :destroy, id: project.group.path + + expect(response).to redirect_to(admin_groups_path) + end + end +end diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index cd98fecd0c7..4ae6364207b 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -75,4 +75,33 @@ describe GroupsController do end end end + + describe 'DELETE #destroy' do + context 'as another user' do + it 'returns 404' do + sign_in(create(:user)) + + delete :destroy, id: group.path + + expect(response.status).to eq(404) + end + end + + context 'as the group owner' do + before do + Sidekiq::Testing.fake! + sign_in(user) + end + + it 'schedules a group destroy' do + expect { delete :destroy, id: group.path }.to change(GroupDestroyWorker.jobs, :size).by(1) + end + + it 'redirects to the root path' do + delete :destroy, id: group.path + + expect(response).to redirect_to(root_path) + end + end + end end diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index e0e041b4e15..0bbba64a6d5 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -564,12 +564,14 @@ describe API::API, api: true do end describe "DELETE /users/:id" do + let!(:namespace) { user.namespace } before { admin } it "deletes user" do delete api("/users/#{user.id}", admin) expect(response).to have_http_status(200) expect { User.find(user.id) }.to raise_error ActiveRecord::RecordNotFound + expect { Namespace.find(namespace.id) }.to raise_error ActiveRecord::RecordNotFound expect(json_response['email']).to eq(user.email) end diff --git a/spec/services/delete_user_service_spec.rb b/spec/services/delete_user_service_spec.rb index 630458f9efc..418a12a83a9 100644 --- a/spec/services/delete_user_service_spec.rb +++ b/spec/services/delete_user_service_spec.rb @@ -9,9 +9,11 @@ describe DeleteUserService, services: true do context 'no options are given' do it 'deletes the user' do - DeleteUserService.new(current_user).execute(user) + user_data = DeleteUserService.new(current_user).execute(user) - expect { User.find(user.id) }.to raise_error(ActiveRecord::RecordNotFound) + expect { user_data['email'].to eq(user.email) } + expect { User.find(user.id) }.to raise_error(ActiveRecord::RecordNotFound) + expect { Namespace.with_deleted.find(user.namespace.id) }.to raise_error(ActiveRecord::RecordNotFound) end it 'will delete the project in the near future' do diff --git a/spec/services/destroy_group_service_spec.rb b/spec/services/destroy_group_service_spec.rb index eca8ddd8ea4..da724643604 100644 --- a/spec/services/destroy_group_service_spec.rb +++ b/spec/services/destroy_group_service_spec.rb @@ -7,38 +7,52 @@ describe DestroyGroupService, services: true do let!(:gitlab_shell) { Gitlab::Shell.new } let!(:remove_path) { group.path + "+#{group.id}+deleted" } - context 'database records' do - before do - destroy_group(group, user) + shared_examples 'group destruction' do |async| + context 'database records' do + before do + destroy_group(group, user, async) + end + + it { expect(Group.all).not_to include(group) } + it { expect(Project.all).not_to include(project) } end - it { expect(Group.all).not_to include(group) } - it { expect(Project.all).not_to include(project) } - end + context 'file system' do + context 'Sidekiq inline' do + before do + # Run sidekiq immediatly to check that renamed dir will be removed + Sidekiq::Testing.inline! { destroy_group(group, user, async) } + end - context 'file system' do - context 'Sidekiq inline' do - before do - # Run sidekiq immediatly to check that renamed dir will be removed - Sidekiq::Testing.inline! { destroy_group(group, user) } + it { expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey } + it { expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_falsey } end - it { expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey } - it { expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_falsey } - end + context 'Sidekiq fake' do + before do + # Dont run sidekiq to check if renamed repository exists + Sidekiq::Testing.fake! { destroy_group(group, user, async) } + end - context 'Sidekiq fake' do - before do - # Dont run sidekiq to check if renamed repository exists - Sidekiq::Testing.fake! { destroy_group(group, user) } + it { expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey } + it { expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_truthy } end + end - it { expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey } - it { expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_truthy } + def destroy_group(group, user, async) + if async + DestroyGroupService.new(group, user).async_execute + else + DestroyGroupService.new(group, user).execute + end end end - def destroy_group(group, user) - DestroyGroupService.new(group, user).execute + describe 'asynchronous delete' do + it_behaves_like 'group destruction', true + end + + describe 'synchronous delete' do + it_behaves_like 'group destruction', false end end diff --git a/spec/workers/group_destroy_worker_spec.rb b/spec/workers/group_destroy_worker_spec.rb new file mode 100644 index 00000000000..4e4eaf9b2f7 --- /dev/null +++ b/spec/workers/group_destroy_worker_spec.rb @@ -0,0 +1,19 @@ +require 'spec_helper' + +describe GroupDestroyWorker do + let(:group) { create(:group) } + let(:user) { create(:admin) } + let!(:project) { create(:project, namespace: group) } + + subject { GroupDestroyWorker.new } + + describe "#perform" do + it "deletes the project" do + subject.perform(group.id, user.id) + + expect(Group.all).not_to include(group) + expect(Project.all).not_to include(project) + expect(Dir.exist?(project.path)).to be_falsey + end + end +end |