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.

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

docs(connlib): add more inline docs to connlib's state #5105

Merged
merged 7 commits into from
May 24, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions rust/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ clippy.unused_async = "warn"
clippy.wildcard_enum_match_arm = "warn" # Ensures we match on all combinations of `Poll`, preventing erroneous suspensions.
clippy.redundant_else = "warn"
clippy.redundant_clone = "warn"
rustdoc.private-intra-doc-links = "allow" # We don't publish any of our docs but want to catch dead links.

[patch.crates-io]
boringtun = { git = "https://github.com/cloudflare/boringtun", branch = "master" }
Expand Down
58 changes: 41 additions & 17 deletions rust/connlib/tunnel/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -281,37 +281,61 @@ fn send_dns_answer(
}
}

/// A SANS-IO implementation of a client's functionality.
thomaseizinger marked this conversation as resolved.
Show resolved Hide resolved
///
/// Internally, this composes a [`snownet::ClientNode`] with firezone's policy engine around resources.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I did cargo doc --no-deps --open -p firezone-tunnel and this link didn't show up?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess because snownet is not in the workspace

Copy link
Member Author

Choose a reason for hiding this comment

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

You need to build the docs for the entire rust/ directory. Then the links work.

/// Clients differ from gateways in that they also implement a DNS resolver for DNS resources.
/// They also initiate connections to gateways based on packets sent to resources.
thomaseizinger marked this conversation as resolved.
Show resolved Hide resolved
pub struct ClientState {
/// The [`snownet::ClientNode`].
///
/// Manages wireguard tunnels to gateways.
thomaseizinger marked this conversation as resolved.
Show resolved Hide resolved
node: ClientNode<GatewayId, RelayId>,
/// All peers gateways we are connected to and the associated, connection-specific state.
thomaseizinger marked this conversation as resolved.
Show resolved Hide resolved
peers: PeerStore<GatewayId, GatewayOnClient>,
/// Which resources are trying to connect to.
thomaseizinger marked this conversation as resolved.
Show resolved Hide resolved
awaiting_connection: HashMap<ResourceId, AwaitingConnectionDetails>,

/// Tracks, which gateway to use for a particular resource.
thomaseizinger marked this conversation as resolved.
Show resolved Hide resolved
resources_gateways: HashMap<ResourceId, GatewayId>,
/// The site a gateway belongs to.
gateways_site: HashMap<GatewayId, SiteId>,
/// The online/offline status of a site.
sites_status: HashMap<SiteId, Status>,

pub dns_resources_internal_ips: HashMap<DnsResource, HashSet<IpAddr>>,
/// All DNS resources we know about, indexed by their domain (could be wildcard domain like *.mycompany.com).
thomaseizinger marked this conversation as resolved.
Show resolved Hide resolved
dns_resources: HashMap<String, ResourceDescriptionDns>,
/// All CIDR resources we know about, indexed by the IP range they cover (like `1.1.0.0/8`).
cidr_resources: IpNetworkTable<ResourceDescriptionCidr>,
pub resource_ids: HashMap<ResourceId, ResourceDescription>,
pub deferred_dns_queries: HashMap<(DnsResource, Rtype), IpPacket<'static>>,

peers: PeerStore<GatewayId, GatewayOnClient>,

node: ClientNode<GatewayId, RelayId>,

pub ip_provider: IpProvider,

dns_mapping: BiMap<IpAddr, DnsServer>,
/// All resources indexed by their ID.
resource_ids: HashMap<ResourceId, ResourceDescription>,

buffered_events: VecDeque<ClientEvent>,
interface_config: Option<InterfaceConfig>,
buffered_packets: VecDeque<IpPacket<'static>>,
/// The DNS resolvers configured on the system prior to connlib starting up.
thomaseizinger marked this conversation as resolved.
Show resolved Hide resolved
system_resolvers: Vec<IpAddr>,

/// DNS queries that we need to forward to the system resolver.
buffered_dns_queries: VecDeque<DnsQuery<'static>>,

/// The (internal) IPs we have assigned for a certain DNS resource.
///
/// 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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is for the special case where the configured upstream DNS server is a resource itself.

@conectado Can probably answer this.

Copy link
Collaborator

@conectado conectado May 24, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I forgot to address this one. Will send another PR once it is merged.

///
/// See [`dns::ResolveStrategy`] for details.
deferred_dns_queries: HashMap<(DnsResource, Rtype), IpPacket<'static>>,
/// Hands out IPs for DNS resources.
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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this do? Do we redo DNS resolving even while connected to a Resource? Or is this the DNS TTL?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question, let me dig into this.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is the former. I clarified this in a comment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 😛

next_dns_refresh: Option<Instant>,

system_resolvers: Vec<IpAddr>,
/// Configuration of the TUN device, when it is up.
interface_config: Option<InterfaceConfig>,

gateways_site: HashMap<GatewayId, SiteId>,
sites_status: HashMap<SiteId, Status>,
buffered_events: VecDeque<ClientEvent>,
buffered_packets: VecDeque<IpPacket<'static>>,
}

#[derive(Debug, Clone, PartialEq, Eq)]
Expand Down
12 changes: 11 additions & 1 deletion rust/connlib/tunnel/src/gateway.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,10 +125,20 @@ where
}
}

/// A SANS-IO implementation of a gateway's functionality.
///
/// Internally, this composes a [`snownet::ServerNode`] with firezone's policy engine around resources.
pub struct GatewayState {
peers: PeerStore<ClientId, ClientOnGateway>,
/// The [`snownet::ClientNode`].
///
/// Manages wireguard tunnels to clients.
node: ServerNode<ClientId, RelayId>,
/// All clients we are connected to and the associated, connection-specific state.
peers: PeerStore<ClientId, ClientOnGateway>,

/// When to next check whether a resource-access policy has expired.
next_expiry_resources_check: Option<Instant>,

buffered_events: VecDeque<GatewayEvent>,
}

Expand Down
7 changes: 6 additions & 1 deletion rust/connlib/tunnel/src/io.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,15 @@ use std::{

const DNS_QUERIES_QUEUE_SIZE: usize = 100;

/// Bundles together all side-effects that connlib needs to have access to.
pub struct Io {
/// The TUN device offered to the user.
///
/// This is the `firezone-tun` network interface that users see when they e.g. type `ip addr` on Linux.
thomaseizinger marked this conversation as resolved.
Show resolved Hide resolved
device: Device,
timeout: Option<Pin<Box<tokio::time::Sleep>>>,
/// The UDP sockets used to send & receive packets from the network.
sockets: Sockets,
timeout: Option<Pin<Box<tokio::time::Sleep>>>,

upstream_dns_servers: HashMap<IpAddr, TokioAsyncResolver>,
forwarded_dns_queries: FuturesTupleSet<
Expand Down
10 changes: 8 additions & 2 deletions rust/connlib/tunnel/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,19 @@ const FIREZONE_MARK: u32 = 0xfd002021;
pub type GatewayTunnel<CB> = Tunnel<CB, GatewayState>;
pub type ClientTunnel<CB> = Tunnel<CB, ClientState>;

/// Tunnel is a wireguard state machine that uses webrtc's ICE channels instead of UDP sockets to communicate between peers.
/// [`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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

🤔 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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

pub struct Tunnel<CB: Callbacks, TRoleState> {
pub callbacks: CB,

/// State that differs per role, i.e. clients vs gateways.
/// (pure) state that differs per role, either [`ClientState`] or [`GatewayState`].
role_state: TRoleState,

/// The [`Io`] component of connlib.
thomaseizinger marked this conversation as resolved.
Show resolved Hide resolved
///
/// Handles all side-effects.
io: Io,

write_buf: Box<[u8; MAX_UDP_SIZE]>,
Expand Down
Loading