From fe05fdae244d98e2e569f6553b6f6a04bc09c74d Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 7 Nov 2023 11:50:40 +1000 Subject: [PATCH] DEV: Introduce S3 transfer acceleration for uploads behind hidden setting (#24238) This commit adds an `enable_s3_transfer_acceleration` site setting, which is hidden to begin with. We are adding this because in certain regions, using https://aws.amazon.com/s3/transfer-acceleration/ can drastically speed up uploads, sometimes as much as 70% in certain regions depending on the target bucket region. This is important for us because we have direct S3 multipart uploads enabled everywhere on our hosting. To start, we only want this on the uploads bucket, not the backup one. Also, this will accelerate both uploads **and** downloads, depending on whether a presigned URL is used for downloading. This is the case when secure uploads is enabled, not anywhere else at this time. To enable the S3 acceleration on downloads more generally would be a more in-depth change, since we currently store S3 Upload record URLs like this: ``` url: "//test.s3.dualstack.us-east-2.amazonaws.com/original/2X/6/123456.png" ``` For acceleration, `s3.dualstack` would need to be changed to `s3-accelerate.dualstack` here. Note that for this to have any effect, Transfer Acceleration must be enabled on the S3 bucket used for uploads per https://docs.aws.amazon.com/AmazonS3/latest/userguide/transfer-acceleration-examples.html. --- config/discourse_defaults.conf | 1 + config/site_settings.yml | 3 ++ lib/file_store/s3_store.rb | 1 + lib/s3_helper.rb | 23 +++++++++++- spec/requests/uploads_controller_spec.rb | 47 ++++++++++++++++++++++-- 5 files changed, 70 insertions(+), 5 deletions(-) diff --git a/config/discourse_defaults.conf b/config/discourse_defaults.conf index b2d96e4bde0..ce589d63b84 100644 --- a/config/discourse_defaults.conf +++ b/config/discourse_defaults.conf @@ -225,6 +225,7 @@ s3_cdn_url = s3_endpoint = s3_http_continue_timeout = s3_install_cors_rule = +s3_enable_transfer_acceleration = # Optionally, specify a separate CDN to be used for static JS assets stored on S3 s3_asset_cdn_url = diff --git a/config/site_settings.yml b/config/site_settings.yml index cc79179ab49..75e5784e6a0 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1462,6 +1462,9 @@ files: enable_s3_uploads: default: false client: true + enable_s3_transfer_acceleration: + default: false + hidden: true s3_use_iam_profile: default: false s3_access_key_id: diff --git a/lib/file_store/s3_store.rb b/lib/file_store/s3_store.rb index 34f14dc9399..d89f3955ea7 100644 --- a/lib/file_store/s3_store.rb +++ b/lib/file_store/s3_store.rb @@ -25,6 +25,7 @@ module FileStore S3Helper.new( s3_bucket, Rails.configuration.multisite ? multisite_tombstone_prefix : TOMBSTONE_PREFIX, + use_accelerate_endpoint: SiteSetting.enable_s3_transfer_acceleration, ) end diff --git a/lib/s3_helper.rb b/lib/s3_helper.rb index 450dc25d777..93c9ca28841 100644 --- a/lib/s3_helper.rb +++ b/lib/s3_helper.rb @@ -47,6 +47,15 @@ class S3Helper setting_klass = use_db_s3_config ? SiteSetting : GlobalSetting options = S3Helper.s3_options(setting_klass) options[:client] = s3_client if s3_client.present? + use_accelerate_endpoint = + ( + if use_db_s3_config + SiteSetting.enable_s3_transfer_acceleration + else + GlobalSetting.s3_enable_transfer_acceleration + end + ) + options[:use_accelerate_endpoint] = !for_backup && use_accelerate_endpoint bucket = if for_backup @@ -349,7 +358,12 @@ class S3Helper def presigned_url(key, method:, expires_in: S3Helper::UPLOAD_URL_EXPIRES_AFTER_SECONDS, opts: {}) Aws::S3::Presigner.new(client: s3_client).presigned_url( method, - { bucket: s3_bucket_name, key: key, expires_in: expires_in }.merge(opts), + { + bucket: s3_bucket_name, + key: key, + expires_in: expires_in, + use_accelerate_endpoint: @s3_options[:use_accelerate_endpoint], + }.merge(opts), ) end @@ -362,7 +376,12 @@ class S3Helper ) Aws::S3::Presigner.new(client: s3_client).presigned_request( method, - { bucket: s3_bucket_name, key: key, expires_in: expires_in }.merge(opts), + { + bucket: s3_bucket_name, + key: key, + expires_in: expires_in, + use_accelerate_endpoint: @s3_options[:use_accelerate_endpoint], + }.merge(opts), ) end diff --git a/spec/requests/uploads_controller_spec.rb b/spec/requests/uploads_controller_spec.rb index 0c8e037e86a..daed9c6b818 100644 --- a/spec/requests/uploads_controller_spec.rb +++ b/spec/requests/uploads_controller_spec.rb @@ -780,6 +780,29 @@ RSpec.describe UploadsController do ) end + context "when enable_s3_transfer_acceleration is true" do + before { SiteSetting.enable_s3_transfer_acceleration = true } + + it "uses the s3-accelerate endpoint for presigned URLs" do + post "/uploads/generate-presigned-put.json", + **{ + params: { + file_name: "test.png", + file_size: 1024, + type: "card_background", + metadata: { + "sha1-checksum" => "testing", + "blah" => "wontbeincluded", + }, + }, + } + expect(response.status).to eq(200) + + result = response.parsed_body + expect(result["url"]).to include("s3-accelerate") + end + end + describe "rate limiting" do before { RateLimiter.enable } @@ -1045,7 +1068,7 @@ RSpec.describe UploadsController do XML stub_request( :get, - "https://s3-upload-bucket.s3.us-west-1.amazonaws.com/#{external_upload_stub.key}?max-parts=1&uploadId=#{mock_multipart_upload_id}", + "https://s3-upload-bucket.#{SiteSetting.enable_s3_transfer_acceleration ? "s3-accelerate" : "s3.us-west-1"}.amazonaws.com/#{external_upload_stub.key}?max-parts=1&uploadId=#{mock_multipart_upload_id}", ).to_return({ status: 200, body: list_multipart_result }) end @@ -1129,6 +1152,24 @@ RSpec.describe UploadsController do ) end + context "when enable_s3_transfer_acceleration is true" do + before { SiteSetting.enable_s3_transfer_acceleration = true } + + it "uses the s3-accelerate endpoint for presigned URLs" do + stub_list_multipart_request + post "/uploads/batch-presign-multipart-parts.json", + params: { + unique_identifier: external_upload_stub.unique_identifier, + part_numbers: [2, 3, 4], + } + + expect(response.status).to eq(200) + result = response.parsed_body + expect(result["presigned_urls"].keys).to eq(%w[2 3 4]) + expect(result["presigned_urls"]["2"]).to include("s3-accelerate") + end + end + describe "rate limiting" do before { RateLimiter.enable } @@ -1173,7 +1214,7 @@ RSpec.describe UploadsController do describe "#complete_multipart" do let(:upload_base_url) do - "https://#{SiteSetting.s3_upload_bucket}.s3.#{SiteSetting.s3_region}.amazonaws.com" + "https://#{SiteSetting.s3_upload_bucket}.#{SiteSetting.enable_s3_transfer_acceleration ? "s3-accelerate" : "s3.#{SiteSetting.s3_region}"}.amazonaws.com" end let(:mock_multipart_upload_id) do "ibZBv_75gd9r8lH_gqXatLdxMVpAlj6CFTR.OwyF3953YdwbcQnMA2BLGn8Lx12fQNICtMw5KyteFeHw.Sjng--" @@ -1396,7 +1437,7 @@ RSpec.describe UploadsController do describe "#abort_multipart" do let(:upload_base_url) do - "https://#{SiteSetting.s3_upload_bucket}.s3.#{SiteSetting.s3_region}.amazonaws.com" + "https://#{SiteSetting.s3_upload_bucket}.#{SiteSetting.enable_s3_transfer_acceleration ? "s3-accelerate" : "s3.#{SiteSetting.s3_region}"}.amazonaws.com" end let(:mock_multipart_upload_id) do "ibZBv_75gd9r8lH_gqXatLdxMVpAlj6CFTR.OwyF3953YdwbcQnMA2BLGn8Lx12fQNICtMw5KyteFeHw.Sjng--"