From 6e1ac710ecabd75ed151b8b45b15fb5d098b736f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 21 Oct 2026 00:56:12 +0200 Subject: [PATCH 1/2] fix(aws/ec2): refuse an offering priced above the approved upfront PurchaseCommitment bought the first matching offering without comparing its price to the recommendation's CommitmentCost, and left the API's LimitPrice unset, so a repriced or differently-priced offering bypassed the approved cap. Refuse when the offering's total upfront (FixedPrice x Count) exceeds the approved upfront by more than 5%, and send that ceiling as LimitPrice. Recommendations with no positive upfront are unchanged. Other services' purchase paths from #53 are not covered here. Part of #53 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 4 + .../internal/offeringprice/offeringprice.go | 24 +++++ .../offeringprice/offeringprice_test.go | 7 ++ providers/aws/services/ec2/client.go | 35 ++++++++ .../services/ec2/purchase_price_guard_test.go | 87 +++++++++++++++++++ 5 files changed, 157 insertions(+) create mode 100644 providers/aws/services/ec2/purchase_price_guard_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fb5fe87..282f8308 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -220,6 +220,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). sending an empty `ProductDescription` filter, send the engine lowercased as ElastiCache reports it, and verify the chosen offering's node type, duration and engine before buying it (#53). +- AWS EC2 RI purchases refuse an offering whose total upfront exceeds the + recommendation's approved upfront by more than 5%, and send the same ceiling + as the API's `LimitPrice` (USD only), so a repriced or differently-priced + offering can no longer bypass the approved cap (#53). - **`ReshapeBuffer` rejects nil payment caps** instead of mapping them to `math.MaxFloat64`: `MaxPaymentPerExchangeUSD` and `MaxPaymentDailyUSD` are now required, so an omitted cap can no longer mean unlimited spend on diff --git a/providers/aws/internal/offeringprice/offeringprice.go b/providers/aws/internal/offeringprice/offeringprice.go index a278a94c..002a3e69 100644 --- a/providers/aws/internal/offeringprice/offeringprice.go +++ b/providers/aws/internal/offeringprice/offeringprice.go @@ -135,3 +135,27 @@ func AssumedUSD(service, region string) (string, error) { } return "USD", nil } + +// UpfrontTolerance is the fraction by which a resolved offering's upfront may +// exceed the approved recommendation's upfront before a purchase is refused. +// It absorbs Cost Explorer rounding; anything beyond it means the offering was +// repriced or a differently-priced variant matched, so the approval no longer +// covers it. +const UpfrontTolerance = 0.05 + +// UpfrontCeiling is the most a purchase may charge upfront for an approved +// upfront amount. +func UpfrontCeiling(approved float64) float64 { + return approved * (1 + UpfrontTolerance) +} + +// CheckUpfront fails when offeredTotal exceeds UpfrontCeiling(approved), both +// compared in whole cents. approved must be positive: a zero upfront is either +// a no-upfront option or a missing figure, and neither gives a ceiling. +func CheckUpfront(service string, offeredTotal, approved float64) error { + if math.Round(offeredTotal*100) > math.Round(UpfrontCeiling(approved)*100) { + return fmt.Errorf("%s offering upfront %.2f exceeds the approved upfront %.2f by more than %.0f%%; refusing to purchase (re-approve at the current price)", + service, offeredTotal, approved, UpfrontTolerance*100) + } + return nil +} diff --git a/providers/aws/internal/offeringprice/offeringprice_test.go b/providers/aws/internal/offeringprice/offeringprice_test.go index 15dfb68c..ef375bae 100644 --- a/providers/aws/internal/offeringprice/offeringprice_test.go +++ b/providers/aws/internal/offeringprice/offeringprice_test.go @@ -58,3 +58,10 @@ func TestCurrency(t *testing.T) { _, err = AssumedUSD("S", "cn-north-1") require.Error(t, err) } + +func TestCheckUpfront_BoundaryIsTheCeilingInCents(t *testing.T) { + t.Parallel() + require.NoError(t, CheckUpfront("EC2", 1050, 1000)) + require.ErrorContains(t, CheckUpfront("EC2", 1050.01, 1000), "exceeds the approved upfront") + require.NoError(t, CheckUpfront("EC2", 0, 1000)) +} diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index 6b8219cf..133e1afc 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -167,10 +167,17 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati return result, result.Error } + limit, err := upfrontLimit(rec, offering) + if err != nil { + result.Error = err + return result, result.Error + } + // 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 + LimitPrice: limit, } // Execute the purchase. A single attempt: the API has no ClientToken, so an @@ -217,6 +224,34 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati return result, nil } +// upfrontLimit checks the resolved offering's total upfront against the +// approved recommendation's upfront and returns the server-side LimitPrice that +// caps the order at the same ceiling. It returns nil, nil when the +// recommendation has no positive upfront to cap (no-upfront options, or a +// missing figure). An offering that exceeds the ceiling, or that is priced in a +// currency LimitPrice cannot express, is refused. +func upfrontLimit(rec common.Recommendation, offering *types.ReservedInstancesOffering) (*types.ReservedInstanceLimitPrice, error) { + approved := rec.CommitmentCost + if approved <= 0 { + return nil, nil + } + id := aws.ToString(offering.ReservedInstancesOfferingId) + if offering.FixedPrice == nil { + return nil, fmt.Errorf("EC2 offering %s has no FixedPrice; cannot compare it with the approved upfront %.2f", id, approved) + } + if cc := offering.CurrencyCode; cc != "" && cc != types.CurrencyCodeValuesUsd { + return nil, fmt.Errorf("EC2 offering %s is priced in %s; the approved upfront cap supports USD only", id, cc) + } + total := fixedPriceCents(*offering.FixedPrice) * float64(rec.Count) / 100 + if err := offeringprice.CheckUpfront("EC2", total, approved); err != nil { + return nil, fmt.Errorf("EC2 offering %s: %w", id, err) + } + return &types.ReservedInstanceLimitPrice{ + Amount: aws.Float64(math.Round(offeringprice.UpfrontCeiling(approved)*100) / 100), + CurrencyCode: types.CurrencyCodeValuesUsd, + }, nil +} + // findRIByIdempotencyToken looks for an active or payment-pending Reserved // Instance tagged with the given idempotency token (issue #636). It returns the // RI ID and true when exactly such an RI exists, so a re-driven purchase can diff --git a/providers/aws/services/ec2/purchase_price_guard_test.go b/providers/aws/services/ec2/purchase_price_guard_test.go new file mode 100644 index 00000000..d10859b5 --- /dev/null +++ b/providers/aws/services/ec2/purchase_price_guard_test.go @@ -0,0 +1,87 @@ +package ec2 + +import ( + "context" + "testing" + + "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" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +// pricedRec is a 2-instance partial-upfront rec approved at a $1000 total upfront. +func pricedRec() common.Recommendation { + r := matchRec() + r.Count = 2 + r.PaymentOption = "partial-upfront" + r.CommitmentCost = 1000 + return r +} + +func pricedOffering(fixed float32) types.ReservedInstancesOffering { + return fullOffering("priced", "", func(o *types.ReservedInstancesOffering) { + o.OfferingType = types.OfferingTypeValuesPartialUpfront + o.FixedPrice = aws.Float32(fixed) + o.CurrencyCode = types.CurrencyCodeValuesUsd + }) +} + +func TestPurchaseCommitment_RefusesOfferingAboveApprovedUpfront(t *testing.T) { + t.Parallel() + // 2 x 600 = 1200 against an approved 1000 (ceiling 1050). + m := offeringsMock(pricedOffering(600)) + res, err := (&Client{client: m, region: "us-east-1"}).PurchaseCommitment(context.Background(), pricedRec(), common.PurchaseOptions{}) + require.ErrorContains(t, err, "exceeds the approved upfront") + assert.False(t, res.Success) + m.AssertNotCalled(t, "PurchaseReservedInstancesOffering", mock.Anything, mock.Anything) +} + +func TestPurchaseCommitment_SetsLimitPriceAtApprovedCeiling(t *testing.T) { + t.Parallel() + var got *ec2.PurchaseReservedInstancesOfferingInput + m := offeringsMock(pricedOffering(500)) // 2 x 500 = 1000 == approved + m.ExpectedCalls = m.ExpectedCalls[:1] // drop the permissive Purchase stub + m.On("PurchaseReservedInstancesOffering", mock.Anything, mock.MatchedBy(func(in *ec2.PurchaseReservedInstancesOfferingInput) bool { + got = in + return true + })).Return(&ec2.PurchaseReservedInstancesOfferingOutput{ReservedInstancesId: aws.String("ri-x")}, nil) + m.On("CreateTags", mock.Anything, mock.Anything).Return(&ec2.CreateTagsOutput{}, nil).Maybe() + _, err := (&Client{client: m, region: "us-east-1"}).PurchaseCommitment(context.Background(), pricedRec(), common.PurchaseOptions{}) + require.NoError(t, err) + require.NotNil(t, got.LimitPrice) + assert.InDelta(t, 1050.0, aws.ToFloat64(got.LimitPrice.Amount), 0.001) + assert.Equal(t, types.CurrencyCodeValuesUsd, got.LimitPrice.CurrencyCode) +} + +func TestPurchaseCommitment_NoLimitPriceWithoutApprovedUpfront(t *testing.T) { + t.Parallel() + var got *ec2.PurchaseReservedInstancesOfferingInput + m := &MockEC2Client{} + m.On("DescribeReservedInstancesOfferings", mock.Anything, mock.Anything). + Return(&ec2.DescribeReservedInstancesOfferingsOutput{ReservedInstancesOfferings: []types.ReservedInstancesOffering{fullOffering("n", "")}}, nil) + m.On("PurchaseReservedInstancesOffering", mock.Anything, mock.MatchedBy(func(in *ec2.PurchaseReservedInstancesOfferingInput) bool { + got = in + return true + })).Return(&ec2.PurchaseReservedInstancesOfferingOutput{ReservedInstancesId: aws.String("ri-x")}, nil) + m.On("CreateTags", mock.Anything, mock.Anything).Return(&ec2.CreateTagsOutput{}, nil).Maybe() + _, err := (&Client{client: m, region: "us-east-1"}).PurchaseCommitment(context.Background(), matchRec(), common.PurchaseOptions{}) + require.NoError(t, err) + assert.Nil(t, got.LimitPrice) +} + +func TestUpfrontLimit_RefusesMissingPriceAndNonUSD(t *testing.T) { + t.Parallel() + noPrice := fullOffering("p", "") + _, err := upfrontLimit(pricedRec(), &noPrice) + require.ErrorContains(t, err, "no FixedPrice") + + eur := pricedOffering(500) + eur.CurrencyCode = types.CurrencyCodeValues("EUR") + _, err = upfrontLimit(pricedRec(), &eur) + require.ErrorContains(t, err, "USD only") +} From 55cecd4e3d26eb2b73d769a0eb2b63479d51a396 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 21 Oct 2026 03:37:30 +0200 Subject: [PATCH 2/2] fix(aws/ec2): drop LimitPrice, check the approved currency, keep zero unchecked AWS documents LimitPrice only for Marketplace offerings, which findOffering excludes, so it protected nothing and might be rejected. The local upfront refusal stays unchanged. A recommendation whose commitment currency is set and is not USD is now refused. Zero CommitmentCost stays unchecked because the MCP EC2 tool never sets it. Part of #53 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 +- providers/aws/services/ec2/client.go | 35 ++++++----- .../services/ec2/purchase_price_guard_test.go | 59 +++++++++---------- 3 files changed, 49 insertions(+), 51 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 282f8308..d26317bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -221,9 +221,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ElastiCache reports it, and verify the chosen offering's node type, duration and engine before buying it (#53). - AWS EC2 RI purchases refuse an offering whose total upfront exceeds the - recommendation's approved upfront by more than 5%, and send the same ceiling - as the API's `LimitPrice` (USD only), so a repriced or differently-priced - offering can no longer bypass the approved cap (#53). + recommendation's approved upfront by more than 5% (USD only; a + recommendation without an upfront figure is not checked), so a repriced or + differently-priced offering can no longer bypass the approved cap (#53). - **`ReshapeBuffer` rejects nil payment caps** instead of mapping them to `math.MaxFloat64`: `MaxPaymentPerExchangeUSD` and `MaxPaymentDailyUSD` are now required, so an omitted cap can no longer mean unlimited spend on diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index 133e1afc..5933556b 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -167,8 +167,7 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati return result, result.Error } - limit, err := upfrontLimit(rec, offering) - if err != nil { + if err = checkApprovedUpfront(rec, offering); err != nil { result.Error = err return result, result.Error } @@ -177,7 +176,6 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati 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 - LimitPrice: limit, } // Execute the purchase. A single attempt: the API has no ClientToken, so an @@ -224,32 +222,33 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati return result, nil } -// upfrontLimit checks the resolved offering's total upfront against the -// approved recommendation's upfront and returns the server-side LimitPrice that -// caps the order at the same ceiling. It returns nil, nil when the -// recommendation has no positive upfront to cap (no-upfront options, or a -// missing figure). An offering that exceeds the ceiling, or that is priced in a -// currency LimitPrice cannot express, is refused. -func upfrontLimit(rec common.Recommendation, offering *types.ReservedInstancesOffering) (*types.ReservedInstanceLimitPrice, error) { +// checkApprovedUpfront refuses an offering whose total upfront exceeds the +// approved recommendation's upfront by more than offeringprice.UpfrontTolerance, +// or whose price cannot be compared with it. The API's LimitPrice is not used: +// AWS documents it for Marketplace offerings only, and findOffering excludes +// them. +func checkApprovedUpfront(rec common.Recommendation, offering *types.ReservedInstancesOffering) error { approved := rec.CommitmentCost + // Zero means no upfront or no figure (the MCP EC2 tool never sets it), so + // there is nothing to compare; refusing would break those purchases. if approved <= 0 { - return nil, nil + return nil } id := aws.ToString(offering.ReservedInstancesOfferingId) + if cur := rec.CommitmentCurrency; cur != "" && !strings.EqualFold(cur, "USD") { + return fmt.Errorf("EC2 recommendation upfront is in %s; the approved upfront check supports USD only", cur) + } if offering.FixedPrice == nil { - return nil, fmt.Errorf("EC2 offering %s has no FixedPrice; cannot compare it with the approved upfront %.2f", id, approved) + return fmt.Errorf("EC2 offering %s has no FixedPrice; cannot compare it with the approved upfront %.2f", id, approved) } if cc := offering.CurrencyCode; cc != "" && cc != types.CurrencyCodeValuesUsd { - return nil, fmt.Errorf("EC2 offering %s is priced in %s; the approved upfront cap supports USD only", id, cc) + return fmt.Errorf("EC2 offering %s is priced in %s; the approved upfront check supports USD only", id, cc) } total := fixedPriceCents(*offering.FixedPrice) * float64(rec.Count) / 100 if err := offeringprice.CheckUpfront("EC2", total, approved); err != nil { - return nil, fmt.Errorf("EC2 offering %s: %w", id, err) + return fmt.Errorf("EC2 offering %s: %w", id, err) } - return &types.ReservedInstanceLimitPrice{ - Amount: aws.Float64(math.Round(offeringprice.UpfrontCeiling(approved)*100) / 100), - CurrencyCode: types.CurrencyCodeValuesUsd, - }, nil + return nil } // findRIByIdempotencyToken looks for an active or payment-pending Reserved diff --git a/providers/aws/services/ec2/purchase_price_guard_test.go b/providers/aws/services/ec2/purchase_price_guard_test.go index d10859b5..19ca6be5 100644 --- a/providers/aws/services/ec2/purchase_price_guard_test.go +++ b/providers/aws/services/ec2/purchase_price_guard_test.go @@ -41,47 +41,46 @@ func TestPurchaseCommitment_RefusesOfferingAboveApprovedUpfront(t *testing.T) { m.AssertNotCalled(t, "PurchaseReservedInstancesOffering", mock.Anything, mock.Anything) } -func TestPurchaseCommitment_SetsLimitPriceAtApprovedCeiling(t *testing.T) { +func TestPurchaseCommitment_BuysAtApprovedUpfront(t *testing.T) { t.Parallel() - var got *ec2.PurchaseReservedInstancesOfferingInput - m := offeringsMock(pricedOffering(500)) // 2 x 500 = 1000 == approved - m.ExpectedCalls = m.ExpectedCalls[:1] // drop the permissive Purchase stub - m.On("PurchaseReservedInstancesOffering", mock.Anything, mock.MatchedBy(func(in *ec2.PurchaseReservedInstancesOfferingInput) bool { - got = in - return true - })).Return(&ec2.PurchaseReservedInstancesOfferingOutput{ReservedInstancesId: aws.String("ri-x")}, nil) - m.On("CreateTags", mock.Anything, mock.Anything).Return(&ec2.CreateTagsOutput{}, nil).Maybe() - _, err := (&Client{client: m, region: "us-east-1"}).PurchaseCommitment(context.Background(), pricedRec(), common.PurchaseOptions{}) + m := offeringsMock(pricedOffering(500)) // 2 x 500 == approved 1000 + res, err := (&Client{client: m, region: "us-east-1"}).PurchaseCommitment(context.Background(), pricedRec(), common.PurchaseOptions{}) require.NoError(t, err) - require.NotNil(t, got.LimitPrice) - assert.InDelta(t, 1050.0, aws.ToFloat64(got.LimitPrice.Amount), 0.001) - assert.Equal(t, types.CurrencyCodeValuesUsd, got.LimitPrice.CurrencyCode) + assert.True(t, res.Success) + m.AssertCalled(t, "PurchaseReservedInstancesOffering", mock.Anything, mock.MatchedBy(func(in *ec2.PurchaseReservedInstancesOfferingInput) bool { + return in.LimitPrice == nil + }), mock.Anything) } -func TestPurchaseCommitment_NoLimitPriceWithoutApprovedUpfront(t *testing.T) { +// The MCP EC2 tool never sets CommitmentCost; zero must not block the buy. +func TestPurchaseCommitment_NoApprovedUpfrontIsNotChecked(t *testing.T) { t.Parallel() - var got *ec2.PurchaseReservedInstancesOfferingInput - m := &MockEC2Client{} - m.On("DescribeReservedInstancesOfferings", mock.Anything, mock.Anything). - Return(&ec2.DescribeReservedInstancesOfferingsOutput{ReservedInstancesOfferings: []types.ReservedInstancesOffering{fullOffering("n", "")}}, nil) - m.On("PurchaseReservedInstancesOffering", mock.Anything, mock.MatchedBy(func(in *ec2.PurchaseReservedInstancesOfferingInput) bool { - got = in - return true - })).Return(&ec2.PurchaseReservedInstancesOfferingOutput{ReservedInstancesId: aws.String("ri-x")}, nil) - m.On("CreateTags", mock.Anything, mock.Anything).Return(&ec2.CreateTagsOutput{}, nil).Maybe() - _, err := (&Client{client: m, region: "us-east-1"}).PurchaseCommitment(context.Background(), matchRec(), common.PurchaseOptions{}) + m := offeringsMock(pricedOffering(9000)) + rec := pricedRec() + rec.CommitmentCost = 0 + _, err := (&Client{client: m, region: "us-east-1"}).PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{}) require.NoError(t, err) - assert.Nil(t, got.LimitPrice) } -func TestUpfrontLimit_RefusesMissingPriceAndNonUSD(t *testing.T) { +func TestCheckApprovedUpfront_Currency(t *testing.T) { + t.Parallel() + off := pricedOffering(500) + for _, cur := range []string{"", "USD", "usd"} { + rec := pricedRec() + rec.CommitmentCurrency = cur + require.NoError(t, checkApprovedUpfront(rec, &off), "currency %q", cur) + } + rec := pricedRec() + rec.CommitmentCurrency = "EUR" + require.ErrorContains(t, checkApprovedUpfront(rec, &off), "EUR") +} + +func TestCheckApprovedUpfront_RefusesMissingPriceAndNonUSDOffering(t *testing.T) { t.Parallel() noPrice := fullOffering("p", "") - _, err := upfrontLimit(pricedRec(), &noPrice) - require.ErrorContains(t, err, "no FixedPrice") + require.ErrorContains(t, checkApprovedUpfront(pricedRec(), &noPrice), "no FixedPrice") eur := pricedOffering(500) eur.CurrencyCode = types.CurrencyCodeValues("EUR") - _, err = upfrontLimit(pricedRec(), &eur) - require.ErrorContains(t, err, "USD only") + require.ErrorContains(t, checkApprovedUpfront(pricedRec(), &eur), "USD only") }