sendnrw/netbird
2
0
Fork 0
mirror of https://github.com/netbirdio/netbird.git synced 2026-10-22 21:39:08 +02:00
Code Issues Packages Projects Releases Wiki Activity

[client] Keep the resolv.conf watch armed while repairing DNS in file mode (#8207)

Browse Source 
* [client] Split resolv.conf parsing from file reading

Pure refactor, no behavior change. parseResolvConf parses content that
was already read, so a caller can compare and parse the same snapshot
of the file instead of reading it twice.

* [client] Return the written content from the resolv.conf update function

Pure refactor, no behavior change. The repair watcher needs to know what
NetBird wrote so it can tell its own write apart from a third party's.
Callers ignore the content for now.

* [client] Keep the resolv.conf watch armed while repairing

In file mode the repair watcher removed its inotify watch, rewrote
resolv.conf and re-added the watch, without looking at the file again.
A second third-party write landing in that window raised no event, so
the file stayed on the upstream resolver until something else touched
it. systemd-resolved with the stub listener off, the Amazon Linux 2023
default, writes the file twice in a row on restart and does not rewrite
it on an unchanged DHCP renewal, so the host stayed off NetBird DNS
while the client reported connected.

The watch now stays armed during the repair. NetBird's own write is
recognized by comparing the file with the content it wrote, which also
covers a write whose search list was truncated to the resolv.conf
limits and would otherwise fail the parameter check forever. The
watcher checks the file once when it is armed, since a change made
before that produced no event.

* [client] Consume inotify errors in the resolv.conf watcher

fsnotify blocks its reader until an error is received. The watcher
never read the Errors channel, so a single error, such as an event
queue overflow, stopped all further events and silently disabled the
repair. The watcher now consumes errors and checks the file, since an
overflow means events were dropped.

* [client] Parse resolv.conf one line at a time

Pure refactor, no behavior change. The per-line handling moves into
parseResolvConfLine, which keeps parseResolvConf under the cognitive
complexity limit.

* [client] Drop the comment restating parseResolvConfLine
This commit is contained in:
Riccardo Manfrin
2026-10-20 13:35:15 +02:00
committed by GitHub
parent 1f70970792
commit 7420c97930
4 changed files with 297 additions and 99 deletions
Download Patch File Download Diff File Expand all files Collapse all files
client/internal/dns/file_parser_unix.go
+64 -50
View File
@@ -35,11 +35,7 @@ func parseBackupResolvConf() (*resolvConf, error) {
}
func parseResolvConfFile(resolvConfFile string) (*resolvConf, error) {
rconf := &resolvConf{
searchDomains: make([]string, 0),
nameServers: make([]netip.Addr, 0),
others: make([]string, 0),
}
rconf := newResolvConf()
file, err := os.Open(resolvConfFile)
if err != nil {
@@ -56,56 +52,74 @@ func parseResolvConfFile(resolvConfFile string) (*resolvConf, error) {
return rconf, fmt.Errorf("failed to read %s file: %w", resolvConfFile, err)
}
return parseResolvConf(cur)
}
// parseResolvConf parses resolv.conf content. The returned config is never nil, so callers can
// fall back to its empty lists on error.
func parseResolvConf(cur []byte) (*resolvConf, error) {
rconf := newResolvConf()
if len(cur) == 0 {
return rconf, fmt.Errorf("file is empty")
}
for _, line := range strings.Split(string(cur), "\n") {
line = strings.TrimSpace(line)
if strings.HasPrefix(line, "#") {
continue
}
if strings.HasPrefix(line, "domain") {
continue
}
if strings.HasPrefix(line, "options") && strings.Contains(line, "rotate") {
line = strings.ReplaceAll(line, "rotate", "")
splitLines := strings.Fields(line)
if len(splitLines) == 1 {
continue
}
line = strings.Join(splitLines, " ")
}
if strings.HasPrefix(line, "search") {
splitLines := strings.Fields(line)
if len(splitLines) < 2 {
continue
}
rconf.searchDomains = splitLines[1:]
continue
}
if strings.HasPrefix(line, "nameserver") {
splitLines := strings.Fields(line)
if len(splitLines) != 2 {
continue
}
if addr, err := netip.ParseAddr(splitLines[1]); err == nil {
rconf.nameServers = append(rconf.nameServers, addr.Unmap())
} else {
log.Warnf("invalid nameserver address in resolv.conf: %s, skipping", splitLines[1])
}
continue
}
if line != "" {
rconf.others = append(rconf.others, line)
}
parseResolvConfLine(rconf, strings.TrimSpace(line))
}
return rconf, nil
}
func parseResolvConfLine(rconf *resolvConf, line string) {
if strings.HasPrefix(line, "#") {
return
}
if strings.HasPrefix(line, "domain") {
return
}
if strings.HasPrefix(line, "options") && strings.Contains(line, "rotate") {
line = strings.ReplaceAll(line, "rotate", "")
splitLines := strings.Fields(line)
if len(splitLines) == 1 {
return
}
line = strings.Join(splitLines, " ")
}
if strings.HasPrefix(line, "search") {
splitLines := strings.Fields(line)
if len(splitLines) < 2 {
return
}
rconf.searchDomains = splitLines[1:]
return
}
if strings.HasPrefix(line, "nameserver") {
splitLines := strings.Fields(line)
if len(splitLines) != 2 {
return
}
if addr, err := netip.ParseAddr(splitLines[1]); err == nil {
rconf.nameServers = append(rconf.nameServers, addr.Unmap())
} else {
log.Warnf("invalid nameserver address in resolv.conf: %s, skipping", splitLines[1])
}
return
}
if line != "" {
rconf.others = append(rconf.others, line)
}
}
func newResolvConf() *resolvConf {
return &resolvConf{
searchDomains: make([]string, 0),
nameServers: make([]netip.Addr, 0),
others: make([]string, 0),
}
}
client/internal/dns/file_repair_unix.go
+69 -38
View File
@@ -3,7 +3,9 @@
package dns
import (
"bytes"
"net/netip"
"os"
"path"
"path/filepath"
"sync"
@@ -23,7 +25,8 @@ var (
}
)
type repairConfFn func([]string, netip.Addr, *resolvConf, *statemanager.Manager) error
// repairConfFn writes the NetBird-managed resolv.conf and returns the exact content it wrote.
type repairConfFn func([]string, netip.Addr, *resolvConf, *statemanager.Manager) ([]byte, error)
type repair struct {
operationFile string
@@ -43,7 +46,9 @@ func newRepair(operationFile string, updateFn repairConfFn) *repair {
}
}
func (f *repair) watchFileChanges(nbSearchDomains []string, nbNameserverIP netip.Addr, stateManager *statemanager.Manager) {
// watchFileChanges keeps resolv.conf pointing at NetBird. written is the content NetBird last
// wrote to the file, used to recognize the echo of its own writes.
func (f *repair) watchFileChanges(nbSearchDomains []string, nbNameserverIP netip.Addr, written []byte, stateManager *statemanager.Manager) {
if f.inotify != nil {
return
}
@@ -54,54 +59,80 @@ func (f *repair) watchFileChanges(nbSearchDomains []string, nbNameserverIP netip
log.Errorf("failed to start inotify watcher for resolv.conf: %s", err)
return
}
if err := inotify.Add(f.watchDir); err != nil {
log.Errorf("failed to add inotify watch for resolv.conf: %s", err)
if err := inotify.Close(); err != nil {
log.Debugf("failed to close resolv.conf inotify: %v", err)
}
return
}
f.inotify = inotify
f.inotifyWg.Add(1)
go func() {
defer f.inotifyWg.Done()
for event := range f.inotify.Events {
if !f.isEventRelevant(event) {
continue
}
log.Tracef("%s changed, check if it is broken", f.operationFile)
rConf, err := parseResolvConfFile(f.operationFile)
if err != nil {
log.Warnf("failed to parse resolv conf: %s", err)
continue
}
log.Debugf("check resolv.conf parameters: %s", rConf)
if !isNbParamsMissing(nbSearchDomains, nbNameserverIP, rConf) {
log.Tracef("resolv.conf still correct, skip the update")
continue
}
log.Info("broken params in resolv.conf, repairing it...")
err = f.inotify.Remove(f.watchDir)
if err != nil {
log.Errorf("failed to rm inotify watch for resolv.conf: %s", err)
}
err = f.updateFn(nbSearchDomains, nbNameserverIP, rConf, stateManager)
if err != nil {
log.Errorf("failed to repair resolv.conf: %v", err)
}
err = f.inotify.Add(f.watchDir)
if err != nil {
log.Errorf("failed to re-add inotify watch for resolv.conf: %s", err)
return
// A change that landed before the watch was armed produced no event.
lastWritten := f.checkAndRepair(nbSearchDomains, nbNameserverIP, written, stateManager)
for {
select {
case event, ok := <-inotify.Events:
if !ok {
return
}
if !f.isEventRelevant(event) {
continue
}
case err, ok := <-inotify.Errors:
if !ok {
return
}
// fsnotify stops delivering events until an error is received, and an
// error such as a queue overflow means events were dropped.
log.Warnf("resolv.conf inotify error, checking the file: %v", err)
}
lastWritten = f.checkAndRepair(nbSearchDomains, nbNameserverIP, lastWritten, stateManager)
}
}()
}
err = f.inotify.Add(f.watchDir)
// checkAndRepair rewrites resolv.conf when NetBird's parameters are missing from it and returns
// the content NetBird last wrote. The watch stays armed while repairing, so a third-party write
// racing the repair still raises an event; the echo of NetBird's own write is skipped by content.
func (f *repair) checkAndRepair(nbSearchDomains []string, nbNameserverIP netip.Addr, lastWritten []byte, stateManager *statemanager.Manager) []byte {
content, err := os.ReadFile(f.operationFile)
if err != nil {
log.Errorf("failed to add inotify watch for resolv.conf: %s", err)
return
log.Warnf("failed to read resolv.conf: %s", err)
return lastWritten
}
if bytes.Equal(content, lastWritten) {
log.Tracef("%s still holds the NetBird managed content, skip the check", f.operationFile)
return lastWritten
}
log.Tracef("%s changed, check if it is broken", f.operationFile)
rConf, err := parseResolvConf(content)
if err != nil {
log.Warnf("failed to parse resolv conf: %s", err)
return lastWritten
}
log.Debugf("check resolv.conf parameters: %s", rConf)
if !isNbParamsMissing(nbSearchDomains, nbNameserverIP, rConf) {
log.Tracef("resolv.conf still correct, skip the update")
return lastWritten
}
log.Info("broken params in resolv.conf, repairing it...")
written, err := f.updateFn(nbSearchDomains, nbNameserverIP, rConf, stateManager)
if err != nil {
log.Errorf("failed to repair resolv.conf: %v", err)
return lastWritten
}
return written
}
func (f *repair) stopWatchFileChanges() {
client/internal/dns/file_repair_unix_test.go
+159 -6
View File
@@ -4,12 +4,19 @@ package dns
import (
"context"
"fmt"
"net/netip"
"os"
"path/filepath"
"strings"
"sync/atomic"
"testing"
"time"
"github.com/fsnotify/fsnotify"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/netbirdio/netbird/client/internal/statemanager"
"github.com/netbirdio/netbird/util"
)
@@ -106,14 +113,14 @@ nameserver 8.8.8.8`,
var changed bool
ctx, cancel := context.WithTimeout(context.Background(), time.Second)
updateFn := func([]string, netip.Addr, *resolvConf, *statemanager.Manager) error {
updateFn := func([]string, netip.Addr, *resolvConf, *statemanager.Manager) ([]byte, error) {
changed = true
cancel()
return nil
return nil, nil
}
r := newRepair(operationFile, updateFn)
r.watchFileChanges([]string{"netbird.cloud"}, netip.MustParseAddr("10.0.0.1"), nil)
r.watchFileChanges([]string{"netbird.cloud"}, netip.MustParseAddr("10.0.0.1"), nil, nil)
err = os.WriteFile(operationFile, []byte(tt.touchedConfContent), 0755)
if err != nil {
@@ -153,14 +160,14 @@ searchdomain netbird.cloud something`
var changed bool
ctx, cancel := context.WithTimeout(context.Background(), time.Second)
updateFn := func([]string, netip.Addr, *resolvConf, *statemanager.Manager) error {
updateFn := func([]string, netip.Addr, *resolvConf, *statemanager.Manager) ([]byte, error) {
changed = true
cancel()
return nil
return nil, nil
}
r := newRepair(tmpLink, updateFn)
r.watchFileChanges([]string{"netbird.cloud"}, netip.MustParseAddr("10.0.0.1"), nil)
r.watchFileChanges([]string{"netbird.cloud"}, netip.MustParseAddr("10.0.0.1"), nil, nil)
err = os.WriteFile(tmpLink, []byte(modifyContent), 0755)
if err != nil {
@@ -175,3 +182,149 @@ searchdomain netbird.cloud something`
t.Errorf("unexpected result: want: %v, got: %v", true, false)
}
}
// systemd-resolved can rewrite resolv.conf twice in a row, e.g. on restart. When the second write
// lands right after NetBird's repair, the watcher must still see it and repair again; otherwise the
// host silently stays on the upstream resolver until the next unrelated change of the file.
func Test_repairCatchesWriteRacingRepair(t *testing.T) {
good := "nameserver 10.0.0.1\nsearch netbird.cloud\n"
upstream := "nameserver 8.8.8.8\nsearch .\n"
operationFile := filepath.Join(t.TempDir(), "resolv.conf")
require.NoError(t, os.WriteFile(operationFile, []byte(good), 0644))
var calls atomic.Int32
updateFn := func([]string, netip.Addr, *resolvConf, *statemanager.Manager) ([]byte, error) {
n := calls.Add(1)
if err := os.WriteFile(operationFile, []byte(good), 0644); err != nil {
return nil, err
}
if n == 1 {
// the third party's second write lands right after the repair
if err := writeFileAtomic(operationFile, upstream); err != nil {
return nil, err
}
}
return []byte(good), nil
}
r := newRepair(operationFile, updateFn)
r.watchFileChanges([]string{"netbird.cloud"}, netip.MustParseAddr("10.0.0.1"), []byte(good), nil)
t.Cleanup(r.stopWatchFileChanges)
require.NoError(t, writeFileAtomic(operationFile, upstream))
assert.Eventually(t, func() bool {
content, err := os.ReadFile(operationFile)
return err == nil && string(content) == good
}, 2*time.Second, 10*time.Millisecond, "resolv.conf should be repaired after the racing write")
assert.GreaterOrEqual(t, calls.Load(), int32(2), "the racing write should trigger a second repair")
}
// A change made between NetBird's write and the arming of the watch produces no event, so the
// watcher must check the file as soon as it starts.
func Test_repairChecksFileWhenArmed(t *testing.T) {
good := "nameserver 10.0.0.1\nsearch netbird.cloud\n"
operationFile := filepath.Join(t.TempDir(), "resolv.conf")
require.NoError(t, os.WriteFile(operationFile, []byte("nameserver 8.8.8.8\n"), 0644))
repaired := make(chan struct{}, 1)
updateFn := func([]string, netip.Addr, *resolvConf, *statemanager.Manager) ([]byte, error) {
select {
case repaired <- struct{}{}:
default:
}
return []byte(good), os.WriteFile(operationFile, []byte(good), 0644)
}
r := newRepair(operationFile, updateFn)
r.watchFileChanges([]string{"netbird.cloud"}, netip.MustParseAddr("10.0.0.1"), []byte(good), nil)
t.Cleanup(r.stopWatchFileChanges)
select {
case <-repaired:
case <-time.After(2 * time.Second):
t.Fatal("watcher should repair a file changed before it was armed")
}
}
// When NetBird has more search domains than resolv.conf can hold, its own write lacks some of them
// and fails the parameter check. The watcher must recognize that write as its own and not repair
// it again in a loop.
func Test_repairNoLoopOnTruncatedSearchDomains(t *testing.T) {
nbDomains := make([]string, 0, fileMaxNumberOfSearchDomains+2)
for i := range fileMaxNumberOfSearchDomains + 2 {
nbDomains = append(nbDomains, fmt.Sprintf("d%d.netbird.cloud", i))
}
truncated := fmt.Sprintf("nameserver 10.0.0.1\nsearch %s\n", strings.Join(nbDomains[:fileMaxNumberOfSearchDomains], " "))
operationFile := filepath.Join(t.TempDir(), "resolv.conf")
require.NoError(t, os.WriteFile(operationFile, []byte(truncated), 0644))
var calls atomic.Int32
updateFn := func([]string, netip.Addr, *resolvConf, *statemanager.Manager) ([]byte, error) {
calls.Add(1)
return []byte(truncated), os.WriteFile(operationFile, []byte(truncated), 0644)
}
r := newRepair(operationFile, updateFn)
r.watchFileChanges(nbDomains, netip.MustParseAddr("10.0.0.1"), []byte(truncated), nil)
t.Cleanup(r.stopWatchFileChanges)
require.NoError(t, writeFileAtomic(operationFile, "nameserver 8.8.8.8\n"))
require.Eventually(t, func() bool { return calls.Load() >= 1 }, 2*time.Second, 10*time.Millisecond,
"the third-party write should be repaired")
time.Sleep(500 * time.Millisecond)
assert.Equal(t, int32(1), calls.Load(), "NetBird's own truncated write should not trigger another repair")
}
// fsnotify blocks its reader until an error is received, and an error such as a queue overflow
// means events were dropped. The watcher must consume errors and check the file, so it keeps working
// and catches a change whose event was lost.
func Test_repairChecksFileOnWatcherError(t *testing.T) {
good := "nameserver 10.0.0.1\nsearch netbird.cloud\n"
operationFile := filepath.Join(t.TempDir(), "resolv.conf")
require.NoError(t, os.WriteFile(operationFile, []byte(good), 0644))
// A hard link in another directory changes the file without an event in the watched one,
// standing in for an event the kernel dropped.
alias := filepath.Join(t.TempDir(), "alias")
require.NoError(t, os.Link(operationFile, alias))
repaired := make(chan struct{}, 1)
updateFn := func([]string, netip.Addr, *resolvConf, *statemanager.Manager) ([]byte, error) {
select {
case repaired <- struct{}{}:
default:
}
return []byte(good), os.WriteFile(operationFile, []byte(good), 0644)
}
r := newRepair(operationFile, updateFn)
r.watchFileChanges([]string{"netbird.cloud"}, netip.MustParseAddr("10.0.0.1"), []byte(good), nil)
t.Cleanup(r.stopWatchFileChanges)
require.NoError(t, os.WriteFile(alias, []byte("nameserver 8.8.8.8\n"), 0644))
select {
case r.inotify.Errors <- fsnotify.ErrEventOverflow:
case <-time.After(2 * time.Second):
t.Fatal("watcher should consume inotify errors")
}
select {
case <-repaired:
case <-time.After(2 * time.Second):
t.Fatal("watcher should check the file after an inotify error")
}
}
// writeFileAtomic replaces the file the way systemd-resolved does: write a temporary file in the same
// directory, then rename it over the target.
func writeFileAtomic(target, content string) error {
tmp := target + ".tmp"
if err := os.WriteFile(tmp, []byte(content), 0644); err != nil {
return err
}
return os.Rename(tmp, target)
}
client/internal/dns/file_unix.go
+5 -5
View File
@@ -61,11 +61,11 @@ func (f *fileConfigurator) applyDNSConfig(config HostDNSConfig, stateManager *st
f.repair.stopWatchFileChanges()
err = f.updateConfig(nbSearchDomains, f.nbNameserverIP, resolvConf, stateManager)
written, err := f.updateConfig(nbSearchDomains, f.nbNameserverIP, resolvConf, stateManager)
if err != nil {
return err
}
f.repair.watchFileChanges(nbSearchDomains, f.nbNameserverIP, stateManager)
f.repair.watchFileChanges(nbSearchDomains, f.nbNameserverIP, written, stateManager)
return nil
}
@@ -74,7 +74,7 @@ func (f *fileConfigurator) getOriginalNameservers() []netip.Addr {
return f.originalNameservers
}
func (f *fileConfigurator) updateConfig(nbSearchDomains []string, nbNameserverIP netip.Addr, cfg *resolvConf, stateManager *statemanager.Manager) error {
func (f *fileConfigurator) updateConfig(nbSearchDomains []string, nbNameserverIP netip.Addr, cfg *resolvConf, stateManager *statemanager.Manager) ([]byte, error) {
searchDomainList := mergeSearchDomains(nbSearchDomains, cfg.searchDomains)
buf := prepareResolvConfContent(
@@ -90,7 +90,7 @@ func (f *fileConfigurator) updateConfig(nbSearchDomains []string, nbNameserverIP
if restoreErr != nil {
log.Errorf("attempt to restore default file failed with error: %s", err)
}
return fmt.Errorf("creating resolver file %s. Error: %w", defaultResolvConfPath, err)
return nil, fmt.Errorf("creating resolver file %s. Error: %w", defaultResolvConfPath, err)
}
log.Infof("created a NetBird managed %s file with the DNS settings. Added %d search domains. Search list: %s", defaultResolvConfPath, len(searchDomainList), searchDomainList)
@@ -100,7 +100,7 @@ func (f *fileConfigurator) updateConfig(nbSearchDomains []string, nbNameserverIP
log.Errorf("failed to create unclean shutdown resolv.conf backup: %s", err)
}
return nil
return buf.Bytes(), nil
}
func (f *fileConfigurator) restoreHostDNS() error {