Skip to content

fix(gateway): evaluate fastest nameserver every 60s#9060

Merged
thomaseizinger merged 4 commits into
mainfrom
fix/re-evaluate-nameserver
May 9, 2025
Merged

fix(gateway): evaluate fastest nameserver every 60s#9060
thomaseizinger merged 4 commits into
mainfrom
fix/re-evaluate-nameserver

Conversation

@thomaseizinger

Copy link
Copy Markdown
Member

Currently, the Gateway reads all nameservers from /etc/resolv.conf on startup and evaluates the fastest one to use for SRV and TXT DNS queries that are forwarded by the Client. If the machine just booted and we do not have Internet connectivity just yet, this fails which leaves the Gateway in state where it cannot fulfill those queries.

In order to ensure we always use the fastest one and to self-heal from such situations, we add a 60s timer that refreshes this state. Currently, this will not re-read the nameservers from /etc/resolv.conf but still use the same IPs read on startup.

@thomaseizinger thomaseizinger requested review from Copilot and jamilbk May 9, 2025 01:49
@vercel

vercel Bot commented May 9, 2025

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
firezone ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 9, 2025 2:17am

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a periodic re-evaluation mechanism for the fastest nameserver, ensuring that the Gateway remains functional even if the initial nameserver evaluation fails after a boot without connectivity.

  • The evaluation function in nameserver_set now returns early if there are no nameservers and logs the evaluation process.
  • A timer is added in the I/O module to trigger a re-evaluation every 60 seconds.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
rust/connlib/tunnel/src/io/nameserver_set.rs Added early-return check on empty nameserver set and logging for evaluation; note the removal of a clear() call.
rust/connlib/tunnel/src/io.rs Introduced a re-evaluation timer and corresponding constant, and adjusted the nameserver initialization.
Comments suppressed due to low confidence (1)

rust/connlib/tunnel/src/io.rs:50

  • [nitpick] Consider renaming 'reval_nameserver_interval' to 'reeval_nameserver_interval' to maintain consistency with the constant RE_EVALUATE_NAMESERVER_INTERVAL.
reval_nameserver_interval: tokio::time::Interval,

@@ -51,7 +51,12 @@ impl NameserverSet {
}

pub fn evaluate(&mut self) {

Copilot AI May 9, 2025

Copy link

Choose a reason for hiding this comment

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

Consider whether clearing self.nameserver_by_rtt is still needed inside evaluate() to avoid retaining outdated evaluations across invocations.

Suggested change
pub fn evaluate(&mut self) {
pub fn evaluate(&mut self) {
self.nameserver_by_rtt.clear();

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I removed the clearing in order to avoid a service disruption while we are re-evaluating the nameservers.

@thomaseizinger

Copy link
Copy Markdown
Member Author

Currently, this will not re-read the nameservers from /etc/resolv.conf but still use the same IPs read on startup.

@jamilbk If we want to do this too, we can do so in a separate change. It is a bit more involved to do that so I've deferred it for now to prioritise this fix.

@jamilbk

jamilbk commented May 9, 2025

Copy link
Copy Markdown
Member

Currently, this will not re-read the nameservers from /etc/resolv.conf but still use the same IPs read on startup.

@jamilbk If we want to do this too, we can do so in a separate change. It is a bit more involved to do that so I've deferred it for now to prioritise this fix.

Yeah that's fine / sounds good. Given our luck we should probably get an issue open for it if it's not something on the immediate radar. Someone somewhere will hit this and it will take us + them a few hours / days to debug.

Comment on lines +31 to +32
Fixes an issue where service discovery for DNS resources would fail
in case the Gateway's started up with no network connectivity.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
Fixes an issue where service discovery for DNS resources would fail
in case the Gateway's started up with no network connectivity.
Fixes an issue where service discovery for DNS resources would fail
if the Gateway started up with no network connectivity.

@thomaseizinger thomaseizinger added this pull request to the merge queue May 9, 2025
Merged via the queue into main with commit ac339ff May 9, 2025
@thomaseizinger thomaseizinger deleted the fix/re-evaluate-nameserver branch May 9, 2025 03:52
github-merge-queue Bot pushed a commit that referenced this pull request May 9, 2025
When we implemented #8350, we chose an error handling strategy that
would shutdown the Gateway in case we didn't have a nameserver selected
for handling those SRV and TXT queries. At the time, this was deemed to
be sufficiently rare to be an adequate strategy. We have since learned
that this can indeed happen when the Gateway starts without network
connectivity which is quite common when using tools such as terraform to
provision infrastructure.

In #9060, we fix this by re-evaluating the fastest nameserver on a
timer. This however doesn't change the error handling strategy when we
don't have a working nameserver at all. It is practically impossible to
have a working Gateway yet us being unable to select a nameserver. We
read them from `/etc/resolv.conf` which is what `libc` uses to also
resolve the domain we connect to for the WebSocket. A working WebSocket
connection is required for us to establish connections to Clients, which
in turn is a precursor to us receiving DNS queries from a Client.

It causes unnecessary complexity to have a code path that can
potentially terminate the Gateway, yet is practically unreachable. To
fix this situation, we remove this code path and instead reply with a
DNS SERVFAIL error.

---------

Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants