From 4cf0dcd323cfd0db0d31bed08bcd448392443600 Mon Sep 17 00:00:00 2001 From: JP Sullivan Date: Fri, 10 Apr 2020 10:39:05 +0100 Subject: [PATCH] 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 | 7 ++++++- pkg/query/storeset.go | 17 ++++++++++++----- pkg/query/storeset_test.go | 6 +++--- 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/cmd/thanos/query.go b/cmd/thanos/query.go index 85cf31c66fa..94101f153d3 100644 --- a/cmd/thanos/query.go +++ b/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, diff --git a/pkg/query/storeset.go b/pkg/query/storeset.go index 3406fc2162b..10bee18124d 100644 --- a/pkg/query/storeset.go +++ b/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) diff --git a/pkg/query/storeset_test.go b/pkg/query/storeset_test.go index 5bd98fd2338..489cc573d95 100644 --- a/pkg/query/storeset_test.go +++ b/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