Commit cb8a425b authored by Stan Hu's avatar Stan Hu

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 to run execute directly on Projects::DestroyService.

Closes #17893
parent d4f987b2
...@@ -92,6 +92,7 @@ v 8.11.0 (unreleased) ...@@ -92,6 +92,7 @@ v 8.11.0 (unreleased)
- Bump gitlab_git to lazy load compare commits - Bump gitlab_git to lazy load compare commits
- Reduce number of queries made for merge_requests/:id/diffs - Reduce number of queries made for merge_requests/:id/diffs
- Sensible state specific default sort order for issues and merge requests !5453 (tomb0y) - 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 - Fix RequestProfiler::Middleware error when code is reloaded in development
- Catch what warden might throw when profiling requests to re-throw it - 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 - Avoid commit lookup on diff_helper passing existing local variable to the helper method
......
...@@ -48,9 +48,9 @@ class Admin::GroupsController < Admin::ApplicationController ...@@ -48,9 +48,9 @@ class Admin::GroupsController < Admin::ApplicationController
end end
def destroy 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 end
private private
......
...@@ -87,9 +87,9 @@ class GroupsController < Groups::ApplicationController ...@@ -87,9 +87,9 @@ class GroupsController < Groups::ApplicationController
end end
def destroy 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 end
protected protected
......
class Namespace < ActiveRecord::Base class Namespace < ActiveRecord::Base
acts_as_paranoid
include Sortable include Sortable
include Gitlab::ShellAdapter include Gitlab::ShellAdapter
......
...@@ -21,6 +21,11 @@ class DeleteUserService ...@@ -21,6 +21,11 @@ class DeleteUserService
::Projects::DestroyService.new(project, current_user, skip_repo: true).async_execute ::Projects::DestroyService.new(project, current_user, skip_repo: true).async_execute
end 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
end end
...@@ -5,13 +5,23 @@ class DestroyGroupService ...@@ -5,13 +5,23 @@ class DestroyGroupService
@group, @current_user = group, user @group, @current_user = group, user
end 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 def execute
group.projects.each do |project| 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 # Skip repository removal because we remove directory with namespace
# that contain all this repositories # that contain all these repositories
::Projects::DestroyService.new(project, current_user, skip_repo: true).async_execute ::Projects::DestroyService.new(project, current_user, skip_repo: true).execute
end end
group.destroy group.really_destroy!
end end
end end
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
# rubocop:disable all # rubocop:disable all
class FixNamespaces < ActiveRecord::Migration class FixNamespaces < ActiveRecord::Migration
DOWNTIME = false
def up def up
Namespace.where('name <> path and type is null').each do |namespace| namespaces = exec_query('SELECT id, path FROM namespaces WHERE name <> path and type is null')
namespace.update_attribute(:name, namespace.path)
namespaces.each do |row|
id = row['id']
path = row['path']
exec_query("UPDATE namespaces SET name = '#{path}' WHERE id = #{id}")
end end
end end
......
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
...@@ -640,9 +640,11 @@ ActiveRecord::Schema.define(version: 20160810142633) do ...@@ -640,9 +640,11 @@ ActiveRecord::Schema.define(version: 20160810142633) do
t.boolean "share_with_group_lock", default: false t.boolean "share_with_group_lock", default: false
t.integer "visibility_level", default: 20, null: false t.integer "visibility_level", default: 20, null: false
t.boolean "request_access_enabled", default: true, null: false t.boolean "request_access_enabled", default: true, null: false
t.datetime "deleted_at"
end end
add_index "namespaces", ["created_at"], name: "index_namespaces_on_created_at", using: :btree 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", unique: true, using: :btree
add_index "namespaces", ["name"], name: "index_namespaces_on_name_trigram", using: :gin, opclasses: {"name"=>"gin_trgm_ops"} 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 add_index "namespaces", ["owner_id"], name: "index_namespaces_on_owner_id", using: :btree
......
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
...@@ -75,4 +75,33 @@ describe GroupsController do ...@@ -75,4 +75,33 @@ describe GroupsController do
end end
end 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 end
...@@ -564,12 +564,14 @@ describe API::API, api: true do ...@@ -564,12 +564,14 @@ describe API::API, api: true do
end end
describe "DELETE /users/:id" do describe "DELETE /users/:id" do
let!(:namespace) { user.namespace }
before { admin } before { admin }
it "deletes user" do it "deletes user" do
delete api("/users/#{user.id}", admin) delete api("/users/#{user.id}", admin)
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
expect { User.find(user.id) }.to raise_error ActiveRecord::RecordNotFound 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) expect(json_response['email']).to eq(user.email)
end end
......
...@@ -9,9 +9,11 @@ describe DeleteUserService, services: true do ...@@ -9,9 +9,11 @@ describe DeleteUserService, services: true do
context 'no options are given' do context 'no options are given' do
it 'deletes the user' do it 'deletes the user' do
DeleteUserService.new(current_user).execute(user) user_data = DeleteUserService.new(current_user).execute(user)
expect { user_data['email'].to eq(user.email) }
expect { User.find(user.id) }.to raise_error(ActiveRecord::RecordNotFound) expect { User.find(user.id) }.to raise_error(ActiveRecord::RecordNotFound)
expect { Namespace.with_deleted.find(user.namespace.id) }.to raise_error(ActiveRecord::RecordNotFound)
end end
it 'will delete the project in the near future' do it 'will delete the project in the near future' do
......
...@@ -7,9 +7,10 @@ describe DestroyGroupService, services: true do ...@@ -7,9 +7,10 @@ describe DestroyGroupService, services: true do
let!(:gitlab_shell) { Gitlab::Shell.new } let!(:gitlab_shell) { Gitlab::Shell.new }
let!(:remove_path) { group.path + "+#{group.id}+deleted" } let!(:remove_path) { group.path + "+#{group.id}+deleted" }
shared_examples 'group destruction' do |async|
context 'database records' do context 'database records' do
before do before do
destroy_group(group, user) destroy_group(group, user, async)
end end
it { expect(Group.all).not_to include(group) } it { expect(Group.all).not_to include(group) }
...@@ -20,7 +21,7 @@ describe DestroyGroupService, services: true do ...@@ -20,7 +21,7 @@ describe DestroyGroupService, services: true do
context 'Sidekiq inline' do context 'Sidekiq inline' do
before do before do
# Run sidekiq immediatly to check that renamed dir will be removed # Run sidekiq immediatly to check that renamed dir will be removed
Sidekiq::Testing.inline! { destroy_group(group, user) } Sidekiq::Testing.inline! { destroy_group(group, user, async) }
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, group.path)).to be_falsey }
...@@ -30,7 +31,7 @@ describe DestroyGroupService, services: true do ...@@ -30,7 +31,7 @@ describe DestroyGroupService, services: true do
context 'Sidekiq fake' do context 'Sidekiq fake' do
before do before do
# Dont run sidekiq to check if renamed repository exists # Dont run sidekiq to check if renamed repository exists
Sidekiq::Testing.fake! { destroy_group(group, user) } Sidekiq::Testing.fake! { destroy_group(group, user, async) }
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, group.path)).to be_falsey }
...@@ -38,7 +39,20 @@ describe DestroyGroupService, services: true do ...@@ -38,7 +39,20 @@ describe DestroyGroupService, services: true do
end end
end end
def destroy_group(group, user) def destroy_group(group, user, async)
if async
DestroyGroupService.new(group, user).async_execute
else
DestroyGroupService.new(group, user).execute DestroyGroupService.new(group, user).execute
end end
end
end
describe 'asynchronous delete' do
it_behaves_like 'group destruction', true
end
describe 'synchronous delete' do
it_behaves_like 'group destruction', false
end
end end
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment