diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b84ea95..f5b6c14b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Notices +- **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 + route Azure Redis purchases to the managedredis client. The refusal wraps + `common.ErrRefusedBeforePurchase` (nothing was sent), not an upfront-approval + failure, so approving and retrying does not help. - **Federation IaC bundles downloaded before 2026-04-22 need to be re-downloaded** to get zero-touch registration. Older bundles silently skip auto-registration unless manually edited (Terraform `registration.tf` @@ -179,6 +185,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Changed - **BREAKING `azure`:** Azure Redis recommendations now come only from managedredis and are labelled `memorydb` (`ServiceMemoryDB`), both from Consumption and from Advisor rows. The cache client's `GetRecommendations` returns none, so `GetRecommendationsForService(ServiceCache)` is empty for Azure, and consumers that stored Azure Redis plans or settings under `cache` must also match `memorydb`. Azure Redis recommendations keep `CacheDetails` `Engine` and `NodeType` but no longer report `Shards` (0, unknown) (#692). +- **`azure`:** the cache service no longer purchases Azure Redis reservations; + managedredis (`ServiceMemoryDB`) is the only service that buys `RedisCache`, + so one Redis recommendation cannot be bought once through each service. + `cache.PurchaseCommitment` now refuses before any API call with an error + wrapping `common.ErrRefusedBeforePurchase`. Consumers that buy an Azure + `ServiceCache` recommendation must route it to managedredis (#538). - `ListTargetOfferings` now walks up to 50 pages (about 5000 offerings) instead of 10, and returns an error wrapping the new `ec2.ErrTargetOfferingsTruncated` (with no list) when that bound is hit, diff --git a/providers/azure/go.mod b/providers/azure/go.mod index 09507a8f..a30de384 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-20261026141505-6c5673cd5518 + github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261104063122-3efa339480ac github.com/stretchr/testify v1.12.1 golang.org/x/sync v0.24.0 ) @@ -23,8 +23,8 @@ require ( require ( github.com/Azure/azure-sdk-for-go/sdk/internal v1.13.0 // indirect github.com/AzureAD/microsoft-authentication-library-for-go v1.10.1 // indirect - github.com/aws/aws-sdk-go-v2 v1.41.5 // indirect - github.com/aws/smithy-go v1.24.2 // indirect + github.com/aws/aws-sdk-go-v2 v1.47.4 // indirect + github.com/aws/smithy-go v1.28.6 // indirect github.com/golang-jwt/jwt/v5 v5.3.1 // indirect github.com/google/uuid v1.6.0 // indirect github.com/kylelemons/godebug v1.1.0 // indirect diff --git a/providers/azure/go.sum b/providers/azure/go.sum index 63e53b60..cfd17080 100644 --- a/providers/azure/go.sum +++ b/providers/azure/go.sum @@ -36,12 +36,12 @@ 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.10.1 h1:0O6j18nQDoIff6/mtosT5LNU2WehEwp+LqfX6699oqY= github.com/AzureAD/microsoft-authentication-library-for-go v1.10.1/go.mod h1:xdYAf5bjkOpsd3auA8riiJW4vneBubt9caavL7626jQ= -github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261026141505-6c5673cd5518 h1:2wyUUf+lBH+Qo3ULGiHOhn9drt+WyqLoR8fV/3q21a0= -github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261026141505-6c5673cd5518/go.mod h1:rEJErEsL40Np/ttf0RDoNNC2XQsN1GmWyFf34y0JLD8= -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= -github.com/aws/smithy-go v1.24.2/go.mod h1:YE2RhdIuDbA5E5bTdciG9KrW3+TiEONeUWCqxX9i1Fc= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261104063122-3efa339480ac h1:EvEUCt1uH5oBtPcJJPt1mX6XYQWAKtL9QBSo9CfeaOw= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261104063122-3efa339480ac/go.mod h1:pseCFRgm1rzBKomktjw6E0ayxmVE7XfHHiOmew4ka+g= +github.com/aws/aws-sdk-go-v2 v1.47.4 h1:bTvk6UdSSSvaQsfLVj1JRDEEqj/PDRwGf1KanaRzau0= +github.com/aws/aws-sdk-go-v2 v1.47.4/go.mod h1:XkrjAZvr705Nytv6PkPUhoSB2W9+vVHBp5oAWpkh8Ak= +github.com/aws/smithy-go v1.28.6 h1:Pj+/sUviNYjow8ELRtNwKHZ9ECeF+7pATX3+shjLIrw= +github.com/aws/smithy-go v1.28.6/go.mod h1:YE2RhdIuDbA5E5bTdciG9KrW3+TiEONeUWCqxX9i1Fc= github.com/golang-jwt/jwt/v5 v5.3.1 h1:kYf81DTWFe7t+1VvL7eS+jKFVWaUnK9cB1qbwn63YCY= github.com/golang-jwt/jwt/v5 v5.3.1/go.mod h1:fxCRLWMO43lRc8nhHWY6LGqRcf+1gQWArsqaEUEa5bE= github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0= diff --git a/providers/azure/services/cache/client.go b/providers/azure/services/cache/client.go index 67b98aa6..acb1035a 100644 --- a/providers/azure/services/cache/client.go +++ b/providers/azure/services/cache/client.go @@ -12,7 +12,6 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/redis/armredis/v3" - "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/internal/httpclient" @@ -201,79 +200,12 @@ func (c *Client) convertRedisReservation(detail *armconsumption.ReservationDetai return commitment } -// PurchaseCommitment purchases Redis Cache reserved capacity using the two-step -// calculatePrice->purchase flow required by Azure's Reservations API (issue #677). -func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { - result := common.PurchaseResult{ - Recommendation: rec, - DryRun: false, - Success: false, - Timestamp: time.Now(), - } - - // Source is required so the resulting reservation is attributable to CUDly - // in the portal via the purchase-automation tag. The dedupe key for - // idempotent re-drives is now opts.IdempotencyToken (issue #721, applied - // in reservations.DoIdempotentPurchaseTwoStep); source remains mandatory - // for attribution. - if opts.Source == "" { - result.Error = fmt.Errorf("purchase source is required for Azure reservation purchases") - return result, result.Error - } - if strings.TrimSpace(rec.ResourceType) == "" { - result.Error = fmt.Errorf("resource type is required") - return result, result.Error - } - if rec.Count <= 0 { - result.Error = fmt.Errorf("quantity must be greater than zero, got %d", rec.Count) - return result, result.Error - } - - termYears, termErr := reservations.ParseTermYears(rec.Term) - if termErr != nil { - result.Error = termErr - return result, result.Error - } - if _, termEnumErr := reservations.ReservationTermForYears(termYears); termEnumErr != nil { - result.Error = termEnumErr - return result, result.Error - } - billingPlan, billingPlanErr := reservations.BillingPlanForPaymentOption(rec.PaymentOption) - if billingPlanErr != nil { - result.Error = billingPlanErr - return result, result.Error - } - - bodyBytes, err := reservations.BuildPurchaseBody(reservations.PurchaseParams{ - ReservedResourceType: string(armreservations.ReservedResourceTypeRedisCache), - DisplayName: func(r common.Recommendation) string { - return reservations.BuildDisplayName(reservations.DisplayNameFields{ - Service: "redis", - Region: c.region, - ResourceType: r.ResourceType, - Count: r.Count, - Term: r.Term, - Payment: r.PaymentOption, - Now: time.Now(), - }) - }, - }, rec, opts, c.region, c.subscriptionID, billingPlan) - if err != nil { - result.Error = fmt.Errorf("failed to marshal request: %w", err) - return result, result.Error - } - - reservationOrderID, existing, err := reservations.Submit(ctx, c.cred, c.httpClient, bodyBytes, opts, rec.CommitmentCurrency) - if err != nil { - result.Error = err - return result, result.Error - } - - result.Success = true - result.ExistingCommitment = existing - result.CommitmentID = reservationOrderID - result.Cost = reservations.UpfrontCostForBillingPlan(billingPlan) - return result, nil +// PurchaseCommitment refuses: managedredis (ServiceMemoryDB) is the only Azure service that +// buys RedisCache reservations (#538), so one Redis recommendation cannot be bought twice. +func (c *Client) PurchaseCommitment(_ context.Context, rec common.Recommendation, _ common.PurchaseOptions) (common.PurchaseResult, error) { + err := fmt.Errorf("%w: Azure Redis reservations are purchased through the %s service, not %s", + common.ErrRefusedBeforePurchase, common.ServiceMemoryDB, common.ServiceCache) + return common.PurchaseResult{Recommendation: rec, Timestamp: time.Now(), Error: err}, err } // ValidateOffering validates that a Redis Cache SKU exists. diff --git a/providers/azure/services/cache/client_test.go b/providers/azure/services/cache/client_test.go index e645c1da..3ca58d62 100644 --- a/providers/azure/services/cache/client_test.go +++ b/providers/azure/services/cache/client_test.go @@ -1,17 +1,11 @@ package cache import ( - "bytes" "context" - "encoding/json" "errors" - "io" "net/http" "testing" - "time" - "github.com/Azure/azure-sdk-for-go/sdk/azcore" - "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" "github.com/Azure/azure-sdk-for-go/sdk/azcore/to" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/redis/armredis/v3" @@ -796,432 +790,6 @@ func TestCacheClient_GetCommonSKUs(t *testing.T) { // Test the to package is properly imported (used in tests). var _ = to.Ptr("test") -// MockTokenCredential for testing PurchaseCommitment. -type MockTokenCredential struct { - token string - err error -} - -func (m *MockTokenCredential) GetToken(ctx context.Context, options policy.TokenRequestOptions) (azcore.AccessToken, error) { - if m.err != nil { - return azcore.AccessToken{}, m.err - } - return azcore.AccessToken{ - Token: m.token, - ExpiresOn: time.Now().Add(time.Hour), - }, nil -} - -// calcPriceRespJSON returns a minimal calculatePrice response JSON for tests. -func calcPriceRespJSON(orderID string) string { - return `{"properties":{"billingCurrencyTotal":{"currencyCode":"USD"},"reservationOrderId":"` + orderID + `"}}` -} - -func TestCacheClient_PurchaseCommitment_Success(t *testing.T) { - t.Parallel() - ctx := context.Background() - mockHTTP := &mocks.MockHTTPClient{} - mockCred := &MockTokenCredential{token: "test-token"} - client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - - calculateResp7 := mocks.CreateMockHTTPResponse(http.StatusOK, calcPriceRespJSON("cache-order-001")) - t.Cleanup(func() { - require.NoError(t, calculateResp7.Body.Close()) - }) - mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { - return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" - })).Return(calculateResp7, nil).Once() - purchaseResp8 := mocks.CreateMockHTTPResponse(http.StatusOK, `{}`) - t.Cleanup(func() { - require.NoError(t, purchaseResp8.Body.Close()) - }) - mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { - return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/cache-order-001/purchase" - })).Return(purchaseResp8, nil).Once() - - rec := common.Recommendation{ - ResourceType: "Premium_P1", - Term: "1yr", - Count: 1, - CommitmentCost: 1000.0, - PaymentOption: "no-upfront", - } - - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) - require.NoError(t, err) - assert.True(t, result.Success) - assert.Equal(t, "cache-order-001", result.CommitmentID) - require.NotNil(t, result.Cost) - assert.Zero(t, *result.Cost) - mockHTTP.AssertExpectations(t) -} - -func TestCacheClient_PurchaseCommitment_3YearTerm(t *testing.T) { - t.Parallel() - ctx := context.Background() - mockHTTP := &mocks.MockHTTPClient{} - mockCred := &MockTokenCredential{token: "test-token"} - client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - - calculateResp9 := mocks.CreateMockHTTPResponse(http.StatusOK, calcPriceRespJSON("cache-order-3yr")) - t.Cleanup(func() { - require.NoError(t, calculateResp9.Body.Close()) - }) - mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { - return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" - })).Return(calculateResp9, nil).Once() - purchaseResp10 := mocks.CreateMockHTTPResponse(http.StatusCreated, `{}`) - t.Cleanup(func() { - require.NoError(t, purchaseResp10.Body.Close()) - }) - mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { - return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/cache-order-3yr/purchase" - })).Return(purchaseResp10, nil).Once() - - rec := common.Recommendation{ - ResourceType: "Premium_P1", - Term: "3yr", - Count: 1, - CommitmentCost: 2500.0, - PaymentOption: "no-upfront", - } - - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) - require.NoError(t, err) - assert.True(t, result.Success) - assert.Equal(t, "cache-order-3yr", result.CommitmentID) - mockHTTP.AssertExpectations(t) -} - -func TestCacheClient_PurchaseCommitment_Accepted(t *testing.T) { - t.Parallel() - ctx := context.Background() - mockHTTP := &mocks.MockHTTPClient{} - mockCred := &MockTokenCredential{token: "test-token"} - client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - - calculateResp11 := mocks.CreateMockHTTPResponse(http.StatusOK, calcPriceRespJSON("cache-order-202")) - t.Cleanup(func() { - require.NoError(t, calculateResp11.Body.Close()) - }) - mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { - return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" - })).Return(calculateResp11, nil).Once() - purchaseResp12 := mocks.CreateMockHTTPResponse(http.StatusAccepted, `{}`) - t.Cleanup(func() { - require.NoError(t, purchaseResp12.Body.Close()) - }) - mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { - return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/cache-order-202/purchase" - })).Return(purchaseResp12, nil).Once() - - rec := common.Recommendation{ - ResourceType: "Premium_P1", - Term: "1yr", - Count: 1, - CommitmentCost: 1000.0, - PaymentOption: "no-upfront", - } - - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) - require.NoError(t, err) - assert.True(t, result.Success) - mockHTTP.AssertExpectations(t) -} - -func TestCacheClient_PurchaseCommitment_TokenError(t *testing.T) { - t.Parallel() - ctx := context.Background() - mockHTTP := &mocks.MockHTTPClient{} - mockCred := &MockTokenCredential{err: errors.New("token error")} - client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - - rec := common.Recommendation{ - ResourceType: "Premium_P1", - Term: "1yr", - Count: 1, - PaymentOption: "no-upfront", - } - - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) - require.Error(t, err) - assert.False(t, result.Success) - assert.Contains(t, err.Error(), "failed to get access token") -} - -func TestCacheClient_PurchaseCommitment_HTTPError(t *testing.T) { - t.Parallel() - ctx := context.Background() - mockHTTP := &mocks.MockHTTPClient{} - mockCred := &MockTokenCredential{token: "test-token"} - client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - - mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { - return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" - })).Return(nil, errors.New("network error")).Once() - - rec := common.Recommendation{ - ResourceType: "Premium_P1", - Term: "1yr", - Count: 1, - PaymentOption: "no-upfront", - } - - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) - require.Error(t, err) - assert.False(t, result.Success) - assert.Contains(t, err.Error(), "calculatePrice HTTP call") -} - -func TestCacheClient_PurchaseCommitment_BadStatus(t *testing.T) { - t.Parallel() - ctx := context.Background() - mockHTTP := &mocks.MockHTTPClient{} - mockCred := &MockTokenCredential{token: "test-token"} - client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - - calculateResp13 := mocks.CreateMockHTTPResponse(http.StatusOK, calcPriceRespJSON("cache-order-bad")) - t.Cleanup(func() { - require.NoError(t, calculateResp13.Body.Close()) - }) - mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { - return r.URL.Path == "/providers/Microsoft.Capacity/calculatePrice" - })).Return(calculateResp13, nil).Once() - purchaseResp14 := mocks.CreateMockHTTPResponse(http.StatusBadRequest, `{"error": "invalid request"}`) - t.Cleanup(func() { - require.NoError(t, purchaseResp14.Body.Close()) - }) - mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { - return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/cache-order-bad/purchase" - })).Return(purchaseResp14, nil).Once() - - rec := common.Recommendation{ - ResourceType: "Premium_P1", - Term: "1yr", - Count: 1, - PaymentOption: "no-upfront", - } - - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) - require.Error(t, err) - assert.False(t, result.Success) - assert.Contains(t, err.Error(), "reservation purchase failed with status 400") - mockHTTP.AssertExpectations(t) -} - -// TestCacheClient_PurchaseCommitment_TagInjection verifies that the -// purchase-automation tag carrying opts.Source is present in the -// calculatePrice request body. Without this regression test the dedupe -// guard introduced for the Azure two-step flow could regress silently: -// the call would succeed without the tag and re-driven purchases would -// duplicate reservations server-side. -func TestCacheClient_PurchaseCommitment_TagInjection(t *testing.T) { - t.Parallel() - const orderID = "cache-tag-test" - const source = common.PurchaseSourceWeb - - ctx := context.Background() - mockHTTP := &mocks.MockHTTPClient{} - mockCred := &MockTokenCredential{token: "test-token"} - client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - - var capturedBody []byte - calculateResp15 := mocks.CreateMockHTTPResponse(http.StatusOK, calcPriceRespJSON(orderID)) - t.Cleanup(func() { - require.NoError(t, calculateResp15.Body.Close()) - }) - mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { - if r.URL.Path != "/providers/Microsoft.Capacity/calculatePrice" { - return false - } - capturedBody, _ = io.ReadAll(r.Body) - r.Body = io.NopCloser(bytes.NewReader(capturedBody)) - return true - })).Return(calculateResp15, nil).Once() - purchaseResp16 := mocks.CreateMockHTTPResponse(http.StatusOK, `{}`) - t.Cleanup(func() { - require.NoError(t, purchaseResp16.Body.Close()) - }) - mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { - return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/"+orderID+"/purchase" - })).Return(purchaseResp16, nil).Once() - - rec := common.Recommendation{ResourceType: "Premium_P1", Term: "1yr", Count: 1, CommitmentCost: 1000.0, PaymentOption: "no-upfront"} - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: source}) - require.NoError(t, err) - assert.True(t, result.Success) - - var body map[string]interface{} - require.NoError(t, json.Unmarshal(capturedBody, &body)) - tags, hasTags := body["tags"].(map[string]interface{}) - require.True(t, hasTags, "tags field must be present in calculatePrice body when Source is set") - assert.Equal(t, source, tags[common.PurchaseTagKey], "tag value must match opts.Source") - mockHTTP.AssertExpectations(t) -} - -// TestCacheClient_PurchaseCommitment_BillingPlan pins the billingPlan wiring -// (issue #1502, mirroring PR #1495's fix for compute): Azure Cache for Redis -// reserved capacity supports both Upfront and Monthly billing -// (learn.microsoft.com/azure/azure-cache-for-redis/cache-reserved-pricing), -// so rec.PaymentOption must map onto the correct armreservations -// ReservationBillingPlan value in the purchase body rather than silently -// defaulting to Azure's Upfront behavior for a no-upfront/monthly rec. -func TestCacheClient_PurchaseCommitment_BillingPlan(t *testing.T) { - t.Parallel() - cases := []struct { - name string - paymentOption string - wantPlan string - wantErrSub string - }{ - {name: "all-upfront maps to Upfront", paymentOption: "all-upfront", wantPlan: "Upfront"}, - {name: "no-upfront maps to Monthly", paymentOption: "no-upfront", wantPlan: "Monthly"}, - {name: "partial-upfront is rejected", paymentOption: "partial-upfront", wantErrSub: "partial-upfront has no azure equivalent"}, - {name: "empty payment option is rejected", paymentOption: "", wantErrSub: "azure reservations support only upfront or monthly billing"}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - ctx := context.Background() - mockHTTP := &mocks.MockHTTPClient{} - mockCred := &MockTokenCredential{token: "test-token"} - client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - - rec := common.Recommendation{ResourceType: "Premium_P1", Term: "1yr", Count: 1, PaymentOption: tc.paymentOption} - - if tc.wantErrSub != "" { - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) - require.Error(t, err) - assert.False(t, result.Success) - assert.Contains(t, err.Error(), tc.wantErrSub) - mockHTTP.AssertNotCalled(t, "Do", mock.Anything) - return - } - - const orderID = "cache-billingplan-test" - var capturedBody []byte - calculateResp17 := mocks.CreateMockHTTPResponse(http.StatusOK, calcPriceRespJSON(orderID)) - t.Cleanup(func() { - require.NoError(t, calculateResp17.Body.Close()) - }) - mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { - if r.URL.Path != "/providers/Microsoft.Capacity/calculatePrice" { - return false - } - capturedBody, _ = io.ReadAll(r.Body) - r.Body = io.NopCloser(bytes.NewReader(capturedBody)) - return true - })).Return(calculateResp17, nil).Once() - purchaseResp18 := mocks.CreateMockHTTPResponse(http.StatusOK, `{}`) - t.Cleanup(func() { - require.NoError(t, purchaseResp18.Body.Close()) - }) - mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { - return r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/"+orderID+"/purchase" - })).Return(purchaseResp18, nil).Once() - - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) - require.NoError(t, err) - assert.True(t, result.Success) - - var body map[string]interface{} - require.NoError(t, json.Unmarshal(capturedBody, &body)) - props, ok := body["properties"].(map[string]interface{}) - require.True(t, ok, "properties map missing from reservation body") - assert.Equal(t, tc.wantPlan, props["billingPlan"]) - if tc.wantPlan == "Monthly" { - require.NotNil(t, result.Cost) - assert.Zero(t, *result.Cost) - } else { - assert.Nil(t, result.Cost) - } - mockHTTP.AssertExpectations(t) - }) - } -} - -// TestCacheClient_PurchaseCommitment_RequiresSource pins the dedupe guard: -// PurchaseCommitment must reject an empty opts.Source before issuing any HTTP -// call. Azure mints the reservation order ID server-side, so the -// purchase-automation tag derived from Source is the only stable dedupe -// signal CUDly controls -- proceeding without it would allow a re-driven -// purchase to create a duplicate reservation. -func TestCacheClient_PurchaseCommitment_RequiresSource(t *testing.T) { - t.Parallel() - ctx := context.Background() - mockHTTP := &mocks.MockHTTPClient{} - mockCred := &MockTokenCredential{token: "test-token"} - client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - - rec := common.Recommendation{ResourceType: "Premium_P1", Term: "1yr", Count: 1} - result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{}) - require.Error(t, err) - assert.False(t, result.Success) - assert.Contains(t, err.Error(), "purchase source is required") - // No HTTP call may be issued when the guard rejects the request. - mockHTTP.AssertNotCalled(t, "Do", mock.Anything) -} - -// TestCacheClient_PurchaseCommitment_DisplayNameConformsToAzureAllowlist guards -// against regression: displayName in the calculatePrice body must match -// [A-Za-z0-9_-]{1,64} (Azure rejects DisplayNameInvalid otherwise). -func TestCacheClient_PurchaseCommitment_DisplayNameConformsToAzureAllowlist(t *testing.T) { - t.Parallel() - ctx := context.Background() - mockHTTP := &mocks.MockHTTPClient{} - mockCred := &MockTokenCredential{token: "test-token"} - client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) - - const orderID = "azure-cache-displayname" - var capturedDisplayName string - calculateResp19 := mocks.CreateMockHTTPResponse(http.StatusOK, calcPriceRespJSON(orderID)) - t.Cleanup(func() { - require.NoError(t, calculateResp19.Body.Close()) - }) - mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { - if r.Method != http.MethodPost || r.URL.Path != "/providers/Microsoft.Capacity/calculatePrice" { - return false - } - if r.Body == nil { - return true - } - bodyBytes, _ := io.ReadAll(r.Body) - r.Body = io.NopCloser(bytes.NewReader(bodyBytes)) - var body map[string]interface{} - if err := json.Unmarshal(bodyBytes, &body); err == nil { - if props, ok := body["properties"].(map[string]interface{}); ok { - if dn, ok := props["displayName"].(string); ok { - capturedDisplayName = dn - } - } - } - return true - })).Return(calculateResp19, nil).Once() - purchaseResp20 := mocks.CreateMockHTTPResponse(http.StatusOK, `{}`) - t.Cleanup(func() { - require.NoError(t, purchaseResp20.Body.Close()) - }) - mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { - return r.Method == http.MethodPost && - r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/"+orderID+"/purchase" - })).Return(purchaseResp20, nil).Once() - - rec := common.Recommendation{ - ResourceType: "Premium_P1", - Term: "1yr", - Count: 1, - CommitmentCost: 1000.0, - PaymentOption: "no-upfront", - } - _, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) - require.NoError(t, err) - assert.NotEmpty(t, capturedDisplayName) - assert.Regexp(t, `^[A-Za-z0-9_-]{1,64}$`, capturedDisplayName) - // Rich-format guards: service code is correct and SKU is preserved - // (see providers/azure/services/internal/reservations/displayname.go). - assert.Regexp(t, `^redis-`, capturedDisplayName) - assert.Contains(t, capturedDisplayName, "Premium_P1") -} - // infiniteRedisCachesPager is a pager that always reports More()=true, // used to exercise the maxCachesPages budget cap. type infiniteRedisCachesPager struct{} diff --git a/providers/azure/services/cache/existing_commitment_test.go b/providers/azure/services/cache/existing_commitment_test.go deleted file mode 100644 index d8990103..00000000 --- a/providers/azure/services/cache/existing_commitment_test.go +++ /dev/null @@ -1,135 +0,0 @@ -package cache - -import ( - "context" - "io" - "net/http" - "strings" - "testing" - "time" - - "github.com/Azure/azure-sdk-for-go/sdk/azcore" - "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - "github.com/LeanerCloud/cloud-commitments-go/pkg/common" -) - -// Issue #211: PurchaseResult.ExistingCommitment must be true only when the -// shared guard adopted an order already tagged with the idempotency token. - -type existingCommitmentCred struct{} - -func (existingCommitmentCred) GetToken(context.Context, policy.TokenRequestOptions) (azcore.AccessToken, error) { - return azcore.AccessToken{Token: "tok", ExpiresOn: time.Now().Add(time.Hour)}, nil -} - -// existingCommitmentHTTP is a stateless ARM stub: the reservation-orders list -// returns listBody, calculatePrice mints "minted-order", and the purchase POST -// answers purchaseStatus. posts counts calculatePrice and purchase calls. -type existingCommitmentHTTP struct { - listBody string - purchaseStatus int - posts int -} - -func (h *existingCommitmentHTTP) Do(req *http.Request) (*http.Response, error) { - respond := func(status int, body string) (*http.Response, error) { - return &http.Response{StatusCode: status, Body: io.NopCloser(strings.NewReader(body)), Header: http.Header{}}, nil - } - path := req.URL.Path - switch { - case req.Method == http.MethodGet && strings.HasSuffix(path, "/reservationOrders"): - return respond(http.StatusOK, h.listBody) - case req.Method == http.MethodGet: - return respond(http.StatusOK, `{"registrationState":"Registered"}`) - case strings.HasSuffix(path, "/calculatePrice"): - h.posts++ - return respond(http.StatusOK, `{"properties":{"billingCurrencyTotal":{"currencyCode":"USD"},"reservationOrderId":"minted-order"}}`) - case strings.HasSuffix(path, "/purchase"): - h.posts++ - return respond(h.purchaseStatus, `{}`) - } - return respond(http.StatusNotFound, `{}`) -} - -func existingCommitmentRun(t *testing.T, h *existingCommitmentHTTP) (common.PurchaseResult, error) { - t.Helper() - return existingCommitmentRunCurrency(t, h, "") -} - -func existingCommitmentRunCurrency(t *testing.T, h *existingCommitmentHTTP, currency string) (common.PurchaseResult, error) { - t.Helper() - c := NewClientWithHTTP(existingCommitmentCred{}, "sub", "eastus", h) - rec := common.Recommendation{ - ResourceType: "Premium_P1", - Term: "1yr", - Count: 1, - CommitmentCost: 1000.0, - PaymentOption: "no-upfront", - - CommitmentCurrency: currency, - } - return c.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI, IdempotencyToken: "tok-211"}) -} - -func TestPurchaseCommitment_ExistingCommitment_FreshPurchaseNotFlagged(t *testing.T) { - t.Parallel() - h := &existingCommitmentHTTP{listBody: `{"value":[]}`, purchaseStatus: http.StatusOK} - result, err := existingCommitmentRun(t, h) - require.NoError(t, err) - assert.True(t, result.Success) - assert.False(t, result.ExistingCommitment) - assert.Equal(t, "minted-order", result.CommitmentID) - assert.Equal(t, 2, h.posts) -} - -func TestPurchaseCommitment_ExistingCommitment_FailedPurchaseNotFlagged(t *testing.T) { - t.Parallel() - h := &existingCommitmentHTTP{listBody: `{"value":[]}`, purchaseStatus: http.StatusInternalServerError} - result, err := existingCommitmentRun(t, h) - require.Error(t, err) - assert.False(t, result.Success) - assert.False(t, result.ExistingCommitment) -} - -func TestPurchaseCommitment_ExistingCommitment_RedriveAdoptsAndFlags(t *testing.T) { - t.Parallel() - h := &existingCommitmentHTTP{ - listBody: `{"value":[{"name":"adopted-order","properties":{"provisioningState":"Succeeded"},"tags":{"cudly-idempotency-token":"tok-211"}}]}`, - purchaseStatus: http.StatusOK, - } - result, err := existingCommitmentRun(t, h) - require.NoError(t, err) - assert.True(t, result.Success) - assert.True(t, result.ExistingCommitment) - assert.Equal(t, "adopted-order", result.CommitmentID) - assert.Zero(t, h.posts, "an adopted order must not trigger a second purchase") -} - -// A terminal-failed order carrying the token is not adopted, so the purchase -// proceeds and is not flagged. -func TestPurchaseCommitment_ExistingCommitment_FailedOrderNotAdopted(t *testing.T) { - t.Parallel() - h := &existingCommitmentHTTP{ - listBody: `{"value":[{"name":"dead-order","properties":{"provisioningState":"Failed"},"tags":{"cudly-idempotency-token":"tok-211"}}]}`, - purchaseStatus: http.StatusOK, - } - result, err := existingCommitmentRun(t, h) - require.NoError(t, err) - assert.False(t, result.ExistingCommitment) - assert.Equal(t, "minted-order", result.CommitmentID) -} - -// The recommendation currency must reach the shared purchase check: a EUR -// recommendation against a USD calculatePrice is refused before the purchase. -func TestPurchaseCommitment_RecommendationCurrencyMismatchRefused(t *testing.T) { - t.Parallel() - h := &existingCommitmentHTTP{listBody: `{"value":[]}`, purchaseStatus: http.StatusOK} - result, err := existingCommitmentRunCurrency(t, h, "EUR") - require.Error(t, err) - assert.Contains(t, err.Error(), "refusing to purchase") - assert.False(t, result.Success) - assert.Equal(t, 1, h.posts, "only calculatePrice; no purchase request") -} diff --git a/providers/azure/services/cache/purchase_owner_test.go b/providers/azure/services/cache/purchase_owner_test.go new file mode 100644 index 00000000..b33fb746 --- /dev/null +++ b/providers/azure/services/cache/purchase_owner_test.go @@ -0,0 +1,44 @@ +package cache + +import ( + "context" + "errors" + "testing" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/mocks" +) + +type unusedCred struct{ t *testing.T } + +func (c unusedCred) GetToken(context.Context, policy.TokenRequestOptions) (azcore.AccessToken, error) { + c.t.Error("the refusal must not use the credential") + return azcore.AccessToken{}, errors.New("credential used") +} + +// Only managedredis buys RedisCache reservations (#538), so the same Redis +// capacity cannot be bought once through each service. +func TestCacheClient_PurchaseCommitment_RefusesRedisPurchases(t *testing.T) { + t.Parallel() + mockHTTP := &mocks.MockHTTPClient{} + client := NewClientWithHTTP(unusedCred{t}, "test-subscription", "eastus", mockHTTP) + rec := common.Recommendation{Service: common.ServiceCache, ResourceType: "Premium_P1", Term: "1yr", Count: 1, PaymentOption: "no-upfront"} + + result, err := client.PurchaseCommitment(context.Background(), rec, + common.PurchaseOptions{Source: common.PurchaseSourceCLI, IdempotencyToken: "token-1"}) + + require.Error(t, err) + assert.True(t, errors.Is(err, common.ErrRefusedBeforePurchase), "refusal must say nothing was sent: %v", err) + assert.Contains(t, err.Error(), string(common.ServiceMemoryDB)) + assert.False(t, result.Success) + assert.Empty(t, result.CommitmentID) + assert.Equal(t, err, result.Error) + assert.Equal(t, rec, result.Recommendation) + mockHTTP.AssertNotCalled(t, "Do", mock.Anything) +} diff --git a/providers/azure/services/purchasebody_test.go b/providers/azure/services/purchasebody_test.go index e23a06b3..a45f5bac 100644 --- a/providers/azure/services/purchasebody_test.go +++ b/providers/azure/services/purchasebody_test.go @@ -9,7 +9,6 @@ import ( "github.com/stretchr/testify/require" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" - "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/cache" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/compute" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/cosmosdb" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/database" @@ -34,7 +33,6 @@ func TestPurchaseBodies(t *testing.T) { name, resourceType string build func(h *reservationstest.Capture) purchaser }{ - {"cache", "RedisCache", func(h *reservationstest.Capture) purchaser { return cache.NewClientWithHTTP(cred, "sub", "eastus", h) }}, {"compute", "VirtualMachines", func(h *reservationstest.Capture) purchaser { return compute.NewClientWithHTTP(cred, "sub", "eastus", h) }}, diff --git a/providers/azure/services/purchasegolden_test.go b/providers/azure/services/purchasegolden_test.go index 548e1f63..e7520842 100644 --- a/providers/azure/services/purchasegolden_test.go +++ b/providers/azure/services/purchasegolden_test.go @@ -10,7 +10,6 @@ import ( "github.com/stretchr/testify/require" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" - "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/cache" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/compute" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/cosmosdb" "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/database" @@ -52,9 +51,6 @@ type goldenCase struct { func goldenCases() []goldenCase { return []goldenCase{ - {"cache", "RedisCache", func(c *reservationstest.CountingCred, h *reservationstest.Capture) purchaser { - return cache.NewClientWithHTTP(c, "sub", "eastus", h) - }, "redis-eastus-GP_Gen5_8-2x-1yr-monthly-" + goldenMarker, regexp.MustCompile(`^redis-eastus-GP_Gen5_8-2x-1yr-monthly-\d{8}T\d{6}-[0-9a-f]{8}$`), nil}, {"compute", "VirtualMachines", func(c *reservationstest.CountingCred, h *reservationstest.Capture) purchaser { return compute.NewClientWithHTTP(c, "sub", "eastus", h) }, "vm-eastus-GP_Gen5_8-2x-1yr-monthly-" + goldenMarker, regexp.MustCompile(`^vm-eastus-GP_Gen5_8-2x-1yr-monthly-\d{8}T\d{6}-[0-9a-f]{8}$`), []string{methodGet + " " + urlProvider}},