diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fb5fe87..d26317bf 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% (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/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..5933556b 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -167,6 +167,11 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati return result, result.Error } + if err = checkApprovedUpfront(rec, offering); err != nil { + result.Error = err + return result, result.Error + } + // Create the purchase request input := &ec2.PurchaseReservedInstancesOfferingInput{ ReservedInstancesOfferingId: offering.ReservedInstancesOfferingId, @@ -217,6 +222,35 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati return result, nil } +// 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 + } + 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 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 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 fmt.Errorf("EC2 offering %s: %w", id, err) + } + return 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..19ca6be5 --- /dev/null +++ b/providers/aws/services/ec2/purchase_price_guard_test.go @@ -0,0 +1,86 @@ +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_BuysAtApprovedUpfront(t *testing.T) { + t.Parallel() + 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) + assert.True(t, res.Success) + m.AssertCalled(t, "PurchaseReservedInstancesOffering", mock.Anything, mock.MatchedBy(func(in *ec2.PurchaseReservedInstancesOfferingInput) bool { + return in.LimitPrice == nil + }), mock.Anything) +} + +// The MCP EC2 tool never sets CommitmentCost; zero must not block the buy. +func TestPurchaseCommitment_NoApprovedUpfrontIsNotChecked(t *testing.T) { + t.Parallel() + 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) +} + +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", "") + require.ErrorContains(t, checkApprovedUpfront(pricedRec(), &noPrice), "no FixedPrice") + + eur := pricedOffering(500) + eur.CurrencyCode = types.CurrencyCodeValues("EUR") + require.ErrorContains(t, checkApprovedUpfront(pricedRec(), &eur), "USD only") +}