From 07009a1f4893652e152794ae8160a2f46e00772c Mon Sep 17 00:00:00 2001 From: James Lopez Date: Thu, 26 Jul 2018 12:55:21 +0200 Subject: [PATCH] Add Object Storage to GitLab project import - Refactor uploads manager - Refactor importer, update import spec - Add more object storage specs --- app/models/import_export_upload.rb | 1 + .../gitlab_projects_import_service.rb | 3 +- app/uploaders/import_export_uploader.rb | 2 +- ...oject-import-should-use-object-storage.yml | 5 + lib/gitlab/import_export/command_line_util.rb | 15 +++ lib/gitlab/import_export/file_importer.rb | 24 +++- lib/gitlab/import_export/importer.rb | 12 +- lib/gitlab/import_export/uploads_manager.rb | 5 +- lib/gitlab/template_helper.rb | 14 ++- .../import_file_object_storage_spec.rb | 103 ++++++++++++++++ .../import_export/import_file_spec.rb | 1 + .../file_importer_object_storage_spec.rb | 89 ++++++++++++++ .../import_export/file_importer_spec.rb | 4 +- .../importer_object_storage_spec.rb | 115 ++++++++++++++++++ .../lib/gitlab/import_export/importer_spec.rb | 5 +- spec/requests/api/project_import_spec.rb | 2 + .../gitlab_projects_import_service_spec.rb | 2 +- 17 files changed, 380 insertions(+), 22 deletions(-) create mode 100644 changelogs/unreleased/48773-gitlab-project-import-should-use-object-storage.yml create mode 100644 spec/features/projects/import_export/import_file_object_storage_spec.rb create mode 100644 spec/lib/gitlab/import_export/file_importer_object_storage_spec.rb create mode 100644 spec/lib/gitlab/import_export/importer_object_storage_spec.rb diff --git a/app/models/import_export_upload.rb b/app/models/import_export_upload.rb index 40795d7c42f..f0cc5aafcd4 100644 --- a/app/models/import_export_upload.rb +++ b/app/models/import_export_upload.rb @@ -6,6 +6,7 @@ class ImportExportUpload < ActiveRecord::Base belongs_to :project + # These hold the project Import/Export archives (.tar.gz files) mount_uploader :import_file, ImportExportUploader mount_uploader :export_file, ImportExportUploader diff --git a/app/services/projects/gitlab_projects_import_service.rb b/app/services/projects/gitlab_projects_import_service.rb index 615dccc4685..044afa1d5e1 100644 --- a/app/services/projects/gitlab_projects_import_service.rb +++ b/app/services/projects/gitlab_projects_import_service.rb @@ -15,7 +15,7 @@ module Projects end def execute - prepare_template_environment(template_file&.path) + prepare_template_environment(template_file) prepare_import_params @@ -61,7 +61,6 @@ module Projects if template_file params[:import_type] = 'gitlab_project' - params[:import_source] = import_upload_path end params[:import_data] = { data: data } if data.present? diff --git a/app/uploaders/import_export_uploader.rb b/app/uploaders/import_export_uploader.rb index 7c45ba5ca95..716922bc017 100644 --- a/app/uploaders/import_export_uploader.rb +++ b/app/uploaders/import_export_uploader.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class ImportExportUploader < AttachmentUploader - EXTENSION_WHITELIST = %w[tar.gz].freeze + EXTENSION_WHITELIST = %w[tar.gz gz].freeze def extension_whitelist EXTENSION_WHITELIST diff --git a/changelogs/unreleased/48773-gitlab-project-import-should-use-object-storage.yml b/changelogs/unreleased/48773-gitlab-project-import-should-use-object-storage.yml new file mode 100644 index 00000000000..f298380b920 --- /dev/null +++ b/changelogs/unreleased/48773-gitlab-project-import-should-use-object-storage.yml @@ -0,0 +1,5 @@ +--- +title: Add object storage logic to project import +merge_request: 20773 +author: +type: added diff --git a/lib/gitlab/import_export/command_line_util.rb b/lib/gitlab/import_export/command_line_util.rb index 2f163db936b..3adc44f8044 100644 --- a/lib/gitlab/import_export/command_line_util.rb +++ b/lib/gitlab/import_export/command_line_util.rb @@ -18,6 +18,21 @@ module Gitlab private + def download_or_copy_upload(uploader, upload_path) + if uploader.upload.local? + copy_files(uploader.path, upload_path) + else + download(uploader.url, upload_path) + end + end + + def download(url, upload_path) + File.open(upload_path, 'w') do |file| + # Download (stream) file from the uploader's location + IO.copy_stream(URI.parse(url).open, file) + end + end + def tar_with_options(archive:, dir:, options:) execute(%W(tar -#{options} #{archive} -C #{dir} .)) end diff --git a/lib/gitlab/import_export/file_importer.rb b/lib/gitlab/import_export/file_importer.rb index 4c411f4847e..7fd66b4e244 100644 --- a/lib/gitlab/import_export/file_importer.rb +++ b/lib/gitlab/import_export/file_importer.rb @@ -10,15 +10,18 @@ module Gitlab new(*args).import end - def initialize(archive_file:, shared:) + def initialize(project:, archive_file:, shared:) + @project = project @archive_file = archive_file @shared = shared end def import mkdir_p(@shared.export_path) + mkdir_p(@shared.archive_path) - remove_symlinks! + remove_symlinks + copy_archive wait_for_archived_file do decompress_archive @@ -27,7 +30,8 @@ module Gitlab @shared.error(e) false ensure - remove_symlinks! + remove_import_file + remove_symlinks end private @@ -51,7 +55,15 @@ module Gitlab result end - def remove_symlinks! + def copy_archive + return if @archive_file + + @archive_file = File.join(@shared.archive_path, Gitlab::ImportExport.export_filename(project: @project)) + + download_or_copy_upload(@project.import_export_upload.import_file, @archive_file) + end + + def remove_symlinks extracted_files.each do |path| FileUtils.rm(path) if File.lstat(path).symlink? end @@ -59,6 +71,10 @@ module Gitlab true end + def remove_import_file + FileUtils.rm_rf(@archive_file) + end + def extracted_files Dir.glob("#{@shared.export_path}/**/*", File::FNM_DOTMATCH).reject { |f| IGNORED_FILENAMES.include?(File.basename(f)) } end diff --git a/lib/gitlab/import_export/importer.rb b/lib/gitlab/import_export/importer.rb index 63cab07324a..4e179f63d8c 100644 --- a/lib/gitlab/import_export/importer.rb +++ b/lib/gitlab/import_export/importer.rb @@ -35,7 +35,8 @@ module Gitlab end def import_file - Gitlab::ImportExport::FileImporter.import(archive_file: @archive_file, + Gitlab::ImportExport::FileImporter.import(project: @project, + archive_file: @archive_file, shared: @shared) end @@ -91,7 +92,14 @@ module Gitlab end def remove_import_file - FileUtils.rm_rf(@archive_file) + return unless Gitlab::ImportExport.object_storage? + + upload = @project.import_export_upload + + return unless upload&.import_file&.file + + upload.remove_import_file! + upload.save! end def overwrite_project diff --git a/lib/gitlab/import_export/uploads_manager.rb b/lib/gitlab/import_export/uploads_manager.rb index 1110149712d..07875ebb56a 100644 --- a/lib/gitlab/import_export/uploads_manager.rb +++ b/lib/gitlab/import_export/uploads_manager.rb @@ -91,10 +91,7 @@ module Gitlab mkdir_p(File.join(uploads_export_path, secret)) - File.open(upload_path, 'w') do |file| - # Download (stream) file from the uploader's location - IO.copy_stream(URI.parse(upload.file.url).open, file) - end + download_or_copy_upload(upload, upload_path) end end end diff --git a/lib/gitlab/template_helper.rb b/lib/gitlab/template_helper.rb index 3b8e45e0688..3918abaab45 100644 --- a/lib/gitlab/template_helper.rb +++ b/lib/gitlab/template_helper.rb @@ -2,11 +2,17 @@ module Gitlab module TemplateHelper include Gitlab::Utils::StrongMemoize - def prepare_template_environment(file_path) - return unless file_path.present? + def prepare_template_environment(file) + return unless file&.path.present? - FileUtils.mkdir_p(File.dirname(import_upload_path)) - FileUtils.copy_entry(file_path, import_upload_path) + if Gitlab::ImportExport.object_storage? + params[:import_export_upload] = ImportExportUpload.new(import_file: file) + else + FileUtils.mkdir_p(File.dirname(import_upload_path)) + FileUtils.copy_entry(file.path, import_upload_path) + + params[:import_source] = import_upload_path + end end def import_upload_path diff --git a/spec/features/projects/import_export/import_file_object_storage_spec.rb b/spec/features/projects/import_export/import_file_object_storage_spec.rb new file mode 100644 index 00000000000..0d364543916 --- /dev/null +++ b/spec/features/projects/import_export/import_file_object_storage_spec.rb @@ -0,0 +1,103 @@ +require 'spec_helper' + +describe 'Import/Export - project import integration test', :js do + include Select2Helper + + let(:user) { create(:user) } + let(:file) { File.join(Rails.root, 'spec', 'features', 'projects', 'import_export', 'test_project_export.tar.gz') } + let(:export_path) { "#{Dir.tmpdir}/import_file_spec" } + + before do + stub_feature_flags(import_export_object_storage: true) + stub_uploads_object_storage(FileUploader) + allow_any_instance_of(Gitlab::ImportExport).to receive(:storage_path).and_return(export_path) + gitlab_sign_in(user) + end + + after do + FileUtils.rm_rf(export_path, secure: true) + end + + context 'when selecting the namespace' do + let(:user) { create(:admin) } + let!(:namespace) { user.namespace } + let(:project_path) { 'test-project-path' + SecureRandom.hex } + + context 'prefilled the path' do + it 'user imports an exported project successfully' do + visit new_project_path + + select2(namespace.id, from: '#project_namespace_id') + fill_in :project_path, with: project_path, visible: true + click_import_project_tab + click_link 'GitLab export' + + expect(page).to have_content('Import an exported GitLab project') + expect(URI.parse(current_url).query).to eq("namespace_id=#{namespace.id}&path=#{project_path}") + + attach_file('file', file) + click_on 'Import project' + + expect(Project.count).to eq(1) + + project = Project.last + expect(project).not_to be_nil + expect(project.description).to eq("Foo Bar") + expect(project.issues).not_to be_empty + expect(project.merge_requests).not_to be_empty + expect(project_hook_exists?(project)).to be true + expect(wiki_exists?(project)).to be true + expect(project.import_state.status).to eq('finished') + end + end + + context 'path is not prefilled' do + it 'user imports an exported project successfully' do + visit new_project_path + click_import_project_tab + click_link 'GitLab export' + + fill_in :path, with: 'test-project-path', visible: true + attach_file('file', file) + + expect { click_on 'Import project' }.to change { Project.count }.by(1) + + project = Project.last + expect(project).not_to be_nil + expect(page).to have_content("Project 'test-project-path' is being imported") + end + end + end + + it 'invalid project' do + project = create(:project, namespace: user.namespace) + + visit new_project_path + + select2(user.namespace.id, from: '#project_namespace_id') + fill_in :project_path, with: project.name, visible: true + click_import_project_tab + click_link 'GitLab export' + attach_file('file', file) + click_on 'Import project' + + page.within('.flash-container') do + expect(page).to have_content('Project could not be imported') + end + end + + def wiki_exists?(project) + wiki = ProjectWiki.new(project) + wiki.repository.exists? && !wiki.repository.empty? + end + + def project_hook_exists?(project) + Gitlab::GitalyClient::StorageSettings.allow_disk_access do + Gitlab::Git::Hook.new('post-receive', project.repository.raw_repository).exists? + end + end + + def click_import_project_tab + find('#import-project-tab').click + end +end diff --git a/spec/features/projects/import_export/import_file_spec.rb b/spec/features/projects/import_export/import_file_spec.rb index 9cbfb62d872..2d86115de12 100644 --- a/spec/features/projects/import_export/import_file_spec.rb +++ b/spec/features/projects/import_export/import_file_spec.rb @@ -8,6 +8,7 @@ describe 'Import/Export - project import integration test', :js do let(:export_path) { "#{Dir.tmpdir}/import_file_spec" } before do + stub_feature_flags(import_export_object_storage: false) allow_any_instance_of(Gitlab::ImportExport).to receive(:storage_path).and_return(export_path) gitlab_sign_in(user) end diff --git a/spec/lib/gitlab/import_export/file_importer_object_storage_spec.rb b/spec/lib/gitlab/import_export/file_importer_object_storage_spec.rb new file mode 100644 index 00000000000..287745eb40e --- /dev/null +++ b/spec/lib/gitlab/import_export/file_importer_object_storage_spec.rb @@ -0,0 +1,89 @@ +require 'spec_helper' + +describe Gitlab::ImportExport::FileImporter do + let(:shared) { Gitlab::ImportExport::Shared.new(nil) } + let(:storage_path) { "#{Dir.tmpdir}/file_importer_spec" } + let(:valid_file) { "#{shared.export_path}/valid.json" } + let(:symlink_file) { "#{shared.export_path}/invalid.json" } + let(:hidden_symlink_file) { "#{shared.export_path}/.hidden" } + let(:subfolder_symlink_file) { "#{shared.export_path}/subfolder/invalid.json" } + let(:evil_symlink_file) { "#{shared.export_path}/.\nevil" } + + before do + stub_const('Gitlab::ImportExport::FileImporter::MAX_RETRIES', 0) + stub_feature_flags(import_export_object_storage: true) + stub_uploads_object_storage(FileUploader) + + allow_any_instance_of(Gitlab::ImportExport).to receive(:storage_path).and_return(storage_path) + allow_any_instance_of(Gitlab::ImportExport::CommandLineUtil).to receive(:untar_zxf).and_return(true) + allow_any_instance_of(Gitlab::ImportExport::Shared).to receive(:relative_archive_path).and_return('test') + allow(SecureRandom).to receive(:hex).and_return('abcd') + setup_files + end + + after do + FileUtils.rm_rf(storage_path) + end + + context 'normal run' do + before do + described_class.import(project: build(:project), archive_file: '', shared: shared) + end + + it 'removes symlinks in root folder' do + expect(File.exist?(symlink_file)).to be false + end + + it 'removes hidden symlinks in root folder' do + expect(File.exist?(hidden_symlink_file)).to be false + end + + it 'removes evil symlinks in root folder' do + expect(File.exist?(evil_symlink_file)).to be false + end + + it 'removes symlinks in subfolders' do + expect(File.exist?(subfolder_symlink_file)).to be false + end + + it 'does not remove a valid file' do + expect(File.exist?(valid_file)).to be true + end + + it 'creates the file in the right subfolder' do + expect(shared.export_path).to include('test/abcd') + end + end + + context 'error' do + before do + allow_any_instance_of(described_class).to receive(:wait_for_archived_file).and_raise(StandardError) + described_class.import(project: build(:project), archive_file: '', shared: shared) + end + + it 'removes symlinks in root folder' do + expect(File.exist?(symlink_file)).to be false + end + + it 'removes hidden symlinks in root folder' do + expect(File.exist?(hidden_symlink_file)).to be false + end + + it 'removes symlinks in subfolders' do + expect(File.exist?(subfolder_symlink_file)).to be false + end + + it 'does not remove a valid file' do + expect(File.exist?(valid_file)).to be true + end + end + + def setup_files + FileUtils.mkdir_p("#{shared.export_path}/subfolder/") + FileUtils.touch(valid_file) + FileUtils.ln_s(valid_file, symlink_file) + FileUtils.ln_s(valid_file, subfolder_symlink_file) + FileUtils.ln_s(valid_file, hidden_symlink_file) + FileUtils.ln_s(valid_file, evil_symlink_file) + end +end diff --git a/spec/lib/gitlab/import_export/file_importer_spec.rb b/spec/lib/gitlab/import_export/file_importer_spec.rb index 265937f899e..78fccdf1dfc 100644 --- a/spec/lib/gitlab/import_export/file_importer_spec.rb +++ b/spec/lib/gitlab/import_export/file_importer_spec.rb @@ -24,7 +24,7 @@ describe Gitlab::ImportExport::FileImporter do context 'normal run' do before do - described_class.import(archive_file: '', shared: shared) + described_class.import(project: nil, archive_file: '', shared: shared) end it 'removes symlinks in root folder' do @@ -55,7 +55,7 @@ describe Gitlab::ImportExport::FileImporter do context 'error' do before do allow_any_instance_of(described_class).to receive(:wait_for_archived_file).and_raise(StandardError) - described_class.import(archive_file: '', shared: shared) + described_class.import(project: nil, archive_file: '', shared: shared) end it 'removes symlinks in root folder' do diff --git a/spec/lib/gitlab/import_export/importer_object_storage_spec.rb b/spec/lib/gitlab/import_export/importer_object_storage_spec.rb new file mode 100644 index 00000000000..24a994b3611 --- /dev/null +++ b/spec/lib/gitlab/import_export/importer_object_storage_spec.rb @@ -0,0 +1,115 @@ +require 'spec_helper' + +describe Gitlab::ImportExport::Importer do + let(:user) { create(:user) } + let(:test_path) { "#{Dir.tmpdir}/importer_spec" } + let(:shared) { project.import_export_shared } + let(:project) { create(:project) } + let(:import_file) { fixture_file_upload('spec/features/projects/import_export/test_project_export.tar.gz') } + + subject(:importer) { described_class.new(project) } + + before do + allow_any_instance_of(Gitlab::ImportExport).to receive(:storage_path).and_return(test_path) + allow_any_instance_of(Gitlab::ImportExport::FileImporter).to receive(:remove_import_file) + stub_feature_flags(import_export_object_storage: true) + stub_uploads_object_storage(FileUploader) + + FileUtils.mkdir_p(shared.export_path) + ImportExportUpload.create(project: project, import_file: import_file) + end + + after do + FileUtils.rm_rf(test_path) + end + + describe '#execute' do + it 'succeeds' do + importer.execute + + expect(shared.errors).to be_empty + end + + it 'extracts the archive' do + expect(Gitlab::ImportExport::FileImporter).to receive(:import).and_call_original + + importer.execute + end + + it 'checks the version' do + expect(Gitlab::ImportExport::VersionChecker).to receive(:check!).and_call_original + + importer.execute + end + + context 'all restores are executed' do + [ + Gitlab::ImportExport::AvatarRestorer, + Gitlab::ImportExport::RepoRestorer, + Gitlab::ImportExport::WikiRestorer, + Gitlab::ImportExport::UploadsRestorer, + Gitlab::ImportExport::LfsRestorer, + Gitlab::ImportExport::StatisticsRestorer + ].each do |restorer| + it "calls the #{restorer}" do + fake_restorer = double(restorer.to_s) + + expect(fake_restorer).to receive(:restore).and_return(true).at_least(1) + expect(restorer).to receive(:new).and_return(fake_restorer).at_least(1) + + importer.execute + end + end + + it 'restores the ProjectTree' do + expect(Gitlab::ImportExport::ProjectTreeRestorer).to receive(:new).and_call_original + + importer.execute + end + + it 'removes the import file' do + expect(importer).to receive(:remove_import_file).and_call_original + + importer.execute + + expect(project.import_export_upload.import_file&.file).to be_nil + end + end + + context 'when project successfully restored' do + let!(:existing_project) { create(:project, namespace: user.namespace) } + let(:project) { create(:project, namespace: user.namespace, name: 'whatever', path: 'whatever') } + + before do + restorers = double(:restorers, all?: true) + + allow(subject).to receive(:import_file).and_return(true) + allow(subject).to receive(:check_version!).and_return(true) + allow(subject).to receive(:restorers).and_return(restorers) + allow(project).to receive(:import_data).and_return(double(data: { 'original_path' => existing_project.path })) + end + + context 'when import_data' do + context 'has original_path' do + it 'overwrites existing project' do + expect_any_instance_of(::Projects::OverwriteProjectService).to receive(:execute).with(existing_project) + + subject.execute + end + end + + context 'has not original_path' do + before do + allow(project).to receive(:import_data).and_return(double(data: {})) + end + + it 'does not call the overwrite service' do + expect_any_instance_of(::Projects::OverwriteProjectService).not_to receive(:execute).with(existing_project) + + subject.execute + end + end + end + end + end +end diff --git a/spec/lib/gitlab/import_export/importer_spec.rb b/spec/lib/gitlab/import_export/importer_spec.rb index c074e61da26..f07946824c4 100644 --- a/spec/lib/gitlab/import_export/importer_spec.rb +++ b/spec/lib/gitlab/import_export/importer_spec.rb @@ -10,9 +10,10 @@ describe Gitlab::ImportExport::Importer do before do allow_any_instance_of(Gitlab::ImportExport).to receive(:storage_path).and_return(test_path) + allow_any_instance_of(Gitlab::ImportExport::FileImporter).to receive(:remove_import_file) + FileUtils.mkdir_p(shared.export_path) FileUtils.cp(Rails.root.join('spec/features/projects/import_export/test_project_export.tar.gz'), test_path) - allow(subject).to receive(:remove_import_file) end after do @@ -69,7 +70,7 @@ describe Gitlab::ImportExport::Importer do let(:project) { create(:project, namespace: user.namespace, name: 'whatever', path: 'whatever') } before do - restorers = double + restorers = double(:restorers, all?: true) allow(subject).to receive(:import_file).and_return(true) allow(subject).to receive(:check_version!).and_return(true) diff --git a/spec/requests/api/project_import_spec.rb b/spec/requests/api/project_import_spec.rb index 55332f56508..e3fb6cecce9 100644 --- a/spec/requests/api/project_import_spec.rb +++ b/spec/requests/api/project_import_spec.rb @@ -7,6 +7,8 @@ describe API::ProjectImport do let(:namespace) { create(:group) } before do allow_any_instance_of(Gitlab::ImportExport).to receive(:storage_path).and_return(export_path) + stub_feature_flags(import_export_object_storage: true) + stub_uploads_object_storage(FileUploader) namespace.add_owner(user) end diff --git a/spec/services/projects/gitlab_projects_import_service_spec.rb b/spec/services/projects/gitlab_projects_import_service_spec.rb index a2061127698..b5f2c826c97 100644 --- a/spec/services/projects/gitlab_projects_import_service_spec.rb +++ b/spec/services/projects/gitlab_projects_import_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Projects::GitlabProjectsImportService do set(:namespace) { create(:namespace) } let(:path) { 'test-path' } - let(:file) { fixture_file_upload('spec/fixtures/doc_sample.txt', 'text/plain') } + let(:file) { fixture_file_upload('spec/fixtures/project_export.tar.gz') } let(:overwrite) { false } let(:import_params) { { namespace_id: namespace.id, path: path, file: file, overwrite: overwrite } }