Pull request: all: fix client custom upstream comments

Updates #2947.

Squashed commit of the following:

commit 498a05459b1aa00bcffee490acfeecb567025971
Merge: 6a7a2f87 21e2c419
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Tue Apr 13 13:40:29 2021 +0300

    Merge branch 'master' into 2947-cli-ups-comment

commit 6a7a2f87cfd2bdd829b82889890511fef8d84b9b
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Tue Apr 13 13:34:28 2021 +0300

    all: imp code, tests

commit abc0be239f69cfc3e7d0cde2fc952d9157b2cd5d
Merge: 82fb3fcb 6410feeb
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Tue Apr 13 13:17:09 2021 +0300

    Merge branch 'master' into 2947-cli-ups-comment

commit 82fb3fcb49cbc8d439cb5959c1cb84ae49b2257e
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Mon Apr 12 22:13:41 2021 +0300

    all: fix client custom upstream comments

CHANGELOG.md

@@ -45,6 +45,7 @@ and this project adheres to
 
 ### Fixed
 
+- Comment handling in clients' custom upstreams ([#2947]).
 - Overwriting of DHCPv4 options when using the HTTP API ([#2927]).
 - Assumption that MAC addresses always have the length of 6 octets ([#2828]).
 - Support for more than one `/24` subnet in DHCP ([#2541]).
@@ -71,6 +72,7 @@ and this project adheres to
 [#2838]: https://github.com/AdguardTeam/AdGuardHome/issues/2838
 [#2889]: https://github.com/AdguardTeam/AdGuardHome/issues/2889
 [#2927]: https://github.com/AdguardTeam/AdGuardHome/issues/2927
+[#2947]: https://github.com/AdguardTeam/AdGuardHome/issues/2947
 
 
 

internal/aghstrings/strings.go

@@ -19,6 +19,18 @@ func CloneSlice(a []string) (b []string) {
 	return CloneSliceOrEmpty(a)
 }
 
+// FilterOut returns a copy of strs with all strings for which f returned true
+// removed.
+func FilterOut(strs []string, f func(s string) (ok bool)) (filtered []string) {
+	for _, s := range strs {
+		if !f(s) {
+			filtered = append(filtered, s)
+		}
+	}
+
+	return filtered
+}
+
 // InSlice checks if string is in the slice of strings.
 func InSlice(strs []string, str string) (ok bool) {
 	for _, s := range strs {
@@ -30,6 +42,12 @@ func InSlice(strs []string, str string) (ok bool) {
 	return false
 }
 
+// IsCommentOrEmpty returns true of the string starts with a "#" character or is
+// an empty string.
+func IsCommentOrEmpty(s string) (ok bool) {
+	return len(s) == 0 || s[0] == '#'
+}
+
 // SplitNext splits string by a byte and returns the first chunk skipping empty
 // ones.  Whitespaces are trimmed.
 func SplitNext(s *string, sep rune) (chunk string) {

internal/aghstrings/strings_test.go

@@ -36,6 +36,21 @@ func TestCloneSlice_family(t *testing.T) {
 	})
 }
 
+func TestFilterOut(t *testing.T) {
+	strs := []string{
+		"1.2.3.4",
+		"",
+		"# 5.6.7.8",
+	}
+
+	want := []string{
+		"1.2.3.4",
+	}
+
+	got := FilterOut(strs, IsCommentOrEmpty)
+	assert.Equal(t, want, got)
+}
+
 func TestInSlice(t *testing.T) {
 	simpleStrs := []string{"1", "2", "3"}
 

internal/dnsforward/config.go

@@ -289,7 +289,7 @@ func (s *Server) prepareUpstreamSettings() error {
 		upstreams = s.conf.UpstreamDNS
 	}
 
-	upstreams = filterOutComments(upstreams)
+	upstreams = aghstrings.FilterOut(upstreams, aghstrings.IsCommentOrEmpty)
 	upstreamConfig, err := proxy.ParseUpstreamsConfig(
 		upstreams,
 		upstream.Options{

internal/dnsforward/dnsforward.go

@@ -295,8 +295,8 @@ func (s *Server) startLocked() error {
 // faster than ordinary upstreams.
 const defaultLocalTimeout = 1 * time.Second
 
-// collectDNSIPAddrs returns the slice of IP addresses without port number which
-// we are listening on.  For internal use only.
+// collectDNSIPAddrs returns IP addresses the server is listening on without
+// port numbers as a map.  For internal use only.
 func (s *Server) collectDNSIPAddrs() (addrs []string, err error) {
 	addrs = make([]string, len(s.conf.TCPListenAddrs)+len(s.conf.UDPListenAddrs))
 	var i int
@@ -329,28 +329,17 @@ func (s *Server) collectDNSIPAddrs() (addrs []string, err error) {
 	return addrs[:i], nil
 }
 
-// stringSetSubtract subtracts b from a interpreted as sets.
-func stringSetSubtract(a, b []string) (c []string) {
-	// unit is an object to be used as value in set.
-	type unit = struct{}
+// unit is used to show the presence of a value in a set.
+type unit = struct{}
 
-	cSet := make(map[string]unit)
-	for _, k := range a {
-		cSet[k] = unit{}
+// sliceToSet converts a slice of strings into a string set.
+func sliceToSet(strs []string) (set map[string]unit) {
+	set = make(map[string]unit, len(strs))
+	for _, s := range strs {
+		set[s] = unit{}
 	}
 
-	for _, k := range b {
-		delete(cSet, k)
-	}
-
-	c = make([]string, len(cSet))
-	i := 0
-	for k := range cSet {
-		c[i] = k
-		i++
-	}
-
-	return c
+	return set
 }
 
 // setupResolvers initializes the resolvers for local addresses.  For internal
@@ -377,11 +366,17 @@ func (s *Server) setupResolvers(localAddrs []string) (err error) {
 		return err
 	}
 
+	ourAddrsSet := sliceToSet(ourAddrs)
+
 	// TODO(e.burkov): The approach of subtracting sets of strings is not
 	// really applicable here since in case of listening on all network
 	// interfaces we should check the whole interface's network to cut off
 	// all the loopback addresses as well.
-	localAddrs = stringSetSubtract(localAddrs, ourAddrs)
+	localAddrs = aghstrings.FilterOut(localAddrs, func(s string) (ok bool) {
+		_, ok = ourAddrsSet[s]
+
+		return ok
+	})
 
 	var upsConfig proxy.UpstreamConfig
 	upsConfig, err = proxy.ParseUpstreamsConfig(localAddrs, upstream.Options{

internal/dnsforward/http.go

@@ -328,7 +328,7 @@ type upstreamJSON struct {
 // TODO(e.burkov): Move into aghnet or even into dnsproxy.
 func ValidateUpstreams(upstreams []string) (err error) {
 	// No need to validate comments
-	upstreams = filterOutComments(upstreams)
+	upstreams = aghstrings.FilterOut(upstreams, aghstrings.IsCommentOrEmpty)
 
 	// Consider this case valid because defaultDNS will be used
 	if len(upstreams) == 0 {
@@ -510,7 +510,7 @@ func checkPrivateUpstreamExc(u upstream.Upstream) (err error) {
 }
 
 func checkDNS(input string, bootstrap []string, ef excFunc) (err error) {
-	if !isUpstream(input) {
+	if aghstrings.IsCommentOrEmpty(input) {
 		return nil
 	}
 

internal/dnsforward/http_test.go

@@ -9,6 +9,7 @@ import (
 	"net/http/httptest"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/AdguardTeam/AdGuardHome/internal/dnsfilter"
@@ -149,7 +150,7 @@ func TestDNSForwardHTTTP_handleSetConfig(t *testing.T) {
 		wantSet: "",
 	}, {
 		name:    "blocking_mode_bad",
-		wantSet: "blocking_mode: incorrect value\n",
+		wantSet: "blocking_mode: incorrect value",
 	}, {
 		name:    "ratelimit",
 		wantSet: "",
@@ -170,16 +171,19 @@ func TestDNSForwardHTTTP_handleSetConfig(t *testing.T) {
 		wantSet: "",
 	}, {
 		name: "upstream_dns_bad",
-		wantSet: "wrong upstreams specification: missing port in address\n",
+		wantSet: `wrong upstreams specification: address !!!: ` +
+			`missing port in address`,
 	}, {
 		name: "bootstraps_bad",
-		wantSet: "a can not be used as bootstrap dns cause: invalid bootstrap server address: Resolver a is not eligible to be a bootstrap DNS server\n",
+		wantSet: `a can not be used as bootstrap dns cause: ` +
+			`invalid bootstrap server address: ` +
+			`Resolver a is not eligible to be a bootstrap DNS server`,
 	}, {
 		name:    "cache_bad_ttl",
-		wantSet: "cache_ttl_min must be less or equal than cache_ttl_max\n",
+		wantSet: `cache_ttl_min must be less or equal than cache_ttl_max`,
 	}, {
 		name:    "upstream_mode_bad",
-		wantSet: "upstream_mode: incorrect value\n",
+		wantSet: `upstream_mode: incorrect value`,
 	}, {
 		name:    "local_ptr_upstreams_good",
 		wantSet: "",
@@ -209,7 +213,7 @@ func TestDNSForwardHTTTP_handleSetConfig(t *testing.T) {
 			require.Nil(t, err)
 
 			s.handleSetConfig(w, r)
-			assert.Equal(t, tc.wantSet, w.Body.String())
+			assert.Equal(t, tc.wantSet, strings.TrimSuffix(w.Body.String(), "\n"))
 			w.Body.Reset()
 
 			s.handleGetConfig(w, nil)

internal/dnsforward/testdata/TestDNSForwardHTTTP_handleSetConfig.json

@@ -324,7 +324,7 @@
   "upstream_dns_bad": {
     "req": {
       "upstream_dns": [
-        ""
+        "!!!"
       ]
     },
     "want": {

internal/dnsforward/util.go

@@ -67,18 +67,3 @@ func matchDNSName(dnsNames []string, sni string) bool {
 	}
 	return false
 }
-
-// Is not comment
-func isUpstream(line string) bool {
-	return !strings.HasPrefix(line, "#")
-}
-
-func filterOutComments(lines []string) []string {
-	var filtered []string
-	for _, l := range lines {
-		if isUpstream(l) {
-			filtered = append(filtered, l)
-		}
-	}
-	return filtered
-}

internal/home/clients.go

@@ -349,13 +349,14 @@ func (clients *clientsContainer) FindUpstreams(ip string) *proxy.UpstreamConfig
 		return nil
 	}
 
-	if len(c.Upstreams) == 0 {
+	upstreams := aghstrings.FilterOut(c.Upstreams, aghstrings.IsCommentOrEmpty)
+	if len(upstreams) == 0 {
 		return nil
 	}
 
 	if c.upstreamConfig == nil {
 		conf, err := proxy.ParseUpstreamsConfig(
-			c.Upstreams,
+			upstreams,
 			upstream.Options{
 				Bootstrap: config.DNS.BootstrapDNS,
 				Timeout:   dnsforward.DefaultTimeout,