From d7dfc8428f1ccaa76e6b6c06a6dba169d72b63f9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 14 Oct 2026 20:18:32 +0200 Subject: [PATCH] fix(aws): floor Savings Plan commitments to API precision --- .../aws/internal/spcommitment/commitment.go | 48 ++++++++ providers/aws/ladder/precision_test.go | 67 +++++++++++ providers/aws/ladder/purchase.go | 6 +- providers/aws/services/savingsplans/client.go | 15 ++- .../services/savingsplans/precision_test.go | 110 ++++++++++++++++++ 5 files changed, 238 insertions(+), 8 deletions(-) create mode 100644 providers/aws/internal/spcommitment/commitment.go create mode 100644 providers/aws/ladder/precision_test.go create mode 100644 providers/aws/services/savingsplans/precision_test.go diff --git a/providers/aws/internal/spcommitment/commitment.go b/providers/aws/internal/spcommitment/commitment.go new file mode 100644 index 00000000..e54351ed --- /dev/null +++ b/providers/aws/internal/spcommitment/commitment.go @@ -0,0 +1,48 @@ +package spcommitment + +import ( + "fmt" + "math" + "strconv" + "strings" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +const ( + minimumHourlyCommitment = 0.001 + maximumHourlyCommitment = 1000000.0 + maximumDecimalPlaces = 5 +) + +// Normalize copies details so provider normalization cannot change caller authorization data. +func Normalize(rec *common.Recommendation) (string, error) { + details, ok := rec.Details.(*common.SavingsPlanDetails) + if !ok || details == nil { + return "", fmt.Errorf("invalid service details for Savings Plans: recommendation Details must be *common.SavingsPlanDetails, got %T", rec.Details) + } + amount := details.HourlyCommitment + if math.IsNaN(amount) || math.IsInf(amount, 0) || amount <= 0 { + return "", fmt.Errorf("SavingsPlanDetails.HourlyCommitment must be a positive finite value, got %g", amount) + } + if amount < minimumHourlyCommitment || amount > maximumHourlyCommitment { + return "", fmt.Errorf("SavingsPlanDetails.HourlyCommitment must be between %g and %g, got %g", minimumHourlyCommitment, maximumHourlyCommitment, amount) + } + // Truncate decimal intent, rather than multiplying a binary float that can cross a boundary. + wire := strconv.FormatFloat(amount, 'f', -1, 64) + whole, fraction, _ := strings.Cut(wire, ".") + if len(fraction) > maximumDecimalPlaces { + fraction = fraction[:maximumDecimalPlaces] + } + fraction = strings.TrimRight(fraction, "0") + fraction += strings.Repeat("0", max(0, 2-len(fraction))) + wire = whole + "." + fraction + normalized, err := strconv.ParseFloat(wire, 64) + if err != nil { + return "", fmt.Errorf("parse normalized Savings Plans commitment: %w", err) + } + copyDetails := *details + copyDetails.HourlyCommitment = normalized + rec.Details = ©Details + return wire, nil +} diff --git a/providers/aws/ladder/precision_test.go b/providers/aws/ladder/precision_test.go new file mode 100644 index 00000000..52d02e4f --- /dev/null +++ b/providers/aws/ladder/precision_test.go @@ -0,0 +1,67 @@ +package ladder + +import ( + "context" + "math" + "strconv" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/ladder" +) + +func TestPurchaseLayer_CommitmentPrecision(t *testing.T) { + t.Parallel() + for _, layer := range []ladder.LayerType{ladder.LayerComputeSP, ladder.LayerEC2InstanceSP} { + for _, tc := range []struct{ amount, want float64 }{{.001, .001}, {.00499, .00499}, {.005, .005}, {1.234567, 1.23456}, {.29, .29}, {1000000, 1000000}, {math.Nextafter(1000000, 0), 999999.99999}} { + t.Run(string(layer)+strconv.FormatFloat(tc.amount, 'g', -1, 64), func(t *testing.T) { + planType := spPlanTypeCompute + if layer == ladder.LayerEC2InstanceSP { + planType = spPlanTypeEC2Instance + } + p := &fakePurchaser{} + a := newWiredLadder(t, &fakePurchaser{}, p, &fakeExchangeRunner{}) + rec := validSPRec(planType) + original := rec.Details.(*common.SavingsPlanDetails) + original.HourlyCommitment = tc.amount + _, err := a.PurchaseLayer(context.Background(), layer, rec, validPurchaseOpts()) + require.NoError(t, err) + require.Equal(t, 1, p.calls) + details := p.gotRec.Details.(*common.SavingsPlanDetails) + assert.Equal(t, tc.want, details.HourlyCommitment) + assert.NotSame(t, original, details) + assert.Equal(t, tc.amount, original.HourlyCommitment) + assert.Equal(t, "ladder-tok-1", p.gotOpts.IdempotencyToken) + }) + } + } +} + +func TestPurchaseLayer_CommitmentPrecisionRefusesInvalid(t *testing.T) { + t.Parallel() + for _, amount := range []float64{.00099, math.Nextafter(.001, 0), math.Nextafter(1000000, math.Inf(1)), 1000001, math.Inf(-1)} { + t.Run(strconv.FormatFloat(amount, 'g', -1, 64), func(t *testing.T) { + p := &fakePurchaser{} + a := newWiredLadder(t, &fakePurchaser{}, p, &fakeExchangeRunner{}) + rec := validSPRec(spPlanTypeCompute) + rec.Details.(*common.SavingsPlanDetails).HourlyCommitment = amount + _, err := a.PurchaseLayer(context.Background(), ladder.LayerComputeSP, rec, validPurchaseOpts()) + require.Error(t, err) + assert.Zero(t, p.calls) + }) + } + for _, details := range []common.ServiceDetails{nil, (*common.SavingsPlanDetails)(nil)} { + p := &fakePurchaser{} + a := newWiredLadder(t, &fakePurchaser{}, p, &fakeExchangeRunner{}) + rec := validSPRec(spPlanTypeCompute) + rec.Details = details + require.NotPanics(t, func() { + _, err := a.PurchaseLayer(context.Background(), ladder.LayerComputeSP, rec, validPurchaseOpts()) + require.Error(t, err) + }) + assert.Zero(t, p.calls) + } +} diff --git a/providers/aws/ladder/purchase.go b/providers/aws/ladder/purchase.go index 60a9adb2..a89db7b0 100644 --- a/providers/aws/ladder/purchase.go +++ b/providers/aws/ladder/purchase.go @@ -3,10 +3,10 @@ package ladder import ( "context" "fmt" - "math" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/pkg/ladder" + "github.com/LeanerCloud/cloud-commitments-go/providers/aws/internal/spcommitment" ) // PurchaseLayer buys a commitment for the given layer by dispatching to the @@ -136,8 +136,8 @@ func validateSPPurchaseRec(rec *common.Recommendation, wantPlanType string) erro if details.PlanType != wantPlanType { return fmt.Errorf("recommendation plan type %q does not match the dispatched layer's plan type %q", details.PlanType, wantPlanType) } - if math.IsNaN(details.HourlyCommitment) || math.IsInf(details.HourlyCommitment, 0) || details.HourlyCommitment <= 0 { - return fmt.Errorf("SavingsPlanDetails.HourlyCommitment must be a positive finite value, got %g", details.HourlyCommitment) + if _, err := spcommitment.Normalize(rec); err != nil { + return err } return validateTermAndPayment(rec) } diff --git a/providers/aws/services/savingsplans/client.go b/providers/aws/services/savingsplans/client.go index 92521999..eb9beef6 100644 --- a/providers/aws/services/savingsplans/client.go +++ b/providers/aws/services/savingsplans/client.go @@ -16,6 +16,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/providers/aws/internal/purchasecfg" + "github.com/LeanerCloud/cloud-commitments-go/providers/aws/internal/spcommitment" ) // API defines the interface for Savings Plans operations (enables mocking). @@ -218,11 +219,12 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati Timestamp: time.Now(), } - spDetails, ok := rec.Details.(*common.SavingsPlanDetails) - if !ok { - result.Error = fmt.Errorf("invalid service details for Savings Plans") + commitment, err := spcommitment.Normalize(&rec) + if err != nil { + result.Error = err return result, result.Error } + result.Recommendation = rec offeringID, err := c.findOfferingID(ctx, rec, opts.ExecutionID) if err != nil { @@ -232,7 +234,7 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati input := &savingsplans.CreateSavingsPlanInput{ SavingsPlanOfferingId: aws.String(offeringID), - Commitment: aws.String(fmt.Sprintf("%.2f", spDetails.HourlyCommitment)), + Commitment: aws.String(commitment), UpfrontPaymentAmount: nil, // AWS calculates this based on payment option PurchaseTime: aws.Time(time.Now()), Tags: buildSavingsPlanTags(opts.Source), @@ -452,7 +454,7 @@ func (c *Client) collectSPOfferings(ctx context.Context, input *savingsplans.Des // calling outside of a purchase flow (ValidateOffering, GetOfferingDetails). func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation, execID string) (string, error) { spDetails, ok := rec.Details.(*common.SavingsPlanDetails) - if !ok { + if !ok || spDetails == nil { return "", fmt.Errorf("invalid service details for Savings Plans") } @@ -647,6 +649,9 @@ func (c *Client) ValidateOffering(ctx context.Context, rec common.Recommendation // GetOfferingDetails retrieves offering details. func (c *Client) GetOfferingDetails(ctx context.Context, rec common.Recommendation) (*common.OfferingDetails, error) { + if _, err := spcommitment.Normalize(&rec); err != nil { + return nil, err + } offeringID, err := c.findOfferingID(ctx, rec, "") if err != nil { return nil, err diff --git a/providers/aws/services/savingsplans/precision_test.go b/providers/aws/services/savingsplans/precision_test.go new file mode 100644 index 00000000..db2b57da --- /dev/null +++ b/providers/aws/services/savingsplans/precision_test.go @@ -0,0 +1,110 @@ +package savingsplans + +import ( + "context" + "math" + "strconv" + "testing" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/savingsplans" + "github.com/aws/aws-sdk-go-v2/service/savingsplans/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 TestCommitmentPrecision_PurchaseAndPreview(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + amount float64 + wire string + }{ + {.001, "0.001"}, {.00499, "0.00499"}, {.005, "0.005"}, + {1.234567, "1.23456"}, {.29, "0.29"}, {10, "10.00"}, + {1000000, "1000000.00"}, {math.Nextafter(.001, math.Inf(1)), "0.001"}, + {math.Nextafter(1000000, 0), "999999.99999"}, + {math.Nextafter(.29, 0), "0.28999"}, {math.Nextafter(.29, math.Inf(1)), "0.29"}, + } { + t.Run(strconv.FormatFloat(tc.amount, 'g', -1, 64), func(t *testing.T) { + t.Parallel() + m := &MockSavingsPlansClient{} + c := NewClient(aws.Config{Region: "us-east-1"}, types.SavingsPlanTypeCompute) + c.SetSavingsPlansAPI(m) + rec := spRec() + original := rec.Details.(*common.SavingsPlanDetails) + original.HourlyCommitment = tc.amount + original.OfferingID = "offering" + want, err := strconv.ParseFloat(tc.wire, 64) + require.NoError(t, err) + m.On("DescribeSavingsPlansOfferingRates", mock.Anything, mock.Anything). + Return(&savingsplans.DescribeSavingsPlansOfferingRatesOutput{}, nil).Once() + preview, err := c.GetOfferingDetails(context.Background(), rec) + require.NoError(t, err) + assert.Equal(t, want, preview.EffectiveHourlyRate) + assert.Equal(t, want*8760, preview.TotalCost) + assert.Equal(t, (want*8760)/8760, preview.RecurringCost) + m.On("CreateSavingsPlan", mock.Anything, mock.Anything).Run(func(args mock.Arguments) { + input := args.Get(1).(*savingsplans.CreateSavingsPlanInput) + assert.Equal(t, tc.wire, aws.ToString(input.Commitment)) + assert.Equal(t, "stable-token", aws.ToString(input.ClientToken)) + }).Return(&savingsplans.CreateSavingsPlanOutput{SavingsPlanId: aws.String("purchased")}, nil).Twice() + m.On("DescribeSavingsPlans", mock.Anything, mock.Anything). + Return(&savingsplans.DescribeSavingsPlansOutput{SavingsPlans: []types.SavingsPlan{{SavingsPlanId: aws.String("purchased"), UpfrontPaymentAmount: aws.String("123.45")}}}, nil).Twice() + for i := 0; i < 2; i++ { + result, err := c.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{IdempotencyToken: "stable-token"}) + require.NoError(t, err) + require.True(t, result.Success) + normalized := result.Recommendation.Details.(*common.SavingsPlanDetails) + assert.Equal(t, want, normalized.HourlyCommitment) + assert.NotSame(t, original, normalized) + require.NotNil(t, result.Cost) + assert.Equal(t, 123.45, *result.Cost) + rec = result.Recommendation + } + assert.Equal(t, tc.amount, original.HourlyCommitment) + m.AssertExpectations(t) + }) + } +} + +func TestCommitmentPrecision_InvalidInputMakesNoSDKCalls(t *testing.T) { + t.Parallel() + amounts := []float64{0, -1, .00099, math.Nextafter(.001, 0), math.Nextafter(1000000, math.Inf(1)), 1000001, math.NaN(), math.Inf(1), math.Inf(-1)} + inputs := make([]common.ServiceDetails, 0, len(amounts)+2) + inputs = append(inputs, nil, (*common.SavingsPlanDetails)(nil)) + for _, amount := range amounts { + inputs = append(inputs, &common.SavingsPlanDetails{PlanType: "Compute", HourlyCommitment: amount}) + } + for i, details := range inputs { + t.Run(strconv.Itoa(i), func(t *testing.T) { + m := &MockSavingsPlansClient{} + c := NewClient(aws.Config{}, types.SavingsPlanTypeCompute) + c.SetSavingsPlansAPI(m) + rec := spRec() + rec.Details = details + require.NotPanics(t, func() { + result, err := c.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{}) + require.Error(t, err) + assert.False(t, result.Success) + _, err = c.GetOfferingDetails(context.Background(), rec) + require.Error(t, err) + }) + assert.Empty(t, m.Calls) + }) + } +} + +func TestCommitmentPrecision_ValidateOfferingDoesNotRequireCommitment(t *testing.T) { + c := NewClient(aws.Config{}, types.SavingsPlanTypeCompute) + m := &MockSavingsPlansClient{} + c.SetSavingsPlansAPI(m) + rec := spRec() + rec.Details = (*common.SavingsPlanDetails)(nil) + require.NotPanics(t, func() { require.Error(t, c.ValidateOffering(context.Background(), rec)) }) + rec.Details = &common.SavingsPlanDetails{PlanType: "Compute", OfferingID: "offering"} + require.NoError(t, c.ValidateOffering(context.Background(), rec)) + assert.Empty(t, m.Calls) +}