From 7364dc75b61444006db5967cd3c1329f897e9a83 Mon Sep 17 00:00:00 2001 From: Nitin Kumar Date: Fri, 23 Oct 2026 08:45:58 +0530 Subject: [PATCH] fix(aws-cloudwatch): validate metric Period on GetMetricData and GetMetricStatistics An invalid or huge Period overflowed the bucket step and crashed the server with a divide by zero. Period now follows the AWS rule and returns InvalidParameterValue, buckets are counted in int64 seconds, and the math alarm lookback check no longer overflows. --- providers/aws/cloudwatch/cloudwatch.go | 24 ++- providers/aws/cloudwatch/period.go | 52 +++++ providers/aws/cloudwatch/period_test.go | 84 ++++++++ .../aws/cloudwatch/core_alarm_statistics.go | 27 ++- server/aws/cloudwatch/core_metric_data.go | 51 ++++- server/aws/cloudwatch/core_metrics.go | 9 +- .../metric_period_validation_test.go | 193 ++++++++++++++++++ 7 files changed, 428 insertions(+), 12 deletions(-) create mode 100644 providers/aws/cloudwatch/period.go create mode 100644 providers/aws/cloudwatch/period_test.go create mode 100644 server/aws/cloudwatch/metric_period_validation_test.go diff --git a/providers/aws/cloudwatch/cloudwatch.go b/providers/aws/cloudwatch/cloudwatch.go index 79ac744a4..db2b671fd 100644 --- a/providers/aws/cloudwatch/cloudwatch.go +++ b/providers/aws/cloudwatch/cloudwatch.go @@ -178,6 +178,14 @@ func (m *Mock) PutMetricData(ctx context.Context, data []driver.MetricDatum) err // //nolint:gocritic // hugeParam: interface method signature cannot be changed. func (m *Mock) GetMetricData(_ context.Context, input driver.GetMetricInput) (*driver.MetricDataResult, error) { + // Zero keeps meaning the one-minute default. Any other value follows the + // AWS Period rule, so it cannot overflow the bucket step. + if input.Period != 0 { + if err := m.ValidatePeriod("Period", input.Period); err != nil { + return nil, err + } + } + return m.readMetric(&input), nil } @@ -200,7 +208,7 @@ func (m *Mock) readMetric(input *driver.GetMetricInput) *driver.MetricDataResult period := input.Period if period <= 0 { - period = 60 + period = defaultPeriodSeconds } return buildMetricResult(filtered, input.StartTime, period, input.Stat) @@ -272,20 +280,26 @@ func buildMetricResult(filtered []driver.MetricDatum, startTime time.Time, perio // unit reads back as None, like on AWS. result.Unit = alarmeval.EffectiveUnit(unitOf(filtered)) - periodDur := time.Duration(period) * time.Second + // Count in whole seconds, so a long period cannot overflow a Duration. + step := int64(period) + if step <= 0 { + return result + } + + bucket := func(ts time.Time) int64 { return int64(ts.Sub(startTime)/time.Second) / step } for from := 0; from < len(filtered); { - idx := filtered[from].Timestamp.Sub(startTime) / periodDur + idx := bucket(filtered[from].Timestamp) to := from + 1 - for to < len(filtered) && filtered[to].Timestamp.Sub(startTime)/periodDur == idx { + for to < len(filtered) && bucket(filtered[to].Timestamp) == idx { to++ } // A period where an extended statistic is not available, such as a // percentile over negative values, is left out as on AWS. if v, ok := alarmeval.StatValue(filtered[from:to], stat); ok { - result.Timestamps = append(result.Timestamps, startTime.Add(idx*periodDur)) + result.Timestamps = append(result.Timestamps, startTime.Add(time.Duration(idx*step)*time.Second)) result.Values = append(result.Values, v) } diff --git a/providers/aws/cloudwatch/period.go b/providers/aws/cloudwatch/period.go new file mode 100644 index 000000000..93558517f --- /dev/null +++ b/providers/aws/cloudwatch/period.go @@ -0,0 +1,52 @@ +package cloudwatch + +import ( + "math" + "slices" + + "github.com/stackshy/cloudemu/v2/errors" +) + +// Period rules from the MetricStat, MetricDataQuery and GetMetricStatistics +// API references. Period is an Integer of at least 1. A regular resolution +// period is a multiple of 60, and a high-resolution metric also allows 1, 5, +// 10, 20 and 30. +const ( + defaultPeriodSeconds = 60 + maxPeriodSeconds = math.MaxInt32 + periodSecondsInMinute = 60 +) + +// validPeriod reports whether p is 1, 5, 10, 20, 30 or a multiple of 60 that +// fits the API's Integer type. +func validPeriod(p int) bool { + switch { + case p < 1 || p > maxPeriodSeconds: + return false + case p%periodSecondsInMinute == 0: + return true + default: + periods := highResolutionPeriods() + + return slices.Contains(periods[:], p) + } +} + +// highResolutionPeriods are the periods under a minute that a high-resolution +// metric allows. +func highResolutionPeriods() [5]int { + return [5]int{1, 5, 10, 20, 30} +} + +// ValidatePeriod returns an InvalidArgument error when period is not a valid +// metric period. param names the request field in the message, such as +// "MetricDataQueries.member.1.MetricStat.Period". +func (*Mock) ValidatePeriod(param string, period int) error { + if validPeriod(period) { + return nil + } + + return errors.Newf(errors.InvalidArgument, + "The value %d for parameter %s is not a valid period. Valid periods are 1, 5, 10, 20, 30, 60 and any multiple of 60.", + period, param) +} diff --git a/providers/aws/cloudwatch/period_test.go b/providers/aws/cloudwatch/period_test.go new file mode 100644 index 000000000..345c050ac --- /dev/null +++ b/providers/aws/cloudwatch/period_test.go @@ -0,0 +1,84 @@ +package cloudwatch + +import ( + "context" + "strings" + "testing" + "time" + + cerrors "github.com/stackshy/cloudemu/v2/errors" + "github.com/stackshy/cloudemu/v2/services/monitoring/driver" +) + +func TestValidatePeriod(t *testing.T) { + m := newTestMock() + + for _, p := range []int{1, 5, 10, 20, 30, 60, 120, 300, 3600, 86400, 2147483640} { + if err := m.ValidatePeriod("Period", p); err != nil { + t.Errorf("period %d: unexpected error %v", p, err) + } + } + + for _, p := range []int{0, -1, -60, 2, 15, 45, 61, 90, 2147483700, 540431955284459520} { + err := m.ValidatePeriod("MetricDataQueries.member.1.MetricStat.Period", p) + if !cerrors.IsInvalidArgument(err) { + t.Fatalf("period %d: want InvalidArgument, got %v", p, err) + } + + if !strings.Contains(cerrors.Message(err), "parameter MetricDataQueries.member.1.MetricStat.Period is not a valid period") { + t.Errorf("period %d: message should name the parameter: %s", p, cerrors.Message(err)) + } + } +} + +// TestGetMetricDataRejectsBadPeriod covers a Period that used to overflow the +// bucket step to zero (divide by zero) or to a negative value. +func TestGetMetricDataRejectsBadPeriod(t *testing.T) { + m := newTestMock() + ctx := context.Background() + now := m.opts.Clock.Now() + + requireNoError(t, m.PutMetricData(ctx, []driver.MetricDatum{ + {Namespace: "App", MetricName: "Latency", Value: 5, Timestamp: now.Add(-time.Minute)}, + })) + + in := driver.GetMetricInput{ + Namespace: "App", MetricName: "Latency", StartTime: now.Add(-time.Hour), EndTime: now, Stat: "Sum", + } + + for _, p := range []int{-60, 61, 540431955284459520, 9223372036854775800} { + in.Period = p + + if _, err := m.GetMetricData(ctx, in); !cerrors.IsInvalidArgument(err) { + t.Fatalf("period %d: want InvalidArgument, got %v", p, err) + } + } + + in.Period = 0 + + res, err := m.GetMetricData(ctx, in) + requireNoError(t, err) + + if len(res.Values) != 1 || res.Values[0] != 5 { + t.Fatalf("zero period should default to a minute: %+v", res) + } +} + +// TestBuildMetricResultLongPeriod checks that a period longer than a +// Duration can hold still buckets every point into one period. +func TestBuildMetricResultLongPeriod(t *testing.T) { + start := time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC) + data := []driver.MetricDatum{ + {Value: 1, Timestamp: start.Add(time.Hour)}, + {Value: 2, Timestamp: start.Add(48 * time.Hour)}, + } + + res := buildMetricResult(data, start, 540431955284459520, "Sum") + if len(res.Values) != 1 || res.Values[0] != 3 || !res.Timestamps[0].Equal(start) { + t.Fatalf("want one bucket of 3 at start, got %+v", res) + } + + if res := buildMetricResult(data, start, 0, "Sum"); len(res.Values) != 0 { + t.Fatalf("a zero step should return no points, got %+v", res) + } +} diff --git a/server/aws/cloudwatch/core_alarm_statistics.go b/server/aws/cloudwatch/core_alarm_statistics.go index 59ff4dae0..007cc2275 100644 --- a/server/aws/cloudwatch/core_alarm_statistics.go +++ b/server/aws/cloudwatch/core_alarm_statistics.go @@ -67,7 +67,19 @@ func validateAlarmPeriods(cfg *mondriver.AlarmConfig) error { return newWireError(errValidation, "DatapointsToAlarm must be less than or equal to EvaluationPeriods.") } - if len(cfg.Metrics) > 0 || cfg.Period == 0 { + // validateQueryShape checks the MetricStat periods of a metric math + // alarm. Each one is held to the same lookback cap. + if len(cfg.Metrics) > 0 { + for _, p := range alarmPeriods(cfg) { + if err := validateAlarmLookback(p, cfg.EvaluationPeriods); err != nil { + return err + } + } + + return nil + } + + if cfg.Period == 0 { return nil } @@ -79,15 +91,20 @@ func validateAlarmPeriods(cfg *mondriver.AlarmConfig) error { } // validateAlarmLookback checks EvaluationPeriods * Period against the day or -// week cap that applies to period p. +// week cap that applies to period p. It divides instead of multiplying, so a +// huge Period or EvaluationPeriods cannot overflow past the cap. func validateAlarmLookback(p, evaluationPeriods int) error { - lookback := p * max(evaluationPeriods, 1) + if p <= 0 { + return nil + } + + n := max(evaluationPeriods, 1) switch { - case p < hourPeriod && lookback > maxAlarmLookbackDay: + case p < hourPeriod && n > maxAlarmLookbackDay/p: return newWireError(errValidation, "Metrics cannot be checked across more than a day "+ "(EvaluationPeriods * Period must be <= 86400)") - case p >= hourPeriod && lookback > maxAlarmLookbackWeek: + case p >= hourPeriod && n > maxAlarmLookbackWeek/p: return newWireError(errValidation, "Metrics cannot be checked across more than a week "+ "(EvaluationPeriods * Period must be <= 604800)") } diff --git a/server/aws/cloudwatch/core_metric_data.go b/server/aws/cloudwatch/core_metric_data.go index a1d8ed7f7..ae0cdc0f1 100644 --- a/server/aws/cloudwatch/core_metric_data.go +++ b/server/aws/cloudwatch/core_metric_data.go @@ -3,8 +3,10 @@ package cloudwatch import ( "context" "sort" + "strconv" "time" + cerrors "github.com/stackshy/cloudemu/v2/errors" mondriver "github.com/stackshy/cloudemu/v2/services/monitoring/driver" "github.com/stackshy/cloudemu/v2/services/monitoring/metricmath" ) @@ -57,6 +59,10 @@ func (h *Handler) getMetricDataCore(ctx context.Context, in *getMetricDataInput) return getMetricDataResult{}, err } + if err := h.validateQueryPeriods(in.MetricDataQueries); err != nil { + return getMetricDataResult{}, err + } + queries := toDriverQueries(in.MetricDataQueries) eval := h.metricDataEvaluator(ctx, queries, timeOrZero(in.StartTime), timeOrZero(in.EndTime)) rows := make([]metricDataRow, 0, len(in.MetricDataQueries)) @@ -71,7 +77,7 @@ func (h *Handler) getMetricDataCore(ctx context.Context, in *getMetricDataInput) qrows, err := metricDataRows(eval, q, descending) if err != nil { - return getMetricDataResult{}, err + return getMetricDataResult{}, invalidParameterError(err) } rows = append(rows, qrows...) @@ -82,6 +88,49 @@ func (h *Handler) getMetricDataCore(ctx context.Context, in *getMetricDataInput) return getMetricDataResult{Rows: page, NextToken: next}, nil } +// periodValidator is the AWS-local capability that checks a metric Period. +// param names the request field in the error message. +type periodValidator interface { + ValidatePeriod(param string, period int) error +} + +// validateQueryPeriods checks the MetricStat Period and the optional Period +// of every query, including ones that return no data. +func (h *Handler) validateQueryPeriods(queries []metricDataQueryCBR) error { + v, ok := h.monitoring.(periodValidator) + if !ok { + return nil + } + + for i := range queries { + member := "MetricDataQueries.member." + strconv.Itoa(i+1) + + if ms := queries[i].MetricStat; ms != nil { + if err := v.ValidatePeriod(member+".MetricStat.Period", ms.Period); err != nil { + return invalidParameterError(err) + } + } + + if p := queries[i].Period; p != 0 { + if err := v.ValidatePeriod(member+".Period", p); err != nil { + return invalidParameterError(err) + } + } + } + + return nil +} + +// invalidParameterError maps a driver InvalidArgument error to the +// InvalidParameterValue wire error and passes any other error through. +func invalidParameterError(err error) error { + if cerrors.IsInvalidArgument(err) { + return newWireError(errInvalidParameterValue, cerrors.Message(err)) + } + + return err +} + // bandExclusionSource is the AWS-local capability that returns the excluded // training ranges of the detector behind a band. type bandExclusionSource interface { diff --git a/server/aws/cloudwatch/core_metrics.go b/server/aws/cloudwatch/core_metrics.go index 5c3d69227..6ac0ed6d2 100644 --- a/server/aws/cloudwatch/core_metrics.go +++ b/server/aws/cloudwatch/core_metrics.go @@ -306,6 +306,13 @@ func (h *Handler) getMetricStatisticsCore( return getMetricStatisticsResult{}, err } + // Period is required, so zero is out of range here. + if v, ok := h.monitoring.(periodValidator); ok { + if err := v.ValidatePeriod("Period", in.Period); err != nil { + return getMetricStatisticsResult{}, invalidParameterError(err) + } + } + // Callers often ask for several statistics at once and expect all of them // on each datapoint. stats := requestedStats(in) @@ -335,7 +342,7 @@ func (h *Handler) getMetricStatisticsCore( res, err := h.monitoring.GetMetricData(ctx, q) if err != nil { - return getMetricStatisticsResult{}, err + return getMetricStatisticsResult{}, invalidParameterError(err) } acc.add(res, stat, unit) diff --git a/server/aws/cloudwatch/metric_period_validation_test.go b/server/aws/cloudwatch/metric_period_validation_test.go new file mode 100644 index 000000000..412ef9f11 --- /dev/null +++ b/server/aws/cloudwatch/metric_period_validation_test.go @@ -0,0 +1,193 @@ +package cloudwatch_test + +import ( + "io" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + "time" + + "github.com/stackshy/cloudemu/v2/config" + cwprovider "github.com/stackshy/cloudemu/v2/providers/aws/cloudwatch" + cwserver "github.com/stackshy/cloudemu/v2/server/aws/cloudwatch" +) + +// periodRequestTimeout bounds each request, so a Period that sends the server +// into an endless loop fails the test instead of hanging it. +const periodRequestTimeout = 10 * time.Second + +// newPeriodPoster is newQueryPoster with a client timeout. It stores one +// datum of App/Latency so GetMetricData has points to bucket. +func newPeriodPoster(t *testing.T) func(url.Values) (int, string) { + t.Helper() + + ts := httptest.NewServer(cwserver.New(cwprovider.New(config.NewOptions()))) + t.Cleanup(ts.Close) + + client := &http.Client{Timeout: periodRequestTimeout} + + post := func(form url.Values) (int, string) { + req, _ := http.NewRequest(http.MethodPost, ts.URL+"/", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.Header.Set("Authorization", monitoringAuth) + + resp, err := client.Do(req) + if err != nil { + t.Fatalf("post %s: %v", form.Get("Action"), err) + } + defer resp.Body.Close() + + b, _ := io.ReadAll(resp.Body) + + return resp.StatusCode, string(b) + } + + if code, body := post(url.Values{ + "Action": {"PutMetricData"}, "Namespace": {"App"}, + "MetricData.member.1.MetricName": {"Latency"}, "MetricData.member.1.Value": {"5"}, + "MetricData.member.1.Timestamp": {time.Now().Add(-10 * time.Minute).UTC().Format(time.RFC3339)}, + }); code != http.StatusOK { + t.Fatalf("PutMetricData: code=%d body=%s", code, body) + } + + return post +} + +func periodMetricDataForm(statPeriod, exprPeriod string) url.Values { + form := url.Values{ + "Action": {"GetMetricData"}, + "StartTime": {time.Now().Add(-time.Hour).UTC().Format(time.RFC3339)}, + "EndTime": {time.Now().UTC().Format(time.RFC3339)}, + "MetricDataQueries.member.1.Id": {"m1"}, + "MetricDataQueries.member.1.MetricStat.Metric.Namespace": {"App"}, + "MetricDataQueries.member.1.MetricStat.Metric.MetricName": {"Latency"}, + "MetricDataQueries.member.1.MetricStat.Period": {statPeriod}, + "MetricDataQueries.member.1.MetricStat.Stat": {"Sum"}, + } + + if exprPeriod != "" { + form.Set("MetricDataQueries.member.2.Id", "e1") + form.Set("MetricDataQueries.member.2.Expression", "m1 * 2") + form.Set("MetricDataQueries.member.2.Period", exprPeriod) + } + + return form +} + +// TestGetMetricDataPeriodValidation checks the Period rule of MetricStat and +// MetricDataQuery: at least 1, and 1, 5, 10, 20, 30, 60 or a multiple of 60. +// A huge Period used to overflow the bucket step and crash or hang the server. +func TestGetMetricDataPeriodValidation(t *testing.T) { + post := newPeriodPoster(t) + + bad := []struct { + name, statPeriod, exprPeriod, param string + }{ + {"huge multiple of 60 overflowing to a zero step", "540431955284459520", "", "MetricDataQueries.member.1.MetricStat.Period"}, + {"huge near int64 max", "9223372036854775800", "", "MetricDataQueries.member.1.MetricStat.Period"}, + {"above int32", "2147483700", "", "MetricDataQueries.member.1.MetricStat.Period"}, + {"zero", "0", "", "MetricDataQueries.member.1.MetricStat.Period"}, + {"negative", "-60", "", "MetricDataQueries.member.1.MetricStat.Period"}, + {"not a multiple of 60", "61", "", "MetricDataQueries.member.1.MetricStat.Period"}, + {"high resolution value not allowed", "15", "", "MetricDataQueries.member.1.MetricStat.Period"}, + {"negative expression period", "60", "-60", "MetricDataQueries.member.2.Period"}, + {"huge expression period", "60", "540431955284459520", "MetricDataQueries.member.2.Period"}, + } + + for _, tc := range bad { + t.Run(tc.name, func(t *testing.T) { + code, body := post(periodMetricDataForm(tc.statPeriod, tc.exprPeriod)) + if code != http.StatusBadRequest || !strings.Contains(body, "InvalidParameterValue") { + t.Fatalf("want 400 InvalidParameterValue, got code=%d body=%s", code, body) + } + + if !strings.Contains(body, "parameter "+tc.param+" is not a valid period") { + t.Fatalf("message should name %s: %s", tc.param, body) + } + }) + } + + for _, p := range []string{"1", "5", "10", "20", "30", "60", "300", "86400"} { + t.Run("valid "+p, func(t *testing.T) { + code, body := post(periodMetricDataForm(p, p)) + if code != http.StatusOK { + t.Fatalf("period %s: code=%d body=%s", p, code, body) + } + + if !strings.Contains(body, "m1") || !strings.Contains(body, "5") { + t.Fatalf("period %s: want the stored datum back: %s", p, body) + } + }) + } +} + +// TestGetMetricStatisticsPeriodValidation applies the same Period rule to +// GetMetricStatistics, where Period is required. +func TestGetMetricStatisticsPeriodValidation(t *testing.T) { + post := newPeriodPoster(t) + + form := func(period string) url.Values { + return url.Values{ + "Action": {"GetMetricStatistics"}, "Namespace": {"App"}, "MetricName": {"Latency"}, + "StartTime": {time.Now().Add(-time.Hour).UTC().Format(time.RFC3339)}, + "EndTime": {time.Now().UTC().Format(time.RFC3339)}, + "Period": {period}, + "Statistics.member.1": {"Sum"}, + } + } + + for _, p := range []string{"0", "-60", "61", "540431955284459520"} { + t.Run("invalid "+p, func(t *testing.T) { + code, body := post(form(p)) + if code != http.StatusBadRequest || !strings.Contains(body, "InvalidParameterValue") || + !strings.Contains(body, "parameter Period is not a valid period") { + t.Fatalf("want 400 InvalidParameterValue, got code=%d body=%s", code, body) + } + }) + } + + code, body := post(form("300")) + if code != http.StatusOK || !strings.Contains(body, "5") { + t.Fatalf("valid period: code=%d body=%s", code, body) + } +} + +// TestPutMetricAlarmMathPeriodLookback checks that a metric math alarm's +// MetricStat Period obeys the EvaluationPeriods * Period cap, so a huge +// Period cannot reach the evaluator. +func TestPutMetricAlarmMathPeriodLookback(t *testing.T) { + post := newPeriodPoster(t) + + form := func(period, evalPeriods string) url.Values { + return url.Values{ + "Action": {"PutMetricAlarm"}, "AlarmName": {"math"}, "ComparisonOperator": {"GreaterThanThreshold"}, + "EvaluationPeriods": {evalPeriods}, "Threshold": {"1"}, + "Metrics.member.1.Id": {"m1"}, + "Metrics.member.1.ReturnData": {"false"}, + "Metrics.member.1.MetricStat.Metric.Namespace": {"App"}, + "Metrics.member.1.MetricStat.Metric.MetricName": {"Latency"}, + "Metrics.member.1.MetricStat.Period": {period}, + "Metrics.member.1.MetricStat.Stat": {"Sum"}, + "Metrics.member.2.Id": {"e1"}, + "Metrics.member.2.Expression": {"m1 * 2"}, + "Metrics.member.2.ReturnData": {"true"}, + } + } + + for _, tc := range []struct{ period, evalPeriods string }{ + {"540431955284459520", "1"}, + {"60", "1537228672809129302"}, + {"3600", "169"}, + } { + code, body := post(form(tc.period, tc.evalPeriods)) + if code != http.StatusBadRequest || !strings.Contains(body, "ValidationError") { + t.Fatalf("period %s x %s: want 400 ValidationError, got code=%d body=%s", tc.period, tc.evalPeriods, code, body) + } + } + + if code, body := post(form("3600", "168")); code != http.StatusOK { + t.Fatalf("a week of hourly periods: code=%d body=%s", code, body) + } +}