Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Get rid of worker_main_http_uri #13642

Closed
richvdh opened this issue Aug 26, 2022 · 3 comments · Fixed by #14400
Closed

Get rid of worker_main_http_uri #13642

richvdh opened this issue Aug 26, 2022 · 3 comments · Fixed by #14400
Labels
A-Config Configuration, or the documentation thereof A-Workers Problems related to running Synapse in Worker Mode (or replication) O-Occasional Affects or can be seen by some users regularly or most users rarely S-Minor Blocks non-critical functionality, workarounds exist. T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. Z-Cleanup Things we want to get rid of, but aren't actively causing pain

Comments

@richvdh
Copy link
Member

richvdh commented Aug 26, 2022

worker_main_http_uri is used in exactly one place (the handler for keys/upload/) to proxy requests through to the main process. Given we already have the "http replication listener", this is needlessly confusing and annoying to configure.

@richvdh richvdh added A-Workers Problems related to running Synapse in Worker Mode (or replication) S-Minor Blocks non-critical functionality, workarounds exist. T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. O-Occasional Affects or can be seen by some users regularly or most users rarely labels Aug 26, 2022
@DMRobertson DMRobertson added Z-Cleanup Things we want to get rid of, but aren't actively causing pain A-Config Configuration, or the documentation thereof labels Aug 30, 2022
@realtyem
Copy link
Contributor

realtyem commented Oct 29, 2022

I think I tracked down all the information to give this a shot.

  • Looks like the worker_main_http_uri object in the WorkerConfig class is populated from the worker.yaml file at line 158 in workers.py and then pulled into the KeyUploadServlet() class in generic_worker.py at line 148 as self.main_uri. I think from there it gets processed in the same file(I could find no other references to this variable) as part of post_json_get_json() where the rest of the (ascii)string for the uri is appended to the request being sent.
  • The values needed would be
    • scheme to determine if 'http' or 'https' would be appropriate. I think this can gotten from config.server.listeners[1].tls. I think I would have to iterate over all listeners in this section to find the one with a client resource. I would need to use this same listener below for port.
    • netloc to identify if an IP address or hostname is declared. I think I can get this from config.workers.worker_replication_host as it should be the same value and be pointing at the homeserver's IP address or hostname(as appropriate).
    • port to make sure it's pointed at the actual client port and not something else. I think I can get this from the same listener I found for scheme and get it from hs.config.server.listeners[1].port.

I believe it's a simple string concatenation to put the pieces together and set it either at worker_main_http_uri in workers.py(referenced above) or factor the config value out altogether and set it in the KeyUploadServlet()(again, referenced above) directly. I know which one I would choose.

Does that sound about right? Did I miss anything obvious, easier or a better way?

@richvdh
Copy link
Member Author

richvdh commented Oct 31, 2022

There are lots of other bits of worker<->worker communication that work via the "http replication API": see the servlets in https://github.com/matrix-org/synapse/blob/15bdb0da522ba902e6a1c55f7f6775faeb47176a/synapse/replication/http. We should use something similar for uploading keys.

@realtyem
Copy link
Contributor

I'm sorry, for the moment that kind of thing is beyond me.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Config Configuration, or the documentation thereof A-Workers Problems related to running Synapse in Worker Mode (or replication) O-Occasional Affects or can be seen by some users regularly or most users rarely S-Minor Blocks non-critical functionality, workarounds exist. T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. Z-Cleanup Things we want to get rid of, but aren't actively causing pain
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants