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

Rate limiti joins per-room, accounting for joins created by other servers #13169

Closed
wants to merge 23 commits into from

Conversation

DMRobertson
Copy link
Contributor

@DMRobertson DMRobertson commented Jul 4, 2022

This was a bit of a monster. Review commit-by-commit.

There are a bunch of helper commits to start which are fairly independent. [f272919] and
accf9d6 are the actual bits of logic here.

Closes #12710.

@DMRobertson DMRobertson force-pushed the dmr/rate-limit-remote-joins branch 3 times, most recently from d75717f to 3f58763 Compare July 4, 2022 17:10
@DMRobertson DMRobertson marked this pull request as ready for review July 4, 2022 17:33
@DMRobertson DMRobertson requested a review from a team as a code owner July 4, 2022 17:33
David Robertson added 13 commits July 4, 2022 19:10
but don't use it in tests
Warn about replication problem in upgrade notes
and consult it when actioning joins.
Only bump rate limit if we will persist the event; otherwise we'll see
it over replication
- FederatingHomeserverTestCase: keys last 10x longer

  I am guessing that this is the first example of a test which calls
  `make_signed_federation_request` after the reactor has advanced by >=
  1 second until now.

- Increase max_request_body_size to 4KB in tests

  The previous limit prevented the master from accepting some
  replication requests which were 1.3kB in size.
@DMRobertson DMRobertson force-pushed the dmr/rate-limit-remote-joins branch from 3f58763 to 121590a Compare July 4, 2022 18:10
@@ -1380,6 +1380,22 @@ rc_joins:
burst_count: 12
```
---
### `rc_joins_per_room`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind adding info here about what release this config option will be added in?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can do.

synapse/api/ratelimiting.py Outdated Show resolved Hide resolved
@H-Shay
Copy link
Contributor

H-Shay commented Jul 6, 2022

I am going to leave it to someone more confident than me to approve this but from my very limited vantage point it makes sense. My only request would be to make sure you add something in the release notes (and maybe even the upgrade notes?) about adding a new config option, as there have been strident requests that we be very clear about what is being added to the config.

@H-Shay H-Shay requested review from H-Shay and a team and removed request for H-Shay July 6, 2022 20:59
@DMRobertson DMRobertson changed the title Rate limit remote joins Rate limiti joins per-room, accounting for joins created by other servers Jul 7, 2022
Copy link
Member

@erikjohnston erikjohnston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM!

synapse/replication/http/send_event.py Outdated Show resolved Hide resolved
@DMRobertson
Copy link
Contributor Author

@H-Shay, please take a look at dcb1683 onwards and let me know what you think!

@DMRobertson DMRobertson requested a review from H-Shay July 8, 2022 16:33
@H-Shay
Copy link
Contributor

H-Shay commented Jul 8, 2022

The changes you asked me to look at seem great, thanks for doing that!

Simpler, cleaner, faster, stronger.
@DMRobertson DMRobertson force-pushed the dmr/rate-limit-remote-joins branch from 744d4fb to 81eb4ab Compare July 8, 2022 17:55
David Robertson added 5 commits July 11, 2022 21:09
- rename the method to distinguish it from `add_hashes_and_signatures`
- use it in a few other places where it makes sense
@DMRobertson
Copy link
Contributor Author

I have pushed a bunch of changes. I probably shouldn't have snuck in 8377172 but oh well.

This is now fairly large. I am happy to split this up into smaller PRs if you would prefer.

@DMRobertson DMRobertson requested a review from erikjohnston July 11, 2022 20:15
@DMRobertson
Copy link
Contributor Author

Tests failing because joining a room now takes one more query.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement per-room rate-limiting for remote joins
3 participants