diff options
| -rw-r--r-- | changelogs/unreleased/ac-accelerate-wiki-attachments.yml | 5 | ||||
| -rw-r--r-- | lib/api/validations/types/workhorse_file.rb | 20 | ||||
| -rw-r--r-- | lib/api/wikis.rb | 22 | ||||
| -rw-r--r-- | lib/gitlab/middleware/multipart.rb | 18 | ||||
| -rw-r--r-- | spec/requests/api/wikis_spec.rb | 13 | ||||
| -rw-r--r-- | spec/requests/projects/uploads_spec.rb | 38 | ||||
| -rw-r--r-- | spec/support/helpers/workhorse_helpers.rb | 31 |
7 files changed, 137 insertions, 10 deletions
diff --git a/changelogs/unreleased/ac-accelerate-wiki-attachments.yml b/changelogs/unreleased/ac-accelerate-wiki-attachments.yml new file mode 100644 index 00000000000..347a570488e --- /dev/null +++ b/changelogs/unreleased/ac-accelerate-wiki-attachments.yml @@ -0,0 +1,5 @@ +--- +title: Preprocess wiki attachments with GitLab-Workhorse +merge_request: 32663 +author: +type: performance diff --git a/lib/api/validations/types/workhorse_file.rb b/lib/api/validations/types/workhorse_file.rb new file mode 100644 index 00000000000..18d111f6556 --- /dev/null +++ b/lib/api/validations/types/workhorse_file.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +module API + module Validations + module Types + class WorkhorseFile < Virtus::Attribute + def coerce(input) + # Processing of multipart file objects + # is already taken care of by Gitlab::Middleware::Multipart. + # Nothing to do here. + input + end + + def value_coerced?(value) + value.is_a?(::UploadedFile) + end + end + end + end +end diff --git a/lib/api/wikis.rb b/lib/api/wikis.rb index 5724adb2c40..c5a5488950d 100644 --- a/lib/api/wikis.rb +++ b/lib/api/wikis.rb @@ -4,11 +4,21 @@ module API class Wikis < Grape::API helpers do def commit_params(attrs) - { - file_name: attrs[:file][:filename], - file_content: attrs[:file][:tempfile].read, - branch_name: attrs[:branch] - } + # In order to avoid service disruption this can work with an old workhorse without the acceleration + # the first branch of this if must be removed when we drop support for non accelerated uploads + if attrs[:file].is_a?(Hash) + { + file_name: attrs[:file][:filename], + file_content: attrs[:file][:tempfile].read, + branch_name: attrs[:branch] + } + else + { + file_name: attrs[:file].original_filename, + file_content: attrs[:file].read, + branch_name: attrs[:branch] + } + end end params :common_wiki_page_params do @@ -106,7 +116,7 @@ module API success Entities::WikiAttachment end params do - requires :file, type: ::API::Validations::Types::SafeFile, desc: 'The attachment file to be uploaded' + requires :file, types: [::API::Validations::Types::SafeFile, ::API::Validations::Types::WorkhorseFile], desc: 'The attachment file to be uploaded' optional :branch, type: String, desc: 'The name of the branch' end post ":id/wikis/attachments" do diff --git a/lib/gitlab/middleware/multipart.rb b/lib/gitlab/middleware/multipart.rb index 86b28e4e20a..0ee9563c227 100644 --- a/lib/gitlab/middleware/multipart.rb +++ b/lib/gitlab/middleware/multipart.rb @@ -32,7 +32,7 @@ module Gitlab class Handler def initialize(env, message) - @request = ActionDispatch::Request.new(env) + @request = Rack::Request.new(env) @rewritten_fields = message['rewritten_fields'] @open_files = [] end @@ -50,7 +50,7 @@ module Gitlab value = decorate_params_value(value, @request.params[key]) end - @request.update_param(key, value) + update_param(key, value) end yield @@ -92,6 +92,20 @@ module Gitlab ::UploadedFile.from_params(params, key, allowed_paths) end + + # update_params ensures that both rails controllers and rack middleware can find + # workhorse accelerate files in the request + def update_param(key, value) + # we make sure we have key in POST otherwise update_params will add it in GET + @request.POST[key] ||= value + + # this will force Rack::Request to properly update env keys + @request.update_param(key, value) + + # ActionDispatch::Request is based on Rack::Request but it caches params + # inside other env keys, here we ensure everything is updated correctly + ActionDispatch::Request.new(@request.env).update_param(key, value) + end end def initialize(app) diff --git a/spec/requests/api/wikis_spec.rb b/spec/requests/api/wikis_spec.rb index d1b58aac104..97de26650db 100644 --- a/spec/requests/api/wikis_spec.rb +++ b/spec/requests/api/wikis_spec.rb @@ -11,6 +11,8 @@ require 'spec_helper' # because they are 3 edge cases of using wiki pages. describe API::Wikis do + include WorkhorseHelpers + let(:user) { create(:user) } let(:group) { create(:group).tap { |g| g.add_owner(user) } } let(:project_wiki) { create(:project_wiki, project: project, user: user) } @@ -155,7 +157,7 @@ describe API::Wikis do it 'pushes attachment to the wiki repository' do allow(SecureRandom).to receive(:hex).and_return('fixed_hex') - post(api(url, user), params: payload) + workhorse_post_with_file(api(url, user), file_key: :file, params: payload) expect(response).to have_gitlab_http_status(201) expect(json_response).to eq result_hash.deep_stringify_keys @@ -180,6 +182,15 @@ describe API::Wikis do expect(json_response.size).to eq(1) expect(json_response['error']).to eq('file is invalid') end + + it 'is backward compatible with regular multipart uploads' do + allow(SecureRandom).to receive(:hex).and_return('fixed_hex') + + post(api(url, user), params: payload) + + expect(response).to have_gitlab_http_status(201) + expect(json_response).to eq result_hash.deep_stringify_keys + end end describe 'GET /projects/:id/wikis' do diff --git a/spec/requests/projects/uploads_spec.rb b/spec/requests/projects/uploads_spec.rb new file mode 100644 index 00000000000..aca4644289d --- /dev/null +++ b/spec/requests/projects/uploads_spec.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe 'File uploads' do + include WorkhorseHelpers + + let(:project) { create(:project, :public, :repository) } + let(:user) { create(:user) } + + describe 'POST /:namespace/:project/create/:branch' do + let(:branch) { 'master' } + let(:create_url) { project_blob_path(project, branch) } + let(:blob_url) { project_blob_path(project, "#{branch}/dk.png") } + let(:params) do + { + namespace_id: project.namespace, + project_id: project, + id: branch, + branch_name: branch, + file: fixture_file_upload('spec/fixtures/dk.png'), + commit_message: 'Add an image' + } + end + + before do + project.add_maintainer(user) + + login_as(user) + end + + it 'redirects to blob' do + workhorse_post_with_file(create_url, file_key: :file, params: params) + + expect(response).to redirect_to(blob_url) + end + end +end diff --git a/spec/support/helpers/workhorse_helpers.rb b/spec/support/helpers/workhorse_helpers.rb index 4488e5f227e..fdbfe53fa39 100644 --- a/spec/support/helpers/workhorse_helpers.rb +++ b/spec/support/helpers/workhorse_helpers.rb @@ -17,7 +17,36 @@ module WorkhorseHelpers end def workhorse_internal_api_request_header - jwt_token = JWT.encode({ 'iss' => 'gitlab-workhorse' }, Gitlab::Workhorse.secret, 'HS256') { 'HTTP_' + Gitlab::Workhorse::INTERNAL_API_REQUEST_HEADER.upcase.tr('-', '_') => jwt_token } end + + # workhorse_post_with_file will transform file_key inside params as if it was disk accelerated by workhorse + def workhorse_post_with_file(url, file_key:, params:) + workhorse_params = params.dup + file = workhorse_params.delete(file_key) + + workhorse_params.merge!(workhorse_disk_accelerated_file_params(file_key, file)) + + post(url, + params: workhorse_params, + headers: workhorse_rewritten_fields_header('file' => file.path) + ) + end + + private + + def jwt_token(data = {}) + JWT.encode({ 'iss' => 'gitlab-workhorse' }.merge(data), Gitlab::Workhorse.secret, 'HS256') + end + + def workhorse_rewritten_fields_header(fields) + { Gitlab::Middleware::Multipart::RACK_ENV_KEY => jwt_token('rewritten_fields' => fields) } + end + + def workhorse_disk_accelerated_file_params(key, file) + { + "#{key}.name" => file.original_filename, + "#{key}.path" => file.path + } + end end |