Pull request 1937: imp-filter-upd

Squashed commit of the following:

commit 6ce649c06398cf8a6f8e1a90f560fa8205f6500e
Merge: 1c6327e5d 996c6b3ee
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Tue Jul 25 17:42:01 2023 +0300

    Merge branch 'master' into imp-filter-upd

commit 1c6327e5d4c04393abc5d4d3e4b8568d4c6eca23
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Fri Jul 21 17:32:47 2023 +0300

    all: imp code; use renameio/v2 consistently

commit 1669288c9b662d1310f83a4e0d3f1f60731188cd
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Fri Jul 21 16:26:17 2023 +0300

    all: add renameioutil; imp flt upd
This commit is contained in:
Ainar Garipov 2023-07-25 17:47:24 +03:00
parent 996c6b3ee3
commit 698b963e11
16 changed files with 402 additions and 131 deletions

View File

@ -23,6 +23,10 @@ See also the [v0.107.35 GitHub milestone][ms-v0.107.35].
NOTE: Add new changes BELOW THIS COMMENT.
-->
### Changed
- Improved reliability filtering-rule list updates on Unix systems.
### Fixed
- Occasional client information lookup failures that could lead to the DNS

2
go.mod
View File

@ -15,7 +15,7 @@ require (
github.com/go-ping/ping v1.1.0
github.com/google/go-cmp v0.5.9
github.com/google/gopacket v1.1.19
github.com/google/renameio v1.0.1
github.com/google/renameio/v2 v2.0.0
github.com/google/uuid v1.3.0
github.com/insomniacslk/dhcp v0.0.0-20230612134759-b20c9ba983df
github.com/josharian/native v1.1.1-0.20230202152459-5c7d0dd6ab86

4
go.sum
View File

@ -52,8 +52,8 @@ github.com/google/gopacket v1.1.19 h1:ves8RnFZPGiFnTS0uPQStjwru6uO6h+nlr9j6fL7kF
github.com/google/gopacket v1.1.19/go.mod h1:iJ8V8n6KS+z2U1A8pUwu8bW5SyEMkXJB8Yo/Vo+TKTo=
github.com/google/pprof v0.0.0-20230602150820-91b7bce49751 h1:hR7/MlvK23p6+lIw9SN1TigNLn9ZnF3W4SYRKq2gAHs=
github.com/google/pprof v0.0.0-20230602150820-91b7bce49751/go.mod h1:Jh3hGz2jkYak8qXPD19ryItVnUgpgeqzdkY/D0EaeuA=
github.com/google/renameio v1.0.1 h1:Lh/jXZmvZxb0BBeSY5VKEfidcbcbenKjZFzM/q0fSeU=
github.com/google/renameio v1.0.1/go.mod h1:t/HQoYBZSsWSNK35C6CO/TpPLDVWvxOHboWUAweKUpk=
github.com/google/renameio/v2 v2.0.0 h1:UifI23ZTGY8Tt29JbYFiuyIU3eX+RNFtUwefq9qAhxg=
github.com/google/renameio/v2 v2.0.0/go.mod h1:BtmJXm5YlszgC+TD4HOEEUFgkJP3nLxehU6hfe7jRt4=
github.com/google/uuid v1.2.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
github.com/google/uuid v1.3.0 h1:t6JiXgmwXMjEs8VusXIJk2BXHsn+wx8BZdTaoZ5fu7I=
github.com/google/uuid v1.3.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=

View File

@ -14,7 +14,7 @@ import (
"github.com/AdguardTeam/golibs/errors"
"github.com/AdguardTeam/golibs/log"
"github.com/AdguardTeam/golibs/stringutil"
"github.com/google/renameio/maybe"
"github.com/google/renameio/v2/maybe"
"golang.org/x/sys/unix"
)

View File

@ -0,0 +1,52 @@
// Package aghrenameio is a wrapper around package github.com/google/renameio/v2
// that provides a similar stream-based API for both Unix and Windows systems.
// While the Windows API is not technically atomic, it still provides a
// consistent stream-based interface, and atomic renames of files do not seem to
// be possible in all cases anyway.
//
// See https://github.com/google/renameio/issues/1.
//
// TODO(a.garipov): Consider moving to golibs/renameioutil once tried and
// tested.
package aghrenameio
import (
"io/fs"
"github.com/AdguardTeam/golibs/errors"
)
// PendingFile is the interface for pending temporary files.
type PendingFile interface {
// Cleanup closes the file, and removes it without performing the renaming.
// To close and rename the file, use CloseReplace.
Cleanup() (err error)
// CloseReplace closes the temporary file and replaces the destination file
// with it, possibly atomically.
//
// This method is not safe for concurrent use by multiple goroutines.
CloseReplace() (err error)
// Write writes len(b) bytes from b to the File. It returns the number of
// bytes written and an error, if any. Write returns a non-nil error when n
// != len(b).
Write(b []byte) (n int, err error)
}
// NewPendingFile is a wrapper around [renameio.NewPendingFile] on Unix systems
// and [os.CreateTemp] on Windows.
func NewPendingFile(filePath string, mode fs.FileMode) (f PendingFile, err error) {
return newPendingFile(filePath, mode)
}
// WithDeferredCleanup is a helper that performs the necessary cleanups and
// finalizations of the temporary files based on the returned error.
func WithDeferredCleanup(returned error, file PendingFile) (err error) {
// Make sure that any error returned from here is marked as a deferred one.
if returned != nil {
return errors.WithDeferred(returned, file.Cleanup())
}
return errors.WithDeferred(nil, file.CloseReplace())
}

View File

@ -0,0 +1,101 @@
package aghrenameio_test
import (
"io/fs"
"os"
"path/filepath"
"testing"
"github.com/AdguardTeam/AdGuardHome/internal/aghrenameio"
"github.com/AdguardTeam/golibs/errors"
"github.com/AdguardTeam/golibs/testutil"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
// testPerm is the common permission mode for tests.
const testPerm fs.FileMode = 0o644
// Common file data for tests.
var (
initialData = []byte("initial data\n")
newData = []byte("new data\n")
)
func TestPendingFile(t *testing.T) {
t.Parallel()
targetPath := newInitialFile(t)
f, err := aghrenameio.NewPendingFile(targetPath, testPerm)
require.NoError(t, err)
_, err = f.Write(newData)
require.NoError(t, err)
err = f.CloseReplace()
require.NoError(t, err)
gotData, err := os.ReadFile(targetPath)
require.NoError(t, err)
assert.Equal(t, newData, gotData)
}
// newInitialFile is a test helper that returns the path to the file containing
// [initialData].
func newInitialFile(t *testing.T) (targetPath string) {
t.Helper()
dir := t.TempDir()
targetPath = filepath.Join(dir, "target")
err := os.WriteFile(targetPath, initialData, 0o644)
require.NoError(t, err)
return targetPath
}
func TestWithDeferredCleanup(t *testing.T) {
t.Parallel()
const testError errors.Error = "test error"
testCases := []struct {
error error
name string
wantErrMsg string
wantData []byte
}{{
name: "success",
error: nil,
wantErrMsg: "",
wantData: newData,
}, {
name: "error",
error: testError,
wantErrMsg: testError.Error(),
wantData: initialData,
}}
for _, tc := range testCases {
tc := tc
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
targetPath := newInitialFile(t)
f, err := aghrenameio.NewPendingFile(targetPath, testPerm)
require.NoError(t, err)
_, err = f.Write(newData)
require.NoError(t, err)
err = aghrenameio.WithDeferredCleanup(tc.error, f)
testutil.AssertErrorMsg(t, tc.wantErrMsg, err)
gotData, err := os.ReadFile(targetPath)
require.NoError(t, err)
assert.Equal(t, tc.wantData, gotData)
})
}
}

View File

@ -0,0 +1,48 @@
//go:build unix
package aghrenameio
import (
"io/fs"
"github.com/google/renameio/v2"
)
// pendingFile is a wrapper around [*renameio.PendingFile] making it an
// [io.WriteCloser].
type pendingFile struct {
file *renameio.PendingFile
}
// type check
var _ PendingFile = pendingFile{}
// Cleanup implements the [PendingFile] interface for pendingFile.
func (f pendingFile) Cleanup() (err error) {
return f.file.Cleanup()
}
// CloseReplace implements the [PendingFile] interface for pendingFile.
func (f pendingFile) CloseReplace() (err error) {
return f.file.CloseAtomicallyReplace()
}
// Write implements the [PendingFile] interface for pendingFile.
func (f pendingFile) Write(b []byte) (n int, err error) {
return f.file.Write(b)
}
// NewPendingFile is a wrapper around [renameio.NewPendingFile].
//
// f.Close must be called to finish the renaming.
func newPendingFile(filePath string, mode fs.FileMode) (f PendingFile, err error) {
file, err := renameio.NewPendingFile(filePath, renameio.WithPermissions(mode))
if err != nil {
// Don't wrap the error since it's informative enough as is.
return nil, err
}
return pendingFile{
file: file,
}, nil
}

View File

@ -0,0 +1,74 @@
//go:build windows
package aghrenameio
import (
"fmt"
"io/fs"
"os"
"path/filepath"
"github.com/AdguardTeam/golibs/errors"
)
// pendingFile is a wrapper around [*os.File] calling [os.Rename] in its Close
// method.
type pendingFile struct {
file *os.File
targetPath string
}
// type check
var _ PendingFile = (*pendingFile)(nil)
// Cleanup implements the [PendingFile] interface for *pendingFile.
func (f *pendingFile) Cleanup() (err error) {
closeErr := f.file.Close()
err = os.Remove(f.file.Name())
// Put closeErr into the deferred error because that's where it is usually
// expected.
return errors.WithDeferred(err, closeErr)
}
// CloseReplace implements the [PendingFile] interface for *pendingFile.
func (f *pendingFile) CloseReplace() (err error) {
err = f.file.Close()
if err != nil {
return fmt.Errorf("closing: %w", err)
}
err = os.Rename(f.file.Name(), f.targetPath)
if err != nil {
return fmt.Errorf("renaming: %w", err)
}
return nil
}
// Write implements the [PendingFile] interface for *pendingFile.
func (f *pendingFile) Write(b []byte) (n int, err error) {
return f.file.Write(b)
}
// NewPendingFile is a wrapper around [os.CreateTemp].
//
// f.Close must be called to finish the renaming.
func newPendingFile(filePath string, mode fs.FileMode) (f PendingFile, err error) {
// Use the same directory as the file itself, because moves across
// filesystems can be especially problematic.
file, err := os.CreateTemp(filepath.Dir(filePath), "")
if err != nil {
return nil, fmt.Errorf("opening pending file: %w", err)
}
err = file.Chmod(mode)
if err != nil {
return nil, fmt.Errorf("preparing pending file: %w", err)
}
return &pendingFile{
file: file,
targetPath: filePath,
}, nil
}

View File

@ -9,7 +9,7 @@ import (
"github.com/AdguardTeam/golibs/errors"
"github.com/AdguardTeam/golibs/log"
"github.com/google/renameio/maybe"
"github.com/google/renameio/v2/maybe"
"golang.org/x/exp/slices"
)

View File

@ -11,6 +11,7 @@ import (
"time"
"github.com/AdguardTeam/AdGuardHome/internal/aghalg"
"github.com/AdguardTeam/AdGuardHome/internal/aghrenameio"
"github.com/AdguardTeam/AdGuardHome/internal/filtering/rulelist"
"github.com/AdguardTeam/golibs/errors"
"github.com/AdguardTeam/golibs/log"
@ -83,53 +84,53 @@ func (d *DNSFilter) filterSetProperties(
filters = d.WhitelistFilters
}
i := slices.IndexFunc(filters, func(filt FilterYAML) bool { return filt.URL == listURL })
i := slices.IndexFunc(filters, func(flt FilterYAML) bool { return flt.URL == listURL })
if i == -1 {
return false, errFilterNotExist
}
filt := &filters[i]
flt := &filters[i]
log.Debug(
"filtering: set name to %q, url to %s, enabled to %t for filter %s",
newList.Name,
newList.URL,
newList.Enabled,
filt.URL,
flt.URL,
)
defer func(oldURL, oldName string, oldEnabled bool, oldUpdated time.Time, oldRulesCount int) {
if err != nil {
filt.URL = oldURL
filt.Name = oldName
filt.Enabled = oldEnabled
filt.LastUpdated = oldUpdated
filt.RulesCount = oldRulesCount
flt.URL = oldURL
flt.Name = oldName
flt.Enabled = oldEnabled
flt.LastUpdated = oldUpdated
flt.RulesCount = oldRulesCount
}
}(filt.URL, filt.Name, filt.Enabled, filt.LastUpdated, filt.RulesCount)
}(flt.URL, flt.Name, flt.Enabled, flt.LastUpdated, flt.RulesCount)
filt.Name = newList.Name
flt.Name = newList.Name
if filt.URL != newList.URL {
if flt.URL != newList.URL {
if d.filterExistsLocked(newList.URL) {
return false, errFilterExists
}
shouldRestart = true
filt.URL = newList.URL
filt.LastUpdated = time.Time{}
filt.unload()
flt.URL = newList.URL
flt.LastUpdated = time.Time{}
flt.unload()
}
if filt.Enabled != newList.Enabled {
filt.Enabled = newList.Enabled
if flt.Enabled != newList.Enabled {
flt.Enabled = newList.Enabled
shouldRestart = true
}
if filt.Enabled {
if flt.Enabled {
if shouldRestart {
// Download the filter contents.
shouldRestart, err = d.update(filt)
shouldRestart, err = d.update(flt)
}
} else {
// TODO(e.burkov): The validation of the contents of the new URL is
@ -137,7 +138,7 @@ func (d *DNSFilter) filterSetProperties(
// possible to set a bad rules source, but the validation should still
// kick in when the filter is enabled. Consider changing this behavior
// to be stricter.
filt.unload()
flt.unload()
}
return shouldRestart, err
@ -250,24 +251,24 @@ func assignUniqueFilterID() int64 {
// Sets up a timer that will be checking for filters updates periodically
func (d *DNSFilter) periodicallyRefreshFilters() {
const maxInterval = 1 * 60 * 60
intval := 5 // use a dynamically increasing time interval
ivl := 5 // use a dynamically increasing time interval
for {
isNetErr, ok := false, false
if d.FiltersUpdateIntervalHours != 0 {
_, isNetErr, ok = d.tryRefreshFilters(true, true, false)
if ok && !isNetErr {
intval = maxInterval
ivl = maxInterval
}
}
if isNetErr {
intval *= 2
if intval > maxInterval {
intval = maxInterval
ivl *= 2
if ivl > maxInterval {
ivl = maxInterval
}
}
time.Sleep(time.Duration(intval) * time.Second)
time.Sleep(time.Duration(ivl) * time.Second)
}
}
@ -329,20 +330,20 @@ func (d *DNSFilter) refreshFiltersArray(filters *[]FilterYAML, force bool) (int,
return 0, nil, nil, false
}
nfail := 0
failNum := 0
for i := range updateFilters {
uf := &updateFilters[i]
updated, err := d.update(uf)
updateFlags = append(updateFlags, updated)
if err != nil {
nfail++
log.Info("filtering: updating filter from url %q: %s\n", uf.URL, err)
failNum++
log.Error("filtering: updating filter from url %q: %s\n", uf.URL, err)
continue
}
}
if nfail == len(updateFilters) {
if failNum == len(updateFilters) {
return 0, nil, nil, true
}
@ -464,48 +465,6 @@ func (d *DNSFilter) update(filter *FilterYAML) (b bool, err error) {
return b, err
}
// finalizeUpdate closes and gets rid of temporary file f with filter's content
// according to updated. It also saves new values of flt's name, rules number
// and checksum if succeeded.
func (d *DNSFilter) finalizeUpdate(
file *os.File,
flt *FilterYAML,
updated bool,
res *rulelist.ParseResult,
) (err error) {
tmpFileName := file.Name()
// Close the file before renaming it because it's required on Windows.
//
// See https://github.com/adguardTeam/adGuardHome/issues/1553.
err = file.Close()
if err != nil {
return fmt.Errorf("closing temporary file: %w", err)
}
if !updated {
log.Debug("filtering: filter %d from url %q has no changes, skipping", flt.ID, flt.URL)
return os.Remove(tmpFileName)
}
fltPath := flt.Path(d.DataDir)
log.Info("filtering: saving contents of filter %d into %q", flt.ID, fltPath)
// Don't use renameio or maybe packages, since those will require loading
// the whole filter content to the memory on Windows.
err = os.Rename(tmpFileName, fltPath)
if err != nil {
return errors.WithDeferred(err, os.Remove(tmpFileName))
}
flt.Name = aghalg.Coalesce(flt.Name, res.Title)
flt.checksum, flt.RulesCount = res.Checksum, res.RulesCount
return nil
}
// updateIntl updates the flt rewriting it's actual file. It returns true if
// the actual update has been performed.
func (d *DNSFilter) updateIntl(flt *FilterYAML) (ok bool, err error) {
@ -513,63 +472,22 @@ func (d *DNSFilter) updateIntl(flt *FilterYAML) (ok bool, err error) {
var res *rulelist.ParseResult
var tmpFile *os.File
tmpFile, err = os.CreateTemp(filepath.Join(d.DataDir, filterDir), "")
if err != nil {
return false, err
}
defer func() {
finErr := d.finalizeUpdate(tmpFile, flt, ok, res)
if ok && finErr == nil {
log.Info(
"filtering: updated filter %d: %d bytes, %d rules",
flt.ID,
res.BytesWritten,
res.RulesCount,
)
return
}
err = errors.WithDeferred(err, finErr)
}()
// Change the default 0o600 permission to something more acceptable by end
// users.
//
// See https://github.com/AdguardTeam/AdGuardHome/issues/3198.
if err = tmpFile.Chmod(0o644); err != nil {
return false, fmt.Errorf("changing file mode: %w", err)
}
var r io.Reader
if !filepath.IsAbs(flt.URL) {
var resp *http.Response
resp, err = d.HTTPClient.Get(flt.URL)
tmpFile, err := aghrenameio.NewPendingFile(flt.Path(d.DataDir), 0o644)
if err != nil {
log.Info("filtering: requesting filter from %q: %s, skipping", flt.URL, err)
return false, err
}
defer func() { err = errors.WithDeferred(err, resp.Body.Close()) }()
defer func() { err = d.finalizeUpdate(tmpFile, flt, res, err, ok) }()
if resp.StatusCode != http.StatusOK {
log.Info("filtering got status code %d from %q, skipping", resp.StatusCode, flt.URL)
return false, fmt.Errorf("got status code %d, want %d", resp.StatusCode, http.StatusOK)
}
r = resp.Body
} else {
var f *os.File
f, err = os.Open(flt.URL)
r, err := d.reader(flt.URL)
if err != nil {
return false, fmt.Errorf("open file: %w", err)
}
defer func() { err = errors.WithDeferred(err, f.Close()) }()
r = f
// Don't wrap the error since it's informative enough as is.
return false, err
}
defer func() { err = errors.WithDeferred(err, r.Close()) }()
bufPtr := d.bufPool.Get().(*[]byte)
defer d.bufPool.Put(bufPtr)
@ -580,6 +498,78 @@ func (d *DNSFilter) updateIntl(flt *FilterYAML) (ok bool, err error) {
return res.Checksum != flt.checksum && err == nil, err
}
// finalizeUpdate closes and gets rid of temporary file f with filter's content
// according to updated. It also saves new values of flt's name, rules number
// and checksum if succeeded.
func (d *DNSFilter) finalizeUpdate(
file aghrenameio.PendingFile,
flt *FilterYAML,
res *rulelist.ParseResult,
returned error,
updated bool,
) (err error) {
id := flt.ID
if !updated {
if returned == nil {
log.Debug("filtering: filter %d from url %q has no changes, skipping", id, flt.URL)
}
return errors.WithDeferred(returned, file.Cleanup())
}
log.Info("filtering: saving contents of filter %d into %q", id, flt.Path(d.DataDir))
err = file.CloseReplace()
if err != nil {
return fmt.Errorf("finalizing update: %w", err)
}
rulesCount := res.RulesCount
log.Info("filtering: updated filter %d: %d bytes, %d rules", id, res.BytesWritten, rulesCount)
flt.Name = aghalg.Coalesce(flt.Name, res.Title)
flt.checksum = res.Checksum
flt.RulesCount = rulesCount
return nil
}
// reader returns an io.ReadCloser reading filtering-rule list data form either
// a file on the filesystem or the filter's HTTP URL.
func (d *DNSFilter) reader(fltURL string) (r io.ReadCloser, err error) {
if !filepath.IsAbs(fltURL) {
r, err = d.readerFromURL(fltURL)
if err != nil {
return nil, fmt.Errorf("reading from url: %w", err)
}
return r, nil
}
r, err = os.Open(fltURL)
if err != nil {
return nil, fmt.Errorf("opening file: %w", err)
}
return r, nil
}
// readerFromURL returns an io.ReadCloser reading filtering-rule list data form
// the filter's URL.
func (d *DNSFilter) readerFromURL(fltURL string) (r io.ReadCloser, err error) {
resp, err := d.HTTPClient.Get(fltURL)
if err != nil {
// Don't wrap the error since it's informative enough as is.
return nil, err
}
if resp.StatusCode != http.StatusOK {
return nil, fmt.Errorf("got status code %d, want %d", resp.StatusCode, http.StatusOK)
}
return resp.Body, nil
}
// loads filter contents from the file in dataDir
func (d *DNSFilter) load(flt *FilterYAML) (err error) {
fileName := flt.Path(d.DataDir)

View File

@ -20,7 +20,7 @@ import (
"github.com/AdguardTeam/golibs/errors"
"github.com/AdguardTeam/golibs/log"
"github.com/AdguardTeam/golibs/timeutil"
"github.com/google/renameio/maybe"
"github.com/google/renameio/v2/maybe"
"golang.org/x/exp/slices"
yaml "gopkg.in/yaml.v3"
)

View File

@ -17,7 +17,7 @@ import (
"github.com/AdguardTeam/golibs/log"
"github.com/AdguardTeam/golibs/netutil"
"github.com/AdguardTeam/golibs/timeutil"
"github.com/google/renameio/maybe"
"github.com/google/renameio/v2/maybe"
"golang.org/x/crypto/bcrypt"
yaml "gopkg.in/yaml.v3"
)

View File

@ -8,7 +8,7 @@ import (
"github.com/AdguardTeam/AdGuardHome/internal/next/agh"
"github.com/AdguardTeam/AdGuardHome/internal/next/configmgr"
"github.com/AdguardTeam/golibs/log"
"github.com/google/renameio/maybe"
"github.com/google/renameio/v2/maybe"
)
// signalHandler processes incoming signals and shuts services down.

View File

@ -19,7 +19,7 @@ import (
"github.com/AdguardTeam/golibs/errors"
"github.com/AdguardTeam/golibs/log"
"github.com/AdguardTeam/golibs/timeutil"
"github.com/google/renameio/maybe"
"github.com/google/renameio/v2/maybe"
"golang.org/x/exp/slices"
"gopkg.in/yaml.v3"
)

View File

@ -176,6 +176,7 @@ run_linter gocognit --over 10\
./internal/aghchan/\
./internal/aghhttp/\
./internal/aghio/\
./internal/aghrenameio/\
./internal/client/\
./internal/dhcpsvc\
./internal/filtering/hashprefix/\
@ -223,6 +224,7 @@ run_linter gosec --quiet\
./internal/aghio\
./internal/aghnet\
./internal/aghos\
./internal/aghrenameio/\
./internal/aghtest\
./internal/client\
./internal/dhcpd\

View File

@ -12,7 +12,7 @@ import (
"time"
"github.com/AdguardTeam/golibs/log"
"github.com/google/renameio/maybe"
"github.com/google/renameio/v2/maybe"
)
func main() {