From 1b035242110bec96872e7e6e198ccdd68aa40d76 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 17 Oct 2026 03:11:06 +0200 Subject: [PATCH 1/3] build(azure): bump the pkg pin past OrderIDFromToken Move providers/azure from pkg 4566498f2a08 to 7383c3a7db7e so the savingsplans purchase path can call common.OrderIDFromToken (#487). Notable pkg changes pulled in (all additive): - feat(common): OrderIDFromToken and ErrInvalidIdempotencyToken (#487) - feat(common): EC2 platform, tenancy and scope dimensions on Commitment - fix(recfilter): match EC2 commitments on those dimensions when deduplicating - feat(insurance): decode plan line-item identity and read complete pages - docs(ladder): document provider commitment precision at PurchaseLayer Refs #104 Co-Authored-By: Claude Opus 5.5 --- providers/azure/go.mod | 2 +- providers/azure/go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) 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= From 57f14335ca277ffb6021f155e72a64ae9d228582 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 17 Oct 2026 03:12:33 +0200 Subject: [PATCH 2/3] fix(azure/savingsplans)!: refuse a malformed idempotency token before the purchase PUT PurchaseCommitment silently turned a non-empty but malformed idempotency token into a timestamp alias name, so a re-drive would PUT a second alias and buy a second Savings Plan. Derive the alias name with common.OrderIDFromToken right after the currency is resolved, before the body and the alias client are built, and return the error without any PUT. An empty token still falls back to the timestamp name (CLI path). Remove the now caller-less common.ReservationOrderID and its test. BREAKING CHANGE: common.ReservationOrderID was removed in favor of common.OrderIDFromToken, which returns an error wrapping common.ErrInvalidIdempotencyToken for a malformed non-empty token. Refs #104 Co-Authored-By: Claude Opus 5.5 --- pkg/common/tokens.go | 15 --------- pkg/common/tokens_test.go | 14 --------- .../azure/services/savingsplans/client.go | 14 ++++++--- .../services/savingsplans/client_test.go | 31 +++++++++++++++++-- 4 files changed, 39 insertions(+), 35 deletions(-) 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/services/savingsplans/client.go b/providers/azure/services/savingsplans/client.go index 4bd1cf0e..ad789a16 100644 --- a/providers/azure/services/savingsplans/client.go +++ b/providers/azure/services/savingsplans/client.go @@ -348,6 +348,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 +386,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..7df6b634 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,32 @@ 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 any PUT, with or without an injected +// alias client (the second case proves the token is checked before the alias +// client is constructed). 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"} { + mock := &mockOrderAliasClient{} + 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) { From 3ae04d712b33ff9fbb8d0db07dd22629a42c2652 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 17 Oct 2026 03:14:32 +0200 Subject: [PATCH 3/3] test(azure/savingsplans): tighten the malformed-token test and fix the PurchaseCommitment doc Name OrderIDFromToken in the PurchaseCommitment doc and document the malformed-token error. Reword the comments to claim only what the test proves, and give the mock a poller so the fallback mutant fails on an assertion instead of a nil-pointer panic. Refs #104 Co-Authored-By: Claude Opus 5.5 --- providers/azure/services/savingsplans/client.go | 5 +++-- .../azure/services/savingsplans/client_test.go | 13 ++++++++----- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/providers/azure/services/savingsplans/client.go b/providers/azure/services/savingsplans/client.go index ad789a16..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, diff --git a/providers/azure/services/savingsplans/client_test.go b/providers/azure/services/savingsplans/client_test.go index 7df6b634..5b46565d 100644 --- a/providers/azure/services/savingsplans/client_test.go +++ b/providers/azure/services/savingsplans/client_test.go @@ -379,13 +379,16 @@ func TestPurchaseCommitment_DeterministicAliasNameFromToken(t *testing.T) { } // TestPurchaseCommitment_MalformedTokenFailsBeforePUT verifies a non-empty token -// that yields no GUID is refused before any PUT, with or without an injected -// alias client (the second case proves the token is checked before the alias -// client is constructed). Pre-fix the malformed token silently fell back to a -// timestamp alias name and the purchase went ahead. +// 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"} { - mock := &mockOrderAliasClient{} + 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})