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.

Sign up for GitHub

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

Jump to bottom

feat(snownet): timeout connections if we don't receive a candidate within 10s #3790

Merged
merged 8 commits into from
Mar 9, 2024
Merged

feat(snownet): timeout connections if we don't receive a candidate within 10s #3790

merged 8 commits into from
Mar 9, 2024

Conversation

thomaseizinger
Copy link
Member

Previously, we had a dedicated timer for this within the tunnel implementation. Now that we have control over the internals of our connection via snownet, we can timeout the connection if we don't receive a candidate from the remote within 10s.

@vercel Vercel
Copy link

vercel bot commented Feb 28, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
firezone ⬜️ Ignored (Inspect) Visit Preview Mar 8, 2024 2:34am

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 28, 2024 •
edited
Loading

Terraform Cloud Plan Output

Plan: 8 to add, 7 to change, 8 to destroy.

Terraform Cloud Plan

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 28, 2024 •
edited
Loading

Performance Test Results

TCP

Test Name Received/s Sent/s Retransmits
direct-tcp-client2server 65.6 MiB (+0%) 66.2 MiB (+1%) 47 (-2%)
direct-tcp-server2client 53.5 MiB (-1%) 53.8 MiB (-1%) 37 (+3%)
relayed-tcp-client2server 30.0 MiB (+4%) 32.4 MiB (+5%) 105 (-67%)
relayed-tcp-server2client 28.5 MiB (+1%) 30.4 MiB (-0%) 430 (+199%)

UDP

Test Name Total/s Jitter Lost
direct-udp-client2server 50.0 MiB (+0%) 0.17ms (-35%) 0.00% (NaN%)
direct-udp-server2client 50.0 MiB (-0%) 0.21ms (+1%) 0.00% (NaN%)
relayed-udp-client2server 50.0 MiB (-0%) 0.30ms (-8%) 19.85% (+3%)
relayed-udp-server2client 50.0 MiB (+0%) 0.41ms (-10%) 24.66% (+30%)

@thomaseizinger thomaseizinger force-pushed the feat/snownet/timeout-no-candidates branch from 6efae39 to 51661b6 Compare February 28, 2024 07:36
@thomaseizinger thomaseizinger marked this pull request as draft February 28, 2024 07:36
@thomaseizinger thomaseizinger force-pushed the feat/snownet/timeout-no-candidates branch 2 times, most recently from 0771a3d to c9b26b7 Compare February 28, 2024 08:03
@thomaseizinger
Copy link
Member Author

@conectado Do you have any idea why these integration tests could fail with this change? Am I not understanding correctly what this timer did?

@conectado
Copy link
Collaborator

@conectado Do you have any idea why these integration tests could fail with this change? Am I not understanding correctly what this timer did?

Looking at the code it seems equivalent but I don't see the second connection being started in the client logs. I'll later checkout this branch and try to debug it locally.

@thomaseizinger thomaseizinger force-pushed the feat/snownet/timeout-no-candidates branch from 28fe87f to 83cc33e Compare March 4, 2024 05:38
@thomaseizinger thomaseizinger mentioned this pull request Mar 5, 2024
Closed
@thomaseizinger thomaseizinger force-pushed the feat/snownet/timeout-no-candidates branch from 6e1d60b to f0d7de2 Compare March 5, 2024 03:59
thomaseizinger
thomaseizinger commented Mar 5, 2024
View reviewed changes
scripts/nix/flake.nix
@@ -51,7 +51,7 @@
};

devShell = pkgs.mkShell {
packages = [ pkgs.cargo-tauri ];
packages = [ pkgs.cargo-tauri pkgs.iptables ];
Copy link
Member Author

Choose a reason for hiding this comment

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

To run the test scripts locally, we need iptables.

thomaseizinger
thomaseizinger commented Mar 5, 2024
View reviewed changes
rust/connlib/tunnel/src/client.rs
@@ -29,7 +29,6 @@ use tokio::time::{Interval, MissedTickBehavior};
// Using str here because Ipv4/6Network doesn't support `const` 🙃
const IPV4_RESOURCES: &str = "100.96.0.0/11";
const IPV6_RESOURCES: &str = "fd00:2021:1111:8000::/107";
const MAX_CONNECTION_REQUEST_DELAY: Duration = Duration::from_secs(10);
Copy link
Member Author

Choose a reason for hiding this comment

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

The new timeout in snownet is also 10s.

@thomaseizinger thomaseizinger force-pushed the feat/snownet/timeout-no-candidates branch from 253c201 to 81f0205 Compare March 5, 2024 05:07
@thomaseizinger
Copy link
Member Author

This is really strange. I can't reproduce this failure locally.

What seems to happen is that the connection never times out despite the relay being restarted.

@thomaseizinger thomaseizinger force-pushed the feat/snownet/timeout-no-candidates branch from a399e10 to 8d7970d Compare March 5, 2024 06:18
@thomaseizinger
Copy link
Member Author

This is really strange. I can't reproduce this failure locally.

What seems to happen is that the connection never times out despite the relay being restarted.

Ha! It turned out to be a busy-loop when computing the next timeout :)

@thomaseizinger thomaseizinger marked this pull request as ready for review March 5, 2024 06:26
@thomaseizinger thomaseizinger force-pushed the feat/snownet/timeout-no-candidates branch from 8d7970d to a61d6d0 Compare March 5, 2024 06:26
@thomaseizinger
Copy link
Member Author

@conectado Ready for review now. I've split up the commits into atomic ones.

@thomaseizinger thomaseizinger force-pushed the feat/snownet/timeout-no-candidates branch from a61d6d0 to edd804f Compare March 7, 2024 02:28
@thomaseizinger thomaseizinger added this pull request to the merge queue Mar 9, 2024
Merged via the queue into main with commit ea53ae7 Mar 9, 2024
64 checks passed
@thomaseizinger thomaseizinger deleted the feat/snownet/timeout-no-candidates branch March 9, 2024 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@conectado conectado conectado approved these changes

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@thomaseizinger @conectado