diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ccf5706..2fa642c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -217,6 +217,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). - **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/MemoryDB, OpenSearch, Redshift:** `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 (MemoryDB offerings carry no currency, so only the recommendation's currency is checked there), or, for MemoryDB, an upfront offering with a zero price or one bought in a China region (cn-north-1, cn-northwest-1), where the assumed USD price is wrong; a zero approved upfront is not checked. All AWS RI price guards (EC2, RDS, MemoryDB, OpenSearch, Redshift) also refuse a NaN or infinite approved upfront (#53). - AWS ElastiCache purchases refuse a recommendation with no engine instead of sending an empty `ProductDescription` filter, send the engine lowercased as ElastiCache reports it, and verify the chosen offering's node type, duration diff --git a/providers/aws/internal/offeringprice/offeringprice.go b/providers/aws/internal/offeringprice/offeringprice.go index 002a3e69..fbc9fd33 100644 --- a/providers/aws/internal/offeringprice/offeringprice.go +++ b/providers/aws/internal/offeringprice/offeringprice.go @@ -149,10 +149,22 @@ func UpfrontCeiling(approved float64) float64 { return approved * (1 + UpfrontTolerance) } +// NoUpfrontApproved reports whether approved gives no ceiling to check: zero +// or negative means a no-upfront option or a missing figure. NaN and +-Inf are +// not "no figure" (a corrupt one), so they return false and CheckUpfront +// refuses them. +func NoUpfrontApproved(approved float64) bool { + return approved <= 0 && !math.IsInf(approved, -1) +} + // 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. +// compared in whole cents, and when approved is NaN or infinite (every +// comparison against those is false, so they would pass). Callers skip the +// call when NoUpfrontApproved(approved). func CheckUpfront(service string, offeredTotal, approved float64) error { + if math.IsNaN(approved) || math.IsInf(approved, 0) { + return fmt.Errorf("%s approved upfront %v is not a finite amount; refusing to purchase", service, approved) + } 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) diff --git a/providers/aws/internal/offeringprice/offeringprice_test.go b/providers/aws/internal/offeringprice/offeringprice_test.go index ef375bae..557a1f23 100644 --- a/providers/aws/internal/offeringprice/offeringprice_test.go +++ b/providers/aws/internal/offeringprice/offeringprice_test.go @@ -1,6 +1,7 @@ package offeringprice import ( + "math" "testing" "github.com/stretchr/testify/assert" @@ -65,3 +66,24 @@ func TestCheckUpfront_BoundaryIsTheCeilingInCents(t *testing.T) { require.ErrorContains(t, CheckUpfront("EC2", 1050.01, 1000), "exceeds the approved upfront") require.NoError(t, CheckUpfront("EC2", 0, 1000)) } + +func TestCheckUpfront_RefusesNonFiniteApproved(t *testing.T) { + t.Parallel() + for _, svc := range []string{"EC2", "RDS", "MemoryDB", "OpenSearch", "Redshift"} { + for _, approved := range []float64{math.NaN(), math.Inf(1), math.Inf(-1)} { + err := CheckUpfront(svc, 100, approved) + require.ErrorContains(t, err, svc, "%v", approved) + assert.ErrorContains(t, err, "not a finite amount", "%v", approved) + } + } +} + +func TestNoUpfrontApproved(t *testing.T) { + t.Parallel() + assert.True(t, NoUpfrontApproved(0)) + assert.True(t, NoUpfrontApproved(-5)) + assert.False(t, NoUpfrontApproved(1)) + assert.False(t, NoUpfrontApproved(math.NaN())) + assert.False(t, NoUpfrontApproved(math.Inf(1))) + assert.False(t, NoUpfrontApproved(math.Inf(-1))) +} diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index 99f6d609..6642300c 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -231,7 +231,7 @@ func checkApprovedUpfront(rec common.Recommendation, offering *types.ReservedIns 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 { + if offeringprice.NoUpfrontApproved(approved) { return nil } id := aws.ToString(offering.ReservedInstancesOfferingId) diff --git a/providers/aws/services/memorydb/client.go b/providers/aws/services/memorydb/client.go index fffdd703..556679ec 100644 --- a/providers/aws/services/memorydb/client.go +++ b/providers/aws/services/memorydb/client.go @@ -7,6 +7,7 @@ import ( "fmt" "log" "sort" + "strings" "time" "github.com/aws/aws-sdk-go-v2/aws" @@ -166,6 +167,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 + } + input := &memorydb.PurchaseReservedNodesOfferingInput{ ReservedNodesOfferingId: aws.String(offeringID), ReservationId: aws.String(reservationID), @@ -206,6 +214,60 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati return result, nil } +// checkApprovedUpfront refuses an offering whose total upfront (FixedPrice, per +// node, x count) exceeds the approved recommendation's upfront by more than +// offeringprice.UpfrontTolerance, or whose price cannot be compared with it. +// Mirrors rds.checkApprovedUpfront (same semantics; keep them in step). The +// shared comparison is offeringprice.CheckUpfront. MemoryDB 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, so there is nothing to compare. The offering carries no currency +// field, so only the recommendation's currency can be checked. +func (c *Client) checkApprovedUpfront(ctx context.Context, rec common.Recommendation, offeringID string) error { + approved := rec.CommitmentCost + if offeringprice.NoUpfrontApproved(approved) { + return nil + } + if cur := rec.CommitmentCurrency; cur != "" && !strings.EqualFold(cur, "USD") { + return fmt.Errorf("MemoryDB recommendation upfront is in %s; the approved upfront check supports USD only", cur) + } + out, err := c.client.DescribeReservedNodesOfferings(ctx, &memorydb.DescribeReservedNodesOfferingsInput{ + ReservedNodesOfferingId: aws.String(offeringID), + }) + if err != nil { + return fmt.Errorf("failed to read MemoryDB offering %s for the approved upfront check: %w", offeringID, err) + } + if len(out.ReservedNodesOfferings) != 1 { + return fmt.Errorf("MemoryDB offering %s: expected 1 offering for the approved upfront check, got %d", offeringID, len(out.ReservedNodesOfferings)) + } + off := out.ReservedNodesOfferings[0] + // FixedPrice is a plain float64, so 0 on an upfront offering means the price + // is missing (other services refuse a nil price); No Upfront is legitimately 0. + if off.FixedPrice == 0 && aws.ToString(off.OfferingType) != "No Upfront" { + return fmt.Errorf("MemoryDB offering %s: upfront offering has no FixedPrice; refusing to buy against an approved upfront of %.2f", offeringID, approved) + } + if err := c.checkAssumedUSD(off, offeringID); err != nil { + return err + } + if err := offeringprice.CheckUpfront("MemoryDB", off.FixedPrice*float64(rec.Count), approved); err != nil { + return fmt.Errorf("MemoryDB offering %s: %w", offeringID, err) + } + return nil +} + +// checkAssumedUSD refuses a priced offering in a region not billed in USD: the +// offering has no currency field, so the region decides it. No Upfront has no +// price to compare. +func (c *Client) checkAssumedUSD(off types.ReservedNodesOffering, offeringID string) error { + if aws.ToString(off.OfferingType) == "No Upfront" { + return nil + } + if _, err := offeringprice.AssumedUSD("MemoryDB", c.region); err != nil { + return fmt.Errorf("MemoryDB offering %s: %w", offeringID, err) + } + return nil +} + // findReservationByID looks for a nonterminal MemoryDB reserved // node with the given ReservationId (issue #641), so a re-driven purchase can // short-circuit instead of buying a second node. Retired/expired nodes are diff --git a/providers/aws/services/memorydb/purchase_price_guard_test.go b/providers/aws/services/memorydb/purchase_price_guard_test.go new file mode 100644 index 00000000..67eba173 --- /dev/null +++ b/providers/aws/services/memorydb/purchase_price_guard_test.go @@ -0,0 +1,229 @@ +package memorydb + +import ( + "context" + "errors" + "math" + "testing" + + "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" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +func guardOffering(fixed float64) types.ReservedNodesOffering { + return types.ReservedNodesOffering{ + ReservedNodesOfferingId: aws.String("offering-1"), + NodeType: aws.String("db.r6gd.large"), + Duration: 31536000, + OfferingType: aws.String("All Upfront"), + FixedPrice: fixed, + } +} + +// byIDMock serves the page lookup, then answers the by-ID price read with the +// given offerings or error, and accepts the purchase. +func byIDMock(byID []types.ReservedNodesOffering, byIDErr error) *MockMemoryDBClient { + m := &MockMemoryDBClient{} + isByID := func(in *memorydb.DescribeReservedNodesOfferingsInput) bool { + return aws.ToString(in.ReservedNodesOfferingId) == "offering-1" + } + m.On("DescribeReservedNodesOfferings", mock.Anything, mock.MatchedBy(isByID)). + Return(&memorydb.DescribeReservedNodesOfferingsOutput{ReservedNodesOfferings: byID}, byIDErr) + m.On("DescribeReservedNodesOfferings", mock.Anything, mock.MatchedBy(func(in *memorydb.DescribeReservedNodesOfferingsInput) bool { return !isByID(in) })). + Return(&memorydb.DescribeReservedNodesOfferingsOutput{ReservedNodesOfferings: []types.ReservedNodesOffering{guardOffering(0)}}, nil) + m.On("PurchaseReservedNodesOffering", mock.Anything, mock.Anything). + Return(&memorydb.PurchaseReservedNodesOfferingOutput{ReservedNode: &types.ReservedNode{ReservationId: aws.String("rn-1")}}, nil) + return m +} + +func guardMock(fixed float64) *MockMemoryDBClient { + return byIDMock([]types.ReservedNodesOffering{guardOffering(fixed)}, nil) +} + +// guardRec is a 2-node all-upfront rec approved at a $1000 total upfront. +func guardRec() common.Recommendation { + r := mdbIdemRec() + r.Count = 2 + r.CommitmentCost = 1000 + return r +} + +func buy(m *MockMemoryDBClient, 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) // 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, "PurchaseReservedNodesOffering", mock.Anything, mock.Anything) +} + +func TestPurchaseCommitment_BuysAtExactlyTolerance(t *testing.T) { + t.Parallel() + m := guardMock(525) // 2 x 525 = 1050 == ceiling + res, err := buy(m, guardRec()) + require.NoError(t, err) + assert.True(t, res.Success) + m.AssertCalled(t, "PurchaseReservedNodesOffering", mock.Anything, mock.Anything) +} + +func TestPurchaseCommitment_ZeroApprovedUpfrontIsNotChecked(t *testing.T) { + t.Parallel() + m := guardMock(9000) + rec := guardRec() + rec.CommitmentCost = 0 + _, err := buy(m, rec) + require.NoError(t, err) + m.AssertNumberOfCalls(t, "DescribeReservedNodesOfferings", 1) +} + +func TestPurchaseCommitment_RefusesZeroPricedUpfrontOffering(t *testing.T) { + t.Parallel() + m := guardMock(0) + res, err := buy(m, guardRec()) + require.ErrorContains(t, err, "offering-1") + assert.ErrorContains(t, err, "no FixedPrice") + assert.False(t, res.Success) + m.AssertNotCalled(t, "PurchaseReservedNodesOffering", mock.Anything, mock.Anything) +} + +func TestPurchaseCommitment_RefusesNonUSDRec(t *testing.T) { + t.Parallel() + m := guardMock(400) + rec := guardRec() + rec.CommitmentCurrency = "eur" + _, err := buy(m, rec) + require.ErrorContains(t, err, "eur") + m.AssertNotCalled(t, "PurchaseReservedNodesOffering", mock.Anything, mock.Anything) +} + +func TestPurchaseCommitment_EmptyAndLowercaseUSDBuy(t *testing.T) { + t.Parallel() + for _, cur := range []string{"", "usd"} { + rec := guardRec() + rec.CommitmentCurrency = cur + _, err := buy(guardMock(500), rec) + require.NoError(t, err, cur) + } +} + +// No-upfront recs carry CommitmentCost 0, so the price is never read back. +func TestPurchaseCommitment_NoUpfrontUnaffected(t *testing.T) { + t.Parallel() + m := &MockMemoryDBClient{} + noUpfront := guardOffering(0) + noUpfront.OfferingType = aws.String("No Upfront") + m.On("DescribeReservedNodesOfferings", mock.Anything, mock.Anything). + Return(&memorydb.DescribeReservedNodesOfferingsOutput{ReservedNodesOfferings: []types.ReservedNodesOffering{noUpfront}}, nil) + m.On("PurchaseReservedNodesOffering", mock.Anything, mock.Anything). + Return(&memorydb.PurchaseReservedNodesOfferingOutput{ReservedNode: &types.ReservedNode{ReservationId: aws.String("rn-1")}}, nil) + rec := mdbIdemRec() + rec.PaymentOption = "no-upfront" + _, err := buy(m, rec) + require.NoError(t, err) + m.AssertNumberOfCalls(t, "DescribeReservedNodesOfferings", 1) +} + +// A re-drive whose node already exists must return it even after the offering +// was repriced above the approved upfront: nothing is bought, so no refusal. +func TestPurchaseCommitment_ExistingNodeWinsOverRepricedOffering(t *testing.T) { + t.Parallel() + m := guardMock(600) // 2 x 600 = 1200 against an approved 1000 + token := common.DeriveIdempotencyToken("exec-53", 0) + derivedID := common.IdempotentReservationID("memorydb-id-", token) + m.On("DescribeReservedNodes", mock.Anything, mock.Anything). + Return(&memorydb.DescribeReservedNodesOutput{ReservedNodes: []types.ReservedNode{ + {ReservationId: 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, "PurchaseReservedNodesOffering", mock.Anything, mock.Anything) +} + +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, "PurchaseReservedNodesOffering", mock.Anything, mock.Anything) +} + +func TestPurchaseCommitment_RefusesWhenPriceReadIsNotOneOffering(t *testing.T) { + t.Parallel() + two := []types.ReservedNodesOffering{guardOffering(400), guardOffering(400)} + for _, n := range [][]types.ReservedNodesOffering{two, nil} { + m := byIDMock(n, nil) + _, err := buy(m, guardRec()) + require.ErrorContains(t, err, "expected 1 offering") + m.AssertNotCalled(t, "PurchaseReservedNodesOffering", mock.Anything, mock.Anything) + } +} + +func TestPurchaseCommitment_RefusesNonFiniteApprovedUpfront(t *testing.T) { + t.Parallel() + for _, approved := range []float64{math.NaN(), math.Inf(1), math.Inf(-1)} { + m := guardMock(400) + rec := guardRec() + rec.CommitmentCost = approved + res, err := buy(m, rec) + require.ErrorContains(t, err, "not a finite amount", "%v", approved) + assert.False(t, res.Success) + m.AssertNotCalled(t, "PurchaseReservedNodesOffering", mock.Anything, mock.Anything) + } +} + +func buyIn(region string, m *MockMemoryDBClient, rec common.Recommendation) (common.PurchaseResult, error) { + return (&Client{client: m, region: region}).PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{}) +} + +func TestPurchaseCommitment_ChinaRegionRefusesApprovedUpfront(t *testing.T) { + t.Parallel() + for _, region := range []string{"cn-north-1", "cn-northwest-1"} { + m := guardMock(400) + res, err := buyIn(region, m, guardRec()) + require.ErrorContains(t, err, "not billed in USD", region) + assert.False(t, res.Success) + m.AssertNotCalled(t, "PurchaseReservedNodesOffering", mock.Anything, mock.Anything) + } +} + +func TestPurchaseCommitment_ChinaRegionZeroApprovedStillBuys(t *testing.T) { + t.Parallel() + m := guardMock(400) + rec := guardRec() + rec.CommitmentCost = 0 + _, err := buyIn("cn-north-1", m, rec) + require.NoError(t, err) + m.AssertCalled(t, "PurchaseReservedNodesOffering", mock.Anything, mock.Anything) +} + +// A No Upfront offering is exempt from the zero-FixedPrice refusal and the +// China-region refusal even when the rec carries a positive approved upfront. +func TestPurchaseCommitment_NoUpfrontOfferingWithApprovedUpfrontBuys(t *testing.T) { + t.Parallel() + for _, region := range []string{"us-east-1", "cn-north-1"} { + off := guardOffering(0) + off.OfferingType = aws.String("No Upfront") + m := byIDMock([]types.ReservedNodesOffering{off}, nil) + res, err := buyIn(region, m, guardRec()) + require.NoError(t, err, region) + assert.True(t, res.Success) + m.AssertCalled(t, "PurchaseReservedNodesOffering", mock.Anything, mock.Anything) + } +} diff --git a/providers/aws/services/opensearch/client.go b/providers/aws/services/opensearch/client.go index 57236fb6..cfbce7f6 100644 --- a/providers/aws/services/opensearch/client.go +++ b/providers/aws/services/opensearch/client.go @@ -6,6 +6,7 @@ import ( "fmt" "log" "math" + "strings" "sync" "time" @@ -183,18 +184,7 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati // identifier matching the Azure DisplayName format so operators can // identify the reservation in the AWS console without cross-referencing // CUDly's purchase audit log. - reservationName := common.IdempotentReservationID("opensearch-id-", opts.IdempotencyToken) - if reservationName == "" { - reservationName = common.BuildReservationName(common.ReservationNameFields{ - Service: "opensearch", - Region: rec.Region, - ResourceType: rec.ResourceType, - Count: rec.Count, - Term: rec.Term, - Payment: rec.PaymentOption, - Now: time.Now(), - }, "opensearch-reserved-") - } + reservationName := reservationNameFor(rec, opts.IdempotencyToken) // Idempotency dedupe guard (issue #641): short-circuit if a reservation with // the derived name already exists; fail loud on lookup error. @@ -208,6 +198,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, instanceCount); err != nil { + result.Error = err + return result, result.Error + } + input := &opensearch.PurchaseReservedInstanceOfferingInput{ ReservedInstanceOfferingId: aws.String(offeringID), ReservationName: aws.String(reservationName), @@ -247,6 +244,61 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati return result, nil } +// reservationNameFor returns the ReservationName: derived from the idempotency +// token when there is one, else the self-describing name (issue #687). +func reservationNameFor(rec common.Recommendation, token string) string { + if name := common.IdempotentReservationID("opensearch-id-", token); name != "" { + return name + } + return common.BuildReservationName(common.ReservationNameFields{ + Service: "opensearch", + Region: rec.Region, + ResourceType: rec.ResourceType, + Count: rec.Count, + Term: rec.Term, + Payment: rec.PaymentOption, + Now: time.Now(), + }, "opensearch-reserved-") +} + +// checkApprovedUpfront refuses an offering whose total upfront (FixedPrice, per +// instance, x count) exceeds the approved recommendation's upfront by more than +// offeringprice.UpfrontTolerance, or whose price cannot be compared with it. +// Mirrors rds.checkApprovedUpfront (same semantics; keep them in step). The +// shared comparison is offeringprice.CheckUpfront. OpenSearch 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, so there is nothing to compare. +func (c *Client) checkApprovedUpfront(ctx context.Context, rec common.Recommendation, offeringID string, count int32) error { + approved := rec.CommitmentCost + if offeringprice.NoUpfrontApproved(approved) { + return nil + } + if cur := rec.CommitmentCurrency; cur != "" && !strings.EqualFold(cur, "USD") { + return fmt.Errorf("OpenSearch recommendation upfront is in %s; the approved upfront check supports USD only", cur) + } + out, err := c.client.DescribeReservedInstanceOfferings(ctx, &opensearch.DescribeReservedInstanceOfferingsInput{ + ReservedInstanceOfferingId: aws.String(offeringID), + }) + if err != nil { + return fmt.Errorf("failed to read OpenSearch offering %s for the approved upfront check: %w", offeringID, err) + } + if len(out.ReservedInstanceOfferings) != 1 { + return fmt.Errorf("OpenSearch offering %s: expected 1 offering for the approved upfront check, got %d", offeringID, len(out.ReservedInstanceOfferings)) + } + offering := out.ReservedInstanceOfferings[0] + if offering.FixedPrice == nil { + return fmt.Errorf("OpenSearch 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("OpenSearch offering %s is priced in %s; the approved upfront check supports USD only", offeringID, cc) + } + if err := offeringprice.CheckUpfront("OpenSearch", *offering.FixedPrice*float64(count), approved); err != nil { + return fmt.Errorf("OpenSearch offering %s: %w", offeringID, err) + } + return nil +} + // maxReservationPages bounds the idempotency reservation scan. // An incomplete scan refuses purchase rather than reporting no match. const maxReservationPages = 10 diff --git a/providers/aws/services/opensearch/purchase_price_guard_test.go b/providers/aws/services/opensearch/purchase_price_guard_test.go new file mode 100644 index 00000000..9bd198e0 --- /dev/null +++ b/providers/aws/services/opensearch/purchase_price_guard_test.go @@ -0,0 +1,184 @@ +package opensearch + +import ( + "context" + "errors" + "testing" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/opensearch" + "github.com/aws/aws-sdk-go-v2/service/opensearch/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +func guardOffering(fixed *float64, currency string) types.ReservedInstanceOffering { + o := types.ReservedInstanceOffering{ + ReservedInstanceOfferingId: aws.String("offering-1"), + InstanceType: types.OpenSearchPartitionInstanceTypeM5XlargeSearch, + Duration: 94608000, + PaymentOption: types.ReservedInstancePaymentOptionAllUpfront, + FixedPrice: fixed, + } + if currency != "" { + o.CurrencyCode = aws.String(currency) + } + return o +} + +// byIDMock serves the page lookup, then answers the by-ID price read with the +// given offerings or error, and accepts the purchase. +func byIDMock(byID []types.ReservedInstanceOffering, byIDErr error) *MockOpenSearchClient { + m := &MockOpenSearchClient{} + isByID := func(in *opensearch.DescribeReservedInstanceOfferingsInput) bool { + return aws.ToString(in.ReservedInstanceOfferingId) == "offering-1" + } + m.On("DescribeReservedInstanceOfferings", mock.Anything, mock.MatchedBy(isByID)). + Return(&opensearch.DescribeReservedInstanceOfferingsOutput{ReservedInstanceOfferings: byID}, byIDErr) + m.On("DescribeReservedInstanceOfferings", mock.Anything, mock.MatchedBy(func(in *opensearch.DescribeReservedInstanceOfferingsInput) bool { return !isByID(in) })). + Return(&opensearch.DescribeReservedInstanceOfferingsOutput{ReservedInstanceOfferings: []types.ReservedInstanceOffering{guardOffering(nil, "")}}, nil) + m.On("PurchaseReservedInstanceOffering", mock.Anything, mock.Anything). + Return(&opensearch.PurchaseReservedInstanceOfferingOutput{ReservedInstanceId: aws.String("ri-1")}, nil) + return m +} + +func guardMock(fixed float64, currency string) *MockOpenSearchClient { + return byIDMock([]types.ReservedInstanceOffering{guardOffering(aws.Float64(fixed), currency)}, nil) +} + +// guardRec is a 2-instance all-upfront rec approved at a $1000 total upfront. +func guardRec() common.Recommendation { + r := osIdemRec() + r.Count = 2 + r.CommitmentCost = 1000 + return r +} + +func buy(m *MockOpenSearchClient, 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, "PurchaseReservedInstanceOffering", 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, "PurchaseReservedInstanceOffering", mock.Anything, mock.Anything) +} + +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) + m.AssertNumberOfCalls(t, "DescribeReservedInstanceOfferings", 1) +} + +func TestPurchaseCommitment_RefusesNonUSD(t *testing.T) { + t.Parallel() + m := guardMock(400, "EUR") + _, err := buy(m, guardRec()) + require.ErrorContains(t, err, "EUR") + m.AssertNotCalled(t, "PurchaseReservedInstanceOffering", 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, "PurchaseReservedInstanceOffering", mock.Anything, mock.Anything) +} + +func TestPurchaseCommitment_EmptyAndLowercaseUSDBuy(t *testing.T) { + t.Parallel() + _, err := buy(guardMock(500, ""), guardRec()) + require.NoError(t, err) + _, err = buy(guardMock(500, "usd"), guardRec()) + require.NoError(t, err) + rec := guardRec() + rec.CommitmentCurrency = "usd" + _, err = buy(guardMock(500, "USD"), rec) + 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 := &MockOpenSearchClient{} + noUpfront := guardOffering(nil, "") + noUpfront.PaymentOption = types.ReservedInstancePaymentOptionNoUpfront + m.On("DescribeReservedInstanceOfferings", mock.Anything, mock.Anything). + Return(&opensearch.DescribeReservedInstanceOfferingsOutput{ReservedInstanceOfferings: []types.ReservedInstanceOffering{noUpfront}}, nil) + m.On("PurchaseReservedInstanceOffering", mock.Anything, mock.Anything). + Return(&opensearch.PurchaseReservedInstanceOfferingOutput{ReservedInstanceId: aws.String("ri-1")}, nil) + rec := osIdemRec() + rec.PaymentOption = "no-upfront" + _, err := buy(m, rec) + require.NoError(t, err) + m.AssertNumberOfCalls(t, "DescribeReservedInstanceOfferings", 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) + derivedName := common.IdempotentReservationID("opensearch-id-", token) + m.On("DescribeReservedInstances", mock.Anything, mock.Anything). + Return(&opensearch.DescribeReservedInstancesOutput{ReservedInstances: []types.ReservedInstance{ + {ReservedInstanceId: aws.String("os-existing"), ReservationName: aws.String(derivedName), 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, "os-existing", res.CommitmentID) + m.AssertNotCalled(t, "PurchaseReservedInstanceOffering", mock.Anything, mock.Anything) +} + +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, "PurchaseReservedInstanceOffering", mock.Anything, mock.Anything) +} + +func TestPurchaseCommitment_RefusesWhenPriceReadIsNotOneOffering(t *testing.T) { + t.Parallel() + two := []types.ReservedInstanceOffering{guardOffering(aws.Float64(400), "USD"), guardOffering(aws.Float64(400), "USD")} + for _, n := range [][]types.ReservedInstanceOffering{two, nil} { + m := byIDMock(n, nil) + _, err := buy(m, guardRec()) + require.ErrorContains(t, err, "expected 1 offering") + m.AssertNotCalled(t, "PurchaseReservedInstanceOffering", mock.Anything, mock.Anything) + } +} + +func TestPurchaseCommitment_RefusesOfferingWithoutFixedPrice(t *testing.T) { + t.Parallel() + m := byIDMock([]types.ReservedInstanceOffering{guardOffering(nil, "")}, nil) + _, err := buy(m, guardRec()) + require.ErrorContains(t, err, "no FixedPrice") + m.AssertNotCalled(t, "PurchaseReservedInstanceOffering", mock.Anything, mock.Anything) +} diff --git a/providers/aws/services/rds/client.go b/providers/aws/services/rds/client.go index f75be7c7..3ee62b48 100644 --- a/providers/aws/services/rds/client.go +++ b/providers/aws/services/rds/client.go @@ -221,7 +221,7 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati // (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 { + if offeringprice.NoUpfrontApproved(approved) { return nil } if cur := rec.CommitmentCurrency; cur != "" && !strings.EqualFold(cur, "USD") { diff --git a/providers/aws/services/redshift/client.go b/providers/aws/services/redshift/client.go index 0f6461c4..e2701316 100644 --- a/providers/aws/services/redshift/client.go +++ b/providers/aws/services/redshift/client.go @@ -6,6 +6,7 @@ import ( "fmt" "log" "sort" + "strings" "time" "github.com/aws/aws-sdk-go-v2/aws" @@ -145,6 +146,13 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati } } + // 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 + } + 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 @@ -164,11 +172,7 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati if response.ReservedNode != nil { result.Success = true result.CommitmentID = aws.ToString(response.ReservedNode.ReservedNodeId) - // FixedPrice is per node; the reservation total is FixedPrice x count. - if rn := response.ReservedNode; rn.FixedPrice != nil && rn.NodeCount != nil { - total := *rn.FixedPrice * float64(*rn.NodeCount) - result.Cost = &total - } + result.Cost = reservedNodeTotal(response.ReservedNode) } else { // A 200 without a node: the buy most likely happened. result.Error = fmt.Errorf("purchase response was empty: %w", common.ErrOutcomeUnknown) @@ -181,6 +185,54 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati return result, nil } +// reservedNodeTotal is the reservation's upfront total: FixedPrice is per node, +// so it is FixedPrice x NodeCount, or nil when either is missing. +func reservedNodeTotal(rn *redshifttypes.ReservedNode) *float64 { + if rn.FixedPrice == nil || rn.NodeCount == nil { + return nil + } + total := *rn.FixedPrice * float64(*rn.NodeCount) + return &total +} + +// checkApprovedUpfront refuses an offering whose total upfront (FixedPrice, per +// node, x count) exceeds the approved recommendation's upfront by more than +// offeringprice.UpfrontTolerance, or whose price cannot be compared with it. +// Mirrors rds.checkApprovedUpfront (same semantics; keep them in step). The +// shared comparison is offeringprice.CheckUpfront. Redshift 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, so there is nothing to compare. +func (c *Client) checkApprovedUpfront(ctx context.Context, rec common.Recommendation, offeringID string) error { + approved := rec.CommitmentCost + if offeringprice.NoUpfrontApproved(approved) { + return nil + } + if cur := rec.CommitmentCurrency; cur != "" && !strings.EqualFold(cur, "USD") { + return fmt.Errorf("redshift recommendation upfront is in %s; the approved upfront check supports USD only", cur) + } + out, err := c.client.DescribeReservedNodeOfferings(ctx, &redshift.DescribeReservedNodeOfferingsInput{ + ReservedNodeOfferingId: aws.String(offeringID), + }) + if err != nil { + return fmt.Errorf("failed to read Redshift offering %s for the approved upfront check: %w", offeringID, err) + } + if len(out.ReservedNodeOfferings) != 1 { + return fmt.Errorf("redshift offering %s: expected 1 offering for the approved upfront check, got %d", offeringID, len(out.ReservedNodeOfferings)) + } + offering := out.ReservedNodeOfferings[0] + if offering.FixedPrice == nil { + return fmt.Errorf("redshift 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("redshift offering %s is priced in %s; the approved upfront check supports USD only", offeringID, cc) + } + if err := offeringprice.CheckUpfront("Redshift", *offering.FixedPrice*float64(rec.Count), approved); err != nil { + return fmt.Errorf("redshift offering %s: %w", offeringID, err) + } + return nil +} + // maxOfferingPages is the maximum number of DescribeReservedNodeOfferings // pages to walk before giving up. At MaxRecords=100 per page this caps the // search at 500 offerings. Exceeding the cap returns a diagnostic error instead diff --git a/providers/aws/services/redshift/purchase_price_guard_test.go b/providers/aws/services/redshift/purchase_price_guard_test.go new file mode 100644 index 00000000..ddca97f7 --- /dev/null +++ b/providers/aws/services/redshift/purchase_price_guard_test.go @@ -0,0 +1,186 @@ +package redshift + +import ( + "context" + "errors" + "testing" + "time" + + "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" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +func guardOffering(fixed *float64, currency string) types.ReservedNodeOffering { + o := types.ReservedNodeOffering{ + ReservedNodeOfferingId: aws.String("offering-1"), + NodeType: aws.String("ra3.xlplus"), + Duration: aws.Int32(31536000), + ReservedNodeOfferingType: types.ReservedNodeOfferingType("Regular"), + FixedPrice: fixed, + } + if currency != "" { + o.CurrencyCode = aws.String(currency) + } + return o +} + +// byIDMock serves the page lookup (an all-upfront priced offering-1), then +// answers the by-ID price read with the given offerings or error, and accepts +// the purchase. +func byIDMock(byID []types.ReservedNodeOffering, byIDErr error) *MockRedshiftClient { + m := &MockRedshiftClient{} + isByID := func(in *redshift.DescribeReservedNodeOfferingsInput) bool { + return aws.ToString(in.ReservedNodeOfferingId) == "offering-1" + } + m.On("DescribeReservedNodeOfferings", mock.Anything, mock.MatchedBy(isByID)). + Return(&redshift.DescribeReservedNodeOfferingsOutput{ReservedNodeOfferings: byID}, byIDErr) + m.On("DescribeReservedNodeOfferings", mock.Anything, mock.MatchedBy(func(in *redshift.DescribeReservedNodeOfferingsInput) bool { return !isByID(in) })). + Return(&redshift.DescribeReservedNodeOfferingsOutput{ReservedNodeOfferings: []types.ReservedNodeOffering{guardOffering(aws.Float64(1000), "")}}, nil) + expectBuy(m) + return m +} + +func guardMock(fixed float64, currency string) *MockRedshiftClient { + return byIDMock([]types.ReservedNodeOffering{guardOffering(aws.Float64(fixed), currency)}, nil) +} + +// guardRec is a 2-node all-upfront rec approved at a $1000 total upfront. +func guardRec() common.Recommendation { + r := rsIdemRec() + r.Count = 2 + r.CommitmentCost = 1000 + return r +} + +func buy(m *MockRedshiftClient, rec common.Recommendation) (common.PurchaseResult, error) { + return (&Client{client: m, region: "us-east-1"}).PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{}) +} + +func notBought(t *testing.T, m *MockRedshiftClient) { + t.Helper() + m.AssertNotCalled(t, "PurchaseReservedNodeOffering", mock.Anything, mock.Anything) +} + +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) + notBought(t, m) +} + +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, "PurchaseReservedNodeOffering", mock.Anything, mock.Anything) +} + +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) + m.AssertNumberOfCalls(t, "DescribeReservedNodeOfferings", 1) +} + +func TestPurchaseCommitment_RefusesNonUSD(t *testing.T) { + t.Parallel() + m := guardMock(400, "EUR") + _, err := buy(m, guardRec()) + require.ErrorContains(t, err, "EUR") + notBought(t, m) + + m = guardMock(400, "USD") + rec := guardRec() + rec.CommitmentCurrency = "eur" + _, err = buy(m, rec) + require.ErrorContains(t, err, "eur") + notBought(t, m) +} + +func TestPurchaseCommitment_EmptyAndLowercaseUSDBuy(t *testing.T) { + t.Parallel() + _, err := buy(guardMock(500, ""), guardRec()) + require.NoError(t, err) + _, err = buy(guardMock(500, "usd"), guardRec()) + require.NoError(t, err) + rec := guardRec() + rec.CommitmentCurrency = "usd" + _, err = buy(guardMock(500, "USD"), rec) + 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 := &MockRedshiftClient{} + noUpfront := guardOffering(nil, "") + noUpfront.RecurringCharges = []types.RecurringCharge{{RecurringChargeAmount: aws.Float64(0.5), RecurringChargeFrequency: aws.String("Hourly")}} + m.On("DescribeReservedNodeOfferings", mock.Anything, mock.Anything). + Return(&redshift.DescribeReservedNodeOfferingsOutput{ReservedNodeOfferings: []types.ReservedNodeOffering{noUpfront}}, nil) + expectBuy(m) + rec := rsIdemRec() + rec.PaymentOption = "no-upfront" + _, err := buy(m, rec) + require.NoError(t, err) + m.AssertNumberOfCalls(t, "DescribeReservedNodeOfferings", 1) +} + +// A re-drive whose node already exists must return it even after the offering +// was repriced above the approved upfront: nothing is bought, so no refusal. +func TestPurchaseCommitment_ExistingNodeWinsOverRepricedOffering(t *testing.T) { + t.Parallel() + m := guardMock(600, "USD") // 2 x 600 = 1200 against an approved 1000 + expectNodePages(m, []types.ReservedNode{ + dnode("rn-existing", "active", dedupeReqAt.Add(time.Minute), func(n *types.ReservedNode) { n.NodeCount = aws.Int32(2) }), + }) + res, err := (&Client{client: m, region: "us-east-1"}).PurchaseCommitment(context.Background(), guardRec(), dedupeOpts()) + require.NoError(t, err) + assert.True(t, res.Success) + assert.True(t, res.ExistingCommitment) + assert.Equal(t, "rn-existing", res.CommitmentID) + notBought(t, 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) + notBought(t, m) +} + +func TestPurchaseCommitment_RefusesWhenPriceReadIsNotOneOffering(t *testing.T) { + t.Parallel() + two := []types.ReservedNodeOffering{guardOffering(aws.Float64(400), "USD"), guardOffering(aws.Float64(400), "USD")} + for _, n := range [][]types.ReservedNodeOffering{two, nil} { + m := byIDMock(n, nil) + _, err := buy(m, guardRec()) + require.ErrorContains(t, err, "expected 1 offering") + notBought(t, m) + } +} + +func TestPurchaseCommitment_RefusesOfferingWithoutFixedPrice(t *testing.T) { + t.Parallel() + m := byIDMock([]types.ReservedNodeOffering{guardOffering(nil, "")}, nil) + _, err := buy(m, guardRec()) + require.ErrorContains(t, err, "no FixedPrice") + notBought(t, m) +}