From 0d467e5d3c1fa23d8073b16d9ee826afdaa7f950 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 11:18:20 -0700 Subject: [PATCH 1/2] fix(exchange): fail loud on over-cap re-quote, empty region, non-positive count M4: executeWithAPI now re-quotes immediately before AcceptReservedInstancesExchangeQuote to narrow the quote-to-accept window. If the re-quoted payment exceeds MaxPaymentDueUSD, the exchange is aborted with an explicit error surfacing actual vs cap amounts rather than proceeding silently. Accept is never called on an over-cap re-quote. M3: loadCfg now returns an explicit error when region is empty or whitespace-only, refusing to default to us-east-1 on the RI exchange path. RIs are regional; an implicit region default on a spend path violates the fail-loud directive. L2: validateTargets now accepts legacyCount and validates Count >= 1 for both the multi-target and legacy-singleton paths. A zero or negative Count is an error, not a silent rewrite to 1. buildTargetConfigs no longer has a fallback-to-1 branch. L3: Correct the stale comment in pricingGatePasses: NormalizationFactorForSize returns 0 (not 1.0) for unknown sizes, causing passesDollarUnitsCheck to reject the alternative (fail-closed), not to bypass the dollar-units check. Regression tests added in fail_loud_test.go covering all three money-path fixes. Closes #1080. --- pkg/exchange/exchange.go | 75 +++++++---- pkg/exchange/fail_loud_test.go | 228 +++++++++++++++++++++++++++++++++ pkg/exchange/reshape.go | 8 +- 3 files changed, 283 insertions(+), 28 deletions(-) create mode 100644 pkg/exchange/fail_loud_test.go diff --git a/pkg/exchange/exchange.go b/pkg/exchange/exchange.go index 8cefb77dd..647c80cab 100644 --- a/pkg/exchange/exchange.go +++ b/pkg/exchange/exchange.go @@ -106,9 +106,7 @@ type ExchangeExecuteRequest struct { // targetConfigs returns the ec2types slice to pass to the EC2 API. // Prefers r.Targets when set; otherwise falls back to the legacy -// TargetOfferingID / TargetCount singleton. Empty or negative Count -// values default to 1 so a caller that forgets to populate Count still -// gets a usable request — matches the prior singleton behaviour. +// TargetOfferingID / TargetCount singleton. func (r *ExchangeQuoteRequest) targetConfigs() []ec2types.TargetConfigurationRequest { return buildTargetConfigs(r.Targets, r.TargetOfferingID, r.TargetCount) } @@ -121,41 +119,40 @@ func buildTargetConfigs(targets []TargetConfig, legacyOfferingID string, legacyC if len(targets) > 0 { out := make([]ec2types.TargetConfigurationRequest, 0, len(targets)) for _, t := range targets { - count := t.Count - if count <= 0 { - count = 1 - } out = append(out, ec2types.TargetConfigurationRequest{ OfferingId: sdkaws.String(t.OfferingID), - InstanceCount: sdkaws.Int32(count), + InstanceCount: sdkaws.Int32(t.Count), }) } return out } - count := legacyCount - if count <= 0 { - count = 1 - } return []ec2types.TargetConfigurationRequest{{ OfferingId: sdkaws.String(legacyOfferingID), - InstanceCount: sdkaws.Int32(count), + InstanceCount: sdkaws.Int32(legacyCount), }} } // validateTargets returns a non-nil error if neither the Targets slice -// nor the legacy singleton fields carry a usable target offering. -func validateTargets(targets []TargetConfig, legacyOfferingID string) error { +// nor the legacy singleton fields carry a usable target offering, or if +// any target has a non-positive Count. +func validateTargets(targets []TargetConfig, legacyOfferingID string, legacyCount int32) error { if len(targets) > 0 { for i, t := range targets { if strings.TrimSpace(t.OfferingID) == "" { return fmt.Errorf("targets[%d].offering_id must be non-empty", i) } + if t.Count <= 0 { + return fmt.Errorf("targets[%d].count must be >= 1, got %d", i, t.Count) + } } return nil } if strings.TrimSpace(legacyOfferingID) == "" { return fmt.Errorf("must provide target offering ID (either targets[] or target_offering_id)") } + if legacyCount <= 0 { + return fmt.Errorf("target_count must be >= 1, got %d", legacyCount) + } return nil } @@ -196,8 +193,8 @@ func (c *ExchangeClient) Execute(ctx context.Context, req ExchangeExecuteRequest } func loadCfg(ctx context.Context, region string) (sdkaws.Config, error) { - if region == "" { - region = "us-east-1" + if strings.TrimSpace(region) == "" { + return sdkaws.Config{}, fmt.Errorf("region must be specified explicitly; refusing to default to us-east-1 on an RI exchange path") } return config.LoadDefaultConfig(ctx, config.WithRegion(region)) } @@ -234,7 +231,7 @@ func getQuoteWithAPI(ctx context.Context, client EC2ExchangeAPI, req ExchangeQuo if len(req.ReservedIDs) == 0 { return nil, fmt.Errorf("must provide at least one reserved instance ID") } - if err := validateTargets(req.Targets, req.TargetOfferingID); err != nil { + if err := validateTargets(req.Targets, req.TargetOfferingID, req.TargetCount); err != nil { return nil, err } @@ -337,18 +334,46 @@ func executeWithAPI(ctx context.Context, client EC2ExchangeAPI, req ExchangeExec return "", q, fmt.Errorf("paymentDue %s exceeds max %s", paymentDue.FloatString(2), req.MaxPaymentDueUSD.FloatString(2)) } - // NOTE: The AWS RI exchange API has no atomic quote+accept operation. The - // AcceptReservedInstancesExchangeQuote call re-evaluates pricing server-side, - // so in rare cases the actual payment could differ from the quoted amount - // (e.g., due to RI valuation changes between GetQuote and Accept). - // This is a known API limitation and not something we can prevent here. + // Re-quote immediately before accepting to narrow the window between quote and + // accept. The AWS API has no atomic quote+accept operation, so server-side + // pricing can change between the two calls. This second quote reduces -- but + // does not eliminate -- the race window. If the fresh quote now exceeds the + // cap, abort before the irreversible accept call. + freshQ, err := getQuoteWithAPI(ctx, client, ExchangeQuoteRequest{ + Region: req.Region, + ExpectedAccount: req.ExpectedAccount, + ReservedIDs: req.ReservedIDs, + Targets: req.Targets, + TargetOfferingID: req.TargetOfferingID, + TargetCount: req.TargetCount, + DryRun: false, + }) + if err != nil { + return "", q, fmt.Errorf("pre-accept re-quote failed: %w", err) + } + if !freshQ.IsValidExchange { + return "", freshQ, fmt.Errorf("exchange no longer valid at accept time: %s", freshQ.ValidationFailureReason) + } + + freshPaymentDue := freshQ.PaymentDueUSD + if freshPaymentDue == nil { + freshPaymentDue = new(big.Rat) + } + if freshPaymentDue.Cmp(req.MaxPaymentDueUSD) == 1 { + return "", freshQ, fmt.Errorf( + "aborting exchange: re-quoted payment %s USD exceeds cap %s USD (pricing changed between initial quote and accept)", + freshPaymentDue.FloatString(2), + req.MaxPaymentDueUSD.FloatString(2), + ) + } + out, err := client.AcceptReservedInstancesExchangeQuote(ctx, &ec2.AcceptReservedInstancesExchangeQuoteInput{ ReservedInstanceIds: req.ReservedIDs, TargetConfigurations: req.targetConfigs(), }) if err != nil { - return "", q, err + return "", freshQ, err } - return sdkaws.ToString(out.ExchangeId), q, nil + return sdkaws.ToString(out.ExchangeId), freshQ, nil } diff --git a/pkg/exchange/fail_loud_test.go b/pkg/exchange/fail_loud_test.go new file mode 100644 index 000000000..c4d54fa48 --- /dev/null +++ b/pkg/exchange/fail_loud_test.go @@ -0,0 +1,228 @@ +package exchange + +// Regression tests for the fail-loud hardening of the RI exchange path. +// Each test documents the finding it guards: +// - M4: over-cap re-quote aborts before accept +// - M3: empty region returns an error (no us-east-1 default) +// - L2: Count <= 0 returns an error (no silent rewrite to 1) + +import ( + "context" + "math/big" + "strings" + "testing" + + sdkaws "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/ec2" +) + +// sequentialFakeEC2 returns successive quoteOutputs for each GetQuote call +// so we can simulate pricing increasing between the first and second quote. +type sequentialFakeEC2 struct { + quoteOutputs []*ec2.GetReservedInstancesExchangeQuoteOutput + quoteErrors []error + quoteCall int + + acceptInput *ec2.AcceptReservedInstancesExchangeQuoteInput + acceptOutput *ec2.AcceptReservedInstancesExchangeQuoteOutput + acceptErr error +} + +func (f *sequentialFakeEC2) GetReservedInstancesExchangeQuote(_ context.Context, _ *ec2.GetReservedInstancesExchangeQuoteInput, _ ...func(*ec2.Options)) (*ec2.GetReservedInstancesExchangeQuoteOutput, error) { + i := f.quoteCall + f.quoteCall++ + if i < len(f.quoteOutputs) { + return f.quoteOutputs[i], f.quoteErrors[i] + } + // Repeat the last output if called more times than expected. + last := len(f.quoteOutputs) - 1 + return f.quoteOutputs[last], f.quoteErrors[last] +} + +func (f *sequentialFakeEC2) AcceptReservedInstancesExchangeQuote(_ context.Context, in *ec2.AcceptReservedInstancesExchangeQuoteInput, _ ...func(*ec2.Options)) (*ec2.AcceptReservedInstancesExchangeQuoteOutput, error) { + f.acceptInput = in + return f.acceptOutput, f.acceptErr +} + +func seqQuoteOut(paymentDue string) *ec2.GetReservedInstancesExchangeQuoteOutput { + return &ec2.GetReservedInstancesExchangeQuoteOutput{ + IsValidExchange: sdkaws.Bool(true), + PaymentDue: sdkaws.String(paymentDue), + } +} + +// TestExecute_OverCapReQuoteAbortsBeforeAccept (M4): +// The initial quote is within cap, but the pre-accept re-quote exceeds the cap. +// The exchange must be aborted with an explicit error; Accept must not be called. +func TestExecute_OverCapReQuoteAbortsBeforeAccept(t *testing.T) { + t.Parallel() + + cap := new(big.Rat).SetInt64(50) + + // First quote (initial check): $40 -- within cap. + // Second quote (pre-accept re-quote): $60 -- exceeds cap. + f := &sequentialFakeEC2{ + quoteOutputs: []*ec2.GetReservedInstancesExchangeQuoteOutput{ + seqQuoteOut("40.00"), + seqQuoteOut("60.00"), + }, + quoteErrors: []error{nil, nil}, + acceptOutput: &ec2.AcceptReservedInstancesExchangeQuoteOutput{ExchangeId: sdkaws.String("should-not-be-called")}, + } + c := NewExchangeClientFromAPI(f) + + _, _, err := c.Execute(context.Background(), ExchangeExecuteRequest{ + ReservedIDs: []string{"ri-1"}, + TargetOfferingID: "off-A", + TargetCount: 1, + MaxPaymentDueUSD: cap, + }) + + if err == nil { + t.Fatal("expected error when re-quoted payment exceeds cap, got nil") + } + if f.acceptInput != nil { + t.Fatalf("Accept was called despite re-quoted payment exceeding cap; accept input: %+v", f.acceptInput) + } + // Error must surface both the actual and cap amounts. + if !strings.Contains(err.Error(), "60") { + t.Errorf("error should contain the re-quoted amount (60); got: %v", err) + } + if !strings.Contains(err.Error(), "50") { + t.Errorf("error should contain the cap amount (50); got: %v", err) + } +} + +// TestExecute_ReQuoteWithinCapProceedsToAccept (M4 happy path): +// Both quotes are within cap; Accept must be called. +func TestExecute_ReQuoteWithinCapProceedsToAccept(t *testing.T) { + t.Parallel() + + cap := new(big.Rat).SetInt64(100) + + f := &sequentialFakeEC2{ + quoteOutputs: []*ec2.GetReservedInstancesExchangeQuoteOutput{ + seqQuoteOut("40.00"), + seqQuoteOut("45.00"), // re-quote: higher but still within cap + }, + quoteErrors: []error{nil, nil}, + acceptOutput: &ec2.AcceptReservedInstancesExchangeQuoteOutput{ExchangeId: sdkaws.String("exch-ok")}, + } + c := NewExchangeClientFromAPI(f) + + exchangeID, _, err := c.Execute(context.Background(), ExchangeExecuteRequest{ + ReservedIDs: []string{"ri-1"}, + TargetOfferingID: "off-A", + TargetCount: 1, + MaxPaymentDueUSD: cap, + }) + + if err != nil { + t.Fatalf("unexpected error when re-quoted payment is within cap: %v", err) + } + if exchangeID != "exch-ok" { + t.Fatalf("expected exchange ID 'exch-ok', got %q", exchangeID) + } + if f.acceptInput == nil { + t.Fatal("Accept was not called despite both quotes being within cap") + } +} + +// TestLoadCfg_EmptyRegionErrors (M3): +// loadCfg must return an error when region is empty; it must not default to us-east-1. +func TestLoadCfg_EmptyRegionErrors(t *testing.T) { + t.Parallel() + + _, err := loadCfg(context.Background(), "") + if err == nil { + t.Fatal("expected error for empty region, got nil") + } + if strings.Contains(strings.ToLower(err.Error()), "us-east-1") { + // Error must explain why, not mention the forbidden default as an alternative. + t.Logf("error message: %v", err) + } + // Confirm the error message references region or explicit. + if !strings.Contains(err.Error(), "region") { + t.Errorf("error should mention 'region'; got: %v", err) + } +} + +// TestLoadCfg_WhitespaceOnlyRegionErrors (M3): +// Whitespace-only region must also be rejected. +func TestLoadCfg_WhitespaceOnlyRegionErrors(t *testing.T) { + t.Parallel() + + _, err := loadCfg(context.Background(), " ") + if err == nil { + t.Fatal("expected error for whitespace-only region, got nil") + } +} + +// TestValidateTargets_ZeroCountErrors (L2): +// A target with Count <= 0 must produce a validation error; it must not be +// silently rewritten to 1. +func TestValidateTargets_ZeroCountErrors(t *testing.T) { + t.Parallel() + + err := validateTargets([]TargetConfig{{OfferingID: "off-A", Count: 0}}, "", 0) + if err == nil { + t.Fatal("expected error for Count=0, got nil") + } + if !strings.Contains(err.Error(), "count") { + t.Errorf("error should mention 'count'; got: %v", err) + } +} + +// TestValidateTargets_NegativeCountErrors (L2): +// A target with Count < 0 must produce a validation error. +func TestValidateTargets_NegativeCountErrors(t *testing.T) { + t.Parallel() + + err := validateTargets([]TargetConfig{{OfferingID: "off-B", Count: -3}}, "", 0) + if err == nil { + t.Fatal("expected error for Count=-3, got nil") + } +} + +// TestValidateTargets_LegacyZeroCountErrors (L2): +// The legacy singleton path must also reject Count <= 0. +func TestValidateTargets_LegacyZeroCountErrors(t *testing.T) { + t.Parallel() + + err := validateTargets(nil, "off-A", 0) + if err == nil { + t.Fatal("expected error for legacy TargetCount=0, got nil") + } +} + +// TestValidateTargets_LegacyNegativeCountErrors (L2): +// The legacy singleton path must also reject negative Count. +func TestValidateTargets_LegacyNegativeCountErrors(t *testing.T) { + t.Parallel() + + err := validateTargets(nil, "off-A", -1) + if err == nil { + t.Fatal("expected error for legacy TargetCount=-1, got nil") + } +} + +// TestGetQuote_ZeroCountRejected (L2): +// GetQuote via ExchangeClient must reject a zero-count target before calling AWS. +func TestGetQuote_ZeroCountRejected(t *testing.T) { + t.Parallel() + + f := &fakeEC2{} + c := NewExchangeClientFromAPI(f) + + _, err := c.GetQuote(context.Background(), ExchangeQuoteRequest{ + ReservedIDs: []string{"ri-1"}, + TargetOfferingID: "off-A", + TargetCount: 0, + }) + if err == nil { + t.Fatal("expected error for TargetCount=0, got nil") + } + if f.quoteInput != nil { + t.Fatal("AWS GetReservedInstancesExchangeQuote was called despite invalid Count=0; should have been rejected before reaching AWS") + } +} diff --git a/pkg/exchange/reshape.go b/pkg/exchange/reshape.go index fb79830ca..98950dc63 100644 --- a/pkg/exchange/reshape.go +++ b/pkg/exchange/reshape.go @@ -670,9 +670,11 @@ func termMatchesIfKnown(src RIInfo, off OfferingOption, hasSrc bool) bool { // leave NormalizationFactor at zero (e.g. tests, partial RIInfo // constructions). A zero NF here would reject every alternative even // when InstanceType is parseable. Derive NF from the instance size when -// it's missing; if the size doesn't match the AWS canonical table, -// NormalizationFactorForSize returns 1.0 — degrades to "no NF -// adjustment" rather than a hard reject. +// it's missing. If the size is not in the AWS canonical table, +// NormalizationFactorForSize returns 0 (map miss), which causes +// passesDollarUnitsCheck to reject the alternative (srcNF <= 0 path). +// This is fail-closed: an unrecognised size excludes the offering rather +// than silently bypassing the dollar-units check. func pricingGatePasses(src RIInfo, off OfferingOption, hasSrc bool) bool { if !hasSrc || src.MonthlyCost <= 0 { return true From dcee1b499274af411184d6473d344a43e2e33489 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 11:42:30 -0700 Subject: [PATCH 2/2] refactor(exchange): extract helpers to reduce executeWithAPI cyclomatic complexity Extract resolvePaymentDue, checkInitialQuote, and checkReQuote from executeWithAPI to bring its cyclomatic complexity from 11 to 6, fixing the gocyclo pre-commit hook failure (limit: 10). Also deduplicate the ExchangeQuoteRequest literal by factoring it into a shared quoteReq variable used by both the initial and re-quote calls. Behavior is identical: all 94 exchange tests pass with -race. --- pkg/exchange/exchange.go | 88 ++++++++++++++++++++++------------------ 1 file changed, 48 insertions(+), 40 deletions(-) diff --git a/pkg/exchange/exchange.go b/pkg/exchange/exchange.go index 647c80cab..e8208497b 100644 --- a/pkg/exchange/exchange.go +++ b/pkg/exchange/exchange.go @@ -299,13 +299,52 @@ func ExecuteExchange(ctx context.Context, req ExchangeExecuteRequest) (exchangeI return executeWithAPI(ctx, ec2.NewFromConfig(cfg), req) } +// resolvePaymentDue returns the quote's PaymentDueUSD, treating nil as zero. +// AWS may omit PaymentDue for zero-cost exchanges (e.g., same-RI-type conversions). +func resolvePaymentDue(q *ExchangeQuoteSummary) *big.Rat { + if q.PaymentDueUSD != nil { + return q.PaymentDueUSD + } + return new(big.Rat) +} + +// checkInitialQuote returns an error if the quote is invalid or exceeds the spend cap. +func checkInitialQuote(q *ExchangeQuoteSummary, maxPayment *big.Rat) error { + if !q.IsValidExchange { + return fmt.Errorf("exchange is not valid: %s", q.ValidationFailureReason) + } + paymentDue := resolvePaymentDue(q) + if paymentDue.Cmp(maxPayment) == 1 { + return fmt.Errorf("paymentDue %s exceeds max %s", paymentDue.FloatString(2), maxPayment.FloatString(2)) + } + return nil +} + +// checkReQuote returns an error if the pre-accept re-quote is invalid or exceeds the cap. +// It is called immediately before AcceptReservedInstancesExchangeQuote to narrow the +// race window between pricing changes. +func checkReQuote(q *ExchangeQuoteSummary, maxPayment *big.Rat) error { + if !q.IsValidExchange { + return fmt.Errorf("exchange no longer valid at accept time: %s", q.ValidationFailureReason) + } + paymentDue := resolvePaymentDue(q) + if paymentDue.Cmp(maxPayment) == 1 { + return fmt.Errorf( + "aborting exchange: re-quoted payment %s USD exceeds cap %s USD (pricing changed between initial quote and accept)", + paymentDue.FloatString(2), + maxPayment.FloatString(2), + ) + } + return nil +} + // executeWithAPI performs the exchange using an injected EC2ExchangeAPI. func executeWithAPI(ctx context.Context, client EC2ExchangeAPI, req ExchangeExecuteRequest) (string, *ExchangeQuoteSummary, error) { if req.MaxPaymentDueUSD == nil { return "", nil, fmt.Errorf("refusing to execute without max-payment-due-usd guardrail") } - q, err := getQuoteWithAPI(ctx, client, ExchangeQuoteRequest{ + quoteReq := ExchangeQuoteRequest{ Region: req.Region, ExpectedAccount: req.ExpectedAccount, ReservedIDs: req.ReservedIDs, @@ -313,25 +352,14 @@ func executeWithAPI(ctx context.Context, client EC2ExchangeAPI, req ExchangeExec TargetOfferingID: req.TargetOfferingID, TargetCount: req.TargetCount, DryRun: false, - }) - if err != nil { - return "", nil, err } - if !q.IsValidExchange { - return "", q, fmt.Errorf("exchange is not valid: %s", q.ValidationFailureReason) - } - - // AWS may return an empty PaymentDue for zero-cost exchanges (e.g., same-RI-type - // conversions). Treat nil as zero cost so valid zero-cost exchanges are not refused. - paymentDue := q.PaymentDueUSD - if paymentDue == nil { - paymentDue = new(big.Rat) + q, err := getQuoteWithAPI(ctx, client, quoteReq) + if err != nil { + return "", nil, err } - - // paymentDue > max => refuse - if paymentDue.Cmp(req.MaxPaymentDueUSD) == 1 { - return "", q, fmt.Errorf("paymentDue %s exceeds max %s", paymentDue.FloatString(2), req.MaxPaymentDueUSD.FloatString(2)) + if err := checkInitialQuote(q, req.MaxPaymentDueUSD); err != nil { + return "", q, err } // Re-quote immediately before accepting to narrow the window between quote and @@ -339,32 +367,12 @@ func executeWithAPI(ctx context.Context, client EC2ExchangeAPI, req ExchangeExec // pricing can change between the two calls. This second quote reduces -- but // does not eliminate -- the race window. If the fresh quote now exceeds the // cap, abort before the irreversible accept call. - freshQ, err := getQuoteWithAPI(ctx, client, ExchangeQuoteRequest{ - Region: req.Region, - ExpectedAccount: req.ExpectedAccount, - ReservedIDs: req.ReservedIDs, - Targets: req.Targets, - TargetOfferingID: req.TargetOfferingID, - TargetCount: req.TargetCount, - DryRun: false, - }) + freshQ, err := getQuoteWithAPI(ctx, client, quoteReq) if err != nil { return "", q, fmt.Errorf("pre-accept re-quote failed: %w", err) } - if !freshQ.IsValidExchange { - return "", freshQ, fmt.Errorf("exchange no longer valid at accept time: %s", freshQ.ValidationFailureReason) - } - - freshPaymentDue := freshQ.PaymentDueUSD - if freshPaymentDue == nil { - freshPaymentDue = new(big.Rat) - } - if freshPaymentDue.Cmp(req.MaxPaymentDueUSD) == 1 { - return "", freshQ, fmt.Errorf( - "aborting exchange: re-quoted payment %s USD exceeds cap %s USD (pricing changed between initial quote and accept)", - freshPaymentDue.FloatString(2), - req.MaxPaymentDueUSD.FloatString(2), - ) + if err := checkReQuote(freshQ, req.MaxPaymentDueUSD); err != nil { + return "", freshQ, err } out, err := client.AcceptReservedInstancesExchangeQuote(ctx, &ec2.AcceptReservedInstancesExchangeQuoteInput{