From 26392846b7c06505ade24d9d0d320290572e18fa Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 13:54:58 +0100 Subject: [PATCH 1/3] fix(aws): refuse out-of-range purchase counts before any AWS call EC2, ElastiCache, MemoryDB, RDS and Redshift converted rec.Count with int32(rec.Count) unchecked, so a count above math.MaxInt32 wrapped into a wrong quantity. Move OpenSearch's safeInt32Count into offeringprice as PurchaseCount (typed ErrPurchaseCountOutOfRange) and call it first in all five PurchaseCommitment paths, ahead of the idempotency and offering lookups. The error wraps ErrRefusedBeforePurchase. OpenSearch's error text now says "purchase count" instead of "instance count". Two existing not-found tests gain Count: 1, which they omitted. Part of #683 Co-Authored-By: Claude Opus 5.5 --- .../aws/internal/offeringprice/offeringprice.go | 14 ++++++++++++++ .../internal/offeringprice/offeringprice_test.go | 13 +++++++++++++ providers/aws/services/ec2/client.go | 13 ++++++++++++- providers/aws/services/elasticache/client.go | 13 ++++++++++++- providers/aws/services/memorydb/client.go | 13 ++++++++++++- providers/aws/services/memorydb/client_test.go | 1 + providers/aws/services/opensearch/client.go | 13 +------------ providers/aws/services/opensearch/client_test.go | 6 +++--- providers/aws/services/rds/client.go | 13 ++++++++++++- providers/aws/services/redshift/client.go | 10 +++++++++- providers/aws/services/redshift/client_test.go | 1 + 11 files changed, 90 insertions(+), 20 deletions(-) diff --git a/providers/aws/internal/offeringprice/offeringprice.go b/providers/aws/internal/offeringprice/offeringprice.go index fbc9fd33..dd40a6a9 100644 --- a/providers/aws/internal/offeringprice/offeringprice.go +++ b/providers/aws/internal/offeringprice/offeringprice.go @@ -5,6 +5,7 @@ package offeringprice import ( + "errors" "fmt" "math" "strings" @@ -14,6 +15,19 @@ import ( const hourlyFrequency = "Hourly" +// ErrPurchaseCountOutOfRange marks a purchase count PurchaseCount refused. +var ErrPurchaseCountOutOfRange = errors.New("purchase count out of range") + +// PurchaseCount returns n as the int32 the AWS purchase inputs take. It refuses +// a count outside [1, math.MaxInt32] instead of letting int32(n) wrap into a +// wrong, possibly positive, quantity. Call it before any AWS call. +func PurchaseCount(n int) (int32, error) { + if n < 1 || n > math.MaxInt32 { + return 0, fmt.Errorf("%w: purchase count %d is out of valid range [1, %d]", ErrPurchaseCountOutOfRange, n, math.MaxInt32) + } + return int32(n), nil +} + // Charge is one recurring charge of an offering. type Charge struct { Frequency string diff --git a/providers/aws/internal/offeringprice/offeringprice_test.go b/providers/aws/internal/offeringprice/offeringprice_test.go index 557a1f23..0700b589 100644 --- a/providers/aws/internal/offeringprice/offeringprice_test.go +++ b/providers/aws/internal/offeringprice/offeringprice_test.go @@ -87,3 +87,16 @@ func TestNoUpfrontApproved(t *testing.T) { assert.False(t, NoUpfrontApproved(math.Inf(1))) assert.False(t, NoUpfrontApproved(math.Inf(-1))) } + +func TestPurchaseCount(t *testing.T) { + t.Parallel() + for _, n := range []int{1, 7, math.MaxInt32} { + got, err := PurchaseCount(n) + require.NoError(t, err) + assert.EqualValues(t, n, got) + } + for _, n := range []int{math.MaxInt32 + 1, 1 << 40, 0, -1} { + _, err := PurchaseCount(n) + require.ErrorIs(t, err, ErrPurchaseCountOutOfRange, "count %d", n) + } +} diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index d6fea4b5..d38ce345 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -128,6 +128,17 @@ func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitmen // PurchaseCommitment purchases an EC2 Reserved Instance. func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { + // Refuse before any lookup or idempotency short-circuit so a bad count + // never reaches AWS and a re-drive cannot mask it. + count, err := offeringprice.PurchaseCount(rec.Count) + if err != nil { + err = fmt.Errorf("%w: %w", common.ErrRefusedBeforePurchase, err) + return common.PurchaseResult{Recommendation: rec, Timestamp: time.Now(), Error: err}, err + } + return c.purchase(ctx, rec, count, opts) +} + +func (c *Client) purchase(ctx context.Context, rec common.Recommendation, count int32, opts common.PurchaseOptions) (common.PurchaseResult, error) { result := common.PurchaseResult{ Recommendation: rec, DryRun: false, @@ -175,7 +186,7 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati // Create the purchase request input := &ec2.PurchaseReservedInstancesOfferingInput{ ReservedInstancesOfferingId: offering.ReservedInstancesOfferingId, - InstanceCount: aws.Int32(int32(rec.Count)), // #nosec G115 -- Count from CE recommendation; AWS RI purchase limits keep this far below math.MaxInt32 + InstanceCount: aws.Int32(count), } // Execute the purchase. A single attempt: the API has no ClientToken, so an diff --git a/providers/aws/services/elasticache/client.go b/providers/aws/services/elasticache/client.go index 66df52a8..cab6675c 100644 --- a/providers/aws/services/elasticache/client.go +++ b/providers/aws/services/elasticache/client.go @@ -128,6 +128,17 @@ func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitmen // PurchaseCommitment purchases an ElastiCache Reserved Cache Node. func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { + // Refuse before any lookup or idempotency short-circuit so a bad count + // never reaches AWS and a re-drive cannot mask it. + count, err := offeringprice.PurchaseCount(rec.Count) + if err != nil { + err = fmt.Errorf("%w: %w", common.ErrRefusedBeforePurchase, err) + return common.PurchaseResult{Recommendation: rec, Timestamp: time.Now(), Error: err}, err + } + return c.purchase(ctx, rec, count, opts) +} + +func (c *Client) purchase(ctx context.Context, rec common.Recommendation, count int32, opts common.PurchaseOptions) (common.PurchaseResult, error) { result := common.PurchaseResult{ Recommendation: rec, DryRun: false, @@ -175,7 +186,7 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati input := &elasticache.PurchaseReservedCacheNodesOfferingInput{ ReservedCacheNodesOfferingId: aws.String(offeringID), - CacheNodeCount: aws.Int32(int32(rec.Count)), // #nosec G115 -- Count from CE recommendation; AWS RI purchase limits keep this far below math.MaxInt32 + CacheNodeCount: aws.Int32(count), ReservedCacheNodeId: aws.String(reservationID), Tags: c.createPurchaseTags(rec, opts.Source), } diff --git a/providers/aws/services/memorydb/client.go b/providers/aws/services/memorydb/client.go index 42e4c28b..17b940e2 100644 --- a/providers/aws/services/memorydb/client.go +++ b/providers/aws/services/memorydb/client.go @@ -122,6 +122,17 @@ func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitmen // PurchaseCommitment purchases a MemoryDB Reserved Node. func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { + // Refuse before any lookup or idempotency short-circuit so a bad count + // never reaches AWS and a re-drive cannot mask it. + count, err := offeringprice.PurchaseCount(rec.Count) + if err != nil { + err = fmt.Errorf("%w: %w", common.ErrRefusedBeforePurchase, err) + return common.PurchaseResult{Recommendation: rec, Timestamp: time.Now(), Error: err}, err + } + return c.purchase(ctx, rec, count, opts) +} + +func (c *Client) purchase(ctx context.Context, rec common.Recommendation, count int32, opts common.PurchaseOptions) (common.PurchaseResult, error) { result := common.PurchaseResult{ Recommendation: rec, DryRun: false, @@ -177,7 +188,7 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati input := &memorydb.PurchaseReservedNodesOfferingInput{ ReservedNodesOfferingId: aws.String(offeringID), ReservationId: aws.String(reservationID), - NodeCount: aws.Int32(int32(rec.Count)), // #nosec G115 -- Count from CE recommendation; AWS RI purchase limits keep this far below math.MaxInt32 + NodeCount: aws.Int32(count), Tags: c.createPurchaseTags(rec, opts.Source), } diff --git a/providers/aws/services/memorydb/client_test.go b/providers/aws/services/memorydb/client_test.go index 469031ba..b84c08f7 100644 --- a/providers/aws/services/memorydb/client_test.go +++ b/providers/aws/services/memorydb/client_test.go @@ -704,6 +704,7 @@ func TestClient_PurchaseCommitment_OfferingNotFound(t *testing.T) { rec := common.Recommendation{ Service: common.ServiceCache, ResourceType: "db.r6gd.xlarge", + Count: 1, PaymentOption: "partial-upfront", Term: "1yr", Details: &common.CacheDetails{ diff --git a/providers/aws/services/opensearch/client.go b/providers/aws/services/opensearch/client.go index 9dcfee01..a3b3665c 100644 --- a/providers/aws/services/opensearch/client.go +++ b/providers/aws/services/opensearch/client.go @@ -5,7 +5,6 @@ import ( "context" "fmt" "log" - "math" "strings" "sync" "time" @@ -163,7 +162,7 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati // a re-drive carrying an invalid count (negative / int32-overflow) against // an already-existing reservation would short-circuit to Success and never // surface the bad input. Fail loud up front instead. - instanceCount, countErr := safeInt32Count(rec.Count) + instanceCount, countErr := offeringprice.PurchaseCount(rec.Count) if countErr != nil { result.Error = countErr return result, result.Error @@ -661,13 +660,3 @@ func (c *Client) GetValidResourceTypes(ctx context.Context) ([]string, error) { "i3.16xlarge.search", }, nil } - -// safeInt32Count validates that n is a positive value that fits in int32 and -// returns it as int32. This prevents a gosec G115 integer-overflow conversion -// on the PurchaseReservedInstanceOffering InstanceCount field. -func safeInt32Count(n int) (int32, error) { - if n < 1 || n > math.MaxInt32 { - return 0, fmt.Errorf("instance count %d is out of valid range [1, %d]", n, math.MaxInt32) - } - return int32(n), nil -} diff --git a/providers/aws/services/opensearch/client_test.go b/providers/aws/services/opensearch/client_test.go index efa94f07..54e9681b 100644 --- a/providers/aws/services/opensearch/client_test.go +++ b/providers/aws/services/opensearch/client_test.go @@ -1003,7 +1003,7 @@ func TestClient_PurchaseCommitment_Idempotent_GuardShortCircuits(t *testing.T) { // regression guard for the count-validation ordering fix (follow-up to #831). // An invalid instance count must fail loud up front, even when an idempotency // token would otherwise short-circuit against an existing reservation. Before -// the fix, safeInt32Count ran after idempotencyGuard, so this call returned +// the fix, offeringprice.PurchaseCount ran after idempotencyGuard, so this call returned // Success and silently ignored the bad count. The mocks assert the offering // lookup and reservation describe are never reached. func TestClient_PurchaseCommitment_InvalidCount_FailsBeforeIdempotencyGuard(t *testing.T) { @@ -1013,13 +1013,13 @@ func TestClient_PurchaseCommitment_InvalidCount_FailsBeforeIdempotencyGuard(t *t token := common.DeriveIdempotencyToken("exec-badcount", 0) rec := osIdemRec() - rec.Count = 0 // invalid: safeInt32Count rejects n < 1 + rec.Count = 0 // invalid: offeringprice.PurchaseCount rejects n < 1 result, err := client.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{IdempotencyToken: token}) assert.Error(t, err) assert.False(t, result.Success, "invalid count must not report Success via the idempotency short-circuit") if err != nil { - assert.Contains(t, err.Error(), "instance count 0 is out of valid range") + assert.Contains(t, err.Error(), "purchase count 0 is out of valid range") } mockOS.AssertNotCalled(t, "DescribeReservedInstanceOfferings", mock.Anything, mock.Anything) mockOS.AssertNotCalled(t, "DescribeReservedInstances", mock.Anything, mock.Anything) diff --git a/providers/aws/services/rds/client.go b/providers/aws/services/rds/client.go index 83dddf67..0d1f6f40 100644 --- a/providers/aws/services/rds/client.go +++ b/providers/aws/services/rds/client.go @@ -134,6 +134,17 @@ func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitmen // PurchaseCommitment purchases an RDS Reserved Instance. func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { + // Refuse before any lookup or idempotency short-circuit so a bad count + // never reaches AWS and a re-drive cannot mask it. + count, err := offeringprice.PurchaseCount(rec.Count) + if err != nil { + err = fmt.Errorf("%w: %w", common.ErrRefusedBeforePurchase, err) + return common.PurchaseResult{Recommendation: rec, Timestamp: time.Now(), Error: err}, err + } + return c.purchase(ctx, rec, count, opts) +} + +func (c *Client) purchase(ctx context.Context, rec common.Recommendation, count int32, opts common.PurchaseOptions) (common.PurchaseResult, error) { result := common.PurchaseResult{ Recommendation: rec, DryRun: false, @@ -175,7 +186,7 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati input := &rds.PurchaseReservedDBInstancesOfferingInput{ ReservedDBInstancesOfferingId: aws.String(offeringID), ReservedDBInstanceId: aws.String(reservationID), - DBInstanceCount: aws.Int32(int32(rec.Count)), // #nosec G115 -- Count from CE recommendation; AWS RI purchase limits keep this far below math.MaxInt32 + DBInstanceCount: aws.Int32(count), Tags: c.createPurchaseTags(rec, opts.Source), } diff --git a/providers/aws/services/redshift/client.go b/providers/aws/services/redshift/client.go index 056d73ed..45c9a8a9 100644 --- a/providers/aws/services/redshift/client.go +++ b/providers/aws/services/redshift/client.go @@ -134,6 +134,14 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati Timestamp: time.Now(), } + // Refuse before any lookup or idempotency short-circuit so a bad count + // never reaches AWS and a re-drive cannot mask it. + count, countErr := offeringprice.PurchaseCount(rec.Count) + if countErr != nil { + result.Error = fmt.Errorf("%w: %w", common.ErrRefusedBeforePurchase, countErr) + return result, result.Error + } + offeringID, err := c.findOfferingID(ctx, rec, opts.ExecutionID) if err != nil { result.Error = fmt.Errorf("failed to find offering: %w", err) @@ -155,7 +163,7 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati input := &redshift.PurchaseReservedNodeOfferingInput{ ReservedNodeOfferingId: aws.String(offeringID), - NodeCount: aws.Int32(int32(rec.Count)), // #nosec G115 -- Count from CE recommendation; AWS RI purchase limits keep this far below math.MaxInt32 + NodeCount: aws.Int32(count), } // A single attempt: the API has no ClientToken or caller ID, so an SDK retry diff --git a/providers/aws/services/redshift/client_test.go b/providers/aws/services/redshift/client_test.go index 721c13ca..748fb89e 100644 --- a/providers/aws/services/redshift/client_test.go +++ b/providers/aws/services/redshift/client_test.go @@ -563,6 +563,7 @@ func TestClient_PurchaseCommitment_FindOfferingError(t *testing.T) { rec := common.Recommendation{ Service: common.ServiceDataWarehouse, ResourceType: "dc2.large", + Count: 1, PaymentOption: "all-upfront", Term: "1yr", Details: common.DataWarehouseDetails{ From b7bf71f11f2dbf7fb074a77b75ad536c1733fe08 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 13:55:12 +0100 Subject: [PATCH 2/3] test(aws): cover purchase count bounds in five AWS services Each service sends exactly math.MaxInt32 when given it, and refuses MaxInt32+1, 0 and -1 with zero API calls even when an idempotency token is set. Fixes #683 Co-Authored-By: Claude Opus 5.5 --- .../aws/services/ec2/purchase_count_test.go | 61 +++++++++++++++++ .../elasticache/purchase_count_test.go | 67 ++++++++++++++++++ .../services/memorydb/purchase_count_test.go | 56 +++++++++++++++ .../aws/services/rds/purchase_count_test.go | 68 +++++++++++++++++++ .../services/redshift/purchase_count_test.go | 60 ++++++++++++++++ 5 files changed, 312 insertions(+) create mode 100644 providers/aws/services/ec2/purchase_count_test.go create mode 100644 providers/aws/services/elasticache/purchase_count_test.go create mode 100644 providers/aws/services/memorydb/purchase_count_test.go create mode 100644 providers/aws/services/rds/purchase_count_test.go create mode 100644 providers/aws/services/redshift/purchase_count_test.go diff --git a/providers/aws/services/ec2/purchase_count_test.go b/providers/aws/services/ec2/purchase_count_test.go new file mode 100644 index 00000000..eff7d665 --- /dev/null +++ b/providers/aws/services/ec2/purchase_count_test.go @@ -0,0 +1,61 @@ +package ec2 + +import ( + "context" + "math" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/aws/internal/offeringprice" + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/ec2" + "github.com/aws/aws-sdk-go-v2/service/ec2/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestPurchaseCommitment_Count(t *testing.T) { + t.Parallel() + rec := func(n int) common.Recommendation { + return common.Recommendation{ResourceType: "t3.micro", Count: n, PaymentOption: "partial-upfront", Term: "3yr", + Details: &common.ComputeDetails{Platform: "Linux/UNIX", Tenancy: "default", Scope: "Region"}} + } + + t.Run("max int32 is sent exactly", func(t *testing.T) { + t.Parallel() + m := &MockEC2Client{} + m.Test(t) + m.On("DescribeReservedInstancesOfferings", mock.Anything, mock.Anything). + Return(&ec2.DescribeReservedInstancesOfferingsOutput{ReservedInstancesOfferings: []types.ReservedInstancesOffering{{ + ReservedInstancesOfferingId: aws.String("offering-123"), InstanceType: types.InstanceTypeT3Micro, + Duration: aws.Int64(94608000), OfferingType: types.OfferingTypeValuesPartialUpfront, + ProductDescription: types.RIProductDescriptionLinuxUnix, InstanceTenancy: types.TenancyDefault, OfferingClass: types.OfferingClassTypeConvertible, + }}}, nil) + m.On("PurchaseReservedInstancesOffering", mock.Anything, mock.MatchedBy(func(in *ec2.PurchaseReservedInstancesOfferingInput) bool { + return aws.ToInt32(in.InstanceCount) == math.MaxInt32 + })).Return(&ec2.PurchaseReservedInstancesOfferingOutput{ReservedInstancesId: aws.String("ri-1")}, nil).Once() + m.On("CreateTags", mock.Anything, mock.Anything).Return(&ec2.CreateTagsOutput{}, nil) + + result, err := (&Client{client: m, region: "us-east-1"}).PurchaseCommitment(context.Background(), rec(math.MaxInt32), common.PurchaseOptions{}) + require.NoError(t, err) + assert.True(t, result.Success) + m.AssertNumberOfCalls(t, "PurchaseReservedInstancesOffering", 1) + }) + + for _, n := range []int{math.MaxInt32 + 1, 0, -1} { + t.Run("refused before any API call", func(t *testing.T) { + t.Parallel() + m := &MockEC2Client{} + m.Test(t) + opts := common.PurchaseOptions{IdempotencyToken: common.DeriveIdempotencyToken("exec-count", 0)} + + result, err := (&Client{client: m, region: "us-east-1"}).PurchaseCommitment(context.Background(), rec(n), opts) + require.Error(t, err) + assert.False(t, result.Success) + assert.ErrorIs(t, err, offeringprice.ErrPurchaseCountOutOfRange) + assert.ErrorIs(t, err, common.ErrRefusedBeforePurchase) + assert.Empty(t, m.Calls, "a bad count must make no describe and no purchase call") + }) + } +} diff --git a/providers/aws/services/elasticache/purchase_count_test.go b/providers/aws/services/elasticache/purchase_count_test.go new file mode 100644 index 00000000..22dddd25 --- /dev/null +++ b/providers/aws/services/elasticache/purchase_count_test.go @@ -0,0 +1,67 @@ +package elasticache + +import ( + "context" + "math" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/aws/internal/offeringprice" + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/elasticache" + "github.com/aws/aws-sdk-go-v2/service/elasticache/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestPurchaseCommitment_Count(t *testing.T) { + t.Parallel() + rec := func(n int) common.Recommendation { + return common.Recommendation{ + Service: common.ServiceCache, ResourceType: "cache.m5.large", Count: n, + PaymentOption: "all-upfront", Term: "1yr", + Details: &common.CacheDetails{Engine: "redis", NodeType: "cache.m5.large"}, + } + } + + t.Run("max int32 is sent exactly", func(t *testing.T) { + t.Parallel() + m := &MockElastiCacheClient{} + m.Test(t) + m.On("DescribeReservedCacheNodesOfferings", mock.Anything, mock.Anything). + Return(&elasticache.DescribeReservedCacheNodesOfferingsOutput{ + ReservedCacheNodesOfferings: []types.ReservedCacheNodesOffering{{ + ReservedCacheNodesOfferingId: aws.String("offering-123"), + CacheNodeType: aws.String("cache.m5.large"), + ProductDescription: aws.String("redis"), + OfferingType: aws.String("All Upfront"), + Duration: aws.Int32(31536000), + }}, + }, nil) + m.On("PurchaseReservedCacheNodesOffering", mock.Anything, mock.MatchedBy(func(in *elasticache.PurchaseReservedCacheNodesOfferingInput) bool { + return aws.ToInt32(in.CacheNodeCount) == math.MaxInt32 + })).Return(&elasticache.PurchaseReservedCacheNodesOfferingOutput{ReservedCacheNode: &types.ReservedCacheNode{ReservedCacheNodeId: aws.String("rn-1")}}, nil).Once() + + result, err := (&Client{client: m, region: "us-east-1"}).PurchaseCommitment(context.Background(), rec(math.MaxInt32), common.PurchaseOptions{}) + require.NoError(t, err) + assert.True(t, result.Success) + m.AssertNumberOfCalls(t, "PurchaseReservedCacheNodesOffering", 1) + }) + + for _, n := range []int{math.MaxInt32 + 1, 0, -1} { + t.Run("refused before any API call", func(t *testing.T) { + t.Parallel() + m := &MockElastiCacheClient{} + m.Test(t) + opts := common.PurchaseOptions{IdempotencyToken: common.DeriveIdempotencyToken("exec-count", 0)} + + result, err := (&Client{client: m, region: "us-east-1"}).PurchaseCommitment(context.Background(), rec(n), opts) + require.Error(t, err) + assert.False(t, result.Success) + assert.ErrorIs(t, err, offeringprice.ErrPurchaseCountOutOfRange) + assert.ErrorIs(t, err, common.ErrRefusedBeforePurchase) + assert.Empty(t, m.Calls, "a bad count must make no describe and no purchase call") + }) + } +} diff --git a/providers/aws/services/memorydb/purchase_count_test.go b/providers/aws/services/memorydb/purchase_count_test.go new file mode 100644 index 00000000..ee1b5148 --- /dev/null +++ b/providers/aws/services/memorydb/purchase_count_test.go @@ -0,0 +1,56 @@ +package memorydb + +import ( + "context" + "math" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/aws/internal/offeringprice" + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/memorydb" + "github.com/aws/aws-sdk-go-v2/service/memorydb/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestPurchaseCommitment_Count(t *testing.T) { + t.Parallel() + rec := func(n int) common.Recommendation { + r := mdbIdemRec() + r.Count = n + return r + } + + t.Run("max int32 is sent exactly", func(t *testing.T) { + t.Parallel() + m := &MockMemoryDBClient{} + m.Test(t) + expectMDBOffering(m) + m.On("PurchaseReservedNodesOffering", mock.Anything, mock.MatchedBy(func(in *memorydb.PurchaseReservedNodesOfferingInput) bool { + return aws.ToInt32(in.NodeCount) == math.MaxInt32 + })).Return(&memorydb.PurchaseReservedNodesOfferingOutput{ReservedNode: &types.ReservedNode{ReservationId: aws.String("rn-1")}}, nil).Once() + + result, err := (&Client{client: m, region: "eu-west-1"}).PurchaseCommitment(context.Background(), rec(math.MaxInt32), common.PurchaseOptions{}) + require.NoError(t, err) + assert.True(t, result.Success) + m.AssertNumberOfCalls(t, "PurchaseReservedNodesOffering", 1) + }) + + for _, n := range []int{math.MaxInt32 + 1, 0, -1} { + t.Run("refused before any API call", func(t *testing.T) { + t.Parallel() + m := &MockMemoryDBClient{} + m.Test(t) + opts := common.PurchaseOptions{IdempotencyToken: common.DeriveIdempotencyToken("exec-count", 0)} + + result, err := (&Client{client: m, region: "eu-west-1"}).PurchaseCommitment(context.Background(), rec(n), opts) + require.Error(t, err) + assert.False(t, result.Success) + assert.ErrorIs(t, err, offeringprice.ErrPurchaseCountOutOfRange) + assert.ErrorIs(t, err, common.ErrRefusedBeforePurchase) + assert.Empty(t, m.Calls, "a bad count must make no describe and no purchase call") + }) + } +} diff --git a/providers/aws/services/rds/purchase_count_test.go b/providers/aws/services/rds/purchase_count_test.go new file mode 100644 index 00000000..701c76c1 --- /dev/null +++ b/providers/aws/services/rds/purchase_count_test.go @@ -0,0 +1,68 @@ +package rds + +import ( + "context" + "math" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/aws/internal/offeringprice" + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/rds" + "github.com/aws/aws-sdk-go-v2/service/rds/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestPurchaseCommitment_Count(t *testing.T) { + t.Parallel() + rec := func(n int) common.Recommendation { + return common.Recommendation{ + Service: common.ServiceRelationalDB, ResourceType: "db.t3.micro", Count: n, + PaymentOption: "all-upfront", Term: "1yr", + Details: &common.DatabaseDetails{Engine: "mysql", AZConfig: "single-az"}, + } + } + + t.Run("max int32 is sent exactly", func(t *testing.T) { + t.Parallel() + m := &MockRDSClient{} + m.Test(t) + m.On("DescribeReservedDBInstancesOfferings", mock.Anything, mock.Anything). + Return(&rds.DescribeReservedDBInstancesOfferingsOutput{ + ReservedDBInstancesOfferings: []types.ReservedDBInstancesOffering{{ + ReservedDBInstancesOfferingId: aws.String("offering-123"), + DBInstanceClass: aws.String("db.t3.micro"), + ProductDescription: aws.String("mysql"), + MultiAZ: aws.Bool(false), + OfferingType: aws.String("All Upfront"), + Duration: aws.Int32(31536000), + }}, + }, nil) + m.On("PurchaseReservedDBInstancesOffering", mock.Anything, mock.MatchedBy(func(in *rds.PurchaseReservedDBInstancesOfferingInput) bool { + return aws.ToInt32(in.DBInstanceCount) == math.MaxInt32 + })).Return(&rds.PurchaseReservedDBInstancesOfferingOutput{ReservedDBInstance: &types.ReservedDBInstance{ReservedDBInstanceId: aws.String("ri-1")}}, nil).Once() + + result, err := (&Client{client: m, region: "us-east-1"}).PurchaseCommitment(context.Background(), rec(math.MaxInt32), common.PurchaseOptions{}) + require.NoError(t, err) + assert.True(t, result.Success) + m.AssertNumberOfCalls(t, "PurchaseReservedDBInstancesOffering", 1) + }) + + for _, n := range []int{math.MaxInt32 + 1, 0, -1} { + t.Run("refused before any API call", func(t *testing.T) { + t.Parallel() + m := &MockRDSClient{} + m.Test(t) + opts := common.PurchaseOptions{IdempotencyToken: common.DeriveIdempotencyToken("exec-count", 0)} + + result, err := (&Client{client: m, region: "us-east-1"}).PurchaseCommitment(context.Background(), rec(n), opts) + require.Error(t, err) + assert.False(t, result.Success) + assert.ErrorIs(t, err, offeringprice.ErrPurchaseCountOutOfRange) + assert.ErrorIs(t, err, common.ErrRefusedBeforePurchase) + assert.Empty(t, m.Calls, "a bad count must make no describe and no purchase call") + }) + } +} diff --git a/providers/aws/services/redshift/purchase_count_test.go b/providers/aws/services/redshift/purchase_count_test.go new file mode 100644 index 00000000..79257cec --- /dev/null +++ b/providers/aws/services/redshift/purchase_count_test.go @@ -0,0 +1,60 @@ +package redshift + +import ( + "context" + "math" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/aws/internal/offeringprice" + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/redshift" + "github.com/aws/aws-sdk-go-v2/service/redshift/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestPurchaseCommitment_Count(t *testing.T) { + t.Parallel() + rec := func(n int) common.Recommendation { + return common.Recommendation{ResourceType: "dc2.large", Count: n, PaymentOption: "all-upfront", Term: "1yr", + Details: common.DataWarehouseDetails{NodeType: "dc2.large", NumberOfNodes: 2}} + } + + t.Run("max int32 is sent exactly", func(t *testing.T) { + t.Parallel() + m := &MockRedshiftClient{} + m.Test(t) + m.On("DescribeReservedNodeOfferings", mock.Anything, mock.Anything). + Return(&redshift.DescribeReservedNodeOfferingsOutput{ReservedNodeOfferings: []types.ReservedNodeOffering{{ + ReservedNodeOfferingId: aws.String("offering-123"), NodeType: aws.String("dc2.large"), + Duration: aws.Int32(31536000), ReservedNodeOfferingType: types.ReservedNodeOfferingType("Regular"), + FixedPrice: aws.Float64(500), + }}}, nil) + m.On("PurchaseReservedNodeOffering", mock.Anything, mock.MatchedBy(func(in *redshift.PurchaseReservedNodeOfferingInput) bool { + return aws.ToInt32(in.NodeCount) == math.MaxInt32 + })).Return(&redshift.PurchaseReservedNodeOfferingOutput{ReservedNode: &types.ReservedNode{ReservedNodeId: aws.String("rn-1")}}, nil).Once() + + result, err := (&Client{client: m, region: "us-east-1"}).PurchaseCommitment(context.Background(), rec(math.MaxInt32), common.PurchaseOptions{}) + require.NoError(t, err) + assert.True(t, result.Success) + m.AssertNumberOfCalls(t, "PurchaseReservedNodeOffering", 1) + }) + + for _, n := range []int{math.MaxInt32 + 1, 0, -1} { + t.Run("refused before any API call", func(t *testing.T) { + t.Parallel() + m := &MockRedshiftClient{} + m.Test(t) + opts := common.PurchaseOptions{IdempotencyToken: common.DeriveIdempotencyToken("exec-count", 0)} + + result, err := (&Client{client: m, region: "us-east-1"}).PurchaseCommitment(context.Background(), rec(n), opts) + require.Error(t, err) + assert.False(t, result.Success) + assert.ErrorIs(t, err, offeringprice.ErrPurchaseCountOutOfRange) + assert.ErrorIs(t, err, common.ErrRefusedBeforePurchase) + assert.Empty(t, m.Calls, "a bad count must make no describe and no purchase call") + }) + } +} From 0aafe6a3a51faa53751f8b3abe903f67b810cfc7 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 14:20:55 +0100 Subject: [PATCH 3/3] fix(aws): wrap OpenSearch count refusal and name count subtests OpenSearch's count refusal now wraps ErrRefusedBeforePurchase like the other five services, and ErrRefusedBeforePurchase documents the purchase-count bounds check. Refusal subtests are named by count, and a CHANGELOG line records the behavior. Fixes #683 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + pkg/common/types.go | 5 +++-- providers/aws/services/ec2/purchase_count_test.go | 3 ++- providers/aws/services/elasticache/purchase_count_test.go | 3 ++- providers/aws/services/memorydb/purchase_count_test.go | 3 ++- providers/aws/services/opensearch/client.go | 2 +- providers/aws/services/opensearch/client_test.go | 1 + providers/aws/services/rds/purchase_count_test.go | 3 ++- providers/aws/services/redshift/purchase_count_test.go | 3 ++- 9 files changed, 16 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b5e6ec52..39103729 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -273,6 +273,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- **AWS:** purchase counts outside [1, MaxInt32] are refused before any API call in EC2, ElastiCache, MemoryDB, OpenSearch, RDS and Redshift, with `ErrRefusedBeforePurchase` and `offeringprice.ErrPurchaseCountOutOfRange` (#683). - **`azure`:** an error from a reservation purchase retry (attempt 2 or later) now also wraps `common.ErrOutcomeUnknown`. That covers a currency refusal from the retry's calculatePrice, a failed idempotency re-check, a non-retryable purchase failure, a cancelled retry wait and exhausted attempts. A retry runs only after a purchase that failed retryably and may have committed, and the re-check can miss that order, so none of these errors may read as "nothing was bought". The cause stays in the chain, so `errors.Is(err, common.ErrCurrencyNotAccepted)` still matches. The `ErrCurrencyNotAccepted` doc now says it is pre-purchase only without `ErrOutcomeUnknown` (#671). - **Azure purchases check the recommendation currency.** Azure reservation purchases compare the calculatePrice billing currency with the recommendation currency diff --git a/pkg/common/types.go b/pkg/common/types.go index ccdbb4a8..2aaf212e 100644 --- a/pkg/common/types.go +++ b/pkg/common/types.go @@ -39,8 +39,9 @@ var ErrCurrencyNotAccepted = errors.New("currency not accepted") // contract until the platform persists a typed flag (platform #778). var ErrOutcomeUnknown = errors.New("purchase outcome unknown") -// ErrRefusedBeforePurchase wraps any error from the approved-upfront check, -// which runs before the purchase request is built or sent. Its presence is +// ErrRefusedBeforePurchase wraps any error from the approved-upfront check or +// the purchase-count bounds check, both of which run before the purchase +// request is built or sent. Its presence is // sufficient to know that this call sent no purchase request, but its absence // proves nothing: other refusals do not carry it. It describes only the call // that returned it and says nothing about earlier calls with the same diff --git a/providers/aws/services/ec2/purchase_count_test.go b/providers/aws/services/ec2/purchase_count_test.go index eff7d665..3631b36e 100644 --- a/providers/aws/services/ec2/purchase_count_test.go +++ b/providers/aws/services/ec2/purchase_count_test.go @@ -2,6 +2,7 @@ package ec2 import ( "context" + "fmt" "math" "testing" @@ -44,7 +45,7 @@ func TestPurchaseCommitment_Count(t *testing.T) { }) for _, n := range []int{math.MaxInt32 + 1, 0, -1} { - t.Run("refused before any API call", func(t *testing.T) { + t.Run(fmt.Sprintf("refused before any API call, count %d", n), func(t *testing.T) { t.Parallel() m := &MockEC2Client{} m.Test(t) diff --git a/providers/aws/services/elasticache/purchase_count_test.go b/providers/aws/services/elasticache/purchase_count_test.go index 22dddd25..c5e612fd 100644 --- a/providers/aws/services/elasticache/purchase_count_test.go +++ b/providers/aws/services/elasticache/purchase_count_test.go @@ -2,6 +2,7 @@ package elasticache import ( "context" + "fmt" "math" "testing" @@ -50,7 +51,7 @@ func TestPurchaseCommitment_Count(t *testing.T) { }) for _, n := range []int{math.MaxInt32 + 1, 0, -1} { - t.Run("refused before any API call", func(t *testing.T) { + t.Run(fmt.Sprintf("refused before any API call, count %d", n), func(t *testing.T) { t.Parallel() m := &MockElastiCacheClient{} m.Test(t) diff --git a/providers/aws/services/memorydb/purchase_count_test.go b/providers/aws/services/memorydb/purchase_count_test.go index ee1b5148..6af40160 100644 --- a/providers/aws/services/memorydb/purchase_count_test.go +++ b/providers/aws/services/memorydb/purchase_count_test.go @@ -2,6 +2,7 @@ package memorydb import ( "context" + "fmt" "math" "testing" @@ -39,7 +40,7 @@ func TestPurchaseCommitment_Count(t *testing.T) { }) for _, n := range []int{math.MaxInt32 + 1, 0, -1} { - t.Run("refused before any API call", func(t *testing.T) { + t.Run(fmt.Sprintf("refused before any API call, count %d", n), func(t *testing.T) { t.Parallel() m := &MockMemoryDBClient{} m.Test(t) diff --git a/providers/aws/services/opensearch/client.go b/providers/aws/services/opensearch/client.go index a3b3665c..bda7b95c 100644 --- a/providers/aws/services/opensearch/client.go +++ b/providers/aws/services/opensearch/client.go @@ -164,7 +164,7 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati // surface the bad input. Fail loud up front instead. instanceCount, countErr := offeringprice.PurchaseCount(rec.Count) if countErr != nil { - result.Error = countErr + result.Error = fmt.Errorf("%w: %w", common.ErrRefusedBeforePurchase, countErr) return result, result.Error } diff --git a/providers/aws/services/opensearch/client_test.go b/providers/aws/services/opensearch/client_test.go index 54e9681b..23baa837 100644 --- a/providers/aws/services/opensearch/client_test.go +++ b/providers/aws/services/opensearch/client_test.go @@ -1019,6 +1019,7 @@ func TestClient_PurchaseCommitment_InvalidCount_FailsBeforeIdempotencyGuard(t *t assert.Error(t, err) assert.False(t, result.Success, "invalid count must not report Success via the idempotency short-circuit") if err != nil { + assert.ErrorIs(t, err, common.ErrRefusedBeforePurchase) assert.Contains(t, err.Error(), "purchase count 0 is out of valid range") } mockOS.AssertNotCalled(t, "DescribeReservedInstanceOfferings", mock.Anything, mock.Anything) diff --git a/providers/aws/services/rds/purchase_count_test.go b/providers/aws/services/rds/purchase_count_test.go index 701c76c1..c58f7b18 100644 --- a/providers/aws/services/rds/purchase_count_test.go +++ b/providers/aws/services/rds/purchase_count_test.go @@ -2,6 +2,7 @@ package rds import ( "context" + "fmt" "math" "testing" @@ -51,7 +52,7 @@ func TestPurchaseCommitment_Count(t *testing.T) { }) for _, n := range []int{math.MaxInt32 + 1, 0, -1} { - t.Run("refused before any API call", func(t *testing.T) { + t.Run(fmt.Sprintf("refused before any API call, count %d", n), func(t *testing.T) { t.Parallel() m := &MockRDSClient{} m.Test(t) diff --git a/providers/aws/services/redshift/purchase_count_test.go b/providers/aws/services/redshift/purchase_count_test.go index 79257cec..42f29415 100644 --- a/providers/aws/services/redshift/purchase_count_test.go +++ b/providers/aws/services/redshift/purchase_count_test.go @@ -2,6 +2,7 @@ package redshift import ( "context" + "fmt" "math" "testing" @@ -43,7 +44,7 @@ func TestPurchaseCommitment_Count(t *testing.T) { }) for _, n := range []int{math.MaxInt32 + 1, 0, -1} { - t.Run("refused before any API call", func(t *testing.T) { + t.Run(fmt.Sprintf("refused before any API call, count %d", n), func(t *testing.T) { t.Parallel() m := &MockRedshiftClient{} m.Test(t)