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
Closed

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

wants to merge 23 commits into from

Conversation

DMRobertson
Copy link
Contributor

@DMRobertson DMRobertson commented Jul 4, 2022 •
edited
Loading

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
Define config for room-level join limiter
bd873e6 
but don't use it in tests
Rate limiter: describe leaky bucket
453f621
Rate limiter: Pull out some small methods
c2e3025
Rate limiter: Introduce record_action
c594ab7
Notifier: accept callbacks to fire on room joins
9d4cdae
Room member: drive-by-comment
77de159
Replication: include outlier in event rows
ae788ca 
Warn about replication problem in upgrade notes
Add helper to determine if we persist event or not
4230112
Track per-room join rates actioned by this worker
6b47e82 
and consult it when actioning joins.
Only bump rate limit if we will persist the event; otherwise we'll see
it over replication
Snoop on replication to learn about joins on other workers
0bb4122
Test cases
7a14b94
Fixes to unit tests
240e32f 
- 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.
Changelog
121590a
H-Shay
H-Shay reviewed Jul 6, 2022
View reviewed changes
docs/usage/configuration/config_documentation.md
@@ -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.

H-Shay
H-Shay reviewed Jul 6, 2022
View reviewed changes
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
erikjohnston
erikjohnston approved these changes Jul 8, 2022
View reviewed changes
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!

Update rate limiter in the event persister logic
81eb4ab 
Simpler, cleaner, faster, stronger.
David Robertson added 5 commits July 11, 2022 21:09
Use add_hashes_and_signatures_from_other_server
8377172 
- rename the method to distinguish it from `add_hashes_and_signatures`
- use it in a few other places where it makes sense
Don't require ratelimit to notify for joins
47acf46
Also notify of join events via another path
fda6252
Monstrous test update of doom
5cf6700
Don't use newsfiles for config changes
80e2e0b 
per discussion today
@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
Copy link
Contributor Author

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

@DMRobertson DMRobertson mentioned this pull request Jul 12, 2022
Merged
@DMRobertson DMRobertson marked this pull request as draft July 12, 2022 10:56
This was referenced Jul 12, 2022
Merged
Merged
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@H-Shay H-Shay Awaiting requested review from H-Shay

@erikjohnston erikjohnston Awaiting requested review from erikjohnston

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement per-room rate-limiting for remote joins
3 participants
@DMRobertson @H-Shay @erikjohnston