From e5f75a8c3dfbc93a6d22a855a4146db94e0c90bd Mon Sep 17 00:00:00 2001 From: Zahari Dichev Date: Wed, 4 Dec 2019 08:12:01 +0200 Subject: [PATCH] Add validation to ensure stat time window is at least 15s (#3720) * Add stat time window minimum of 10s Signed-off-by: zaharidichev * Address comments Signed-off-by: zaharidichev --- cli/cmd/stat.go | 2 +- cli/cmd/stat_test.go | 12 ++++++++++++ controller/api/util/api_utils.go | 10 ++++++++-- 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/cli/cmd/stat.go b/cli/cmd/stat.go index f0b55134e..51805c8fe 100644 --- a/cli/cmd/stat.go +++ b/cli/cmd/stat.go @@ -169,7 +169,7 @@ If no resource name is specified, displays stats about all resources of the spec } cmd.PersistentFlags().StringVarP(&options.namespace, "namespace", "n", options.namespace, "Namespace of the specified resource") - cmd.PersistentFlags().StringVarP(&options.timeWindow, "time-window", "t", options.timeWindow, "Stat window (for example: \"10s\", \"1m\", \"10m\", \"1h\")") + cmd.PersistentFlags().StringVarP(&options.timeWindow, "time-window", "t", options.timeWindow, "Stat window (for example: \"15s\", \"1m\", \"10m\", \"1h\"). Needs to be at least 15s.") cmd.PersistentFlags().StringVar(&options.toResource, "to", options.toResource, "If present, restricts outbound stats to the specified resource name") cmd.PersistentFlags().StringVar(&options.toNamespace, "to-namespace", options.toNamespace, "Sets the namespace used to lookup the \"--to\" resource; by default the current \"--namespace\" is used") cmd.PersistentFlags().StringVar(&options.fromResource, "from", options.fromResource, "If present, restricts outbound stats from the specified resource name") diff --git a/cli/cmd/stat_test.go b/cli/cmd/stat_test.go index 9b85de04b..e77a8e8e1 100644 --- a/cli/cmd/stat_test.go +++ b/cli/cmd/stat_test.go @@ -191,6 +191,18 @@ func TestStat(t *testing.T) { t.Fatalf("Expected error [%s] instead got [%s]", expectedError, err) } }) + + t.Run("Returns an error if --time-window is not more than 15s", func(t *testing.T) { + options := newStatOptions() + options.timeWindow = "10s" + args := []string{"ns/bar"} + expectedError := "metrics time window needs to be at least 15s" + + _, err := buildStatSummaryRequests(args, options) + if err == nil || err.Error() != expectedError { + t.Fatalf("Expected error [%s] instead got [%s]", expectedError, err) + } + }) } func testStatCall(exp paramsExp, resourceType string, t *testing.T) { diff --git a/controller/api/util/api_utils.go b/controller/api/util/api_utils.go index cc8d8f25a..00f302520 100644 --- a/controller/api/util/api_utils.go +++ b/controller/api/util/api_utils.go @@ -21,7 +21,8 @@ import ( */ var ( - defaultMetricTimeWindow = "1m" + defaultMetricTimeWindow = "1m" + metricTimeWindowLowerBound = time.Second * 15 //the window value needs to equal or larger than that // ValidTargets specifies resource types allowed as a target: // target resource on an inbound query @@ -145,10 +146,15 @@ func GRPCError(err error) error { func BuildStatSummaryRequest(p StatsSummaryRequestParams) (*pb.StatSummaryRequest, error) { window := defaultMetricTimeWindow if p.TimeWindow != "" { - _, err := time.ParseDuration(p.TimeWindow) + w, err := time.ParseDuration(p.TimeWindow) if err != nil { return nil, err } + + if w < metricTimeWindowLowerBound { + return nil, errors.New("metrics time window needs to be at least 15s") + } + window = p.TimeWindow }