Conditionally add TLS when connecting to a store

The grpc-client-dns-server-name needs to propogate to the storeset so that
TLS is conditionally added to the grpc dialOpts based on whether it is set.

Test that the grpc-client-tls-secure flag is also set when the
grpc-client-dns-server-name flag is set to make it clear that a secure store
connection is required when using the latter flag.

cmd/thanos/query.go

@@ -56,7 +56,7 @@ func registerQuery(m map[string]setupFunc, app *kingpin.Application) {
 	key := cmd.Flag("grpc-client-tls-key", "TLS Key for the client's certificate").Default("").String()
 	caCert := cmd.Flag("grpc-client-tls-ca", "TLS CA Certificates to use to verify gRPC servers").Default("").String()
 	serverName := cmd.Flag("grpc-client-server-name", "Server name to verify the hostname on the returned gRPC certificates. See https://tools.ietf.org/html/rfc4366#section-3.1").Default("").String()
-	dnsServerName := cmd.Flag("grpc-client-dns-server-name", "For stores that are DNS, use the dns name as the server name for connection.  Needed when proxying through nginx").Default("false").Bool()
+	dnsServerName := cmd.Flag("grpc-client-dns-server-name", "For stores that are DNS, use the dns name as the server name for connection.  Needed when proxying through nginx, and only applicable if using the secure flag").Default("false").Bool()
 
 	webRoutePrefix := cmd.Flag("web.route-prefix", "Prefix for API and UI endpoints. This allows thanos UI to be served on a sub-path. This option is analogous to --web.route-prefix of Promethus.").Default("").String()
 	webExternalPrefix := cmd.Flag("web.external-prefix", "Static prefix for all HTML links and redirect URLs in the UI query web interface. Actual endpoints are still served on / or the web.route-prefix. This allows thanos UI to be served behind a reverse proxy that strips a URL sub-path.").Default("").String()
@@ -108,6 +108,10 @@ func registerQuery(m map[string]setupFunc, app *kingpin.Application) {
 	storeResponseTimeout := modelDuration(cmd.Flag("store.response-timeout", "If a Store doesn't send any data in this specified duration then a Store will be ignored and partial data will be returned if it's enabled. 0 disables timeout.").Default("0ms"))
 
 	m[comp.String()] = func(g *run.Group, logger log.Logger, reg *prometheus.Registry, tracer opentracing.Tracer, _ <-chan struct{}, _ bool) error {
+		if *dnsServerName && !*secure {
+			return errors.New("grpc-client-dns-server-name specified without grpc-client-tls-secure")
+		}
+
 		selectorLset, err := parseFlagLabels(*selectorLabels)
 		if err != nil {
 			return errors.Wrap(err, "parse federation labels")
@@ -260,6 +264,7 @@ func runQuery(
 			},
 			dialOpts,
 			unhealthyStoreTimeout,
+			dnsServerName,
 			cert,
 			key,
 			caCert,

pkg/query/storeset.go

@@ -181,6 +181,7 @@ type StoreSet struct {
 	storeSpecs          func() []StoreSpec
 	dialOpts            []grpc.DialOption
 	gRPCInfoCallTimeout time.Duration
+	dnsServerName       bool
 	cert                string
 	key                 string
 	caCert              string
@@ -205,6 +206,7 @@ func NewStoreSet(
 	storeSpecs func() []StoreSpec,
 	dialOpts []grpc.DialOption,
 	unhealthyStoreTimeout time.Duration,
+	dnsServerName bool,
 	cert string,
 	key string,
 	caCert string,
@@ -230,6 +232,7 @@ func NewStoreSet(
 		stores:                make(map[string]*storeRef),
 		storeStatuses:         make(map[string]*StoreStatus),
 		unhealthyStoreTimeout: unhealthyStoreTimeout,
+		dnsServerName:         dnsServerName,
 		cert:                  cert,
 		key:                   key,
 		caCert:                caCert,
@@ -445,12 +448,16 @@ func (s *StoreSet) getActiveStores(ctx context.Context, stores map[string]*store
 			st, seenAlready := stores[addr]
 			if !seenAlready {
 				// New store or was unactive and was removed in the past - create new one.
-				tlsCfg, err := tls.NewClientConfig(s.logger, s.cert, s.key, s.caCert, spec.ServerName())
-				if err != nil {
-					level.Warn(s.logger).Log("msg", "update of store node failed", "err", errors.Wrap(err, "setting TLS"), "address", addr)
-					return
+				dialOpts := s.dialOpts
+				// Update the dialOpts if we are asked to add dnsServerName
+				if s.dnsServerName {
+					tlsCfg, err := tls.NewClientConfig(s.logger, s.cert, s.key, s.caCert, spec.ServerName())
+					if err != nil {
+						level.Warn(s.logger).Log("msg", "update of store node failed", "err", errors.Wrap(err, "setting TLS"), "address", addr)
+						return
+					}
+					dialOpts = append(s.dialOpts, grpc.WithTransportCredentials(credentials.NewTLS(tlsCfg)))
 				}
-				dialOpts := append(s.dialOpts, grpc.WithTransportCredentials(credentials.NewTLS(tlsCfg)))
 				conn, err := grpc.DialContext(ctx, addr, dialOpts...)
 				if err != nil {
 					s.updateStoreStatus(&storeRef{addr: addr}, err)

pkg/query/storeset_test.go

@@ -184,7 +184,7 @@ func TestStoreSet_Update(t *testing.T) {
 			specs = append(specs, NewGRPCStoreSpec(addr, false))
 		}
 		return specs
-	}, testGRPCOpts, time.Minute)
+	}, testGRPCOpts, time.Minute, false, "", "", "")
 	storeSet.gRPCInfoCallTimeout = 2 * time.Second
 	defer storeSet.Close()
 
@@ -532,7 +532,7 @@ func TestStoreSet_Update_NoneAvailable(t *testing.T) {
 			specs = append(specs, NewGRPCStoreSpec(addr, false))
 		}
 		return specs
-	}, testGRPCOpts, time.Minute)
+	}, testGRPCOpts, time.Minute, false, "", "", "")
 	storeSet.gRPCInfoCallTimeout = 2 * time.Second
 
 	// Should not matter how many of these we run.
@@ -596,7 +596,7 @@ func TestQuerierStrict(t *testing.T) {
 			NewGRPCStoreSpec(st.StoreAddresses()[0], true),
 			NewGRPCStoreSpec(st.StoreAddresses()[1], false),
 		}
-	}, testGRPCOpts, time.Minute)
+	}, testGRPCOpts, time.Minute, false, "", "", "")
 	defer storeSet.Close()
 	storeSet.gRPCInfoCallTimeout = 1 * time.Second