From 0568d36133081e52f25f05585c1a568c3b828d79 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 7 Nov 2024 11:06:39 +1000 Subject: [PATCH] FIX: Use dualstack S3 endpoint for direct uploads (#29611) When we added direct S3 uploads to Discourse, which use presigned URLs, we never took into account the dualstack endpoints for IPv6 on S3. This commit fixes the issue by using the dualstack endpoints for presigned URLs and requests, which are used in the get-presigned-put and batch-presign-urls endpoints used when directly uploading to S3. It also makes regular S3 requests for `put` and so on use dualstack URLs. It doesn't seem like there is a downside to doing this, but a bunch of specs needed to be updated to reflect this. --- app/models/site_setting.rb | 10 +++-- lib/s3_helper.rb | 4 ++ .../backup_restore/s3_backup_store_spec.rb | 3 +- .../shared_examples_for_backup_store.rb | 8 ++-- spec/lib/s3_helper_spec.rb | 42 ++++++++++++++++++- spec/models/optimized_image_spec.rb | 2 +- .../requests/admin/backups_controller_spec.rb | 14 +++---- spec/requests/uploads_controller_spec.rb | 22 ++++++++-- spec/support/system_helpers.rb | 4 ++ spec/support/uploads_helpers.rb | 8 +++- 10 files changed, 90 insertions(+), 27 deletions(-) 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