From 57757c1c42ce43aeabaa6c8cfcdd937a9054859d Mon Sep 17 00:00:00 2001 From: JP Sullivan Date: Thu, 9 Apr 2020 19:56:13 +0100 Subject: [PATCH 1/5] Add servername to grpc dial options for DNS stores (#1507) To avoid needing a query per remote cluster, get the name to add to the dial options from the dns provider when making the grpc connection. Signed-off-by: JP Sullivan --- CHANGELOG.md | 2 ++ cmd/thanos/query.go | 12 +++++++++++- pkg/discovery/dns/provider.go | 16 ++++++++++++++++ pkg/query/storeset.go | 35 +++++++++++++++++++++++++++++++++-- 4 files changed, 62 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 017ec2e415..aad9961c13 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ We use *breaking* word for marking changes that are not backward compatible (rel ## Unreleased +- [#2407](https://github.com/thanos-io/thanos/pull/2407) Query: Add servername to grpc dial options for DNS stores (Fixes #1507) + ### Fixed - [#2288](https://github.com/thanos-io/thanos/pull/2288) Ruler: Fixes issue #2281 bug in ruler with parsing query addr with path prefix diff --git a/cmd/thanos/query.go b/cmd/thanos/query.go index a21565c59a..85cf31c66f 100644 --- a/cmd/thanos/query.go +++ b/cmd/thanos/query.go @@ -56,6 +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() 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() @@ -147,6 +148,7 @@ func registerQuery(m map[string]setupFunc, app *kingpin.Application) { *key, *caCert, *serverName, + *dnsServerName, *httpBindAddr, time.Duration(*httpGracePeriod), *webRoutePrefix, @@ -188,6 +190,7 @@ func runQuery( key string, caCert string, serverName string, + dnsServerName bool, httpBindAddr string, httpGracePeriod time.Duration, webRoutePrefix string, @@ -240,7 +243,11 @@ func runQuery( func() (specs []query.StoreSpec) { // Add DNS resolved addresses. for _, addr := range dnsProvider.Addresses() { - specs = append(specs, query.NewGRPCStoreSpec(addr, false)) + if dnsServerName { + specs = append(specs, query.NewGRPCStoreSpecServerName(addr, false, dnsProvider.ServerName(addr))) + } else { + specs = append(specs, query.NewGRPCStoreSpec(addr, false)) + } } // Add strict & static nodes. for _, addr := range strictStores { @@ -253,6 +260,9 @@ func runQuery( }, dialOpts, unhealthyStoreTimeout, + cert, + key, + caCert, ) proxy = store.NewProxyStore(logger, reg, stores.Get, component.Query, selectorLset, storeResponseTimeout) queryableCreator = query.NewQueryableCreator(logger, proxy) diff --git a/pkg/discovery/dns/provider.go b/pkg/discovery/dns/provider.go index 0ef944574d..4d508ad2c0 100644 --- a/pkg/discovery/dns/provider.go +++ b/pkg/discovery/dns/provider.go @@ -150,3 +150,19 @@ func (p *Provider) Addresses() []string { } return result } + +// ServerName returns the server name for an address +func (p *Provider) ServerName(addr string) string { + p.Lock() + defer p.Unlock() + + for name, addrs := range p.resolved { + for _, a := range addrs { + if addr == a { + _, n := GetQTypeName(name) + return strings.SplitN(n, ":", 2)[0] + } + } + } + return "" +} diff --git a/pkg/query/storeset.go b/pkg/query/storeset.go index aafb003f3f..3406fc2162 100644 --- a/pkg/query/storeset.go +++ b/pkg/query/storeset.go @@ -20,7 +20,9 @@ import ( "github.com/thanos-io/thanos/pkg/runutil" "github.com/thanos-io/thanos/pkg/store" "github.com/thanos-io/thanos/pkg/store/storepb" + "github.com/thanos-io/thanos/pkg/tls" "google.golang.org/grpc" + "google.golang.org/grpc/credentials" ) const ( @@ -38,6 +40,9 @@ type StoreSpec interface { Metadata(ctx context.Context, client storepb.StoreClient) (labelSets []storepb.LabelSet, mint int64, maxt int64, storeType component.StoreAPI, err error) // StrictStatic returns true if the StoreAPI has been statically defined and it is under a strict mode. StrictStatic() bool + // ServerName returns StoreAPI ServerName for the store spec. + // It is needed to get to a StoreAPI behind an nginx proxy + ServerName() string } type StoreStatus struct { @@ -53,12 +58,19 @@ type StoreStatus struct { type grpcStoreSpec struct { addr string strictstatic bool + serverName string +} + +// NewGRPCStoreSpecServerName creates store pure gRPC spec with a Server Name. +// It uses Info gRPC call to get Metadata. +func NewGRPCStoreSpecServerName(addr string, strictstatic bool, serverName string) StoreSpec { + return &grpcStoreSpec{addr: addr, strictstatic: strictstatic, serverName: serverName} } // NewGRPCStoreSpec creates store pure gRPC spec. // It uses Info gRPC call to get Metadata. func NewGRPCStoreSpec(addr string, strictstatic bool) StoreSpec { - return &grpcStoreSpec{addr: addr, strictstatic: strictstatic} + return &grpcStoreSpec{addr: addr, strictstatic: strictstatic, serverName: ""} } // StrictStatic returns true if the StoreAPI has been statically defined and it is under a strict mode. @@ -71,6 +83,10 @@ func (s *grpcStoreSpec) Addr() string { return s.addr } +func (s *grpcStoreSpec) ServerName() string { + return s.serverName +} + // Metadata method for gRPC store API tries to reach host Info method until context timeout. If we are unable to get metadata after // that time, we assume that the host is unhealthy and return error. func (s *grpcStoreSpec) Metadata(ctx context.Context, client storepb.StoreClient) (labelSets []storepb.LabelSet, mint int64, maxt int64, storeType component.StoreAPI, err error) { @@ -165,6 +181,9 @@ type StoreSet struct { storeSpecs func() []StoreSpec dialOpts []grpc.DialOption gRPCInfoCallTimeout time.Duration + cert string + key string + caCert string updateMtx sync.Mutex storesMtx sync.RWMutex @@ -186,6 +205,9 @@ func NewStoreSet( storeSpecs func() []StoreSpec, dialOpts []grpc.DialOption, unhealthyStoreTimeout time.Duration, + cert string, + key string, + caCert string, ) *StoreSet { storesMetric := newStoreSetNodeCollector() if reg != nil { @@ -208,6 +230,9 @@ func NewStoreSet( stores: make(map[string]*storeRef), storeStatuses: make(map[string]*StoreStatus), unhealthyStoreTimeout: unhealthyStoreTimeout, + cert: cert, + key: key, + caCert: caCert, } return ss } @@ -420,7 +445,13 @@ 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. - conn, err := grpc.DialContext(ctx, addr, s.dialOpts...) + 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))) + conn, err := grpc.DialContext(ctx, addr, dialOpts...) if err != nil { s.updateStoreStatus(&storeRef{addr: addr}, err) level.Warn(s.logger).Log("msg", "update of store node failed", "err", errors.Wrap(err, "dialing connection"), "address", addr) From e80c3421a429bb815b00397fd6f23ac18c2f8355 Mon Sep 17 00:00:00 2001 From: JP Sullivan Date: Fri, 10 Apr 2020 10:39:05 +0100 Subject: [PATCH 2/5] 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. Signed-off-by: JP Sullivan --- 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 85cf31c66f..94101f153d 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 3406fc2162..10bee18124 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 5bd98fd233..489cc573d9 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 From 128242f5f3cb4b2eddc18c3d1e72bec6e8974d1e Mon Sep 17 00:00:00 2001 From: JP Sullivan Date: Fri, 10 Apr 2020 10:59:20 +0100 Subject: [PATCH 3/5] Update docs Signed-off-by: JP Sullivan --- docs/components/query.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/components/query.md b/docs/components/query.md index 563fd5ce4b..aa3b8ab87f 100644 --- a/docs/components/query.md +++ b/docs/components/query.md @@ -288,6 +288,11 @@ Flags: Server name to verify the hostname on the returned gRPC certificates. See https://tools.ietf.org/html/rfc4366#section-3.1 + --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 --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 From c203179b3af983ecd0b98e717257ecb98a812d2a Mon Sep 17 00:00:00 2001 From: JP Sullivan Date: Fri, 10 Apr 2020 11:19:53 +0100 Subject: [PATCH 4/5] Add full stops to complete comment sentences Signed-off-by: JP Sullivan --- pkg/discovery/dns/provider.go | 2 +- pkg/query/storeset.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/discovery/dns/provider.go b/pkg/discovery/dns/provider.go index 4d508ad2c0..e06148389c 100644 --- a/pkg/discovery/dns/provider.go +++ b/pkg/discovery/dns/provider.go @@ -151,7 +151,7 @@ func (p *Provider) Addresses() []string { return result } -// ServerName returns the server name for an address +// ServerName returns the server name for an address. func (p *Provider) ServerName(addr string) string { p.Lock() defer p.Unlock() diff --git a/pkg/query/storeset.go b/pkg/query/storeset.go index 10bee18124..bd56306e3f 100644 --- a/pkg/query/storeset.go +++ b/pkg/query/storeset.go @@ -41,7 +41,7 @@ type StoreSpec interface { // StrictStatic returns true if the StoreAPI has been statically defined and it is under a strict mode. StrictStatic() bool // ServerName returns StoreAPI ServerName for the store spec. - // It is needed to get to a StoreAPI behind an nginx proxy + // It is needed to get to a StoreAPI behind an nginx proxy. ServerName() string } @@ -449,7 +449,7 @@ func (s *StoreSet) getActiveStores(ctx context.Context, stores map[string]*store if !seenAlready { // New store or was unactive and was removed in the past - create new one. dialOpts := s.dialOpts - // Update the dialOpts if we are asked to add dnsServerName + // 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 { From 909189cb673093420a86eb7a2832f8e135fa739b Mon Sep 17 00:00:00 2001 From: JP Sullivan Date: Tue, 14 Apr 2020 19:55:02 +0100 Subject: [PATCH 5/5] Review comment updates * Comment formatting * Changelog formatting * Remove nginx references Signed-off-by: JP Sullivan --- CHANGELOG.md | 2 +- cmd/thanos/query.go | 2 +- docs/components/query.md | 5 ++--- pkg/query/storeset.go | 5 ++--- 4 files changed, 6 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aad9961c13..2e26792dbe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ We use *breaking* word for marking changes that are not backward compatible (rel ## Unreleased -- [#2407](https://github.com/thanos-io/thanos/pull/2407) Query: Add servername to grpc dial options for DNS stores (Fixes #1507) +- [#2407](https://github.com/thanos-io/thanos/pull/2407) Query: Add servername to gRPC dial options for DNS stores (Fixes #1507) ### Fixed diff --git a/cmd/thanos/query.go b/cmd/thanos/query.go index 94101f153d..36976dd3e5 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, and only applicable if using the secure flag").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. 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() diff --git a/docs/components/query.md b/docs/components/query.md index aa3b8ab87f..d9e3f86dfa 100644 --- a/docs/components/query.md +++ b/docs/components/query.md @@ -290,9 +290,8 @@ Flags: https://tools.ietf.org/html/rfc4366#section-3.1 --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 + the server name for connection. Only applicable + if using the secure 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 diff --git a/pkg/query/storeset.go b/pkg/query/storeset.go index bd56306e3f..0634867f63 100644 --- a/pkg/query/storeset.go +++ b/pkg/query/storeset.go @@ -40,8 +40,7 @@ type StoreSpec interface { Metadata(ctx context.Context, client storepb.StoreClient) (labelSets []storepb.LabelSet, mint int64, maxt int64, storeType component.StoreAPI, err error) // StrictStatic returns true if the StoreAPI has been statically defined and it is under a strict mode. StrictStatic() bool - // ServerName returns StoreAPI ServerName for the store spec. - // It is needed to get to a StoreAPI behind an nginx proxy. + // ServerName returns the StoreAPI's server name for the store spec. ServerName() string } @@ -61,7 +60,7 @@ type grpcStoreSpec struct { serverName string } -// NewGRPCStoreSpecServerName creates store pure gRPC spec with a Server Name. +// NewGRPCStoreSpecServerName creates store pure gRPC spec with a server name. // It uses Info gRPC call to get Metadata. func NewGRPCStoreSpecServerName(addr string, strictstatic bool, serverName string) StoreSpec { return &grpcStoreSpec{addr: addr, strictstatic: strictstatic, serverName: serverName}