refactor(connlib): drop all connections when roaming #5308
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Terraform Cloud Plan Output |
|
This already works for CIDR resources but for DNS resources, we still need to wait on #5049. |
Performance Test ResultsTCP
UDP
|
thomaseizinger
force-pushed
the
refactor/connlib/5080-drop-connections
branch
from
0625115
to
9b68bb1
Compare
Closed
thomaseizinger
force-pushed
the
refactor/connlib/5080-drop-connections
branch
from
9b68bb1
to
1b1fc6e
Compare
thomaseizinger
changed the base branch from
main
to
refactor/dns-translation-on-gateway
thomaseizinger
force-pushed
the
refactor/connlib/5080-drop-connections
branch
4 times, most recently
from
1fa1523
to
22ba907
Compare
thomaseizinger
changed the title
thomaseizinger
force-pushed
the
refactor/connlib/5080-drop-connections
branch
from
22ba907
to
8607b64
Compare
thomaseizinger
changed the title
thomaseizinger
commented
Jun 18, 2024
There was a problem hiding this comment.
Had to slightly adopt this download test: curl appears to not send any packets as part of downloading and thus once interrupted due to roaming, the connection does not get re-established. I added --retry 1 and --continue-at - to retry the download on failure. Previously, the file was just appended via stdout which failed the hash-check after the retry. Now we use curl's --output which means we also need to calculate the hash in the container.
There was a problem hiding this comment.
We should have a user-facing documentation of this behavior.
We want users to know that downloads won't resume after roaming automatically, I think this is okay and expected for TCP connections, but it's still good to document.
thomaseizinger
force-pushed
the
refactor/dns-translation-on-gateway
branch
from
cba6435
to
56eeaeb
Compare
thomaseizinger
force-pushed
the
refactor/connlib/5080-drop-connections
branch
from
780eb8c
to
66768c1
Compare
thomaseizinger
force-pushed
the
refactor/dns-translation-on-gateway
branch
from
56eeaeb
to
98f567a
Compare
thomaseizinger
force-pushed
the
refactor/connlib/5080-drop-connections
branch
from
66768c1
to
ad5034d
Compare
thomaseizinger
force-pushed
the
refactor/dns-translation-on-gateway
branch
from
98f567a
to
fe6cced
Compare
thomaseizinger
force-pushed
the
refactor/connlib/5080-drop-connections
branch
from
ad5034d
to
c57e07e
Compare
thomaseizinger
force-pushed
the
refactor/dns-translation-on-gateway
branch
from
fe6cced
to
f3e770f
Compare
thomaseizinger
force-pushed
the
refactor/connlib/5080-drop-connections
branch
from
c57e07e
to
18231be
Compare
thomaseizinger
force-pushed
the
refactor/dns-translation-on-gateway
branch
from
f3e770f
to
5e2a885
Compare
thomaseizinger
force-pushed
the
refactor/connlib/5080-drop-connections
branch
2 times, most recently
from
59eb3dd
to
cc74a8b
Compare
thomaseizinger
force-pushed
the
refactor/connlib/5080-drop-connections
branch
from
cc74a8b
to
412db5b
Compare
conectado
approved these changes
Jun 25, 2024
Comment on lines
+822
to
+824
| for id in ids { | ||
| self.cleanup_connected_gateway(&id); | ||
| } |
There was a problem hiding this comment.
a debug_assert! here that all peers are empty would be nice I think
There was a problem hiding this comment.
We should have a user-facing documentation of this behavior.
We want users to know that downloads won't resume after roaming automatically, I think this is okay and expected for TCP connections, but it's still good to document.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jun 27, 2024
In order to handle DNS resources, connlib intercepts all DNS requests on the system once it has started up. The DNS queries are then forwarded to the original DNS resolver in case the query isn't for one of the configured DNS resources _except_ if the configured DNS resovler is also a CIDR resource. In that case, the DNS query will be tunneled to a gateway and forwarded to the DNS resolver from there. Exactly this configuration results in a dead-lock when roaming networks. To make roaming more reliable, we now drop all connections when detecting a network change (see #5308). As a result, DNS queries cannot be tunneled right away. This isn't usually a problem: We just send a connection intent to the portal to connect to the gateway. Upon a network change, we also reconnect the websocket to the portal which also requires to resolve the domain name. Connlib's DNS resolver is still active at the point and thus, we end up deadlocking ourselves because the DNS query to resolve the portal's domain is waiting for a connection to a gateway that can only be established once we are connected to the portal. To prevent this, we extend connlib with a "known hosts" feature. These are DNS records that are defined statically for the lifetime of a connlib session and can thus always be resolved, regardless of the connection state with the portal or the gateways. We populate these records with the portal's API, allowing the reconnect to work without having connected gateways. --------- Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
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.
Currently,
snownettries to be very clever in how it roams connections. This is/was necessary because we associated DNS-specific state with a connection. More specifically, the assigned proxy IPs for a DNS resource are stored as part of a connection with the gateway.As a result, DNS resources would always break if the underlying connection in
snownetfailed. This is quite error prone and means,snownetmust be very careful to never-ever fail a connection erroneously. With #5049, we no longer store any important state with a connection and thus, can implement roaming in much simpler way: Drop all connections and let the incoming packets create new ones. This is much more robust as we don't have to "patch" existing state insnownetas part of roaming.We test this new functionality by adding a
RoamClienttransition totunnel_test. This ensures roaming works in a lot of scenarios, including relayed and non-relayed situations as well as roaming between either of them. As a result, we can delete several of the more specific test cases ofsnownet.Depends-On: #5049.
Replaces: #5060.
Resolves: #5080.