Skip to content

[client] Always log dns forwader responses#5262

Merged
lixmal merged 1 commit intomainfrom
improve-dns-forwarder-logs
Feb 5, 2026
Merged

[client] Always log dns forwader responses#5262
lixmal merged 1 commit intomainfrom
improve-dns-forwarder-logs

Conversation

@lixmal
Copy link
Copy Markdown
Collaborator

@lixmal lixmal commented Feb 5, 2026

Describe your changes

The forwarder was missing logging for cases like REFUSED when a domain was queried that was not (yet) configured.

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/__

Summary by CodeRabbit

  • Improvements
    • Enhanced DNS response handling with improved timing accuracy and consistency.
    • Optimized UDP response processing for better reliability.
    • Improved error handling and logging during DNS forwarding operations.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Feb 5, 2026

📝 Walkthrough

Walkthrough

The pull request refactors DNS forwarder's response handling by propagating timing information through the query handler chain, centralizing response writing via a new helper function, and introducing UDP-specific truncation logic through a wrapper. Method signatures are updated to pass timing context throughout.

Changes

Cohort / File(s) Summary
Core DNS Forwarding Logic
client/internal/dnsfwd/forwarder.go
Propagated startTime parameter into handleDNSQuery and handleDNSError for time-aware logging. Centralized DNS response writing through new writeResponse helper, eliminating inline response writes. Introduced udpResponseWriter wrapper for UDP-specific EDNS0 size truncation. Unified domain variable naming to lowercased qname. Adjusted call flow between UDP/TCP handlers to funnel through updated handlers with timing context.
Test Updates
client/internal/dnsfwd/forwarder_test.go
Updated test calls to handleDNSQuery to include time.Time parameter. Changed response retrieval from method return values to GetLastResponse on writer object. Adjusted assertions to work with new response retrieval pattern and nil/non-nil expectations.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • pappz

Poem

🐰 Hops of timing, writes consolidated clear,
Response helpers funnel far and near,
UDP truncation wraps with care,
DNS flows with freshened flair!

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: improving DNS forwarder logging to always log responses, which aligns with the raw summary's description of propagating startTime for time-aware logging and centralizing response writing.
Description check ✅ Passed The PR description covers the key section (Describe your changes) explaining the bug fix intent, marks it as a bug fix, and confirms documentation is not needed. However, the Issue ticket number section is empty and no explanation is provided for why documentation isn't needed.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch improve-dns-forwarder-logs

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Feb 5, 2026

Quality Gate Passed Quality Gate passed

Issues
0 New issues
1 Accepted issue

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@lixmal lixmal merged commit 1b96648 into main Feb 5, 2026
39 checks passed
@lixmal lixmal deleted the improve-dns-forwarder-logs branch February 5, 2026 13:34
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.

2 participants