diff --git a/CHANGELOG.md b/CHANGELOG.md index d26317bf..9ccf5706 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -215,6 +215,7 @@ 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..f75be7c7 100644 --- a/providers/aws/services/rds/client.go +++ b/providers/aws/services/rds/client.go @@ -164,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), @@ -204,6 +211,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..11f5e246 --- /dev/null +++ b/providers/aws/services/rds/purchase_price_guard_test.go @@ -0,0 +1,192 @@ +package rds + +import ( + "context" + "errors" + "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) +} + +// 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) +} + +// 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) +}