-
Notifications
You must be signed in to change notification settings - Fork 288
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
Refactor ip in use algorithm #4268
Conversation
Skipping CI for Draft Pull Request. |
Codecov Report
@@ Coverage Diff @@
## main #4268 +/- ##
==========================================
- Coverage 68.51% 68.50% -0.02%
==========================================
Files 404 404
Lines 32940 32939 -1
==========================================
- Hits 22568 22564 -4
- Misses 8924 8926 +2
- Partials 1448 1449 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chrisdoherty4 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
/lgtm
Refactor the IsIPInUse algorithm to be faster and more reliable.
The IsIPInUse algorithm iterated over a series of ports testing if they were open at some target address via TCP. It failed to take into consideration devices that are serving the IP but have no services listening on the checked ports resulting in a false negative.
Instead of relying on ports we can focus on whether the device responded. That can be achieved by either validating a TCP connection was in-fact established, or simply that the connection was reset by the server. The timeout period is still our indicator that the IP is not in use.
This check is still best effort but fixes the false negative case where none of the checked ports are open while also being 2s faster making it more appropriate for use in webhooks if desired.
Additional testing
I manually verified the errors returned by running the algorithm as part of an independent binary.