Pull request 2107: AG-28327-upstream-config-parser

Squashed commit of the following:

commit e496653b10de52676826ed8e0c461e91405603a8
Merge: db2cd04e9 60a978c9a
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Thu Feb 1 18:23:50 2024 +0300

    Merge branch 'master' into AG-28327-upstream-config-parser

commit db2cd04e981dd24998d87f4935ff6590ea7854cd
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Wed Jan 31 16:21:53 2024 +0300

    all: upd proxy

commit e8878179b6d094321d56fb2b75c16c1ba8cf637d
Merge: ccbbae6d6 aa872dfe9
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Wed Jan 31 16:17:34 2024 +0300

    Merge branch 'master' into AG-28327-upstream-config-parser

commit ccbbae6d615e110d7d2d4c2a6b35954311153bcf
Merge: d947d900e 8936c95ec
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Mon Jan 29 18:31:17 2024 +0300

    Merge branch 'master' into AG-28327-upstream-config-parser

commit d947d900e1f759159bc9068589ffe852483cfdd0
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Mon Jan 29 18:26:01 2024 +0300

    dnsforward: imp docs

commit cf9678c098951e2a4bebae7a3a5808d7de4c14c6
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Thu Jan 25 14:18:04 2024 +0300

    dnsforward: imp code

commit 22792a9311cb93b2bb3b804293f87f091b9b81e2
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Wed Jan 24 13:59:28 2024 +0300

    dnsforward: imp code

commit 57ddaaaaaf1009c65f0d9d6b2b1671211f194c85
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Mon Jan 22 20:19:44 2024 +0300

    all: add tests

commit d6732d13adae4ee46410252a33d092e67da3c34a
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Mon Jan 22 18:44:57 2024 +0300

    all: imp errors

commit e14456571ce2ef43fb217f45445729ce6299daf6
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Thu Jan 18 19:05:31 2024 +0300

    dnsforward: imp code

commit a5c106eae902fbc0a169ef9e4d7bf968f1e40bec
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Mon Jan 15 18:36:30 2024 +0300

    dnsforward: imp logs

commit 333b8561aa21d778007f808fb8e931ef3e95d721
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Thu Dec 21 15:06:42 2023 +0300

    all: imp tests

commit 5b19d6b039755577e03ffcc03952724a36f21aa4
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Fri Dec 15 14:21:58 2023 +0300

    all: imp code

commit 15fbd229de336425bde107a4f32175b8af41d876
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Wed Dec 13 14:49:40 2023 +0300

    all: upstream config parser

client/src/__locales/en.json

@@ -236,6 +236,7 @@
     "updated_upstream_dns_toast": "Upstream servers successfully saved",
     "dns_test_ok_toast": "Specified DNS servers are working correctly",
     "dns_test_not_ok_toast": "Server \"{{key}}\": could not be used, please check that you've written it correctly",
+    "dns_test_parsing_error_toast": "Section {{section}}: line {{line}}: could not be used, please check that you've written it correctly",
     "dns_test_warning_toast": "Upstream \"{{key}}\" does not respond to test requests and may not work properly",
     "unblock": "Unblock",
     "block": "Block",

client/src/actions/index.js

@@ -403,6 +403,11 @@ export const testUpstream = (
                 const message = upstreamResponse[key];
                 if (message.startsWith('WARNING:')) {
                     dispatch(addErrorToast({ error: i18next.t('dns_test_warning_toast', { key }) }));
+                } else if (message.endsWith(': parsing error')) {
+                    const info = message.substring(0, message.indexOf(':'));
+                    const [sectionKey, line] = info.split(' ');
+                    const section = i18next.t(sectionKey);
+                    dispatch(addErrorToast({ error: i18next.t('dns_test_parsing_error_toast', { section, line }) }));
                 } else if (message !== 'OK') {
                     dispatch(addErrorToast({ error: i18next.t('dns_test_not_ok_toast', { key }) }));
                 }

go.mod

@@ -3,7 +3,7 @@ module github.com/AdguardTeam/AdGuardHome
 go 1.20
 
 require (
-	github.com/AdguardTeam/dnsproxy v0.63.1
+	github.com/AdguardTeam/dnsproxy v0.64.1
 	github.com/AdguardTeam/golibs v0.19.0
 	github.com/AdguardTeam/urlfilter v0.17.3
 	github.com/NYTimes/gziphandler v1.1.1

go.sum

@@ -1,5 +1,5 @@
-github.com/AdguardTeam/dnsproxy v0.63.1 h1:CilxSuLYcuYpbPCGB7w41UUqWRMu3dvj4c9TvkIrpBg=
+github.com/AdguardTeam/dnsproxy v0.64.1 h1:Cv2nyNYjUeUxouTQmM0aVTR7LWuhCr/Lu+h3DIAWhG8=
-github.com/AdguardTeam/dnsproxy v0.63.1/go.mod h1:dRRAFOjrq4QYM92jGs4lt4BoY0Dm3EY3HkaleoM2Feo=
+github.com/AdguardTeam/dnsproxy v0.64.1/go.mod h1:dRRAFOjrq4QYM92jGs4lt4BoY0Dm3EY3HkaleoM2Feo=
 github.com/AdguardTeam/golibs v0.19.0 h1:y/x+Xn3pDg1ZfQ+QEZapPJqaeVYUIMp/EODMtVhn7PM=
 github.com/AdguardTeam/golibs v0.19.0/go.mod h1:3WunclLLfrVAq7fYQRhd6f168FHOEMssnipVXCxDL/w=
 github.com/AdguardTeam/urlfilter v0.17.3 h1:fg/ObbnO0Cv6aw0tW6N/ETDMhhNvmcUUOZ7HlmKC3rw=

internal/dnsforward/configvalidator.go

@@ -2,54 +2,56 @@ package dnsforward
 
 import (
 	"fmt"
-	"strings"
 	"sync"
 
+	"github.com/AdguardTeam/dnsproxy/proxy"
 	"github.com/AdguardTeam/dnsproxy/upstream"
 	"github.com/AdguardTeam/golibs/errors"
 	"github.com/AdguardTeam/golibs/log"
 	"github.com/miekg/dns"
-	"golang.org/x/exp/slices"
 )
 
-// upstreamConfigValidator parses the [*proxy.UpstreamConfig] and checks the
+// upstreamConfigValidator parses each section of an upstream configuration into
-// actual DNS availability of each upstream.
+// a corresponding [*proxy.UpstreamConfig] and checks the actual DNS
+// availability of each upstream.
 type upstreamConfigValidator struct {
-	// general is the general upstream configuration.
+	// generalUpstreamResults contains upstream results of a general section.
-	general []*upstreamResult
+	generalUpstreamResults map[string]*upstreamResult
 
-	// fallback is the fallback upstream configuration.
+	// fallbackUpstreamResults contains upstream results of a fallback section.
-	fallback []*upstreamResult
+	fallbackUpstreamResults map[string]*upstreamResult
 
-	// private is the private upstream configuration.
+	// privateUpstreamResults contains upstream results of a private section.
-	private []*upstreamResult
+	privateUpstreamResults map[string]*upstreamResult
+
+	// generalParseResults contains parsing results of a general section.
+	generalParseResults []*parseResult
+
+	// fallbackParseResults contains parsing results of a fallback section.
+	fallbackParseResults []*parseResult
+
+	// privateParseResults contains parsing results of a private section.
+	privateParseResults []*parseResult
 }
 
-// upstreamResult is a result of validation of an [upstream.Upstream] within an
+// upstreamResult is a result of parsing of an [upstream.Upstream] within an
 // [proxy.UpstreamConfig].
 type upstreamResult struct {
-	// server is the parsed upstream.  It is nil when there was an error during
+	// server is the parsed upstream.
-	// parsing.
 	server upstream.Upstream
 
-	// err is the error either from parsing or from checking the upstream.
+	// err is the upstream check error.
 	err error
 
-	// original is the piece of configuration that have either been turned to an
-	// upstream or caused an error.
-	original string
-
 	// isSpecific is true if the upstream is domain-specific.
 	isSpecific bool
 }
 
-// compare compares two [upstreamResult]s.  It returns 0 if they are equal, -1
+// parseResult contains a original piece of upstream configuration and a
-// if ur should be sorted before other, and 1 otherwise.
+// corresponding error.
-//
+type parseResult struct {
-// TODO(e.burkov):  Perhaps it makes sense to sort the results with errors near
+	err      *proxy.ParseError
-// the end.
+	original string
-func (ur *upstreamResult) compare(other *upstreamResult) (res int) {
-	return strings.Compare(ur.original, other.original)
 }
 
 // newUpstreamConfigValidator parses the upstream configuration and returns a
@@ -61,97 +63,99 @@ func newUpstreamConfigValidator(
 	private []string,
 	opts *upstream.Options,
 ) (cv *upstreamConfigValidator) {
-	cv = &upstreamConfigValidator{}
+	cv = &upstreamConfigValidator{
+		generalUpstreamResults:  map[string]*upstreamResult{},
+		fallbackUpstreamResults: map[string]*upstreamResult{},
+		privateUpstreamResults:  map[string]*upstreamResult{},
+	}
 
-	for _, line := range general {
+	conf, err := proxy.ParseUpstreamsConfig(general, opts)
-		cv.general = cv.insertLineResults(cv.general, line, opts)
+	cv.generalParseResults = collectErrResults(general, err)
-	}
+	insertConfResults(conf, cv.generalUpstreamResults)
-	for _, line := range fallback {
+
-		cv.fallback = cv.insertLineResults(cv.fallback, line, opts)
+	conf, err = proxy.ParseUpstreamsConfig(fallback, opts)
-	}
+	cv.fallbackParseResults = collectErrResults(fallback, err)
-	for _, line := range private {
+	insertConfResults(conf, cv.fallbackUpstreamResults)
-		cv.private = cv.insertLineResults(cv.private, line, opts)
+
-	}
+	conf, err = proxy.ParseUpstreamsConfig(private, opts)
+	cv.privateParseResults = collectErrResults(private, err)
+	insertConfResults(conf, cv.privateUpstreamResults)
 
 	return cv
 }
 
-// insertLineResults parses line and inserts the result into s.  It can insert
+// collectErrResults parses err and returns parsing results containing the
-// multiple results as well as none.
+// original upstream configuration line and the corresponding error.  err can be
-func (cv *upstreamConfigValidator) insertLineResults(
+// nil.
-	s []*upstreamResult,
+func collectErrResults(lines []string, err error) (results []*parseResult) {
-	line string,
+	if err == nil {
-	opts *upstream.Options,
+		return nil
-) (result []*upstreamResult) {
-	upstreams, isSpecific, err := splitUpstreamLine(line)
-	if err != nil {
-		return cv.insert(s, &upstreamResult{
-			err:      err,
-			original: line,
-		})
 	}
 
-	for _, upstreamAddr := range upstreams {
+	// limit is a maximum length for upstream configuration lines.
-		var res *upstreamResult
+	const limit = 80
-		if upstreamAddr != "#" {
+
-			res = cv.parseUpstream(upstreamAddr, opts)
+	wrapper, ok := err.(errors.WrapperSlice)
-		} else if !isSpecific {
+	if !ok {
-			res = &upstreamResult{
+		log.Debug("dnsforward: configvalidator: unwrapping: %s", err)
-				err:      errNotDomainSpecific,
+
-				original: upstreamAddr,
+		return nil
-			}
+	}
-		} else {
+
+	errs := wrapper.Unwrap()
+	results = make([]*parseResult, 0, len(errs))
+	for i, e := range errs {
+		var parseErr *proxy.ParseError
+		if !errors.As(e, &parseErr) {
+			log.Debug("dnsforward: configvalidator: inserting unexpected error %d: %s", i, err)
+
 			continue
 		}
 
-		res.isSpecific = isSpecific
+		idx := parseErr.Idx
-		s = cv.insert(s, res)
+		line := []rune(lines[idx])
-	}
+		if len(line) > limit {
-
+			line = line[:limit]
-	return s
+			line[limit-1] = '…'
-}
-
-// insert inserts r into slice in a sorted order, except duplicates.  slice must
-// not be nil.
-func (cv *upstreamConfigValidator) insert(
-	s []*upstreamResult,
-	r *upstreamResult,
-) (result []*upstreamResult) {
-	i, has := slices.BinarySearchFunc(s, r, (*upstreamResult).compare)
-	if has {
-		log.Debug("dnsforward: duplicate configuration %q", r.original)
-
-		return s
-	}
-
-	return slices.Insert(s, i, r)
-}
-
-// parseUpstream parses addr and returns the result of parsing.  It returns nil
-// if the specified server points at the default upstream server which is
-// validated separately.
-func (cv *upstreamConfigValidator) parseUpstream(
-	addr string,
-	opts *upstream.Options,
-) (r *upstreamResult) {
-	// Check if the upstream has a valid protocol prefix.
-	//
-	// TODO(e.burkov):  Validate the domain name.
-	if proto, _, ok := strings.Cut(addr, "://"); ok {
-		if !slices.Contains(protocols, proto) {
-			return &upstreamResult{
-				err:      fmt.Errorf("bad protocol %q", proto),
-				original: addr,
-			}
 		}
+
+		results = append(results, &parseResult{
+			original: string(line),
+			err:      parseErr,
+		})
 	}
 
-	ups, err := upstream.AddressToUpstream(addr, opts)
+	return results
+}
 
-	return &upstreamResult{
+// insertConfResults parses conf and inserts the upstream result into results.
-		server:   ups,
+// It can insert multiple results as well as none.
-		err:      err,
+func insertConfResults(conf *proxy.UpstreamConfig, results map[string]*upstreamResult) {
-		original: addr,
+	insertListResults(conf.Upstreams, results, false)
+
+	for _, ups := range conf.DomainReservedUpstreams {
+		insertListResults(ups, results, true)
+	}
+
+	for _, ups := range conf.SpecifiedDomainUpstreams {
+		insertListResults(ups, results, true)
+	}
+}
+
+// insertListResults constructs upstream results from the upstream list and
+// inserts them into results.  It can insert multiple results as well as none.
+func insertListResults(ups []upstream.Upstream, results map[string]*upstreamResult, specific bool) {
+	for _, u := range ups {
+		addr := u.Address()
+		_, ok := results[addr]
+		if ok {
+			continue
+		}
+
+		results[addr] = &upstreamResult{
+			server:     u,
+			isSpecific: specific,
+		}
 	}
 }
 
@@ -187,35 +191,30 @@ func (cv *upstreamConfigValidator) check() {
 	}
 
 	wg := &sync.WaitGroup{}
-	wg.Add(len(cv.general) + len(cv.fallback) + len(cv.private))
+	wg.Add(len(cv.generalUpstreamResults) +
+		len(cv.fallbackUpstreamResults) +
+		len(cv.privateUpstreamResults))
 
-	for _, res := range cv.general {
+	for _, res := range cv.generalUpstreamResults {
-		go cv.checkSrv(res, wg, commonChecker)
+		go checkSrv(res, wg, commonChecker)
 	}
-	for _, res := range cv.fallback {
+	for _, res := range cv.fallbackUpstreamResults {
-		go cv.checkSrv(res, wg, commonChecker)
+		go checkSrv(res, wg, commonChecker)
 	}
-	for _, res := range cv.private {
+	for _, res := range cv.privateUpstreamResults {
-		go cv.checkSrv(res, wg, arpaChecker)
+		go checkSrv(res, wg, arpaChecker)
 	}
 
 	wg.Wait()
 }
 
 // checkSrv runs hc on the server from res, if any, and stores any occurred
-// error in res.  wg is always marked done in the end.  It used to be called in
+// error in res.  wg is always marked done in the end.  It is intended to be
-// a separate goroutine.
+// used as a goroutine.
-func (cv *upstreamConfigValidator) checkSrv(
+func checkSrv(res *upstreamResult, wg *sync.WaitGroup, hc *healthchecker) {
-	res *upstreamResult,
+	defer log.OnPanic(fmt.Sprintf("dnsforward: checking upstream %s", res.server.Address()))
-	wg *sync.WaitGroup,
-	hc *healthchecker,
-) {
 	defer wg.Done()
 
-	if res.server == nil {
-		return
-	}
-
 	res.err = hc.check(res.server)
 	if res.err != nil && res.isSpecific {
 		res.err = domainSpecificTestError{Err: res.err}
@@ -225,65 +224,126 @@ func (cv *upstreamConfigValidator) checkSrv(
 // close closes all the upstreams that were successfully parsed.  It enriches
 // the results with deferred closing errors.
 func (cv *upstreamConfigValidator) close() {
-	for _, slice := range [][]*upstreamResult{cv.general, cv.fallback, cv.private} {
+	all := []map[string]*upstreamResult{
-		for _, r := range slice {
+		cv.generalUpstreamResults,
-			if r.server != nil {
+		cv.fallbackUpstreamResults,
-				r.err = errors.WithDeferred(r.err, r.server.Close())
+		cv.privateUpstreamResults,
-			}
+	}
+
+	for _, m := range all {
+		for _, r := range m {
+			r.err = errors.WithDeferred(r.err, r.server.Close())
 		}
 	}
 }
 
+// sections of the upstream configuration according to the text label of the
+// localization.
+//
+// Keep in sync with client/src/__locales/en.json.
+//
+// TODO(s.chzhen):  Refactor.
+const (
+	generalTextLabel  = "upstream_dns"
+	fallbackTextLabel = "fallback_dns_title"
+	privateTextLabel  = "local_ptr_title"
+)
+
 // status returns all the data collected during parsing, healthcheck, and
 // closing of the upstreams.  The returned map is keyed by the original upstream
 // configuration piece and contains the corresponding error or "OK" if there was
 // no error.
 func (cv *upstreamConfigValidator) status() (results map[string]string) {
-	result := map[string]string{}
+	// Names of the upstream configuration sections for logging.
+	const (
+		generalSection  = "general"
+		fallbackSection = "fallback"
+		privateSection  = "private"
+	)
 
-	for _, res := range cv.general {
+	results = map[string]string{}
-		resultToStatus("general", res, result)
+
+	for original, res := range cv.generalUpstreamResults {
+		upstreamResultToStatus(generalSection, string(original), res, results)
 	}
-	for _, res := range cv.fallback {
+	for original, res := range cv.fallbackUpstreamResults {
-		resultToStatus("fallback", res, result)
+		upstreamResultToStatus(fallbackSection, string(original), res, results)
 	}
-	for _, res := range cv.private {
+	for original, res := range cv.privateUpstreamResults {
-		resultToStatus("private", res, result)
+		upstreamResultToStatus(privateSection, string(original), res, results)
 	}
 
-	return result
+	parseResultToStatus(generalTextLabel, generalSection, cv.generalParseResults, results)
+	parseResultToStatus(fallbackTextLabel, fallbackSection, cv.fallbackParseResults, results)
+	parseResultToStatus(privateTextLabel, privateSection, cv.privateParseResults, results)
+
+	return results
 }
 
-// resultToStatus puts "OK" or an error message from res into resMap.  section
+// upstreamResultToStatus puts "OK" or an error message from res into resMap.
-// is the name of the upstream configuration section, i.e. "general",
+// section is the name of the upstream configuration section, i.e. "general",
 // "fallback", or "private", and only used for logging.
 //
 // TODO(e.burkov):  Currently, the HTTP handler expects that all the results are
 // put together in a single map, which may lead to collisions, see AG-27539.
 // Improve the results compilation.
-func resultToStatus(section string, res *upstreamResult, resMap map[string]string) {
+func upstreamResultToStatus(
+	section string,
+	original string,
+	res *upstreamResult,
+	resMap map[string]string,
+) {
 	val := "OK"
 	if res.err != nil {
 		val = res.err.Error()
 	}
 
-	prevVal := resMap[res.original]
+	prevVal := resMap[original]
 	switch prevVal {
 	case "":
-		resMap[res.original] = val
+		resMap[original] = val
 	case val:
-		log.Debug("dnsforward: duplicating %s config line %q", section, res.original)
+		log.Debug("dnsforward: duplicating %s config line %q", section, original)
 	default:
 		log.Debug(
 			"dnsforward: warning: %s config line %q (%v) had different result %v",
 			section,
 			val,
-			res.original,
+			original,
 			prevVal,
 		)
 	}
 }
 
+// parseResultToStatus puts parsing error messages from results into resMap.
+// section is the name of the upstream configuration section, i.e. "general",
+// "fallback", or "private", and only used for logging.
+//
+// Parsing error message has the following format:
+//
+//	sectionTextLabel line: parsing error
+//
+// Where sectionTextLabel is a section text label of a localization and line is
+// a line number.
+func parseResultToStatus(
+	textLabel string,
+	section string,
+	results []*parseResult,
+	resMap map[string]string,
+) {
+	for _, res := range results {
+		original := res.original
+		_, ok := resMap[original]
+		if ok {
+			log.Debug("dnsforward: duplicating %s parsing error %q", section, original)
+
+			continue
+		}
+
+		resMap[original] = fmt.Sprintf("%s %d: parsing error", textLabel, res.err.Idx+1)
+	}
+}
+
 // domainSpecificTestError is a wrapper for errors returned by checkDNS to mark
 // the tested upstream domain-specific and therefore consider its errors
 // non-critical.
@@ -342,7 +402,7 @@ func (h *healthchecker) check(u upstream.Upstream) (err error) {
 	if err != nil {
 		return fmt.Errorf("couldn't communicate with upstream: %w", err)
 	} else if h.ansEmpty && len(reply.Answer) > 0 {
-		return errWrongResponse
+		return errors.Error("wrong response")
 	}
 
 	return nil

internal/dnsforward/http.go

@@ -10,6 +10,7 @@ import (
 
 	"github.com/AdguardTeam/AdGuardHome/internal/aghhttp"
 	"github.com/AdguardTeam/AdGuardHome/internal/filtering"
+	"github.com/AdguardTeam/dnsproxy/proxy"
 	"github.com/AdguardTeam/dnsproxy/upstream"
 	"github.com/AdguardTeam/golibs/errors"
 	"github.com/AdguardTeam/golibs/log"
@@ -294,7 +295,7 @@ func (req *jsonDNSConfig) checkFallbacks() (err error) {
 		return nil
 	}
 
-	err = ValidateUpstreams(*req.Fallbacks)
+	_, err = proxy.ParseUpstreamsConfig(*req.Fallbacks, &upstream.Options{})
 	if err != nil {
 		return fmt.Errorf("fallback servers: %w", err)
 	}
@@ -344,7 +345,7 @@ func (req *jsonDNSConfig) validate(privateNets netutil.SubnetSet) (err error) {
 // validateUpstreamDNSServers returns an error if any field of req is invalid.
 func (req *jsonDNSConfig) validateUpstreamDNSServers(privateNets netutil.SubnetSet) (err error) {
 	if req.Upstreams != nil {
-		err = ValidateUpstreams(*req.Upstreams)
+		_, err = proxy.ParseUpstreamsConfig(*req.Upstreams, &upstream.Options{})
 		if err != nil {
 			return fmt.Errorf("upstream servers: %w", err)
 		}
@@ -580,9 +581,6 @@ func (s *Server) handleTestUpstreamDNS(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	req.Upstreams = stringutil.FilterOut(req.Upstreams, IsCommentOrEmpty)
-	req.FallbackDNS = stringutil.FilterOut(req.FallbackDNS, IsCommentOrEmpty)
-	req.PrivateUpstreams = stringutil.FilterOut(req.PrivateUpstreams, IsCommentOrEmpty)
 	req.BootstrapDNS = stringutil.FilterOut(req.BootstrapDNS, IsCommentOrEmpty)
 
 	opts := &upstream.Options{

internal/dnsforward/http_test.go

@@ -223,8 +223,9 @@ func TestDNSForwardHTTP_handleSetConfig(t *testing.T) {
 		wantSet: "",
 	}, {
 		name: "upstream_dns_bad",
-		wantSet: `validating dns config: ` +
+		wantSet: `validating dns config: upstream servers: parsing error at index 0: ` +
-			`upstream servers: validating upstream "!!!": not an ip:port`,
+			`cannot prepare the upstream: invalid address !!!: bad hostname "!!!": ` +
+			`bad top-level domain name label "!!!": bad top-level domain name label rune '!'`,
 	}, {
 		name: "bootstraps_bad",
 		wantSet: `validating dns config: checking bootstrap a: not a bootstrap: ParseAddr("a"): ` +
@@ -313,98 +314,6 @@ func TestIsCommentOrEmpty(t *testing.T) {
 	}
 }
 
-func TestValidateUpstreams(t *testing.T) {
-	const sdnsStamp = `sdns://AQMAAAAAAAAAFDE3Ni4xMDMuMTMwLjEzMDo1NDQzINErR_J` +
-		`S3PLCu_iZEIbq95zkSV2LFsigxDIuUso_OQhzIjIuZG5zY3J5cHQuZGVmYXVsdC5uczE` +
-		`uYWRndWFyZC5jb20`
-
-	testCases := []struct {
-		name    string
-		wantErr string
-		set     []string
-	}{{
-		name:    "empty",
-		wantErr: ``,
-		set:     nil,
-	}, {
-		name:    "comment",
-		wantErr: ``,
-		set:     []string{"# comment"},
-	}, {
-		name:    "no_default",
-		wantErr: `no default upstreams specified`,
-		set: []string{
-			"[/host.com/]1.1.1.1",
-			"[//]tls://1.1.1.1",
-			"[/www.host.com/]#",
-			"[/host.com/google.com/]8.8.8.8",
-			"[/host/]" + sdnsStamp,
-		},
-	}, {
-		name:    "with_default",
-		wantErr: ``,
-		set: []string{
-			"[/host.com/]1.1.1.1",
-			"[//]tls://1.1.1.1",
-			"[/www.host.com/]#",
-			"[/host.com/google.com/]8.8.8.8",
-			"[/host/]" + sdnsStamp,
-			"8.8.8.8",
-		},
-	}, {
-		name:    "invalid",
-		wantErr: `validating upstream "dhcp://fake.dns": bad protocol "dhcp"`,
-		set:     []string{"dhcp://fake.dns"},
-	}, {
-		name:    "invalid",
-		wantErr: `validating upstream "1.2.3.4.5": not an ip:port`,
-		set:     []string{"1.2.3.4.5"},
-	}, {
-		name:    "invalid",
-		wantErr: `validating upstream "123.3.7m": not an ip:port`,
-		set:     []string{"123.3.7m"},
-	}, {
-		name: "invalid",
-		wantErr: `splitting upstream line "[/host.com]tls://dns.adguard.com": ` +
-			`missing separator`,
-		set: []string{"[/host.com]tls://dns.adguard.com"},
-	}, {
-		name:    "invalid",
-		wantErr: `validating upstream "[host.ru]#": not an ip:port`,
-		set:     []string{"[host.ru]#"},
-	}, {
-		name:    "valid_default",
-		wantErr: ``,
-		set: []string{
-			"1.1.1.1",
-			"tls://1.1.1.1",
-			"https://dns.adguard.com/dns-query",
-			sdnsStamp,
-			"udp://dns.google",
-			"udp://8.8.8.8",
-			"[/host.com/]1.1.1.1",
-			"[//]tls://1.1.1.1",
-			"[/www.host.com/]#",
-			"[/host.com/google.com/]8.8.8.8",
-			"[/host/]" + sdnsStamp,
-			"[/пример.рф/]8.8.8.8",
-		},
-	}, {
-		name: "bad_domain",
-		wantErr: `splitting upstream line "[/!/]8.8.8.8": domain at index 0: ` +
-			`bad domain name "!": bad top-level domain name label "!": ` +
-			`bad top-level domain name label rune '!'`,
-		set: []string{"[/!/]8.8.8.8"},
-	}}
-
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			err := ValidateUpstreams(tc.set)
-			testutil.AssertErrorMsg(t, tc.wantErr, err)
-		})
-	}
-}
-
 func TestValidateUpstreamsPrivate(t *testing.T) {
 	ss := netutil.SubnetSetFunc(netutil.IsLocallyServed)
 

internal/dnsforward/upstreams.go

@@ -2,10 +2,8 @@ package dnsforward
 
 import (
 	"fmt"
-	"net"
 	"net/netip"
 	"os"
-	"strings"
 	"time"
 
 	"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
@@ -19,28 +17,6 @@ import (
 	"golang.org/x/exp/slices"
 )
 
-const (
-	// errNotDomainSpecific is returned when the upstream should be
-	// domain-specific, but isn't.
-	errNotDomainSpecific errors.Error = "not a domain-specific upstream"
-
-	// errMissingSeparator is returned when the domain-specific part of the
-	// upstream configuration line isn't closed.
-	errMissingSeparator errors.Error = "missing separator"
-
-	// errDupSeparator is returned when the domain-specific part of the upstream
-	// configuration line contains more than one ending separator.
-	errDupSeparator errors.Error = "duplicated separator"
-
-	// errNoDefaultUpstreams is returned when there are no default upstreams
-	// specified in the upstream configuration.
-	errNoDefaultUpstreams errors.Error = "no default upstreams specified"
-
-	// errWrongResponse is returned when the checked upstream replies in an
-	// unexpected way.
-	errWrongResponse errors.Error = "wrong response"
-)
-
 // loadUpstreams parses upstream DNS servers from the configured file or from
 // the configuration itself.
 func (s *Server) loadUpstreams() (upstreams []string, err error) {
@@ -199,84 +175,12 @@ func IsCommentOrEmpty(s string) (ok bool) {
 	return len(s) == 0 || s[0] == '#'
 }
 
-// newUpstreamConfig validates upstreams and returns an appropriate upstream
-// configuration or nil if it can't be built.
-//
-// TODO(e.burkov):  Perhaps proxy.ParseUpstreamsConfig should validate upstreams
-// slice already so that this function may be considered useless.
-func newUpstreamConfig(upstreams []string) (conf *proxy.UpstreamConfig, err error) {
-	// No need to validate comments and empty lines.
-	upstreams = stringutil.FilterOut(upstreams, IsCommentOrEmpty)
-	if len(upstreams) == 0 {
-		// Consider this case valid since it means the default server should be
-		// used.
-		return nil, nil
-	}
-
-	err = validateUpstreamConfig(upstreams)
-	if err != nil {
-		// Don't wrap the error since it's informative enough as is.
-		return nil, err
-	}
-
-	conf, err = proxy.ParseUpstreamsConfig(
-		upstreams,
-		&upstream.Options{
-			Bootstrap: net.DefaultResolver,
-			Timeout:   DefaultTimeout,
-		},
-	)
-	if err != nil {
-		// Don't wrap the error since it's informative enough as is.
-		return nil, err
-	} else if len(conf.Upstreams) == 0 {
-		return nil, errNoDefaultUpstreams
-	}
-
-	return conf, nil
-}
-
-// validateUpstreamConfig validates each upstream from the upstream
-// configuration and returns an error if any upstream is invalid.
-//
-// TODO(e.burkov):  Merge with [upstreamConfigValidator] somehow.
-func validateUpstreamConfig(conf []string) (err error) {
-	for _, u := range conf {
-		var ups []string
-		var isSpecific bool
-		ups, isSpecific, err = splitUpstreamLine(u)
-		if err != nil {
-			// Don't wrap the error since it's informative enough as is.
-			return err
-		}
-
-		for _, addr := range ups {
-			_, err = validateUpstream(addr, isSpecific)
-			if err != nil {
-				return fmt.Errorf("validating upstream %q: %w", addr, err)
-			}
-		}
-	}
-
-	return nil
-}
-
-// ValidateUpstreams validates each upstream and returns an error if any
-// upstream is invalid or if there are no default upstreams specified.
-//
-// TODO(e.burkov):  Merge with [upstreamConfigValidator] somehow.
-func ValidateUpstreams(upstreams []string) (err error) {
-	_, err = newUpstreamConfig(upstreams)
-
-	return err
-}
-
 // ValidateUpstreamsPrivate validates each upstream and returns an error if any
 // upstream is invalid or if there are no default upstreams specified.  It also
 // checks each domain of domain-specific upstreams for being ARPA pointing to
 // a locally-served network.  privateNets must not be nil.
 func ValidateUpstreamsPrivate(upstreams []string, privateNets netutil.SubnetSet) (err error) {
-	conf, err := newUpstreamConfig(upstreams)
+	conf, err := proxy.ParseUpstreamsConfig(upstreams, &upstream.Options{})
 	if err != nil {
 		return fmt.Errorf("creating config: %w", err)
 	}
@@ -308,66 +212,3 @@ func ValidateUpstreamsPrivate(upstreams []string, privateNets netutil.SubnetSet)
 
 	return errors.Annotate(errors.Join(errs...), "checking domain-specific upstreams: %w")
 }
-
-// protocols are the supported URL schemes for upstreams.
-var protocols = []string{"h3", "https", "quic", "sdns", "tcp", "tls", "udp"}
-
-// validateUpstream returns an error if u alongside with domains is not a valid
-// upstream configuration.  useDefault is true if the upstream is
-// domain-specific and is configured to point at the default upstream server
-// which is validated separately.  The upstream is considered domain-specific
-// only if domains is at least not nil.
-func validateUpstream(u string, isSpecific bool) (useDefault bool, err error) {
-	// The special server address '#' means that default server must be used.
-	if useDefault = u == "#" && isSpecific; useDefault {
-		return useDefault, nil
-	}
-
-	// Check if the upstream has a valid protocol prefix.
-	//
-	// TODO(e.burkov):  Validate the domain name.
-	if proto, _, ok := strings.Cut(u, "://"); ok {
-		if !slices.Contains(protocols, proto) {
-			return false, fmt.Errorf("bad protocol %q", proto)
-		}
-	} else if _, err = netip.ParseAddr(u); err == nil {
-		return false, nil
-	} else if _, err = netip.ParseAddrPort(u); err == nil {
-		return false, nil
-	}
-
-	return false, err
-}
-
-// splitUpstreamLine returns the upstreams and the specified domains.  domains
-// is nil when the upstream is not domains-specific.  Otherwise it may also be
-// empty.
-func splitUpstreamLine(upstreamStr string) (upstreams []string, isSpecific bool, err error) {
-	if !strings.HasPrefix(upstreamStr, "[/") {
-		return []string{upstreamStr}, false, nil
-	}
-
-	defer func() { err = errors.Annotate(err, "splitting upstream line %q: %w", upstreamStr) }()
-
-	doms, ups, found := strings.Cut(upstreamStr[2:], "/]")
-	if !found {
-		return nil, false, errMissingSeparator
-	} else if strings.Contains(ups, "/]") {
-		return nil, false, errDupSeparator
-	}
-
-	for i, host := range strings.Split(doms, "/") {
-		if host == "" {
-			continue
-		}
-
-		err = netutil.ValidateDomainName(strings.TrimPrefix(host, "*."))
-		if err != nil {
-			return nil, false, fmt.Errorf("domain at index %d: %w", i, err)
-		}
-
-		isSpecific = true
-	}
-
-	return strings.Fields(ups), isSpecific, nil
-}

internal/dnsforward/upstreams_internal_test.go

@@ -100,8 +100,7 @@ func TestUpstreamConfigValidator(t *testing.T) {
 		name:    "bad_specification",
 		general: []string{"[/domain.example/]/]1.2.3.4"},
 		want: map[string]string{
-			"[/domain.example/]/]1.2.3.4": `splitting upstream line ` +
+			"[/domain.example/]/]1.2.3.4": generalTextLabel + " 1: parsing error",
-				`"[/domain.example/]/]1.2.3.4": duplicated separator`,
 		},
 	}, {
 		name:     "all_different",
@@ -120,23 +119,9 @@ func TestUpstreamConfigValidator(t *testing.T) {
 		fallback: []string{"[/example/" + goodUps},
 		private:  []string{"[/example//bad.123/]" + goodUps},
 		want: map[string]string{
-			`[/example/]/]` + goodUps: `splitting upstream line ` +
+			"[/example/]/]" + goodUps:        generalTextLabel + " 1: parsing error",
-				`"[/example/]/]` + goodUps + `": duplicated separator`,
+			"[/example/" + goodUps:           fallbackTextLabel + " 1: parsing error",
-			`[/example/` + goodUps: `splitting upstream line ` +
+			"[/example//bad.123/]" + goodUps: privateTextLabel + " 1: parsing error",
-				`"[/example/` + goodUps + `": missing separator`,
-			`[/example//bad.123/]` + goodUps: `splitting upstream line ` +
-				`"[/example//bad.123/]` + goodUps + `": domain at index 2: ` +
-				`bad domain name "bad.123": ` +
-				`bad top-level domain name label "123": all octets are numeric`,
-		},
-	}, {
-		name: "non-specific_default",
-		general: []string{
-			"#",
-			"[/example/]#",
-		},
-		want: map[string]string{
-			"#": "not a domain-specific upstream",
 		},
 	}, {
 		name: "bad_proto",
@@ -144,7 +129,15 @@ func TestUpstreamConfigValidator(t *testing.T) {
 			"bad://1.2.3.4",
 		},
 		want: map[string]string{
-			"bad://1.2.3.4": `bad protocol "bad"`,
+			"bad://1.2.3.4": generalTextLabel + " 1: parsing error",
+		},
+	}, {
+		name: "truncated_line",
+		general: []string{
+			"This is a very long line.  It will cause a parsing error and will be truncated here.",
+		},
+		want: map[string]string{
+			"This is a very long line.  It will cause a parsing error and will be truncated …": "upstream_dns 1: parsing error",
 		},
 	}}
 

internal/home/clients.go

@@ -613,7 +613,7 @@ func (clients *clientsContainer) check(c *persistentClient) (err error) {
 	// TODO(s.chzhen):  Move to the constructor.
 	slices.Sort(c.Tags)
 
-	err = dnsforward.ValidateUpstreams(c.Upstreams)
+	_, err = proxy.ParseUpstreamsConfig(c.Upstreams, &upstream.Options{})
 	if err != nil {
 		return fmt.Errorf("invalid upstream servers: %w", err)
 	}