diff --git a/internal/analytics/collector.go b/internal/analytics/collector.go index 859ec423..6f9f9bea 100644 --- a/internal/analytics/collector.go +++ b/internal/analytics/collector.go @@ -158,7 +158,7 @@ func aggregatePurchases(ctx context.Context, purchases []config.PurchaseHistoryR } activePurchases++ - commitmentType := commitmentTypeFor(p.Service) + commitmentType := commitmentTypeFor(p.Provider, p.Service) key := aggKey(p, commitmentType) agg := serviceMap[key] @@ -198,15 +198,6 @@ func aggregatePurchases(ctx context.Context, purchases []config.PurchaseHistoryR return serviceMap, activePurchases, skippedBadTerm, nil } -// commitmentTypeFor maps a service to its commitment_type. SavingsPlans is the -// only Savings Plan service today; everything else is a Reserved Instance. -func commitmentTypeFor(service string) string { - if service == "SavingsPlans" { - return "SavingsPlan" - } - return "RI" -} - // buildSnapshots converts the aggregation map into snapshot rows. total_usage is // nil when no contributing row carried a recurring cost; coverage_percentage is // nil because purchase_history carries no on-demand baseline to derive it from diff --git a/internal/analytics/collector_test.go b/internal/analytics/collector_test.go index da1d7742..199446d1 100644 --- a/internal/analytics/collector_test.go +++ b/internal/analytics/collector_test.go @@ -653,7 +653,7 @@ func TestCollectorCollect(t *testing.T) { assert.Equal(t, "123456789012", s.AccountID) assert.Equal(t, "aws", s.Provider) assert.Equal(t, "rds", s.Service) - assert.Equal(t, "RI", s.CommitmentType) + assert.Equal(t, string(common.CommitmentReservedInstance), s.CommitmentType) assert.Greater(t, s.TotalSavings, 0.0) require.NotNil(t, s.TotalCommitment) assert.Greater(t, *s.TotalCommitment, 0.0) @@ -689,16 +689,35 @@ func TestCollectorCollect(t *testing.T) { assert.Len(t, store.savedSnapshots, 3) }) - t.Run("sets SavingsPlan commitment type for SavingsPlans service", func(t *testing.T) { + // #339 (A06-019): the type comes from (provider, service). Purchase history + // stores Savings Plans as "savingsplans" or a per-plan-type slug, never the + // "SavingsPlans" the old mapper matched, and GCP purchases are CUDs. + t.Run("derives commitment type from provider and service", func(t *testing.T) { store := &mockAnalyticsStore{} cfgStore := &mockConfigStore{ getActivePurchaseHistoryFunc: func(ctx context.Context, asOf time.Time, accountIDs []string, externalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, error) { - return []config.PurchaseHistoryRecord{activeRecord("aws", "SavingsPlans", "us-east-1", 1, 500, 2000)}, nil + return []config.PurchaseHistoryRecord{ + activeRecord("aws", "savingsplans", "us-east-1", 1, 500, 2000), + activeRecord("aws", "savings-plans-compute", "us-east-1", 1, 500, 2000), + activeRecord("gcp", "compute", "us-central1", 1, 300, 0), + activeRecord("azure", "compute", "eastus", 1, 200, 900), + activeRecord("aws", "ec2", "us-east-1", 1, 100, 400), + }, nil }, } require.NoError(t, newTestCollector(t, store, cfgStore).Collect(context.Background())) - require.Len(t, store.savedSnapshots, 1) - assert.Equal(t, "SavingsPlan", store.savedSnapshots[0].CommitmentType) + got := make(map[string]string, len(store.savedSnapshots)) + for _, s := range store.savedSnapshots { + got[s.Provider+"/"+s.Service] = s.CommitmentType + require.NoError(t, validateCommitmentType(s.CommitmentType), "the store CHECK must admit what the collector emits") + } + assert.Equal(t, map[string]string{ + "aws/savingsplans": string(common.CommitmentSavingsPlan), + "aws/savings-plans-compute": string(common.CommitmentSavingsPlan), + "gcp/compute": string(common.CommitmentCUD), + "azure/compute": string(common.CommitmentReservedInstance), + "aws/ec2": string(common.CommitmentReservedInstance), + }, got) }) t.Run("returns error when BulkInsertSnapshots fails", func(t *testing.T) { diff --git a/internal/analytics/commitment_type.go b/internal/analytics/commitment_type.go new file mode 100644 index 00000000..02e7a7a6 --- /dev/null +++ b/internal/analytics/commitment_type.go @@ -0,0 +1,62 @@ +package analytics + +import ( + "fmt" + "strings" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +// Legacy commitment_type literals written before #339. The savings_snapshots +// CHECK still admits them (migration 000145) so a not-yet-replaced replica can +// keep writing during a rolling deploy; the collector never emits them. +const ( + legacyCommitmentRI = "RI" + legacyCommitmentSavingsPlan = "SavingsPlan" +) + +// allowedCommitmentTypes mirrors the savings_snapshots valid_commitment_type +// CHECK constraint (migration 000145): the pkg/common vocabulary plus the two +// legacy literals. +var allowedCommitmentTypes = map[string]bool{ + string(common.CommitmentReservedInstance): true, + string(common.CommitmentSavingsPlan): true, + string(common.CommitmentCUD): true, + string(common.CommitmentReservedCapacity): true, + legacyCommitmentRI: true, + legacyCommitmentSavingsPlan: true, +} + +// commitmentTypeFor maps a purchase's (provider, service) to its +// commitment_type in the pkg/common vocabulary. GCP commitments are +// committed-use discounts, any Savings Plans service slug is a savings plan, +// and everything else (AWS and Azure reservations) is a reserved instance. +func commitmentTypeFor(provider, service string) string { + switch { + case strings.EqualFold(provider, string(common.ProviderGCP)): + return string(common.CommitmentCUD) + case isSavingsPlanService(service): + return string(common.CommitmentSavingsPlan) + default: + return string(common.CommitmentReservedInstance) + } +} + +// isSavingsPlanService recognizes every Savings Plans service slug persisted in +// purchase_history.service: the umbrella "savingsplans", the four per-plan-type +// slugs, and the pre-#85 "savings-plans" spelling. +func isSavingsPlanService(service string) bool { + s := strings.ToLower(service) + return common.IsSavingsPlan(common.ServiceType(s)) || s == "savings-plans" +} + +// validateCommitmentType rejects a commitment_type that the savings_snapshots +// table CHECK constraint would reject. Extracted so the guard is unit-testable +// directly (the COPY path acquires a real pooled connection that pgxmock can't +// stand in for, so an end-to-end test can only prove the acquire-failure path). +func validateCommitmentType(commitmentType string) error { + if !allowedCommitmentTypes[commitmentType] { + return fmt.Errorf("invalid commitment_type %q", commitmentType) + } + return nil +} diff --git a/internal/analytics/commitment_type_db_test.go b/internal/analytics/commitment_type_db_test.go new file mode 100644 index 00000000..1c58d87f --- /dev/null +++ b/internal/analytics/commitment_type_db_test.go @@ -0,0 +1,63 @@ +//go:build integration + +package analytics + +import ( + "context" + "path/filepath" + "runtime" + "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/assert" + "github.com/stretchr/testify/require" +) + +// TestCollector_GCPCommittedUse_DB is the #339 (A06-019) end-to-end check: the +// collector writes active GCP, AWS Savings Plans and AWS RI purchases through +// the real Postgres store, and each snapshot lands with its pkg/common +// commitment_type. Before the fix all three were stored as "RI". +func TestCollector_GCPCommittedUse_DB(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + + container := testhelpers.RequirePostgresContainer(ctx, t) + t.Cleanup(func() { require.NoError(t, container.Cleanup(context.Background())) }) + _, file, _, _ := runtime.Caller(0) + migrationsPath := filepath.Join(filepath.Dir(file), "..", "database", "postgres", "migrations") + require.NoError(t, migrations.RunMigrations(ctx, container.DB.Pool(), migrationsPath, "", "")) + + store := NewPostgresAnalyticsStore(container.DB) + cfgStore := &mockConfigStore{ + getActivePurchaseHistoryFunc: func(context.Context, time.Time, []string, map[string][]string) ([]config.PurchaseHistoryRecord, error) { + return []config.PurchaseHistoryRecord{ + activeRecord("gcp", "compute", "us-central1", 1, 300, 1200), + activeRecord("aws", "savingsplans", "us-east-1", 1, 500, 2000), + activeRecord("aws", "ec2", "us-east-1", 1, 100, 400), + }, nil + }, + } + collector, err := NewCollector(CollectorConfig{AnalyticsStore: store}, cfgStore) + require.NoError(t, err) + require.NoError(t, collector.Collect(ctx)) + + rows, err := container.DB.Pool().Query(ctx, `SELECT provider, service, commitment_type FROM savings_snapshots`) + require.NoError(t, err) + defer rows.Close() + got := map[string]string{} + for rows.Next() { + var provider, service, commitmentType string + require.NoError(t, rows.Scan(&provider, &service, &commitmentType)) + got[provider+"/"+service] = commitmentType + } + require.NoError(t, rows.Err()) + assert.Equal(t, map[string]string{ + "gcp/compute": string(common.CommitmentCUD), + "aws/savingsplans": string(common.CommitmentSavingsPlan), + "aws/ec2": string(common.CommitmentReservedInstance), + }, got) +} diff --git a/internal/analytics/postgres_analytics.go b/internal/analytics/postgres_analytics.go index 20088ca8..bacecca0 100644 --- a/internal/analytics/postgres_analytics.go +++ b/internal/analytics/postgres_analytics.go @@ -140,17 +140,6 @@ func (s *PostgresAnalyticsStore) SaveSnapshot(ctx context.Context, snapshot *Sav return nil } -// validateCommitmentType rejects a commitment_type that the savings_snapshots -// table CHECK constraint would reject. Extracted so the guard is unit-testable -// directly (the COPY path acquires a real pooled connection that pgxmock can't -// stand in for, so an end-to-end test can only prove the acquire-failure path). -func validateCommitmentType(commitmentType string) error { - if commitmentType != "RI" && commitmentType != "SavingsPlan" { - return fmt.Errorf("invalid commitment_type %q (want RI or SavingsPlan)", commitmentType) - } - return nil -} - // BulkInsertSnapshots inserts multiple snapshots efficiently via COPY. func (s *PostgresAnalyticsStore) BulkInsertSnapshots(ctx context.Context, snapshots []SavingsSnapshot) error { if len(snapshots) == 0 { diff --git a/internal/analytics/postgres_analytics_mock_test.go b/internal/analytics/postgres_analytics_mock_test.go index 9bdbfb74..5f164b1a 100644 --- a/internal/analytics/postgres_analytics_mock_test.go +++ b/internal/analytics/postgres_analytics_mock_test.go @@ -176,27 +176,30 @@ func TestUUIDGeneration(t *testing.T) { }) } -// TestCommitmentTypeLogic verifies commitment type determination. +// TestCommitmentTypeLogic pins the (provider, service) -> commitment_type map +// (#339): every Savings Plans slug is a savings plan, GCP is a CUD, and the +// rest are reserved instances. func TestCommitmentTypeLogic(t *testing.T) { - t.Run("SavingsPlans service gets SavingsPlan type", func(t *testing.T) { - service := "SavingsPlans" - commitmentType := "RI" - if service == "SavingsPlans" { - commitmentType = "SavingsPlan" - } - assert.Equal(t, "SavingsPlan", commitmentType) - }) - - t.Run("other services get RI type", func(t *testing.T) { - services := []string{"rds", "elasticache", "opensearch", "ec2"} - for _, service := range services { - commitmentType := "RI" - if service == "SavingsPlans" { - commitmentType = "SavingsPlan" - } - assert.Equal(t, "RI", commitmentType) - } - }) + cases := []struct{ provider, service, want string }{ + {"aws", "savingsplans", "savings-plan"}, + {"aws", "savings-plans", "savings-plan"}, + {"aws", "SavingsPlans", "savings-plan"}, + {"aws", "savings-plans-compute", "savings-plan"}, + {"aws", "savings-plans-ec2instance", "savings-plan"}, + {"aws", "savings-plans-sagemaker", "savings-plan"}, + {"aws", "savings-plans-database", "savings-plan"}, + {"aws", "ec2", "reserved-instance"}, + {"aws", "rds", "reserved-instance"}, + {"aws", "elasticache", "reserved-instance"}, + {"aws", "opensearch", "reserved-instance"}, + {"azure", "compute", "reserved-instance"}, + {"azure", "database", "reserved-instance"}, + {"gcp", "compute", "committed-use"}, + {"GCP", "cloudsql", "committed-use"}, + } + for _, tc := range cases { + assert.Equal(t, tc.want, commitmentTypeFor(tc.provider, tc.service), "%s/%s", tc.provider, tc.service) + } } // TestBulkInsertEmptySlice verifies empty slice handling. diff --git a/internal/analytics/postgres_analytics_test.go b/internal/analytics/postgres_analytics_test.go index ca511d25..bffb2ae5 100644 --- a/internal/analytics/postgres_analytics_test.go +++ b/internal/analytics/postgres_analytics_test.go @@ -250,6 +250,11 @@ func TestValidateCommitmentType(t *testing.T) { t.Run("accepts SavingsPlan", func(t *testing.T) { assert.NoError(t, validateCommitmentType("SavingsPlan")) }) + t.Run("accepts the pkg/common vocabulary", func(t *testing.T) { + for _, v := range []string{"reserved-instance", "savings-plan", "committed-use", "reserved-capacity"} { + assert.NoError(t, validateCommitmentType(v), v) + } + }) t.Run("rejects an unknown value", func(t *testing.T) { err := validateCommitmentType("bogus") require.Error(t, err) diff --git a/internal/database/postgres/migrations/000145_savings_snapshots_commitment_type_vocabulary.down.sql b/internal/database/postgres/migrations/000145_savings_snapshots_commitment_type_vocabulary.down.sql new file mode 100644 index 00000000..42831fee --- /dev/null +++ b/internal/database/postgres/migrations/000145_savings_snapshots_commitment_type_vocabulary.down.sql @@ -0,0 +1,18 @@ +-- Revert 000145 (issue #339): restore the two-value commitment_type CHECK. +-- +-- Rows are mapped back to the only labels the old collector could emit: +-- savings-plan becomes 'SavingsPlan' and everything else 'RI'. This is lossy on +-- purpose. GCP committed-use rows go back to 'RI', which is what the pre-#339 +-- code wrote for them. + +ALTER TABLE savings_snapshots DROP CONSTRAINT IF EXISTS valid_commitment_type; + +UPDATE savings_snapshots +SET commitment_type = CASE + WHEN commitment_type = 'savings-plan' THEN 'SavingsPlan' + ELSE 'RI' + END +WHERE commitment_type NOT IN ('RI', 'SavingsPlan'); + +ALTER TABLE savings_snapshots + ADD CONSTRAINT valid_commitment_type CHECK (commitment_type IN ('RI', 'SavingsPlan')); diff --git a/internal/database/postgres/migrations/000145_savings_snapshots_commitment_type_vocabulary.up.sql b/internal/database/postgres/migrations/000145_savings_snapshots_commitment_type_vocabulary.up.sql new file mode 100644 index 00000000..5e529679 --- /dev/null +++ b/internal/database/postgres/migrations/000145_savings_snapshots_commitment_type_vocabulary.up.sql @@ -0,0 +1,45 @@ +-- Issue #339 (audit finding A06-019): savings_snapshots.commitment_type uses +-- the pkg/common CommitmentType vocabulary. +-- +-- The collector labeled every commitment "RI" except a service literally named +-- "SavingsPlans", which purchase_history never stores (Savings Plans are +-- "savingsplans" or a "savings-plans-" slug). So GCP committed-use +-- discounts and AWS Savings Plans were both stored as "RI", and the CHECK only +-- admitted the two literals that mapper could emit. The collector now derives +-- the type from (provider, service) as one of the pkg/common values: +-- reserved-instance, savings-plan, committed-use, reserved-capacity. +-- +-- The CHECK keeps admitting the legacy 'RI' / 'SavingsPlan' literals so a +-- replica still on the old binary can write during a rolling deploy without +-- failing its collection run. The collector never emits them, and the backfill +-- below rewrites every existing legacy row. +-- +-- The whole file runs in one transaction (golang-migrate), and ALTER TABLE +-- holds an ACCESS EXCLUSIVE lock on savings_snapshots and its partitions until +-- commit. A concurrent collector insert waits for the commit and is then +-- checked against the new constraint, which admits both vocabularies, so it +-- cannot fail because of this migration. All partitions are created with +-- PARTITION OF, so they inherit the constraint from the parent, and DROP/ADD on +-- the parent applies to every partition. + +ALTER TABLE savings_snapshots DROP CONSTRAINT IF EXISTS valid_commitment_type; + +-- Relabel existing rows from (provider, service), as the collector now does. +UPDATE savings_snapshots +SET commitment_type = CASE + WHEN commitment_type = 'SavingsPlan' THEN 'savings-plan' + WHEN lower(provider) = 'gcp' THEN 'committed-use' + WHEN lower(service) IN ( + 'savingsplans', 'savings-plans', + 'savings-plans-compute', 'savings-plans-ec2instance', + 'savings-plans-sagemaker', 'savings-plans-database' + ) THEN 'savings-plan' + ELSE 'reserved-instance' + END +WHERE commitment_type IN ('RI', 'SavingsPlan'); + +ALTER TABLE savings_snapshots + ADD CONSTRAINT valid_commitment_type CHECK (commitment_type IN ( + 'reserved-instance', 'savings-plan', 'committed-use', 'reserved-capacity', + 'RI', 'SavingsPlan' + )); diff --git a/internal/database/postgres/migrations/000145_savings_snapshots_commitment_type_vocabulary_test.go b/internal/database/postgres/migrations/000145_savings_snapshots_commitment_type_vocabulary_test.go new file mode 100644 index 00000000..5dd8e96a --- /dev/null +++ b/internal/database/postgres/migrations/000145_savings_snapshots_commitment_type_vocabulary_test.go @@ -0,0 +1,123 @@ +//go:build integration + +package migrations_test + +import ( + "context" + "errors" + "fmt" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/jackc/pgx/v5/pgconn" + "github.com/jackc/pgx/v5/pgxpool" + "github.com/stretchr/testify/require" +) + +const commitmentTypeVocabularyVersion = 145 + +// insertTypedSnapshot inserts one savings_snapshots row and returns the error +// so callers can assert CHECK rejections. +func insertTypedSnapshot(ctx context.Context, pool *pgxpool.Pool, id string, ts time.Time, provider, service, commitmentType string) error { + _, err := pool.Exec(ctx, ` + INSERT INTO savings_snapshots (id, account_id, timestamp, provider, service, region, commitment_type) + VALUES ($1, 'acct-339', $2, $3, $4, 'us-east-1', $5)`, id, ts, provider, service, commitmentType) + return err +} + +func requireCheckViolation(t *testing.T, err error) { + t.Helper() + var pgErr *pgconn.PgError + require.True(t, errors.As(err, &pgErr), "want a Postgres error, got %v", err) + require.Equal(t, "23514", pgErr.Code, "want check_violation, got %s: %s", pgErr.Code, pgErr.Message) + require.Equal(t, "valid_commitment_type", pgErr.ConstraintName) +} + +func commitmentTypeOf(ctx context.Context, t *testing.T, pool *pgxpool.Pool, id string) string { + t.Helper() + var v string + require.NoError(t, pool.QueryRow(ctx, `SELECT commitment_type FROM savings_snapshots WHERE id = $1`, id).Scan(&v)) + return v +} + +// TestMigration_SavingsSnapshotsCommitmentTypeVocabulary covers 000145 (#339, +// A06-019): the CHECK admits the pkg/common vocabulary (plus the legacy +// literals), existing rows are relabeled from (provider, service) in every +// partition, and down restores the two-value CHECK. +func TestMigration_SavingsSnapshotsCommitmentTypeVocabulary(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequirePostgresContainer(ctx, t) + t.Cleanup(func() { require.NoError(t, container.Cleanup(context.Background())) }) + pool, path := container.DB.Pool(), getMigrationsPath() + prev := previousMigrationVersion(t, commitmentTypeVocabularyVersion) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) + + // One month gets its own partition; the other month lands in the default + // partition, so the relabel and the constraint are checked on both. + _, err := pool.Exec(ctx, `SELECT create_savings_snapshot_partition('2031-05-15'::date)`) + require.NoError(t, err) + inPartition := time.Date(2031, 5, 10, 12, 0, 0, 0, time.UTC) + inDefault := time.Date(2041, 7, 10, 12, 0, 0, 0, time.UTC) + + // Rows as the pre-fix collector wrote them. + legacy := []struct { + id, provider, service, commitmentType string + ts time.Time + want string + }{ + {"00000000-0000-0000-0000-000000003391", "aws", "ec2", "RI", inPartition, "reserved-instance"}, + {"00000000-0000-0000-0000-000000003392", "aws", "savingsplans", "RI", inDefault, "savings-plan"}, + {"00000000-0000-0000-0000-000000003393", "aws", "savings-plans-compute", "RI", inPartition, "savings-plan"}, + {"00000000-0000-0000-0000-000000003394", "aws", "SavingsPlans", "SavingsPlan", inDefault, "savings-plan"}, + {"00000000-0000-0000-0000-000000003395", "gcp", "compute", "RI", inPartition, "committed-use"}, + {"00000000-0000-0000-0000-000000003396", "gcp", "cloudsql", "RI", inDefault, "committed-use"}, + {"00000000-0000-0000-0000-000000003397", "azure", "compute", "RI", inDefault, "reserved-instance"}, + } + for _, r := range legacy { + require.NoError(t, insertTypedSnapshot(ctx, pool, r.id, r.ts, r.provider, r.service, r.commitmentType)) + } + + // The bug at the schema level: a GCP CUD cannot be stored as one. + requireCheckViolation(t, insertTypedSnapshot(ctx, pool, + "00000000-0000-0000-0000-000000003390", inPartition, "gcp", "compute", "committed-use")) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, commitmentTypeVocabularyVersion)) + + for _, r := range legacy { + require.Equal(t, r.want, commitmentTypeOf(ctx, t, pool, r.id), "%s/%s", r.provider, r.service) + } + + // Every pkg/common value is admitted in both partitions, the legacy + // literals still are (rolling deploy), and anything else is not. + n := 0 + for _, ts := range []time.Time{inPartition, inDefault} { + for _, v := range []string{"reserved-instance", "savings-plan", "committed-use", "reserved-capacity", "RI", "SavingsPlan"} { + n++ + id := fmt.Sprintf("00000000-0000-0000-0001-%012d", n) + require.NoError(t, insertTypedSnapshot(ctx, pool, id, ts, "aws", "ec2", v), "%s at %s", v, ts) + } + requireCheckViolation(t, insertTypedSnapshot(ctx, pool, + fmt.Sprintf("00000000-0000-0000-0002-%012d", ts.Year()), ts, "aws", "ec2", "CUD")) + } + + t.Run("down restores the two-value CHECK and up relabels again", func(t *testing.T) { + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, prev)) + + var bad int + require.NoError(t, pool.QueryRow(ctx, + `SELECT count(*) FROM savings_snapshots WHERE commitment_type NOT IN ('RI', 'SavingsPlan')`).Scan(&bad)) + require.Zero(t, bad) + require.Equal(t, "SavingsPlan", commitmentTypeOf(ctx, t, pool, "00000000-0000-0000-0000-000000003392")) + require.Equal(t, "RI", commitmentTypeOf(ctx, t, pool, "00000000-0000-0000-0000-000000003395")) + requireCheckViolation(t, insertTypedSnapshot(ctx, pool, + "00000000-0000-0000-0003-000000000001", inPartition, "gcp", "compute", "committed-use")) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, commitmentTypeVocabularyVersion)) + for _, r := range legacy { + require.Equal(t, r.want, commitmentTypeOf(ctx, t, pool, r.id), "%s/%s", r.provider, r.service) + } + }) +} diff --git a/internal/purchase/approvals_test.go b/internal/purchase/approvals_test.go index 36cc27ec..6d7c4ed3 100644 --- a/internal/purchase/approvals_test.go +++ b/internal/purchase/approvals_test.go @@ -283,7 +283,9 @@ func TestManager_ApproveAndExecute_EmptyPlanID(t *testing.T) { } store.On("TransitionExecutionStatus", ctx, "exec-direct-1", approveFromStatuses, "running", (*string)(nil)).Return(updated, nil) // No GetPurchasePlan / GetPlanAccounts / UpdatePurchasePlan calls — see AssertNotCalled below. - sender.On("SendPurchaseConfirmation", mock.Anything, mock.Anything).Return(nil) + // No recommendations, so nothing is bought and no "purchase confirmed" + // email goes out (#339): with no SendPurchaseConfirmation expectation + // registered, a send fails the test. store.On("SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) _, _, err := manager.ApproveAndExecute(ctx, "exec-direct-1", "operator@example.com", nil, nil) diff --git a/internal/purchase/coverage_extra_test.go b/internal/purchase/coverage_extra_test.go index 3737064d..b40b596a 100644 --- a/internal/purchase/coverage_extra_test.go +++ b/internal/purchase/coverage_extra_test.go @@ -412,7 +412,8 @@ func TestProcessMessage_ApproveHappyPath(t *testing.T) { // mistake) would mismatch the mock expectation and fail the test. plan := &config.PurchasePlan{ID: planID, Name: "test-plan"} mockStore.On("GetPurchasePlan", mock.Anything, planID).Return(plan, nil) - mockEmail.On("SendPurchaseConfirmation", mock.Anything, mock.Anything).Return(nil) + // Nothing is selected, so nothing is bought and no "purchase confirmed" + // email goes out (#339). mockStore.On("SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) mockStore.On("CompletePlanStep", mock.Anything, planID, 1).Return(nil) @@ -425,6 +426,7 @@ func TestProcessMessage_ApproveHappyPath(t *testing.T) { err := manager.ProcessMessage(ctx, `{"type":"approve","execution_id":"exec-appv","token":"correct-token","actor_email":"owner@example.com"}`) require.NoError(t, err) mockStore.AssertExpectations(t) + mockEmail.AssertNotCalled(t, "SendPurchaseConfirmation", mock.Anything, mock.Anything) mockEmail.AssertExpectations(t) } @@ -585,7 +587,8 @@ func TestProcessMessage_ApproveFourEyesOn_DifferentApproverSucceeds(t *testing.T mockStore.On("TransitionExecutionStatus", ctx, "exec-appv-diff", []string{"pending", "notified"}, "running", (*string)(nil)).Return(approved, nil) plan := &config.PurchasePlan{ID: planID, Name: "test-plan"} mockStore.On("GetPurchasePlan", mock.Anything, planID).Return(plan, nil) - mockEmail.On("SendPurchaseConfirmation", mock.Anything, mock.Anything).Return(nil) + // Nothing is selected, so nothing is bought and no "purchase confirmed" + // email goes out (#339). mockStore.On("SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) mockStore.On("CompletePlanStep", mock.Anything, planID, 1).Return(nil) @@ -598,6 +601,7 @@ func TestProcessMessage_ApproveFourEyesOn_DifferentApproverSucceeds(t *testing.T err := manager.ProcessMessage(ctx, `{"type":"approve","execution_id":"exec-appv-diff","token":"correct-token","actor_email":"approver@example.com"}`) require.NoError(t, err) mockStore.AssertExpectations(t) + mockEmail.AssertNotCalled(t, "SendPurchaseConfirmation", mock.Anything, mock.Anything) mockEmail.AssertExpectations(t) } diff --git a/internal/purchase/empty_selection_notify_test.go b/internal/purchase/empty_selection_notify_test.go new file mode 100644 index 00000000..b91e25bb --- /dev/null +++ b/internal/purchase/empty_selection_notify_test.go @@ -0,0 +1,108 @@ +package purchase + +import ( + "context" + "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" +) + +// unselectedRecs is a plan step whose recommendations are all deselected, so +// processPurchaseRecommendations short-circuits before any cloud API call. +func unselectedRecs() []config.RecommendationRecord { + return []config.RecommendationRecord{ + {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 300, Savings: 50}, + {Provider: "aws", Service: "rds", ResourceType: "db.r5.large", Region: "us-east-1", Count: 1, UpfrontCost: 600, Savings: 90}, + } +} + +// TestExecutePurchase_SingleAccount_NothingSelected_SendsNoConfirmation is the +// #339 (A05-015) regression guard for the single-account path: a run with +// nothing selected buys nothing, so it must not email a "purchase confirmed" +// for zero commitments and $0. +func TestExecutePurchase_SingleAccount_NothingSelected_SendsNoConfirmation(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockEmail := new(MockEmailSender) + mockFactory := new(MockProviderFactory) + + plan := &config.PurchasePlan{ID: "plan-339", Name: "Plan 339"} + exec := &config.PurchaseExecution{ + ExecutionID: "exec-339-single", + PlanID: "plan-339", + StepNumber: 1, + Recommendations: unselectedRecs(), + } + mockStore.On("GetPurchasePlan", mock.Anything, "plan-339").Return(plan, nil) + mockEmail.On("SendPurchaseConfirmation", mock.Anything, mock.Anything).Return(nil).Maybe() + + manager := &Manager{ + config: mockStore, + email: mockEmail, + providerFactory: mockFactory, + dashboardURL: "https://dashboard.example.com", + } + + require.NoError(t, manager.executePurchase(ctx, exec)) + + mockEmail.AssertNotCalled(t, "SendPurchaseConfirmation", mock.Anything, mock.Anything) + mockFactory.AssertNotCalled(t, "CreateAndValidateProvider", mock.Anything, mock.Anything, mock.Anything) + for i := range exec.Recommendations { + assert.False(t, exec.Recommendations[i].Purchased) + } +} + +// TestExecuteForAccount_NothingSelected_SendsNoConfirmation is the same #339 +// guard for the multi-account fan-out path, which gated the confirmation on +// "no per-rec errors" and so also confirmed an account that bought nothing. +func TestExecuteForAccount_NothingSelected_SendsNoConfirmation(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockEmail := new(MockEmailSender) + allowFailedNotification(mockEmail) + mockFactory := new(MockProviderFactory) + + const acctID = "aaaaaaaa-0000-0000-0000-000000000339" + account := config.CloudAccount{ID: acctID, Name: "Prod", Provider: "aws", ExternalID: "111111111111", AWSAuthMode: "access_keys"} + baseExec := &config.PurchaseExecution{ + ExecutionID: "exec-339-base", + PlanID: "plan-339", + Source: common.PurchaseSourceWeb, + Recommendations: unselectedRecs(), + } + plan := &config.PurchasePlan{ID: "plan-339", Name: "Plan 339"} + + var savedExec *config.PurchaseExecution + mockStore.SavePurchaseExecutionFn = func(_ context.Context, e *config.PurchaseExecution) error { + c := *e + savedExec = &c + return nil + } + mockEmail.On("SendPurchaseConfirmation", mock.Anything, mock.Anything).Return(nil).Maybe() + + credStore := &MockCredentialStore{ + LoadRawFn: func(_ context.Context, _, _ string) ([]byte, error) { + return []byte(`{"access_key_id":"AKIAIOSFODNN7EXAMPLE","secret_access_key":"wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"}`), nil + }, + } + + manager := &Manager{ + config: mockStore, + email: mockEmail, + providerFactory: mockFactory, + credStore: credStore, + dashboardURL: "https://dashboard.example.com", + } + + committed, err := manager.executeForAccount(ctx, baseExec, plan, account) + require.NoError(t, err) + assert.False(t, committed, "nothing was selected, so nothing committed") + + require.NotNil(t, savedExec, "the per-account row is still saved") + mockEmail.AssertNotCalled(t, "SendPurchaseConfirmation", mock.Anything, mock.Anything) + mockFactory.AssertNotCalled(t, "CreateAndValidateProvider", mock.Anything, mock.Anything, mock.Anything) +} diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 5696de36..b4cd5c6e 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -134,11 +134,14 @@ func (m *Manager) executeSingleAccount(ctx context.Context, exec *config.Purchas return fmt.Errorf("some purchases failed: %v", purchaseErrors) } - // At least one rec committed (full or partial success): the confirmation - // must go out for the recs that purchased. buildPurchaseConfirmationData - // only lists Purchased recs, so a partial run notifies on exactly those. - if notifyErr := m.sendPurchaseNotification(ctx, exec, plan, totalSavings, totalUpfront); notifyErr != nil { - logging.Errorf("Failed to send confirmation: %v", notifyErr) + // Confirm only when at least one rec committed (full or partial success). + // buildPurchaseConfirmationData only lists Purchased recs, so a partial run + // notifies on exactly those, and a run with nothing selected (it never + // reached a cloud API) sends no "purchase confirmed" email for $0 (#339). + if anyRecPurchased(exec.Recommendations) { + if notifyErr := m.sendPurchaseNotification(ctx, exec, plan, totalSavings, totalUpfront); notifyErr != nil { + logging.Errorf("Failed to send confirmation: %v", notifyErr) + } } if len(purchaseErrors) > 0 { @@ -329,7 +332,7 @@ func (m *Manager) executeForAccount(ctx context.Context, baseExec *config.Purcha // Resolve the final status/error/completion stamp from the per-rec outcome // (#642), and learn whether any rec committed (#1014). - partial, committed := applyAccountOutcome(&acctExec, purchaseErrors) + _, committed = applyAccountOutcome(&acctExec, purchaseErrors) if saveErr := m.config.SavePurchaseExecution(ctx, &acctExec); saveErr != nil { return committed, fmt.Errorf("AUDIT LOSS: failed to save execution record for account %s: %w", account.ID, saveErr) @@ -337,8 +340,10 @@ func (m *Manager) executeForAccount(ctx context.Context, baseExec *config.Purcha // Send the confirmation whenever at least one rec committed (full or // partial success). buildPurchaseConfirmationData only lists Purchased - // recs, so a partial run notifies on exactly the recs that fired. - if len(purchaseErrors) == 0 || partial { + // recs, so a partial run notifies on exactly the recs that fired. Gating on + // committed rather than "no errors" keeps an account with nothing selected + // from getting a "purchase confirmed" email for $0 (#339). + if committed { if err := m.sendPurchaseNotification(ctx, &acctExec, plan, totalSavings, totalUpfront); err != nil { logging.Errorf("Failed to send confirmation for account %s: %v", account.ID, err) } diff --git a/internal/server/health.go b/internal/server/health.go index b4ccfa09..6bf6ff9a 100644 --- a/internal/server/health.go +++ b/internal/server/health.go @@ -99,10 +99,10 @@ func (app *Application) collectHealth(ctx context.Context) HealthStatus { // pointers when the mutex is free, and while initialization holds it // report both stores as pending rather than race on half-written state. if app.dbMu.TryLock() { - configStore, authService, db := app.Config, app.Auth, app.DB + configStore, authService, db, connected := app.Config, app.Auth, app.DB, app.dbConnected app.dbMu.Unlock() - health.Checks["config_store"] = app.checkConfigStore(ctx, configStore, db) + health.Checks["config_store"] = app.checkConfigStore(ctx, configStore, db, connected) if health.Checks["config_store"].Status != "healthy" { health.Status = "degraded" } @@ -169,10 +169,10 @@ func (app *Application) checkMigrations() CheckResult { } } -// checkConfigStore checks the health of the configuration store. The store and -// connection come in as a snapshot taken under dbMu by collectHealth, so this -// function never reads pointers ensureDB may be rewriting. -func (app *Application) checkConfigStore(ctx context.Context, configStore config.StoreInterface, db *database.Connection) CheckResult { +// checkConfigStore checks the health of the configuration store. The store, +// connection and connected flag come in as a snapshot taken under dbMu by +// collectHealth, so this function never reads state ensureDB may be rewriting. +func (app *Application) checkConfigStore(ctx context.Context, configStore config.StoreInterface, db *database.Connection, connected bool) CheckResult { // Check if config store exists if configStore == nil { // If using PostgreSQL with lazy initialization, DB might not be connected yet @@ -188,6 +188,18 @@ func (app *Application) checkConfigStore(ctx context.Context, configStore config } } + // With lazy PostgreSQL init the store is only usable once ensureDB has + // finished. reinitializeAfterConnect sets app.Config before steps that can + // still fail; ensureDB then closes the pool and clears app.DB but leaves + // app.Config over the closed pool. Without this check that state reported + // healthy (#339). ensureDB retries on the next request. + if app.dbConfig != nil && !connected { + return CheckResult{ + Status: "unhealthy", + Message: "Database initialization did not complete; config store is not connected", + } + } + // If using PostgreSQL, check database connection health if db != nil { if err := db.HealthCheck(ctx); err != nil { diff --git a/internal/server/health_reinit_failure_integration_test.go b/internal/server/health_reinit_failure_integration_test.go new file mode 100644 index 00000000..65598308 --- /dev/null +++ b/internal/server/health_reinit_failure_integration_test.go @@ -0,0 +1,64 @@ +//go:build integration + +package server + +import ( + "context" + "strings" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestHealth_ConfigStoreAfterFailedReinitialization is the #339 (A06-009) +// regression test on the real path: ensureDB connects to a live Postgres, +// reinitializeAfterConnect sets app.Config and then fails (here at the AWS +// config load, via a profile that does not exist), and ensureDB closes the +// pool. The config_store check used to report "healthy" over that closed pool. +func TestHealth_ConfigStoreAfterFailedReinitialization(t *testing.T) { + for _, key := range []string{ + "CREDENTIAL_ENCRYPTION_KEY_SECRET_ARN", "CREDENTIAL_ENCRYPTION_KEY_SECRET_NAME", + "CREDENTIAL_ENCRYPTION_KEY_SECRET_ID", "CREDENTIAL_ENCRYPTION_ALLOW_DEV_KEY", + "CUDLY_SIGNING_KEY_ID", "CUDLY_SIGNING_KEY_VAULT_URL", "CUDLY_SIGNING_KEY_NAME", + "CUDLY_SIGNING_KEY_RESOURCE", "AWS_LAMBDA_RUNTIME_API", + } { + t.Setenv(key, "") + } + t.Setenv("CREDENTIAL_ENCRYPTION_KEY", strings.Repeat("12", 32)) + t.Setenv("CUDLY_ISSUER_URL", "https://health-339.example.test") + t.Setenv("CUDLY_SOURCE_CLOUD", "aws") + t.Setenv("SCHEDULED_TASK_AUTH_MODE", "disabled") + t.Setenv("AWS_EC2_METADATA_DISABLED", "true") + t.Setenv("AWS_CONFIG_FILE", "/dev/null") + t.Setenv("AWS_SHARED_CREDENTIALS_FILE", "/dev/null") + // LoadDefaultConfig fails on a named profile missing from the config file, + // after reinitializeAfterConnect has already published app.Config. + t.Setenv("AWS_PROFILE", "issue-339-missing-profile") + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + pg, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, pg.Cleanup(context.Background())) }) + + app, err := NewApplicationFromDeps(ctx, ApplicationConfig{DashboardURL: "https://health-339.example.test"}, + ExternalDeps{DBConfig: pg.Config, EmailSender: &noopEmailSender{}}) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, app.Close()) }) + + err = app.ensureDB(ctx) + require.Error(t, err) + require.Contains(t, err.Error(), "failed to reinitialize after DB connect") + require.NotNil(t, app.Config, "the failure path leaves app.Config over the closed pool") + require.Nil(t, app.DB) + require.False(t, app.dbConnected) + + health := app.collectHealth(ctx) + assert.Equal(t, "unhealthy", health.Checks["config_store"].Status, + "config_store must not report healthy over a store whose pool ensureDB closed") + assert.Equal(t, "degraded", health.Status) + assert.False(t, health.ready()) +} diff --git a/internal/server/health_test.go b/internal/server/health_test.go index 53727f78..80447860 100644 --- a/internal/server/health_test.go +++ b/internal/server/health_test.go @@ -368,6 +368,25 @@ func TestCheckConfigStore(t *testing.T) { }, expectedStatus: "pending", }, + { + // #339: reinitializeAfterConnect set Config, then failed, and + // ensureDB closed the pool and cleared DB without connecting. + name: "config store left over a failed lazy init", + setupApp: func(app *Application) { + app.Config = &mockConfigStoreForHealth{} + app.dbConfig = &databaseConfigStub{} + }, + expectedStatus: "unhealthy", + }, + { + name: "config store after a completed lazy init", + setupApp: func(app *Application) { + app.Config = &mockConfigStoreForHealth{} + app.dbConfig = &databaseConfigStub{} + app.dbConnected = true + }, + expectedStatus: "healthy", + }, } for _, tt := range tests { @@ -378,7 +397,7 @@ func TestCheckConfigStore(t *testing.T) { tt.setupApp(app) } - result := app.checkConfigStore(ctx, app.Config, app.DB) + result := app.checkConfigStore(ctx, app.Config, app.DB, app.dbConnected) testutil.AssertEqual(t, tt.expectedStatus, result.Status) }) } diff --git a/internal/server/readiness_test.go b/internal/server/readiness_test.go index f68f7780..60b545de 100644 --- a/internal/server/readiness_test.go +++ b/internal/server/readiness_test.go @@ -98,6 +98,8 @@ func TestReadinessFailedMigrations(t *testing.T) { Config: &mockConfigStoreForHealth{}, Auth: createHealthyAuthService(), dbConfig: &dbConfigForReadiness, + // ensureDB finished: the stores above are the live ones. + dbConnected: true, } app.recordMigrationResult(context.DeadlineExceeded) @@ -120,6 +122,8 @@ func TestReadinessPendingMigrations(t *testing.T) { Config: &mockConfigStoreForHealth{}, Auth: createHealthyAuthService(), dbConfig: &dbConfigForReadiness, + // ensureDB finished: the stores above are the live ones. + dbConnected: true, } req := httptest.NewRequestWithContext(context.Background(), "GET", "/ready", nil) @@ -145,6 +149,8 @@ func TestReadinessDisabledMigrationsStayReady(t *testing.T) { Config: &mockConfigStoreForHealth{}, Auth: createHealthyAuthService(), dbConfig: &noMigrate, + // ensureDB finished: the stores above are the live ones. + dbConnected: true, } req := httptest.NewRequestWithContext(context.Background(), "GET", "/ready", nil)