docs(connlib): add more inline docs to connlib's state #5105
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Terraform Cloud Plan Output |
Performance Test ResultsTCP
UDP
|
thomaseizinger
force-pushed
the
docs/tunnel
branch
from
091a6ee
to
4cd35e5
Compare
| @@ -281,37 +281,61 @@ fn send_dns_answer( | |||
| } | |||
| } | |||
|
|
|||
| /// A SANS-IO implementation of a client's functionality. | |||
| /// | |||
| /// Internally, this composes a [`snownet::ClientNode`] with firezone's policy engine around resources. | |||
There was a problem hiding this comment.
I did cargo doc --no-deps --open -p firezone-tunnel and this link didn't show up?
There was a problem hiding this comment.
I guess because snownet is not in the workspace
There was a problem hiding this comment.
You need to build the docs for the entire rust/ directory. Then the links work.
| /// | ||
| /// We assign one internal IP per externally resolved IP. | ||
| dns_resources_internal_ips: HashMap<DnsResource, HashSet<IpAddr>>, | ||
| /// DNS queries we can only answer once we have connected to the resource. |
There was a problem hiding this comment.
Is that connecting to the Resource or to the Gateway? It's the Gateway that resolves DNS, and the Resource doesn't do anything except accepting incoming connections after we've resolved it, right?
There was a problem hiding this comment.
I think this is for the special case where the configured upstream DNS server is a resource itself.
@conectado Can probably answer this.
There was a problem hiding this comment.
These are for any DNS query that hasn't been answered by the gateway yet, we use this to create a response later on.
we don't really connect to a resource ever, but we have the gateway grant access to a resource, at that point it's resolved.
There was a problem hiding this comment.
Ah I forgot to address this one. Will send another PR once it is merged.
| ip_provider: IpProvider, | ||
| /// Maps from connlib-assigned IP of a DNS server back to the originally configured system DNS resolver. | ||
| dns_mapping: BiMap<IpAddr, DnsServer>, | ||
| /// When to next refresh DNS resources. |
There was a problem hiding this comment.
What does this do? Do we redo DNS resolving even while connected to a Resource? Or is this the DNS TTL?
There was a problem hiding this comment.
Good question, let me dig into this.
There was a problem hiding this comment.
It is the former. I clarified this in a comment.
There was a problem hiding this comment.
the only reason we don't use the TTL here is because we don't have it, we would need to use something like hickory on the gateway too to obtain it.
with the DNS refactor we are doing now this will go away anyways and refreshing will look really different with a whole new set of problems 😛
rust/connlib/tunnel/src/lib.rs
Outdated
| /// [`Tunnel`] glues together connlib's [`Io`] component and the respective (pure) state of a client or gateway. | ||
| /// | ||
| /// Most of connlib's functionality is implemented as a pure state machine in [`ClientState`] and [`GatewayState`]. | ||
| /// The only job of [`Tunnel`] is to take input from the TUN [`Device`], [`Sockets`] or time and pass it to the respective state. |
There was a problem hiding this comment.
🤔 And the Portal connection via WebSocket / phoenix channel is not part of that pure state machine, right? It's also part of the Tunnel?
So for my goal of exercising the platform-specific I/O code of the Clients, even in Windows where a Portal can't be run, I could make a mock Tunnel implementation that uses the pure state machine but has fake I/O, similar to your prop tests, and then has a simulated portal?
Or is the WebSocket managed even higher, in ClientTunnel and GatewayTunnel wrappers?
There was a problem hiding this comment.
I think it should be part of the Tunnel. There is no reason why the Eventloops for gateways and client couldn't be in Tunnel (those manage the portal connection). Doing that is tracked in #3837.
But even once that is done, the connection to the portal is a side-effect and thus would likely be contained in Io or live at least next to it.
ClientState and GatewayState are already pure state machines so you can already run them on Windows without a portal or TUN device.
So for my goal of exercising the platform-specific I/O code of the Clients, even in Windows
Our proptests mock everything. IO, portal, TUN device. Our (linux) integration tests don't mock anything, they use a real portal, real TUN device and real sockets.
I don't think it makes much sense to mock only parts of this. If we want to test sockets and TUN device on Windows, and thus perform real network communication, we already have an abstraction layer that can cross OS boundaries: the network. I'd try to somehow connect to a real portal and real gateway from a Windows-VM. Like, can we run a Linux container in a Windows VM on GitHub actions and connect to that?
There was a problem hiding this comment.
Actually, maybe we could. Github docs say the Windows runners have Docker installed.
But I haven't tried it because I don't want to spend all day fighting the CI again. Maybe if it was done in a side project so every run doesn't take 6 CPU-hours 🤔 And then integrated if it succeeds
It's just that, that's one more thing that has to be downloaded, installed, set up, another few gigabytes of junk to update. Testing everything end-to-end is great but I'd also just like to test the Windows to Windows Client integration sometimes.
There was a problem hiding this comment.
It's just that, that's one more thing that has to be downloaded, installed, set up, another few gigabytes of junk to update. Testing everything end-to-end is great but I'd also just like to test the Windows to Windows Client integration sometimes.
Yeah that is definitely worthwhile testing. I've found this: actions/runner-images#2216 (comment)
Seems like we need a self-hosted Windows runner for that.
There was a problem hiding this comment.
Crud, yeah that was why I didn't try it before. Github doesn't do nested virtualization and Docker / WSL2 needs that to run Linux on Windows.
Co-authored-by: Reactor Scram <ReactorScram@users.noreply.github.com> Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
This is a follow-up to #5035. I didn't end up renaming
Tunnel,GatewayStateorClientStatebut I've added some comments with my understanding of what the state is we are tracking and tried to group the fields together in a logical way.