diff --git a/app/models/site_setting.rb b/app/models/site_setting.rb index e635d85b1f3..c5742cb6b45 100644 --- a/app/models/site_setting.rb +++ b/app/models/site_setting.rb @@ -168,6 +168,10 @@ class SiteSetting < ActiveRecord::Base end end + def self.use_dualstack_endpoint + !SiteSetting.Upload.s3_region.start_with?("cn-") + end + def self.enable_s3_uploads SiteSetting.enable_s3_uploads || GlobalSetting.use_s3? end @@ -190,10 +194,10 @@ class SiteSetting < ActiveRecord::Base # cf. http://docs.aws.amazon.com/general/latest/gr/rande.html#s3_region if SiteSetting.s3_endpoint.blank? || SiteSetting.s3_endpoint.end_with?("amazonaws.com") - if SiteSetting.Upload.s3_region.start_with?("cn-") - "//#{bucket}.s3.#{SiteSetting.Upload.s3_region}.amazonaws.com.cn" - else + if SiteSetting.Upload.use_dualstack_endpoint "//#{bucket}.s3.dualstack.#{SiteSetting.Upload.s3_region}.amazonaws.com" + else + "//#{bucket}.s3.#{SiteSetting.Upload.s3_region}.amazonaws.com.cn" end else "//#{bucket}.#{url_basename}" diff --git a/lib/s3_helper.rb b/lib/s3_helper.rb index d362756c0b3..5cd05c50c6e 100644 --- a/lib/s3_helper.rb +++ b/lib/s3_helper.rb @@ -50,6 +50,7 @@ class S3Helper options[:client] = s3_client if s3_client.present? options[:use_accelerate_endpoint] = !for_backup && SiteSetting.Upload.enable_s3_transfer_acceleration + options[:use_dualstack_endpoint] = SiteSetting.Upload.use_dualstack_endpoint bucket = if for_backup @@ -265,6 +266,7 @@ class S3Helper opts[:endpoint] = SiteSetting.s3_endpoint if SiteSetting.s3_endpoint.present? opts[:http_continue_timeout] = SiteSetting.s3_http_continue_timeout + opts[:use_dualstack_endpoint] = SiteSetting.Upload.use_dualstack_endpoint unless obj.s3_use_iam_profile opts[:access_key_id] = obj.s3_access_key_id @@ -362,6 +364,7 @@ class S3Helper key: key, expires_in: expires_in, use_accelerate_endpoint: @s3_options[:use_accelerate_endpoint], + use_dualstack_endpoint: @s3_options[:use_dualstack_endpoint], }.merge(opts), ) end @@ -380,6 +383,7 @@ class S3Helper key: key, expires_in: expires_in, use_accelerate_endpoint: @s3_options[:use_accelerate_endpoint], + use_dualstack_endpoint: @s3_options[:use_dualstack_endpoint], }.merge(opts), ) end diff --git a/spec/lib/backup_restore/s3_backup_store_spec.rb b/spec/lib/backup_restore/s3_backup_store_spec.rb index 1d94b55932b..bb3ecd2ca95 100644 --- a/spec/lib/backup_restore/s3_backup_store_spec.rb +++ b/spec/lib/backup_restore/s3_backup_store_spec.rb @@ -82,9 +82,8 @@ RSpec.describe BackupRestore::S3BackupStore do end, ) + setup_s3 SiteSetting.s3_backup_bucket = "s3-backup-bucket" - SiteSetting.s3_access_key_id = "s3-access-key-id" - SiteSetting.s3_secret_access_key = "s3-secret-access-key" SiteSetting.backup_location = BackupLocationSiteSetting::S3 end diff --git a/spec/lib/backup_restore/shared_examples_for_backup_store.rb b/spec/lib/backup_restore/shared_examples_for_backup_store.rb index 3c43e8e8c1f..39f6c11eb3f 100644 --- a/spec/lib/backup_restore/shared_examples_for_backup_store.rb +++ b/spec/lib/backup_restore/shared_examples_for_backup_store.rb @@ -171,16 +171,14 @@ RSpec.shared_examples "backup store" do end it "runs if SiteSetting.automatic_backups_enabled? is true" do - stub_request( - :get, - "https://s3-backup-bucket.s3.amazonaws.com/?list-type=2&prefix=default/", - ).to_return(status: 200, body: "", headers: {}) - stub_request(:head, "https://s3-backup-bucket.s3.amazonaws.com/").to_return( + base_backup_s3_url = "https://s3-backup-bucket.s3.dualstack.us-west-1.amazonaws.com" + stub_request(:get, "#{base_backup_s3_url}/?list-type=2&prefix=default/").to_return( status: 200, body: "", headers: { }, ) + stub_request(:head, "#{base_backup_s3_url}/").to_return(status: 200, body: "", headers: {}) SiteSetting.automatic_backups_enabled = true scheduleBackup = Jobs::ScheduleBackup.new diff --git a/spec/lib/s3_helper_spec.rb b/spec/lib/s3_helper_spec.rb index 30705d7cc47..97aa29efb3c 100644 --- a/spec/lib/s3_helper_spec.rb +++ b/spec/lib/s3_helper_spec.rb @@ -41,10 +41,10 @@ RSpec.describe "S3Helper" do stub_request( :get, - "https://bob.s3.#{SiteSetting.s3_region}.amazonaws.com/?lifecycle", + "https://bob.s3.dualstack.#{SiteSetting.s3_region}.amazonaws.com/?lifecycle", ).to_return(status: 200, body: @lifecycle, headers: {}) - stub_request(:put, "https://bob.s3.#{SiteSetting.s3_region}.amazonaws.com/?lifecycle") + stub_request(:put, "https://bob.s3.dualstack.#{SiteSetting.s3_region}.amazonaws.com/?lifecycle") .with do |req| hash = Hash.from_xml(req.body.to_s) rules = hash["LifecycleConfiguration"]["Rule"] @@ -248,4 +248,42 @@ RSpec.describe "S3Helper" do s3_helper.delete_objects(%w[object/one.txt object/two.txt]) end end + + describe "#presigned_url" do + let(:s3_helper) { S3Helper.new("test-bucket", "", client: client) } + + it "uses the S3 dualstack endpoint" do + expect(s3_helper.presigned_url("test/key.jpeg", method: :get_object)).to include("dualstack") + end + + context "for a China S3 region" do + before { SiteSetting.s3_region = "cn-northwest-1" } + + it "does not use the S3 dualstack endpoint" do + expect(s3_helper.presigned_url("test/key.jpeg", method: :get_object)).not_to include( + "dualstack", + ) + end + end + end + + describe "#presigned_request" do + let(:s3_helper) { S3Helper.new("test-bucket", "", client: client) } + + it "uses the S3 dualstack endpoint" do + expect(s3_helper.presigned_request("test/key.jpeg", method: :get_object)[0]).to include( + "dualstack", + ) + end + + context "for a China S3 region" do + before { SiteSetting.s3_region = "cn-northwest-1" } + + it "does not use the S3 dualstack endpoint" do + expect(s3_helper.presigned_request("test/key.jpeg", method: :get_object)[0]).not_to include( + "dualstack", + ) + end + end + end end diff --git a/spec/models/optimized_image_spec.rb b/spec/models/optimized_image_spec.rb index 4b3c6eb9624..4a6f206bcbb 100644 --- a/spec/models/optimized_image_spec.rb +++ b/spec/models/optimized_image_spec.rb @@ -334,7 +334,7 @@ RSpec.describe OptimizedImage do ) stub_request( :put, - %r{https://#{SiteSetting.s3_upload_bucket}\.s3\.#{SiteSetting.s3_region}\.amazonaws.com#{optimized_path}}, + %r{https://#{SiteSetting.s3_upload_bucket}\.s3\.dualstack\.#{SiteSetting.s3_region}\.amazonaws.com#{optimized_path}}, ).to_return(status: 200, headers: { "ETag" => "someetag" }) end diff --git a/spec/requests/admin/backups_controller_spec.rb b/spec/requests/admin/backups_controller_spec.rb index 49cf2d78530..031e2ed21b4 100644 --- a/spec/requests/admin/backups_controller_spec.rb +++ b/spec/requests/admin/backups_controller_spec.rb @@ -880,15 +880,13 @@ RSpec.describe Admin::BackupsController do SiteSetting.enable_direct_s3_uploads = true SiteSetting.s3_backup_bucket = "s3-backup-bucket" SiteSetting.backup_location = BackupLocationSiteSetting::S3 - stub_request(:head, "https://s3-backup-bucket.s3.us-west-1.amazonaws.com/").to_return( - status: 200, - body: "", - headers: { - }, - ) stub_request( :head, - "https://s3-backup-bucket.s3.us-west-1.amazonaws.com/default/test.tar.gz", + "https://s3-backup-bucket.s3.dualstack.us-west-1.amazonaws.com/", + ).to_return(status: 200, body: "", headers: {}) + stub_request( + :head, + "https://s3-backup-bucket.s3.dualstack.us-west-1.amazonaws.com/default/test.tar.gz", ).to_return(backup_file_exists_response) end @@ -936,7 +934,7 @@ RSpec.describe Admin::BackupsController do XML stub_request( :post, - "https://s3-backup-bucket.s3.us-west-1.amazonaws.com/temp/default/#{test_bucket_prefix}/28fccf8259bbe75b873a2bd2564b778c/2u98j832nx93272x947823.gz?uploads", + "https://s3-backup-bucket.s3.dualstack.us-west-1.amazonaws.com/temp/default/#{test_bucket_prefix}/28fccf8259bbe75b873a2bd2564b778c/2u98j832nx93272x947823.gz?uploads", ).to_return(status: 200, body: create_multipart_result) end diff --git a/spec/requests/uploads_controller_spec.rb b/spec/requests/uploads_controller_spec.rb index d198a065ef4..da6a6ab9570 100644 --- a/spec/requests/uploads_controller_spec.rb +++ b/spec/requests/uploads_controller_spec.rb @@ -874,6 +874,7 @@ RSpec.describe UploadsController do expect(result["key"]).to include(FileStore::S3Store::TEMPORARY_UPLOAD_PREFIX) expect(result["url"]).to include(FileStore::S3Store::TEMPORARY_UPLOAD_PREFIX) expect(result["url"]).to include("Amz-Expires") + expect(result["url"]).to include("dualstack") end it "includes accepted metadata in the response when provided" do @@ -1040,7 +1041,7 @@ RSpec.describe UploadsController do XML stub_request( :post, - "https://s3-upload-bucket.s3.us-west-1.amazonaws.com/uploads/default/#{test_bucket_prefix}/temp/28fccf8259bbe75b873a2bd2564b778c/test.png?uploads", + "https://s3-upload-bucket.s3.dualstack.us-west-1.amazonaws.com/uploads/default/#{test_bucket_prefix}/temp/28fccf8259bbe75b873a2bd2564b778c/test.png?uploads", ).to_return({ status: 200, body: create_multipart_result }) end @@ -1184,7 +1185,7 @@ RSpec.describe UploadsController do XML stub_request( :get, - "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}", + "https://s3-upload-bucket.#{SiteSetting.enable_s3_transfer_acceleration ? "s3-accelerate.dualstack" : "s3.dualstack.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 @@ -1268,6 +1269,19 @@ RSpec.describe UploadsController do ) end + it "uses dualstack endpoint for presigned URLs based on S3 region" 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"]["2"]).to include("dualstack") + end + context "when enable_s3_transfer_acceleration is true" do before { SiteSetting.enable_s3_transfer_acceleration = true } @@ -1328,7 +1342,7 @@ RSpec.describe UploadsController do describe "#complete_multipart" do let(:upload_base_url) do - "https://#{SiteSetting.s3_upload_bucket}.#{SiteSetting.enable_s3_transfer_acceleration ? "s3-accelerate" : "s3.#{SiteSetting.s3_region}"}.amazonaws.com" + "https://#{SiteSetting.s3_upload_bucket}.#{SiteSetting.enable_s3_transfer_acceleration ? "s3-accelerate.dualstack" : "s3.dualstack.#{SiteSetting.s3_region}"}.amazonaws.com" end let(:mock_multipart_upload_id) do "ibZBv_75gd9r8lH_gqXatLdxMVpAlj6CFTR.OwyF3953YdwbcQnMA2BLGn8Lx12fQNICtMw5KyteFeHw.Sjng--" @@ -1549,7 +1563,7 @@ RSpec.describe UploadsController do describe "#abort_multipart" do let(:upload_base_url) do - "https://#{SiteSetting.s3_upload_bucket}.#{SiteSetting.enable_s3_transfer_acceleration ? "s3-accelerate" : "s3.#{SiteSetting.s3_region}"}.amazonaws.com" + "https://#{SiteSetting.s3_upload_bucket}.#{SiteSetting.enable_s3_transfer_acceleration ? "s3-accelerate.dualstack" : "s3.dualstack.#{SiteSetting.s3_region}"}.amazonaws.com" end let(:mock_multipart_upload_id) do "ibZBv_75gd9r8lH_gqXatLdxMVpAlj6CFTR.OwyF3953YdwbcQnMA2BLGn8Lx12fQNICtMw5KyteFeHw.Sjng--" diff --git a/spec/support/system_helpers.rb b/spec/support/system_helpers.rb index 6e6ee1e30c8..184992bd3fc 100644 --- a/spec/support/system_helpers.rb +++ b/spec/support/system_helpers.rb @@ -139,6 +139,10 @@ module SystemHelpers SiteSetting.s3_secret_access_key = MinioRunner.config.minio_root_password SiteSetting.s3_endpoint = MinioRunner.config.minio_server_url + # This is necessary for Minio because you cannot use dualstack + # at the same time as using a custom S3 endpoint. + SiteSetting.Upload.stubs(:use_dualstack_endpoint).returns(false) + SiteSetting.enable_direct_s3_uploads = enable_direct_s3_uploads SiteSetting.secure_uploads = enable_secure_uploads diff --git a/spec/support/uploads_helpers.rb b/spec/support/uploads_helpers.rb index b307a52d975..0d338de479c 100644 --- a/spec/support/uploads_helpers.rb +++ b/spec/support/uploads_helpers.rb @@ -10,9 +10,11 @@ module UploadsHelpers SiteSetting.s3_access_key_id = "some key" SiteSetting.s3_secret_access_key = "some secrets3_region key" + dualstack = SiteSetting.Upload.use_dualstack_endpoint ? ".dualstack" : "" + stub_request( :head, - "https://#{SiteSetting.s3_upload_bucket}.s3.#{SiteSetting.s3_region}.amazonaws.com/", + "https://#{SiteSetting.s3_upload_bucket}.s3#{dualstack}.#{SiteSetting.s3_region}.amazonaws.com/", ) end @@ -22,8 +24,10 @@ module UploadsHelpers end def stub_upload(upload) + dualstack = SiteSetting.Upload.use_dualstack_endpoint ? ".dualstack" : "" + url = - %r{https://#{SiteSetting.s3_upload_bucket}.s3.#{SiteSetting.s3_region}.amazonaws.com/original/\d+X.*#{upload.sha1}.#{upload.extension}\?acl} + %r{https://#{SiteSetting.s3_upload_bucket}.s3#{dualstack}.#{SiteSetting.s3_region}.amazonaws.com/original/\d+X.*#{upload.sha1}.#{upload.extension}\?acl} stub_request(:put, url) end