Skip to content

Conversation

@hakansa
Copy link
Member

@hakansa hakansa commented Oct 2, 2025

Describe your changes

[client] Implement DNS query caching in DNSForwarder

Issue ticket number and link

Stack

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)

By submitting this pull request, you confirm that you have read and agree to the terms of the Contributor License Agreement.

Documentation

Select exactly one:

  • I added/updated documentation for this change
  • Documentation is not needed for this change (explain why)
    .

Docs PR URL (required if "docs added" is checked)

Paste the PR link from https://github.com/netbirdio/docs here:

https://github.com/netbirdio/docs/pull/__

@Copilot Copilot AI review requested due to automatic review settings October 2, 2025 12:01
Copy link
Contributor

Copilot AI left a comment

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 implements DNS query caching in the DNSForwarder to improve performance and provide fallback responses when upstream DNS servers fail. The cache stores successful DNS query results and serves them when upstream resolution fails.

  • Added a new cache component to store DNS query results by domain and query type
  • Integrated cache storage after successful DNS resolutions
  • Enhanced error handling to serve cached responses as fallback when upstream DNS fails

Reviewed Changes

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

File Description
client/internal/dnsfwd/cache.go New cache implementation with thread-safe operations for storing and retrieving DNS records
client/internal/dnsfwd/forwarder.go Integrated cache into DNSForwarder and enhanced error handling to use cached responses as fallback

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@hakansa hakansa marked this pull request as ready for review October 2, 2025 12:28
Copy link
Collaborator

@lixmal lixmal left a comment

Choose a reason for hiding this comment

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

  • Currently the cache is unbounded and there is no way to manually clear it, can we do something about it?
  • NXDOMAIN is not cached, but it should be
  • UpdateDomains might remove some domains in which case we should remove those from the cache as well

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 8, 2025

@hakansa
Copy link
Member Author

hakansa commented Oct 8, 2025

@lixmal added cache for NXDOMAIN, also added a method to remove stale cache entries on UpdateDomains.

Regarding to the clearing cache, maybe we can expire not used cache items after some time but i think it can be implemented later /cc @mlsmaycon

@mlsmaycon mlsmaycon merged commit 7683328 into main Oct 8, 2025
36 checks passed
@mlsmaycon mlsmaycon deleted the feat/dns-fwd-cache branch October 8, 2025 14:54
@solariz
Copy link

solariz commented Oct 9, 2025

Thanks for implementing that, can help quite a lot.
Not a go coder but if I may ask some things I did not figured out looking at the code:

  • is it only cached for the time netbird is running?
  • On long running instances records could potentially get quite big
  • so for later improvements cache should respect ttl
  • for now it is only implemented as fallback if the initial lookup failed, would be great to have it in future -maybe first optional- to answer from cache first if the record is fresh. But for sure this needs a garbage collection and reliant ttl cleanup

Again thanks for implementing this fallback.

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.

4 participants