summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--changelogs/unreleased/ac-accelerate-wiki-attachments.yml5
-rw-r--r--lib/api/validations/types/workhorse_file.rb20
-rw-r--r--lib/api/wikis.rb22
-rw-r--r--lib/gitlab/middleware/multipart.rb18
-rw-r--r--spec/requests/api/wikis_spec.rb13
-rw-r--r--spec/requests/projects/uploads_spec.rb38
-rw-r--r--spec/support/helpers/workhorse_helpers.rb31
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