all: add todo

CHANGELOG.md

@@ -25,8 +25,8 @@ NOTE: Add new changes BELOW THIS COMMENT.
 
 ### Added
 
-- Ability to specify multiple domain specific upstreams per line (e.g.
-  `[/domain1/../domain2/]upstream1 upstream2 .. upstreamN`) ([#4977]).
+- Ability to specify multiple domain specific upstreams per line e.g.
+  `[/domain1/../domain2/]upstream1 upstream2 .. upstreamN` ([#4977]).
 
 ### Fixed
 

internal/dnsforward/http.go

@@ -445,21 +445,10 @@ func newUpstreamConfig(upstreams []string) (conf *proxy.UpstreamConfig, err erro
 		return nil, nil
 	}
 
-	for _, u := range upstreams {
-		var ups []string
-		var domains []string
-		ups, domains, err = separateUpstream(u)
-		if err != nil {
-			// Don't wrap the error since it's informative enough as is.
-			return nil, err
-		}
-
-		for _, addr := range ups {
-			_, err = validateUpstream(addr, domains)
-			if err != nil {
-				return nil, fmt.Errorf("validating upstream %q: %w", addr, err)
-			}
-		}
+	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(
@@ -470,6 +459,7 @@ func newUpstreamConfig(upstreams []string) (conf *proxy.UpstreamConfig, err erro
 		},
 	)
 	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, errors.Error("no default upstreams specified")
@@ -478,6 +468,31 @@ func newUpstreamConfig(upstreams []string) (conf *proxy.UpstreamConfig, err erro
 	return conf, nil
 }
 
+// validateUpstreamConfig validates each upstream from the upstream
+// configuration and returns an error if any upstream is invalid.
+//
+// TODO(e.burkov):  Move into aghnet or even into dnsproxy.
+func validateUpstreamConfig(conf []string) (err error) {
+	for _, u := range conf {
+		var ups []string
+		var domains []string
+		ups, domains, err = separateUpstream(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, domains)
+			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.
 //
@@ -686,36 +701,6 @@ func (err domainSpecificTestError) Error() (msg string) {
 	return fmt.Sprintf("WARNING: %s", err.error)
 }
 
-// upstreamFromAddr creates the [upstream.Upstream] using opts and information
-// from [s.dnsFilter.EtcHosts].  It returns an error if the line is not a valid
-// upstream line, see [upstream.AddressToUpstream].  It's a caller's
-// responsibility to close u.
-func (s *Server) upstreamFromAddr(
-	addr string,
-	opts *upstream.Options,
-) (u upstream.Upstream, err error) {
-	opts = &upstream.Options{
-		Bootstrap:  opts.Bootstrap,
-		Timeout:    opts.Timeout,
-		PreferIPv6: opts.PreferIPv6,
-	}
-
-	// dnsFilter can be nil during application update.
-	if s.dnsFilter != nil {
-		recs := s.dnsFilter.EtcHostsRecords(extractUpstreamHost(addr))
-		for _, rec := range recs {
-			opts.ServerIPAddrs = append(opts.ServerIPAddrs, rec.Addr.AsSlice())
-		}
-		sortNetIPAddrs(opts.ServerIPAddrs, opts.PreferIPv6)
-	}
-	u, err = upstream.AddressToUpstream(addr, opts)
-	if err != nil {
-		return nil, fmt.Errorf("creating upstream for %q: %w", addr, err)
-	}
-
-	return u, nil
-}
-
 // checkDNS parses line, creates DNS upstreams using opts, and checks if the
 // upstreams are exchanging correctly.  It returns a map where key is an
 // upstream address and value is "OK", if the upstream exchanges correctly, or
@@ -760,12 +745,12 @@ func (s *Server) checkDNS(
 	return result
 }
 
-// checkUpstreamAddr creates the DNS upstream using opts and upstreamAddr.
-// Checks if the DNS upstream echanges correctly.  It returns an error if
-// upstreamAddr is not valid DNS upstream address or the upstream is not
-// exchanging correctly.
+// checkUpstreamAddr creates the DNS upstream using opts and information from
+// [s.dnsFilter.EtcHosts].  Checks if the DNS upstream exchanges correctly.  It
+// returns an error if addr is not valid DNS upstream address or the upstream
+// is not exchanging correctly.
 func (s *Server) checkUpstreamAddr(
-	upstreamAddr string,
+	addr string,
 	specific bool,
 	opts *upstream.Options,
 	check healthCheckFunc,
@@ -776,10 +761,26 @@ func (s *Server) checkUpstreamAddr(
 		}
 	}()
 
-	u, err := s.upstreamFromAddr(upstreamAddr, opts)
-	if err != nil || u == nil {
-		return err
+	opts = &upstream.Options{
+		Bootstrap:  opts.Bootstrap,
+		Timeout:    opts.Timeout,
+		PreferIPv6: opts.PreferIPv6,
 	}
+
+	// dnsFilter can be nil during application update.
+	if s.dnsFilter != nil {
+		recs := s.dnsFilter.EtcHostsRecords(extractUpstreamHost(addr))
+		for _, rec := range recs {
+			opts.ServerIPAddrs = append(opts.ServerIPAddrs, rec.Addr.AsSlice())
+		}
+		sortNetIPAddrs(opts.ServerIPAddrs, opts.PreferIPv6)
+	}
+
+	u, err := upstream.AddressToUpstream(addr, opts)
+	if err != nil {
+		return fmt.Errorf("creating upstream for %q: %w", addr, err)
+	}
+
 	defer func() { err = errors.WithDeferred(err, u.Close()) }()
 
 	return check(u)
@@ -809,10 +810,13 @@ func (s *Server) handleTestUpstreamDNS(w http.ResponseWriter, r *http.Request) {
 		opts.Bootstrap = defaultBootstrap
 	}
 
-	var wg sync.WaitGroup
-	var m sync.Map
+	wg := &sync.WaitGroup{}
+	m := &sync.Map{}
 
+	// TODO(s.chzhen):  Separate to a different structure/file.
 	worker := func(upstreamLine string, check healthCheckFunc) {
+		defer log.OnPanic("dnsforward: checking upstreams")
+
 		res := s.checkDNS(upstreamLine, opts, check)
 		for u, e := range res {
 			m.Store(u, e)
@@ -837,10 +841,10 @@ func (s *Server) handleTestUpstreamDNS(w http.ResponseWriter, r *http.Request) {
 
 	result := map[string]string{}
 	m.Range(func(k, v any) bool {
-		u := k.(string)
-		e := v.(string)
+		ups := k.(string)
+		status := v.(string)
 
-		result[u] = e
+		result[ups] = status
 
 		return true
 	})

internal/dnsforward/http_test.go

@@ -49,13 +49,18 @@ func loadTestData(t *testing.T, casesFileName string, cases any) {
 	require.NoError(t, err)
 }
 
-const jsonExt = ".json"
+const (
+	jsonExt = ".json"
+
+	// testBlockedRespTTL is the TTL for blocked responses to use in tests.
+	testBlockedRespTTL = 10
+)
 
 func TestDNSForwardHTTP_handleGetConfig(t *testing.T) {
 	filterConf := &filtering.Config{
 		ProtectionEnabled:     true,
 		BlockingMode:          filtering.BlockingModeDefault,
-		BlockedResponseTTL:    10,
+		BlockedResponseTTL:    testBlockedRespTTL,
 		SafeBrowsingEnabled:   true,
 		SafeBrowsingCacheSize: 1000,
 		SafeSearchConf:        filtering.SafeSearchConfig{Enabled: true},
@@ -130,12 +135,10 @@ func TestDNSForwardHTTP_handleGetConfig(t *testing.T) {
 }
 
 func TestDNSForwardHTTP_handleSetConfig(t *testing.T) {
-	const defaultBlockedResponseTTL = 10
-
 	filterConf := &filtering.Config{
 		ProtectionEnabled:     true,
 		BlockingMode:          filtering.BlockingModeDefault,
-		BlockedResponseTTL:    defaultBlockedResponseTTL,
+		BlockedResponseTTL:    testBlockedRespTTL,
 		SafeBrowsingEnabled:   true,
 		SafeBrowsingCacheSize: 1000,
 		SafeSearchConf:        filtering.SafeSearchConfig{Enabled: true},
@@ -255,7 +258,7 @@ func TestDNSForwardHTTP_handleSetConfig(t *testing.T) {
 				s.dnsFilter.SetBlockingMode(filtering.BlockingModeDefault, netip.Addr{}, netip.Addr{})
 				s.conf = defaultConf
 				s.conf.Config.EDNSClientSubnet = &EDNSClientSubnet{}
-				s.dnsFilter.SetBlockedResponseTTL(defaultBlockedResponseTTL)
+				s.dnsFilter.SetBlockedResponseTTL(testBlockedRespTTL)
 			})
 
 			rBody := io.NopCloser(bytes.NewReader(caseData.Req))