Pull request: 5044 Close upstreams

Merge in DNS/adguard-home from 5044-close-upstreams to master

Closes #5044.

Squashed commit of the following:

commit e121380ecb32bd2664d47f0968c68509156404c1
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Wed Oct 19 15:54:17 2022 +0300

    all: upd proxy again

commit ce7fa539a7430a1a197fd45e7988697010c684db
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Wed Oct 19 14:30:46 2022 +0300

    home: imp docs, names

commit 851c5b8128149941cc469e6192ec9b4b1f92b0b5
Merge: b9ee5d63 d2a09e49
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Wed Oct 19 14:21:44 2022 +0300

    Merge branch 'master' into 5044-close-upstreams

commit b9ee5d6348e696ff0b44dabee601469c545c8bd9
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Wed Oct 19 14:20:15 2022 +0300

    all: close upstreams more

commit eaca476319dc64e7986e26e67110005938cf1278
Merge: f924bc7a 8dba4ecd
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Oct 18 18:33:53 2022 +0300

    Merge branch 'master' into 5044-close-upstreams

commit f924bc7a836001f8bb7463de2b5ddaf1be1a53c1
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Oct 18 18:23:54 2022 +0300

    all: imp code, docs

commit 011fde16aa912fc78e3d6f60375cee73a0d88709
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Oct 18 17:26:40 2022 +0300

    all: upd dnsproxy

internal/aghtest/interface.go

@@ -162,6 +162,7 @@ var _ upstream.Upstream = (*UpstreamMock)(nil)
 type UpstreamMock struct {
 	OnAddress  func() (addr string)
 	OnExchange func(req *dns.Msg) (resp *dns.Msg, err error)
+	OnClose    func() (err error)
 }
 
 // Address implements the [upstream.Upstream] interface for *UpstreamMock.
@@ -173,3 +174,8 @@ func (u *UpstreamMock) Address() (addr string) {
 func (u *UpstreamMock) Exchange(req *dns.Msg) (resp *dns.Msg, err error) {
 	return u.OnExchange(req)
 }
+
+// Close implements the [upstream.Upstream] interface for *UpstreamMock.
+func (u *UpstreamMock) Close() (err error) {
+	return u.OnClose()
+}

internal/aghtest/upstream.go

@@ -8,6 +8,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/AdguardTeam/dnsproxy/upstream"
 	"github.com/AdguardTeam/golibs/errors"
 	"github.com/miekg/dns"
 	"github.com/stretchr/testify/require"
@@ -31,6 +32,8 @@ type Upstream struct {
 	Addr string
 }
 
+var _ upstream.Upstream = (*Upstream)(nil)
+
 // RespondTo returns a response with answer if req has class cl, question type
 // qt, and target targ.
 func RespondTo(t testing.TB, req *dns.Msg, cl, qt uint16, targ, answer string) (resp *dns.Msg) {
@@ -68,7 +71,7 @@ func RespondTo(t testing.TB, req *dns.Msg, cl, qt uint16, targ, answer string) (
 	return resp
 }
 
-// Exchange implements the upstream.Upstream interface for *Upstream.
+// Exchange implements the [upstream.Upstream] interface for *Upstream.
 //
 // TODO(a.garipov): Split further into handlers.
 func (u *Upstream) Exchange(m *dns.Msg) (resp *dns.Msg, err error) {
@@ -114,11 +117,16 @@ func (u *Upstream) Exchange(m *dns.Msg) (resp *dns.Msg, err error) {
 	return resp, nil
 }
 
-// Address implements upstream.Upstream interface for *Upstream.
+// Address implements [upstream.Upstream] interface for *Upstream.
 func (u *Upstream) Address() string {
 	return u.Addr
 }
 
+// Close implements [upstream.Upstream] interface for *Upstream.
+func (u *Upstream) Close() (err error) {
+	return nil
+}
+
 // NewBlockUpstream returns an [*UpstreamMock] that works like an upstream that
 // supports hash-based safe-browsing/adult-blocking feature.  If shouldBlock is
 // true, hostname's actual hash is returned, blocking it.  Otherwise, it returns

internal/dnsforward/dnsforward.go

@@ -518,7 +518,7 @@ func validateBlockingMode(mode BlockingMode, blockingIPv4, blockingIPv6 net.IP)
 }
 
 // prepareInternalProxy initializes the DNS proxy that is used for internal DNS
-// queries, such at client PTR resolving and updater hostname resolving.
+// queries, such as public clients PTR resolving and updater hostname resolving.
 func (s *Server) prepareInternalProxy() (err error) {
 	conf := &proxy.Config{
 		CacheEnabled:   true,
@@ -558,16 +558,37 @@ func (s *Server) Stop() error {
 	return s.stopLocked()
 }
 
-// stopLocked stops the DNS server without locking. For internal use only.
-func (s *Server) stopLocked() error {
+// stopLocked stops the DNS server without locking.  For internal use only.
+func (s *Server) stopLocked() (err error) {
+	var errs []error
+
 	if s.dnsProxy != nil {
-		err := s.dnsProxy.Stop()
+		err = s.dnsProxy.Stop()
 		if err != nil {
-			return fmt.Errorf("could not stop the DNS server properly: %w", err)
+			errs = append(errs, fmt.Errorf("could not stop primary resolvers properly: %w", err))
 		}
 	}
 
-	s.isRunning = false
+	if s.internalProxy != nil && s.internalProxy.UpstreamConfig != nil {
+		err = s.internalProxy.UpstreamConfig.Close()
+		if err != nil {
+			errs = append(errs, fmt.Errorf("could not stop internal resolvers properly: %w", err))
+		}
+	}
+
+	if s.localResolvers != nil && s.localResolvers.UpstreamConfig != nil {
+		err = s.localResolvers.UpstreamConfig.Close()
+		if err != nil {
+			errs = append(errs, fmt.Errorf("could not stop local resolvers properly: %w", err))
+		}
+	}
+
+	if len(errs) > 0 {
+		return errors.List("stopping DNS server", errs...)
+	} else {
+		s.isRunning = false
+	}
+
 	return nil
 }
 

internal/dnsforward/http.go

@@ -603,6 +603,7 @@ func checkDNS(
 	if err != nil {
 		return fmt.Errorf("failed to choose upstream for %q: %w", upstreamAddr, err)
 	}
+	defer func() { err = errors.WithDeferred(err, u.Close()) }()
 
 	if err = healthCheck(u); err != nil {
 		err = fmt.Errorf("upstream %q fails to exchange: %w", upstreamAddr, err)

internal/home/clients.go

@@ -21,6 +21,8 @@ import (
 	"github.com/AdguardTeam/golibs/log"
 	"github.com/AdguardTeam/golibs/netutil"
 	"github.com/AdguardTeam/golibs/stringutil"
+	"golang.org/x/exp/maps"
+	"golang.org/x/exp/slices"
 )
 
 const clientsUpdatePeriod = 10 * time.Minute
@@ -50,6 +52,18 @@ type Client struct {
 	UseOwnBlockedServices bool
 }
 
+// closeUpstreams closes the client-specific upstream config of c if any.
+func (c *Client) closeUpstreams() (err error) {
+	if c.upstreamConfig != nil {
+		err = c.upstreamConfig.Close()
+		if err != nil {
+			return fmt.Errorf("closing upstreams of client %q: %w", c.Name, err)
+		}
+	}
+
+	return nil
+}
+
 type clientSource uint
 
 // Client sources.  The order determines the priority.
@@ -651,6 +665,10 @@ func (clients *clientsContainer) Del(name string) (ok bool) {
 		return false
 	}
 
+	if err := c.closeUpstreams(); err != nil {
+		log.Error("client container: removing client %s: %s", name, err)
+	}
+
 	// update Name index
 	delete(clients.list, name)
 
@@ -709,7 +727,7 @@ func (clients *clientsContainer) Update(name string, c *Client) (err error) {
 			}
 		}
 
-		// update ID index
+		//  Update ID index.
 		for _, id := range prev.IDs {
 			delete(clients.idIndex, id)
 		}
@@ -718,14 +736,17 @@ func (clients *clientsContainer) Update(name string, c *Client) (err error) {
 		}
 	}
 
-	// update Name index
+	// Update name index.
 	if prev.Name != c.Name {
 		delete(clients.list, prev.Name)
 		clients.list[c.Name] = prev
 	}
 
-	// update upstreams cache
-	c.upstreamConfig = nil
+	// Update upstreams cache.
+	err = c.closeUpstreams()
+	if err != nil {
+		return err
+	}
 
 	*prev = *c
 
@@ -910,3 +931,24 @@ func (clients *clientsContainer) updateFromDHCP(add bool) {
 
 	log.Debug("clients: added %d client aliases from dhcp", n)
 }
+
+// Close gracefully closes all the client-specific upstream configurations of
+// the persistent clients.
+func (clients *clientsContainer) Close() (err error) {
+	persistent := maps.Values(clients.list)
+	slices.SortFunc(persistent, func(a, b *Client) (less bool) { return a.Name < b.Name })
+
+	var errs []error
+
+	for _, cli := range persistent {
+		if err = cli.closeUpstreams(); err != nil {
+			errs = append(errs, err)
+		}
+	}
+
+	if len(errs) > 0 {
+		return errors.List("closing client specific upstreams", errs...)
+	}
+
+	return nil
+}

internal/home/clientshttp.go

@@ -179,6 +179,7 @@ func (clients *clientsContainer) handleDelClient(w http.ResponseWriter, r *http.
 
 	if !clients.Del(cj.Name) {
 		aghhttp.Error(r, w, http.StatusBadRequest, "Client not found")
+
 		return
 	}
 

internal/home/dns.go

@@ -431,17 +431,23 @@ func reconfigureDNSServer() (err error) {
 	return nil
 }
 
-func stopDNSServer() error {
+func stopDNSServer() (err error) {
 	if !isRunning() {
 		return nil
 	}
 
-	err := Context.dnsServer.Stop()
+	err = Context.dnsServer.Stop()
 	if err != nil {
-		return fmt.Errorf("couldn't stop forwarding DNS server: %w", err)
+		return fmt.Errorf("stopping forwarding dns server: %w", err)
+	}
+
+	err = Context.clients.Close()
+	if err != nil {
+		return fmt.Errorf("closing clients container: %w", err)
 	}
 
 	closeDNSServer()
+
 	return nil
 }
 

internal/home/home.go

@@ -122,7 +122,6 @@ func Main(clientBuildFS fs.FS) {
 			case syscall.SIGHUP:
 				Context.clients.Reload()
 				Context.tls.reload()
-
 			default:
 				cleanup(context.Background())
 				cleanupAlways()

internal/next/dnssvc/dnssvc_test.go

@@ -9,6 +9,7 @@ import (
 	"github.com/AdguardTeam/AdGuardHome/internal/aghtest"
 	"github.com/AdguardTeam/AdGuardHome/internal/next/dnssvc"
 	"github.com/AdguardTeam/dnsproxy/upstream"
+	"github.com/AdguardTeam/golibs/errors"
 	"github.com/miekg/dns"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -25,6 +26,8 @@ func TestService(t *testing.T) {
 	const (
 		bootstrapAddr = "bootstrap.example"
 		upstreamAddr  = "upstream.example"
+
+		closeErr errors.Error = "closing failed"
 	)
 
 	ups := &aghtest.UpstreamMock{
@@ -36,6 +39,9 @@ func TestService(t *testing.T) {
 
 			return resp, nil
 		},
+		OnClose: func() (err error) {
+			return closeErr
+		},
 	}
 
 	c := &dnssvc.Config{
@@ -85,5 +91,5 @@ func TestService(t *testing.T) {
 	defer cancel()
 
 	err = svc.Shutdown(ctx)
-	require.NoError(t, err)
+	require.ErrorIs(t, err, closeErr)
 }