REFACTOR: Proxy letter avatars in rails instead of nginx

REFACTOR: Proxy letter avatars in rails instead of nginx

Co-authored-by: Sam Saffron sam.saffron@gmail.com Co-authored-by: David Taylor david@taylorhq.com

This gives more control over the request. In particular we can easily lookup DNS dynamically, instead of only upon NGINX startup. Previously, NGINX was looking up IP for the letter avatar service and caching the CDN IP address, this caused issues if CDN changed IP, in which letter avatars would be broken till a container restarted.

NGINX config has been updated to add caching. This change will require a container rebuild.

The proxy will now function in development environments, so the patch for letter_avatar_proxy has been removed.

diff --git a/app/controllers/user_avatars_controller.rb b/app/controllers/user_avatars_controller.rb
index 79c3715..0fa0d32 100644
--- a/app/controllers/user_avatars_controller.rb
+++ b/app/controllers/user_avatars_controller.rb
@@ -35,7 +35,7 @@ class UserAvatarsController < ApplicationController
   def show_proxy_letter
     is_asset_path
 
-    if SiteSetting.external_system_avatars_url !~ /^\/letter_avatar_proxy/
+    if SiteSetting.external_system_avatars_url !~ /^\/letter(_avatar)?_proxy/
       raise Discourse::NotFound
     end
 
@@ -43,17 +43,8 @@ class UserAvatarsController < ApplicationController
     params.require(:color)
     params.require(:version)
     params.require(:size)
-
     hijack do
-      identity = LetterAvatar::Identity.new
-      identity.letter = params[:letter].to_s[0].upcase
-      identity.color = params[:color].scan(/../).map(&:hex)
-      image = LetterAvatar.generate(params[:letter].to_s, params[:size].to_i, identity: identity)
-
-      response.headers["Last-Modified"] = File.ctime(image).httpdate
-      response.headers["Content-Length"] = File.size(image).to_s
-      immutable_for(1.year)
-      send_file image, disposition: nil
+      proxy_avatar("https://avatars.discourse.org/#{params[:version]}/letter/#{params[:letter]}/#{params[:color]}/#{params[:size]}.png", Time.new('1990-01-01'))
     end
   end
 
diff --git a/config/nginx.sample.conf b/config/nginx.sample.conf
index 48bb7a3..d98fe99 100644
--- a/config/nginx.sample.conf
+++ b/config/nginx.sample.conf
@@ -189,9 +189,9 @@ server {
     }
 
     # This big block is needed so we can selectively enable
-    # acceleration for backups and avatars
+    # acceleration for backups, avatars, sprites and so on.
     # see note about repetition above
-    location ~ ^/(svg-sprite/|letter_avatar/|user_avatar|highlight-js|stylesheets|theme-javascripts|favicon/proxied|service-worker) {
+    location ~ ^/(svg-sprite/|letter_avatar/|letter_avatar_proxy/|user_avatar|highlight-js|stylesheets|theme-javascripts|favicon/proxied|service-worker) {
       proxy_set_header Host $http_host;
       proxy_set_header X-Real-IP $remote_addr;
       proxy_set_header X-Request-Start "t=${msec}";
@@ -214,30 +214,6 @@ server {
       break;
     }
 
-    location /letter_avatar_proxy/ {
-      # Don't send any client headers to the avatars service
-      proxy_method GET;
-      proxy_pass_request_headers off;
-      proxy_pass_request_body off;
-
-      # Don't let cookies interrupt caching, and don't pass them to the
-      # client
-      proxy_ignore_headers "Set-Cookie";
-      proxy_hide_header "Set-Cookie";
-      proxy_hide_header "X-Discourse-Username";
-      proxy_hide_header "X-Runtime";
-
-      proxy_cache one;
-      # shared in multisite
-      proxy_cache_key $request_uri;
-      proxy_cache_valid 200 7d;
-      proxy_cache_valid 404 1m;
-      proxy_set_header Connection "";
-
-      proxy_pass https://avatars.discourse.org/;
-      break;
-    }
-
     # we need buffering off for message bus
     location /message-bus/ {
       proxy_set_header X-Request-Start "t=${msec}";
diff --git a/config/routes.rb b/config/routes.rb
index 955d0b1..08c5c8c 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -450,7 +450,6 @@ Discourse::Application.routes.draw do
   get "letter_avatar/:username/:size/:version.png" => "user_avatars#show_letter", format: false, constraints: { hostname: /[\w\.-]+/, size: /\d+/, username: RouteFormat.username }
   get "user_avatar/:hostname/:username/:size/:version.png" => "user_avatars#show", format: false, constraints: { hostname: /[\w\.-]+/, size: /\d+/, username: RouteFormat.username }
 
-  # in most production settings this is bypassed
   get "letter_avatar_proxy/:version/letter/:letter/:color/:size.png" => "user_avatars#show_proxy_letter"
 
   get "svg-sprite/:hostname/svg-:version.js" => "svg_sprite#show", format: false, constraints: { hostname: /[\w\.-]+/, version: /\h{40}/ }
diff --git a/config/site_settings.yml b/config/site_settings.yml
index 6081b71..df374a0 100644
--- a/config/site_settings.yml
+++ b/config/site_settings.yml
@@ -1074,7 +1074,7 @@ files:
     client: true
     shadowed_by_global: true
   external_system_avatars_url:
-    default: "/letter_avatar_proxy/v2/letter/{first_letter}/{color}/{size}.png"
+    default: "/letter_avatar_proxy/v3/letter/{first_letter}/{color}/{size}.png"
     client: true
     regex: '^((https?:)?\/)?\/.+[^\/]'
     shadowed_by_global: true
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 1775351..a8dea6d 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1147,7 +1147,7 @@ describe User do
       expect(user.small_avatar_url).to eq("//test.localhost/letter_avatar/sam/45/#{LetterAvatar.version}.png")
 
       SiteSetting.external_system_avatars_enabled = true
-      expect(user.small_avatar_url).to eq("//test.localhost/letter_avatar_proxy/v2/letter/s/5f9b8f/45.png")
+      expect(user.small_avatar_url).to eq("//test.localhost/letter_avatar_proxy/v3/letter/s/5f9b8f/45.png")
     end
 
   end
diff --git a/spec/requests/user_avatars_controller_spec.rb b/spec/requests/user_avatars_controller_spec.rb
index 030959c..d244ead 100644
--- a/spec/requests/user_avatars_controller_spec.rb
+++ b/spec/requests/user_avatars_controller_spec.rb
@@ -10,7 +10,8 @@ describe UserAvatarsController do
     end
 
     it 'returns an avatar if we are allowing the proxy' do
-      get "/letter_avatar_proxy/v2/letter/a/aaaaaa/360.png"
+      stub_request(:get, "https://avatars.discourse.org/v3/letter/a/aaaaaa/360.png").to_return(body: 'image')
+      get "/letter_avatar_proxy/v3/letter/a/aaaaaa/360.png"
       expect(response.status).to eq(200)
     end
   end

GitHub sha: c10941bb

This commit has been mentioned on Discourse Meta. There might be relevant details there: