From 48d702f76ac2e651ce788abf006799efcd482152 Mon Sep 17 00:00:00 2001 From: Ainar Garipov Date: Tue, 13 Apr 2021 13:44:29 +0300 Subject: [PATCH] Pull request: all: fix client custom upstream comments Updates #2947. Squashed commit of the following: commit 498a05459b1aa00bcffee490acfeecb567025971 Merge: 6a7a2f87 21e2c419 Author: Ainar Garipov Date: Tue Apr 13 13:40:29 2021 +0300 Merge branch 'master' into 2947-cli-ups-comment commit 6a7a2f87cfd2bdd829b82889890511fef8d84b9b Author: Ainar Garipov Date: Tue Apr 13 13:34:28 2021 +0300 all: imp code, tests commit abc0be239f69cfc3e7d0cde2fc952d9157b2cd5d Merge: 82fb3fcb 6410feeb Author: Ainar Garipov Date: Tue Apr 13 13:17:09 2021 +0300 Merge branch 'master' into 2947-cli-ups-comment commit 82fb3fcb49cbc8d439cb5959c1cb84ae49b2257e Author: Ainar Garipov Date: Mon Apr 12 22:13:41 2021 +0300 all: fix client custom upstream comments --- CHANGELOG.md | 2 + internal/aghstrings/strings.go | 18 +++++++++ internal/aghstrings/strings_test.go | 15 +++++++ internal/dnsforward/config.go | 2 +- internal/dnsforward/dnsforward.go | 39 ++++++++----------- internal/dnsforward/http.go | 4 +- internal/dnsforward/http_test.go | 20 ++++++---- .../TestDNSForwardHTTTP_handleSetConfig.json | 4 +- internal/dnsforward/util.go | 15 ------- internal/home/clients.go | 5 ++- 10 files changed, 72 insertions(+), 52 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 61b55eef..9b9541d5 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/internal/aghstrings/strings.go b/internal/aghstrings/strings.go index f42dded6..c4993fd5 100644 --- a/internal/aghstrings/strings.go +++ b/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) { diff --git a/internal/aghstrings/strings_test.go b/internal/aghstrings/strings_test.go index 304e0164..3cb5723f 100644 --- a/internal/aghstrings/strings_test.go +++ b/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"} diff --git a/internal/dnsforward/config.go b/internal/dnsforward/config.go index edc6f4f1..d90200ed 100644 --- a/internal/dnsforward/config.go +++ b/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{ diff --git a/internal/dnsforward/dnsforward.go b/internal/dnsforward/dnsforward.go index 450897aa..8951f45d 100644 --- a/internal/dnsforward/dnsforward.go +++ b/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{ diff --git a/internal/dnsforward/http.go b/internal/dnsforward/http.go index 12dd170b..f83241fb 100644 --- a/internal/dnsforward/http.go +++ b/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 } diff --git a/internal/dnsforward/http_test.go b/internal/dnsforward/http_test.go index 1724ae97..78311da5 100644 --- a/internal/dnsforward/http_test.go +++ b/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: "", @@ -169,17 +170,20 @@ func TestDNSForwardHTTTP_handleSetConfig(t *testing.T) { name: "upstream_mode_fastest_addr", wantSet: "", }, { - name: "upstream_dns_bad", - wantSet: "wrong upstreams specification: missing port in address\n", + name: "upstream_dns_bad", + 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", + 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`, }, { 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) diff --git a/internal/dnsforward/testdata/TestDNSForwardHTTTP_handleSetConfig.json b/internal/dnsforward/testdata/TestDNSForwardHTTTP_handleSetConfig.json index f1771a19..e7f98c6e 100644 --- a/internal/dnsforward/testdata/TestDNSForwardHTTTP_handleSetConfig.json +++ b/internal/dnsforward/testdata/TestDNSForwardHTTTP_handleSetConfig.json @@ -324,7 +324,7 @@ "upstream_dns_bad": { "req": { "upstream_dns": [ - "" + "!!!" ] }, "want": { @@ -522,4 +522,4 @@ "local_ptr_upstreams": [] } } -} \ No newline at end of file +} diff --git a/internal/dnsforward/util.go b/internal/dnsforward/util.go index 871447eb..5fdde967 100644 --- a/internal/dnsforward/util.go +++ b/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 -} diff --git a/internal/home/clients.go b/internal/home/clients.go index 25bc6d8f..6d22bb19 100644 --- a/internal/home/clients.go +++ b/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,