diff --git a/pkg/common/tokens.go b/pkg/common/tokens.go index 2974ca66..96d84493 100644 --- a/pkg/common/tokens.go +++ b/pkg/common/tokens.go @@ -106,21 +106,6 @@ func IdempotencyGUID(token string) string { return fmt.Sprintf("%s-%s-%s-%s-%s", h[0:8], h[8:12], h[12:16], h[16:20], h[20:32]) } -// ReservationOrderID returns the Azure reservationOrderID to PUT for a purchase: -// the deterministic GUID derived from token when one is supplied (issue #641, so -// a re-drive re-PUTs the same idempotent order), otherwise fallback (the caller's -// prior non-idempotent ID, e.g. a random GUID or a timestamp). Centralizing the -// choice keeps each executor's PurchaseCommitment a single statement and avoids -// repeating the same empty-token guard across every Azure service. -// -// Prefer OrderIDFromToken, which rejects a malformed non-empty token; this function is removed once providers/azure switches (#104). -func ReservationOrderID(token, fallback string) string { - if guid := IdempotencyGUID(token); guid != "" { - return guid - } - return fallback -} - // ErrInvalidIdempotencyToken is returned by OrderIDFromToken when a non-empty // token cannot be turned into a reservation order GUID. var ErrInvalidIdempotencyToken = errors.New("common: invalid idempotency token") diff --git a/pkg/common/tokens_test.go b/pkg/common/tokens_test.go index acc471a7..a0b250bd 100644 --- a/pkg/common/tokens_test.go +++ b/pkg/common/tokens_test.go @@ -113,20 +113,6 @@ func TestIdempotencyGUID_ShortTokenReturnsEmpty(t *testing.T) { assert.Equal(t, "", IdempotencyGUID("abc"), "sub-32-char token must yield empty") } -func TestReservationOrderID(t *testing.T) { - token := DeriveIdempotencyToken("exec-1", 0) - - // With a token: deterministic GUID, never the fallback. - got := ReservationOrderID(token, "fallback-id") - assert.Equal(t, IdempotencyGUID(token), got, "a supplied token must derive the GUID") - assert.Equal(t, got, ReservationOrderID(token, "other-fallback"), - "same token must yield the same order ID regardless of fallback") - - // No usable token: caller's fallback is returned verbatim. - assert.Equal(t, "fallback-id", ReservationOrderID("", "fallback-id")) - assert.Equal(t, "fallback-id", ReservationOrderID("abc", "fallback-id")) -} - func TestOrderIDFromToken(t *testing.T) { hex32 := "0123456789abcdef0123456789abcdef" derived := DeriveIdempotencyToken("exec-1", 0) diff --git a/providers/azure/go.mod b/providers/azure/go.mod index 6346bc83..4ea54ae5 100644 --- a/providers/azure/go.mod +++ b/providers/azure/go.mod @@ -15,7 +15,7 @@ require ( github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armsubscriptions v1.3.0 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/search/armsearch v1.4.0 github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/sql/armsql v1.2.0 - github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261016163842-4566498f2a08 + github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261017001455-7383c3a7db7e github.com/stretchr/testify v1.11.1 golang.org/x/sync v0.23.0 ) diff --git a/providers/azure/go.sum b/providers/azure/go.sum index 5aeb4c36..932b78b5 100644 --- a/providers/azure/go.sum +++ b/providers/azure/go.sum @@ -38,8 +38,8 @@ github.com/AzureAD/microsoft-authentication-extensions-for-go/cache v0.1.1 h1:WJ github.com/AzureAD/microsoft-authentication-extensions-for-go/cache v0.1.1/go.mod h1:tCcJZ0uHAmvjsVYzEFivsRTN00oz5BEsRgQHu5JZ9WE= github.com/AzureAD/microsoft-authentication-library-for-go v1.6.0 h1:XRzhVemXdgvJqCH0sFfrBUTnUJSBrBf7++ypk+twtRs= github.com/AzureAD/microsoft-authentication-library-for-go v1.6.0/go.mod h1:HKpQxkWaGLJ+D/5H8QRpyQXA1eKjxkFlOMwck5+33Jk= -github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261016163842-4566498f2a08 h1:5gtjJzL+IexMiQ4QvMHYHozLgEqehQUIiiO8rbnu9fs= -github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261016163842-4566498f2a08/go.mod h1:SCDEgHG6evjPh0RsoXof+J3aMmGXNFMrwbJIle7rMpQ= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261017001455-7383c3a7db7e h1:SeuL9mqUKVNiDa4yXU9IdaKKlwNo/iaaDmDXMHClw+o= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261017001455-7383c3a7db7e/go.mod h1:SCDEgHG6evjPh0RsoXof+J3aMmGXNFMrwbJIle7rMpQ= github.com/aws/aws-sdk-go-v2 v1.41.5 h1:dj5kopbwUsVUVFgO4Fi5BIT3t4WyqIDjGKCangnV/yY= github.com/aws/aws-sdk-go-v2 v1.41.5/go.mod h1:mwsPRE8ceUUpiTgF7QmQIJ7lgsKUPQOUl3o72QBrE1o= github.com/aws/smithy-go v1.24.2 h1:FzA3bu/nt/vDvmnkg+R8Xl46gmzEDam6mZ1hzmwXFng= diff --git a/providers/azure/services/savingsplans/client.go b/providers/azure/services/savingsplans/client.go index 4bd1cf0e..c3b3b600 100644 --- a/providers/azure/services/savingsplans/client.go +++ b/providers/azure/services/savingsplans/client.go @@ -314,10 +314,11 @@ func commitmentCurrency(d *common.SavingsPlanDetails) (string, error) { // PUT with a previously-used name returns the existing order alias rather than // creating a second savings plan. When opts.IdempotencyToken is set (the // purchase-execution path), the alias name is derived deterministically from it -// via common.ReservationOrderID so a re-drive of a stranded execution (issue +// via common.OrderIDFromToken so a re-drive of a stranded execution (issue // #636) re-PUTs the same alias instead of double-buying. When the token is empty // (the CLI path, which has no owning execution), the prior timestamp-based name -// is retained. +// is retained. A non-empty malformed token returns an error wrapping +// common.ErrInvalidIdempotencyToken before any PUT. func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { result := common.PurchaseResult{ Recommendation: rec, @@ -348,6 +349,16 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati return result, result.Error } displayName := fmt.Sprintf("cudly-%s-%s", spDetails.PlanType, rec.Term) + // Derive a deterministic alias name from the idempotency token when present + // so a re-drive PUTs the same alias (idempotent), falling back to the prior + // timestamp-based name on the CLI path (empty token). A malformed non-empty + // token fails here, before the body or alias client exists, so it can never + // downgrade to a non-idempotent purchase. + aliasName, err := common.OrderIDFromToken(opts.IdempotencyToken, fmt.Sprintf("cudly-%d", time.Now().UnixNano())) + if err != nil { + result.Error = err + return result, result.Error + } body := armbillingbenefits.SavingsPlanOrderAliasModel{ SKU: &armbillingbenefits.SKU{Name: toPtr(spDetails.PlanType)}, @@ -376,10 +387,6 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati aliasClient = &realOrderAliasClient{client: client} } - // Derive a deterministic alias name from the idempotency token when present - // so a re-drive PUTs the same alias (idempotent), falling back to the prior - // timestamp-based name on the CLI path (empty token). - aliasName := common.ReservationOrderID(opts.IdempotencyToken, fmt.Sprintf("cudly-%d", time.Now().UnixNano())) poller, err := aliasClient.BeginCreate(ctx, aliasName, body, nil) if err != nil { result.Error = fmt.Errorf("failed to begin savings plan purchase: %w", err) diff --git a/providers/azure/services/savingsplans/client_test.go b/providers/azure/services/savingsplans/client_test.go index 838a4f97..5b46565d 100644 --- a/providers/azure/services/savingsplans/client_test.go +++ b/providers/azure/services/savingsplans/client_test.go @@ -356,14 +356,15 @@ func TestPurchaseCommitment_DeterministicAliasNameFromToken(t *testing.T) { } token := common.DeriveIdempotencyToken("exec-1", 0) - wantName := common.ReservationOrderID(token, "") + wantName, err := common.OrderIDFromToken(token, "") + require.NoError(t, err) require.NotEmpty(t, wantName, "token must yield a deterministic GUID") // First drive. mock1 := &mockOrderAliasClient{createPoller: &mockPoller{resp: resp}} c1 := NewClient(nil, "sub", "eastus") c1.SetOrderAliasClient(mock1) - _, err := c1.PurchaseCommitment(context.Background(), makeRec("1yr"), common.PurchaseOptions{IdempotencyToken: token}) + _, err = c1.PurchaseCommitment(context.Background(), makeRec("1yr"), common.PurchaseOptions{IdempotencyToken: token}) require.NoError(t, err) assert.Equal(t, wantName, mock1.capturedName) @@ -377,6 +378,35 @@ func TestPurchaseCommitment_DeterministicAliasNameFromToken(t *testing.T) { "re-drive with the same idempotency token must reuse the alias name (PUT is idempotent)") } +// TestPurchaseCommitment_MalformedTokenFailsBeforePUT verifies a non-empty token +// that yields no GUID is refused before BeginCreate. The case without an +// injected alias client checks the error is the token error, not "failed to +// create order alias client". Pre-fix the malformed token silently fell back to +// a timestamp alias name and the purchase went ahead. +func TestPurchaseCommitment_MalformedTokenFailsBeforePUT(t *testing.T) { + for _, token := range []string{"abc", "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz"} { + id := "/subscriptions/sub/providers/Microsoft.BillingBenefits/savingsPlanOrders/o" + mock := &mockOrderAliasClient{createPoller: &mockPoller{resp: armbillingbenefits.SavingsPlanOrderAliasClientCreateResponse{ + SavingsPlanOrderAliasModel: armbillingbenefits.SavingsPlanOrderAliasModel{ID: &id}, + }}} + c := NewClient(nil, "sub", "eastus") + c.SetOrderAliasClient(mock) + result, err := c.PurchaseCommitment(context.Background(), makeRec("1yr"), common.PurchaseOptions{IdempotencyToken: token}) + require.Error(t, err) + assert.ErrorIs(t, err, common.ErrInvalidIdempotencyToken) + assert.False(t, result.Success) + assert.Zero(t, mock.calls, "no BeginCreate for a malformed token") + assert.NotContains(t, err.Error(), token) + } + + // No injected alias client and a nil credential: the token error must win + // over "failed to create order alias client". + c := NewClient(nil, "sub", "eastus") + _, err := c.PurchaseCommitment(context.Background(), makeRec("1yr"), common.PurchaseOptions{IdempotencyToken: "abc"}) + require.Error(t, err) + assert.ErrorIs(t, err, common.ErrInvalidIdempotencyToken) +} + // TestPurchaseCommitment_EmptyTokenUsesTimestampName verifies the CLI path (no // idempotency token) retains a non-deterministic timestamp-based alias name. func TestPurchaseCommitment_EmptyTokenUsesTimestampName(t *testing.T) {