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

Track the number of active inbound and outbound peer connections #2912

Merged
merged 4 commits into from
Oct 21, 2021

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Oct 20, 2021

Motivation

To limit the number of connections in Zebra, we need to be able to track how many open connections there are.

Solution

  • Track the number of open connections using a new ActiveConnectionCounter type
  • Create a ConnectionTracker type that increases the count when it's created, and decreases the count when it's dropped
  • Increase the count before opening new connections
  • Reduce the connection count when Connections close or fail, by adding a ConnectionTracker to each Connection

Closes #2903.
Closes #2904.

This PR will get tested as part of tickets #1850 and #2902, when we actually enforce connection limits.

Review

@oxarbitrage and @dconnolly wanted to work on the security tickets that depend on this PR.
So they probably want to review it.

This PR is on the critical path for the Zebra beta, so it's urgent.

Reviewer Checklist

Security edge-cases:

  • Zebra should check and increase the connection limit as early as possible, before it uses any resources for the connection
  • Zebra should send the drop signal as late as possible, after it has dropped everything else that's used by the connection
  • How can we make sure the inbound and outbound signals don't block each other?
    • They use completely separate channel instances via a wrapper type.
  • How can we make sure we don't get the inbound and outbound signals mixed up?
    • Separate channels are created within the inbound and outbound tasks. They aren't shared.

Follow Up Work

@teor2345 teor2345 added P-High C-security Category: Security issues I-unbounded-growth Zebra keeps using resources, without any limit A-network Area: Network protocol updates or fixes labels Oct 20, 2021
@teor2345 teor2345 self-assigned this Oct 20, 2021
@zfnd-bot zfnd-bot bot added this to In progress in 🦓 Oct 20, 2021
@teor2345 teor2345 marked this pull request as draft October 20, 2021 05:03
@teor2345 teor2345 force-pushed the connection-count branch 2 times, most recently from 8a9d0eb to 286ce22 Compare October 21, 2021 04:35
@teor2345 teor2345 changed the title WIP: Track the number of open peer connections Track the number of active inbound and outbound peer connections Oct 21, 2021
And reduce the count when each connection fails.
@teor2345 teor2345 marked this pull request as ready for review October 21, 2021 06:03
oxarbitrage
oxarbitrage previously approved these changes Oct 21, 2021
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

I think this looks great.

zebra-network/src/peer_set/limit.rs Outdated Show resolved Hide resolved
Co-authored-by: Alfredo Garcia <oxarbitrage@gmail.com>
@teor2345 teor2345 enabled auto-merge (squash) October 21, 2021 20:24
@teor2345 teor2345 merged commit 4cdd12e into main Oct 21, 2021
🦓 automation moved this from In progress to Done Oct 21, 2021
@teor2345 teor2345 deleted the connection-count branch October 21, 2021 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-network Area: Network protocol updates or fixes C-security Category: Security issues I-unbounded-growth Zebra keeps using resources, without any limit
Projects
No open projects
🦓
  
Done
2 participants