Pull request 1897: nextapi-write-conf

Squashed commit of the following:

commit 72f25ffe73d6b8216b01e590fba66fb5f6944113
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Wed Jun 28 21:29:04 2023 +0300

    next: add conf writing, validation

internal/dhcpd/migrate.go

@@ -51,6 +51,9 @@ func migrateDB(conf *ServerConfig) (err error) {
 	oldLeasesPath := filepath.Join(conf.WorkDir, dbFilename)
 	dataDirPath := filepath.Join(conf.DataDir, dataFilename)
 
+	// #nosec G304 -- Trust this path, since it's taken from the old file name
+	// relative to the working directory and should generally be considered
+	// safe.
 	file, err := os.Open(oldLeasesPath)
 	if errors.Is(err, os.ErrNotExist) {
 		// Nothing to migrate.

internal/dnsforward/config.go

@@ -540,6 +540,7 @@ func (s *Server) prepareIpsetListSettings() (err error) {
 		return s.ipset.init(s.conf.IpsetList)
 	}
 
+	// #nosec G304 -- Trust the path explicitly given by the user.
 	data, err := os.ReadFile(fn)
 	if err != nil {
 		return err

internal/next/changelog.md

@@ -24,6 +24,7 @@ enough.
 
 ### Fixed
 
+- `--check-config` breaking the configuration file ([#4067]).
 - Inconsistent application of `--work-dir/-w` ([#2598], [#2902]).
 - The order of `-v/--verbose` and `--version` being significant ([#2893]).
 
@@ -36,4 +37,5 @@ enough.
 [#2598]: https://github.com/AdguardTeam/AdGuardHome/issues/2598
 [#2893]: https://github.com/AdguardTeam/AdGuardHome/issues/2893
 [#2902]: https://github.com/AdguardTeam/AdGuardHome/issues/2902
+[#4067]: https://github.com/AdguardTeam/AdGuardHome/issues/4067
 [#5676]: https://github.com/AdguardTeam/AdGuardHome/issues/5676

internal/next/cmd/cmd.go

@@ -19,8 +19,6 @@ import (
 func Main(frontend fs.FS) {
 	start := time.Now()
 
-	// Initial Configuration
-
 	cmdName := os.Args[0]
 	opts, err := parseOptions(cmdName, os.Args[1:])
 	exitCode, needExit := processOptions(opts, cmdName, err)
@@ -39,9 +37,7 @@ func Main(frontend fs.FS) {
 		check(err)
 	}
 
-	// Web Service
-
-	confMgr, err := configmgr.New(opts.confFile, frontend, start)
+	confMgr, err := newConfigMgr(opts.confFile, frontend, start)
 	check(err)
 
 	web := confMgr.Web()
@@ -73,6 +69,15 @@ func ctxWithDefaultTimeout() (ctx context.Context, cancel context.CancelFunc) {
 	return context.WithTimeout(context.Background(), defaultTimeout)
 }
 
+// newConfigMgr returns a new configuration manager using defaultTimeout as the
+// context timeout.
+func newConfigMgr(confFile string, frontend fs.FS, start time.Time) (m *configmgr.Manager, err error) {
+	ctx, cancel := ctxWithDefaultTimeout()
+	defer cancel()
+
+	return configmgr.New(ctx, confFile, frontend, start)
+}
+
 // check is a simple error-checking helper.  It must only be used within Main.
 func check(err error) {
 	if err != nil {

internal/next/cmd/opt.go

@@ -8,6 +8,7 @@ import (
 	"os"
 	"strings"
 
+	"github.com/AdguardTeam/AdGuardHome/internal/next/configmgr"
 	"github.com/AdguardTeam/AdGuardHome/internal/version"
 	"golang.org/x/exp/slices"
 )
@@ -55,9 +56,8 @@ type options struct {
 	webAddrs []netip.AddrPort
 
 	// checkConfig, if true, instructs AdGuard Home to check the configuration
-	// file and exit with a corresponding exit code.
-	//
-	// TODO(a.garipov): Use.
+	// file, optionally print an error message to stdout, and exit with a
+	// corresponding exit code.
 	checkConfig bool
 
 	// disableUpdate, if true, prevents AdGuard Home from automatically checking
@@ -183,7 +183,7 @@ var commandLineOptions = []*commandLineOption{
 
 	checkConfigIdx: {
 		defaultValue: false,
-		description:  "Check configuration and quit.",
+		description:  "Check configuration, print errors to stdout, and quit.",
 		long:         "check-config",
 		short:        "",
 		valueType:    "",
@@ -399,5 +399,16 @@ func processOptions(
 		return 0, true
 	}
 
+	if opts.checkConfig {
+		err := configmgr.Validate(opts.confFile)
+		if err != nil {
+			_, _ = io.WriteString(os.Stdout, err.Error()+"\n")
+
+			return 1, true
+		}
+
+		return 0, true
+	}
+
 	return 0, false
 }

internal/next/cmd/signal.go

@@ -7,7 +7,6 @@ import (
 
 	"github.com/AdguardTeam/AdGuardHome/internal/aghos"
 	"github.com/AdguardTeam/AdGuardHome/internal/next/agh"
-	"github.com/AdguardTeam/AdGuardHome/internal/next/configmgr"
 	"github.com/AdguardTeam/golibs/log"
 )
 
@@ -63,7 +62,7 @@ func (h *signalHandler) reconfigure() {
 	// reconfigured without the full shutdown, and the error handling is
 	// currently not the best.
 
-	confMgr, err := configmgr.New(h.confFile, h.frontend, h.start)
+	confMgr, err := newConfigMgr(h.confFile, h.frontend, h.start)
 	check(err)
 
 	web := confMgr.Web()

internal/next/configmgr/config.go

@@ -1,8 +1,10 @@
 package configmgr
 
 import (
+	"fmt"
 	"net/netip"
 
+	"github.com/AdguardTeam/golibs/errors"
 	"github.com/AdguardTeam/golibs/timeutil"
 )
 
@@ -19,6 +21,38 @@ type config struct {
 	Verbose    bool `yaml:"verbose"`
 }
 
+const errNoConf errors.Error = "configuration not found"
+
+// validate returns an error if the configuration structure is invalid.
+func (c *config) validate() (err error) {
+	if c == nil {
+		return errNoConf
+	}
+
+	// TODO(a.garipov): Add more validations.
+
+	// Keep this in the same order as the fields in the config.
+	validators := []struct {
+		validate func() (err error)
+		name     string
+	}{{
+		validate: c.DNS.validate,
+		name:     "dns",
+	}, {
+		validate: c.HTTP.validate,
+		name:     "http",
+	}}
+
+	for _, v := range validators {
+		err = v.validate()
+		if err != nil {
+			return fmt.Errorf("%s: %w", v.name, err)
+		}
+	}
+
+	return nil
+}
+
 // dnsConfig is the on-disk DNS configuration.
 //
 // TODO(a.garipov): Validate.
@@ -32,6 +66,21 @@ type dnsConfig struct {
 	UseDNS64            bool              `yaml:"use_dns64"`
 }
 
+// validate returns an error if the DNS configuration structure is invalid.
+//
+// TODO(a.garipov): Add more validations.
+func (c *dnsConfig) validate() (err error) {
+	// TODO(a.garipov): Add more validations.
+	switch {
+	case c == nil:
+		return errNoConf
+	case c.UpstreamTimeout.Duration <= 0:
+		return newMustBePositiveError("upstream_timeout", c.UpstreamTimeout)
+	default:
+		return nil
+	}
+}
+
 // httpConfig is the on-disk web API configuration.
 //
 // TODO(a.garipov): Validate.
@@ -41,3 +90,17 @@ type httpConfig struct {
 	Timeout         timeutil.Duration `yaml:"timeout"`
 	ForceHTTPS      bool              `yaml:"force_https"`
 }
+
+// validate returns an error if the HTTP configuration structure is invalid.
+//
+// TODO(a.garipov): Add more validations.
+func (c *httpConfig) validate() (err error) {
+	switch {
+	case c == nil:
+		return errNoConf
+	case c.Timeout.Duration <= 0:
+		return newMustBePositiveError("timeout", c.Timeout)
+	default:
+		return nil
+	}
+}

internal/next/configmgr/configmgr.go

@@ -1,5 +1,7 @@
 // Package configmgr defines the AdGuard Home on-disk configuration entities and
 // configuration manager.
+//
+// TODO(a.garipov): Add tests.
 package configmgr
 
 import (
@@ -14,6 +16,10 @@ import (
 	"github.com/AdguardTeam/AdGuardHome/internal/next/dnssvc"
 	"github.com/AdguardTeam/AdGuardHome/internal/next/websvc"
 	"github.com/AdguardTeam/golibs/errors"
+	"github.com/AdguardTeam/golibs/log"
+	"github.com/AdguardTeam/golibs/timeutil"
+	"github.com/google/renameio/maybe"
+	"golang.org/x/exp/slices"
 	"gopkg.in/yaml.v3"
 )
 
@@ -39,17 +45,58 @@ type Manager struct {
 	fileName string
 }
 
+// Validate returns an error if the configuration file with the given name does
+// not exist or is invalid.
+func Validate(fileName string) (err error) {
+	conf, err := read(fileName)
+	if err != nil {
+		// Don't wrap the error, because it's informative enough as is.
+		return err
+	}
+
+	// Don't wrap the error, because it's informative enough as is.
+	return conf.validate()
+}
+
 // New creates a new *Manager that persists changes to the file pointed to by
 // fileName.  It reads the configuration file and populates the service fields.
 // start is the startup time of AdGuard Home.
 func New(
+	ctx context.Context,
 	fileName string,
 	frontend fs.FS,
 	start time.Time,
 ) (m *Manager, err error) {
+	conf, err := read(fileName)
+	if err != nil {
+		// Don't wrap the error, because it's informative enough as is.
+		return nil, err
+	}
+
+	err = conf.validate()
+	if err != nil {
+		return nil, fmt.Errorf("validating config: %w", err)
+	}
+
+	m = &Manager{
+		updMu:    &sync.RWMutex{},
+		current:  conf,
+		fileName: fileName,
+	}
+
+	err = m.assemble(ctx, conf, frontend, start)
+	if err != nil {
+		return nil, fmt.Errorf("creating config manager: %w", err)
+	}
+
+	return m, nil
+}
+
+// read reads and decodes configuration from the provided filename.
+func read(fileName string) (conf *config, err error) {
 	defer func() { err = errors.Annotate(err, "reading config: %w") }()
 
-	conf := &config{}
+	conf = &config{}
 	f, err := os.Open(fileName)
 	if err != nil {
 		// Don't wrap the error, because it's informative enough as is.
@@ -63,27 +110,7 @@ func New(
 		return nil, err
 	}
 
-	// TODO(a.garipov): Validate the configuration structure.  Return an error
-	// if it's incorrect.
-
-	m = &Manager{
-		updMu:    &sync.RWMutex{},
-		current:  conf,
-		fileName: fileName,
-	}
-
-	// TODO(a.garipov): Get the context with the timeout from the arguments?
-	const assemblyTimeout = 5 * time.Second
-	ctx, cancel := context.WithTimeout(context.Background(), assemblyTimeout)
-	defer cancel()
-
-	err = m.assemble(ctx, conf, frontend, start)
-	if err != nil {
-		// Don't wrap the error, because it's informative enough as is.
-		return nil, err
-	}
-
-	return m, nil
+	return conf, nil
 }
 
 // assemble creates all services and puts them into the corresponding fields.
@@ -128,6 +155,23 @@ func (m *Manager) assemble(
 	return nil
 }
 
+// write writes the current configuration to disk.
+func (m *Manager) write() (err error) {
+	b, err := yaml.Marshal(m.current)
+	if err != nil {
+		return fmt.Errorf("encoding: %w", err)
+	}
+
+	err = maybe.WriteFile(m.fileName, b, 0o755)
+	if err != nil {
+		return fmt.Errorf("writing: %w", err)
+	}
+
+	log.Info("configmgr: written to %q", m.fileName)
+
+	return nil
+}
+
 // DNS returns the current DNS service.  It is safe for concurrent use.
 func (m *Manager) DNS() (dns agh.ServiceWithConfig[*dnssvc.Config]) {
 	m.updMu.RLock()
@@ -150,7 +194,9 @@ func (m *Manager) UpdateDNS(ctx context.Context, c *dnssvc.Config) (err error) {
 		return fmt.Errorf("reassembling dnssvc: %w", err)
 	}
 
-	return nil
+	m.updateCurrentDNS(c)
+
+	return m.write()
 }
 
 // updateDNS recreates the DNS service.  m.updMu is expected to be locked.
@@ -172,6 +218,17 @@ func (m *Manager) updateDNS(ctx context.Context, c *dnssvc.Config) (err error) {
 	return nil
 }
 
+// updateCurrentDNS updates the DNS configuration in the current config.
+func (m *Manager) updateCurrentDNS(c *dnssvc.Config) {
+	m.current.DNS.Addresses = slices.Clone(c.Addresses)
+	m.current.DNS.BootstrapDNS = slices.Clone(c.BootstrapServers)
+	m.current.DNS.UpstreamDNS = slices.Clone(c.UpstreamServers)
+	m.current.DNS.DNS64Prefixes = slices.Clone(c.DNS64Prefixes)
+	m.current.DNS.UpstreamTimeout = timeutil.Duration{Duration: c.UpstreamTimeout}
+	m.current.DNS.BootstrapPreferIPv6 = c.BootstrapPreferIPv6
+	m.current.DNS.UseDNS64 = c.UseDNS64
+}
+
 // Web returns the current web service.  It is safe for concurrent use.
 func (m *Manager) Web() (web agh.ServiceWithConfig[*websvc.Config]) {
 	m.updMu.RLock()
@@ -194,7 +251,9 @@ func (m *Manager) UpdateWeb(ctx context.Context, c *websvc.Config) (err error) {
 		return fmt.Errorf("reassembling websvc: %w", err)
 	}
 
-	return nil
+	m.updateCurrentWeb(c)
+
+	return m.write()
 }
 
 // updateWeb recreates the web service.  m.upd is expected to be locked.
@@ -213,3 +272,11 @@ func (m *Manager) updateWeb(ctx context.Context, c *websvc.Config) (err error) {
 
 	return nil
 }
+
+// updateCurrentWeb updates the web configuration in the current config.
+func (m *Manager) updateCurrentWeb(c *websvc.Config) {
+	m.current.HTTP.Addresses = slices.Clone(c.Addresses)
+	m.current.HTTP.SecureAddresses = slices.Clone(c.SecureAddresses)
+	m.current.HTTP.Timeout = timeutil.Duration{Duration: c.Timeout}
+	m.current.HTTP.ForceHTTPS = c.ForceHTTPS
+}

internal/next/configmgr/error.go

@@ -0,0 +1,27 @@
+package configmgr
+
+import (
+	"fmt"
+
+	"github.com/AdguardTeam/golibs/timeutil"
+	"golang.org/x/exp/constraints"
+)
+
+// numberOrDuration is the constraint for integer types along with
+// timeutil.Duration.
+type numberOrDuration interface {
+	constraints.Integer | timeutil.Duration
+}
+
+// newMustBePositiveError returns an error about the value that must be positive
+// but isn't.  prop is the name of the property to mention in the error message.
+//
+// TODO(a.garipov): Consider moving such helpers to golibs and use in AdGuardDNS
+// as well.
+func newMustBePositiveError[T numberOrDuration](prop string, v T) (err error) {
+	if s, ok := any(v).(fmt.Stringer); ok {
+		return fmt.Errorf("%s must be positive, got %s", prop, s)
+	}
+
+	return fmt.Errorf("%s must be positive, got %d", prop, v)
+}

scripts/make/go-lint.sh

@@ -197,8 +197,25 @@ run_linter nilness ./...
 
 run_linter -e shadow --strict ./...
 
-# TODO(a.garipov): Enable in v0.108.0.
-# run_linter gosec --quiet ./...
+# TODO(a.garipov): Enable for all.
+run_linter gosec --quiet\
+	./internal/aghalg\
+	./internal/aghchan\
+	./internal/aghhttp\
+	./internal/aghio\
+	./internal/aghnet\
+	./internal/aghos\
+	./internal/aghtest\
+	./internal/dhcpd\
+	./internal/dhcpsvc\
+	./internal/dnsforward\
+	./internal/next\
+	./internal/schedule\
+	./internal/stats\
+	./internal/tools\
+	./internal/version\
+	./internal/whois\
+	;
 
 # TODO(a.garipov): Enable --blank?
 run_linter errcheck --asserts ./...