refactor: add UpstreamGroup.UpstreamsConfig to make values accessible

Move `startVerifyUpstream` to `upstreams.startVerify` so it's accessible via `UpstreamGroup` and we don't need to pass `startVerify` to all resolver constructors that call `NewUpstreamResolver`. Also has the nice benefit of greatly reducing the usage of `GetConfig`.
ThinkChaos · Nov 22, 2023 · 06f080e · 06f080e
1 parent 3ea4012
commit 06f080e
Show file tree

Hide file tree

Showing 24 changed files with 466 additions and 519 deletions.
diff --git a/config/config.go b/config/config.go
@@ -189,44 +189,44 @@ func (b *BootstrappedUpstreamConfig) UnmarshalYAML(unmarshal func(interface{}) e
 //
 //nolint:maligned
 type Config struct {
-	Upstreams           UpstreamsConfig           `yaml:"upstreams"`
-	ConnectIPVersion    IPVersion                 `yaml:"connectIPVersion"`
-	CustomDNS           CustomDNSConfig           `yaml:"customDNS"`
-	Conditional         ConditionalUpstreamConfig `yaml:"conditional"`
-	Blocking            BlockingConfig            `yaml:"blocking"`
-	ClientLookup        ClientLookupConfig        `yaml:"clientLookup"`
-	Caching             CachingConfig             `yaml:"caching"`
-	QueryLog            QueryLogConfig            `yaml:"queryLog"`
-	Prometheus          MetricsConfig             `yaml:"prometheus"`
-	Redis               RedisConfig               `yaml:"redis"`
-	Log                 log.Config                `yaml:"log"`
-	Ports               PortsConfig               `yaml:"ports"`
-	DoHUserAgent        string                    `yaml:"dohUserAgent"`
-	MinTLSServeVer      string                    `yaml:"minTlsServeVersion" default:"1.2"`
-	StartVerifyUpstream bool                      `yaml:"startVerifyUpstream" default:"false"`
-	CertFile            string                    `yaml:"certFile"`
-	KeyFile             string                    `yaml:"keyFile"`
-	BootstrapDNS        BootstrapDNSConfig        `yaml:"bootstrapDns"`
-	HostsFile           HostsFileConfig           `yaml:"hostsFile"`
-	FQDNOnly            FQDNOnly                  `yaml:"fqdnOnly"`
-	Filtering           FilteringConfig           `yaml:"filtering"`
-	EDE                 EDE                       `yaml:"ede"`
-	ECS                 ECS                       `yaml:"ecs"`
-	SUDN                SUDN                      `yaml:"specialUseDomains"`
+	Upstreams        UpstreamsConfig           `yaml:"upstreams"`
+	ConnectIPVersion IPVersion                 `yaml:"connectIPVersion"`
+	CustomDNS        CustomDNSConfig           `yaml:"customDNS"`
+	Conditional      ConditionalUpstreamConfig `yaml:"conditional"`
+	Blocking         BlockingConfig            `yaml:"blocking"`
+	ClientLookup     ClientLookupConfig        `yaml:"clientLookup"`
+	Caching          CachingConfig             `yaml:"caching"`
+	QueryLog         QueryLogConfig            `yaml:"queryLog"`
+	Prometheus       MetricsConfig             `yaml:"prometheus"`
+	Redis            RedisConfig               `yaml:"redis"`
+	Log              log.Config                `yaml:"log"`
+	Ports            PortsConfig               `yaml:"ports"`
+	DoHUserAgent     string                    `yaml:"dohUserAgent"`
+	MinTLSServeVer   string                    `yaml:"minTlsServeVersion" default:"1.2"`
+	CertFile         string                    `yaml:"certFile"`
+	KeyFile          string                    `yaml:"keyFile"`
+	BootstrapDNS     BootstrapDNSConfig        `yaml:"bootstrapDns"`
+	HostsFile        HostsFileConfig           `yaml:"hostsFile"`
+	FQDNOnly         FQDNOnly                  `yaml:"fqdnOnly"`
+	Filtering        FilteringConfig           `yaml:"filtering"`
+	EDE              EDE                       `yaml:"ede"`
+	ECS              ECS                       `yaml:"ecs"`
+	SUDN             SUDN                      `yaml:"specialUseDomains"`
 
 	// Deprecated options
 	Deprecated struct {
-		Upstream        *UpstreamGroups `yaml:"upstream"`
-		UpstreamTimeout *Duration       `yaml:"upstreamTimeout"`
-		DisableIPv6     *bool           `yaml:"disableIPv6"`
-		LogLevel        *log.Level      `yaml:"logLevel"`
-		LogFormat       *log.FormatType `yaml:"logFormat"`
-		LogPrivacy      *bool           `yaml:"logPrivacy"`
-		LogTimestamp    *bool           `yaml:"logTimestamp"`
-		DNSPorts        *ListenConfig   `yaml:"port"`
-		HTTPPorts       *ListenConfig   `yaml:"httpPort"`
-		HTTPSPorts      *ListenConfig   `yaml:"httpsPort"`
-		TLSPorts        *ListenConfig   `yaml:"tlsPort"`
+		Upstream            *UpstreamGroups `yaml:"upstream"`
+		UpstreamTimeout     *Duration       `yaml:"upstreamTimeout"`
+		DisableIPv6         *bool           `yaml:"disableIPv6"`
+		LogLevel            *log.Level      `yaml:"logLevel"`
+		LogFormat           *log.FormatType `yaml:"logFormat"`
+		LogPrivacy          *bool           `yaml:"logPrivacy"`
+		LogTimestamp        *bool           `yaml:"logTimestamp"`
+		DNSPorts            *ListenConfig   `yaml:"port"`
+		HTTPPorts           *ListenConfig   `yaml:"httpPort"`
+		HTTPSPorts          *ListenConfig   `yaml:"httpsPort"`
+		TLSPorts            *ListenConfig   `yaml:"tlsPort"`
+		StartVerifyUpstream *bool           `yaml:"startVerifyUpstream"`
 	} `yaml:",inline"`
 }
 
@@ -514,14 +514,15 @@ func (cfg *Config) migrate(logger *logrus.Entry) bool {
 				cfg.Filtering.QueryTypes.Insert(dns.Type(dns.TypeAAAA))
 			}
 		}),
-		"port":         Move(To("ports.dns", &cfg.Ports)),
-		"httpPort":     Move(To("ports.http", &cfg.Ports)),
-		"httpsPort":    Move(To("ports.https", &cfg.Ports)),
-		"tlsPort":      Move(To("ports.tls", &cfg.Ports)),
-		"logLevel":     Move(To("log.level", &cfg.Log)),
-		"logFormat":    Move(To("log.format", &cfg.Log)),
-		"logPrivacy":   Move(To("log.privacy", &cfg.Log)),
-		"logTimestamp": Move(To("log.timestamp", &cfg.Log)),
+		"port":                Move(To("ports.dns", &cfg.Ports)),
+		"httpPort":            Move(To("ports.http", &cfg.Ports)),
+		"httpsPort":           Move(To("ports.https", &cfg.Ports)),
+		"tlsPort":             Move(To("ports.tls", &cfg.Ports)),
+		"logLevel":            Move(To("log.level", &cfg.Log)),
+		"logFormat":           Move(To("log.format", &cfg.Log)),
+		"logPrivacy":          Move(To("log.privacy", &cfg.Log)),
+		"logTimestamp":        Move(To("log.timestamp", &cfg.Log)),
+		"startVerifyUpstream": Move(To("upstreams.startVerify", &cfg.Upstreams)),
 	})
 
 	usesDepredOpts = cfg.Blocking.migrate(logger) || usesDepredOpts

diff --git a/config/config_test.go b/config/config_test.go
@@ -770,6 +770,7 @@ bootstrapDns:
 
 func defaultTestFileConfig() {
 	Expect(config.Ports.DNS).Should(Equal(ListenConfig{"55553", ":55554", "[::1]:55555"}))
+	Expect(config.Upstreams.StartVerify).Should(BeFalse())
 	Expect(config.Upstreams.Groups["default"]).Should(HaveLen(3))
 	Expect(config.Upstreams.Groups["default"][0].Host).Should(Equal("8.8.8.8"))
 	Expect(config.Upstreams.Groups["default"][1].Host).Should(Equal("8.8.4.4"))
@@ -798,14 +799,14 @@ func defaultTestFileConfig() {
 
 	Expect(config.DoHUserAgent).Should(Equal("testBlocky"))
 	Expect(config.MinTLSServeVer).Should(Equal("1.3"))
-	Expect(config.StartVerifyUpstream).Should(BeFalse())
 
 	Expect(GetConfig()).Should(Not(BeNil()))
 }
 
 func writeConfigYml(tmpDir *helpertest.TmpFolder) *helpertest.TmpFile {
 	return tmpDir.CreateStringFile("config.yml",
 		"upstreams:",
+		"  startVerify: false",
 		"  groups:",
 		"    default:",
 		"      - tcp+udp:8.8.8.8",
@@ -860,7 +861,7 @@ func writeConfigYml(tmpDir *helpertest.TmpFolder) *helpertest.TmpFile {
 		"logLevel: debug",
 		"dohUserAgent: testBlocky",
 		"minTlsServeVersion: 1.3",
-		"startVerifyUpstream: false")
+	)
 }
 
 func writeConfigDir(tmpDir *helpertest.TmpFolder) error {

diff --git a/config/upstreams.go b/config/upstreams.go
@@ -8,9 +8,10 @@ const UpstreamDefaultCfgName = "default"
 
 // UpstreamsConfig upstream servers configuration
 type UpstreamsConfig struct {
-	Timeout  Duration         `yaml:"timeout" default:"2s"`
-	Groups   UpstreamGroups   `yaml:"groups"`
-	Strategy UpstreamStrategy `yaml:"strategy" default:"parallel_best"`
+	Timeout     Duration         `yaml:"timeout" default:"2s"`
+	Groups      UpstreamGroups   `yaml:"groups"`
+	Strategy    UpstreamStrategy `yaml:"strategy" default:"parallel_best"`
+	StartVerify bool             `yaml:"startVerify" default:"false"`
 }
 
 type UpstreamGroups map[string][]Upstream
@@ -37,21 +38,40 @@ func (c *UpstreamsConfig) LogConfig(logger *logrus.Entry) {
 
 // UpstreamGroup represents the config for one group (upstream branch)
 type UpstreamGroup struct {
-	Name      string
-	Upstreams []Upstream
+	UpstreamsConfig
+
+	Name string // group name
+}
+
+// NewUpstreamGroup creates an UpstreamGroup with the given name and upstreams.
+//
+// The upstreams from `cfg.Groups` are ignored.
+func NewUpstreamGroup(name string, cfg UpstreamsConfig, upstreams []Upstream) UpstreamGroup {
+	group := UpstreamGroup{
+		Name:            name,
+		UpstreamsConfig: cfg,
+	}
+
+	group.Groups = UpstreamGroups{name: upstreams}
+
+	return group
+}
+
+func (c *UpstreamGroup) Upstreams() []Upstream {
+	return c.Groups[c.Name]
 }
 
 // IsEnabled implements `config.Configurable`.
 func (c *UpstreamGroup) IsEnabled() bool {
-	return len(c.Upstreams) != 0
+	return len(c.Upstreams()) != 0
 }
 
 // LogConfig implements `config.Configurable`.
 func (c *UpstreamGroup) LogConfig(logger *logrus.Entry) {
 	logger.Info("group: ", c.Name)
 	logger.Info("upstreams:")
 
-	for _, upstream := range c.Upstreams {
+	for _, upstream := range c.Upstreams() {
 		logger.Infof("  - %s", upstream)
 	}
 }
diff --git a/config/upstreams_test.go b/config/upstreams_test.go
@@ -65,13 +65,13 @@ var _ = Describe("ParallelBestConfig", func() {
 		var cfg UpstreamGroup
 
 		BeforeEach(func() {
-			cfg = UpstreamGroup{
-				Name: UpstreamDefaultCfgName,
-				Upstreams: []Upstream{
-					{Host: "host1"},
-					{Host: "host2"},
-				},
-			}
+			upstreamsCfg, err := WithDefaults[UpstreamsConfig]()
+			Expect(err).Should(Succeed())
+
+			cfg = NewUpstreamGroup("test", upstreamsCfg, []Upstream{
+				{Host: "host1"},
+				{Host: "host2"},
+			})
 		})
 
 		Describe("IsEnabled", func() {
@@ -102,7 +102,7 @@ var _ = Describe("ParallelBestConfig", func() {
 				cfg.LogConfig(logger)
 
 				Expect(hook.Calls).ShouldNot(BeEmpty())
-				Expect(hook.Messages).Should(ContainElement(ContainSubstring("group: default")))
+				Expect(hook.Messages).Should(ContainElement(ContainSubstring("group: test")))
 				Expect(hook.Messages).Should(ContainElement(ContainSubstring("upstreams:")))
 				Expect(hook.Messages).Should(ContainElement(ContainSubstring(":host1:")))
 				Expect(hook.Messages).Should(ContainElement(ContainSubstring(":host2:")))

diff --git a/docs/config.yml b/docs/config.yml
@@ -24,9 +24,8 @@ upstreams:
   strategy: parallel_best
   # optional: timeout to query the upstream resolver. Default: 2s
   timeout: 2s
-
-# optional: If true, blocky will fail to start unless at least one upstream server per group is reachable. Default: false
-startVerifyUpstream: true
+  # optional: If true, blocky will fail to start unless at least one upstream server per group is reachable. Default: false
+  startVerify: false
 
 # optional: Determines how blocky will create outgoing connections. This impacts both upstreams, and lists.
 # accepted: dual, v4, v6

diff --git a/docs/configuration.md b/docs/configuration.md
@@ -17,7 +17,6 @@ configuration properties as [JSON](config.yml).
 | keyFile             | path                | no        |               | Path to cert and key file for SSL encryption (DoH and DoT); if empty, self-signed certificate is generated |
 | dohUserAgent        | string              | no        |               | HTTP User Agent for DoH upstreams                                                                          |
 | minTlsServeVersion  | string              | no        | 1.2           | Minimum TLS version that the DoT and DoH server use to serve those encrypted DNS requests                  |
-| startVerifyUpstream | bool                | no        | false         | If true, blocky will fail to start unless at least one upstream server per group is reachable.             |
 | connectIPVersion    | enum (dual, v4, v6) | no        | dual          | IP version to use for outgoing connections (dual, v4, v6)                                                  |
 
 !!! example
@@ -70,6 +69,16 @@ All logging options are optional.
 
 ## Upstreams configuration
 
+| Parameter             | Type                                 | Default value | Description                                                                                     |
+| --------------------- | ------------------------------------ | ------------- | ----------------------------------------------------------------------------------------------- |
+| usptreams.groups      | map of name to upstream              | none          | Upstream DNS servers to use, in groups.                                                         |
+| usptreams.startVerify | bool                                 | false         | If true, blocky will fail to start unless at least one upstream server per group is functional. |
+| usptreams.strategy    | enum (parallel_best, random, strict) | parallel_best | Upstream server usage strategy.                                                                 |
+| usptreams.timeout     | bool                                 | true          | Upstream connection timeout.                                                                    |
+
+
+### Upstream Groups
+
 To resolve a DNS query, blocky needs external public or private DNS resolvers. Blocky supports DNS resolvers with
 following network protocols (net part of the resolver URL):
 
@@ -133,6 +142,22 @@ The logic determining what group a client belongs to follows a strict order: IP,
 
 If a client matches multiple client name or CIDR groups, a warning is logged and the first found group is used.
 
+### Upstream connection timeout
+
+Blocky will wait 2 seconds (default value) for the response from the external upstream DNS server. You can change this
+value by setting the `timeout` configuration parameter (in **duration format**).
+
+!!! example
+
+    ```yaml
+    upstreams:
+      timeout: 5s
+      groups:
+        default:
+          - 46.182.19.48
+          - 80.241.218.68
+    ```
+
 ### Upstream strategy
 
 Blocky supports different upstream strategies (default `parallel_best`) that determine how and to which upstream DNS servers requests are forwarded.
@@ -160,21 +185,6 @@ Currently available strategies:
           - 9.8.7.6
     ```
 
-### Upstream lookup timeout
-
-Blocky will wait 2 seconds (default value) for the response from the external upstream DNS server. You can change this
-value by setting the `timeout` configuration parameter (in **duration format**).
-
-!!! example
-
-    ```yaml
-    upstreams:
-      timeout: 5s
-      groups:
-        default:
-          - 46.182.19.48
-          - 80.241.218.68
-    ```
 
 ## Bootstrap DNS configuration
 

diff --git a/resolver/bootstrap.go b/resolver/bootstrap.go
@@ -65,7 +65,7 @@ func NewBootstrap(ctx context.Context, cfg *config.Config) (b *Bootstrap, err er
 		},
 	}
 
-	bootstraped, err := newBootstrapedResolvers(b, cfg.BootstrapDNS)
+	bootstraped, err := newBootstrapedResolvers(b, cfg.BootstrapDNS, cfg.Upstreams)
 	if err != nil {
 		return nil, err
 	}
@@ -76,11 +76,8 @@ func NewBootstrap(ctx context.Context, cfg *config.Config) (b *Bootstrap, err er
 		return b, nil
 	}
 
-	// Bootstrap doesn't have a `LogConfig` method, and since that's the only place
-	// where `ParallelBestResolver` uses its config, we can just use an empty one.
-	pbCfg := config.UpstreamGroup{Name: upstreamDefaultCfgName}
-
-	parallelResolver := newParallelBestResolver(pbCfg, bootstraped.Resolvers())
+	pbCfg := config.NewUpstreamGroup("<bootstrap>", cfg.Upstreams, nil)
+	pbCfg.UpstreamsConfig.Groups = nil // To be on the safe side it doesn't try to use anything besides the bootstrap
 
 	// Always enable prefetching to avoid stalling user requests
 	// Otherwise, a request to blocky could end up waiting for 2 DNS requests:
@@ -100,14 +97,14 @@ func NewBootstrap(ctx context.Context, cfg *config.Config) (b *Bootstrap, err er
 		NewFilteringResolver(cfg.Filtering),
 		// false: no metrics, to not overwrite the main blocking resolver ones
 		newCachingResolver(ctx, cachingCfg, nil, false),
-		parallelResolver,
+		newParallelBestResolver(pbCfg, bootstraped.Resolvers()),
 	)
 
 	return b, nil
 }
 
 func (b *Bootstrap) UpstreamIPs(ctx context.Context, r *UpstreamResolver) (*IPSet, error) {
-	hostname := r.upstream.Host
+	hostname := r.cfg.Host
 
 	if ip := net.ParseIP(hostname); ip != nil { // nil-safe when hostname is an IP: makes writing test easier
 		return newIPSet([]net.IP{ip}), nil
@@ -249,7 +246,9 @@ func (b *Bootstrap) resolveType(ctx context.Context, hostname string, qType dns.
 // map of bootstraped resolvers their hardcoded IPs
 type bootstrapedResolvers map[Resolver][]net.IP
 
-func newBootstrapedResolvers(b *Bootstrap, cfg config.BootstrapDNSConfig) (bootstrapedResolvers, error) {
+func newBootstrapedResolvers(
+	b *Bootstrap, cfg config.BootstrapDNSConfig, upstreamsCfg config.UpstreamsConfig,
+) (bootstrapedResolvers, error) {
 	upstreamIPs := make(bootstrapedResolvers, len(cfg))
 
 	var multiErr *multierror.Error
@@ -289,7 +288,7 @@ func newBootstrapedResolvers(b *Bootstrap, cfg config.BootstrapDNSConfig) (boots
 			continue
 		}
 
-		resolver := newUpstreamResolverUnchecked(upstream, b)
+		resolver := newUpstreamResolverUnchecked(newUpstreamConfig(upstream, upstreamsCfg), b)
 
 		upstreamIPs[resolver] = ips
 	}

diff --git a/resolver/bootstrap_test.go b/resolver/bootstrap_test.go
@@ -32,7 +32,6 @@ var _ = Describe("Bootstrap", Label("bootstrap"), func() {
 	)
 
 	BeforeEach(func() {
-		config.GetConfig().Upstreams.Strategy = config.UpstreamStrategyParallelBest
 		sutConfig = &config.Config{
 			BootstrapDNS: []config.BootstrappedUpstreamConfig{
 				{
@@ -43,6 +42,7 @@ var _ = Describe("Bootstrap", Label("bootstrap"), func() {
 					IPs: []net.IP{net.IPv4zero},
 				},
 			},
+			Upstreams: defaultUpstreamsConfig,
 		}
 
 		ctx, cancelFn = context.WithCancel(context.Background())
@@ -327,7 +327,7 @@ var _ = Describe("Bootstrap", Label("bootstrap"), func() {
 
 				upstream.Host = "localhost" // force bootstrap to do resolve, and not just return the IP as is
 
-				r := newUpstreamResolverUnchecked(upstream, sut)
+				r := newUpstreamResolverUnchecked(newUpstreamConfig(upstream, sutConfig.Upstreams), sut)
 
 				rsp, err := r.Resolve(ctx, mainReq)
 				Expect(err).Should(Succeed())