From 2a546aa609d1cd8ebd979fc54a0a556ed34f1633 Mon Sep 17 00:00:00 2001 From: Eugene Burkov Date: Tue, 13 Feb 2024 13:19:22 +0300 Subject: [PATCH] Pull request 2149: 6711 watch hosts Updates #6711. Squashed commit of the following: commit 3ddfe809f76325c2d4cda0715a7bcc15e76a2388 Merge: 185957cd0 d338451fa Author: Eugene Burkov Date: Tue Feb 13 13:01:30 2024 +0300 Merge branch 'master' into 6711-watch-hosts commit 185957cd01516e5955e388108615e6f131d6ad71 Author: Eugene Burkov Date: Fri Feb 9 18:11:41 2024 +0300 aghos: imp docs commit 3afbbcbb7ab6cc60c7c40ef8acd5b3ddf52cb3d1 Author: Eugene Burkov Date: Fri Feb 9 15:40:02 2024 +0300 all: upd golibs, imp fswatcher --- CHANGELOG.md | 5 ++ go.mod | 2 +- go.sum | 4 +- internal/aghnet/hostscontainer_test.go | 4 ++ internal/aghnet/net.go | 3 +- internal/aghos/fswatcher.go | 99 +++++++++++++++++--------- internal/aghos/os.go | 13 +--- internal/aghos/os_linux.go | 3 +- internal/aghos/os_unix.go | 5 -- internal/aghos/os_windows.go | 16 ----- internal/aghtest/interface.go | 18 +++-- internal/arpdb/arpdb.go | 3 +- internal/dnsforward/dnsforward_test.go | 1 + internal/dnsforward/http_test.go | 1 + internal/filtering/hosts_test.go | 1 + internal/filtering/rewrites.go | 3 +- internal/home/control.go | 6 +- internal/home/home.go | 2 +- 18 files changed, 102 insertions(+), 87 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f90726c..0fc8b003 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,12 +57,17 @@ NOTE: Add new changes BELOW THIS COMMENT. - Go 1.21 support. Future versions will require at least Go 1.22 to build. +### Fixed + +- Incorrect tracking of the system hosts file's changes ([#6711]). + ### Removed - Go 1.20 support, as it has reached end of life. [#5992]: https://github.com/AdguardTeam/AdGuardHome/issues/5992 [#6679]: https://github.com/AdguardTeam/AdGuardHome/issues/6679 +[#6711]: https://github.com/AdguardTeam/AdGuardHome/issues/6711 [go-toolchain]: https://go.dev/blog/toolchain diff --git a/go.mod b/go.mod index fe7f2374..ab79dd17 100644 --- a/go.mod +++ b/go.mod @@ -4,7 +4,7 @@ go 1.21.7 require ( github.com/AdguardTeam/dnsproxy v0.65.0 - github.com/AdguardTeam/golibs v0.20.0 + github.com/AdguardTeam/golibs v0.20.1 github.com/AdguardTeam/urlfilter v0.17.3 github.com/NYTimes/gziphandler v1.1.1 github.com/ameshkov/dnscrypt/v2 v2.2.7 diff --git a/go.sum b/go.sum index bffe716d..7493be2d 100644 --- a/go.sum +++ b/go.sum @@ -1,7 +1,7 @@ github.com/AdguardTeam/dnsproxy v0.65.0 h1:mqJjVSkqoqPwThY3tTvnLHQ/AYBYrfWmK2ER91fu4FE= github.com/AdguardTeam/dnsproxy v0.65.0/go.mod h1:AGYMLPk2zX+I3NIUYS12KUI296mkCyfoMF/luy2uqdk= -github.com/AdguardTeam/golibs v0.20.0 h1:A9FIdYq7wUKhFYy3z+YZ/Aw5oFUYgW+xgaVAJ0pnnPY= -github.com/AdguardTeam/golibs v0.20.0/go.mod h1:3WunclLLfrVAq7fYQRhd6f168FHOEMssnipVXCxDL/w= +github.com/AdguardTeam/golibs v0.20.1 h1:ol8qLjWGZhU9paMMwN+OLWVTUigGsXa29iVTyd62VKY= +github.com/AdguardTeam/golibs v0.20.1/go.mod h1:bgcMgRviCKyU6mkrX+RtT/OsKPFzyppelfRsksMG3KU= github.com/AdguardTeam/urlfilter v0.17.3 h1:fg/ObbnO0Cv6aw0tW6N/ETDMhhNvmcUUOZ7HlmKC3rw= github.com/AdguardTeam/urlfilter v0.17.3/go.mod h1:Jru7jFfeH2CoDf150uDs+rRYcZBzHHBz05r9REyDKyE= github.com/NYTimes/gziphandler v1.1.1 h1:ZUDjpQae29j0ryrS0u/B8HZfJBtBQHjqw2rQ2cqUQ3I= diff --git a/internal/aghnet/hostscontainer_test.go b/internal/aghnet/hostscontainer_test.go index 813b369d..9393155d 100644 --- a/internal/aghnet/hostscontainer_test.go +++ b/internal/aghnet/hostscontainer_test.go @@ -67,6 +67,7 @@ func TestNewHostsContainer(t *testing.T) { } hc, err := aghnet.NewHostsContainer(testFS, &aghtest.FSWatcher{ + OnStart: func() (_ error) { panic("not implemented") }, OnEvents: onEvents, OnAdd: onAdd, OnClose: func() (err error) { return nil }, @@ -93,6 +94,7 @@ func TestNewHostsContainer(t *testing.T) { t.Run("nil_fs", func(t *testing.T) { require.Panics(t, func() { _, _ = aghnet.NewHostsContainer(nil, &aghtest.FSWatcher{ + OnStart: func() (_ error) { panic("not implemented") }, // Those shouldn't panic. OnEvents: func() (e <-chan struct{}) { return nil }, OnAdd: func(name string) (err error) { return nil }, @@ -111,6 +113,7 @@ func TestNewHostsContainer(t *testing.T) { const errOnAdd errors.Error = "error" errWatcher := &aghtest.FSWatcher{ + OnStart: func() (_ error) { panic("not implemented") }, OnEvents: func() (e <-chan struct{}) { panic("not implemented") }, OnAdd: func(name string) (err error) { return errOnAdd }, OnClose: func() (err error) { return nil }, @@ -155,6 +158,7 @@ func TestHostsContainer_refresh(t *testing.T) { t.Cleanup(func() { close(eventsCh) }) w := &aghtest.FSWatcher{ + OnStart: func() (_ error) { panic("not implemented") }, OnEvents: func() (e <-chan event) { return eventsCh }, OnAdd: func(name string) (err error) { assert.Equal(t, "dir", name) diff --git a/internal/aghnet/net.go b/internal/aghnet/net.go index ac9fb7bd..71e81b02 100644 --- a/internal/aghnet/net.go +++ b/internal/aghnet/net.go @@ -17,6 +17,7 @@ import ( "github.com/AdguardTeam/dnsproxy/upstream" "github.com/AdguardTeam/golibs/errors" "github.com/AdguardTeam/golibs/log" + "github.com/AdguardTeam/golibs/osutil" ) // DialContextFunc is the semantic alias for dialing functions, such as @@ -32,7 +33,7 @@ var ( netInterfaceAddrs = net.InterfaceAddrs // rootDirFS is the filesystem pointing to the root directory. - rootDirFS = aghos.RootDirFS() + rootDirFS = osutil.RootDirFS() ) // ErrNoStaticIPInfo is returned by IfaceHasStaticIP when no information about diff --git a/internal/aghos/fswatcher.go b/internal/aghos/fswatcher.go index 1694242e..ff40ed64 100644 --- a/internal/aghos/fswatcher.go +++ b/internal/aghos/fswatcher.go @@ -8,6 +8,8 @@ import ( "github.com/AdguardTeam/golibs/errors" "github.com/AdguardTeam/golibs/log" + "github.com/AdguardTeam/golibs/osutil" + "github.com/AdguardTeam/golibs/stringutil" "github.com/fsnotify/fsnotify" ) @@ -18,31 +20,38 @@ type event = struct{} // FSWatcher tracks all the fyle system events and notifies about those. // // TODO(e.burkov, a.garipov): Move into another package like aghfs. +// +// TODO(e.burkov): Add tests. type FSWatcher interface { + // Start starts watching the added files. + Start() (err error) + + // Close stops watching the files and closes an update channel. io.Closer - // Events should return a read-only channel which notifies about events. + // Events returns the channel to notify about the file system events. Events() (e <-chan event) - // Add should check if the file named name is accessible and starts tracking - // it. + // Add starts tracking the file. It returns an error if the file can't be + // tracked. It must not be called after Start. Add(name string) (err error) } // osWatcher tracks the file system provided by the OS. type osWatcher struct { - // w is the actual notifier that is handled by osWatcher. - w *fsnotify.Watcher + // watcher is the actual notifier that is handled by osWatcher. + watcher *fsnotify.Watcher // events is the channel to notify. events chan event + + // files is the set of tracked files. + files *stringutil.Set } -const ( - // osWatcherPref is a prefix for logging and wrapping errors in osWathcer's - // methods. - osWatcherPref = "os watcher" -) +// osWatcherPref is a prefix for logging and wrapping errors in osWathcer's +// methods. +const osWatcherPref = "os watcher" // NewOSWritesWatcher creates FSWatcher that tracks the real file system of the // OS and notifies only about writing events. @@ -55,25 +64,27 @@ func NewOSWritesWatcher() (w FSWatcher, err error) { return nil, fmt.Errorf("creating watcher: %w", err) } - fsw := &osWatcher{ - w: watcher, - events: make(chan event, 1), - } - - go fsw.handleErrors() - go fsw.handleEvents() - - return fsw, nil + return &osWatcher{ + watcher: watcher, + events: make(chan event, 1), + files: stringutil.NewSet(), + }, nil } -// handleErrors handles accompanying errors. It is intended to be used as a -// goroutine. -func (w *osWatcher) handleErrors() { - defer log.OnPanic(fmt.Sprintf("%s: handling errors", osWatcherPref)) +// type check +var _ FSWatcher = (*osWatcher)(nil) - for err := range w.w.Errors { - log.Error("%s: %s", osWatcherPref, err) - } +// Start implements the FSWatcher interface for *osWatcher. +func (w *osWatcher) Start() (err error) { + go w.handleErrors() + go w.handleEvents() + + return nil +} + +// Close implements the FSWatcher interface for *osWatcher. +func (w *osWatcher) Close() (err error) { + return w.watcher.Close() } // Events implements the FSWatcher interface for *osWatcher. @@ -81,22 +92,30 @@ func (w *osWatcher) Events() (e <-chan event) { return w.events } -// Add implements the FSWatcher interface for *osWatcher. +// Add implements the [FSWatcher] interface for *osWatcher. // // TODO(e.burkov): Make it accept non-existing files to detect it's creating. func (w *osWatcher) Add(name string) (err error) { defer func() { err = errors.Annotate(err, "%s: %w", osWatcherPref) }() - if _, err = fs.Stat(RootDirFS(), name); err != nil { + fi, err := fs.Stat(osutil.RootDirFS(), name) + if err != nil { return fmt.Errorf("checking file %q: %w", name, err) } - return w.w.Add(filepath.Join("/", name)) -} + name = filepath.Join("/", name) + w.files.Add(name) -// Close implements the FSWatcher interface for *osWatcher. -func (w *osWatcher) Close() (err error) { - return w.w.Close() + // Watch the directory and filter the events by the file name, since the + // common recomendation to the fsnotify package is to watch the directory + // instead of the file itself. + // + // See https://pkg.go.dev/github.com/fsnotify/fsnotify@v1.7.0#readme-watching-a-file-doesn-t-work-well. + if !fi.IsDir() { + name = filepath.Dir(name) + } + + return w.watcher.Add(name) } // handleEvents notifies about the received file system's event if needed. It @@ -106,9 +125,9 @@ func (w *osWatcher) handleEvents() { defer close(w.events) - ch := w.w.Events + ch := w.watcher.Events for e := range ch { - if e.Op&fsnotify.Write == 0 { + if e.Op&fsnotify.Write == 0 || !w.files.Has(e.Name) { continue } @@ -131,3 +150,13 @@ func (w *osWatcher) handleEvents() { } } } + +// handleErrors handles accompanying errors. It used to be called in a separate +// goroutine. +func (w *osWatcher) handleErrors() { + defer log.OnPanic(fmt.Sprintf("%s: handling errors", osWatcherPref)) + + for err := range w.watcher.Errors { + log.Error("%s: %s", osWatcherPref, err) + } +} diff --git a/internal/aghos/os.go b/internal/aghos/os.go index e5f524f7..c357d11d 100644 --- a/internal/aghos/os.go +++ b/internal/aghos/os.go @@ -7,7 +7,6 @@ import ( "bufio" "fmt" "io" - "io/fs" "os" "os/exec" "path" @@ -18,7 +17,6 @@ import ( "github.com/AdguardTeam/golibs/errors" "github.com/AdguardTeam/golibs/log" - "github.com/AdguardTeam/golibs/mathutil" ) // UnsupportedError is returned by functions and methods when a particular @@ -63,7 +61,7 @@ func RunCommand(command string, arguments ...string) (code int, output []byte, e cmd := exec.Command(command, arguments...) out, err := cmd.Output() - out = out[:mathutil.Min(len(out), MaxCmdOutputSize)] + out = out[:min(len(out), MaxCmdOutputSize)] if err != nil { if eerr := new(exec.ExitError); errors.As(err, &eerr) { @@ -142,7 +140,7 @@ func parsePSOutput(r io.Reader, cmdName string, ignore []int) (largest, instNum } instNum++ - largest = mathutil.Max(largest, cur) + largest = max(largest, cur) } if err = s.Err(); err != nil { return 0, 0, fmt.Errorf("scanning stdout: %w", err) @@ -156,13 +154,6 @@ func IsOpenWrt() (ok bool) { return isOpenWrt() } -// RootDirFS returns the [fs.FS] rooted at the operating system's root. On -// Windows it returns the fs.FS rooted at the volume of the system directory -// (usually, C:). -func RootDirFS() (fsys fs.FS) { - return rootDirFS() -} - // NotifyReconfigureSignal notifies c on receiving reconfigure signals. func NotifyReconfigureSignal(c chan<- os.Signal) { notifyReconfigureSignal(c) diff --git a/internal/aghos/os_linux.go b/internal/aghos/os_linux.go index daffb621..4e8a31ea 100644 --- a/internal/aghos/os_linux.go +++ b/internal/aghos/os_linux.go @@ -7,6 +7,7 @@ import ( "os" "syscall" + "github.com/AdguardTeam/golibs/osutil" "github.com/AdguardTeam/golibs/stringutil" ) @@ -40,7 +41,7 @@ func isOpenWrt() (ok bool) { } return nil, !stringutil.ContainsFold(string(data), osNameData), nil - }).Walk(RootDirFS(), etcReleasePattern) + }).Walk(osutil.RootDirFS(), etcReleasePattern) return err == nil && ok } diff --git a/internal/aghos/os_unix.go b/internal/aghos/os_unix.go index b6ba0a21..f52fab02 100644 --- a/internal/aghos/os_unix.go +++ b/internal/aghos/os_unix.go @@ -3,17 +3,12 @@ package aghos import ( - "io/fs" "os" "os/signal" "golang.org/x/sys/unix" ) -func rootDirFS() (fsys fs.FS) { - return os.DirFS("/") -} - func notifyReconfigureSignal(c chan<- os.Signal) { signal.Notify(c, unix.SIGHUP) } diff --git a/internal/aghos/os_windows.go b/internal/aghos/os_windows.go index b30aa719..2c2620eb 100644 --- a/internal/aghos/os_windows.go +++ b/internal/aghos/os_windows.go @@ -3,29 +3,13 @@ package aghos import ( - "io/fs" "os" "os/signal" - "path/filepath" "syscall" - "github.com/AdguardTeam/golibs/log" "golang.org/x/sys/windows" ) -func rootDirFS() (fsys fs.FS) { - // TODO(a.garipov): Use a better way if golang/go#44279 is ever resolved. - sysDir, err := windows.GetSystemDirectory() - if err != nil { - log.Error("aghos: getting root filesystem: %s; using C:", err) - - // Assume that C: is the safe default. - return os.DirFS("C:") - } - - return os.DirFS(filepath.VolumeName(sysDir)) -} - func setRlimit(val uint64) (err error) { return Unsupported("setrlimit") } diff --git a/internal/aghtest/interface.go b/internal/aghtest/interface.go index f21f6e57..b4804322 100644 --- a/internal/aghtest/interface.go +++ b/internal/aghtest/interface.go @@ -26,14 +26,25 @@ import ( // FSWatcher is a fake [aghos.FSWatcher] implementation for tests. type FSWatcher struct { + OnStart func() (err error) + OnClose func() (err error) OnEvents func() (e <-chan struct{}) OnAdd func(name string) (err error) - OnClose func() (err error) } // type check var _ aghos.FSWatcher = (*FSWatcher)(nil) +// Start implements the [aghos.FSWatcher] interface for *FSWatcher. +func (w *FSWatcher) Start() (err error) { + return w.OnStart() +} + +// Close implements the [aghos.FSWatcher] interface for *FSWatcher. +func (w *FSWatcher) Close() (err error) { + return w.OnClose() +} + // Events implements the [aghos.FSWatcher] interface for *FSWatcher. func (w *FSWatcher) Events() (e <-chan struct{}) { return w.OnEvents() @@ -44,11 +55,6 @@ func (w *FSWatcher) Add(name string) (err error) { return w.OnAdd(name) } -// Close implements the [aghos.FSWatcher] interface for *FSWatcher. -func (w *FSWatcher) Close() (err error) { - return w.OnClose() -} - // Package agh // ServiceWithConfig is a fake [agh.ServiceWithConfig] implementation for tests. diff --git a/internal/arpdb/arpdb.go b/internal/arpdb/arpdb.go index 8277310a..8405e4ba 100644 --- a/internal/arpdb/arpdb.go +++ b/internal/arpdb/arpdb.go @@ -14,6 +14,7 @@ import ( "github.com/AdguardTeam/golibs/errors" "github.com/AdguardTeam/golibs/log" "github.com/AdguardTeam/golibs/netutil" + "github.com/AdguardTeam/golibs/osutil" ) // Variables and functions to substitute in tests. @@ -22,7 +23,7 @@ var ( aghosRunCommand = aghos.RunCommand // rootDirFS is the filesystem pointing to the root directory. - rootDirFS = aghos.RootDirFS() + rootDirFS = osutil.RootDirFS() ) // Interface stores and refreshes the network neighborhood reported by ARP diff --git a/internal/dnsforward/dnsforward_test.go b/internal/dnsforward/dnsforward_test.go index ece14ba3..0cbb21cb 100644 --- a/internal/dnsforward/dnsforward_test.go +++ b/internal/dnsforward/dnsforward_test.go @@ -1330,6 +1330,7 @@ func TestPTRResponseFromHosts(t *testing.T) { var eventsCalledCounter uint32 hc, err := aghnet.NewHostsContainer(testFS, &aghtest.FSWatcher{ + OnStart: func() (_ error) { panic("not implemented") }, OnEvents: func() (e <-chan struct{}) { assert.Equal(t, uint32(1), atomic.AddUint32(&eventsCalledCounter, 1)) diff --git a/internal/dnsforward/http_test.go b/internal/dnsforward/http_test.go index b642eb2c..66499746 100644 --- a/internal/dnsforward/http_test.go +++ b/internal/dnsforward/http_test.go @@ -418,6 +418,7 @@ func TestServer_HandleTestUpstreamDNS(t *testing.T) { }, }, &aghtest.FSWatcher{ + OnStart: func() (_ error) { panic("not implemented") }, OnEvents: func() (e <-chan struct{}) { return nil }, OnAdd: func(_ string) (err error) { return nil }, OnClose: func() (err error) { return nil }, diff --git a/internal/filtering/hosts_test.go b/internal/filtering/hosts_test.go index baa6675c..5ea7eff3 100644 --- a/internal/filtering/hosts_test.go +++ b/internal/filtering/hosts_test.go @@ -40,6 +40,7 @@ func TestDNSFilter_CheckHost_hostsContainer(t *testing.T) { }, } watcher := &aghtest.FSWatcher{ + OnStart: func() (_ error) { panic("not implemented") }, OnEvents: func() (e <-chan struct{}) { return nil }, OnAdd: func(name string) (err error) { return nil }, OnClose: func() (err error) { return nil }, diff --git a/internal/filtering/rewrites.go b/internal/filtering/rewrites.go index ba759f6c..5ac2ffcc 100644 --- a/internal/filtering/rewrites.go +++ b/internal/filtering/rewrites.go @@ -8,7 +8,6 @@ import ( "github.com/AdguardTeam/golibs/errors" "github.com/AdguardTeam/golibs/log" - "github.com/AdguardTeam/golibs/mathutil" "github.com/miekg/dns" ) @@ -181,7 +180,7 @@ func findRewrites( if isWildcard(r.Domain) { // Don't use rewrites[:0], because we need to return at least one // item here. - rewrites = rewrites[:mathutil.Max(1, i)] + rewrites = rewrites[:max(1, i)] break } diff --git a/internal/home/control.go b/internal/home/control.go index db0c2c5e..ec9192fd 100644 --- a/internal/home/control.go +++ b/internal/home/control.go @@ -15,7 +15,6 @@ import ( "github.com/AdguardTeam/AdGuardHome/internal/version" "github.com/AdguardTeam/golibs/httphdr" "github.com/AdguardTeam/golibs/log" - "github.com/AdguardTeam/golibs/mathutil" "github.com/AdguardTeam/golibs/netutil" "github.com/NYTimes/gziphandler" ) @@ -145,10 +144,7 @@ func handleStatus(w http.ResponseWriter, r *http.Request) { // Make sure that we don't send negative numbers to the frontend, // since enough time might have passed to make the difference less // than zero. - protectionDisabledDuration = mathutil.Max( - 0, - time.Until(*protectionDisabledUntil).Milliseconds(), - ) + protectionDisabledDuration = max(0, time.Until(*protectionDisabledUntil).Milliseconds()) } resp = statusResponse{ diff --git a/internal/home/home.go b/internal/home/home.go index e3230424..22f6130c 100644 --- a/internal/home/home.go +++ b/internal/home/home.go @@ -250,7 +250,7 @@ func setupHostsContainer() (err error) { return errors.Join(fmt.Errorf("initializing hosts container: %w", err), closeErr) } - return nil + return hostsWatcher.Start() } // setupOpts sets up command-line options.