mirror of
https://github.com/outbackdingo/firezone.git
synced 2026-01-27 10:18:54 +00:00
chore(connlib): harmonise naming of IDs (#10038)
When filtering through logs in Sentry, it is useful to narrow them down by context of a client, gateway or resource. Currently, these fields are sometimes called `client`, `cid`, `client_id` etc and the same for the Gateway and Resources. To make this filtering easier, name all of them `cid` for Client IDs, `gid` for Gateway IDs and `rid` for Resource IDs.
This commit is contained in:
Thomas Eizinger
@@ -201,15 +201,15 @@ impl Eventloop {
|
||||
fn handle_tunnel_event(&mut self, event: firezone_tunnel::ClientEvent) -> Option<Event> {
|
||||
match event {
|
||||
firezone_tunnel::ClientEvent::AddedIceCandidates {
|
||||
conn_id: gateway,
|
||||
conn_id: gid,
|
||||
candidates,
|
||||
} => {
|
||||
tracing::debug!(%gateway, ?candidates, "Sending new ICE candidates to gateway");
|
||||
tracing::debug!(%gid, ?candidates, "Sending new ICE candidates to gateway");
|
||||
|
||||
self.portal.send(
|
||||
PHOENIX_TOPIC,
|
||||
EgressMessages::BroadcastIceCandidates(GatewaysIceCandidates {
|
||||
gateway_ids: vec![gateway],
|
||||
gateway_ids: vec![gid],
|
||||
candidates,
|
||||
}),
|
||||
);
|
||||
@@ -217,15 +217,15 @@ impl Eventloop {
|
||||
None
|
||||
}
|
||||
firezone_tunnel::ClientEvent::RemovedIceCandidates {
|
||||
conn_id: gateway,
|
||||
conn_id: gid,
|
||||
candidates,
|
||||
} => {
|
||||
tracing::debug!(%gateway, ?candidates, "Sending invalidated ICE candidates to gateway");
|
||||
tracing::debug!(%gid, ?candidates, "Sending invalidated ICE candidates to gateway");
|
||||
|
||||
self.portal.send(
|
||||
PHOENIX_TOPIC,
|
||||
EgressMessages::BroadcastInvalidatedIceCandidates(GatewaysIceCandidates {
|
||||
gateway_ids: vec![gateway],
|
||||
gateway_ids: vec![gid],
|
||||
candidates,
|
||||
}),
|
||||
);
|
||||
|
||||
@@ -589,12 +589,12 @@ impl ClientState {
|
||||
self.drain_node_events();
|
||||
}
|
||||
|
||||
#[tracing::instrument(level = "debug", skip_all, fields(%resource_id))]
|
||||
#[tracing::instrument(level = "debug", skip_all, fields(%rid))]
|
||||
#[expect(clippy::too_many_arguments)]
|
||||
pub fn handle_flow_created(
|
||||
&mut self,
|
||||
resource_id: ResourceId,
|
||||
gateway_id: GatewayId,
|
||||
rid: ResourceId,
|
||||
gid: GatewayId,
|
||||
gateway_key: PublicKey,
|
||||
gateway_tun: IpConfig,
|
||||
site_id: SiteId,
|
||||
@@ -603,30 +603,27 @@ impl ClientState {
|
||||
gateway_ice: IceCredentials,
|
||||
now: Instant,
|
||||
) -> anyhow::Result<Result<(), NoTurnServers>> {
|
||||
tracing::debug!(%gateway_id, "New flow authorized for resource");
|
||||
tracing::debug!(%gid, "New flow authorized for resource");
|
||||
|
||||
let resource = self
|
||||
.resources_by_id
|
||||
.get(&resource_id)
|
||||
.context("Unknown resource")?;
|
||||
let resource = self.resources_by_id.get(&rid).context("Unknown resource")?;
|
||||
|
||||
let Some(pending_flow) = self.pending_flows.remove(&resource_id) else {
|
||||
let Some(pending_flow) = self.pending_flows.remove(&rid) else {
|
||||
tracing::debug!("No pending flow");
|
||||
|
||||
return Ok(Ok(()));
|
||||
};
|
||||
|
||||
if let Some(old_gateway_id) = self.resources_gateways.insert(resource_id, gateway_id)
|
||||
if let Some(old_gateway_id) = self.resources_gateways.insert(rid, gid)
|
||||
&& self.peers.get(&old_gateway_id).is_some()
|
||||
{
|
||||
assert_eq!(
|
||||
old_gateway_id, gateway_id,
|
||||
"Resources are not expected to change gateways without a previous message, resource_id = {resource_id}"
|
||||
old_gateway_id, gid,
|
||||
"Resources are not expected to change gateways without a previous message, resource_id = {rid}"
|
||||
)
|
||||
}
|
||||
|
||||
match self.node.upsert_connection(
|
||||
gateway_id,
|
||||
gid,
|
||||
gateway_key,
|
||||
Secret::new(preshared_key.expose_secret().0),
|
||||
snownet::Credentials {
|
||||
@@ -642,19 +639,19 @@ impl ClientState {
|
||||
Ok(()) => {}
|
||||
Err(e) => return Ok(Err(e)),
|
||||
};
|
||||
self.resources_gateways.insert(resource_id, gateway_id);
|
||||
self.gateways_site.insert(gateway_id, site_id);
|
||||
self.recently_connected_gateways.put(gateway_id, ());
|
||||
self.resources_gateways.insert(rid, gid);
|
||||
self.gateways_site.insert(gid, site_id);
|
||||
self.recently_connected_gateways.put(gid, ());
|
||||
|
||||
if self.peers.get(&gateway_id).is_none() {
|
||||
if self.peers.get(&gid).is_none() {
|
||||
self.peers
|
||||
.insert(GatewayOnClient::new(gateway_id, gateway_tun), &[]);
|
||||
.insert(GatewayOnClient::new(gid, gateway_tun), &[]);
|
||||
};
|
||||
|
||||
// Allow looking up the Gateway via its TUN IP.
|
||||
// Resources are not allowed to be in our CG-NAT range, therefore in practise this cannot overlap with resources.
|
||||
self.peers.add_ip(&gateway_id, &gateway_tun.v4.into());
|
||||
self.peers.add_ip(&gateway_id, &gateway_tun.v6.into());
|
||||
self.peers.add_ip(&gid, &gateway_tun.v4.into());
|
||||
self.peers.add_ip(&gid, &gateway_tun.v6.into());
|
||||
|
||||
// Deal with buffered packets
|
||||
|
||||
@@ -663,17 +660,14 @@ impl ClientState {
|
||||
|
||||
match resource {
|
||||
Resource::Cidr(_) | Resource::Internet(_) => {
|
||||
self.peers.add_ips_with_resource(
|
||||
&gateway_id,
|
||||
resource.addresses().into_iter(),
|
||||
&resource_id,
|
||||
);
|
||||
self.peers
|
||||
.add_ips_with_resource(&gid, resource.addresses().into_iter(), &rid);
|
||||
|
||||
// For CIDR and Internet resources, we can directly queue the buffered packets.
|
||||
for packet in buffered_resource_packets {
|
||||
encapsulate_and_buffer(
|
||||
packet,
|
||||
gateway_id,
|
||||
gid,
|
||||
now,
|
||||
&mut self.node,
|
||||
&mut self.buffered_transmits,
|
||||
@@ -687,7 +681,7 @@ impl ClientState {
|
||||
|
||||
// 2. Buffered UDP DNS queries for the Gateway
|
||||
for packet in pending_flow.udp_dns_queries {
|
||||
let gateway = self.peers.get(&gateway_id).context("Unknown peer")?; // If this error happens we have a bug: We just inserted it above.
|
||||
let gateway = self.peers.get(&gid).context("Unknown peer")?; // If this error happens we have a bug: We just inserted it above.
|
||||
|
||||
let upstream = gateway.tun_dns_server_endpoint(packet.destination());
|
||||
let packet =
|
||||
@@ -695,7 +689,7 @@ impl ClientState {
|
||||
|
||||
encapsulate_and_buffer(
|
||||
packet,
|
||||
gateway_id,
|
||||
gid,
|
||||
now,
|
||||
&mut self.node,
|
||||
&mut self.buffered_transmits,
|
||||
@@ -768,10 +762,10 @@ impl ClientState {
|
||||
self.cleanup_connected_gateway(&disconnected_gateway);
|
||||
}
|
||||
|
||||
#[tracing::instrument(level = "debug", skip_all, fields(%resource))]
|
||||
#[tracing::instrument(level = "debug", skip_all, fields(%rid))]
|
||||
fn on_not_connected_resource(
|
||||
&mut self,
|
||||
resource: ResourceId,
|
||||
rid: ResourceId,
|
||||
trigger: impl Into<ConnectionTrigger>,
|
||||
now: Instant,
|
||||
) {
|
||||
@@ -779,9 +773,9 @@ impl ClientState {
|
||||
|
||||
let trigger = trigger.into();
|
||||
|
||||
debug_assert!(self.resources_by_id.contains_key(&resource));
|
||||
debug_assert!(self.resources_by_id.contains_key(&rid));
|
||||
|
||||
match self.pending_flows.entry(resource) {
|
||||
match self.pending_flows.entry(rid) {
|
||||
Entry::Vacant(v) => {
|
||||
v.insert(PendingFlow::new(now, trigger));
|
||||
}
|
||||
@@ -804,7 +798,7 @@ impl ClientState {
|
||||
|
||||
self.buffered_events
|
||||
.push_back(ClientEvent::ConnectionIntent {
|
||||
resource,
|
||||
resource: rid,
|
||||
connected_gateway_ids: self.connected_gateway_ids(),
|
||||
})
|
||||
}
|
||||
@@ -915,7 +909,7 @@ impl ClientState {
|
||||
.map(|(_, r)| *r)
|
||||
.filter(|resource| self.is_resource_enabled(resource))
|
||||
.inspect(
|
||||
|resource| tracing::trace!(target: "tunnel_test_coverage", %destination, %resource, "Packet for DNS resource"),
|
||||
|rid| tracing::trace!(target: "tunnel_test_coverage", %destination, %rid, "Packet for DNS resource"),
|
||||
);
|
||||
|
||||
// We don't need to filter from here because resources are removed from the active_cidr_resources as soon as they are disabled.
|
||||
@@ -924,7 +918,7 @@ impl ClientState {
|
||||
.longest_match(destination)
|
||||
.map(|(_, res)| res.id)
|
||||
.inspect(
|
||||
|resource| tracing::trace!(target: "tunnel_test_coverage", %destination, %resource, "Packet for CIDR resource"),
|
||||
|rid| tracing::trace!(target: "tunnel_test_coverage", %destination, %rid, "Packet for CIDR resource"),
|
||||
);
|
||||
|
||||
maybe_dns_resource_id
|
||||
|
||||
@@ -283,7 +283,7 @@ impl StubResolver {
|
||||
self.get_or_assign_aaaa_records(domain.clone(), resource)
|
||||
}
|
||||
(RecordType::SRV | RecordType::TXT, Some(resource)) => {
|
||||
tracing::debug!(%qtype, resource = %resource.id, "Forwarding query for DNS resource to corresponding site");
|
||||
tracing::debug!(%qtype, rid = %resource.id, "Forwarding query for DNS resource to corresponding site");
|
||||
|
||||
return ResolveStrategy::RecurseSite(resource.id);
|
||||
}
|
||||
@@ -323,11 +323,11 @@ impl StubResolver {
|
||||
}
|
||||
}
|
||||
|
||||
pub fn is_subdomain(name: &dns_types::DomainName, resource: &str) -> bool {
|
||||
let pattern = match Pattern::new(resource) {
|
||||
pub fn is_subdomain(name: &dns_types::DomainName, pattern: &str) -> bool {
|
||||
let pattern = match Pattern::new(pattern) {
|
||||
Ok(p) => p,
|
||||
Err(e) => {
|
||||
tracing::warn!(%resource, "Unable to parse pattern: {}", err_with_src(&e));
|
||||
tracing::warn!(%pattern, "Unable to parse pattern: {}", err_with_src(&e));
|
||||
return false;
|
||||
}
|
||||
};
|
||||
|
||||
@@ -193,15 +193,15 @@ impl GatewayState {
|
||||
self.drain_node_events();
|
||||
}
|
||||
|
||||
#[tracing::instrument(level = "debug", skip_all, fields(%resource, %client))]
|
||||
pub fn remove_access(&mut self, client: &ClientId, resource: &ResourceId) {
|
||||
let Some(peer) = self.peers.get_mut(client) else {
|
||||
#[tracing::instrument(level = "debug", skip_all, fields(%rid, %cid))]
|
||||
pub fn remove_access(&mut self, cid: &ClientId, rid: &ResourceId) {
|
||||
let Some(peer) = self.peers.get_mut(cid) else {
|
||||
return;
|
||||
};
|
||||
|
||||
peer.remove_resource(resource);
|
||||
peer.remove_resource(rid);
|
||||
if peer.is_emptied() {
|
||||
self.peers.remove(client);
|
||||
self.peers.remove(cid);
|
||||
}
|
||||
|
||||
tracing::debug!("Access removed");
|
||||
@@ -230,11 +230,11 @@ impl GatewayState {
|
||||
})
|
||||
}
|
||||
|
||||
#[tracing::instrument(level = "debug", skip_all, fields(%client_id))]
|
||||
#[tracing::instrument(level = "debug", skip_all, fields(%cid))]
|
||||
#[expect(clippy::too_many_arguments)]
|
||||
pub fn authorize_flow(
|
||||
&mut self,
|
||||
client_id: ClientId,
|
||||
cid: ClientId,
|
||||
client_key: PublicKey,
|
||||
preshared_key: SecretKey,
|
||||
client_ice: IceCredentials,
|
||||
@@ -245,7 +245,7 @@ impl GatewayState {
|
||||
now: Instant,
|
||||
) -> Result<(), NoTurnServers> {
|
||||
self.node.upsert_connection(
|
||||
client_id,
|
||||
cid,
|
||||
client_key,
|
||||
Secret::new(preshared_key.expose_secret().0),
|
||||
Credentials {
|
||||
@@ -259,7 +259,7 @@ impl GatewayState {
|
||||
now,
|
||||
)?;
|
||||
|
||||
let result = self.allow_access(client_id, client_tun, expires_at, resource, None);
|
||||
let result = self.allow_access(cid, client_tun, expires_at, resource, None);
|
||||
debug_assert!(
|
||||
result.is_ok(),
|
||||
"`allow_access` should never fail without a `DnsResourceEntry`"
|
||||
@@ -495,7 +495,7 @@ fn handle_p2p_control_packet(
|
||||
};
|
||||
|
||||
if !peer.is_allowed(req.resource) {
|
||||
tracing::warn!(cid = %peer.id(), resource = %req.resource, "Received `AssignedIpsEvent` for resource that is not allowed");
|
||||
tracing::warn!(cid = %peer.id(), rid = %req.resource, "Received `AssignedIpsEvent` for resource that is not allowed");
|
||||
|
||||
let packet = dns_resource_nat::domain_status(
|
||||
req.resource,
|
||||
|
||||
@@ -209,7 +209,7 @@ impl ClientOnGateway {
|
||||
resource: crate::messages::gateway::ResourceDescription,
|
||||
expires_at: Option<DateTime<Utc>>,
|
||||
) {
|
||||
tracing::info!(client = %self.id, resource = %resource.id(), expires = ?expires_at.map(|e| e.to_rfc3339()), "Allowing access to resource");
|
||||
tracing::info!(cid = %self.id, rid = %resource.id(), expires = ?expires_at.map(|e| e.to_rfc3339()), "Allowing access to resource");
|
||||
|
||||
match self.resources.entry(resource.id()) {
|
||||
hash_map::Entry::Vacant(v) => {
|
||||
@@ -233,13 +233,9 @@ impl ClientOnGateway {
|
||||
self.recalculate_filters();
|
||||
}
|
||||
|
||||
pub(crate) fn update_resource_expiry(
|
||||
&mut self,
|
||||
resource: ResourceId,
|
||||
new_expiry: DateTime<Utc>,
|
||||
) {
|
||||
let Some(resource) = self.resources.get_mut(&resource) else {
|
||||
tracing::debug!(%resource, "Unknown resource");
|
||||
pub(crate) fn update_resource_expiry(&mut self, rid: ResourceId, new_expiry: DateTime<Utc>) {
|
||||
let Some(resource) = self.resources.get_mut(&rid) else {
|
||||
tracing::debug!(%rid, "Unknown resource");
|
||||
|
||||
return;
|
||||
};
|
||||
@@ -256,11 +252,11 @@ impl ClientOnGateway {
|
||||
}
|
||||
|
||||
pub(crate) fn retain_authorizations(&mut self, authorization: BTreeSet<ResourceId>) {
|
||||
for (resource, _) in self
|
||||
for (rid, _) in self
|
||||
.resources
|
||||
.extract_if(|resource, _| !authorization.contains(resource))
|
||||
{
|
||||
tracing::info!(%resource, "Revoking resource authorization");
|
||||
tracing::info!(%rid, "Revoking resource authorization");
|
||||
}
|
||||
|
||||
self.recalculate_filters();
|
||||
|
||||
@@ -525,12 +525,12 @@ impl RefClient {
|
||||
if let Some(overlapping_resource) = table.exact_match(resource.address)
|
||||
&& self.is_connected_to_internet_or_cidr(*overlapping_resource)
|
||||
{
|
||||
tracing::debug!(%overlapping_resource, resource = %resource.id, address = %resource.address, "Already connected to resource with this exact address, retaining existing route");
|
||||
tracing::debug!(%overlapping_resource, rid = %resource.id, address = %resource.address, "Already connected to resource with this exact address, retaining existing route");
|
||||
|
||||
continue;
|
||||
}
|
||||
|
||||
tracing::debug!(resource = %resource.id, address = %resource.address, "Adding CIDR route");
|
||||
tracing::debug!(rid = %resource.id, address = %resource.address, "Adding CIDR route");
|
||||
|
||||
table.insert(resource.address, resource.id);
|
||||
}
|
||||
@@ -750,16 +750,16 @@ impl RefClient {
|
||||
}
|
||||
}
|
||||
|
||||
fn set_resource_online(&mut self, resource: ResourceId) {
|
||||
let Some(site) = self.site_for_resource(resource) else {
|
||||
tracing::error!(%resource, "Unknown resource or multi-site resource");
|
||||
fn set_resource_online(&mut self, rid: ResourceId) {
|
||||
let Some(site) = self.site_for_resource(rid) else {
|
||||
tracing::error!(%rid, "Unknown resource or multi-site resource");
|
||||
return;
|
||||
};
|
||||
|
||||
let previous = self.site_status.insert(site.id, ResourceStatus::Online);
|
||||
|
||||
if previous.is_none_or(|s| s != ResourceStatus::Online) {
|
||||
tracing::debug!(%resource, "Resource is now online");
|
||||
tracing::debug!(%rid, "Resource is now online");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -767,17 +767,17 @@ impl RefClient {
|
||||
self.is_connected_to_cidr(resource) || self.is_connected_to_internet(resource)
|
||||
}
|
||||
|
||||
fn connect_to_internet_or_cidr_resource(&mut self, resource: ResourceId) {
|
||||
if self.internet_resource.is_some_and(|r| r == resource) {
|
||||
fn connect_to_internet_or_cidr_resource(&mut self, rid: ResourceId) {
|
||||
if self.internet_resource.is_some_and(|r| r == rid) {
|
||||
self.connected_internet_resource = true;
|
||||
return;
|
||||
}
|
||||
|
||||
if self.cidr_resources.iter().any(|(_, r)| *r == resource) {
|
||||
let is_new = self.connected_cidr_resources.insert(resource);
|
||||
if self.cidr_resources.iter().any(|(_, r)| *r == rid) {
|
||||
let is_new = self.connected_cidr_resources.insert(rid);
|
||||
|
||||
if is_new {
|
||||
tracing::debug!(%resource, "Now connected to CIDR resource");
|
||||
tracing::debug!(%rid, "Now connected to CIDR resource");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -423,17 +423,17 @@ impl Eventloop {
|
||||
},
|
||||
));
|
||||
for Authorization {
|
||||
client_id,
|
||||
resource_id,
|
||||
client_id: cid,
|
||||
resource_id: rid,
|
||||
expires_at,
|
||||
} in authorizations
|
||||
{
|
||||
if let Err(e) = self.tunnel.state_mut().update_access_authorization_expiry(
|
||||
client_id,
|
||||
resource_id,
|
||||
expires_at,
|
||||
) {
|
||||
tracing::debug!(%client_id, %resource_id, "Failed to update access authorization: {e:#}");
|
||||
if let Err(e) = self
|
||||
.tunnel
|
||||
.state_mut()
|
||||
.update_access_authorization_expiry(cid, rid, expires_at)
|
||||
{
|
||||
tracing::debug!(%cid, %rid, "Failed to update access authorization: {e:#}");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -474,19 +474,19 @@ impl Eventloop {
|
||||
msg:
|
||||
IngressMessages::AccessAuthorizationExpiryUpdated(
|
||||
AccessAuthorizationExpiryUpdated {
|
||||
client_id,
|
||||
resource_id,
|
||||
client_id: cid,
|
||||
resource_id: rid,
|
||||
expires_at,
|
||||
},
|
||||
),
|
||||
..
|
||||
} => {
|
||||
if let Err(e) = self.tunnel.state_mut().update_access_authorization_expiry(
|
||||
client_id,
|
||||
resource_id,
|
||||
expires_at,
|
||||
) {
|
||||
tracing::warn!(%client_id, %resource_id, "Failed to update expiry of access authorization: {e:#}")
|
||||
if let Err(e) = self
|
||||
.tunnel
|
||||
.state_mut()
|
||||
.update_access_authorization_expiry(cid, rid, expires_at)
|
||||
{
|
||||
tracing::warn!(%cid, %rid, "Failed to update expiry of access authorization: {e:#}")
|
||||
};
|
||||
}
|
||||
phoenix_channel::Event::ErrorResponse { topic, req_id, res } => {
|
||||
@@ -518,7 +518,7 @@ impl Eventloop {
|
||||
let addresses = match result {
|
||||
Ok(addresses) => addresses,
|
||||
Err(e) => {
|
||||
tracing::debug!(client = %req.client.id, reference = %req.reference, "DNS resolution failed as part of connection request: {e:#}");
|
||||
tracing::debug!(cid = %req.client.id, reference = %req.reference, "DNS resolution failed as part of connection request: {e:#}");
|
||||
|
||||
return; // Fail the connection so the client runs into a timeout.
|
||||
}
|
||||
@@ -558,10 +558,10 @@ impl Eventloop {
|
||||
.domain
|
||||
.map(|r| DnsResourceNatEntry::new(r, addresses)),
|
||||
) {
|
||||
let client = req.client.id;
|
||||
let cid = req.client.id;
|
||||
|
||||
self.tunnel.state_mut().cleanup_connection(&client);
|
||||
tracing::debug!(%client, "Connection request failed: {e:#}");
|
||||
self.tunnel.state_mut().cleanup_connection(&cid);
|
||||
tracing::debug!(%cid, "Connection request failed: {e:#}");
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -586,7 +586,7 @@ impl Eventloop {
|
||||
let addresses = match result {
|
||||
Ok(addresses) => addresses,
|
||||
Err(e) => {
|
||||
tracing::debug!(client = %req.client_id, reference = %req.reference, "DNS resolution failed as part of allow access request: {e:#}");
|
||||
tracing::debug!(cid = %req.client_id, reference = %req.reference, "DNS resolution failed as part of allow access request: {e:#}");
|
||||
|
||||
vec![]
|
||||
}
|
||||
@@ -602,7 +602,7 @@ impl Eventloop {
|
||||
req.resource,
|
||||
req.payload.map(|r| DnsResourceNatEntry::new(r, addresses)),
|
||||
) {
|
||||
tracing::warn!(client = %req.client_id, "Allow access request failed: {e:#}");
|
||||
tracing::warn!(cid = %req.client_id, "Allow access request failed: {e:#}");
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user