diff --git a/CHANGELOG.md b/CHANGELOG.md index de33cca2..577770a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Notices +- **EC2 re-drives now refuse when several RIs carry the idempotency token.** + If more than one active or payment-pending RI has the token (#724), the + purchase is refused instead of adopting the first one: no purchase is sent and + the error wraps `common.ErrRefusedBeforePurchase`, names the count and the + masked token, and needs a human to reconcile. + - **Azure Redis purchases must go through managedredis (`ServiceMemoryDB`).** The Azure recommendations adapter still labels Redis recommendations `ServiceCache`, and the cache client now refuses every purchase (#538), so diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index bca12331..a90298fb 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -160,7 +160,7 @@ func (c *Client) purchase(ctx context.Context, rec common.Recommendation, count // would defeat the guard and risk a double-buy on a re-drive. Fail // loudly so the recovery sweep treats it as not-yet-purchased and // retries the whole guarded path, rather than silently buying. - result.Error = fmt.Errorf("idempotency lookup failed before EC2 RI purchase (refusing to purchase to avoid a possible double-buy): %w", lookupErr) + result.Error = idempotencyLookupError(lookupErr) return result, result.Error } if existingRI != nil { @@ -230,6 +230,15 @@ func (c *Client) purchase(ctx context.Context, rec common.Recommendation, count return result, nil } +// idempotencyLookupError passes an ambiguity refusal through unchanged and +// wraps any other lookup failure as a refusal to purchase. +func idempotencyLookupError(lookupErr error) error { + if errors.Is(lookupErr, common.ErrRefusedBeforePurchase) { + return lookupErr + } + return fmt.Errorf("idempotency lookup failed before EC2 RI purchase (refusing to purchase to avoid a possible double-buy): %w", lookupErr) +} + // 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: @@ -276,12 +285,17 @@ func adoptExistingRI(result *common.PurchaseResult, ri *types.ReservedInstances, } // findRIByIdempotencyToken looks for an active or payment-pending Reserved -// Instance tagged with the given idempotency token (issue #636). It returns the -// first such RI, or nil when none exists, so a re-driven purchase can +// Instance tagged with the given idempotency token (issue #636). It returns +// the single such RI, or nil when none exists, so a re-driven purchase can // short-circuit instead of buying a second commitment and report the adopted -// RI's real offering class (issue #211). Retired/canceled RIs are -// excluded (they carry the same state filter as GetExistingCommitments) so a -// returned or expired commitment does not suppress a legitimate fresh purchase. +// RI's real offering class (issue #211). Entries without a ReservedInstancesId +// are ignored. When more than one RI carries the token the commitment state is +// ambiguous (issue #724), so it fails closed: the error wraps +// common.ErrRefusedBeforePurchase (this call sent no purchase; ErrOutcomeUnknown +// would wrongly claim it may have), names the count and the masked token, and +// needs a human to reconcile. Retired/canceled RIs are excluded (they carry the +// same state filter as GetExistingCommitments) so a returned or expired +// commitment does not suppress a legitimate fresh purchase. func (c *Client) findRIByIdempotencyToken(ctx context.Context, token string) (*types.ReservedInstances, error) { input := &ec2.DescribeReservedInstancesInput{ Filters: []types.Filter{ @@ -300,13 +314,21 @@ func (c *Client) findRIByIdempotencyToken(ctx context.Context, token string) (*t if err != nil { return nil, fmt.Errorf("failed to describe reserved instances for idempotency check: %w", err) } + var matches []*types.ReservedInstances for i := range response.ReservedInstances { - ri := &response.ReservedInstances[i] - if ri.ReservedInstancesId != nil { - return ri, nil + if ri := &response.ReservedInstances[i]; ri.ReservedInstancesId != nil { + matches = append(matches, ri) } } - return nil, nil + switch len(matches) { + case 0: + return nil, nil + case 1: + return matches[0], nil + default: + return nil, fmt.Errorf("%w: %d active or payment-pending EC2 RIs carry idempotency token %s; commitment state is ambiguous and needs a human to reconcile", + common.ErrRefusedBeforePurchase, len(matches), common.MaskToken(token)) + } } // tagReservedInstance applies the standard CUDly tag set (including the diff --git a/providers/aws/services/ec2/client_test.go b/providers/aws/services/ec2/client_test.go index 3e35352d..b7462440 100644 --- a/providers/aws/services/ec2/client_test.go +++ b/providers/aws/services/ec2/client_test.go @@ -2525,3 +2525,79 @@ func TestListTargetOfferings_CancelMidWalk_StopsWithContextError(t *testing.T) { assert.Nil(t, offs) assert.Equal(t, 3, calls) } + +// TestPurchaseCommitment_IdempotencyTokenMatchCount covers issue #724: a +// re-drive adopts exactly one tagged RI, buys on zero, ignores ID-less entries +// and fails closed (no purchase) when several RIs carry the token. +func TestPurchaseCommitment_IdempotencyTokenMatchCount(t *testing.T) { + token := common.DeriveIdempotencyToken("exec-724", 0) + ri := func(id string) types.ReservedInstances { + return types.ReservedInstances{ReservedInstancesId: aws.String(id)} + } + + tests := []struct { + name string + ris []types.ReservedInstances + wantAdopt string + wantBuy bool + wantAmbig bool + wantCountIn string + }{ + {name: "zero matches buys", ris: nil, wantBuy: true}, + {name: "only id-less entries buys", ris: []types.ReservedInstances{{}, {}}, wantBuy: true}, + {name: "one match adopts", ris: []types.ReservedInstances{ri("ri-a")}, wantAdopt: "ri-a"}, + {name: "two matches fail closed", ris: []types.ReservedInstances{ri("ri-a"), ri("ri-b")}, wantAmbig: true, wantCountIn: "2 "}, + {name: "three matches fail closed", ris: []types.ReservedInstances{ri("ri-a"), ri("ri-b"), ri("ri-c")}, wantAmbig: true, wantCountIn: "3 "}, + {name: "id-less entry plus one RI adopts", ris: []types.ReservedInstances{{}, ri("ri-a")}, wantAdopt: "ri-a"}, + {name: "two entries one id-less adopts", ris: []types.ReservedInstances{ri("ri-a"), {}}, wantAdopt: "ri-a"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mockEC2 := &MockEC2Client{} + client := &Client{client: mockEC2, region: "us-east-1"} + mockEC2.On("DescribeReservedInstances", mock.Anything, mock.Anything). + Return(&ec2.DescribeReservedInstancesOutput{ReservedInstances: tt.ris}, nil).Once() + if tt.wantBuy { + // Reaching the offering lookup proves the guard fell through to a purchase. + mockEC2.On("DescribeReservedInstancesOfferings", mock.Anything, mock.Anything). + Return(nil, errors.New("offering lookup reached")).Once() + } + rec := common.Recommendation{ + ResourceType: "t3.micro", + Count: 1, + PaymentOption: "all-upfront", + Term: "1yr", + Details: &common.ComputeDetails{Platform: "Linux/UNIX", Tenancy: "default", Scope: "Region"}, + } + + result, err := client.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{IdempotencyToken: token}) + + switch { + case tt.wantAdopt != "": + require.NoError(t, err) + assert.True(t, result.Success) + assert.True(t, result.ExistingCommitment) + assert.Equal(t, tt.wantAdopt, result.CommitmentID) + case tt.wantAmbig: + require.Error(t, err) + assert.False(t, result.Success) + assert.False(t, result.ExistingCommitment) + assert.Empty(t, result.CommitmentID) + assert.ErrorIs(t, err, common.ErrRefusedBeforePurchase) + assert.NotErrorIs(t, err, common.ErrOutcomeUnknown) + assert.Contains(t, err.Error(), tt.wantCountIn) + assert.Contains(t, err.Error(), common.MaskToken(token)) + assert.NotContains(t, err.Error(), token) + assert.Contains(t, err.Error(), "ambiguous") + assert.NotContains(t, err.Error(), "idempotency lookup failed") + assert.Equal(t, err, result.Error) + mockEC2.AssertNotCalled(t, "PurchaseReservedInstancesOffering", mock.Anything, mock.Anything) + mockEC2.AssertNotCalled(t, "DescribeReservedInstancesOfferings", mock.Anything, mock.Anything) + case tt.wantBuy: + require.Error(t, err) + assert.Contains(t, err.Error(), "offering lookup reached") + } + mockEC2.AssertExpectations(t) + }) + } +}