From 17c0f14a20564a52df8da0f6fb819510cb520558 Mon Sep 17 00:00:00 2001 From: Saswata Mukherjee Date: Wed, 24 Nov 2021 12:22:05 +0530 Subject: [PATCH 1/4] Add validation for querier address flags Signed-off-by: Saswata Mukherjee --- cmd/thanos/query.go | 49 ++++++++++++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 18 deletions(-) diff --git a/cmd/thanos/query.go b/cmd/thanos/query.go index 518729de57..6c247fb5f6 100644 --- a/cmd/thanos/query.go +++ b/cmd/thanos/query.go @@ -191,20 +191,24 @@ func registerQuery(app *extkingpin.App) { } } - if dup := firstDuplicate(*stores); dup != "" { - return errors.Errorf("Address %s is duplicated for --store flag.", dup) + if err := validateAddrs(*endpoints); err != nil { + return errors.Wrap(err, "validating --endpoint flags") } - if dup := firstDuplicate(*ruleEndpoints); dup != "" { - return errors.Errorf("Address %s is duplicated for --rule flag.", dup) + if err := validateAddrs(*stores); err != nil { + return errors.Wrap(err, "validating --store flags") } - if dup := firstDuplicate(*metadataEndpoints); dup != "" { - return errors.Errorf("Address %s is duplicated for --metadata flag.", dup) + if err := validateAddrs(*ruleEndpoints); err != nil { + return errors.Wrap(err, "validating --rule flags") } - if dup := firstDuplicate(*exemplarEndpoints); dup != "" { - return errors.Errorf("Address %s is duplicated for --exemplar flag.", dup) + if err := validateAddrs(*metadataEndpoints); err != nil { + return errors.Wrap(err, "validating --metadata flags") + } + + if err := validateAddrs(*exemplarEndpoints); err != nil { + return errors.Wrap(err, "validating --exemplar flags") } httpLogOpts, err := logging.ParseHTTPOptions(*reqLogDecision, reqLogConfig) @@ -217,8 +221,8 @@ func registerQuery(app *extkingpin.App) { return errors.Wrap(err, "error while parsing config for request logging") } - if dup := firstDuplicate(*targetEndpoints); dup != "" { - return errors.Errorf("Address %s is duplicated for --target flag.", dup) + if err := validateAddrs(*targetEndpoints); err != nil { + return errors.Wrap(err, "validating --target flags") } var fileSD *file.Discovery @@ -733,20 +737,29 @@ func removeDuplicateEndpointSpecs(logger log.Logger, duplicatedStores prometheus return deduplicated } -// firstDuplicate returns the first duplicate string in the given string slice -// or empty string if none was found. -func firstDuplicate(ss []string) string { +// validateAddrs checks an address slice for duplicates and empty or invalid elements. +func validateAddrs(addrs []string) error { set := map[string]struct{}{} - for _, s := range ss { - if _, ok := set[s]; ok { - return s + for _, addr := range addrs { + if addr == "" { + return errors.New("Address is empty.") + } + + qtypeAndName := strings.SplitN(addr, "+", 2) + hostAndPort := strings.SplitN(addr, ":", 2) + if len(qtypeAndName) != 2 && len(hostAndPort) != 2 { + return errors.Errorf("Address %s is not of : format or a valid DNS query.", addr) } - set[s] = struct{}{} + if _, ok := set[addr]; ok { + return errors.Errorf("Address %s is duplicated.", addr) + } + + set[addr] = struct{}{} } - return "" + return nil } // engineFactory creates from 1 to 3 promql.Engines depending on From df7c6430e6746880cf413503b935e65b403557ad Mon Sep 17 00:00:00 2001 From: Saswata Mukherjee Date: Wed, 24 Nov 2021 13:50:15 +0530 Subject: [PATCH 2/4] Add CHANGELOG Signed-off-by: Saswata Mukherjee --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b52eeab9a..a284df8208 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re - [#4856](https://github.com/thanos-io/thanos/pull/4856) Mixin: Add Query Frontend Grafana dashboard. - [#4874](https://github.com/thanos-io/thanos/pull/4874) Query: Add `--endpoint-strict` flag to statically configure Thanos API server endpoints. It is similar to `--store-strict` but supports passing any Thanos gRPC APIs: StoreAPI, MetadataAPI, RulesAPI, TargetsAPI and ExemplarsAPI. - [#4868](https://github.com/thanos-io/thanos/pull/4868) Rule: Support ruleGroup limit introduced by Prometheus v2.31.0. +- [#4897](https://github.com/thanos-io/thanos/pull/4897) Querier: Add validation for querier address flags. ### Fixed From 9a5d3f777986e51af4176fa3166c2d5ab0471e95 Mon Sep 17 00:00:00 2001 From: Saswata Mukherjee Date: Wed, 24 Nov 2021 13:50:38 +0530 Subject: [PATCH 3/4] Fix test Signed-off-by: Saswata Mukherjee --- test/e2e/info_api_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/test/e2e/info_api_test.go b/test/e2e/info_api_test.go index 71226e8f86..233610dbb4 100644 --- a/test/e2e/info_api_test.go +++ b/test/e2e/info_api_test.go @@ -74,7 +74,6 @@ func TestInfo(t *testing.T) { sidecar1.InternalEndpoint("grpc"), sidecar2.InternalEndpoint("grpc"), sidecar3.InternalEndpoint("grpc"), - sidecar3.InternalEndpoint("grpc"), store.InternalEndpoint("grpc"), ). Build() From 1bcb0fdadf6efbc984a47ae4be4f3e4a6d80d1ad Mon Sep 17 00:00:00 2001 From: Saswata Mukherjee Date: Thu, 25 Nov 2021 18:26:42 +0530 Subject: [PATCH 4/4] Change to custom kingpin parser Signed-off-by: Saswata Mukherjee --- cmd/thanos/query.go | 73 +++++++---------------------------------- pkg/extkingpin/flags.go | 53 ++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 61 deletions(-) diff --git a/cmd/thanos/query.go b/cmd/thanos/query.go index 6c247fb5f6..3fcf31b542 100644 --- a/cmd/thanos/query.go +++ b/cmd/thanos/query.go @@ -104,25 +104,25 @@ func registerQuery(app *extkingpin.App) { selectorLabels := cmd.Flag("selector-label", "Query selector labels that will be exposed in info endpoint (repeated)."). PlaceHolder("=\"\"").Strings() - endpoints := cmd.Flag("endpoint", "Addresses of statically configured Thanos API servers (repeatable). The scheme may be prefixed with 'dns+' or 'dnssrv+' to detect Thanos API servers through respective DNS lookups."). - PlaceHolder("").Strings() + endpoints := extkingpin.Addrs(cmd.Flag("endpoint", "Addresses of statically configured Thanos API servers (repeatable). The scheme may be prefixed with 'dns+' or 'dnssrv+' to detect Thanos API servers through respective DNS lookups."). + PlaceHolder("")) - stores := cmd.Flag("store", "Deprecation Warning - This flag is deprecated and replaced with `endpoint`. Addresses of statically configured store API servers (repeatable). The scheme may be prefixed with 'dns+' or 'dnssrv+' to detect store API servers through respective DNS lookups."). - PlaceHolder("").Strings() + stores := extkingpin.Addrs(cmd.Flag("store", "Deprecation Warning - This flag is deprecated and replaced with `endpoint`. Addresses of statically configured store API servers (repeatable). The scheme may be prefixed with 'dns+' or 'dnssrv+' to detect store API servers through respective DNS lookups."). + PlaceHolder("")) // TODO(bwplotka): Hidden because we plan to extract discovery to separate API: https://github.com/thanos-io/thanos/issues/2600. - ruleEndpoints := cmd.Flag("rule", "Deprecation Warning - This flag is deprecated and replaced with `endpoint`. Experimental: Addresses of statically configured rules API servers (repeatable). The scheme may be prefixed with 'dns+' or 'dnssrv+' to detect rule API servers through respective DNS lookups."). - Hidden().PlaceHolder("").Strings() + ruleEndpoints := extkingpin.Addrs(cmd.Flag("rule", "Deprecation Warning - This flag is deprecated and replaced with `endpoint`. Experimental: Addresses of statically configured rules API servers (repeatable). The scheme may be prefixed with 'dns+' or 'dnssrv+' to detect rule API servers through respective DNS lookups."). + Hidden().PlaceHolder("")) - metadataEndpoints := cmd.Flag("metadata", "Deprecation Warning - This flag is deprecated and replaced with `endpoint`. Experimental: Addresses of statically configured metadata API servers (repeatable). The scheme may be prefixed with 'dns+' or 'dnssrv+' to detect metadata API servers through respective DNS lookups."). - Hidden().PlaceHolder("").Strings() + metadataEndpoints := extkingpin.Addrs(cmd.Flag("metadata", "Deprecation Warning - This flag is deprecated and replaced with `endpoint`. Experimental: Addresses of statically configured metadata API servers (repeatable). The scheme may be prefixed with 'dns+' or 'dnssrv+' to detect metadata API servers through respective DNS lookups."). + Hidden().PlaceHolder("")) - exemplarEndpoints := cmd.Flag("exemplar", "Deprecation Warning - This flag is deprecated and replaced with `endpoint`. Experimental: Addresses of statically configured exemplars API servers (repeatable). The scheme may be prefixed with 'dns+' or 'dnssrv+' to detect exemplars API servers through respective DNS lookups."). - Hidden().PlaceHolder("").Strings() + exemplarEndpoints := extkingpin.Addrs(cmd.Flag("exemplar", "Deprecation Warning - This flag is deprecated and replaced with `endpoint`. Experimental: Addresses of statically configured exemplars API servers (repeatable). The scheme may be prefixed with 'dns+' or 'dnssrv+' to detect exemplars API servers through respective DNS lookups."). + Hidden().PlaceHolder("")) // TODO(atunik): Hidden because we plan to extract discovery to separate API: https://github.com/thanos-io/thanos/issues/2600. - targetEndpoints := cmd.Flag("target", "Deprecation Warning - This flag is deprecated and replaced with `endpoint`. Experimental: Addresses of statically configured target API servers (repeatable). The scheme may be prefixed with 'dns+' or 'dnssrv+' to detect target API servers through respective DNS lookups."). - Hidden().PlaceHolder("").Strings() + targetEndpoints := extkingpin.Addrs(cmd.Flag("target", "Deprecation Warning - This flag is deprecated and replaced with `endpoint`. Experimental: Addresses of statically configured target API servers (repeatable). The scheme may be prefixed with 'dns+' or 'dnssrv+' to detect target API servers through respective DNS lookups."). + Hidden().PlaceHolder("")) strictStores := cmd.Flag("store-strict", "Deprecation Warning - This flag is deprecated and replaced with `endpoint-strict`. Addresses of only statically configured store API servers that are always used, even if the health check fails. Useful if you have a caching layer on top."). PlaceHolder("").Strings() @@ -191,26 +191,6 @@ func registerQuery(app *extkingpin.App) { } } - if err := validateAddrs(*endpoints); err != nil { - return errors.Wrap(err, "validating --endpoint flags") - } - - if err := validateAddrs(*stores); err != nil { - return errors.Wrap(err, "validating --store flags") - } - - if err := validateAddrs(*ruleEndpoints); err != nil { - return errors.Wrap(err, "validating --rule flags") - } - - if err := validateAddrs(*metadataEndpoints); err != nil { - return errors.Wrap(err, "validating --metadata flags") - } - - if err := validateAddrs(*exemplarEndpoints); err != nil { - return errors.Wrap(err, "validating --exemplar flags") - } - httpLogOpts, err := logging.ParseHTTPOptions(*reqLogDecision, reqLogConfig) if err != nil { return errors.Wrap(err, "error while parsing config for request logging") @@ -221,10 +201,6 @@ func registerQuery(app *extkingpin.App) { return errors.Wrap(err, "error while parsing config for request logging") } - if err := validateAddrs(*targetEndpoints); err != nil { - return errors.Wrap(err, "validating --target flags") - } - var fileSD *file.Discovery if len(*fileSDFiles) > 0 { conf := &file.SDConfig{ @@ -737,31 +713,6 @@ func removeDuplicateEndpointSpecs(logger log.Logger, duplicatedStores prometheus return deduplicated } -// validateAddrs checks an address slice for duplicates and empty or invalid elements. -func validateAddrs(addrs []string) error { - set := map[string]struct{}{} - - for _, addr := range addrs { - if addr == "" { - return errors.New("Address is empty.") - } - - qtypeAndName := strings.SplitN(addr, "+", 2) - hostAndPort := strings.SplitN(addr, ":", 2) - if len(qtypeAndName) != 2 && len(hostAndPort) != 2 { - return errors.Errorf("Address %s is not of : format or a valid DNS query.", addr) - } - - if _, ok := set[addr]; ok { - return errors.Errorf("Address %s is duplicated.", addr) - } - - set[addr] = struct{}{} - } - - return nil -} - // engineFactory creates from 1 to 3 promql.Engines depending on // dynamicLookbackDelta and eo.LookbackDelta and returns a function // that returns appropriate engine for given maxSourceResolutionMillis. diff --git a/pkg/extkingpin/flags.go b/pkg/extkingpin/flags.go index a7468b83a9..8f1f1c7608 100644 --- a/pkg/extkingpin/flags.go +++ b/pkg/extkingpin/flags.go @@ -9,6 +9,7 @@ import ( "time" extflag "github.com/efficientgo/tools/extkingpin" + "github.com/pkg/errors" "github.com/prometheus/common/model" "gopkg.in/alecthomas/kingpin.v2" ) @@ -20,6 +21,58 @@ func ModelDuration(flags *kingpin.FlagClause) *model.Duration { return value } +// Custom parser for IP address flags. +type addressSlice []string + +// addressSlice conforms to flag.Value interface. +func (a *addressSlice) Set(value string) error { + *a = append(*a, value) + if err := validateAddrs(*a); err != nil { + return err + } + return nil +} + +func (a *addressSlice) String() string { + return strings.Join(*a, ",") +} + +// Ensure flag is repeatable. +func (a *addressSlice) IsCumulative() bool { + return true +} + +func Addrs(flags *kingpin.FlagClause) (target *addressSlice) { + target = &addressSlice{} + flags.SetValue((*addressSlice)(target)) + return +} + +// validateAddrs checks an address slice for duplicates and empty or invalid elements. +func validateAddrs(addrs addressSlice) error { + set := map[string]struct{}{} + + for _, addr := range addrs { + if addr == "" { + return errors.New("Address is empty.") + } + + qtypeAndName := strings.SplitN(addr, "+", 2) + hostAndPort := strings.SplitN(addr, ":", 2) + if len(qtypeAndName) != 2 && len(hostAndPort) != 2 { + return errors.Errorf("Address %s is not of : format or a valid DNS query.", addr) + } + + if _, ok := set[addr]; ok { + return errors.Errorf("Address %s is duplicated.", addr) + } + + set[addr] = struct{}{} + } + + return nil +} + // RegisterGRPCFlags registers flags commonly used to configure gRPC servers with. func RegisterGRPCFlags(cmd FlagClause) ( grpcBindAddr *string,