diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index fe0fc1a3..760df4a8 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -364,11 +364,7 @@ func (h *Handler) runPlannedPurchase(ctx context.Context, req *events.LambdaFunc }, nil } -// executeFailureError maps a failure of the shared approve-and-execute funnel -// (run-now, session approve, direct execute) to its HTTP error. action names -// what the caller tried ("started", "approved", "direct-executed"). A non-nil -// final means the purchase ran and money may have moved, so it is never a 409. -// ErrNotFound and ErrExecutionNotInExpectedStatus are 409 only before a claim. +// Stale Azure prices are refused before purchase, even after a successful claim. func executeFailureError(executionID, action string, final *config.PurchaseExecution, err error) error { switch { case errors.Is(err, purchase.ErrFourEyesDenied): @@ -379,6 +375,8 @@ func executeFailureError(executionID, action string, final *config.PurchaseExecu return fmt.Errorf("execution %s could not be %s: %w", executionID, action, err) case errors.Is(err, config.ErrAuditLoss): return NewClientError(500, fmt.Sprintf("execution %s ran but its final status could not be saved: %v", executionID, err)) + case errors.Is(err, purchase.ErrStaleAzurePricing): + return NewClientError(409, err.Error()) default: return NewClientErrorWithDetails(502, fmt.Sprintf("execution %s failed: %v", executionID, err), map[string]any{"execution_id": executionID, "status": final.Status}) @@ -687,6 +685,10 @@ func (h *Handler) requireSessionBeforeLookup(ctx context.Context, req *events.La // whose status no longer allows the action is 409. Anything else stays a server error. func tokenActionError(err error) error { switch { + case errors.Is(err, config.ErrAuditLoss): + return err + case errors.Is(err, purchase.ErrStaleAzurePricing): + return NewClientError(409, err.Error()) case errors.Is(err, purchase.ErrInvalidApprovalToken): return NewClientError(403, "invalid approval token") case errors.Is(err, purchase.ErrApprovalTokenExpired): diff --git a/internal/api/handler_purchases_retry_redrive_test.go b/internal/api/handler_purchases_retry_redrive_test.go index 199cea7c..d7b00137 100644 --- a/internal/api/handler_purchases_retry_redrive_test.go +++ b/internal/api/handler_purchases_retry_redrive_test.go @@ -124,6 +124,12 @@ func executeRedriveSuccessor(t *testing.T, successor *config.PurchaseExecution) running.Status = "running" store := new(MockConfigStore) + for _, rec := range approved.Recommendations { + if rec.Provider == "azure" { + store.On("ListStoredRecommendations", mock.Anything, config.RecommendationFilter{Provider: "azure", RequireAzurePricingMigration: true}).Return(approved.Recommendations, nil).Once() + break + } + } t.Cleanup(func() { store.AssertExpectations(t) }) store.On("GetExecutionByID", mock.Anything, approved.ExecutionID).Return(&approved, nil).Once() @@ -287,6 +293,7 @@ func TestRetryOfUnknownProviderRowIsRefused(t *testing.T) { // first attempt had in fact landed. func TestRetryOfFailedAzureReservationStillPurchasesOnce(t *testing.T) { failed := redriveFailedRow(redriveAzureRIExecID, "azure", "compute") + failed.Recommendations[0].ID = "current-azure" tokens, err := purchasesFiredByRetry(t, failed, sessionRetryReq()) diff --git a/internal/api/purchase_pricing_test.go b/internal/api/purchase_pricing_test.go index c07041d5..f19dcb67 100644 --- a/internal/api/purchase_pricing_test.go +++ b/internal/api/purchase_pricing_test.go @@ -4,11 +4,13 @@ import ( "context" "encoding/json" "errors" + "fmt" "testing" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" "github.com/aws/aws-lambda-go/events" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -1148,3 +1150,55 @@ func TestRouter_PurchasesExecute_EmptyIDMismatchedTenancyRefused(t *testing.T) { assert.Equal(t, 409, ce.code) mockStore.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) } + +func TestAzureCommitmentUnitsRegression(t *testing.T) { + upfront := config.RecommendationRecord{Provider: "azure", Term: 3, Count: 1, UpfrontCost: 3600} + monthly := config.RecommendationRecord{Provider: "azure", Term: 3, Count: 1, MonthlyCost: float64Ptr(100)} + assert.Equal(t, recTotalCommitment(&upfront), recTotalCommitment(&monthly)) + assert.Equal(t, 3600.0, recTotalCommitment(&monthly)) + assert.Equal(t, 3600.0, buildRecommendationsResponse([]config.RecommendationRecord{upfront, monthly}).Summary.TotalUpfrontCost) + stored := upfront + stored.MonthlyCost = float64Ptr(10) + stored.OnDemandCost = float64Ptr(150) + stored.Savings = 50 + req := stored + req.Count = 2 + priced, err := priceFromStored(&req, &stored, 0) + require.NoError(t, err) + assert.Equal(t, 7200.0, priced.UpfrontCost) + assert.Equal(t, 20.0, *priced.MonthlyCost) + assert.Equal(t, 300.0, *priced.OnDemandCost) + assert.Equal(t, 100.0, priced.Savings) +} + +func TestAzureCorrectedPriceExceedsOldUnitsCap(t *testing.T) { + ctx := context.Background() + store := new(MockConfigStore) + authService := new(MockAuthService) + authService.On("ValidateSession", ctx, "azure-cap").Return(&Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "capped@example.com"}, nil) + authService.grantPermissions([]auth.Permission{{Action: auth.ActionExecute, Resource: auth.ResourcePurchases, Constraints: &auth.PermissionConstraints{MaxPurchaseAmount: 1000}}}) + store.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil).Maybe() + store.On("GetPendingExecutions", mock.Anything).Return([]config.PurchaseExecution{}, nil).Maybe() + expectStoredRecs(store, config.RecommendationRecord{ID: "azure-corrected", Provider: "azure", Service: "compute", Region: "westeurope", ResourceType: "Standard_D2s_v3", Count: 1, Term: 3, Payment: "upfront", UpfrontCost: 3600, Savings: 50}) + h := &Handler{config: store, auth: authService} + _, err := h.executePurchase(ctx, &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer azure-cap"}, Body: `{"recommendations":[{"id":"azure-corrected","provider":"azure","service":"compute","region":"westeurope","resource_type":"Standard_D2s_v3","count":1,"term":3,"payment":"all-upfront","upfront_cost":70,"savings":50}]}`}) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 403, ce.code) + assert.Contains(t, err.Error(), "exceeds the constraints configured on your execute permission for purchases") + store.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) +} + +func TestStaleAzurePricingErrorMapping(t *testing.T) { + stale := fmt.Errorf("%w: %w", purchase.ErrStaleAzurePricing, config.ErrAzurePricingNotReady) + audit := fmt.Errorf("%w: %w", config.ErrAuditLoss, stale) + for _, mapped := range []error{executeFailureError("id", "approved", &config.PurchaseExecution{Status: "failed"}, stale), tokenActionError(stale)} { + ce, ok := IsClientError(mapped) + require.True(t, ok) + assert.Equal(t, 409, ce.code) + } + ce, ok := IsClientError(executeFailureError("id", "approved", &config.PurchaseExecution{Status: "failed"}, audit)) + require.True(t, ok) + assert.Equal(t, 500, ce.code) + assert.ErrorIs(t, tokenActionError(audit), config.ErrAuditLoss) +} diff --git a/internal/config/errors.go b/internal/config/errors.go index f8ecf87e..fbb3ddd1 100644 --- a/internal/config/errors.go +++ b/internal/config/errors.go @@ -2,6 +2,8 @@ package config import "errors" +var ErrAzurePricingNotReady = errors.New("azure pricing migration not ready") + // ErrNotFound is returned when a requested config-store row does not exist. var ErrNotFound = errors.New("not found") diff --git a/internal/config/store_postgres_azure_pricing_test.go b/internal/config/store_postgres_azure_pricing_test.go new file mode 100644 index 00000000..03515bef --- /dev/null +++ b/internal/config/store_postgres_azure_pricing_test.go @@ -0,0 +1,67 @@ +package config + +import ( + "context" + "errors" + "testing" + + "github.com/pashagolub/pgxmock/v4" + "github.com/stretchr/testify/require" +) + +func TestListStoredRecommendationsAzurePricingReadiness(t *testing.T) { + queryErr := errors.New("migration query failed") + for _, tc := range []struct { + name string + version int + dirty, missing, ready bool + err error + }{ + {name: "old", version: 105}, + {name: "dirty", version: 106, dirty: true}, + {name: "missing", missing: true}, + {name: "query error", err: queryErr}, + {name: "ready", version: 106, ready: true}, + {name: "later", version: 107, ready: true}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + pool := newMock(t) + defer pool.Close() + query := pool.ExpectQuery("SELECT version, dirty FROM schema_migrations") + if tc.err != nil { + query.WillReturnError(tc.err) + } else { + rows := pgxmock.NewRows([]string{"version", "dirty"}) + if !tc.missing { + rows.AddRow(tc.version, tc.dirty) + } + query.WillReturnRows(rows) + } + if tc.ready { + pool.ExpectQuery("SELECT payload FROM recommendations WHERE provider = \\$1").WithArgs("azure").WillReturnRows(pgxmock.NewRows([]string{"payload"})) + } + _, err := storeWith(pool).ListStoredRecommendations(context.Background(), RecommendationFilter{Provider: "azure", RequireAzurePricingMigration: true}) + switch { + case tc.ready: + require.NoError(t, err) + case tc.err != nil: + require.ErrorIs(t, err, queryErr) + require.NotErrorIs(t, err, ErrAzurePricingNotReady) + default: + require.ErrorIs(t, err, ErrAzurePricingNotReady) + } + require.NoError(t, pool.ExpectationsWereMet()) + }) + } +} + +func TestListStoredRecommendationsDisplayDoesNotRequireAzureMigration(t *testing.T) { + t.Parallel() + pool := newMock(t) + defer pool.Close() + pool.ExpectQuery("SELECT payload FROM recommendations WHERE provider = \\$1").WithArgs("azure").WillReturnRows(pgxmock.NewRows([]string{"payload"})) + _, err := storeWith(pool).ListStoredRecommendations(context.Background(), RecommendationFilter{Provider: "azure"}) + require.NoError(t, err) + require.NoError(t, pool.ExpectationsWereMet()) +} diff --git a/internal/config/store_postgres_recommendations.go b/internal/config/store_postgres_recommendations.go index aafb4eed..1e3ce00d 100644 --- a/internal/config/store_postgres_recommendations.go +++ b/internal/config/store_postgres_recommendations.go @@ -3,6 +3,7 @@ package config import ( "context" "encoding/json" + "errors" "fmt" "math" "strings" @@ -19,6 +20,8 @@ import ( // gives 4500 params which stays conservative and leaves headroom). const recommendationsBatchSize = 500 +const azurePricingMigrationVersion = 106 + // ReplaceRecommendations wipes the recommendations table and reinserts the // full snapshot inside a single transaction. Used for a force-full-resync // path; the steady-state write path (see commit 6) is UpsertRecommendations. @@ -364,12 +367,34 @@ func recOnDemandBaseline(rec *RecommendationRecord) (float64, bool) { return *rec.MonthlyCost + rec.Savings + amortized, true } +func (s *PostgresStore) checkAzurePricingMigration(ctx context.Context, required bool) error { + if !required { + return nil + } + var version int + var dirty bool + err := s.db.QueryRow(ctx, `SELECT version, dirty FROM schema_migrations`).Scan(&version, &dirty) + if errors.Is(err, pgx.ErrNoRows) { + return ErrAzurePricingNotReady + } + if err != nil { + return fmt.Errorf("check Azure pricing migration: %w", err) + } + if version < azurePricingMigrationVersion || dirty { + return ErrAzurePricingNotReady + } + return nil +} + // ListStoredRecommendations reads recommendations matching the filter. // SQL-pushed conditions (Provider, Service, Region, AccountIDs, // MinSavingsUSD) are applied in SQL so Postgres prunes the rows; the // MinSavingsPct filter is applied in-process because the on-demand // baseline lives inside the JSONB payload (not a native column). func (s *PostgresStore) ListStoredRecommendations(ctx context.Context, filter RecommendationFilter) ([]RecommendationRecord, error) { + if err := s.checkAzurePricingMigration(ctx, filter.RequireAzurePricingMigration); err != nil { + return nil, err + } whereClause, args := buildRecommendationFilter(filter) rows, err := s.db.Query(ctx, `SELECT payload FROM recommendations`+whereClause, args...) if err != nil { diff --git a/internal/config/store_postgres_recommendations_test.go b/internal/config/store_postgres_recommendations_test.go index 833fd5e3..baf1ef32 100644 --- a/internal/config/store_postgres_recommendations_test.go +++ b/internal/config/store_postgres_recommendations_test.go @@ -509,3 +509,23 @@ func TestPostgresStore_SetRecommendationsCollectionError_DoesNotClearOwnerMarker } func float64Ptr(f float64) *float64 { return &f } + +func TestPostgresStore_UpsertRecommendations_ReplacesAzureWrongUnits(t *testing.T) { + ctx := context.Background() + store, cleanup := setupRecommendationsStore(ctx, t) + defer cleanup() + rec := config.RecommendationRecord{ID: "old-units", Provider: "azure", Service: "compute", Region: "westeurope", ResourceType: "Standard_D2s_v3", Count: 1, Term: 3, Payment: "all-upfront", UpfrontCost: 70, MonthlyCost: float64Ptr(10), Savings: 50} + collects := []config.SuccessfulCollect{{Provider: "azure"}} + now := time.Now().UTC() + require.NoError(t, store.UpsertRecommendations(ctx, now, []config.RecommendationRecord{rec}, collects)) + rec.ID = "corrected" + rec.UpfrontCost = 3600 + rec.MonthlyCost = nil + require.NoError(t, store.UpsertRecommendations(ctx, now.Add(time.Minute), []config.RecommendationRecord{rec}, collects)) + got, err := store.ListStoredRecommendations(ctx, config.RecommendationFilter{Provider: "azure"}) + require.NoError(t, err) + require.Len(t, got, 1) + assert.Equal(t, "corrected", got[0].ID) + assert.Equal(t, 3600.0, got[0].UpfrontCost) + assert.Nil(t, got[0].MonthlyCost) +} diff --git a/internal/config/types.go b/internal/config/types.go index a148d61f..eb5689ae 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -659,13 +659,14 @@ type PurchaseSuppression struct { // query rather than in SQL (avoids a computed column). These two filters are // independent and can be combined. type RecommendationFilter struct { - Provider string // "aws" / "azure" / "gcp" / "" (all) - Service string // "" = all services - Region string // "" = all regions - AccountIDs []string // nil/empty = all accounts - MinSavingsUSD float64 // 0 = no floor on monthly savings dollar amount - MinSavingsPct float64 // 0 = no floor on savings percentage (0–100 scale) - ID string // "" = all ids; non-empty = exact match on the id column + RequireAzurePricingMigration bool + Provider string // "aws" / "azure" / "gcp" / "" (all) + Service string // "" = all services + Region string // "" = all regions + AccountIDs []string // nil/empty = all accounts + MinSavingsUSD float64 // 0 = no floor on monthly savings dollar amount + MinSavingsPct float64 // 0 = no floor on savings percentage (0–100 scale) + ID string // "" = all ids; non-empty = exact match on the id column } // PurchasePlanFilter parameterises ListPurchasePlans. Zero-value means "no diff --git a/internal/database/postgres/migrations/000106_invalidate_azure_recs_lookback_units.down.sql b/internal/database/postgres/migrations/000106_invalidate_azure_recs_lookback_units.down.sql new file mode 100644 index 00000000..8d5f3561 --- /dev/null +++ b/internal/database/postgres/migrations/000106_invalidate_azure_recs_lookback_units.down.sql @@ -0,0 +1 @@ +-- Wrong-unit prices cannot be restored; a successful collection supplies corrected values. diff --git a/internal/database/postgres/migrations/000106_invalidate_azure_recs_lookback_units.up.sql b/internal/database/postgres/migrations/000106_invalidate_azure_recs_lookback_units.up.sql new file mode 100644 index 00000000..a340b61d --- /dev/null +++ b/internal/database/postgres/migrations/000106_invalidate_azure_recs_lookback_units.up.sql @@ -0,0 +1,6 @@ +-- Quiesce old Azure collectors and deploy the fixed collector with this migration. +-- Rows collected before migration, even with the fix, need a successful recollection. +UPDATE recommendations + SET payload = jsonb_set(jsonb_set(payload, '{upfront_cost}', 'null'::jsonb), + '{monthly_cost}', 'null'::jsonb) + WHERE payload->>'provider' = 'azure'; diff --git a/internal/database/postgres/migrations/000106_invalidate_azure_recs_lookback_units_test.go b/internal/database/postgres/migrations/000106_invalidate_azure_recs_lookback_units_test.go new file mode 100644 index 00000000..50dd53bd --- /dev/null +++ b/internal/database/postgres/migrations/000106_invalidate_azure_recs_lookback_units_test.go @@ -0,0 +1,66 @@ +//go:build integration + +package migrations_test + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestMigration_InvalidateAzureRecsLookbackUnits(t *testing.T) { + ctx := context.Background() + path := getMigrationsPath() + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 105)) + fixtures := []string{ + `{"provider":"azure","payment":"upfront","id":"azure-upfront","term":3,"count":1,"upfront_cost":38.5,"monthly_cost":12.1,"savings":40,"resource_type":"Standard_D4s_v5"}`, + `{"provider":"azure","payment":"monthly","id":"azure-monthly","term":3,"count":1,"upfront_cost":4.2,"monthly_cost":55,"savings":40,"resource_type":"Standard_D4s_v5"}`, + `{"provider":"aws","payment":"upfront","id":"aws-upfront","term":3,"count":1,"upfront_cost":500,"monthly_cost":0,"savings":25,"resource_type":"Standard_D4s_v5","on_demand_cost":90}`, + } + expected := make([]map[string]any, len(fixtures)) + for i, payload := range fixtures { + require.NoError(t, json.Unmarshal([]byte(payload), &expected[i])) + _, err = pool.Exec(ctx, `INSERT INTO recommendations + (collected_at,provider,service,region,resource_type,engine,payload,upfront_cost,monthly_savings,term,payment_option) + VALUES (NOW(),$1,'compute','westeurope','Standard_D4s_v5','',$2::jsonb,0,0,3,$3)`, + expected[i]["provider"], payload, expected[i]["payment"]) + require.NoError(t, err) + if expected[i]["provider"] == "azure" { + expected[i]["upfront_cost"] = nil + expected[i]["monthly_cost"] = nil + } + } + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 106)) + sql, err := os.ReadFile(filepath.Join(path, "000106_invalidate_azure_recs_lookback_units.up.sql")) + require.NoError(t, err) + for pass := 0; pass < 3; pass++ { + if pass == 1 { + _, err = pool.Exec(ctx, string(sql)) + require.NoError(t, err) + } + if pass == 2 { + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 105)) + } + for _, payload := range expected { + var got string + require.NoError(t, pool.QueryRow(ctx, `SELECT payload::text FROM recommendations WHERE provider=$1 AND payment_option=$2`, payload["provider"], payload["payment"]).Scan(&got)) + want, err := json.Marshal(payload) + require.NoError(t, err) + assert.JSONEq(t, string(want), got) + } + var count int + require.NoError(t, pool.QueryRow(ctx, `SELECT COUNT(*) FROM recommendations`).Scan(&count)) + assert.Equal(t, 3, count) + } +} diff --git a/internal/purchase/armed_redrive_test.go b/internal/purchase/armed_redrive_test.go index 12d9af2f..7c6fb635 100644 --- a/internal/purchase/armed_redrive_test.go +++ b/internal/purchase/armed_redrive_test.go @@ -202,7 +202,9 @@ func TestFireScheduledDelayedPurchasesRefusesArmedAzureSavingsPlanRetry(t *testi // above. func TestFirstAzureSavingsPlanPurchaseStillExecutes(t *testing.T) { exec := armedExecution("azure", "savingsplans", 0, "approved") + exec.Recommendations[0].ID = "current-azure" mgr, store, rec := armedHarness(t, common.ServiceSavingsPlansAll) + store.On("ListStoredRecommendations", mock.Anything, config.RecommendationFilter{Provider: "azure", RequireAzurePricingMigration: true}).Return(exec.Recommendations, nil).Once() store.On("GetExecutionByID", mock.Anything, exec.ExecutionID).Return(exec, nil).Maybe() expectClaim(store, exec, []string{"approved", "pending", "notified"}, "running") diff --git a/internal/purchase/azure_pricing.go b/internal/purchase/azure_pricing.go new file mode 100644 index 00000000..b328ff35 --- /dev/null +++ b/internal/purchase/azure_pricing.go @@ -0,0 +1,89 @@ +package purchase + +import ( + "context" + "errors" + "fmt" + "math" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +var ErrStaleAzurePricing = errors.New("stale Azure recommendation pricing; refresh recommendations and submit a new purchase") + +func (m *Manager) loadCurrentAzurePricing(ctx context.Context) ([]config.RecommendationRecord, error) { + rows, err := m.config.ListStoredRecommendations(ctx, config.RecommendationFilter{Provider: "azure", RequireAzurePricingMigration: true}) + if err != nil { + if errors.Is(err, config.ErrAzurePricingNotReady) { + return nil, fmt.Errorf("%w: %w", ErrStaleAzurePricing, err) + } + return nil, fmt.Errorf("load current Azure recommendation pricing: %w", err) + } + return rows, nil +} + +func (m *Manager) staleAzurePricingRefusal(ctx context.Context, exec *config.PurchaseExecution) error { + var azure []config.RecommendationRecord + for i := range exec.Recommendations { + rec := &exec.Recommendations[i] + if rec.Provider == "azure" { + azure = append(azure, *rec) + } + } + if len(azure) == 0 { + return nil + } + rows, err := m.loadCurrentAzurePricing(ctx) + if err != nil { + return err + } + byID := make(map[string]config.RecommendationRecord, len(rows)) + for i := range rows { + byID[rows[i].ID] = rows[i] + } + for i := range azure { + rec := &azure[i] + stored, ok := byID[rec.ID] + if rec.ID == "" || !ok || !azurePricingMatches(*rec, stored) { + return fmt.Errorf("%w: Azure recommendation %q no longer has the approved price", ErrStaleAzurePricing, rec.ID) + } + } + return nil +} + +func azurePricingMatches(rec, stored config.RecommendationRecord) bool { + if rec.Count <= 0 || stored.Count <= 0 { + return false + } + total := stored.UpfrontCost + if stored.MonthlyCost != nil { + total += *stored.MonthlyCost * float64(stored.Term) * 12 + } + if total <= 0 || !azureCostEqual(total, total, 1) { + return false + } + ratio := float64(rec.Count) / float64(stored.Count) + if !azureCostEqual(rec.UpfrontCost, stored.UpfrontCost, ratio) { + return false + } + if rec.MonthlyCost == nil || stored.MonthlyCost == nil { + return rec.MonthlyCost == nil && stored.MonthlyCost == nil + } + return azureCostEqual(*rec.MonthlyCost, *stored.MonthlyCost, ratio) +} + +func azureCostEqual(a, b, ratio float64) bool { + if a < 0 || b < 0 || math.IsNaN(a) || math.IsNaN(b) || math.IsInf(a, 0) || math.IsInf(b, 0) { + return false + } + b *= ratio + if math.IsInf(b, 0) { + return false + } + // Nonnegative IEEE-754 bit patterns are ordered; allow only rounding drift. + x, y := math.Float64bits(math.Abs(a)), math.Float64bits(math.Abs(b)) + if x > y { + return x-y <= 8 + } + return y-x <= 8 +} diff --git a/internal/purchase/azure_pricing_integration_test.go b/internal/purchase/azure_pricing_integration_test.go new file mode 100644 index 00000000..75a7d87c --- /dev/null +++ b/internal/purchase/azure_pricing_integration_test.go @@ -0,0 +1,95 @@ +//go:build integration + +package purchase + +import ( + "context" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/require" +) + +type azurePricingPostgresStore struct { + *MockConfigStore + pricing *config.PostgresStore +} + +func (s *azurePricingPostgresStore) ListStoredRecommendations(ctx context.Context, filter config.RecommendationFilter) ([]config.RecommendationRecord, error) { + return s.pricing.ListStoredRecommendations(ctx, filter) +} + +func TestApproveAzurePricingRequiresMigrationAndRecollection(t *testing.T) { + ctx := context.Background() + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + t.Cleanup(func() { + if cleanupErr := container.Cleanup(ctx); cleanupErr != nil { + t.Logf("container cleanup: %v", cleanupErr) + } + }) + pool := container.DB.Pool() + require.NoError(t, migrations.MigrateToVersion(ctx, pool, rampMigrationsPath(), 105)) + pricing := config.NewPostgresStore(container.DB) + rec := armedExecution("azure", "compute", 0, "pending").Recommendations[0] + rec.ID, rec.UpfrontCost = "azure-old-units", 70 + rec.Payment = "upfront" + collects := []config.SuccessfulCollect{{Provider: "azure"}} + require.NoError(t, pricing.UpsertRecommendations(ctx, time.Now(), []config.RecommendationRecord{rec}, collects)) + + approve := func(t *testing.T, expected error, purchases int) { + t.Helper() + exec := armedExecution("azure", "compute", 0, "pending") + exec.PlanID = "" + exec.Recommendations = []config.RecommendationRecord{rec} + mgr, store, recorder := armedHarness(t, common.ServiceCompute) + mgr.config = &azurePricingPostgresStore{MockConfigStore: store, pricing: pricing} + expectClaim(store, exec, []string{"pending", "notified"}, "approved") + var saved *config.PurchaseExecution + store.SavePurchaseExecutionFn = func(_ context.Context, e *config.PurchaseExecution) error { + copy := *e + saved = © + return nil + } + _, _, approveErr := mgr.ApproveAndExecute(ctx, exec.ExecutionID, "operator@example.com", nil) + require.Equal(t, purchases, recorder.count()) + require.NotNil(t, saved) + if purchases == 1 { + require.NoError(t, approveErr) + require.Equal(t, "completed", saved.Status) + } else { + require.Error(t, approveErr) + require.Equal(t, "failed", saved.Status) + if expected != nil { + require.ErrorIs(t, approveErr, expected) + } else { + require.NotErrorIs(t, approveErr, ErrStaleAzurePricing) + } + } + store.AssertExpectations(t) + } + + t.Run("identical old positive prices at 105", func(t *testing.T) { approve(t, ErrStaleAzurePricing, 0) }) + _, err = pool.Exec(ctx, `UPDATE schema_migrations SET version = 106, dirty = true`) + require.NoError(t, err) + t.Run("dirty 106", func(t *testing.T) { approve(t, config.ErrAzurePricingNotReady, 0) }) + _, err = pool.Exec(ctx, `TRUNCATE schema_migrations`) + require.NoError(t, err) + t.Run("missing migration row", func(t *testing.T) { approve(t, config.ErrAzurePricingNotReady, 0) }) + _, err = pool.Exec(ctx, `ALTER TABLE schema_migrations RENAME TO azure_pricing_fixture_migrations`) + require.NoError(t, err) + t.Run("migration query failure", func(t *testing.T) { approve(t, nil, 0) }) + _, err = pool.Exec(ctx, `ALTER TABLE azure_pricing_fixture_migrations RENAME TO schema_migrations`) + require.NoError(t, err) + _, err = pool.Exec(ctx, `INSERT INTO schema_migrations (version, dirty) VALUES (105, false)`) + require.NoError(t, err) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, rampMigrationsPath(), 106)) + t.Run("106 invalidated old prices", func(t *testing.T) { approve(t, ErrStaleAzurePricing, 0) }) + rec.UpfrontCost = 700 + require.NoError(t, pricing.UpsertRecommendations(ctx, time.Now().Add(time.Minute), []config.RecommendationRecord{rec}, collects)) + t.Run("recollected prices purchase once", func(t *testing.T) { approve(t, nil, 1) }) +} diff --git a/internal/purchase/azure_pricing_test.go b/internal/purchase/azure_pricing_test.go new file mode 100644 index 00000000..dc016fbe --- /dev/null +++ b/internal/purchase/azure_pricing_test.go @@ -0,0 +1,162 @@ +package purchase + +import ( + "context" + "errors" + "fmt" + "math" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestAzurePricingRefusal(t *testing.T) { + base := config.RecommendationRecord{ID: "azure-current", Provider: "azure", Service: "compute", Region: "westeurope", ResourceType: "Standard_D2s_v3", Count: 2, Term: 3, UpfrontCost: 200, Selected: true} + cases := []struct { + name string + change func(*config.RecommendationRecord, *config.RecommendationRecord) + missing bool + storeErr error + }{ + {"invalidated", func(_, r *config.RecommendationRecord) { r.UpfrontCost = 0 }, false, nil}, + {"repriced", func(_, r *config.RecommendationRecord) { r.UpfrontCost = 201 }, false, nil}, + {"missing", nil, true, nil}, + {"zero count", func(_, r *config.RecommendationRecord) { r.Count = 0 }, false, nil}, + {"negative price", func(_, r *config.RecommendationRecord) { r.UpfrontCost = -1 }, false, nil}, + {"nan price", func(_, r *config.RecommendationRecord) { r.UpfrontCost = math.NaN() }, false, nil}, + {"infinite price", func(_, r *config.RecommendationRecord) { r.UpfrontCost = math.Inf(1) }, false, nil}, + {"monthly nil differs from zero", func(_, r *config.RecommendationRecord) { r.MonthlyCost = new(float64) }, false, nil}, + {"persisted negative price", func(r, _ *config.RecommendationRecord) { r.UpfrontCost = -1 }, false, nil}, + {"persisted nan price", func(r, _ *config.RecommendationRecord) { r.UpfrontCost = math.NaN() }, false, nil}, + {"persisted infinite price", func(r, _ *config.RecommendationRecord) { r.UpfrontCost = math.Inf(1) }, false, nil}, + {"persisted zero count", func(r, _ *config.RecommendationRecord) { r.Count = 0 }, false, nil}, + {"persisted negative count", func(r, _ *config.RecommendationRecord) { r.Count = -1 }, false, nil}, + {"negative subnormal upfront", func(req, stored *config.RecommendationRecord) { + req.Count, req.UpfrontCost, req.MonthlyCost, stored.UpfrontCost, stored.MonthlyCost = 1, 0, new(50.0), -math.SmallestNonzeroFloat64, new(100.0) + }, false, nil}, + {"negative subnormal monthly", func(req, stored *config.RecommendationRecord) { + req.Count, req.UpfrontCost, req.MonthlyCost, stored.MonthlyCost = 1, 100, new(0.0), new(-math.SmallestNonzeroFloat64) + }, false, nil}, + {"store error", nil, false, errors.New("database unavailable")}, + {"migration not ready", nil, false, fmt.Errorf("readiness: %w", config.ErrAzurePricingNotReady)}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + exec := armedExecution("azure", "compute", 0, "approved") + exec.Recommendations = []config.RecommendationRecord{base} + mgr, store, recorder := armedHarness(t, common.ServiceCompute) + stored := base + if tc.change != nil { + tc.change(&exec.Recommendations[0], &stored) + } + rows := []config.RecommendationRecord{stored} + if tc.missing { + rows = nil + } + store.On("ListStoredRecommendations", mock.Anything, config.RecommendationFilter{Provider: "azure", RequireAzurePricingMigration: true}).Return(rows, tc.storeErr).Maybe() + err := mgr.executeAndFinalize(context.Background(), exec) + assert.Zero(t, recorder.count()) + assert.Equal(t, "failed", exec.Status) + if tc.storeErr != nil { + assert.ErrorIs(t, err, tc.storeErr) + if errors.Is(tc.storeErr, config.ErrAzurePricingNotReady) { + assert.ErrorIs(t, err, ErrStaleAzurePricing) + } else { + assert.NotErrorIs(t, err, ErrStaleAzurePricing) + } + } else { + assert.ErrorIs(t, err, ErrStaleAzurePricing) + } + }) + } +} + +func TestAzurePricingRefusalEntryPoints(t *testing.T) { + for _, entry := range []string{"approve", "direct", "empty ID", "audit save failure"} { + t.Run(entry, func(t *testing.T) { + exec := armedExecution("azure", "compute", 0, "pending") + exec.Recommendations[0].ID = "azure-current" + if entry == "empty ID" { + exec.Recommendations[0].ID = "" + } + mgr, store, recorder := armedHarness(t, common.ServiceCompute) + store.On("ListStoredRecommendations", mock.Anything, config.RecommendationFilter{Provider: "azure", RequireAzurePricingMigration: true}).Return([]config.RecommendationRecord{}, nil).Maybe() + store.On("GetExecutionByID", mock.Anything, exec.ExecutionID).Return(exec, nil).Maybe() + var saved *config.PurchaseExecution + store.SavePurchaseExecutionFn = func(_ context.Context, e *config.PurchaseExecution) error { + copy := *e + saved = © + if entry == "audit save failure" { + return errors.New("save failed") + } + return nil + } + expectClaim(store, exec, []string{"pending", "notified"}, "approved") + var err error + if entry == "approve" { + _, _, err = mgr.ApproveAndExecute(context.Background(), exec.ExecutionID, "operator@example.com", nil) + } else { + _, _, err = mgr.DirectExecute(context.Background(), exec.ExecutionID, "operator@example.com", nil) + } + assert.Zero(t, recorder.count()) + require.Error(t, err) + require.NotNil(t, saved) + assert.Equal(t, "failed", saved.Status) + if entry == "audit save failure" { + assert.ErrorIs(t, err, config.ErrAuditLoss) + } + }) + } +} + +func TestAzurePricingUnchangedStillPurchases(t *testing.T) { + for _, variant := range []struct { + name, provider string + upfront float64 + monthly *float64 + }{ + {"azure", "azure", 200, nil}, {"aws", "aws", 200, nil}, {"inexact Azure upfront", "azure", 39.8, nil}, + {"monthly", "azure", 0, new(200.0)}, {"upfront zero monthly", "azure", 200, new(0.0)}, + } { + for _, count := range []int{1, 2, 3, 4, 5} { + t.Run(fmt.Sprintf("%s/collected-%d", variant.name, count), func(t *testing.T) { + exec := armedExecution(variant.provider, "compute", 0, "approved") + exec.PlanID = "" + exec.Recommendations[0].ID = "current" + exec.Recommendations[0].Count = 2 + exec.Recommendations[0].UpfrontCost = variant.upfront + exec.Recommendations[0].MonthlyCost = variant.monthly + mgr, store, recorder := armedHarness(t, common.ServiceCompute) + stored := exec.Recommendations[0] + stored.Count = count + stored.UpfrontCost = variant.upfront * float64(count) / 2 + if variant.monthly != nil { + stored.MonthlyCost = new(*variant.monthly * float64(count) / 2) + } + if variant.provider == "azure" { + store.On("ListStoredRecommendations", mock.Anything, config.RecommendationFilter{Provider: "azure", RequireAzurePricingMigration: true}).Return([]config.RecommendationRecord{stored}, nil).Once() + } + require.NoError(t, mgr.executeAndFinalize(context.Background(), exec)) + assert.Equal(t, 1, recorder.count()) + if variant.provider == "aws" { + store.AssertNotCalled(t, "ListStoredRecommendations", mock.Anything, mock.Anything) + } + store.AssertExpectations(t) + }) + } + } +} + +func TestAzureCostEqualRoundingBound(t *testing.T) { + within := 100.0 + for i := 0; i < 8; i++ { + within = math.Nextafter(within, math.Inf(1)) + } + beyond := math.Nextafter(within, math.Inf(1)) + assert.True(t, azureCostEqual(100, within, 1)) + assert.False(t, azureCostEqual(100, beyond, 1)) +} diff --git a/internal/purchase/manager.go b/internal/purchase/manager.go index fe76e60b..3d1630e4 100644 --- a/internal/purchase/manager.go +++ b/internal/purchase/manager.go @@ -221,6 +221,9 @@ func (m *Manager) executeAndFinalize(ctx context.Context, exec *config.PurchaseE // Last line of defense before money moves (issue #1718). Every executor // entry point funnels through here, so one check covers all of them. execErr := armedRedriveRefusal(exec) + if execErr == nil { + execErr = m.staleAzurePricingRefusal(ctx, exec) + } if execErr == nil { execErr = m.executePurchase(ctx, exec) } diff --git a/internal/purchase/manager_test.go b/internal/purchase/manager_test.go index 947dc2b6..9ccd7b76 100644 --- a/internal/purchase/manager_test.go +++ b/internal/purchase/manager_test.go @@ -688,9 +688,10 @@ func TestManager_RecoverStrandedApprovals_AzureReservationRedrives(t *testing.T) Status: "approved", StepNumber: 1, Recommendations: []config.RecommendationRecord{ - {Provider: "azure", Service: "compute", ResourceType: "Standard_D4s_v3", Region: "eastus", Count: 1, UpfrontCost: 300.0, Selected: true, Purchased: false}, + {ID: "current-azure", Provider: "azure", Service: "compute", ResourceType: "Standard_D4s_v3", Region: "eastus", Count: 1, UpfrontCost: 300.0, Selected: true, Purchased: false}, }, } + mockStore.On("ListStoredRecommendations", mock.Anything, config.RecommendationFilter{Provider: "azure", RequireAzurePricingMigration: true}).Return(stranded.Recommendations, nil).Once() runningRow := stranded runningRow.Status = "running"