Support MSC3916 by adding a federation `/download` endpoint #17172

H-Shay · 2024-05-08T22:38:06Z

MSC3916 outlines the addition of a new federation endpoint to serve media downloads to other servers. This PR implements that endpoint and the new multipart/mixed response format that it returns. Future PRs will implement the rest of MSC3916, including altering the new client-server /download endpoint to use the new federation endpoint for remote download requests.

Should be reviewable commit-by-commit

…classes when fetching media for federation download

synapse/federation/transport/server/_base.py

synapse/media/media_storage.py

H-Shay · 2024-05-08T22:48:20Z

Does this require more tests? If so, what should be tested?

H-Shay · 2024-05-09T00:55:52Z

complement run failing with CreateDirtyDeployment failed: CreateDirtyServer: Failed to deploy image complement-synapse : Error response from daemon: Conflict. The container name "/complement_fed_dirty_hs1" is already in use by container "08dc1e60340dfb37e078f29731cb4d102e1149a6afb2f5f62fc41564fd9caeac". You have to remove (or rename) that container to be able to reuse that name. - pretty sure that's not this PR? Normally I would just hit the re-run button but I no longer have that power...

anoadragon453 · 2024-05-09T14:37:00Z

complement run failing with CreateDirtyDeployment failed: CreateDirtyServer: Failed to deploy image complement-synapse : Error response from daemon: Conflict. The container name "/complement_fed_dirty_hs1" is already in use by container "08dc1e60340dfb37e078f29731cb4d102e1149a6afb2f5f62fc41564fd9caeac". You have to remove (or rename) that container to be able to reuse that name. - pretty sure that's not this PR? Normally I would just hit the re-run button but I no longer have that power...

I think that error is a red herring. In the Synapse logs, I'm seeing the following exception:

  Error during startup:
  Traceback (most recent call last):
  pusher1 | 2024-05-09 12:22:43,513 - synapse.replication.tcp.redis - 292 - INFO - sentinel - Connecting to redis server localhost:6379
    File "/usr/local/lib/python3.11/site-packages/synapse/app/_base.py", line 258, in wrapper
      await cb(*args, **kwargs)
    File "/usr/local/lib/python3.11/site-packages/synapse/app/_base.py", line 594, in start
      hs.start_listening()
    File "/usr/local/lib/python3.11/site-packages/synapse/app/generic_worker.py", line 247, in start_listening
      self._listen_http(listener)
    File "/usr/local/lib/python3.11/site-packages/synapse/app/generic_worker.py", line 187, in _listen_http
      resources[FEDERATION_PREFIX] = TransportLayerServer(self)
                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^
    File "/usr/local/lib/python3.11/site-packages/synapse/federation/transport/server/__init__.py", line 75, in __init__
      self.register_servlets()
    File "/usr/local/lib/python3.11/site-packages/synapse/federation/transport/server/__init__.py", line 78, in register_servlets
      register_servlets(
    File "/usr/local/lib/python3.11/site-packages/synapse/federation/transport/server/__init__.py", line 318, in register_servlets
      servletclass(
    File "/usr/local/lib/python3.11/site-packages/synapse/federation/transport/server/federation.py", line 812, in __init__
      self.media_repo = self.hs.get_media_repository()
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    File "/usr/local/lib/python3.11/site-packages/synapse/server.py", line 220, in _get
      dep = builder(self)
            ^^^^^^^^^^^^^
    File "/usr/local/lib/python3.11/site-packages/synapse/server.py", line 686, in get_media_repository
      return MediaRepository(self)
             ^^^^^^^^^^^^^^^^^^^^^
    File "/usr/local/lib/python3.11/site-packages/synapse/media/media_repository.py", line 92, in __init__
      self.max_upload_size = hs.config.media.max_upload_size
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  AttributeError: 'ContentRepositoryConfig' object has no attribute 'max_upload_size'

The error about conflicting containers is certainly unhelpful though...

H-Shay · 2024-05-09T17:31:13Z

Weird! If it's an attribute error why did only one of the three complement tests fail? Don't they use the same image? I'm gonna dig deeper but I'm surprised...

Edit: looks like this was due to workers trying to instantiate the federation download servlet (which relies on the media repo) when the media repo has been disabled via config. I wondered why this wasn't picked up by the Sytest worker runs but I suspect that it is due to the fact that sytest doesn't appear to use the configure_workers_and_start script which explicitly disables the media repo on most workers. Anyhow I think the error is resolved now :)

anoadragon453

A fantastic start - a few questions below. In addition, this functionality should be gated before an experimental config option.

synapse/federation/transport/server/federation.py

synapse/media/media_storage.py

synapse/media/storage_provider.py

anoadragon453

Nearly there! Just a few small things now.

synapse/media/media_storage.py

tests/federation/test_federation_media.py

synapse/media/media_storage.py

synapse/media/_base.py

synapse/federation/transport/server/_base.py

synapse/media/storage_provider.py

tests/federation/test_federation_media.py

H-Shay · 2024-05-14T22:45:16Z

It seems very unlikely to me that the sytest failure is a result of the last round of changes as I only added a test, newlines, and a comment...

anoadragon453

It seems very unlikely to me that the sytest failure is a result of the last round of changes as I only added a test, newlines, and a comment...

Looks like a flake. I've re-ran the tests.

synapse/media/storage_provider.py

synapse/media/_base.py

synapse/federation/transport/server/_base.py

synapse/media/_base.py

#17213) [MSC3916](https://github.com/matrix-org/matrix-spec-proposals/blob/rav/authentication-for-media/proposals/3916-authentication-for-media.md) adds new media endpoints under `_matrix/client`. This PR adds the `/preview_url`, `/config`, and `/thumbnail` endpoints. `/download` will be added in a follow-up PR once the work for the federation `/download` endpoint is complete (see #17172). Should be reviewable commit-by-commit.

element-hq#17213) [MSC3916](https://github.com/matrix-org/matrix-spec-proposals/blob/rav/authentication-for-media/proposals/3916-authentication-for-media.md) adds new media endpoints under `_matrix/client`. This PR adds the `/preview_url`, `/config`, and `/thumbnail` endpoints. `/download` will be added in a follow-up PR once the work for the federation `/download` endpoint is complete (see element-hq#17172). Should be reviewable commit-by-commit.

H-Shay · 2024-06-05T22:26:17Z

Updated to reflect that new version of MSC removes server_name from the endpoint.

anoadragon453

This looks pretty solid to me! A couple tiny notes, and then I think this is good to go.

anoadragon453 · 2024-06-06T10:40:02Z

synapse/federation/transport/server/__init__.py

+                            "Federation `/download` enpoint will not be enabled as your storage "
+                            "provider is not compatible with this endpoint."
+                        )
+                        load_download_endpoint = False


nit: there could be a break here to avoid checking further storage provider implementations when one is already incompatible.

anoadragon453 · 2024-06-06T10:41:16Z

synapse/federation/transport/server/__init__.py

+                    if "federation" not in signature.parameters:
+                        logger.warning(
+                            "Federation `/download` enpoint will not be enabled as your storage "
+                            "provider is not compatible with this endpoint."


it would be nice to mention the classname of the storage provider which isn't compatible, so the sysadmin knows which repo to make an issue on :)

anoadragon453 · 2024-06-06T10:46:48Z

synapse/federation/transport/server/__init__.py

+                    signature = inspect.signature(provider.backend.fetch)
+                    if "federation" not in signature.parameters:
+                        logger.warning(
+                            "Federation `/download` enpoint will not be enabled as your storage "


Suggested change

"Federation `/download` enpoint will not be enabled as your storage "

"Federation media `/download` endpoint will not be enabled as your storage "

H-Shay added 6 commits May 8, 2024 15:10

add federation /download servlet

e9e8b50

add MultipartResponder and MultipartFileSender classes and use these …

913e67e

…classes when fetching media for federation download

add multipart responder function

6a8063b

use multipart responder when fetching media for download

447cbcc

tests

d317c5e

cleanup + note

cbf9154

H-Shay requested a review from a team as a code owner May 8, 2024 22:38

newsfragment

b6803f6

H-Shay commented May 8, 2024

View reviewed changes

synapse/federation/transport/server/_base.py Show resolved Hide resolved

H-Shay commented May 8, 2024

View reviewed changes

synapse/media/media_storage.py Show resolved Hide resolved

maybe fix olddeps error??

522e752

dont load federation download servlet if the media repo is not enabled

a015532

anoadragon453 self-requested a review May 10, 2024 10:44

anoadragon453 requested changes May 13, 2024

View reviewed changes

H-Shay added 2 commits May 13, 2024 15:04

improve readability

85bd3a5

gate endpoint behind experimental config option

ef41492

anoadragon453 requested changes May 14, 2024

View reviewed changes

turt2live mentioned this pull request May 14, 2024

MSC3916: Authentication for media matrix-org/matrix-spec-proposals#3916

Open

H-Shay added 2 commits May 14, 2024 13:44

add comment and test

cbac3f5

lint

faa346f

H-Shay requested a review from anoadragon453 May 14, 2024 22:57

anoadragon453 reviewed May 15, 2024

View reviewed changes

synapse/media/storage_provider.py Show resolved Hide resolved

synapse/media/_base.py Show resolved Hide resolved

synapse/federation/transport/server/_base.py Show resolved Hide resolved

erikjohnston reviewed May 17, 2024

View reviewed changes

synapse/media/_base.py Outdated Show resolved Hide resolved

H-Shay mentioned this pull request May 17, 2024

Support MSC3916 by adding unstable media endpoints to _matrix/client #17213

Merged

H-Shay added 4 commits June 5, 2024 12:26

use multipart-mixed

66993dd

don't load endpoint if storage provider implementation is incompatible

dbf803d

tests

0305870

Merge branch 'develop' into shay/3916_pt_1

e8ef34b

H-Shay requested a review from anoadragon453 June 5, 2024 20:15

remove servername from endpoint

157e5d7

anoadragon453 requested changes Jun 6, 2024

View reviewed changes

requested changes

fb7ce78

H-Shay requested a review from anoadragon453 June 6, 2024 17:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support MSC3916 by adding a federation `/download` endpoint #17172

Support MSC3916 by adding a federation `/download` endpoint #17172

H-Shay commented May 8, 2024 •

edited

H-Shay commented May 8, 2024

H-Shay commented May 9, 2024

anoadragon453 commented May 9, 2024

H-Shay commented May 9, 2024 •

edited

anoadragon453 left a comment

anoadragon453 left a comment

H-Shay commented May 14, 2024

anoadragon453 left a comment

H-Shay commented Jun 5, 2024

anoadragon453 left a comment

anoadragon453 Jun 6, 2024

anoadragon453 Jun 6, 2024

anoadragon453 Jun 6, 2024

	"Federation `/download` enpoint will not be enabled as your storage "
	"Federation media `/download` endpoint will not be enabled as your storage "

Support MSC3916 by adding a federation /download endpoint #17172

Are you sure you want to change the base?

Support MSC3916 by adding a federation /download endpoint #17172

Conversation

H-Shay commented May 8, 2024 • edited

H-Shay commented May 8, 2024

H-Shay commented May 9, 2024

anoadragon453 commented May 9, 2024

H-Shay commented May 9, 2024 • edited

anoadragon453 left a comment

Choose a reason for hiding this comment

anoadragon453 left a comment

Choose a reason for hiding this comment

H-Shay commented May 14, 2024

anoadragon453 left a comment

Choose a reason for hiding this comment

H-Shay commented Jun 5, 2024

anoadragon453 left a comment

Choose a reason for hiding this comment

anoadragon453 Jun 6, 2024

Choose a reason for hiding this comment

anoadragon453 Jun 6, 2024

Choose a reason for hiding this comment

anoadragon453 Jun 6, 2024

Choose a reason for hiding this comment

Support MSC3916 by adding a federation `/download` endpoint #17172

Support MSC3916 by adding a federation `/download` endpoint #17172

H-Shay commented May 8, 2024 •

edited

H-Shay commented May 9, 2024 •

edited