Internal Admin API and Draupnir Hjack Command Config #3389
Conversation
And also make the internal admin API be automatically activated when this capability is used.
| @@ -4307,6 +4309,7 @@ matrix_synapse_container_labels_public_client_root_redirection_enabled: "{{ matr | |||
| matrix_synapse_container_labels_public_client_root_redirection_url: "{{ (('https://' if matrix_playbook_ssl_enabled else 'http://') + matrix_server_fqn_element) if matrix_client_element_enabled else '' }}" | |||
|
|
|||
| matrix_synapse_container_labels_public_client_synapse_admin_api_enabled: "{{ matrix_synapse_admin_enabled }}" | |||
| matrix_synapse_container_labels_internal_client_synapse_admin_api_enabled: "{{ matrix_bot_draupnir_admin_api_enabled }}" | |||
There was a problem hiding this comment.
The matrix_synapse_container_labels_internal_client_synapse_admin_api_enabled variable is written to here, but it's not defined in defaults/main.yml in the matrix-synapse role.
There should be a definition with some default value (false) there.
| @@ -4466,6 +4469,7 @@ matrix_synapse_reverse_proxy_companion_container_labels_traefik_hostname: "{{ ma | |||
|
|
|||
| matrix_synapse_reverse_proxy_companion_container_labels_public_client_synapse_client_api_enabled: "{{ matrix_synapse_container_labels_public_client_synapse_client_api_enabled }}" | |||
| matrix_synapse_reverse_proxy_companion_container_labels_public_client_synapse_admin_api_enabled: "{{ matrix_synapse_container_labels_public_client_synapse_admin_api_enabled }}" | |||
| matrix_synapse_reverse_proxy_companion_container_labels_internal_client_synapse_admin_api_enabled: "{{ matrix_synapse_container_labels_internal_client_synapse_admin_api_enabled }}" | |||
There was a problem hiding this comment.
Like matrix_synapse_container_labels_internal_client_synapse_admin_api_enabled, the matrix_synapse_reverse_proxy_companion_container_labels_internal_client_synapse_admin_api_enabled variable is written to here, but it's not defined in defaults/main.yml in the matrix-synapse-reverse-proxy-companion role.
There should be a definition with some default value (false) there.
| # # | ||
| ############################################################ | ||
|
|
||
| traefik.http.routers.matrix-synapse-reverse-proxy-companion-internal-client-synapse-client-api.rule=PathPrefix(`/_synapse/client`) |
There was a problem hiding this comment.
It'd be better if the rule is coming from a variable (matrix_synapse_reverse_proxy_companion_container_labels_internal_client_synapse_admin_api_traefik_rule), defined in default/main.yml in the matrix-synapse-reverse-proxy-companion role.
Here's an example:
You may even consider adding a _priority variable (like this), getting its value from the matrix-synapse role. In the matrix-synapse role, a priority of 0 is a good default, I suppose.
|
|
||
|
|
||
| traefik.http.routers.matrix-synapse-reverse-proxy-companion-internal-client-synapse-client-api.service=matrix-synapse-reverse-proxy-companion-client-api | ||
| traefik.http.routers.matrix-synapse-reverse-proxy-companion-internal-client-synapse-client-api.entrypoints=matrix-internal-matrix-client-api |
There was a problem hiding this comment.
It'd be better if the entrypoints are coming from a variable (matrix_synapse_reverse_proxy_companion_container_labels_internal_client_synapse_admin_api_traefik_entrypoints), defined in default/main.yml in the matrix-synapse-reverse-proxy-companion role.
Then, group_vars/matrix_servers can pass the correct internal entrypoint like this:
matrix_synapse_reverse_proxy_companion_container_labels_internal_client_synapse_admin_api_traefik_entrypo: "{{ matrix_playbook_internal_matrix_client_api_traefik_entrypoint_name }}"Here's an example:
| traefik.http.routers.matrix-synapse-reverse-proxy-companion-internal-client-synapse-admin-api.rule=PathPrefix(`/_synapse/admin`) | ||
|
|
||
|
|
||
| traefik.http.routers.matrix-synapse-reverse-proxy-companion-internal-client-synapse-admin-api.service=matrix-synapse-reverse-proxy-companion-client-api | ||
| traefik.http.routers.matrix-synapse-reverse-proxy-companion-internal-client-synapse-admin-api.entrypoints=matrix-internal-matrix-client-api |
There was a problem hiding this comment.
Same as for /_synapse/client above, it'd be better if these are not using hardcoded values, but the values are coming from variables.
| @@ -119,6 +119,44 @@ traefik.http.routers.matrix-synapse-reverse-proxy-companion-public-client-synaps | |||
| ############################################################ | |||
| {% endif %} | |||
|
|
|||
| {% if matrix_synapse_reverse_proxy_companion_container_labels_internal_client_synapse_admin_api_enabled %} | |||
There was a problem hiding this comment.
It seems like matrix_synapse_reverse_proxy_companion_container_labels_internal_client_synapse_admin_api_enabled actually enables 2 things right now: /_synapse/client and /_synapse/admin.
For the "public" exposure of these, we have dedicated variables: matrix_synapse_reverse_proxy_companion_container_labels_public_client_synapse_client_api_enabled and matrix_synapse_reverse_proxy_companion_container_labels_public_client_synapse_admin_api_enabled.
So it's odd to have a single variable (matrix_synapse_reverse_proxy_companion_container_labels_internal_client_synapse_admin_api_enabled) exposing both right now.
I suggest to create an additional variable (matrix_synapse_reverse_proxy_companion_container_labels_internal_client_synapse_client_api_enabled) and use that one to decide whether to expose /_synapse/client.
Of course, you can wire the new variable (matrix_synapse_reverse_proxy_companion_container_labels_internal_client_synapse_client_api_enabled) in group_vars/matrix_servers the same way (based on matrix_bot_draupnir_room_hijack_enabled).
I don't know if Draupnir room hijacking requires both /_synapse/admin and /_synapse/client though. If it only requires the former, then there's no need to enable the latter based on matrix_bot_draupnir_room_hijack_enabled.
You've added labels to matrix-synapse-reverse-proxy-companion, which handles the workers-being-enabled case.
Similar labels should be added to roles/custom/matrix-synapse/templates/labels.j2 as well.
Along with those, variables for each of these should be defined in the matrix-synapse role. Example:
matrix_synapse_container_labels_internal_client_synapse_admin_api_enabledmatrix_synapse_container_labels_internal_client_synapse_admin_api_traefik_rulematrix_synapse_container_labels_internal_client_synapse_admin_api_entrypointsmatrix_synapse_container_labels_internal_client_synapse_client_api_enabledmatrix_synapse_container_labels_internal_client_synapse_client_api_traefik_rulematrix_synapse_container_labels_internal_client_synapse_client_api_entrypoints
The matrix-synapse-reverse-proxy-companion role tends to inherit (via group_vars/matrix_overrides) various variable values from matrix-synapse (here is an example), so you can wire the new matrix-synapse-reverse-proxy-companion variables (enabled, entrypoints, etc.) the same way.
There was a problem hiding this comment.
I should note that when it comes to this PR i am blindly taking the labels from the issue in question.
So like it makes sense that these flaws exist. And i need to thank you for this very thuroughly written review.
I will try to take a look at this during the week to see how much of this stuff i can fix on my own based on all this valuable feedback and trying to look at other roles more closely to understand how they are plumbed. (Primarily Synapse admin i think i will need to look closer at as i think it helps a lot.)
As for the exact API surface Draupnir needs for its admin commands i will come back with an answer on that front as i honestly dont know but i know i can find out.
There was a problem hiding this comment.
That's understandable! Thank you for looking into it!
Co-authored-by: Slavi Pantaleev <slavi@devture.com>
This PR was made to resolve #3308 as it popped up again and on the draupnir side with hjacking of rooms in issue the-draupnir-project/Draupnir#463.
Now this PR is admittedly not my best work but its something. I simply am way too out of my depth here to be sure this is the right way forward.
I threw something together as well the Labels are claimed to work in #3308 (comment) so i assume they are good enough for a start.
Docs for this are matter that can be resolved after the primary problem in this PR is resolved.
Now this PR wont fix the polling. Why? Polling doesnt work in Draupnir in general currently it looks like.