Pull request 2210: AG-32341-client-duplicate-uids

Squashed commit of the following:

commit 6f83ec8a4f0dfb2360912116a48d35d1f7df7f66
Merge: 2fea9c06a 2383ab57c
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Fri Apr 26 19:23:15 2024 +0300

    Merge branch 'master' into AG-32341-client-duplicate-uids

commit 2fea9c06af9a0b349e612620292bc629f514fead
Merge: 672a30ce4 b9d5e5ba0
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Fri Apr 26 14:00:49 2024 +0300

    Merge branch 'master' into AG-32341-client-duplicate-uids

commit 672a30ce402f51819d741ea92ec5283eea4e6813
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Fri Apr 26 13:59:59 2024 +0300

    all: upd chlog

commit 7c2b26e3ec
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Wed Apr 24 21:08:47 2024 +0300

    all: imp chlog

commit fcca9afe0c
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Wed Apr 24 18:56:34 2024 +0300

    all: fix client duplicate uids

CHANGELOG.md

@@ -32,8 +32,18 @@ NOTE: Add new changes BELOW THIS COMMENT.
 - Private rDNS resolution now also affects `SOA` and `NS` requests ([#6882]).
 - Rewrite rules mechanics was changed due to improve resolving in safe search.
 
+### Deprecated
+
+- Currently, AdGuard Home skips persistent clients that have duplicate fields
+  when reading them from the configuration file.  This behaviour is deprecated
+  and will cause errors on startup in a future release.
+
 ### Fixed
 
+- Acceptance of duplicate UIDs for persistent clients at startup.  See also the
+  section on client settings on the [Wiki page][wiki-config].
+- Domain specifications for top-level domains not considered for requests to
+  unqualified domains ([#6744]).
 - Support for link-local subnets, i.e. `fe80::/16`, as client identifiers
   ([#6312]).
 - Issues with QUIC and HTTP/3 upstreams on older Linux kernel versions
@@ -55,6 +65,7 @@ NOTE: Add new changes BELOW THIS COMMENT.
 [#6192]: https://github.com/AdguardTeam/AdGuardHome/issues/6192
 [#6312]: https://github.com/AdguardTeam/AdGuardHome/issues/6312
 [#6422]: https://github.com/AdguardTeam/AdGuardHome/issues/6422
+[#6744]: https://github.com/AdguardTeam/AdGuardHome/issues/6744
 [#6854]: https://github.com/AdguardTeam/AdGuardHome/issues/6854
 [#6875]: https://github.com/AdguardTeam/AdGuardHome/issues/6875
 [#6882]: https://github.com/AdguardTeam/AdGuardHome/issues/6882

go.mod

@@ -3,8 +3,7 @@ module github.com/AdguardTeam/AdGuardHome
 go 1.22.2
 
 require (
-	// TODO(a.garipov): Use a tagged version once released.
-	github.com/AdguardTeam/dnsproxy v0.70.1-0.20240424112457-69feed2dd25e
+	github.com/AdguardTeam/dnsproxy v0.71.0
 	github.com/AdguardTeam/golibs v0.23.2
 	github.com/AdguardTeam/urlfilter v0.18.0
 	github.com/NYTimes/gziphandler v1.1.1
@@ -29,8 +28,8 @@ require (
 	// own code for that.  Perhaps, use gopacket.
 	github.com/mdlayher/raw v0.1.0
 	github.com/miekg/dns v1.1.58
-	// TODO(a.garipov): Use a tagged version once released.
-	github.com/quic-go/quic-go v0.42.1-0.20240424132812-713525777535
+	// TODO(a.garipov): Use release version.
+	github.com/quic-go/quic-go v0.42.1-0.20240424141022-12aa63824c7f
 	github.com/stretchr/testify v1.9.0
 	github.com/ti-mo/netfilter v0.5.1
 	go.etcd.io/bbolt v1.3.9

go.sum

@@ -1,5 +1,5 @@
-github.com/AdguardTeam/dnsproxy v0.70.1-0.20240424112457-69feed2dd25e h1:ju0wprmCakjAOIuvKrmLU+hUFiStIsreWVk4JWmQPm0=
-github.com/AdguardTeam/dnsproxy v0.70.1-0.20240424112457-69feed2dd25e/go.mod h1:eWyFj9zVMdJ4tjHULulfFIXiu6GID/aVvewLVVXLXWE=
+github.com/AdguardTeam/dnsproxy v0.71.0 h1:7epvSeTtjTdwbeifRJK6jTXldoJwqd3GWqAYDlSo7zQ=
+github.com/AdguardTeam/dnsproxy v0.71.0/go.mod h1:rCaCL4m4n63sgwTOyUVdc7MC42PlUYBt11Fz/UjD+kM=
 github.com/AdguardTeam/golibs v0.23.2 h1:rMjYantwtQ39e8G4zBQ6ZLlm4s3XH30Bc9VxhoOHwao=
 github.com/AdguardTeam/golibs v0.23.2/go.mod h1:o9i55Sx6v7qogRQeqaBfmLbC/pZqeMBWi015U5PTDY0=
 github.com/AdguardTeam/urlfilter v0.18.0 h1:ZZzwODC/ADpjJSODxySrrUnt/fvOCfGFaCW6j+wsGfQ=
@@ -101,8 +101,8 @@ github.com/power-devops/perfstat v0.0.0-20210106213030-5aafc221ea8c h1:ncq/mPwQF
 github.com/power-devops/perfstat v0.0.0-20210106213030-5aafc221ea8c/go.mod h1:OmDBASR4679mdNQnz2pUhc2G8CO2JrUAVFDRBDP/hJE=
 github.com/quic-go/qpack v0.4.0 h1:Cr9BXA1sQS2SmDUWjSofMPNKmvF6IiIfDRmgU0w1ZCo=
 github.com/quic-go/qpack v0.4.0/go.mod h1:UZVnYIfi5GRk+zI9UMaCPsmZ2xKJP7XBUvVyT1Knj9A=
-github.com/quic-go/quic-go v0.42.1-0.20240424132812-713525777535 h1:63/XLGwhqZUU0L4DPWihJH9tyqwmvfaxNPjU9I/Av/M=
-github.com/quic-go/quic-go v0.42.1-0.20240424132812-713525777535/go.mod h1:132kz4kL3F9vxhW3CtQJLDVwcFe5wdWeJXXijhsO57M=
+github.com/quic-go/quic-go v0.42.1-0.20240424141022-12aa63824c7f h1:L7x60Z6AW2giF/SvbDpMglGHJxtmFJV03khPwXLDScU=
+github.com/quic-go/quic-go v0.42.1-0.20240424141022-12aa63824c7f/go.mod h1:132kz4kL3F9vxhW3CtQJLDVwcFe5wdWeJXXijhsO57M=
 github.com/shirou/gopsutil/v3 v3.23.7 h1:C+fHO8hfIppoJ1WdsVm1RoI0RwXoNdfTK7yWXV0wVj4=
 github.com/shirou/gopsutil/v3 v3.23.7/go.mod h1:c4gnmoRC0hQuaLqvxnx1//VXQ0Ms/X9UnJF8pddY5z4=
 github.com/shoenig/go-m1cpu v0.1.6 h1:nxdKQNcEB6vzgA2E2bvzKIYRuNj7XNJ4S/aRSwKzFtM=

internal/client/index.go

@@ -6,6 +6,7 @@ import (
 	"net/netip"
 
 	"github.com/AdguardTeam/AdGuardHome/internal/aghalg"
+	"github.com/AdguardTeam/golibs/errors"
 )
 
 // macKey contains MAC as byte array of 6, 8, or 20 bytes.
@@ -82,15 +83,25 @@ func (ci *Index) Add(c *Persistent) {
 	ci.uidToClient[c.UID] = c
 }
 
+// ErrDuplicateUID is an error returned by [Index.Clashes] when adding a
+// persistent client with a UID that already exists in an index.
+const ErrDuplicateUID errors.Error = "duplicate uid"
+
 // Clashes returns an error if the index contains a different persistent client
 // with at least a single identifier contained by c.  c must be non-nil.
 func (ci *Index) Clashes(c *Persistent) (err error) {
+	_, ok := ci.uidToClient[c.UID]
+	if ok {
+		return ErrDuplicateUID
+	}
+
 	for _, id := range c.ClientIDs {
-		existing, ok := ci.clientIDToUID[id]
+		var existing UID
+		existing, ok = ci.clientIDToUID[id]
 		if ok && existing != c.UID {
 			p := ci.uidToClient[existing]
 
-			return fmt.Errorf("another client %q uses the same ID %q", p.Name, id)
+			return fmt.Errorf("another client %q uses the same ClientID %q", p.Name, id)
 		}
 	}
 

internal/dnsforward/stats.go

@@ -130,7 +130,7 @@ func (s *Server) logQuery(dctx *dnsContext, ip net.IP, processingTime time.Durat
 	s.queryLog.Add(p)
 }
 
-// updatesStats writes the request into statistics.
+// updateStats writes the request data into statistics.
 func (s *Server) updateStats(dctx *dnsContext, clientIP string, processingTime time.Duration) {
 	pctx := dctx.proxyCtx
 

internal/home/clients.go

@@ -286,7 +286,13 @@ func (clients *clientsContainer) addFromConfig(
 
 		_, err = clients.add(cli)
 		if err != nil {
-			log.Error("clients: adding client at index %d %s: %s", i, cli.Name, err)
+			if errors.Is(err, client.ErrDuplicateUID) {
+				return fmt.Errorf("clients: adding client %s at index %d: %w", cli.Name, i, err)
+			}
+
+			// TODO(s.chzhen):  Return an error instead of logging if more
+			// stringent requirements are implemented.
+			log.Error("clients: adding client %s at index %d: %s", cli.Name, i, err)
 		}
 	}
 

internal/home/service.go

@@ -306,7 +306,7 @@ func handleServiceStatusCommand(s service.Service) {
 	}
 }
 
-// handleServiceStatusCommand handles service "install" command
+// handleServiceInstallCommand handles service "install" command.
 func handleServiceInstallCommand(s service.Service) {
 	err := svcAction(s, "install")
 	if err != nil {
@@ -340,7 +340,7 @@ AdGuard Home is now available at the following addresses:`)
 	}
 }
 
-// handleServiceStatusCommand handles service "uninstall" command
+// handleServiceUninstallCommand handles service "uninstall" command.
 func handleServiceUninstallCommand(s service.Service) {
 	if aghos.IsOpenWrt() {
 		// On OpenWrt it is important to run disable command first