diff --git a/internal/database/postgres/migrations/000133_purchase_history_reverify_ec2_offering_class.down.sql b/internal/database/postgres/migrations/000133_purchase_history_reverify_ec2_offering_class.down.sql new file mode 100644 index 00000000..79fc9836 --- /dev/null +++ b/internal/database/postgres/migrations/000133_purchase_history_reverify_ec2_offering_class.down.sql @@ -0,0 +1,6 @@ +-- Intentionally a no-op: the up migration cannot record which rows it cleared +-- without also keeping the stale class it exists to remove, and re-stamping +-- every NULL EC2 row 'convertible' would re-block Standard RIs (and overwrite +-- pre-000087 rows that were never stamped). Cleared rows that were already +-- re-fetched from AWS keep their authoritative class. +SELECT 1; diff --git a/internal/database/postgres/migrations/000133_purchase_history_reverify_ec2_offering_class.up.sql b/internal/database/postgres/migrations/000133_purchase_history_reverify_ec2_offering_class.up.sql new file mode 100644 index 00000000..99d79cb1 --- /dev/null +++ b/internal/database/postgres/migrations/000133_purchase_history_reverify_ec2_offering_class.up.sql @@ -0,0 +1,28 @@ +-- Clear stale EC2 RI offering_class stamps so they are re-read from AWS +-- (platform#524). +-- +-- Before platform#509, savePurchaseHistory stamped every AWS 'ec2' row +-- 'convertible' whatever global_config.offering_class said. Rows bought while +-- that setting was 'standard' are Standard RIs recorded as convertible, so the +-- marketplace-sell guard refuses them and the History page hides Sell. +-- global_config keeps no history, so the database alone cannot tell those rows +-- from genuinely Convertible ones; only AWS DescribeReservedInstances can. +-- +-- Owner option (a) on the issue: drop the convertible stamp on every AWS EC2 +-- RI row (both the legacy 'ec2' and the canonical 'compute' slug, which also +-- clears any post-#509 re-drive stamp that recorded the class configured at +-- retry time). NULL is the column's "unknown" state: the marketplace-list +-- handler then calls populateOfferingClass, which fetches the class from AWS +-- and re-stamps the row, so a Standard RI becomes sellable and a Convertible +-- one is stamped convertible again after a single refused (400) Sell attempt. +-- +-- Only 'convertible' is cleared. A 'standard' stamp is left alone (this issue +-- is about Standard RIs being blocked, not the reverse), and a concurrent +-- StampOfferingClass that commits 'standard' first is not overwritten: +-- under READ COMMITTED the UPDATE re-checks the WHERE clause on the +-- committed row version and skips it. +UPDATE purchase_history +SET offering_class = NULL +WHERE provider = 'aws' + AND service IN ('ec2', 'compute') + AND LOWER(offering_class) = 'convertible'; diff --git a/internal/database/postgres/migrations/000133_purchase_history_reverify_ec2_offering_class_test.go b/internal/database/postgres/migrations/000133_purchase_history_reverify_ec2_offering_class_test.go new file mode 100644 index 00000000..d7d9215a --- /dev/null +++ b/internal/database/postgres/migrations/000133_purchase_history_reverify_ec2_offering_class_test.go @@ -0,0 +1,135 @@ +//go:build integration +// +build integration + +package migrations_test + +import ( + "context" + "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/pgxpool" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const migration133Version = 133 + +// insertOfferingClassRow inserts one purchase_history row with the given +// provider, service and offering_class (nil stores NULL). +func insertOfferingClassRow(ctx context.Context, t *testing.T, pool *pgxpool.Pool, purchaseID, provider, service string, offeringClass *string) { + t.Helper() + _, err := pool.Exec(ctx, ` + INSERT INTO purchase_history ( + account_id, purchase_id, timestamp, provider, service, region, + resource_type, term, payment, offering_class + ) VALUES ('test-account', $1, $2, $3, $4, 'us-east-1', 'm5.large', 1, 'all-upfront', $5)`, + purchaseID, time.Now(), provider, service, offeringClass) + require.NoError(t, err) +} + +// offeringClassOf returns the row's offering_class, nil when NULL. +func offeringClassOf(ctx context.Context, t *testing.T, pool *pgxpool.Pool, purchaseID string) *string { + t.Helper() + var class *string + require.NoError(t, pool.QueryRow(ctx, + `SELECT offering_class FROM purchase_history WHERE purchase_id = $1`, purchaseID).Scan(&class)) + return class +} + +// TestMigration133_ClearsConvertibleEC2Stamps locks down migration 000133 +// (issue #524): before #509 every AWS 'ec2' purchase_history row was stamped +// 'convertible' even when the class setting was 'standard', which blocks +// selling a Standard RI. The migration clears the convertible stamp on AWS EC2 +// rows (legacy 'ec2' and canonical 'compute' slugs) so the marketplace handler +// re-fetches the class from AWS, and leaves every other row alone. +// +// Fail-before: with the UPDATE removed from the up file, the cleared rows keep +// 'convertible' and the NULL assertions fail. +func TestMigration133_ClearsConvertibleEC2Stamps(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) + defer cancel() + + container := testhelpers.RequirePostgresContainer(ctx, t) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + migrationsPath := getMigrationsPath() + + prev := previousMigrationVersion(t, migration133Version) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, prev)) + + cases := []struct { + purchaseID, provider, service string + before, after *string + }{ + // The defect: a stale convertible stamp on an EC2 RI. + {"ri-ec2-convertible", "aws", "ec2", strPtr("convertible"), nil}, + {"ri-compute-convertible", "aws", "compute", strPtr("convertible"), nil}, + // The sell guard compares case-insensitively, so the reset does too. + {"ri-ec2-convertible-upper", "aws", "ec2", strPtr("Convertible"), nil}, + // A standard stamp is already sellable and must not be touched. + {"ri-ec2-standard", "aws", "ec2", strPtr("standard"), strPtr("standard")}, + {"ri-compute-standard", "aws", "compute", strPtr("standard"), strPtr("standard")}, + // Already unknown: stays unknown. + {"ri-ec2-null", "aws", "ec2", nil, nil}, + // Out of scope: other AWS services and other providers. + {"ri-rds-convertible", "aws", "rds", strPtr("convertible"), strPtr("convertible")}, + {"azure-compute-convertible", "azure", "compute", strPtr("convertible"), strPtr("convertible")}, + } + for _, c := range cases { + insertOfferingClassRow(ctx, t, pool, c.purchaseID, c.provider, c.service, c.before) + } + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, migration133Version)) + for _, c := range cases { + assert.Equal(t, c.after, offeringClassOf(ctx, t, pool, c.purchaseID), "after 000133: %s", c.purchaseID) + } + + // The down migration is a no-op: it must apply cleanly and must not + // re-stamp cleared rows convertible (that would re-block Standard RIs). + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, prev)) + for _, c := range cases { + assert.Equal(t, c.after, offeringClassOf(ctx, t, pool, c.purchaseID), "after rollback: %s", c.purchaseID) + } +} + +// TestMigration133_ConcurrentStandardStampWins covers the marketplace handler +// stamping a row 'standard' from AWS (StampOfferingClass) while the migration +// runs. The migration queues behind the stamp's row lock; once the stamp +// commits, the UPDATE re-checks its WHERE clause on the new row version and +// must leave the authoritative 'standard' in place rather than clearing it. +func TestMigration133_ConcurrentStandardStampWins(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) + defer cancel() + + container := testhelpers.RequirePostgresContainer(ctx, t) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + migrationsPath := getMigrationsPath() + + prev := previousMigrationVersion(t, migration133Version) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, prev)) + + const purchaseID = "ri-ec2-race" + insertOfferingClassRow(ctx, t, pool, purchaseID, "aws", "ec2", strPtr("convertible")) + + stamp, err := pool.Begin(ctx) + require.NoError(t, err) + defer func() { _ = stamp.Rollback(ctx) }() + _, err = stamp.Exec(ctx, `UPDATE purchase_history SET offering_class = $1 WHERE purchase_id = $2`, "standard", purchaseID) + require.NoError(t, err) + + done := make(chan error, 1) + go func() { done <- migrations.MigrateToVersion(ctx, pool, migrationsPath, migration133Version) }() + + // pg_stat_activity truncates query text at 1024 bytes, which cuts the + // migration file inside its header comment, so match on that comment. + testhelpers.WaitForLockWait(ctx, t, pool, "%stale EC2 RI offering_class%") + require.NoError(t, stamp.Commit(ctx)) + require.NoError(t, testhelpers.Await(t, done)) + + assert.Equal(t, strPtr("standard"), offeringClassOf(ctx, t, pool, purchaseID), + "a standard stamp committed while 000133 waited must survive the migration") +}