From 84494d2c4a0a9381e030c02709c5a4158d789f6a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 25 Oct 2026 13:57:01 +0100 Subject: [PATCH 1/3] fix(aws/rds): refuse an offering priced above the approved upfront PurchaseCommitment read no price before buying. It now reads the resolved offering back by ID and refuses when FixedPrice x count exceeds the approved upfront by more than 5%, or when either side is non-USD. A zero approved upfront (the MCP path, no-upfront) is not checked. Mirrors the EC2 guard (#611) and reuses offeringprice.CheckUpfront. Refs #53 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 + providers/aws/services/ec2/client.go | 2 +- providers/aws/services/rds/client.go | 43 +++++++ .../services/rds/purchase_price_guard_test.go | 116 ++++++++++++++++++ 4 files changed, 162 insertions(+), 1 deletion(-) create mode 100644 providers/aws/services/rds/purchase_price_guard_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index d26317bf..72918443 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -215,6 +215,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- **AWS/RDS:** `PurchaseCommitment` refuses an offering whose upfront (`FixedPrice` x count) exceeds the approved recommendation upfront by more than 5%, or that is priced in a non-USD currency; a zero approved upfront is not checked (#53). + - Bump Go to 1.26.10 to pick up the fix for GO-2026-6809 (crypto/x509), which `govulncheck` now reports against 1.26.9. - AWS ElastiCache purchases refuse a recommendation with no engine instead of sending an empty `ProductDescription` filter, send the engine lowercased as diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index 5933556b..99f6d609 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -226,7 +226,7 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati // 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. +// them. rds.Client.checkApprovedUpfront mirrors this (keep them in step). 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 diff --git a/providers/aws/services/rds/client.go b/providers/aws/services/rds/client.go index 3bec057c..0989d869 100644 --- a/providers/aws/services/rds/client.go +++ b/providers/aws/services/rds/client.go @@ -148,6 +148,11 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati return result, result.Error } + if err = c.checkApprovedUpfront(ctx, rec, offeringID); err != nil { + result.Error = err + return result, result.Error + } + reservationID := c.deriveReservationID(rec, opts) // Idempotency dedupe guard (issue #641). When a token is supplied, look for a @@ -204,6 +209,44 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati return result, nil } +// checkApprovedUpfront refuses an offering whose total upfront (FixedPrice x +// count) exceeds the approved recommendation's upfront by more than +// offeringprice.UpfrontTolerance, or whose price cannot be compared with it. +// Mirrors ec2.checkApprovedUpfront (same semantics; keep them in step). The +// shared comparison is offeringprice.CheckUpfront. RDS has no server-side price +// cap, so this is the only guard. The offering is read back by ID, and only +// when an upfront was approved: CommitmentCost 0 means no upfront or no figure +// (the MCP never sets it), so there is nothing to compare. +func (c *Client) checkApprovedUpfront(ctx context.Context, rec common.Recommendation, offeringID string) error { + approved := rec.CommitmentCost + if approved <= 0 { + return nil + } + if cur := rec.CommitmentCurrency; cur != "" && !strings.EqualFold(cur, "USD") { + return fmt.Errorf("RDS recommendation upfront is in %s; the approved upfront check supports USD only", cur) + } + out, err := c.client.DescribeReservedDBInstancesOfferings(ctx, &rds.DescribeReservedDBInstancesOfferingsInput{ + ReservedDBInstancesOfferingId: aws.String(offeringID), + }) + if err != nil { + return fmt.Errorf("failed to read RDS offering %s for the approved upfront check: %w", offeringID, err) + } + if len(out.ReservedDBInstancesOfferings) != 1 { + return fmt.Errorf("RDS offering %s: expected 1 offering for the approved upfront check, got %d", offeringID, len(out.ReservedDBInstancesOfferings)) + } + offering := out.ReservedDBInstancesOfferings[0] + if offering.FixedPrice == nil { + return fmt.Errorf("RDS offering %s has no FixedPrice; cannot compare it with the approved upfront %.2f", offeringID, approved) + } + if cc := aws.ToString(offering.CurrencyCode); cc != "" && !strings.EqualFold(cc, "USD") { + return fmt.Errorf("RDS offering %s is priced in %s; the approved upfront check supports USD only", offeringID, cc) + } + if err := offeringprice.CheckUpfront("RDS", *offering.FixedPrice*float64(rec.Count), approved); err != nil { + return fmt.Errorf("RDS offering %s: %w", offeringID, err) + } + return nil +} + // deriveReservationID returns the ReservedDBInstanceId to use. When an // idempotency token is supplied (issue #641) the ID is derived deterministically // from it, so a re-drive sends the identical ID and RDS rejects the duplicate diff --git a/providers/aws/services/rds/purchase_price_guard_test.go b/providers/aws/services/rds/purchase_price_guard_test.go new file mode 100644 index 00000000..24b73ceb --- /dev/null +++ b/providers/aws/services/rds/purchase_price_guard_test.go @@ -0,0 +1,116 @@ +package rds + +import ( + "context" + "testing" + + "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" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +// guardMock serves the exact-match page lookup, then the by-ID price read with +// the given FixedPrice and currency, and accepts the purchase. +func guardMock(fixed float64, currency string) *MockRDSClient { + m := &MockRDSClient{} + byID := func(in *rds.DescribeReservedDBInstancesOfferingsInput) bool { + return aws.ToString(in.ReservedDBInstancesOfferingId) == "offering-1" + } + priced := exactOffering("offering-1") + priced.FixedPrice = aws.Float64(fixed) + if currency != "" { + priced.CurrencyCode = aws.String(currency) + } + m.On("DescribeReservedDBInstancesOfferings", mock.Anything, mock.MatchedBy(byID)). + Return(&rds.DescribeReservedDBInstancesOfferingsOutput{ReservedDBInstancesOfferings: []types.ReservedDBInstancesOffering{priced}}, nil) + m.On("DescribeReservedDBInstancesOfferings", mock.Anything, mock.MatchedBy(func(in *rds.DescribeReservedDBInstancesOfferingsInput) bool { return !byID(in) })). + Return(&rds.DescribeReservedDBInstancesOfferingsOutput{ReservedDBInstancesOfferings: []types.ReservedDBInstancesOffering{exactOffering("offering-1")}}, nil) + m.On("PurchaseReservedDBInstancesOffering", mock.Anything, mock.Anything). + Return(&rds.PurchaseReservedDBInstancesOfferingOutput{ReservedDBInstance: &types.ReservedDBInstance{ReservedDBInstanceId: aws.String("ri-1")}}, nil) + return m +} + +// guardRec is a 2-instance all-upfront rec approved at a $1000 total upfront. +func guardRec() common.Recommendation { + r := idempotencyTestRec() + r.Count = 2 + r.CommitmentCost = 1000 + return r +} + +func buy(m *MockRDSClient, rec common.Recommendation) (common.PurchaseResult, error) { + return (&Client{client: m, region: "us-east-1"}).PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{}) +} + +func TestPurchaseCommitment_RefusesOfferingAboveApprovedUpfront(t *testing.T) { + t.Parallel() + m := guardMock(525.01, "USD") // 2 x 525.01 = 1050.02 > ceiling 1050.00 + res, err := buy(m, guardRec()) + require.ErrorContains(t, err, "exceeds the approved upfront") + assert.ErrorContains(t, err, "1050.02") + assert.ErrorContains(t, err, "1000.00") + assert.False(t, res.Success) + m.AssertNotCalled(t, "PurchaseReservedDBInstancesOffering", mock.Anything, mock.Anything) +} + +func TestPurchaseCommitment_BuysAtExactlyTolerance(t *testing.T) { + t.Parallel() + m := guardMock(525, "USD") // 2 x 525 = 1050 == ceiling + res, err := buy(m, guardRec()) + require.NoError(t, err) + assert.True(t, res.Success) + m.AssertCalled(t, "PurchaseReservedDBInstancesOffering", mock.Anything, mock.Anything) +} + +// The MCP never sets CommitmentCost; zero must not block the buy. +func TestPurchaseCommitment_ZeroApprovedUpfrontIsNotChecked(t *testing.T) { + t.Parallel() + m := guardMock(9000, "USD") + rec := guardRec() + rec.CommitmentCost = 0 + _, err := buy(m, rec) + require.NoError(t, err) +} + +func TestPurchaseCommitment_RefusesNonUSD(t *testing.T) { + t.Parallel() + m := guardMock(400, "EUR") + _, err := buy(m, guardRec()) + require.ErrorContains(t, err, "EUR") + m.AssertNotCalled(t, "PurchaseReservedDBInstancesOffering", mock.Anything, mock.Anything) + + m = guardMock(400, "USD") + rec := guardRec() + rec.CommitmentCurrency = "eur" + _, err = buy(m, rec) + require.ErrorContains(t, err, "eur") + m.AssertNotCalled(t, "PurchaseReservedDBInstancesOffering", mock.Anything, mock.Anything) +} + +func TestPurchaseCommitment_EmptyCurrencyIsUSD(t *testing.T) { + t.Parallel() + _, err := buy(guardMock(500, ""), guardRec()) + require.NoError(t, err) +} + +// No-upfront recs carry CommitmentCost 0, so the price is never read back. +func TestPurchaseCommitment_NoUpfrontUnaffected(t *testing.T) { + t.Parallel() + m := &MockRDSClient{} + noUpfront := exactOffering("offering-1") + noUpfront.OfferingType = aws.String("No Upfront") + m.On("DescribeReservedDBInstancesOfferings", mock.Anything, mock.Anything). + Return(&rds.DescribeReservedDBInstancesOfferingsOutput{ReservedDBInstancesOfferings: []types.ReservedDBInstancesOffering{noUpfront}}, nil) + m.On("PurchaseReservedDBInstancesOffering", mock.Anything, mock.Anything). + Return(&rds.PurchaseReservedDBInstancesOfferingOutput{ReservedDBInstance: &types.ReservedDBInstance{ReservedDBInstanceId: aws.String("ri-1")}}, nil) + rec := idempotencyTestRec() + rec.PaymentOption = "no-upfront" + _, err := buy(m, rec) + require.NoError(t, err) + m.AssertNumberOfCalls(t, "DescribeReservedDBInstancesOfferings", 1) +} From 1cebf623f4f1a2a8a90da51362efad2423a02f70 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 25 Oct 2026 14:11:49 +0100 Subject: [PATCH 2/3] fix(aws/rds): run the price guard after the idempotency dedupe A re-drive whose RI already exists must return it, not be refused for a later price rise. Matches the EC2 order. Also drops a stray blank line in the CHANGELOG list. Refs #53 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 - providers/aws/services/rds/client.go | 12 +++++++----- .../services/rds/purchase_price_guard_test.go | 19 +++++++++++++++++++ 3 files changed, 26 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 72918443..9ccf5706 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -216,7 +216,6 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed - **AWS/RDS:** `PurchaseCommitment` refuses an offering whose upfront (`FixedPrice` x count) exceeds the approved recommendation upfront by more than 5%, or that is priced in a non-USD currency; a zero approved upfront is not checked (#53). - - Bump Go to 1.26.10 to pick up the fix for GO-2026-6809 (crypto/x509), which `govulncheck` now reports against 1.26.9. - AWS ElastiCache purchases refuse a recommendation with no engine instead of sending an empty `ProductDescription` filter, send the engine lowercased as diff --git a/providers/aws/services/rds/client.go b/providers/aws/services/rds/client.go index 0989d869..f75be7c7 100644 --- a/providers/aws/services/rds/client.go +++ b/providers/aws/services/rds/client.go @@ -148,11 +148,6 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati return result, result.Error } - if err = c.checkApprovedUpfront(ctx, rec, offeringID); err != nil { - result.Error = err - return result, result.Error - } - reservationID := c.deriveReservationID(rec, opts) // Idempotency dedupe guard (issue #641). When a token is supplied, look for a @@ -169,6 +164,13 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati return result, nil } + // After the dedupe: a re-drive that already bought must not be refused for a + // later price rise (ec2/client.go does the same). + if err = c.checkApprovedUpfront(ctx, rec, offeringID); err != nil { + result.Error = err + return result, result.Error + } + // Create the purchase request input := &rds.PurchaseReservedDBInstancesOfferingInput{ ReservedDBInstancesOfferingId: aws.String(offeringID), diff --git a/providers/aws/services/rds/purchase_price_guard_test.go b/providers/aws/services/rds/purchase_price_guard_test.go index 24b73ceb..4cddc36d 100644 --- a/providers/aws/services/rds/purchase_price_guard_test.go +++ b/providers/aws/services/rds/purchase_price_guard_test.go @@ -114,3 +114,22 @@ func TestPurchaseCommitment_NoUpfrontUnaffected(t *testing.T) { require.NoError(t, err) m.AssertNumberOfCalls(t, "DescribeReservedDBInstancesOfferings", 1) } + +// A re-drive whose RI already exists must return it even after the offering +// was repriced above the approved upfront: nothing is bought, so no refusal. +func TestPurchaseCommitment_ExistingRIWinsOverRepricedOffering(t *testing.T) { + t.Parallel() + m := guardMock(600, "USD") // 2 x 600 = 1200 against an approved 1000 + token := common.DeriveIdempotencyToken("exec-53", 0) + derivedID := common.IdempotentReservationID("rds-id-", token) + m.On("DescribeReservedDBInstances", mock.Anything, mock.Anything). + Return(&rds.DescribeReservedDBInstancesOutput{ReservedDBInstances: []types.ReservedDBInstance{ + {ReservedDBInstanceId: aws.String(derivedID), State: aws.String("active")}, + }}, nil) + res, err := (&Client{client: m, region: "us-east-1"}).PurchaseCommitment(context.Background(), guardRec(), common.PurchaseOptions{IdempotencyToken: token}) + require.NoError(t, err) + assert.True(t, res.Success) + assert.True(t, res.ExistingCommitment) + assert.Equal(t, derivedID, res.CommitmentID) + m.AssertNotCalled(t, "PurchaseReservedDBInstancesOffering", mock.Anything, mock.Anything) +} From e231bf31bb3e8f2157acd6408ab21b8634a5ab99 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 25 Oct 2026 14:39:28 +0100 Subject: [PATCH 3/3] test(aws/rds): kill surviving mutants in the approved upfront guard Cover a failed price read, a price read returning other than one offering, a nil FixedPrice, and lowercase usd on the rec and the offering. Refs #53 Co-Authored-By: Claude Opus 5.5 --- .../services/rds/purchase_price_guard_test.go | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/providers/aws/services/rds/purchase_price_guard_test.go b/providers/aws/services/rds/purchase_price_guard_test.go index 4cddc36d..11f5e246 100644 --- a/providers/aws/services/rds/purchase_price_guard_test.go +++ b/providers/aws/services/rds/purchase_price_guard_test.go @@ -2,6 +2,7 @@ package rds import ( "context" + "errors" "testing" "github.com/aws/aws-sdk-go-v2/aws" @@ -133,3 +134,59 @@ func TestPurchaseCommitment_ExistingRIWinsOverRepricedOffering(t *testing.T) { assert.Equal(t, derivedID, res.CommitmentID) m.AssertNotCalled(t, "PurchaseReservedDBInstancesOffering", mock.Anything, mock.Anything) } + +// byIDMock serves the exact-match page lookup, then answers the by-ID price +// read with the given offerings or error. +func byIDMock(byID []types.ReservedDBInstancesOffering, byIDErr error) *MockRDSClient { + m := &MockRDSClient{} + isByID := func(in *rds.DescribeReservedDBInstancesOfferingsInput) bool { + return aws.ToString(in.ReservedDBInstancesOfferingId) == "offering-1" + } + m.On("DescribeReservedDBInstancesOfferings", mock.Anything, mock.MatchedBy(isByID)). + Return(&rds.DescribeReservedDBInstancesOfferingsOutput{ReservedDBInstancesOfferings: byID}, byIDErr) + m.On("DescribeReservedDBInstancesOfferings", mock.Anything, mock.MatchedBy(func(in *rds.DescribeReservedDBInstancesOfferingsInput) bool { return !isByID(in) })). + Return(&rds.DescribeReservedDBInstancesOfferingsOutput{ReservedDBInstancesOfferings: []types.ReservedDBInstancesOffering{exactOffering("offering-1")}}, nil) + m.On("PurchaseReservedDBInstancesOffering", mock.Anything, mock.Anything). + Return(&rds.PurchaseReservedDBInstancesOfferingOutput{ReservedDBInstance: &types.ReservedDBInstance{ReservedDBInstanceId: aws.String("ri-1")}}, nil) + return m +} + +func TestPurchaseCommitment_RefusesWhenPriceReadFails(t *testing.T) { + t.Parallel() + m := byIDMock(nil, errors.New("throttled")) + res, err := buy(m, guardRec()) + require.ErrorContains(t, err, "approved upfront check") + assert.ErrorContains(t, err, "throttled") + assert.False(t, res.Success) + m.AssertNotCalled(t, "PurchaseReservedDBInstancesOffering", mock.Anything, mock.Anything) +} + +func TestPurchaseCommitment_RefusesWhenPriceReadIsNotOneOffering(t *testing.T) { + t.Parallel() + two := []types.ReservedDBInstancesOffering{exactOffering("offering-1"), exactOffering("offering-1")} + for _, n := range [][]types.ReservedDBInstancesOffering{two, nil} { + m := byIDMock(n, nil) + _, err := buy(m, guardRec()) + require.ErrorContains(t, err, "expected 1 offering") + m.AssertNotCalled(t, "PurchaseReservedDBInstancesOffering", mock.Anything, mock.Anything) + } +} + +func TestPurchaseCommitment_RefusesOfferingWithoutFixedPrice(t *testing.T) { + t.Parallel() + m := byIDMock([]types.ReservedDBInstancesOffering{exactOffering("offering-1")}, nil) // FixedPrice nil + _, err := buy(m, guardRec()) + require.ErrorContains(t, err, "no FixedPrice") + m.AssertNotCalled(t, "PurchaseReservedDBInstancesOffering", mock.Anything, mock.Anything) +} + +func TestPurchaseCommitment_LowercaseUSDBuys(t *testing.T) { + t.Parallel() + rec := guardRec() + rec.CommitmentCurrency = "usd" + m := guardMock(500, "usd") + res, err := buy(m, rec) + require.NoError(t, err) + assert.True(t, res.Success) + m.AssertCalled(t, "PurchaseReservedDBInstancesOffering", mock.Anything, mock.Anything) +}