From badd64ceee5427e141a3cc6ebd6ae32ef8d5fe9a Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
Date: Fri, 2 Dec 2022 10:07:25 +0000
Subject: [PATCH] PERF: Add GlobalSetting to redirect avatars instead of
 proxying (#19281)

When uploads are stored on S3, by default Discourse will fetch the avatars and proxy them through to the requesting client. This is simple, but it can lead to significant inbound/outbound network load in the hosting environment.

This commit adds an optional redirect_avatar_requests GlobalSetting. When enabled, requests for user avatars will be redirected to the S3 asset instead of being proxied. This adds an extra round-trip for clients, but it should significantly reduce server load. To mitigate that extra round-trip for clients, a CDN with 'follow redirect' capability could be used.
---
 app/controllers/user_avatars_controller.rb    |  7 +++++
 config/discourse_defaults.conf                |  3 +++
 spec/requests/user_avatars_controller_spec.rb | 27 +++++++++++++++++++
 3 files changed, 37 insertions(+)

diff --git a/app/controllers/user_avatars_controller.rb b/app/controllers/user_avatars_controller.rb
index 3f148cb477..7a525359fd 100644
--- a/app/controllers/user_avatars_controller.rb
+++ b/app/controllers/user_avatars_controller.rb
@@ -125,6 +125,8 @@ class UserAvatarsController < ApplicationController
       if optimized.local?
         optimized_path = Discourse.store.path_for(optimized)
         image = optimized_path if File.exist?(optimized_path)
+      elsif GlobalSetting.redirect_avatar_requests
+        return redirect_s3_avatar(Discourse.store.cdn_url(optimized.url))
       else
         return proxy_avatar(Discourse.store.cdn_url(optimized.url), upload.created_at)
       end
@@ -179,6 +181,11 @@ class UserAvatarsController < ApplicationController
     send_file path, disposition: nil
   end
 
+  def redirect_s3_avatar(url)
+    immutable_for 1.hour
+    redirect_to url, allow_other_host: true
+  end
+
   # this protects us from a DoS
   def render_blank
     path = Rails.root + "public/images/avatar.png"
diff --git a/config/discourse_defaults.conf b/config/discourse_defaults.conf
index f2b84768c7..d45d0d1f4f 100644
--- a/config/discourse_defaults.conf
+++ b/config/discourse_defaults.conf
@@ -359,3 +359,6 @@ long_polling_interval =
 
 # Moves asset preloading from tags in the response document head to response headers
 preload_link_header = false
+
+# When using an external upload store, redirect `user_avatar` requests instead of proxying
+redirect_avatar_requests = false
diff --git a/spec/requests/user_avatars_controller_spec.rb b/spec/requests/user_avatars_controller_spec.rb
index 9a59f660e3..4ed33e3b82 100644
--- a/spec/requests/user_avatars_controller_spec.rb
+++ b/spec/requests/user_avatars_controller_spec.rb
@@ -100,6 +100,33 @@ RSpec.describe UserAvatarsController do
       expect(response).to redirect_to("http://awesome.com/boom/user_avatar/default/#{user.encoded_username(lower: true)}/98/#{upload.id}_#{OptimizedImage::VERSION}.png")
     end
 
+    it "redirects to external store when enabled" do
+      global_setting :redirect_avatar_requests, true
+      setup_s3
+      SiteSetting.avatar_sizes = "100|49"
+      SiteSetting.s3_cdn_url = "https://s3-cdn.example.com"
+      set_cdn_url("https://app-cdn.example.com")
+
+      upload = Fabricate(:upload, url: "//#{SiteSetting.s3_upload_bucket}.s3.dualstack.us-west-1.amazonaws.com/upload/path")
+
+      optimized_image = Fabricate(:optimized_image,
+        sha1: SecureRandom.hex << "A" * 8,
+        upload: upload,
+        width: 98,
+        height: 98,
+        url: "//#{SiteSetting.s3_upload_bucket}.s3.dualstack.us-west-1.amazonaws.com/optimized/path",
+        version: OptimizedImage::VERSION
+      )
+
+      user = Fabricate(:user, uploaded_avatar_id: upload.id)
+
+      get "/user_avatar/default/#{user.username}/98/#{upload.id}.png"
+
+      expect(response.status).to eq(302)
+      expect(response.location).to eq("https://s3-cdn.example.com/optimized/path")
+      expect(response.headers["Cache-Control"]).to eq('max-age=31556952, public, immutable')
+    end
+
     it 'serves new version for old urls' do
       user = Fabricate(:user)
       SiteSetting.avatar_sizes = "45"