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

eviljeff commented 2 months ago

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 commented 2 months ago

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

diox commented 2 months ago

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

diox commented 2 months ago

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)

KevinMind commented 2 months ago

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.

eviljeff commented 2 months ago

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)

KevinMind commented 2 months ago

@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 commented 2 months ago

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

KevinMind commented 2 months ago

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.

diox commented 2 months ago

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

KevinMind commented 2 months ago

@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.

diox commented 2 months ago

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)

willdurand commented 2 months ago

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/ {

alexandruschek commented 1 month ago

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!

mozilla / addons