From 5f60fde0dbb8585185046637e2d04fa56f52c692 Mon Sep 17 00:00:00 2001 From: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com> Date: Tue, 29 Sep 2020 16:21:35 +0200 Subject: [PATCH 1/9] Don't propagate DNS not found error. Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com> --- pkg/discovery/dns/provider.go | 2 +- pkg/discovery/dns/resolver.go | 10 +++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/pkg/discovery/dns/provider.go b/pkg/discovery/dns/provider.go index e80f7aac49..43dd980a2e 100644 --- a/pkg/discovery/dns/provider.go +++ b/pkg/discovery/dns/provider.go @@ -57,7 +57,7 @@ func (t ResolverType) ToResolver(logger log.Logger) ipLookupResolver { // If empty resolver type is net.DefaultResolver. func NewProvider(logger log.Logger, reg prometheus.Registerer, resolverType ResolverType) *Provider { p := &Provider{ - resolver: NewResolver(resolverType.ToResolver(logger)), + resolver: NewResolver(resolverType.ToResolver(logger), resolverType), resolved: make(map[string][]string), logger: logger, resolverAddrs: extprom.NewTxGaugeVec(reg, prometheus.GaugeOpts{ diff --git a/pkg/discovery/dns/resolver.go b/pkg/discovery/dns/resolver.go index ef73054768..99b2b34a9d 100644 --- a/pkg/discovery/dns/resolver.go +++ b/pkg/discovery/dns/resolver.go @@ -38,11 +38,12 @@ type ipLookupResolver interface { type dnsSD struct { resolver ipLookupResolver + resolverType ResolverType } // NewResolver creates a resolver with given underlying resolver. -func NewResolver(resolver ipLookupResolver) Resolver { - return &dnsSD{resolver: resolver} +func NewResolver(resolver ipLookupResolver, resolverType ResolverType) Resolver { + return &dnsSD{resolver: resolver, resolverType: resolverType} } func (s *dnsSD) Resolve(ctx context.Context, name string, qtype QType) ([]string, error) { @@ -71,7 +72,10 @@ func (s *dnsSD) Resolve(ctx context.Context, name string, qtype QType) ([]string } ips, err := s.resolver.LookupIPAddr(ctx, host) if err != nil { - return nil, errors.Wrapf(err, "lookup IP addresses %q", host) + dnsErr, ok := err.(*net.DNSError) + if !(s.resolverType == GolangResolverType && ok && dnsErr.IsNotFound) { + return nil, errors.Wrapf(err, "lookup IP addresses %q", host) + } } for _, ip := range ips { res = append(res, appendScheme(scheme, net.JoinHostPort(ip.String(), port))) From 92725c02ec289ad2ddd56d4b4ad66ffc7596b5c1 Mon Sep 17 00:00:00 2001 From: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com> Date: Tue, 29 Sep 2020 16:37:08 +0200 Subject: [PATCH 2/9] Update change log. Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com> --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 94abc0e0e6..eb91ae13d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re ## Unreleased +### Fixed + +## [#3257](https://github.com/thanos-io/thanos/pull/3257) Ruler: Prevent Ruler from crashing when using default DNS to lookup hosts that results in "No such hosts" errors. + ## [v0.16.0](https://github.com/thanos-io/thanos/releases) - Release in progress ### Fixed From a5e4ce92cf224de5bcddab58a753fdb95909d4f0 Mon Sep 17 00:00:00 2001 From: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com> Date: Tue, 29 Sep 2020 16:50:36 +0200 Subject: [PATCH 3/9] Add DNS resolver type for mock resolver. Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com> --- pkg/discovery/dns/provider.go | 1 + pkg/discovery/dns/resolver.go | 2 +- pkg/discovery/dns/resolver_test.go | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/discovery/dns/provider.go b/pkg/discovery/dns/provider.go index 43dd980a2e..4df7c07f05 100644 --- a/pkg/discovery/dns/provider.go +++ b/pkg/discovery/dns/provider.go @@ -37,6 +37,7 @@ type ResolverType string const ( GolangResolverType ResolverType = "golang" MiekgdnsResolverType ResolverType = "miekgdns" + MockdnsResolverType ResolverType = "mockdns" ) func (t ResolverType) ToResolver(logger log.Logger) ipLookupResolver { diff --git a/pkg/discovery/dns/resolver.go b/pkg/discovery/dns/resolver.go index 99b2b34a9d..d9be897829 100644 --- a/pkg/discovery/dns/resolver.go +++ b/pkg/discovery/dns/resolver.go @@ -37,7 +37,7 @@ type ipLookupResolver interface { } type dnsSD struct { - resolver ipLookupResolver + resolver ipLookupResolver resolverType ResolverType } diff --git a/pkg/discovery/dns/resolver_test.go b/pkg/discovery/dns/resolver_test.go index 5663ca7380..b6635c8fe3 100644 --- a/pkg/discovery/dns/resolver_test.go +++ b/pkg/discovery/dns/resolver_test.go @@ -184,7 +184,7 @@ func TestDnsSD_Resolve(t *testing.T) { func testDnsSd(t *testing.T, tt DNSSDTest) { ctx := context.TODO() - dnsSD := dnsSD{tt.resolver} + dnsSD := dnsSD{tt.resolver, MockdnsResolverType} result, err := dnsSD.Resolve(ctx, tt.addr, tt.qtype) if tt.expectedErr != nil { From 84824e6864c80bfad084fdd7a799f2c2a008bc0d Mon Sep 17 00:00:00 2001 From: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com> Date: Wed, 30 Sep 2020 10:43:17 +0200 Subject: [PATCH 4/9] Remove trailing white space in changelog. Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com> --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eb91ae13d2..d2b500361a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re ### Fixed -## [#3257](https://github.com/thanos-io/thanos/pull/3257) Ruler: Prevent Ruler from crashing when using default DNS to lookup hosts that results in "No such hosts" errors. +## [#3257](https://github.com/thanos-io/thanos/pull/3257) Ruler: Prevent Ruler from crashing when using default DNS to lookup hosts that results in "No such hosts" errors. ## [v0.16.0](https://github.com/thanos-io/thanos/releases) - Release in progress From e30a981ad8cd897ee1b13db383da1c1ceed4bad4 Mon Sep 17 00:00:00 2001 From: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com> Date: Wed, 30 Sep 2020 15:32:16 +0200 Subject: [PATCH 5/9] Add flag to ensure new behaviour only impacts ruler AM resolution. Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com> --- cmd/thanos/query.go | 2 ++ cmd/thanos/rule.go | 2 ++ pkg/cacheutil/memcached_client.go | 1 + pkg/discovery/dns/provider.go | 5 ++--- pkg/discovery/dns/provider_test.go | 2 +- pkg/discovery/dns/resolver.go | 26 +++++++++++++++++++++----- pkg/discovery/dns/resolver_test.go | 4 +++- 7 files changed, 32 insertions(+), 10 deletions(-) diff --git a/cmd/thanos/query.go b/cmd/thanos/query.go index 4ce74da495..374265db16 100644 --- a/cmd/thanos/query.go +++ b/cmd/thanos/query.go @@ -265,6 +265,7 @@ func runQuery( logger, extprom.WrapRegistererWithPrefix("thanos_querier_store_apis_", reg), dns.ResolverType(dnsSDResolver), + true, ) for _, store := range strictStores { @@ -277,6 +278,7 @@ func runQuery( logger, extprom.WrapRegistererWithPrefix("thanos_querier_rule_apis_", reg), dns.ResolverType(dnsSDResolver), + true, ) var ( diff --git a/cmd/thanos/rule.go b/cmd/thanos/rule.go index f72bbedfd2..17079cb943 100644 --- a/cmd/thanos/rule.go +++ b/cmd/thanos/rule.go @@ -338,6 +338,7 @@ func runRule( logger, extprom.WrapRegistererWithPrefix("thanos_ruler_query_apis_", reg), dns.ResolverType(dnsSDResolver), + true, ) var queryClients []*http_util.Client for _, cfg := range queryCfg { @@ -401,6 +402,7 @@ func runRule( logger, extprom.WrapRegistererWithPrefix("thanos_ruler_alertmanagers_", reg), dns.ResolverType(dnsSDResolver), + false, ) var alertmgrs []*alert.Alertmanager for _, cfg := range alertingCfg.Alertmanagers { diff --git a/pkg/cacheutil/memcached_client.go b/pkg/cacheutil/memcached_client.go index 15751109dc..3becb31e79 100644 --- a/pkg/cacheutil/memcached_client.go +++ b/pkg/cacheutil/memcached_client.go @@ -219,6 +219,7 @@ func newMemcachedClient( logger, extprom.WrapRegistererWithPrefix("thanos_memcached_", reg), dns.GolangResolverType, + true, ) c := &memcachedClient{ diff --git a/pkg/discovery/dns/provider.go b/pkg/discovery/dns/provider.go index 4df7c07f05..11867345ac 100644 --- a/pkg/discovery/dns/provider.go +++ b/pkg/discovery/dns/provider.go @@ -37,7 +37,6 @@ type ResolverType string const ( GolangResolverType ResolverType = "golang" MiekgdnsResolverType ResolverType = "miekgdns" - MockdnsResolverType ResolverType = "mockdns" ) func (t ResolverType) ToResolver(logger log.Logger) ipLookupResolver { @@ -56,9 +55,9 @@ func (t ResolverType) ToResolver(logger log.Logger) ipLookupResolver { // NewProvider returns a new empty provider with a given resolver type. // If empty resolver type is net.DefaultResolver. -func NewProvider(logger log.Logger, reg prometheus.Registerer, resolverType ResolverType) *Provider { +func NewProvider(logger log.Logger, reg prometheus.Registerer, resolverType ResolverType, returnErrOnNotFound bool) *Provider { p := &Provider{ - resolver: NewResolver(resolverType.ToResolver(logger), resolverType), + resolver: NewResolver(resolverType.ToResolver(logger), logger, returnErrOnNotFound), resolved: make(map[string][]string), logger: logger, resolverAddrs: extprom.NewTxGaugeVec(reg, prometheus.GaugeOpts{ diff --git a/pkg/discovery/dns/provider_test.go b/pkg/discovery/dns/provider_test.go index 668d5a65c8..87277c3d22 100644 --- a/pkg/discovery/dns/provider_test.go +++ b/pkg/discovery/dns/provider_test.go @@ -22,7 +22,7 @@ func TestProvider(t *testing.T) { "127.0.0.5:19095", } - prv := NewProvider(log.NewNopLogger(), nil, "") + prv := NewProvider(log.NewNopLogger(), nil, "", true) prv.resolver = &mockResolver{ res: map[string][]string{ "a": ips[:2], diff --git a/pkg/discovery/dns/resolver.go b/pkg/discovery/dns/resolver.go index d9be897829..9837f8e362 100644 --- a/pkg/discovery/dns/resolver.go +++ b/pkg/discovery/dns/resolver.go @@ -9,6 +9,9 @@ import ( "strconv" "strings" + "github.com/go-kit/kit/log" + "github.com/go-kit/kit/log/level" + "github.com/pkg/errors" ) @@ -37,13 +40,16 @@ type ipLookupResolver interface { } type dnsSD struct { - resolver ipLookupResolver - resolverType ResolverType + resolver ipLookupResolver + logger log.Logger + // https://github.com/thanos-io/thanos/issues/3186 + // This flag is used to prevent components from crashing if hosts are not found. + returnErrOnNotFound bool } // NewResolver creates a resolver with given underlying resolver. -func NewResolver(resolver ipLookupResolver, resolverType ResolverType) Resolver { - return &dnsSD{resolver: resolver, resolverType: resolverType} +func NewResolver(resolver ipLookupResolver, logger log.Logger, returnErrOnNotFound bool) Resolver { + return &dnsSD{resolver: resolver, logger: logger, returnErrOnNotFound: returnErrOnNotFound} } func (s *dnsSD) Resolve(ctx context.Context, name string, qtype QType) ([]string, error) { @@ -73,9 +79,13 @@ func (s *dnsSD) Resolve(ctx context.Context, name string, qtype QType) ([]string ips, err := s.resolver.LookupIPAddr(ctx, host) if err != nil { dnsErr, ok := err.(*net.DNSError) - if !(s.resolverType == GolangResolverType && ok && dnsErr.IsNotFound) { + // https://github.com/thanos-io/thanos/issues/3186 + // Default DNS resolver can make thanos components crash if DSN resolutions results in EAI_NONAME. + // the flag returnErrOnNotFound can be used to prevent such crash. + if !(!s.returnErrOnNotFound && ok && dnsErr.IsNotFound) { return nil, errors.Wrapf(err, "lookup IP addresses %q", host) } + level.Error(s.logger).Log("msg", "failed to lookup IP addresses", "host", host, "err", err) } for _, ip := range ips { res = append(res, appendScheme(scheme, net.JoinHostPort(ip.String(), port))) @@ -110,6 +120,12 @@ func (s *dnsSD) Resolve(ctx context.Context, name string, qtype QType) ([]string return nil, errors.Errorf("invalid lookup scheme %q", qtype) } + // https://github.com/thanos-io/thanos/issues/3186 + // This happens when miekg is used as resolver. When the host cannot be found, nothing is returned. + if res == nil && err == nil { + level.Warn(s.logger).Log("msg", "IP address lookup yielded no results nor errors", "host", host) + } + return res, nil } diff --git a/pkg/discovery/dns/resolver_test.go b/pkg/discovery/dns/resolver_test.go index b6635c8fe3..a12a9be163 100644 --- a/pkg/discovery/dns/resolver_test.go +++ b/pkg/discovery/dns/resolver_test.go @@ -9,6 +9,8 @@ import ( "sort" "testing" + "github.com/go-kit/kit/log" + "github.com/pkg/errors" "github.com/thanos-io/thanos/pkg/testutil" ) @@ -184,7 +186,7 @@ func TestDnsSD_Resolve(t *testing.T) { func testDnsSd(t *testing.T, tt DNSSDTest) { ctx := context.TODO() - dnsSD := dnsSD{tt.resolver, MockdnsResolverType} + dnsSD := dnsSD{tt.resolver, log.NewNopLogger(), false} result, err := dnsSD.Resolve(ctx, tt.addr, tt.qtype) if tt.expectedErr != nil { From 5e3c66096151fe5cbd249297eea953b1089f0b47 Mon Sep 17 00:00:00 2001 From: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com> Date: Thu, 1 Oct 2020 10:08:41 +0200 Subject: [PATCH 6/9] Make method signature change backwards compatible Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com> --- cmd/thanos/query.go | 4 ++-- cmd/thanos/rule.go | 4 ++-- pkg/cacheutil/memcached_client.go | 2 +- pkg/discovery/dns/provider.go | 29 ++++++++++++++++++++++++++++- pkg/discovery/dns/provider_test.go | 2 +- 5 files changed, 34 insertions(+), 7 deletions(-) diff --git a/cmd/thanos/query.go b/cmd/thanos/query.go index 374265db16..f8df4cf22b 100644 --- a/cmd/thanos/query.go +++ b/cmd/thanos/query.go @@ -261,7 +261,7 @@ func runQuery( } fileSDCache := cache.New() - dnsStoreProvider := dns.NewProvider( + dnsStoreProvider := dns.NewProviderWithReturnOnErrorIfNotFound( logger, extprom.WrapRegistererWithPrefix("thanos_querier_store_apis_", reg), dns.ResolverType(dnsSDResolver), @@ -274,7 +274,7 @@ func runQuery( } } - dnsRuleProvider := dns.NewProvider( + dnsRuleProvider := dns.NewProviderWithReturnOnErrorIfNotFound( logger, extprom.WrapRegistererWithPrefix("thanos_querier_rule_apis_", reg), dns.ResolverType(dnsSDResolver), diff --git a/cmd/thanos/rule.go b/cmd/thanos/rule.go index 17079cb943..0c05559169 100644 --- a/cmd/thanos/rule.go +++ b/cmd/thanos/rule.go @@ -334,7 +334,7 @@ func runRule( } } - queryProvider := dns.NewProvider( + queryProvider := dns.NewProviderWithReturnOnErrorIfNotFound( logger, extprom.WrapRegistererWithPrefix("thanos_ruler_query_apis_", reg), dns.ResolverType(dnsSDResolver), @@ -398,7 +398,7 @@ func runRule( level.Warn(logger).Log("msg", "no alertmanager configured") } - amProvider := dns.NewProvider( + amProvider := dns.NewProviderWithReturnOnErrorIfNotFound( logger, extprom.WrapRegistererWithPrefix("thanos_ruler_alertmanagers_", reg), dns.ResolverType(dnsSDResolver), diff --git a/pkg/cacheutil/memcached_client.go b/pkg/cacheutil/memcached_client.go index 3becb31e79..93fcf7795c 100644 --- a/pkg/cacheutil/memcached_client.go +++ b/pkg/cacheutil/memcached_client.go @@ -215,7 +215,7 @@ func newMemcachedClient( config MemcachedClientConfig, reg prometheus.Registerer, ) (*memcachedClient, error) { - dnsProvider := dns.NewProvider( + dnsProvider := dns.NewProviderWithReturnOnErrorIfNotFound( logger, extprom.WrapRegistererWithPrefix("thanos_memcached_", reg), dns.GolangResolverType, diff --git a/pkg/discovery/dns/provider.go b/pkg/discovery/dns/provider.go index 11867345ac..a11eccb9f0 100644 --- a/pkg/discovery/dns/provider.go +++ b/pkg/discovery/dns/provider.go @@ -53,9 +53,36 @@ func (t ResolverType) ToResolver(logger log.Logger) ipLookupResolver { return r } +// Deprecated. Use NewProviderWithReturnOnErrorIfNotFound instead. +// // NewProvider returns a new empty provider with a given resolver type. // If empty resolver type is net.DefaultResolver. -func NewProvider(logger log.Logger, reg prometheus.Registerer, resolverType ResolverType, returnErrOnNotFound bool) *Provider { +// TODO(OGKevin): Refactor code to use new method and eventually rename NewProviderWithReturnOnErrorIfNotFound back to NewProvider. +func NewProvider(logger log.Logger, reg prometheus.Registerer, resolverType ResolverType) *Provider { + p := &Provider{ + resolver: NewResolver(resolverType.ToResolver(logger), logger, true), + resolved: make(map[string][]string), + logger: logger, + resolverAddrs: extprom.NewTxGaugeVec(reg, prometheus.GaugeOpts{ + Name: "dns_provider_results", + Help: "The number of resolved endpoints for each configured address", + }, []string{"addr"}), + resolverLookupsCount: promauto.With(reg).NewCounter(prometheus.CounterOpts{ + Name: "dns_lookups_total", + Help: "The number of DNS lookups resolutions attempts", + }), + resolverFailuresCount: promauto.With(reg).NewCounter(prometheus.CounterOpts{ + Name: "dns_failures_total", + Help: "The number of DNS lookup failures", + }), + } + + return p +} + +// NewProviderWithReturnOnErrorIfNotFound returns a new empty provider with a given resolver type. +// If empty resolver type is net.DefaultResolver. +func NewProviderWithReturnOnErrorIfNotFound(logger log.Logger, reg prometheus.Registerer, resolverType ResolverType, returnErrOnNotFound bool) *Provider { p := &Provider{ resolver: NewResolver(resolverType.ToResolver(logger), logger, returnErrOnNotFound), resolved: make(map[string][]string), diff --git a/pkg/discovery/dns/provider_test.go b/pkg/discovery/dns/provider_test.go index 87277c3d22..d342b925b1 100644 --- a/pkg/discovery/dns/provider_test.go +++ b/pkg/discovery/dns/provider_test.go @@ -22,7 +22,7 @@ func TestProvider(t *testing.T) { "127.0.0.5:19095", } - prv := NewProvider(log.NewNopLogger(), nil, "", true) + prv := NewProviderWithReturnOnErrorIfNotFound(log.NewNopLogger(), nil, "", true) prv.resolver = &mockResolver{ res: map[string][]string{ "a": ips[:2], From 76b52631c66323677315c1010312e44b89084a65 Mon Sep 17 00:00:00 2001 From: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com> Date: Fri, 2 Oct 2020 21:09:53 +0200 Subject: [PATCH 7/9] Minor refactoring of logical sequence and dub code. Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com> --- pkg/discovery/dns/provider.go | 20 +------------------- pkg/discovery/dns/resolver.go | 5 ++--- 2 files changed, 3 insertions(+), 22 deletions(-) diff --git a/pkg/discovery/dns/provider.go b/pkg/discovery/dns/provider.go index a11eccb9f0..097be1310b 100644 --- a/pkg/discovery/dns/provider.go +++ b/pkg/discovery/dns/provider.go @@ -59,25 +59,7 @@ func (t ResolverType) ToResolver(logger log.Logger) ipLookupResolver { // If empty resolver type is net.DefaultResolver. // TODO(OGKevin): Refactor code to use new method and eventually rename NewProviderWithReturnOnErrorIfNotFound back to NewProvider. func NewProvider(logger log.Logger, reg prometheus.Registerer, resolverType ResolverType) *Provider { - p := &Provider{ - resolver: NewResolver(resolverType.ToResolver(logger), logger, true), - resolved: make(map[string][]string), - logger: logger, - resolverAddrs: extprom.NewTxGaugeVec(reg, prometheus.GaugeOpts{ - Name: "dns_provider_results", - Help: "The number of resolved endpoints for each configured address", - }, []string{"addr"}), - resolverLookupsCount: promauto.With(reg).NewCounter(prometheus.CounterOpts{ - Name: "dns_lookups_total", - Help: "The number of DNS lookups resolutions attempts", - }), - resolverFailuresCount: promauto.With(reg).NewCounter(prometheus.CounterOpts{ - Name: "dns_failures_total", - Help: "The number of DNS lookup failures", - }), - } - - return p + return NewProviderWithReturnOnErrorIfNotFound(logger, reg, resolverType, true) } // NewProviderWithReturnOnErrorIfNotFound returns a new empty provider with a given resolver type. diff --git a/pkg/discovery/dns/resolver.go b/pkg/discovery/dns/resolver.go index 9837f8e362..780763195b 100644 --- a/pkg/discovery/dns/resolver.go +++ b/pkg/discovery/dns/resolver.go @@ -78,11 +78,10 @@ func (s *dnsSD) Resolve(ctx context.Context, name string, qtype QType) ([]string } ips, err := s.resolver.LookupIPAddr(ctx, host) if err != nil { - dnsErr, ok := err.(*net.DNSError) // https://github.com/thanos-io/thanos/issues/3186 - // Default DNS resolver can make thanos components crash if DSN resolutions results in EAI_NONAME. + // Default DNS resolver can make thanos components crash if DNS resolutions results in EAI_NONAME. // the flag returnErrOnNotFound can be used to prevent such crash. - if !(!s.returnErrOnNotFound && ok && dnsErr.IsNotFound) { + if dnsErr, ok := err.(*net.DNSError); !ok || !dnsErr.IsNotFound || s.returnErrOnNotFound { return nil, errors.Wrapf(err, "lookup IP addresses %q", host) } level.Error(s.logger).Log("msg", "failed to lookup IP addresses", "host", host, "err", err) From b43907ec52484fa96a93468e4768f100703133b6 Mon Sep 17 00:00:00 2001 From: Kevin Hellemun <17928966+ogkevin@users.noreply.github.com> Date: Sun, 11 Oct 2020 12:56:02 +0200 Subject: [PATCH 8/9] Remove flag and make it default behaviour Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com> --- cmd/thanos/query.go | 6 ++---- cmd/thanos/rule.go | 6 ++---- pkg/cacheutil/memcached_client.go | 3 +-- pkg/discovery/dns/provider.go | 10 +--------- pkg/discovery/dns/provider_test.go | 2 +- pkg/discovery/dns/resolver.go | 23 ++++++++++------------- pkg/discovery/dns/resolver_test.go | 2 +- 7 files changed, 18 insertions(+), 34 deletions(-) diff --git a/cmd/thanos/query.go b/cmd/thanos/query.go index f8df4cf22b..4ce74da495 100644 --- a/cmd/thanos/query.go +++ b/cmd/thanos/query.go @@ -261,11 +261,10 @@ func runQuery( } fileSDCache := cache.New() - dnsStoreProvider := dns.NewProviderWithReturnOnErrorIfNotFound( + dnsStoreProvider := dns.NewProvider( logger, extprom.WrapRegistererWithPrefix("thanos_querier_store_apis_", reg), dns.ResolverType(dnsSDResolver), - true, ) for _, store := range strictStores { @@ -274,11 +273,10 @@ func runQuery( } } - dnsRuleProvider := dns.NewProviderWithReturnOnErrorIfNotFound( + dnsRuleProvider := dns.NewProvider( logger, extprom.WrapRegistererWithPrefix("thanos_querier_rule_apis_", reg), dns.ResolverType(dnsSDResolver), - true, ) var ( diff --git a/cmd/thanos/rule.go b/cmd/thanos/rule.go index 0c05559169..f72bbedfd2 100644 --- a/cmd/thanos/rule.go +++ b/cmd/thanos/rule.go @@ -334,11 +334,10 @@ func runRule( } } - queryProvider := dns.NewProviderWithReturnOnErrorIfNotFound( + queryProvider := dns.NewProvider( logger, extprom.WrapRegistererWithPrefix("thanos_ruler_query_apis_", reg), dns.ResolverType(dnsSDResolver), - true, ) var queryClients []*http_util.Client for _, cfg := range queryCfg { @@ -398,11 +397,10 @@ func runRule( level.Warn(logger).Log("msg", "no alertmanager configured") } - amProvider := dns.NewProviderWithReturnOnErrorIfNotFound( + amProvider := dns.NewProvider( logger, extprom.WrapRegistererWithPrefix("thanos_ruler_alertmanagers_", reg), dns.ResolverType(dnsSDResolver), - false, ) var alertmgrs []*alert.Alertmanager for _, cfg := range alertingCfg.Alertmanagers { diff --git a/pkg/cacheutil/memcached_client.go b/pkg/cacheutil/memcached_client.go index 93fcf7795c..15751109dc 100644 --- a/pkg/cacheutil/memcached_client.go +++ b/pkg/cacheutil/memcached_client.go @@ -215,11 +215,10 @@ func newMemcachedClient( config MemcachedClientConfig, reg prometheus.Registerer, ) (*memcachedClient, error) { - dnsProvider := dns.NewProviderWithReturnOnErrorIfNotFound( + dnsProvider := dns.NewProvider( logger, extprom.WrapRegistererWithPrefix("thanos_memcached_", reg), dns.GolangResolverType, - true, ) c := &memcachedClient{ diff --git a/pkg/discovery/dns/provider.go b/pkg/discovery/dns/provider.go index 097be1310b..9f72902a9e 100644 --- a/pkg/discovery/dns/provider.go +++ b/pkg/discovery/dns/provider.go @@ -53,20 +53,12 @@ func (t ResolverType) ToResolver(logger log.Logger) ipLookupResolver { return r } -// Deprecated. Use NewProviderWithReturnOnErrorIfNotFound instead. -// // NewProvider returns a new empty provider with a given resolver type. // If empty resolver type is net.DefaultResolver. // TODO(OGKevin): Refactor code to use new method and eventually rename NewProviderWithReturnOnErrorIfNotFound back to NewProvider. func NewProvider(logger log.Logger, reg prometheus.Registerer, resolverType ResolverType) *Provider { - return NewProviderWithReturnOnErrorIfNotFound(logger, reg, resolverType, true) -} - -// NewProviderWithReturnOnErrorIfNotFound returns a new empty provider with a given resolver type. -// If empty resolver type is net.DefaultResolver. -func NewProviderWithReturnOnErrorIfNotFound(logger log.Logger, reg prometheus.Registerer, resolverType ResolverType, returnErrOnNotFound bool) *Provider { p := &Provider{ - resolver: NewResolver(resolverType.ToResolver(logger), logger, returnErrOnNotFound), + resolver: NewResolver(resolverType.ToResolver(logger), logger), resolved: make(map[string][]string), logger: logger, resolverAddrs: extprom.NewTxGaugeVec(reg, prometheus.GaugeOpts{ diff --git a/pkg/discovery/dns/provider_test.go b/pkg/discovery/dns/provider_test.go index d342b925b1..668d5a65c8 100644 --- a/pkg/discovery/dns/provider_test.go +++ b/pkg/discovery/dns/provider_test.go @@ -22,7 +22,7 @@ func TestProvider(t *testing.T) { "127.0.0.5:19095", } - prv := NewProviderWithReturnOnErrorIfNotFound(log.NewNopLogger(), nil, "", true) + prv := NewProvider(log.NewNopLogger(), nil, "") prv.resolver = &mockResolver{ res: map[string][]string{ "a": ips[:2], diff --git a/pkg/discovery/dns/resolver.go b/pkg/discovery/dns/resolver.go index 780763195b..679834f7b2 100644 --- a/pkg/discovery/dns/resolver.go +++ b/pkg/discovery/dns/resolver.go @@ -42,14 +42,11 @@ type ipLookupResolver interface { type dnsSD struct { resolver ipLookupResolver logger log.Logger - // https://github.com/thanos-io/thanos/issues/3186 - // This flag is used to prevent components from crashing if hosts are not found. - returnErrOnNotFound bool } // NewResolver creates a resolver with given underlying resolver. -func NewResolver(resolver ipLookupResolver, logger log.Logger, returnErrOnNotFound bool) Resolver { - return &dnsSD{resolver: resolver, logger: logger, returnErrOnNotFound: returnErrOnNotFound} +func NewResolver(resolver ipLookupResolver, logger log.Logger) Resolver { + return &dnsSD{resolver: resolver, logger: logger} } func (s *dnsSD) Resolve(ctx context.Context, name string, qtype QType) ([]string, error) { @@ -78,13 +75,15 @@ func (s *dnsSD) Resolve(ctx context.Context, name string, qtype QType) ([]string } ips, err := s.resolver.LookupIPAddr(ctx, host) if err != nil { - // https://github.com/thanos-io/thanos/issues/3186 - // Default DNS resolver can make thanos components crash if DNS resolutions results in EAI_NONAME. - // the flag returnErrOnNotFound can be used to prevent such crash. - if dnsErr, ok := err.(*net.DNSError); !ok || !dnsErr.IsNotFound || s.returnErrOnNotFound { + // We exclude error from std Golang resolver for the case of the domain (e.g `NXDOMAIN`) not being found by DNS + // server. Since `miekg` does not consider this as an error, when the host cannot be found, empty slice will be + // returned. + if dnsErr, ok := err.(*net.DNSError); !ok || !dnsErr.IsNotFound { return nil, errors.Wrapf(err, "lookup IP addresses %q", host) } - level.Error(s.logger).Log("msg", "failed to lookup IP addresses", "host", host, "err", err) + if ips == nil { + level.Error(s.logger).Log("msg", "failed to lookup IP addresses", "host", host, "err", err) + } } for _, ip := range ips { res = append(res, appendScheme(scheme, net.JoinHostPort(ip.String(), port))) @@ -119,10 +118,8 @@ func (s *dnsSD) Resolve(ctx context.Context, name string, qtype QType) ([]string return nil, errors.Errorf("invalid lookup scheme %q", qtype) } - // https://github.com/thanos-io/thanos/issues/3186 - // This happens when miekg is used as resolver. When the host cannot be found, nothing is returned. if res == nil && err == nil { - level.Warn(s.logger).Log("msg", "IP address lookup yielded no results nor errors", "host", host) + level.Warn(s.logger).Log("msg", "IP address lookup yielded no results. No host found or no addresses found", "host", host) } return res, nil diff --git a/pkg/discovery/dns/resolver_test.go b/pkg/discovery/dns/resolver_test.go index a12a9be163..2eef7b4fbe 100644 --- a/pkg/discovery/dns/resolver_test.go +++ b/pkg/discovery/dns/resolver_test.go @@ -186,7 +186,7 @@ func TestDnsSD_Resolve(t *testing.T) { func testDnsSd(t *testing.T, tt DNSSDTest) { ctx := context.TODO() - dnsSD := dnsSD{tt.resolver, log.NewNopLogger(), false} + dnsSD := dnsSD{tt.resolver, log.NewNopLogger()} result, err := dnsSD.Resolve(ctx, tt.addr, tt.qtype) if tt.expectedErr != nil { From 19ab4a3bb0c719f8d6a563922c73792574fccc07 Mon Sep 17 00:00:00 2001 From: Kevin Hellemun <17928966+ogkevin@users.noreply.github.com> Date: Sun, 11 Oct 2020 13:01:53 +0200 Subject: [PATCH 9/9] Remove unneeded todo comment. Signed-off-by: Kevin Hellemun <17928966+OGKevin@users.noreply.github.com> --- pkg/discovery/dns/provider.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/discovery/dns/provider.go b/pkg/discovery/dns/provider.go index 9f72902a9e..75c3b02fd1 100644 --- a/pkg/discovery/dns/provider.go +++ b/pkg/discovery/dns/provider.go @@ -55,7 +55,6 @@ func (t ResolverType) ToResolver(logger log.Logger) ipLookupResolver { // NewProvider returns a new empty provider with a given resolver type. // If empty resolver type is net.DefaultResolver. -// TODO(OGKevin): Refactor code to use new method and eventually rename NewProviderWithReturnOnErrorIfNotFound back to NewProvider. func NewProvider(logger log.Logger, reg prometheus.Registerer, resolverType ResolverType) *Provider { p := &Provider{ resolver: NewResolver(resolverType.ToResolver(logger), logger),