Skip to content
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

subscriptions: kick resub ddos protection #3212

Merged
merged 11 commits into from
Feb 22, 2024

Conversation

arthyn
Copy link
Member

@arthyn arthyn commented Feb 5, 2024

Fixes LAND-1527 by adding a library with a mechanism for us to delay resubscriptions by 30 seconds to ensure that we don't take up all CPU/event throughput of a ship when in a kick + resubscribe loop. This was added to only groups and channels, but could stand to be applied elsewhere.

PR Checklist

  • Includes changes to desk files
  • Describes how you tested the PR locally (test ship vs livenet)
  • If a new feature, includes automated tests
  • Comments added anywhere logic may be confusing without context

@arthyn arthyn requested review from midsum-salrux and Fang- February 8, 2024 01:02
desk/lib/subscriber.hoon Outdated Show resolved Hide resolved
desk/tests/app/groups.hoon Outdated Show resolved Hide resolved
@arthyn arthyn changed the title (WIP) subscriptions: kick resub ddos protection subscriptions: kick resub ddos protection Feb 13, 2024
@arthyn arthyn requested a review from Fang- February 13, 2024 01:19
Copy link

linear bot commented Feb 13, 2024

@arthyn arthyn marked this pull request as ready for review February 13, 2024 01:19
Copy link
Member

@Fang- Fang- left a comment

Choose a reason for hiding this comment

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

Mostly nits, but also two important things about +unsubscribe.

desk/lib/subscriber.hoon Show resolved Hide resolved
desk/lib/subscriber.hoon Outdated Show resolved Hide resolved
desk/lib/subscriber.hoon Outdated Show resolved Hide resolved
desk/app/groups.hoon Outdated Show resolved Hide resolved
desk/app/groups.hoon Outdated Show resolved Hide resolved
desk/lib/subscriber.hoon Outdated Show resolved Hide resolved
@arthyn arthyn requested a review from Fang- February 16, 2024 21:27
@arthyn
Copy link
Member Author

arthyn commented Feb 20, 2024

@Fang- added missing leave, but unsure how to test since we don't have a way to trigger an unsubscribe from a group we don't have yet so might need your help there.

@Fang-
Copy link
Member

Fang- commented Feb 21, 2024

I wouldn't be super concerned with testing that, in this case it's plainly visible from the code that we always emit a leave.

But also: the dumb version of that test is testing the library directly. That is, calling the +unsubscribe from the library with any argument, in any context, and seeing that a %leave gets sent despite the subscription not being in the provided state. You don't strictly need to go through an agent, when all you want to do is test library behavior.

I do still notice a lack of +unsubscribe calls in /app/channels. It has a bunch of "naked" %leaves, and one remaining "naked" %watch. Probably want to be consistent about doing every un/subscription through the library, even if the delay case isn't relevant for some particular ones.

Lastly I'd consider adding a little comment at the top of the file explaining that all subscriptions must be managed through the library. Or pointing that out at the /+ of it, or something similar. Just so contributors (internal ones too) are more likely to catch on.

Copy link
Member

@Fang- Fang- left a comment

Choose a reason for hiding this comment

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

Latest commits look good. Thank you for your service!

@arthyn arthyn merged commit ccb391d into develop Feb 22, 2024
1 check passed
@arthyn arthyn deleted the hm/resubscribe-ddos-prevention branch February 22, 2024 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants