From f04471e4229218903df17232db955cd583a423ef Mon Sep 17 00:00:00 2001 From: David Taylor Date: Fri, 15 Feb 2019 17:45:09 +0000 Subject: [PATCH] REFACTOR: Proxy letter avatars in rails instead of nginx Co-authored-by: Sam Saffron Co-authored-by: David Taylor 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. --- app/controllers/user_avatars_controller.rb | 13 ++------- config/nginx.sample.conf | 28 ++----------------- config/routes.rb | 1 - config/site_settings.yml | 2 +- spec/models/user_spec.rb | 2 +- spec/requests/user_avatars_controller_spec.rb | 3 +- 6 files changed, 8 insertions(+), 41 deletions(-) diff --git a/app/controllers/user_avatars_controller.rb b/app/controllers/user_avatars_controller.rb index 79c3715a09b..0fa0d32c701 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 9c87fa31470..2fb043f1b5c 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 eb9f3ef7d82..55a0aa8d48d 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 d64c32469c9..7bdcc2ac667 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 732e8c46275..d4e8c35638f 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 030959c7481..d244ead5045 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