-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
groups: fix role deletion DDOS #3355
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fang-
reviewed
Mar 20, 2024
Co-authored-by: fang <git@fang.io>
Co-authored-by: fang <git@fang.io>
…scape-apps into hm/fix-role-deletions-ddos
Fang-
reviewed
Mar 22, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better already! Some further comments on implementation details, if you'll forgive my nitpicking.
Fang-
approved these changes
Mar 27, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will do!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR should fix LAND-1698 by doing all our role removals in one event rather than an event for each member of the group. It also prevents channels from attempting to resubscribe to channels that are no longer associated with the group. This is untested, but should fix the issues we've been seeing.
todos:
As far as we understand the issue with role deletion was as follows:
host
delete role in UI (1 event) -> verify group cabal(role) data -> check every member's roles for old roles -> poke self for each member that needs a role change (5k events) -> disseminate events -> each event goes to 5k subscribers ... eventually receive resubscribes for channels
subscribers
receive role change event for each member (5k events) -> recheck permissions of each channel -> for each channel that we can read, try to resubscribe if subscription is missing (5-10 events)
So we can calculate these by multiplying for each step. Also some of these events were ran multiple times:
5 tries * 5000 role change events * 5000 ships to distribute each event to = 125 million facts needing to be sent from nibset
meanwhile each time a ship hears any of those events it sends back 5 more events trying to resubscribe to deleted channels so 125m * 5 = 625 million
so in total facts/messages:
which means nibset has been chewing through 1.375 b events. This makes me think we should have diagrams for each event flow to make sure we never cascade like this.
PR Checklist