addons icon indicating copy to clipboard operation
addons copied to clipboard

[Bug]: user-media served content is broken, locally

Open eviljeff opened this issue 1 year ago • 13 comments

What happened?

A path such as http://olympia.test/user-media/version-previews/full/0/1.png should work, if "storage/shared_storage/uploads/version-previews/full/0/1.png" exists, but locally it's 404'ing. It's actually available at http://olympia.test/user-media/shared_storage/uploads/version-previews/full/0/1.png, which is wrong.

What did you expect to happen?

we serve content from the urls that django generates (which are the same path structure as on dev/stage/prod).

Is there an existing issue for this?

  • [X] I have searched the existing issues

┆Issue is synchronized with this Jira Task

eviljeff avatar Sep 17 '24 09:09 eviljeff

Probable regression candidate: https://github.com/mozilla/addons-server/pull/22409

eviljeff avatar Sep 17 '24 09:09 eviljeff

Version previews (autogenerated theme previews), Add-on previews (screenshots uploaded by the developer) and custom icons are broken that way locally.

diox avatar Sep 17 '24 09:09 diox

We'll have to play with aliases I suspect. Interestingly patch that regressed this removed some aliases for xpis, but on dev/stage/prod we do use them (leaving the previews/icons alone, they work sort of out of the box)

diox avatar Sep 17 '24 10:09 diox

Related https://github.com/mozilla/addons/issues/14886

KevinMind avatar Sep 17 '24 10:09 KevinMind

I think the first step to solve this will be adding the tests I reference in #14886 this is sufficiently complex and esoteric that we should cover the configuration with tests to ensure it works as we make changes.

KevinMind avatar Sep 17 '24 10:09 KevinMind

I think the first step to solve this will be adding the tests I reference in #14886 this is sufficiently complex and esoteric that we should cover the configuration with tests to ensure it works as we make changes.

I agree it'd be good to add some tests at the same time we fix it, assuming it doesn't balloon the scope so much it takes too long to actually fix. (Any other tests we think are needed for general docker,etc config can be added in a follow-up)

eviljeff avatar Sep 17 '24 11:09 eviljeff

@eviljeff can you provide STR mapping sample request paths to expected host file locations? example /user-media/test.txt ./storage/test.txt (that one should work now)

But it's unclear exactly how we are creating a reliance on these non standard paths so it's hard to test my patch fixing this..

KevinMind avatar Sep 18 '24 11:09 KevinMind

LInk to the original sin https://github.com/mozilla/addons-server/commit/bc186195bc5da7fc0f7268a11f67a5470a64c454#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3

KevinMind avatar Sep 18 '24 11:09 KevinMind

How are guarded-addons and sitemaps even handled? I see we had mappings for them, but they are not "saved" in the .gitkeep files.

Also why are we mapping /user-media/addons to ./storage/files... why not sync the names? then we wouldn't need special routing for that path.

We also don't seem to need special routing for guarded addons at all.. or for sitemaps.. but idk how to test sitemaps as the view logic seems a bit complicated.

KevinMind avatar Sep 18 '24 12:09 KevinMind

guarded-addons is no longer a thing and can go away, FYI.

Files that we are referencing directly:

  • Theme previews (automatically generated, attached to a Version through VersionPreview). Typical URL: /user-media/version-previews/full/3970/3970835.svg?modified=1702917049
  • Add-on Previews (uploaded by developers, goes through some processing, attached to an Addon through Preview). Typical URL: /user-media/previews/thumbs/238/238546.jpg?modified=1622132421
  • Add-on custom icons (uploaded by developers, goes through some processing, attached to an Addon though icon_type). Typical URL: /user-media/addon_icons/607/607454-64.png?modified=mcrushed
  • User avatars (uploaded by developers, goes through some processing, attached to an UserProfile through picture_type). Typical URL: /user-media/userpics/06/0106/10160106/10160106.png?modified=1698231186

Files that we are serving through X-Accel-Redirect (need the dir to have internal in nginx):

  • XPI files (uploaded by developers, goes through some processing, attached to FileUpload or File). Typical URL: /firefox/downloads/file/4328681/ublock_origin-1.59.0.xpi, /uploads/file/dummy/?access_token=blah
  • Source files (uploaded by developers). Typical URL: /firefox/downloads/source/123456
  • Activity attachments (uploaded by reviewers). Typical URL: /activity/attachment/4568
  • Sitemap files (generated by manage.py cron write_sitemaps, triggered by cron in dev/stage/prod). Typical URL: /sitemap.xml?section=amo

diox avatar Sep 18 '24 14:09 diox

@diox do you have a mapping of the urls to the location in the storage directory? From what I can see the specific problem is that these mappings are not straight forward and so we need not only the url but the path where we expect the file to exist.

KevinMind avatar Sep 24 '24 10:09 KevinMind

For files we are referencing directly:

  • Theme previews are under {root_storage}/shared_storage/uploads/version-previews
  • Add-on previews are under {root_storage}/shared_storage/uploads/previews
  • Icons are under {root_storage}/shared_storage/uploads/addon_icons
  • User avatars are under {root_storage}/shared_storage/uploads/userpics

As you can see, for those the URL all map directly /user-media/ to {root_storage}/shared_storage/uploads/, nothing fancy.

For files we're serving through X-Accel-Redirect the URL should not matter at all. We just need the various dirs to be in a location that has internal set (and possibly an alias to the same dir). See https://github.com/mozilla-it/webservices-infra/blob/11890a7248ecc8f600889624bac3b61f94bd6832/amo/k8s/amo-proxy/configs/addons.conf.tpl#L70-L95 (+ https://github.com/mozilla-it/webservices-infra/pull/2824/files for attachments)

diox avatar Sep 24 '24 10:09 diox

Ha oops, I think something like this should fix the issue:

diff --git a/docker/nginx/addons.conf b/docker/nginx/addons.conf
index e53f6ac26a..1d87ec43ac 100644
--- a/docker/nginx/addons.conf
+++ b/docker/nginx/addons.conf
@@ -20,7 +20,7 @@ server {
     }
 
     location /user-media/ {
-        alias /srv/user-media/;
+        alias /srv/user-media/shared_storage/uploads/;
     }
 
     location ~ ^/api/ {

willdurand avatar Sep 24 '24 15:09 willdurand

Hello,

I see this has the local dev env label, but I did some checks on -DEV environment to make sure everything works as expected. If there is need for any additional verification, please reach out.

Thank you!

alexandruschek avatar Oct 10 '24 13:10 alexandruschek