Skip to content
Permalink
Browse files

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.
  • Loading branch information...
davidtaylorhq authored and SamSaffron committed Feb 15, 2019
1 parent 944f824 commit f04471e4229218903df17232db955cd583a423ef
@@ -35,25 +35,16 @@ def refresh_gravatar
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/

This comment has been minimized.

Copy link
@davidtaylorhq

davidtaylorhq Feb 18, 2019

Author Member

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

raise Discourse::NotFound
end

params.require(:letter)
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

@@ -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}";
@@ -451,7 +451,6 @@
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]+)*)?/ }
@@ -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
@@ -1147,7 +1147,7 @@ def test_user?(opts = {})
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
@@ -10,7 +10,8 @@
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

3 comments on commit f04471e

@discoursebot

This comment has been minimized.

Copy link

replied Feb 18, 2019

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

https://meta.discourse.org/t/restarting-container-when-internet-is-down-causes-discourse-to-fail/42615/10

@discoursebot

This comment has been minimized.

Copy link

replied Feb 18, 2019

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

https://meta.discourse.org/t/users-profile-image-not-showing/109224/29

Please sign in to comment.
You can’t perform that action at this time.