diff --git a/CHANGELOG.md b/CHANGELOG.md index b2da837f..210d11d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -286,6 +286,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). actually bought. Before, 2.5 became 3 with the upfront for 2.5, and the approved-upfront check refused the purchase. The on-demand baseline and break-even months are unchanged (#635). +- **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/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 333828f1..2a1e09b9 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/ec2/purchase_count_test.go b/providers/aws/services/ec2/purchase_count_test.go new file mode 100644 index 00000000..3631b36e --- /dev/null +++ b/providers/aws/services/ec2/purchase_count_test.go @@ -0,0 +1,62 @@ +package ec2 + +import ( + "context" + "fmt" + "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(fmt.Sprintf("refused before any API call, count %d", n), 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/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/elasticache/purchase_count_test.go b/providers/aws/services/elasticache/purchase_count_test.go new file mode 100644 index 00000000..c5e612fd --- /dev/null +++ b/providers/aws/services/elasticache/purchase_count_test.go @@ -0,0 +1,68 @@ +package elasticache + +import ( + "context" + "fmt" + "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(fmt.Sprintf("refused before any API call, count %d", n), 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/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/memorydb/purchase_count_test.go b/providers/aws/services/memorydb/purchase_count_test.go new file mode 100644 index 00000000..6af40160 --- /dev/null +++ b/providers/aws/services/memorydb/purchase_count_test.go @@ -0,0 +1,57 @@ +package memorydb + +import ( + "context" + "fmt" + "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(fmt.Sprintf("refused before any API call, count %d", n), 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/opensearch/client.go b/providers/aws/services/opensearch/client.go index 9dcfee01..bda7b95c 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,9 +162,9 @@ 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 + result.Error = fmt.Errorf("%w: %w", common.ErrRefusedBeforePurchase, 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..23baa837 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,14 @@ 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.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) 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/rds/purchase_count_test.go b/providers/aws/services/rds/purchase_count_test.go new file mode 100644 index 00000000..c58f7b18 --- /dev/null +++ b/providers/aws/services/rds/purchase_count_test.go @@ -0,0 +1,69 @@ +package rds + +import ( + "context" + "fmt" + "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(fmt.Sprintf("refused before any API call, count %d", n), 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/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{ 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..42f29415 --- /dev/null +++ b/providers/aws/services/redshift/purchase_count_test.go @@ -0,0 +1,61 @@ +package redshift + +import ( + "context" + "fmt" + "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(fmt.Sprintf("refused before any API call, count %d", n), 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") + }) + } +}