Commit 344d8cd6 authored by Nick Thomas's avatar Nick Thomas

Merge branch 'sh-workhorse-api-upload-acceleration' into 'master'

Accelerate uploads via API with Workhorse [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!57250
parents 9831778f 0f44f727
# frozen_string_literal: true # frozen_string_literal: true
class UploadService class UploadService
# Temporarily introduced for upload API: https://gitlab.com/gitlab-org/gitlab/-/issues/325788
attr_accessor :override_max_attachment_size
def initialize(model, file, uploader_class = FileUploader, **uploader_context) def initialize(model, file, uploader_class = FileUploader, **uploader_context)
@model, @file, @uploader_class, @uploader_context = model, file, uploader_class, uploader_context @model, @file, @uploader_class, @uploader_context = model, file, uploader_class, uploader_context
end end
...@@ -19,6 +22,6 @@ class UploadService ...@@ -19,6 +22,6 @@ class UploadService
attr_reader :model, :file, :uploader_class, :uploader_context attr_reader :model, :file, :uploader_class, :uploader_context
def max_attachment_size def max_attachment_size
Gitlab::CurrentSettings.max_attachment_size.megabytes.to_i override_max_attachment_size || Gitlab::CurrentSettings.max_attachment_size.megabytes.to_i
end end
end end
---
title: Accelerate uploads via API with Workhorse
merge_request: 57250
author:
type: performance
---
name: enforce_max_attachment_size_upload_api
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/57250
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/325787
milestone: '13.11'
type: development
group: group::source code
default_enabled: false
...@@ -13,6 +13,8 @@ module API ...@@ -13,6 +13,8 @@ module API
feature_category :projects, ['/projects/:id/custom_attributes', '/projects/:id/custom_attributes/:key'] feature_category :projects, ['/projects/:id/custom_attributes', '/projects/:id/custom_attributes/:key']
PROJECT_ATTACHMENT_SIZE_EXEMPT = 1.gigabyte
helpers do helpers do
# EE::API::Projects would override this method # EE::API::Projects would override this method
def apply_filters(projects) def apply_filters(projects)
...@@ -52,6 +54,19 @@ module API ...@@ -52,6 +54,19 @@ module API
accepted! accepted!
end end
def exempt_from_global_attachment_size?(user_project)
list = ::Gitlab::RackAttack::UserAllowlist.new(ENV['GITLAB_UPLOAD_API_ALLOWLIST'])
list.include?(user_project.id)
end
# Temporarily introduced for upload API: https://gitlab.com/gitlab-org/gitlab/-/issues/325788
def project_attachment_size(user_project)
return PROJECT_ATTACHMENT_SIZE_EXEMPT if exempt_from_global_attachment_size?(user_project)
return user_project.max_attachment_size if Feature.enabled?(:enforce_max_attachment_size_upload_api, user_project)
PROJECT_ATTACHMENT_SIZE_EXEMPT
end
end end
helpers do helpers do
...@@ -545,13 +560,25 @@ module API ...@@ -545,13 +560,25 @@ module API
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
desc 'Workhorse authorize the file upload' do
detail 'This feature was introduced in GitLab 13.11'
end
post ':id/uploads/authorize', feature_category: :not_owned do
require_gitlab_workhorse!
status 200
content_type Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE
FileUploader.workhorse_authorize(has_length: false, maximum_size: project_attachment_size(user_project))
end
desc 'Upload a file' desc 'Upload a file'
params do params do
# TODO: remove rubocop disable - https://gitlab.com/gitlab-org/gitlab/issues/14960 requires :file, types: [Rack::Multipart::UploadedFile, ::API::Validations::Types::WorkhorseFile], desc: 'The attachment file to be uploaded'
requires :file, type: File, desc: 'The file to be uploaded' # rubocop:disable Scalability/FileUploads
end end
post ":id/uploads", feature_category: :not_owned do post ":id/uploads", feature_category: :not_owned do
upload = UploadService.new(user_project, params[:file]).execute service = UploadService.new(user_project, params[:file])
service.override_max_attachment_size = project_attachment_size(user_project)
upload = service.execute
present upload, with: Entities::ProjectUpload present upload, with: Entities::ProjectUpload
end end
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe 'Upload an attachment', :api, :js do
include_context 'file upload requests helpers'
let_it_be(:project) { create(:project) }
let_it_be(:user) { project.owner }
let_it_be(:personal_access_token) { create(:personal_access_token, user: user) }
let(:api_path) { "/projects/#{project_id}/uploads" }
let(:url) { capybara_url(api(api_path)) }
let(:file) { fixture_file_upload('spec/fixtures/dk.png') }
subject do
HTTParty.post(
url,
headers: { 'PRIVATE-TOKEN' => personal_access_token.token },
body: { file: file }
)
end
shared_examples 'for an attachment' do
it 'creates files' do
expect { subject }
.to change { Upload.count }.by(1)
end
it { expect(subject.code).to eq(201) }
end
context 'with an integer project ID' do
let(:project_id) { project.id }
it_behaves_like 'handling file uploads', 'for an attachment'
end
context 'with an encoded project ID' do
let(:project_id) { "#{project.namespace.path}%2F#{project.path}" }
it_behaves_like 'handling file uploads', 'for an attachment'
end
end
...@@ -1461,12 +1461,73 @@ RSpec.describe API::Projects do ...@@ -1461,12 +1461,73 @@ RSpec.describe API::Projects do
end end
end end
describe "POST /projects/:id/uploads/authorize" do
include WorkhorseHelpers
let(:headers) { workhorse_internal_api_request_header.merge({ 'HTTP_GITLAB_WORKHORSE' => 1 }) }
context 'with authorized user' do
it "returns 200" do
post api("/projects/#{project.id}/uploads/authorize", user), headers: headers
expect(response).to have_gitlab_http_status(:ok)
expect(json_response['MaximumSize']).to eq(project.max_attachment_size)
end
end
context 'with unauthorized user' do
it "returns 404" do
post api("/projects/#{project.id}/uploads/authorize", user2), headers: headers
expect(response).to have_gitlab_http_status(:not_found)
end
end
context 'with exempted project' do
before do
stub_env('GITLAB_UPLOAD_API_ALLOWLIST', project.id)
end
it "returns 200" do
post api("/projects/#{project.id}/uploads/authorize", user), headers: headers
expect(response).to have_gitlab_http_status(:ok)
expect(json_response['MaximumSize']).to eq(1.gigabyte)
end
end
context 'with upload size enforcement disabled' do
before do
stub_feature_flags(enforce_max_attachment_size_upload_api: false)
end
it "returns 200" do
post api("/projects/#{project.id}/uploads/authorize", user), headers: headers
expect(response).to have_gitlab_http_status(:ok)
expect(json_response['MaximumSize']).to eq(1.gigabyte)
end
end
context 'with no Workhorse headers' do
it "returns 403" do
post api("/projects/#{project.id}/uploads/authorize", user)
expect(response).to have_gitlab_http_status(:forbidden)
end
end
end
describe "POST /projects/:id/uploads" do describe "POST /projects/:id/uploads" do
before do before do
project project
end end
it "uploads the file and returns its info" do it "uploads the file and returns its info" do
expect_next_instance_of(UploadService) do |instance|
expect(instance).to receive(:override_max_attachment_size=).with(project.max_attachment_size).and_call_original
end
post api("/projects/#{project.id}/uploads", user), params: { file: fixture_file_upload("spec/fixtures/dk.png", "image/png") } post api("/projects/#{project.id}/uploads", user), params: { file: fixture_file_upload("spec/fixtures/dk.png", "image/png") }
expect(response).to have_gitlab_http_status(:created) expect(response).to have_gitlab_http_status(:created)
...@@ -1476,6 +1537,34 @@ RSpec.describe API::Projects do ...@@ -1476,6 +1537,34 @@ RSpec.describe API::Projects do
expect(json_response['full_path']).to start_with("/#{project.namespace.path}/#{project.path}/uploads") expect(json_response['full_path']).to start_with("/#{project.namespace.path}/#{project.path}/uploads")
end end
shared_examples 'capped upload attachments' do
it "limits the upload to 1 GB" do
expect_next_instance_of(UploadService) do |instance|
expect(instance).to receive(:override_max_attachment_size=).with(1.gigabyte).and_call_original
end
post api("/projects/#{project.id}/uploads", user), params: { file: fixture_file_upload("spec/fixtures/dk.png", "image/png") }
expect(response).to have_gitlab_http_status(:created)
end
end
context 'with exempted project' do
before do
stub_env('GITLAB_UPLOAD_API_ALLOWLIST', project.id)
end
it_behaves_like 'capped upload attachments'
end
context 'with upload size enforcement disabled' do
before do
stub_feature_flags(enforce_max_attachment_size_upload_api: false)
end
it_behaves_like 'capped upload attachments'
end
end end
describe "GET /projects/:id/groups" do describe "GET /projects/:id/groups" do
......
...@@ -67,6 +67,29 @@ RSpec.describe UploadService do ...@@ -67,6 +67,29 @@ RSpec.describe UploadService do
it { expect(@link_to_file).to eq({}) } it { expect(@link_to_file).to eq({}) }
end end
describe '#override_max_attachment_size' do
let(:txt) { fixture_file_upload('spec/fixtures/doc_sample.txt', 'text/plain') }
let(:service) { described_class.new(@project, txt, FileUploader) }
subject { service.execute.to_h }
before do
allow(txt).to receive(:size) { 100.megabytes.to_i }
end
it 'allows the upload' do
service.override_max_attachment_size = 101.megabytes
expect(subject.keys).to eq(%i(alt url markdown))
end
it 'disallows the upload' do
service.override_max_attachment_size = 99.megabytes
expect(subject).to eq({})
end
end
end end
def upload_file(project, file) def upload_file(project, file)
......
...@@ -302,6 +302,9 @@ func configureRoutes(u *upstream) { ...@@ -302,6 +302,9 @@ func configureRoutes(u *upstream) {
// Requirements Import via UI upload acceleration // Requirements Import via UI upload acceleration
u.route("POST", projectPattern+`requirements_management/requirements/import_csv`, upload.Accelerate(api, signingProxy, preparers.uploads)), u.route("POST", projectPattern+`requirements_management/requirements/import_csv`, upload.Accelerate(api, signingProxy, preparers.uploads)),
// Uploads via API
u.route("POST", apiProjectPattern+`uploads\z`, upload.Accelerate(api, signingProxy, preparers.uploads)),
// Explicitly proxy API requests // Explicitly proxy API requests
u.route("", apiPattern, proxy), u.route("", apiPattern, proxy),
u.route("", ciAPIPattern, proxy), u.route("", ciAPIPattern, proxy),
......
...@@ -116,6 +116,9 @@ func TestAcceleratedUpload(t *testing.T) { ...@@ -116,6 +116,9 @@ func TestAcceleratedUpload(t *testing.T) {
{"POST", `/example`, false}, {"POST", `/example`, false},
{"POST", `/uploads/personal_snippet`, true}, {"POST", `/uploads/personal_snippet`, true},
{"POST", `/uploads/user`, true}, {"POST", `/uploads/user`, true},
{"POST", `/api/v4/projects/1/uploads`, true},
{"POST", `/api/v4/projects/group%2Fproject/uploads`, true},
{"POST", `/api/v4/projects/group%2Fsubgroup%2Fproject/uploads`, true},
{"POST", `/api/v4/projects/1/wikis/attachments`, false}, {"POST", `/api/v4/projects/1/wikis/attachments`, false},
{"POST", `/api/v4/projects/group%2Fproject/wikis/attachments`, false}, {"POST", `/api/v4/projects/group%2Fproject/wikis/attachments`, false},
{"POST", `/api/v4/projects/group%2Fsubgroup%2Fproject/wikis/attachments`, false}, {"POST", `/api/v4/projects/group%2Fsubgroup%2Fproject/wikis/attachments`, false},
......
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