fix(connlib): deterministically route packets in case of overlap #5082
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
|
Draft because it is stacked, otherwise ready for review. @jamilbk Please review the PR description to make sure the new behaviour is what we want. |
Terraform Cloud Plan Output |
Performance Test ResultsTCP
UDP
|
thomaseizinger
force-pushed
the
fix/connlib/pick-gateway-based-on-resource-ip
branch
from
09bd673
to
65d52a1
Compare
rust/connlib/tunnel/src/client.rs
Outdated
| let maybe_dns_resource_id = self | ||
| .dns_resources_internal_ips | ||
| .iter() | ||
| .find_map(|(r, i)| i.contains(&destination).then_some(r.id)); |
There was a problem hiding this comment.
Can this cause a performance regression? Might be worth the extra state keeping an additional table mapping ip addrs to peers?
There was a problem hiding this comment.
Yeah I thought about that too. I think using an IpNetworkTable would make sense here. More ergonomic to use as well.
There was a problem hiding this comment.
I don't think we need that, since we always assign IpAddrs for dns resources or you want to combine both CIDR and DNS on a single table?
There was a problem hiding this comment.
Ah, you mean a regular HashMap. Yeah that makes more sense. We'd always to an exact_match so it is redundant to use an IpNetworkTable.
There was a problem hiding this comment.
Hmm, it could be combined though! Like into a more general "routing table":
- CIDR resources go straight into that routing table
- Resolved DNS IPs also go into that table
Each packet goes through the path of:
- Lookup destination IP in routing table to find our
ResourceId - Lookup gateway based on resource ID
- Connect to gateway if aren't yet connected
There was a problem hiding this comment.
I've captured this idea in #5114.
|
@conectado I've swapped the map to go from IP to |
Currently, we only consult the IP ranges of our configured resources for the initial connection to a gateway. Once a connection is established, packets are routed based on an IP range associated with that gateway. This is inconsistent and actually causes problems in case the user configures overlapping resources. In particular, adding a resource with an overlapping but narrower IP network range to a client that is already connected to a gateway with an overlapping but wider range will cause all packets for the newly added resource to be routed to the already connected gateway.
To fix this, we consult the IP network table of resources for each packet to figure out, which resource is the most appropriate one. Then, we pick the gateway that is configured for this resource. If we aren't connected to that gateway or if we don't know about a gateway for this resource, we emit a connection intent.
In case the portal wants to use an already connected gateway for that resource, we handle that using the "reuse connection" message to the portal.
In fixing this, I also realised that I think this has (positive) audit consequences. In particular, this will now correctly report access to a resource if it is overlapping as described above (i.e. a narrower overlapping resource is added whilst being connected to one with a wider range). I believe that previously, this access would have not been reported because we would have simply routed the packet to the already connected gateway.
Fixes: #5054.