Policy server part 1: Actually call the policy server

[turt2live]

Roughly reviewable commit-by-commit.

This is the first part of adding policy server support to Synapse. Other parts (unordered), which may or may not be bundled into fewer PRs, include:

• Implementation of a bulk API
• Supporting a moderation server config (the fallback_* options of https://github.com/element-hq/policyserv_spam_checker )
• Adding an "early event hook" for appservices to receive federation transactions before events are processed formally
• Performance and stability improvements

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• Code style is correct
  (run the linters)

[turt2live]

I'm not overly familiar with Synapse's codebase yet, and struggling to find good places to copy/paste some code to set up some tests. I think ideally we have tests on RoomPolicyHandler, but I'm just not sure how to make a test that can spin up a second homeserver to "join" the room to appease the is_in_room check within RoomPolicyHandler. I'm confident we can intercept the outbound /check call for testing, but getting the room to have 2 servers in it looks difficult at best.

Thoughts (or code - please just push to the branch 😇) are appreciated on this.

(equally, I'm personally okay to just land this without tests, but I suspect team policy might override that opinion)

[kegsay]

I'm just not sure how to make a test that can spin up a second homeserver to "join" the room to appease the is_in_room check within RoomPolicyHandler

https://github.com/element-hq/synapse/blob/develop/tests/handlers/test_federation_event.py#L48 is probably your best bet to copypasta I think? You can see it injecting a remote member https://github.com/element-hq/synapse/blob/develop/tests/handlers/test_federation_event.py#L114

[kegsay]

Nit: I'd rather use f-strings personally, they are waaaay more ergonomic. E.g

logger.warning(f"get_pdu_policy_recommendation: server {destination} responded with error, assuming OK recommendation: {e}")

[erikjohnston]

We don't use f-strings in loggers, while they are more ergonomic it means that you are passing in the formatted string, rather than allowing the loggers to do so. The main consequence is that if the logger is disabled you don't necessarily want to pay the cost of string interpolation.

Otherwise, yeah f-strings are awesome.

[turt2live]

with my limited python exposure, +1 to fstrings, but definitely don't want to make things harder for the logger

[kegsay]

And it can't be run on the same domain with a .well-known lookup or something? I'd rather we didn't hard code this given if/when Synapse can be a PS we need to remember to remove this specific conditional instead of just intuitively pointing the via at the HS.

[turt2live]

I'm not entirely sure that Synapse will be happy with trying to contact itself over federation because that's a pretty weird thing to do in Matrix. Any Synapse support for being a policy server (either natively or via modules) would involve a performant hook anyway, and would be inserted here to short-circuit the federation call.

Removing the check would also obviously slow down message sending in rooms we know are misconfigured (not pointing to a real policy server). With the current highly experimental architecture we also do not expect that pre-existing servers will be policy servers, and operators will need to pick a dedicated domain name without human users on that server. This will probably change down the line.

I'll add some text to the docstring to reduce the chances of forgetting in the meantime. Hopefully when someone writes a test for "can Synapse be a policy server", they'll worst case find the if statement by stepping through the call path.

[turt2live]

I also ended up adding a test for this, so the failure should be even more clear if we do add such functionality

[turt2live]

I don't know what I did to make the sytests angry aside from merging in develop

[devonh]

I don't know what I did to make the sytests angry aside from merging in develop

Seems to have just been flakey tests sadly

[devonh]

I think this is good to merge now. It follows the same patterns as the spam checker code where applicable so has pretty strong precedence. The new tests are also very appreciated.