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 9c87fa3..2fb043f 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 eb9f3ef..55a0aa8 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -451,7 +451,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-:theme_ids-:version.js" => "svg_sprite#show", format: false, constraints: { hostname: /[\w\.-]+/, version: /\h{40}/, theme_ids: /([0-9]+(,[0-9]+)*)?/ }
diff --git a/config/site_settings.yml b/config/site_settings.yml
index d64c324..7bdcc2a 100644
--- a/config/site_settings.yml
+++ b/config/site_settings.yml
@@ -1077,7 +1077,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 732e8c4..d4e8c35 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: f04471e4

1 Like

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

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

Now that the letter_proxy route has been removed, this change can be reverted

1 Like

DEV: Remove reference to non-existent `letter_proxy` route