Pull request: 5191-update-flag

Merge in DNS/adguard-home from 5191-update-flag to master Updates #5191. Updates #4223. Squashed commit of the following: commit fbace49 Author: Eugene Burkov <E.Burkov@AdGuard.COM> Date: Mon Jan 9 12:05:16 2023 +0400 all: imp code, docs commit 8237dce Merge: ca9518f bbdcc67 Author: Eugene Burkov <E.Burkov@AdGuard.COM> Date: Fri Dec 30 14:45:55 2022 +0400 Merge branch 'master' into 5191-update-flag commit ca9518f Author: Eugene Burkov <E.Burkov@AdGuard.COM> Date: Thu Dec 29 20:36:33 2022 +0400 home: imp code commit 1dc6c7c Author: Eugene Burkov <E.Burkov@AdGuard.COM> Date: Thu Dec 29 18:26:08 2022 +0400 all: imp code, docs commit 7bbe893 Author: Eugene Burkov <E.Burkov@AdGuard.COM> Date: Thu Dec 29 03:44:48 2022 +0400 home: restart service on update commit e0d3c28 Author: Eugene Burkov <E.Burkov@AdGuard.COM> Date: Thu Dec 29 03:22:49 2022 +0400 all: update on first run commit 0aa4e78 Author: Eugene Burkov <E.Burkov@AdGuard.COM> Date: Thu Dec 29 02:47:30 2022 +0400 all: move some code to init less commit 68aebfa Author: Eugene Burkov <E.Burkov@AdGuard.COM> Date: Thu Dec 29 00:36:00 2022 +0400 WIP commit 2c7fb97 Author: Eugene Burkov <E.Burkov@AdGuard.COM> Date: Wed Dec 28 14:15:59 2022 +0400 home: imp logs commit 4b06d08 Author: Eugene Burkov <E.Burkov@AdGuard.COM> Date: Tue Dec 27 19:21:17 2022 +0400 all: fix update flag
AdguardTeam · Jan 9, 2023 · d8d7a5c · d8d7a5c
1 parent bbdcc67
commit d8d7a5c
Show file tree

Hide file tree

Showing 13 changed files with 213 additions and 93 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -42,6 +42,9 @@ See also the [v0.107.21 GitHub milestone][ms-v0.107.21].
 
 ### Fixed
 
+- `AdGuardHome --update` freezing when another instance of AdGuard Home is
+  running ([#4223], [#5191]).
+- The `--update` flag performing an update even with the same version.
 - Failing HTTPS redirection on saving the encryption settings ([#4898]).
 - Zeroing rules counter of erroneusly edited filtering rule lists ([#5290]).
 - Filters updating strategy, which could sometimes lead to use of broken or
@@ -50,6 +53,7 @@ See also the [v0.107.21 GitHub milestone][ms-v0.107.21].
   server to stop responding ([#5251]).
 
 [#4898]: https://github.com/AdguardTeam/AdGuardHome/issues/4898
+[#5191]: https://github.com/AdguardTeam/AdGuardHome/issues/5191
 [#5238]: https://github.com/AdguardTeam/AdGuardHome/issues/5238
 [#5251]: https://github.com/AdguardTeam/AdGuardHome/issues/5251
 [#5258]: https://github.com/AdguardTeam/AdGuardHome/issues/5258

diff --git a/internal/dnsforward/dnsforward.go b/internal/dnsforward/dnsforward.go
@@ -530,14 +530,14 @@ func validateBlockingMode(mode BlockingMode, blockingIPv4, blockingIPv6 net.IP)
 // prepareInternalProxy initializes the DNS proxy that is used for internal DNS
 // queries, such as public clients PTR resolving and updater hostname resolving.
 func (s *Server) prepareInternalProxy() (err error) {
+	srvConf := s.conf
 	conf := &proxy.Config{
 		CacheEnabled:   true,
 		CacheSizeBytes: 4096,
-		UpstreamConfig: s.conf.UpstreamConfig,
+		UpstreamConfig: srvConf.UpstreamConfig,
 		MaxGoroutines:  int(s.conf.MaxGoroutines),
 	}
 
-	srvConf := s.conf
 	setProxyUpstreamMode(
 		conf,
 		srvConf.AllServers,

diff --git a/internal/home/controlupdate.go b/internal/home/controlupdate.go
@@ -123,7 +123,7 @@ func handleUpdate(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	err = Context.updater.Update()
+	err = Context.updater.Update(false)
 	if err != nil {
 		aghhttp.Error(r, w, http.StatusInternalServerError, "%s", err)
 

diff --git a/internal/home/dns.go b/internal/home/dns.go
@@ -9,7 +9,9 @@ import (
 	"path/filepath"
 
 	"github.com/AdguardTeam/AdGuardHome/internal/aghalg"
+	"github.com/AdguardTeam/AdGuardHome/internal/aghhttp"
 	"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
+	"github.com/AdguardTeam/AdGuardHome/internal/dhcpd"
 	"github.com/AdguardTeam/AdGuardHome/internal/dnsforward"
 	"github.com/AdguardTeam/AdGuardHome/internal/filtering"
 	"github.com/AdguardTeam/AdGuardHome/internal/querylog"
@@ -39,17 +41,13 @@ func onConfigModified() {
 	}
 }
 
-// initDNSServer creates an instance of the dnsforward.Server
-// Please note that we must do it even if we don't start it
-// so that we had access to the query log and the stats
-func initDNSServer() (err error) {
+// initDNS updates all the fields of the [Context] needed to initialize the DNS
+// server and initializes it at last.  It also must not be called unless
+// [config] and [Context] are initialized.
+func initDNS() (err error) {
 	baseDir := Context.getDataDir()
 
-	var anonFunc aghnet.IPMutFunc
-	if config.DNS.AnonymizeClientIP {
-		anonFunc = querylog.AnonymizeIP
-	}
-	anonymizer := aghnet.NewIPMut(anonFunc)
+	anonymizer := config.anonymizer()
 
 	statsConf := stats.Config{
 		Filename:       filepath.Join(baseDir, "stats.db"),
@@ -82,34 +80,46 @@ func initDNSServer() (err error) {
 		return err
 	}
 
-	var privateNets netutil.SubnetSet
-	switch len(config.DNS.PrivateNets) {
-	case 0:
-		// Use an optimized locally-served matcher.
-		privateNets = netutil.SubnetSetFunc(netutil.IsLocallyServed)
-	case 1:
-		privateNets, err = netutil.ParseSubnet(config.DNS.PrivateNets[0])
-		if err != nil {
-			return fmt.Errorf("preparing the set of private subnets: %w", err)
-		}
-	default:
-		var nets []*net.IPNet
-		nets, err = netutil.ParseSubnets(config.DNS.PrivateNets...)
-		if err != nil {
-			return fmt.Errorf("preparing the set of private subnets: %w", err)
-		}
+	tlsConf := &tlsConfigSettings{}
+	Context.tls.WriteDiskConfig(tlsConf)
+
+	return initDNSServer(
+		Context.filters,
+		Context.stats,
+		Context.queryLog,
+		Context.dhcpServer,
+		anonymizer,
+		httpRegister,
+		tlsConf,
+	)
+}
 
-		privateNets = netutil.SliceSubnetSet(nets)
+// initDNSServer initializes the [context.dnsServer].  To only use the internal
+// proxy, none of the arguments are required, but tlsConf still must not be nil,
+// in other cases all the arguments also must not be nil.  It also must not be
+// called unless [config] and [Context] are initialized.
+func initDNSServer(
+	filters *filtering.DNSFilter,
+	sts stats.Interface,
+	qlog querylog.QueryLog,
+	dhcpSrv dhcpd.Interface,
+	anonymizer *aghnet.IPMut,
+	httpReg aghhttp.RegisterFunc,
+	tlsConf *tlsConfigSettings,
+) (err error) {
+	privateNets, err := parseSubnetSet(config.DNS.PrivateNets)
+	if err != nil {
+		return fmt.Errorf("preparing set of private subnets: %w", err)
 	}
 
 	p := dnsforward.DNSCreateParams{
-		DNSFilter:   Context.filters,
-		Stats:       Context.stats,
-		QueryLog:    Context.queryLog,
+		DNSFilter:   filters,
+		Stats:       sts,
+		QueryLog:    qlog,
 		PrivateNets: privateNets,
 		Anonymizer:  anonymizer,
 		LocalDomain: config.DHCP.LocalDomainName,
-		DHCPServer:  Context.dhcpServer,
+		DHCPServer:  dhcpSrv,
 	}
 
 	Context.dnsServer, err = dnsforward.NewServer(p)
@@ -120,15 +130,15 @@ func initDNSServer() (err error) {
 	}
 
 	Context.clients.dnsServer = Context.dnsServer
-	var dnsConfig dnsforward.ServerConfig
-	dnsConfig, err = generateServerConfig()
+
+	dnsConf, err := generateServerConfig(tlsConf, httpReg)
 	if err != nil {
 		closeDNSServer()
 
 		return fmt.Errorf("generateServerConfig: %w", err)
 	}
 
-	err = Context.dnsServer.Prepare(&dnsConfig)
+	err = Context.dnsServer.Prepare(&dnsConf)
 	if err != nil {
 		closeDNSServer()
 
@@ -146,6 +156,32 @@ func initDNSServer() (err error) {
 	return nil
 }
 
+// parseSubnetSet parses a slice of subnets.  If the slice is empty, it returns
+// a subnet set that matches all locally served networks, see
+// [netutil.IsLocallyServed].
+func parseSubnetSet(nets []string) (s netutil.SubnetSet, err error) {
+	switch len(nets) {
+	case 0:
+		// Use an optimized function-based matcher.
+		return netutil.SubnetSetFunc(netutil.IsLocallyServed), nil
+	case 1:
+		s, err = netutil.ParseSubnet(nets[0])
+		if err != nil {
+			return nil, err
+		}
+
+		return s, nil
+	default:
+		var nets []*net.IPNet
+		nets, err = netutil.ParseSubnets(config.DNS.PrivateNets...)
+		if err != nil {
+			return nil, err
+		}
+
+		return netutil.SliceSubnetSet(nets), nil
+	}
+}
+
 func isRunning() bool {
 	return Context.dnsServer != nil && Context.dnsServer.IsRunning()
 }
@@ -193,20 +229,21 @@ func ipsToUDPAddrs(ips []netip.Addr, port int) (udpAddrs []*net.UDPAddr) {
 	return udpAddrs
 }
 
-func generateServerConfig() (newConf dnsforward.ServerConfig, err error) {
+func generateServerConfig(
+	tlsConf *tlsConfigSettings,
+	httpReg aghhttp.RegisterFunc,
+) (newConf dnsforward.ServerConfig, err error) {
 	dnsConf := config.DNS
 	hosts := aghalg.CoalesceSlice(dnsConf.BindHosts, []netip.Addr{netutil.IPv4Localhost()})
 	newConf = dnsforward.ServerConfig{
 		UDPListenAddrs:  ipsToUDPAddrs(hosts, dnsConf.Port),
 		TCPListenAddrs:  ipsToTCPAddrs(hosts, dnsConf.Port),
 		FilteringConfig: dnsConf.FilteringConfig,
 		ConfigModified:  onConfigModified,
-		HTTPRegister:    httpRegister,
+		HTTPRegister:    httpReg,
 		OnDNSRequest:    onDNSRequest,
 	}
 
-	tlsConf := tlsConfigSettings{}
-	Context.tls.WriteDiskConfig(&tlsConf)
 	if tlsConf.Enabled {
 		newConf.TLSConfig = tlsConf.TLSConfig
 		newConf.TLSConfig.ServerName = tlsConf.ServerName
@@ -224,7 +261,7 @@ func generateServerConfig() (newConf dnsforward.ServerConfig, err error) {
 		}
 
 		if tlsConf.PortDNSCrypt != 0 {
-			newConf.DNSCryptConfig, err = newDNSCrypt(hosts, tlsConf)
+			newConf.DNSCryptConfig, err = newDNSCrypt(hosts, *tlsConf)
 			if err != nil {
 				// Don't wrap the error, because it's already
 				// wrapped by newDNSCrypt.
@@ -413,7 +450,11 @@ func startDNSServer() error {
 
 func reconfigureDNSServer() (err error) {
 	var newConf dnsforward.ServerConfig
-	newConf, err = generateServerConfig()
+
+	tlsConf := &tlsConfigSettings{}
+	Context.tls.WriteDiskConfig(tlsConf)
+
+	newConf, err = generateServerConfig(tlsConf, httpRegister)
 	if err != nil {
 		return fmt.Errorf("generating forwarding dns server config: %w", err)
 	}

diff --git a/internal/home/home.go b/internal/home/home.go
@@ -455,6 +455,10 @@ func run(opts options, clientBuildFS fs.FS) {
 	err = setupConfig(opts)
 	fatalOnError(err)
 
+	// TODO(e.burkov):  This could be made earlier, probably as the option's
+	// effect.
+	cmdlineUpdate(opts)
+
 	if !Context.firstRun {
 		// Save the updated config
 		err = config.write()
@@ -522,7 +526,7 @@ func run(opts options, clientBuildFS fs.FS) {
 	fatalOnError(err)
 
 	if !Context.firstRun {
-		err = initDNSServer()
+		err = initDNS()
 		fatalOnError(err)
 
 		Context.tls.start()
@@ -543,20 +547,24 @@ func run(opts options, clientBuildFS fs.FS) {
 		}
 	}
 
-	// TODO(a.garipov): This could be made much earlier and could be done on
-	// the first run as well, but to achieve this we need to bypass requests
-	// over dnsforward resolver.
-	cmdlineUpdate(opts)
-
 	Context.web.Start()
 
 	// wait indefinitely for other go-routines to complete their job
 	select {}
 }
 
+func (c *configuration) anonymizer() (ipmut *aghnet.IPMut) {
+	var anonFunc aghnet.IPMutFunc
+	if c.DNS.AnonymizeClientIP {
+		anonFunc = querylog.AnonymizeIP
+	}
+
+	return aghnet.NewIPMut(anonFunc)
+}
+
 // startMods initializes and starts the DNS server after installation.
-func startMods() error {
-	err := initDNSServer()
+func startMods() (err error) {
+	err = initDNS()
 	if err != nil {
 		return err
 	}
@@ -927,8 +935,8 @@ func getHTTPProxy(_ *http.Request) (*url.URL, error) {
 
 // jsonError is a generic JSON error response.
 //
-// TODO(a.garipov): Merge together with the implementations in .../dhcpd and
-// other packages after refactoring the web handler registering.
+// TODO(a.garipov): Merge together with the implementations in [dhcpd] and other
+// packages after refactoring the web handler registering.
 type jsonError struct {
 	// Message is the error message, an opaque string.
 	Message string `json:"message"`
@@ -940,30 +948,40 @@ func cmdlineUpdate(opts options) {
 		return
 	}
 
-	log.Info("starting update")
-
-	if Context.firstRun {
-		log.Info("update not allowed on first run")
+	// Initialize the DNS server to use the internal resolver which the updater
+	// needs to be able to resolve the update source hostname.
+	//
+	// TODO(e.burkov):  We could probably initialize the internal resolver
+	// separately.
+	err := initDNSServer(nil, nil, nil, nil, nil, nil, &tlsConfigSettings{})
+	fatalOnError(err)
 
-		os.Exit(0)
-	}
+	log.Info("cmdline update: performing update")
 
-	_, err := Context.updater.VersionInfo(true)
+	updater := Context.updater
+	info, err := updater.VersionInfo(true)
 	if err != nil {
-		vcu := Context.updater.VersionCheckURL()
+		vcu := updater.VersionCheckURL()
 		log.Error("getting version info from %s: %s", vcu, err)
 
-		os.Exit(0)
+		os.Exit(1)
 	}
 
-	if Context.updater.NewVersion() == "" {
+	if info.NewVersion == version.Version() {
 		log.Info("no updates available")
 
 		os.Exit(0)
 	}
 
-	err = Context.updater.Update()
+	err = updater.Update(Context.firstRun)
 	fatalOnError(err)
 
+	err = restartService()
+	if err != nil {
+		log.Debug("restarting service: %s", err)
+		log.Info("AdGuard Home was not installed as a service. " +
+			"Please restart running instances of AdGuardHome manually.")
+	}
+
 	os.Exit(0)
 }
diff --git a/internal/home/options.go b/internal/home/options.go
@@ -229,7 +229,7 @@ var cmdLineOpts = []cmdLineOpt{{
 	updateNoValue:   func(o options) (options, error) { o.performUpdate = true; return o, nil },
 	effect:          nil,
 	serialize:       func(o options) (val string, ok bool) { return "", o.performUpdate },
-	description:     "Update application and exit.",
+	description:     "Update the current binary and restart the service in case it's installed.",
 	longName:        "update",
 	shortName:       "",
 }, {