Pull request 1984: AG-25392 confmigrate vol.1

Merge in DNS/adguard-home from AG-25392-confmigrate to master

Squashed commit of the following:

commit 695717573e228a71e387d1b597f0d32f2eb20e67
Merge: 2c4f3e096 27ec6cd59
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Thu Aug 31 16:22:55 2023 +0300

    Merge branch 'master' into AG-25392-confmigrate

commit 2c4f3e096bb14724c0d0fcc20e5ac1462068504e
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Thu Aug 31 16:13:14 2023 +0300

    all: imp code

commit 0fc6854598a67fc5ea74a93ff8c99b32886f43f1
Merge: 719f2db95 a2ca8b5b4
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Wed Aug 30 18:30:36 2023 +0300

    Merge branch 'master' into AG-25392-confmigrate

commit 719f2db95a337f343752f5b18ce935bae83127be
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Wed Aug 30 15:50:52 2023 +0300

    home: don't reread config

commit 2e25fb738b11675d25574da9e5eebacd72a793ba
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Wed Aug 30 15:43:57 2023 +0300

    all: imp code

commit be3021f03097e18228dd9904dacc283f2576472e
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Wed Aug 30 15:25:46 2023 +0300

    all: introduce confmigrate

internal/confmigrate/confmigrate.go

@@ -0,0 +1,143 @@
+// Package confmigrate provides a way to upgrade the YAML configuration file.
+package confmigrate
+
+import (
+	"bytes"
+	"fmt"
+
+	"github.com/AdguardTeam/golibs/log"
+	yaml "gopkg.in/yaml.v3"
+)
+
+// CurrentSchemaVersion is the current schema version.
+const CurrentSchemaVersion = 26
+
+// These aliases are provided for convenience.
+type (
+	yarr = []any
+	yobj = map[string]any
+)
+
+// Config is a the configuration for initializing a [Migrator].
+type Config struct {
+	// WorkingDir is an absolute path to the working directory of AdGuardHome.
+	WorkingDir string
+}
+
+// Migrator performs the YAML configuration file migrations.
+type Migrator struct {
+	// workingDir is an absolute path to the working directory of AdGuardHome.
+	workingDir string
+}
+
+// New creates a new Migrator.
+func New(cfg *Config) (m *Migrator) {
+	return &Migrator{
+		workingDir: cfg.WorkingDir,
+	}
+}
+
+// Migrate does necessary upgrade operations if needed.  It returns the new
+// configuration file body, and a boolean indicating whether the configuration
+// file was actually upgraded.
+func (m *Migrator) Migrate(body []byte) (newBody []byte, upgraded bool, err error) {
+	// read a config file into an interface map, so we can manipulate values without losing any
+	diskConf := yobj{}
+	err = yaml.Unmarshal(body, &diskConf)
+	if err != nil {
+		log.Printf("parsing config file for upgrade: %s", err)
+
+		return nil, false, err
+	}
+
+	schemaVersionVal, ok := diskConf["schema_version"]
+	log.Tracef("got schema version %v", schemaVersionVal)
+	if !ok {
+		// no schema version, set it to 0
+		schemaVersionVal = 0
+	}
+
+	schemaVersion, ok := schemaVersionVal.(int)
+	if !ok {
+		err = fmt.Errorf("configuration file contains non-integer schema_version, abort")
+		log.Println(err)
+
+		return nil, false, err
+	}
+
+	if schemaVersion == CurrentSchemaVersion {
+		// do nothing
+		return body, false, nil
+	}
+
+	err = m.upgradeConfigSchema(schemaVersion, diskConf)
+	if err != nil {
+		log.Printf("upgrading configuration file: %s", err)
+
+		return nil, false, err
+	}
+
+	buf := &bytes.Buffer{}
+	enc := yaml.NewEncoder(buf)
+	enc.SetIndent(2)
+
+	err = enc.Encode(diskConf)
+	if err != nil {
+		return nil, false, fmt.Errorf("generating new config: %w", err)
+	}
+
+	return buf.Bytes(), true, nil
+}
+
+// upgradeFunc is a function that upgrades a config and returns an error.
+type upgradeFunc = func(diskConf yobj) (err error)
+
+// Upgrade from oldVersion to newVersion
+func (m *Migrator) upgradeConfigSchema(oldVersion int, diskConf yobj) (err error) {
+	upgrades := []upgradeFunc{
+		m.upgradeSchema0to1,
+		m.upgradeSchema1to2,
+		upgradeSchema2to3,
+		upgradeSchema3to4,
+		upgradeSchema4to5,
+		upgradeSchema5to6,
+		upgradeSchema6to7,
+		upgradeSchema7to8,
+		upgradeSchema8to9,
+		upgradeSchema9to10,
+		upgradeSchema10to11,
+		upgradeSchema11to12,
+		upgradeSchema12to13,
+		upgradeSchema13to14,
+		upgradeSchema14to15,
+		upgradeSchema15to16,
+		upgradeSchema16to17,
+		upgradeSchema17to18,
+		upgradeSchema18to19,
+		upgradeSchema19to20,
+		upgradeSchema20to21,
+		upgradeSchema21to22,
+		upgradeSchema22to23,
+		upgradeSchema23to24,
+		upgradeSchema24to25,
+		upgradeSchema25to26,
+	}
+
+	n := 0
+	for i, u := range upgrades {
+		if i >= oldVersion {
+			err = u(diskConf)
+			if err != nil {
+				return err
+			}
+
+			n++
+		}
+	}
+
+	if n == 0 {
+		return fmt.Errorf("unknown configuration schema version %d", oldVersion)
+	}
+
+	return nil
+}

internal/confmigrate/upgrade.go

@@ -1,7 +1,6 @@
-package home
+package confmigrate
 
 import (
-	"bytes"
 	"fmt"
 	"net/netip"
 	"net/url"
@@ -17,133 +16,15 @@ import (
 	"github.com/AdguardTeam/golibs/log"
 	"github.com/AdguardTeam/golibs/netutil"
 	"github.com/AdguardTeam/golibs/timeutil"
-	"github.com/google/renameio/v2/maybe"
 	"golang.org/x/crypto/bcrypt"
-	yaml "gopkg.in/yaml.v3"
 )
 
-// currentSchemaVersion is the current schema version.
+// upgradeSchema0to1 deletes the unused dnsfilter.txt file, since the following
-const currentSchemaVersion = 26
+// versions store filters in data/filters/.
-
+func (m *Migrator) upgradeSchema0to1(diskConf yobj) (err error) {
-// These aliases are provided for convenience.
-type (
-	yarr = []any
-	yobj = map[string]any
-)
-
-// Performs necessary upgrade operations if needed
-func upgradeConfig() error {
-	// read a config file into an interface map, so we can manipulate values without losing any
-	diskConf := yobj{}
-	body, err := readConfigFile()
-	if err != nil {
-		return err
-	}
-
-	err = yaml.Unmarshal(body, &diskConf)
-	if err != nil {
-		log.Printf("parsing config file for upgrade: %s", err)
-
-		return err
-	}
-
-	schemaVersionInterface, ok := diskConf["schema_version"]
-	log.Tracef("got schema version %v", schemaVersionInterface)
-	if !ok {
-		// no schema version, set it to 0
-		schemaVersionInterface = 0
-	}
-
-	schemaVersion, ok := schemaVersionInterface.(int)
-	if !ok {
-		err = fmt.Errorf("configuration file contains non-integer schema_version, abort")
-		log.Println(err)
-		return err
-	}
-
-	if schemaVersion == currentSchemaVersion {
-		// do nothing
-		return nil
-	}
-
-	return upgradeConfigSchema(schemaVersion, diskConf)
-}
-
-// upgradeFunc is a function that upgrades a config and returns an error.
-type upgradeFunc = func(diskConf yobj) (err error)
-
-// Upgrade from oldVersion to newVersion
-func upgradeConfigSchema(oldVersion int, diskConf yobj) (err error) {
-	upgrades := []upgradeFunc{
-		upgradeSchema0to1,
-		upgradeSchema1to2,
-		upgradeSchema2to3,
-		upgradeSchema3to4,
-		upgradeSchema4to5,
-		upgradeSchema5to6,
-		upgradeSchema6to7,
-		upgradeSchema7to8,
-		upgradeSchema8to9,
-		upgradeSchema9to10,
-		upgradeSchema10to11,
-		upgradeSchema11to12,
-		upgradeSchema12to13,
-		upgradeSchema13to14,
-		upgradeSchema14to15,
-		upgradeSchema15to16,
-		upgradeSchema16to17,
-		upgradeSchema17to18,
-		upgradeSchema18to19,
-		upgradeSchema19to20,
-		upgradeSchema20to21,
-		upgradeSchema21to22,
-		upgradeSchema22to23,
-		upgradeSchema23to24,
-		upgradeSchema24to25,
-		upgradeSchema25to26,
-	}
-
-	n := 0
-	for i, u := range upgrades {
-		if i >= oldVersion {
-			err = u(diskConf)
-			if err != nil {
-				return err
-			}
-
-			n++
-		}
-	}
-
-	if n == 0 {
-		return fmt.Errorf("unknown configuration schema version %d", oldVersion)
-	}
-
-	buf := &bytes.Buffer{}
-	enc := yaml.NewEncoder(buf)
-	enc.SetIndent(2)
-
-	err = enc.Encode(diskConf)
-	if err != nil {
-		return fmt.Errorf("generating new config: %w", err)
-	}
-
-	config.fileData = buf.Bytes()
-	confFile := config.getConfigFilename()
-	err = maybe.WriteFile(confFile, config.fileData, 0o644)
-	if err != nil {
-		return fmt.Errorf("writing new config: %w", err)
-	}
-
-	return nil
-}
-
-// The first schema upgrade:
-// No more "dnsfilter.txt", filters are now kept in data/filters/
-func upgradeSchema0to1(diskConf yobj) (err error) {
 	log.Printf("%s(): called", funcName())
 
-	dnsFilterPath := filepath.Join(Context.workDir, "dnsfilter.txt")
+	dnsFilterPath := filepath.Join(m.workingDir, "dnsfilter.txt")
 	log.Printf("deleting %s as we don't need it anymore", dnsFilterPath)
 	err = os.Remove(dnsFilterPath)
 	if err != nil && !errors.Is(err, os.ErrNotExist) {
@@ -157,13 +38,20 @@ func upgradeSchema0to1(diskConf yobj) (err error) {
 	return nil
 }
 
-// Second schema upgrade:
+// upgradeSchema1to2 performs the following changes:
-// coredns is now dns in config
+//
-// delete 'Corefile', since we don't use that anymore
+//	# BEFORE:
-func upgradeSchema1to2(diskConf yobj) (err error) {
+//	'dns':
+//	  # …
+//
+//	# AFTER:
+//	# …
+//
+// It also deletes the Corefile file, since it isn't used anymore.
+func (m *Migrator) upgradeSchema1to2(diskConf yobj) (err error) {
 	log.Printf("%s(): called", funcName())
 
-	coreFilePath := filepath.Join(Context.workDir, "Corefile")
+	coreFilePath := filepath.Join(m.workingDir, "Corefile")
 	log.Printf("deleting %s as we don't need it anymore", coreFilePath)
 	err = os.Remove(coreFilePath)
 	if err != nil && !errors.Is(err, os.ErrNotExist) {
@@ -292,12 +180,12 @@ func upgradeSchema4to5(diskConf yobj) error {
 		log.Fatalf("Can't use password \"%s\": bcrypt.GenerateFromPassword: %s", passStr, err)
 		return nil
 	}
-	u := webUser{
+	u := yobj{
-		Name:         nameStr,
+		"name":     nameStr,
-		PasswordHash: string(hash),
+		"password": string(hash),
 	}
-	users := []webUser{u}
+	diskConf["users"] = yarr{u}
-	diskConf["users"] = users
+
 	return nil
 }
 
@@ -794,12 +682,12 @@ func upgradeSchema13to14(diskConf yobj) (err error) {
 
 	diskConf["clients"] = yobj{
 		"persistent": clientsVal,
-		"runtime_sources": &clientSourcesConfig{
+		"runtime_sources": yobj{
-			WHOIS:     true,
+			"whois": true,
-			ARP:       true,
+			"arp":   true,
-			RDNS:      rdnsSrc,
+			"rdns":  rdnsSrc,
-			DHCP:      true,
+			"dhcp":  true,
-			HostsFile: true,
+			"hosts": true,
 		},
 	}
 

internal/confmigrate/upgrade_test.go

@@ -1,4 +1,4 @@
-package home
+package confmigrate
 
 import (
 	"testing"
@@ -11,12 +11,16 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-// TODO(a.garipov): Cover all migrations, use a testdata/ dir.
+// TODO(e.burkov): Cover all migrations, use a testdata/ dir.
 
 func TestUpgradeSchema1to2(t *testing.T) {
 	diskConf := testDiskConf(1)
 
-	err := upgradeSchema1to2(diskConf)
+	m := New(&Config{
+		WorkingDir: "",
+	})
+
+	err := m.upgradeSchema1to2(diskConf)
 	require.NoError(t, err)
 
 	require.Equal(t, diskConf["schema_version"], 2)
@@ -651,10 +655,10 @@ func TestUpgradeSchema12to13(t *testing.T) {
 func TestUpgradeSchema13to14(t *testing.T) {
 	const newSchemaVer = 14
 
-	testClient := &clientObject{
+	testClient := yobj{
-		Name:              "agh-client",
+		"name":                "agh-client",
-		IDs:               []string{"id1"},
+		"ids":                 []string{"id1"},
-		UseGlobalSettings: true,
+		"use_global_settings": true,
 	}
 
 	testCases := []struct {
@@ -668,37 +672,37 @@ func TestUpgradeSchema13to14(t *testing.T) {
 			// The clients field will be added anyway.
 			"clients": yobj{
 				"persistent": yarr{},
-				"runtime_sources": &clientSourcesConfig{
+				"runtime_sources": yobj{
-					WHOIS:     true,
+					"whois": true,
-					ARP:       true,
+					"arp":   true,
-					RDNS:      false,
+					"rdns":  false,
-					DHCP:      true,
+					"dhcp":  true,
-					HostsFile: true,
+					"hosts": true,
 				},
 			},
 		},
 		name: "no_clients",
 	}, {
 		in: yobj{
-			"clients": []*clientObject{testClient},
+			"clients": yarr{testClient},
 		},
 		want: yobj{
 			"schema_version": newSchemaVer,
 			"clients": yobj{
-				"persistent": []*clientObject{testClient},
+				"persistent": yarr{testClient},
-				"runtime_sources": &clientSourcesConfig{
+				"runtime_sources": yobj{
-					WHOIS:     true,
+					"whois": true,
-					ARP:       true,
+					"arp":   true,
-					RDNS:      false,
+					"rdns":  false,
-					DHCP:      true,
+					"dhcp":  true,
-					HostsFile: true,
+					"hosts": true,
 				},
 			},
 		},
 		name: "no_dns",
 	}, {
 		in: yobj{
-			"clients": []*clientObject{testClient},
+			"clients": yarr{testClient},
 			"dns": yobj{
 				"resolve_clients": true,
 			},
@@ -706,13 +710,13 @@ func TestUpgradeSchema13to14(t *testing.T) {
 		want: yobj{
 			"schema_version": newSchemaVer,
 			"clients": yobj{
-				"persistent": []*clientObject{testClient},
+				"persistent": yarr{testClient},
-				"runtime_sources": &clientSourcesConfig{
+				"runtime_sources": yobj{
-					WHOIS:     true,
+					"whois": true,
-					ARP:       true,
+					"arp":   true,
-					RDNS:      true,
+					"rdns":  true,
-					DHCP:      true,
+					"dhcp":  true,
-					HostsFile: true,
+					"hosts": true,
 				},
 			},
 			"dns": yobj{},

internal/home/config.go

@@ -10,6 +10,7 @@ import (
 
 	"github.com/AdguardTeam/AdGuardHome/internal/aghalg"
 	"github.com/AdguardTeam/AdGuardHome/internal/aghtls"
+	"github.com/AdguardTeam/AdGuardHome/internal/confmigrate"
 	"github.com/AdguardTeam/AdGuardHome/internal/dhcpd"
 	"github.com/AdguardTeam/AdGuardHome/internal/dnsforward"
 	"github.com/AdguardTeam/AdGuardHome/internal/filtering"
@@ -415,7 +416,7 @@ var config = &configuration{
 		MaxAge:     3,
 	},
 	OSConfig:      &osConfig{},
-	SchemaVersion: currentSchemaVersion,
+	SchemaVersion: confmigrate.CurrentSchemaVersion,
 	Theme:         ThemeAuto,
 }
 
@@ -431,6 +432,7 @@ func (c *configuration) getConfigFilename() string {
 	if !filepath.IsAbs(configFile) {
 		configFile = filepath.Join(Context.workDir, configFile)
 	}
+
 	return configFile
 }
 
@@ -450,21 +452,56 @@ func validateBindHosts(conf *configuration) (err error) {
 	return nil
 }
 
-// parseConfig loads configuration from the YAML file
+// parseConfig loads configuration from the YAML file, upgrading it if
+// necessary.
 func parseConfig() (err error) {
-	var fileData []byte
+	// Do the upgrade if necessary.
-	fileData, err = readConfigFile()
+	config.fileData, err = readConfigFile()
 	if err != nil {
 		return err
 	}
 
-	config.fileData = nil
+	migrator := confmigrate.New(&confmigrate.Config{
-	err = yaml.Unmarshal(fileData, &config)
+		WorkingDir: Context.workDir,
+	})
+
+	var upgraded bool
+	config.fileData, upgraded, err = migrator.Migrate(config.fileData)
+	if err != nil {
+		// Don't wrap the error, because it's informative enough as is.
+		return err
+	} else if upgraded {
+		err = maybe.WriteFile(config.getConfigFilename(), config.fileData, 0o644)
+		if err != nil {
+			return fmt.Errorf("writing new config: %w", err)
+		}
+	}
+
+	err = yaml.Unmarshal(config.fileData, &config)
 	if err != nil {
 		// Don't wrap the error since it's informative enough as is.
 		return err
 	}
 
+	err = validateConfig()
+	if err != nil {
+		return err
+	}
+
+	if config.DNS.UpstreamTimeout.Duration == 0 {
+		config.DNS.UpstreamTimeout = timeutil.Duration{Duration: dnsforward.DefaultTimeout}
+	}
+
+	err = setContextTLSCipherIDs()
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
+// validateConfig returns error if the configuration is invalid.
+func validateConfig() (err error) {
 	err = validateBindHosts(config)
 	if err != nil {
 		// Don't wrap the error since it's informative enough as is.
@@ -500,15 +537,6 @@ func parseConfig() (err error) {
 		config.Filtering.FiltersUpdateIntervalHours = 24
 	}
 
-	if config.DNS.UpstreamTimeout.Duration == 0 {
-		config.DNS.UpstreamTimeout = timeutil.Duration{Duration: dnsforward.DefaultTimeout}
-	}
-
-	err = setContextTLSCipherIDs()
-	if err != nil {
-		return err
-	}
-
 	return nil
 }
 

internal/home/home.go

@@ -145,14 +145,8 @@ func setupContext(opts options) (err error) {
 		return nil
 	}
 
-	// Do the upgrade if necessary.
+	err = parseConfig()
-	err = upgradeConfig()
 	if err != nil {
-		// Don't wrap the error, because it's informative enough as is.
-		return err
-	}
-
-	if err = parseConfig(); err != nil {
 		log.Error("parsing configuration file: %s", err)
 
 		os.Exit(1)

scripts/make/go-lint.sh

@@ -177,6 +177,7 @@ run_linter gocyclo --over 10 .
 gocognit_paths="\
 ./internal/aghnet/   20
 ./internal/querylog/ 20
+./internal/confmigrate/ 19
 ./internal/dnsforward/  19
 ./internal/home/        19
 ./internal/aghtls/ 18
@@ -240,6 +241,7 @@ run_linter gosec --quiet\
 	./internal/aghrenameio/\
 	./internal/aghtest\
 	./internal/client\
+	./internal/confmigrate\
 	./internal/dhcpd\
 	./internal/dhcpsvc\
 	./internal/dnsforward\