From 452e2dff4423953d66790763985e2524f17433e8 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Tue, 20 Oct 2026 01:45:10 +0100 Subject: [PATCH] fix(db): let ri_exchange_history.account_id hold Azure and GCP identifiers ri_exchange_history.account_id has been VARCHAR(20) with CHECK (account_id ~ '^\d{12}$') since 000009: the exact shape of an AWS account ID. An Azure subscription GUID overflows the width (22001), and any non-AWS value, including a GCP project ID that fits in 20 characters, is rejected by the CHECK (23514) even at VARCHAR(255). Latent today (only the AWS exchange path writes this table), but it would drop the audit row of an already-billed exchange as soon as an Azure or GCP writer lands. Migration 000137 widens the column to VARCHAR(255) (matching cloud_accounts.external_id and purchase_history.account_id) and drops the AWS-only CHECK. Each half is conditioned on the catalog state, so fresh, deployed, partially fixed and re-run databases all converge, and both are re-checked afterwards. The table lock is taken before any catalog read so a concurrent replay waits and then no-ops instead of failing with "constraint does not exist". The down migration refuses while any non-AWS row exists, then narrows and restores the CHECK. The AWS shape is now enforced in Go at the write boundary that knows the provider: the AWS exchange store adapter refuses any account ID that is not 12 digits before calling the store, so AWS rows keep the guarantee the CHECK gave them, including refusing the "unknown" placeholder resolveAccountID returns when STS fails. Closes #144 Co-Authored-By: Claude Opus 5.5 --- internal/config/store_postgres_db_test.go | 9 +- ...nge_history_account_id_multicloud.down.sql | 75 +++++ ...hange_history_account_id_multicloud.up.sql | 137 +++++++++ ...ange_history_account_id_multicloud_test.go | 285 ++++++++++++++++++ internal/server/handler_ri_exchange.go | 26 ++ internal/server/handler_ri_exchange_test.go | 59 +++- 6 files changed, 579 insertions(+), 12 deletions(-) create mode 100644 internal/database/postgres/migrations/000137_ri_exchange_history_account_id_multicloud.down.sql create mode 100644 internal/database/postgres/migrations/000137_ri_exchange_history_account_id_multicloud.up.sql create mode 100644 internal/database/postgres/migrations/000137_ri_exchange_history_account_id_multicloud_test.go diff --git a/internal/config/store_postgres_db_test.go b/internal/config/store_postgres_db_test.go index 04a458be0..ba0e5544f 100644 --- a/internal/config/store_postgres_db_test.go +++ b/internal/config/store_postgres_db_test.go @@ -1204,11 +1204,10 @@ func TestPostgresStoreDB_SaveRIExchangeRecord_DefaultsEmptyPaymentDue(t *testing ctx := context.Background() record := &RIExchangeRecord{ - // AccountID must satisfy the ri_exchange_history schema - // invariants: VARCHAR(20) + CHECK (account_id ~ '^\d{12}$'). - // The earlier test value "payment-due-test-account" tripped both - // the length cap and the regex check — replaced with a - // 12-digit AWS-style account ID. + // A 12-digit AWS-style account ID, the shape every row written + // by the AWS exchange path carries (enforced in Go by the + // server's exchange store adapter since migration 000137 + // dropped the column's AWS-only CHECK, issue #144). AccountID: "123456789012", ExchangeID: "payment-due-test-exchange", Region: "us-east-1", diff --git a/internal/database/postgres/migrations/000137_ri_exchange_history_account_id_multicloud.down.sql b/internal/database/postgres/migrations/000137_ri_exchange_history_account_id_multicloud.down.sql new file mode 100644 index 000000000..2702f35f8 --- /dev/null +++ b/internal/database/postgres/migrations/000137_ri_exchange_history_account_id_multicloud.down.sql @@ -0,0 +1,75 @@ +-- Reverse 000137: narrow ri_exchange_history.account_id back to VARCHAR(20) +-- and restore CHECK (account_id ~ '^\d{12}$'). +-- +-- Lossy by nature, so it FAILS LOUDLY instead of truncating or deleting. Once +-- 000137 is applied a row may carry an Azure subscription GUID or a GCP project +-- ID; neither can satisfy the restored CHECK, and the GUID cannot fit +-- VARCHAR(20). An exchange-history row is the audit record of money already +-- moved, so the rollback refuses while any account_id is not a 12-digit AWS +-- account ID, names the count, and leaves the decision to the operator. +-- +-- Reverse exactly what the up did, nothing more: narrow only from VARCHAR(255) +-- (atttypmod 259), which is what the up produces; a wider or unbounded column +-- was not created by 000137 and is left alone. Re-add the CHECK only when no +-- equivalent one is present, under the name 000009 gave it. +-- +-- The table lock is taken first so no row can be inserted between the count +-- and the ALTERs (the same reasoning as the up migration's lock). Unlike the +-- up, narrowing a varchar is NOT binary-coercible: it rewrites the table and +-- rebuilds idx_ri_exchange_history_account, and ADD CONSTRAINT scans every row. +-- +-- Recovering from a refused rollback: golang-migrate marks the version dirty +-- before running this file and maybeAutoHealDirty will not clear it. This file +-- raises before changing the schema, so the database still matches 000137. +-- Set CUDLY_FORCE_MIGRATION_VERSION=137 (the CURRENT version), redeploy, remove +-- the env var, then archive or reconcile the non-AWS rows if the rollback is +-- still wanted. + +DO $$ +DECLARE + typmod INTEGER; + col SMALLINT; + non_aws BIGINT; + existing INTEGER; +BEGIN + LOCK TABLE ri_exchange_history IN ACCESS EXCLUSIVE MODE; + + SELECT count(*) INTO non_aws + FROM ri_exchange_history + WHERE account_id !~ '^\d{12}$'; + + IF non_aws > 0 THEN + RAISE EXCEPTION + 'refusing to restore the AWS-only account_id constraint on ri_exchange_history: ' + '% row(s) hold an account ID that is not a 12-digit AWS account ID ' + '(an Azure subscription GUID or GCP project ID). Narrowing would drop ' + 'or truncate exchange audit rows. Archive or reconcile those rows before ' + 'rolling back migration 000137.', + non_aws; + END IF; + + SELECT a.atttypmod, a.attnum INTO typmod, col + FROM pg_attribute a + WHERE a.attrelid = 'ri_exchange_history'::regclass + AND a.attname = 'account_id' + AND NOT a.attisdropped; + + -- 259 == VARCHAR(255) (atttypmod is n + 4). + IF typmod = 259 THEN + ALTER TABLE ri_exchange_history + ALTER COLUMN account_id TYPE VARCHAR(20); + END IF; + + SELECT count(*) INTO existing + FROM pg_constraint c + WHERE c.conrelid = 'ri_exchange_history'::regclass + AND c.contype = 'c' + AND c.conkey = ARRAY[col] + AND position('^\d{12}$' IN pg_get_constraintdef(c.oid)) > 0; + + IF existing = 0 THEN + ALTER TABLE ri_exchange_history + ADD CONSTRAINT ri_exchange_history_account_id_check + CHECK (account_id ~ '^\d{12}$'); + END IF; +END $$; diff --git a/internal/database/postgres/migrations/000137_ri_exchange_history_account_id_multicloud.up.sql b/internal/database/postgres/migrations/000137_ri_exchange_history_account_id_multicloud.up.sql new file mode 100644 index 000000000..759c4d038 --- /dev/null +++ b/internal/database/postgres/migrations/000137_ri_exchange_history_account_id_multicloud.up.sql @@ -0,0 +1,137 @@ +-- Migration 000137: let ri_exchange_history.account_id hold any cloud's +-- account identifier (issue #144). +-- +-- Since 000009 the column has been +-- +-- account_id VARCHAR(20) NOT NULL CHECK (account_id ~ '^\d{12}$') +-- +-- which is the exact shape of an AWS account ID and nothing else. An Azure +-- subscription ID is a 36-character GUID; a GCP project ID is 6 to 30 +-- lowercase letters, digits and hyphens. There are two independent barriers +-- and BOTH have to go: +-- +-- 1. VARCHAR(20) rejects a 36-character subscription GUID (SQLSTATE 22001). +-- 2. The regex CHECK rejects every non-AWS value, including a short GCP +-- project ID that fits in 20 characters, and would keep rejecting an +-- Azure GUID even at VARCHAR(255) (SQLSTATE 23514). +-- +-- A migration that only widened the column would pass a width test and still +-- reject every Azure and GCP row. This one widens the column to +-- VARCHAR(255), matching cloud_accounts.external_id, +-- savings_snapshots.account_id (000067) and purchase_history.account_id +-- (000095), and drops the AWS-only CHECK. +-- +-- No replacement CHECK: AWS account IDs, Azure subscription GUIDs and GCP +-- project IDs share no common shape, and the table has no provider column to +-- key a provider-aware constraint on. The shape is validated in Go at the +-- write boundary instead, where the provider is known: the only writer today +-- is the AWS exchange store adapter (internal/server/handler_ri_exchange.go), +-- which refuses anything but a 12-digit AWS account ID, so AWS rows keep +-- exactly the guarantee this CHECK gave them -- including refusing the +-- "unknown" placeholder resolveAccountID returns when STS fails. This matches +-- purchase_history.account_id, which has never had a CHECK. +-- +-- Today no non-AWS value can reach this column (the Azure exchange path writes +-- no ri_exchange_history row), so nothing is broken yet; this lands the schema +-- change before a multi-cloud writer does, not after its first lost row. +-- +-- ------------------------------------------------------------------ +-- Guards +-- ------------------------------------------------------------------ +-- Correct on a fresh database, an already-deployed one, a re-run under the +-- auto-heal path, and a partially fixed one (widened but still CHECKed, or +-- unCHECKed but still narrow): each half is conditioned on the observed +-- catalog state, because IF NOT EXISTS cannot repair a wrong column type or +-- an existing constraint. +-- +-- The table is resolved through 'ri_exchange_history'::regclass, which follows +-- search_path exactly like the bare ALTER TABLE statements below do (see +-- 000095 for why that matters). The column's absence is a hard failure, and +-- both halves are re-checked from the catalog afterwards, so this migration +-- can never be recorded as applied without having done the work. +-- +-- The CHECK is found by catalog, not by name: the inline constraint in 000009 +-- was auto-named ri_exchange_history_account_id_check, but a database restored +-- or hand-repaired from a dump may carry another name. Only single-column CHECKs +-- on account_id that contain the 12-digit regex are dropped. +-- +-- ------------------------------------------------------------------ +-- Concurrency +-- ------------------------------------------------------------------ +-- golang-migrate serializes its own runs with an advisory lock, but the file is +-- also re-run outside it (auto-heal, an operator replaying it by hand). The +-- table lock is taken FIRST, before any catalog read: otherwise a second run +-- that started while the first was still uncommitted would read the +-- pre-commit catalog, see a CHECK the first run has already dropped, block on +-- that run's lock and then fail with "constraint does not exist" once it +-- commits. Under READ COMMITTED every statement in the DO block takes a fresh +-- snapshot, so once the lock is granted all reads below see the committed +-- state. The lock is held to the end of the migration's transaction. +-- +-- Cost: widening a varchar is binary-coercible, so Postgres skips the table +-- rewrite and the rebuild of idx_ri_exchange_history_account (000009), and +-- dropping a CHECK is catalog-only. ACCESS EXCLUSIVE is held only for that. + +DO $$ +DECLARE + typmod INTEGER; + col SMALLINT; + con RECORD; + remaining INTEGER; +BEGIN + LOCK TABLE ri_exchange_history IN ACCESS EXCLUSIVE MODE; + + SELECT a.atttypmod, a.attnum INTO typmod, col + FROM pg_attribute a + WHERE a.attrelid = 'ri_exchange_history'::regclass + AND a.attname = 'account_id' + AND NOT a.attisdropped; + + IF col IS NULL THEN + RAISE EXCEPTION + 'migration 000137: column ri_exchange_history.account_id does not exist'; + END IF; + + -- atttypmod is n + 4 for VARCHAR(n); -1 is unbounded varchar, which is + -- already wider than VARCHAR(255) and must not be narrowed. + IF typmod <> -1 AND typmod - 4 < 255 THEN + ALTER TABLE ri_exchange_history + ALTER COLUMN account_id TYPE VARCHAR(255); + END IF; + + FOR con IN + SELECT c.conname + FROM pg_constraint c + WHERE c.conrelid = 'ri_exchange_history'::regclass + AND c.contype = 'c' + AND c.conkey = ARRAY[col] + AND position('^\d{12}$' IN pg_get_constraintdef(c.oid)) > 0 + LOOP + EXECUTE format('ALTER TABLE ri_exchange_history DROP CONSTRAINT %I', con.conname); + END LOOP; + + -- Post-checks, read back from the catalog. + SELECT a.atttypmod INTO typmod + FROM pg_attribute a + WHERE a.attrelid = 'ri_exchange_history'::regclass + AND a.attname = 'account_id' + AND NOT a.attisdropped; + + IF typmod <> -1 AND typmod - 4 < 255 THEN + RAISE EXCEPTION + 'migration 000137 failed to widen ri_exchange_history.account_id: still VARCHAR(%)', + typmod - 4; + END IF; + + SELECT count(*) INTO remaining + FROM pg_constraint c + WHERE c.conrelid = 'ri_exchange_history'::regclass + AND c.contype = 'c' + AND c.conkey = ARRAY[col] + AND position('^\d{12}$' IN pg_get_constraintdef(c.oid)) > 0; + + IF remaining > 0 THEN + RAISE EXCEPTION + 'migration 000137 failed to drop the AWS-only CHECK on ri_exchange_history.account_id'; + END IF; +END $$; diff --git a/internal/database/postgres/migrations/000137_ri_exchange_history_account_id_multicloud_test.go b/internal/database/postgres/migrations/000137_ri_exchange_history_account_id_multicloud_test.go new file mode 100644 index 000000000..f24c1a52b --- /dev/null +++ b/internal/database/postgres/migrations/000137_ri_exchange_history_account_id_multicloud_test.go @@ -0,0 +1,285 @@ +//go:build integration + +package migrations_test + +import ( + "context" + "errors" + "os" + "path/filepath" + "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/assert" + "github.com/stretchr/testify/require" +) + +const migration137 = 137 + +// Account identifiers ri_exchange_history.account_id must hold once exchanges +// are persisted for every cloud (issue #144). +const ( + exchAWSID = "123456789012" // the only shape 000009 allowed + exchAzureSub = "3f2504e0-4f89-11d3-9a0c-0305e82c3301" // 36 chars: over VARCHAR(20) and not 12 digits + exchGCPProject = "cudly-production-analytics-001" // 30 chars, the documented GCP maximum + exchGCPShortProj = "cudly-prod-0042" // fits VARCHAR(20); only the CHECK rejects it +) + +// insertExchangeRow issues an INSERT carrying every NOT NULL column of +// ri_exchange_history that has no default, the same column set +// SaveRIExchangeRecord always writes. +func insertExchangeRow(ctx context.Context, pool *pgxpool.Pool, accountID string) error { + _, err := pool.Exec(ctx, ` + INSERT INTO ri_exchange_history ( + account_id, region, source_ri_ids, source_instance_type, source_count, + target_offering_id, target_instance_type, target_count + ) VALUES ($1, 'eastus', ARRAY['src-1'], 'Standard_D2s_v3', 1, 'offer-1', 'Standard_D4s_v3', 1)`, + accountID) + return err +} + +func requireSQLState(t *testing.T, err error, code, why string) { + t.Helper() + require.Error(t, err, why) + var pgErr *pgconn.PgError + require.True(t, errors.As(err, &pgErr), "expected a Postgres error, got %v", err) + assert.Equal(t, code, pgErr.Code, why) +} + +// exchangeAccountIDWidth reads the column width from information_schema, an +// independent probe from the pg_attribute lookup the migration itself uses. +// It returns -1 for unbounded varchar. +func exchangeAccountIDWidth(ctx context.Context, t *testing.T, pool *pgxpool.Pool) int { + t.Helper() + var width *int + require.NoError(t, pool.QueryRow(ctx, ` + SELECT character_maximum_length FROM information_schema.columns + WHERE table_schema = current_schema() + AND table_name = 'ri_exchange_history' AND column_name = 'account_id'`).Scan(&width)) + if width == nil { + return -1 + } + return *width +} + +// requireAcceptsEveryCloud asserts the post-fix contract by behavior: an AWS, +// Azure and GCP identifier each insert and round-trip untruncated. +func requireAcceptsEveryCloud(ctx context.Context, t *testing.T, pool *pgxpool.Pool) { + t.Helper() + for _, id := range []string{exchAWSID, exchAzureSub, exchGCPProject, exchGCPShortProj} { + require.NoError(t, insertExchangeRow(ctx, pool, id), "account_id %q must be accepted", id) + var n int + require.NoError(t, pool.QueryRow(ctx, + `SELECT count(*) FROM ri_exchange_history WHERE account_id = $1`, id).Scan(&n)) + assert.Positive(t, n, "account_id %q must round-trip untruncated", id) + } +} + +func readMigration137(t *testing.T, direction string) string { + t.Helper() + sql, err := os.ReadFile(filepath.Join(getMigrationsPath(), + "000137_ri_exchange_history_account_id_multicloud."+direction+".sql")) + require.NoError(t, err) + return string(sql) +} + +func startMigrationDB(t *testing.T) (context.Context, *pgxpool.Pool) { + t.Helper() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + t.Cleanup(cancel) + container := testhelpers.RequirePostgresContainer(ctx, t) + t.Cleanup(func() { _ = container.Cleanup(context.Background()) }) + return ctx, container.DB.Pool() +} + +// TestMigration137_RIExchangeHistoryAccountIDMultiCloud is the +// fail-before/pass-after test for issue #144: before 000137 an Azure +// subscription ID is rejected by the width (22001) and a GCP project ID that +// fits the width is still rejected by the CHECK (23514); after it, every cloud's +// identifier is stored. +func TestMigration137_RIExchangeHistoryAccountIDMultiCloud(t *testing.T) { + ctx, pool := startMigrationDB(t) + path := getMigrationsPath() + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, previousMigrationVersion(t, migration137))) + require.Equal(t, 20, exchangeAccountIDWidth(ctx, t, pool)) + require.NoError(t, insertExchangeRow(ctx, pool, exchAWSID), "an AWS account ID always fit") + requireSQLState(t, insertExchangeRow(ctx, pool, exchAzureSub), "22001", + "pre-fix: an Azure subscription GUID overflows VARCHAR(20)") + requireSQLState(t, insertExchangeRow(ctx, pool, exchGCPShortProj), "23514", + "pre-fix: a GCP project ID that fits the width is rejected by the AWS-only CHECK") + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, migration137)) + assert.Equal(t, 255, exchangeAccountIDWidth(ctx, t, pool)) + requireAcceptsEveryCloud(ctx, t, pool) + + _, err := pool.Exec(ctx, readMigration137(t, "up")) + require.NoError(t, err, "re-running the up migration must be a no-op") + assert.Equal(t, 255, exchangeAccountIDWidth(ctx, t, pool)) +} + +// TestMigration137_RepairsPartialStates runs the up SQL against databases where +// only one barrier, or an unexpected variant of one, is present. Widening +// alone is not a fix (the CHECK still rejects Azure at any width), so each case +// must end with every cloud's identifier accepted. +func TestMigration137_RepairsPartialStates(t *testing.T) { + ctx, pool := startMigrationDB(t) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, getMigrationsPath(), + previousMigrationVersion(t, migration137))) + up := readMigration137(t, "up") + + for _, tc := range []struct { + name string + setup string + wantWidth int + }{ + {"widened but still checked", + `ALTER TABLE ri_exchange_history ALTER COLUMN account_id TYPE VARCHAR(255)`, 255}, + {"check dropped but still narrow", + `ALTER TABLE ri_exchange_history DROP CONSTRAINT ri_exchange_history_account_id_check`, 255}, + {"check under another name", + `ALTER TABLE ri_exchange_history RENAME CONSTRAINT ri_exchange_history_account_id_check TO restored_acct_shape`, 255}, + {"unbounded varchar is left wider", + `ALTER TABLE ri_exchange_history ALTER COLUMN account_id TYPE VARCHAR`, -1}, + {"wider than 255 is not narrowed", + `ALTER TABLE ri_exchange_history ALTER COLUMN account_id TYPE VARCHAR(300)`, 300}, + } { + t.Run(tc.name, func(t *testing.T) { + resetExchangeAccountIDTo000009(ctx, t, pool) + _, err := pool.Exec(ctx, tc.setup) + require.NoError(t, err) + + _, err = pool.Exec(ctx, up) + require.NoError(t, err) + assert.Equal(t, tc.wantWidth, exchangeAccountIDWidth(ctx, t, pool)) + requireAcceptsEveryCloud(ctx, t, pool) + }) + } +} + +// resetExchangeAccountIDTo000009 puts the column back to exactly what 000009 +// created, so each partial-state case starts from the same schema. +func resetExchangeAccountIDTo000009(ctx context.Context, t *testing.T, pool *pgxpool.Pool) { + t.Helper() + _, err := pool.Exec(ctx, ` + DELETE FROM ri_exchange_history; + ALTER TABLE ri_exchange_history + DROP CONSTRAINT IF EXISTS ri_exchange_history_account_id_check, + DROP CONSTRAINT IF EXISTS restored_acct_shape, + ALTER COLUMN account_id TYPE VARCHAR(20), + ADD CONSTRAINT ri_exchange_history_account_id_check CHECK (account_id ~ '^\d{12}$');`) + require.NoError(t, err) +} + +// TestMigration137_ConcurrentRunsSerialize races two runs of the up SQL (the +// auto-heal or a manual replay running alongside a deploy). The second run +// starts while the first has done all its work but not committed. Without the +// table lock taken before the catalog reads, the second run sees the CHECK the +// first has already dropped, waits on the first run's lock, and then fails with +// "constraint does not exist" once it commits. +func TestMigration137_ConcurrentRunsSerialize(t *testing.T) { + ctx, pool := startMigrationDB(t) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, getMigrationsPath(), + previousMigrationVersion(t, migration137))) + up := readMigration137(t, "up") + + for _, tc := range []struct{ name, setup string }{ + {"from the 000009 schema", ``}, + {"from widened but still checked", + `ALTER TABLE ri_exchange_history ALTER COLUMN account_id TYPE VARCHAR(255)`}, + } { + t.Run(tc.name, func(t *testing.T) { + resetExchangeAccountIDTo000009(ctx, t, pool) + if tc.setup != "" { + _, err := pool.Exec(ctx, tc.setup) + require.NoError(t, err) + } + + first, err := pool.Begin(ctx) + require.NoError(t, err) + defer func() { _ = first.Rollback(context.Background()) }() + _, err = first.Exec(ctx, up) + require.NoError(t, err) + + second := make(chan error, 1) + go func() { + _, execErr := pool.Exec(ctx, up) + second <- execErr + }() + + require.Eventually(t, func() bool { + var waiting int + if qErr := pool.QueryRow(ctx, ` + SELECT count(*) FROM pg_locks + WHERE NOT granted AND relation = 'ri_exchange_history'::regclass`).Scan(&waiting); qErr != nil { + return false + } + return waiting > 0 + }, 30*time.Second, 50*time.Millisecond, "the second run must block on the first") + + require.NoError(t, first.Commit(ctx)) + select { + case execErr := <-second: + require.NoError(t, execErr, "the second run must see the committed state and no-op") + case <-time.After(30 * time.Second): + t.Fatal("the second run did not finish after the first committed") + } + assert.Equal(t, 255, exchangeAccountIDWidth(ctx, t, pool)) + requireAcceptsEveryCloud(ctx, t, pool) + }) + } +} + +// TestMigration137_DownRestoresAWSConstraintWhenSafe covers the rollback's +// success path: with only AWS rows present it narrows the column and puts the +// CHECK back, and the up applies again afterwards. +func TestMigration137_DownRestoresAWSConstraintWhenSafe(t *testing.T) { + ctx, pool := startMigrationDB(t) + path := getMigrationsPath() + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, migration137)) + require.NoError(t, insertExchangeRow(ctx, pool, exchAWSID)) + + // One step, so exactly 000137's down runs. + require.NoError(t, migrations.RollbackMigrations(ctx, pool, path, 1)) + assert.Equal(t, 20, exchangeAccountIDWidth(ctx, t, pool), "the rollback must narrow the column") + requireSQLState(t, insertExchangeRow(ctx, pool, exchGCPShortProj), "23514", + "the rollback must restore the AWS-only CHECK") + var stored string + require.NoError(t, pool.QueryRow(ctx, `SELECT account_id FROM ri_exchange_history`).Scan(&stored)) + assert.Equal(t, exchAWSID, stored, "the lossless rollback must keep the AWS row") + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, migration137), + "the up must re-apply after a rollback") + requireAcceptsEveryCloud(ctx, t, pool) +} + +// TestMigration137_DownRefusesNonAWSRows: the rollback must fail loudly, with +// the rows intact, rather than truncate or drop exchange audit rows. The GCP +// case fits VARCHAR(20), so only the CHECK guard catches it. +func TestMigration137_DownRefusesNonAWSRows(t *testing.T) { + for _, id := range []string{exchAzureSub, exchGCPShortProj} { + t.Run(id, func(t *testing.T) { + ctx, pool := startMigrationDB(t) + path := getMigrationsPath() + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, migration137)) + require.NoError(t, insertExchangeRow(ctx, pool, id)) + + err := migrations.RollbackMigrations(ctx, pool, path, 1) + require.Error(t, err, "rolling back with a non-AWS row present must fail") + // The migrate error embeds the file's SQL text, so match the + // formatted row count: it appears only in the raised message. + assert.Contains(t, err.Error(), "1 row(s) hold an account ID that is not a 12-digit AWS account ID") + + assert.Equal(t, 255, exchangeAccountIDWidth(ctx, t, pool), "the refused rollback must not narrow") + var stored string + require.NoError(t, pool.QueryRow(ctx, `SELECT account_id FROM ri_exchange_history`).Scan(&stored)) + assert.Equal(t, id, stored, "the refused rollback must leave the row untouched") + }) + } +} diff --git a/internal/server/handler_ri_exchange.go b/internal/server/handler_ri_exchange.go index 73406b5bb..201f110d8 100644 --- a/internal/server/handler_ri_exchange.go +++ b/internal/server/handler_ri_exchange.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "log" + "regexp" "time" "github.com/aws/aws-sdk-go-v2/aws" @@ -304,7 +305,29 @@ func newConfigExchangeStoreAdapter(store config.StoreInterface) *configExchangeS return &configExchangeStoreAdapter{store: store} } +// awsAccountIDPattern is the shape of an AWS account ID: exactly 12 ASCII digits +// (RE2's \d is ASCII-only). +var awsAccountIDPattern = regexp.MustCompile(`^\d{12}$`) + +// requireAWSExchangeAccountID refuses an exchange record whose account ID is not +// an AWS account ID. Every record this adapter writes comes from the AWS EC2 +// exchange engine, so its account ID must be the 12-digit account STS +// reported. ri_exchange_history.account_id itself no longer enforces that +// shape (migration 000137 dropped the AWS-only CHECK so the column can hold +// Azure and GCP identifiers, issue #144); this check keeps the guarantee for +// AWS rows at the boundary where the provider is known, including refusing the +// "unknown" placeholder resolveAccountID falls back to when STS fails. +func requireAWSExchangeAccountID(accountID string) error { + if !awsAccountIDPattern.MatchString(accountID) { + return fmt.Errorf("refusing to record RI exchange: account ID %q is not a 12-digit AWS account ID", accountID) + } + return nil +} + func (a *configExchangeStoreAdapter) SaveRIExchangeRecord(ctx context.Context, record *exchange.ExchangeRecord) error { + if err := requireAWSExchangeAccountID(record.AccountID); err != nil { + return err + } cfgRecord := exchangeToConfigRecord(record) if err := a.store.SaveRIExchangeRecord(ctx, cfgRecord); err != nil { return err @@ -314,6 +337,9 @@ func (a *configExchangeStoreAdapter) SaveRIExchangeRecord(ctx context.Context, r } func (a *configExchangeStoreAdapter) ReserveRIExchange(ctx context.Context, record *exchange.ExchangeRecord, dailyCapUSD, perExchangeCapUSD string) (string, error) { + if err := requireAWSExchangeAccountID(record.AccountID); err != nil { + return "", err + } cfgRecord := exchangeToConfigRecord(record) ceiling, err := a.store.ReserveRIExchange(ctx, cfgRecord, dailyCapUSD, perExchangeCapUSD) if err != nil { diff --git a/internal/server/handler_ri_exchange_test.go b/internal/server/handler_ri_exchange_test.go index 5ab105a12..0e00b6bcc 100644 --- a/internal/server/handler_ri_exchange_test.go +++ b/internal/server/handler_ri_exchange_test.go @@ -226,7 +226,7 @@ func TestConfigExchangeStoreAdapter(t *testing.T) { t.Run("SaveRIExchangeRecord", func(t *testing.T) { ctx := testutil.TestContext(t) record := &exchange.ExchangeRecord{ - AccountID: "123456789", + AccountID: "123456789012", Region: "us-east-1", SourceRIIDs: []string{"ri-123"}, SourceInstanceType: "m5.large", @@ -244,7 +244,7 @@ func TestConfigExchangeStoreAdapter(t *testing.T) { if savedRecord == nil { t.Fatal("expected record to be saved") } - testutil.AssertEqual(t, "123456789", savedRecord.AccountID) + testutil.AssertEqual(t, "123456789012", savedRecord.AccountID) testutil.AssertEqual(t, "m5.large", savedRecord.SourceInstanceType) testutil.AssertEqual(t, "m6i.large", savedRecord.TargetInstanceType) testutil.AssertEqual(t, "saved-id", record.ID) @@ -252,7 +252,10 @@ func TestConfigExchangeStoreAdapter(t *testing.T) { t.Run("ReserveRIExchange", func(t *testing.T) { ladderRunID := "ladder-run-1" - record := &exchange.ExchangeRecord{PaymentDue: "5", Status: "processing", Mode: "auto", LadderRunID: &ladderRunID} + record := &exchange.ExchangeRecord{ + AccountID: "123456789012", + PaymentDue: "5", Status: "processing", Mode: "auto", LadderRunID: &ladderRunID, + } ceiling, err := adapter.ReserveRIExchange(context.Background(), record, "20", "10") testutil.AssertNoError(t, err) testutil.AssertEqual(t, "10.000000", ceiling) @@ -281,6 +284,48 @@ func TestConfigExchangeStoreAdapter(t *testing.T) { }) } +// TestConfigExchangeStoreAdapter_RefusesNonAWSAccountID pins the Go-side half +// of issue #144. Migration 000137 dropped the AWS-only CHECK on +// ri_exchange_history.account_id so the column can hold Azure and GCP +// identifiers; the AWS exchange adapter must keep refusing anything that is not +// a 12-digit AWS account ID before it reaches the store, or the STS-failure +// placeholder "unknown" would start landing in the exchange audit table. +func TestConfigExchangeStoreAdapter_RefusesNonAWSAccountID(t *testing.T) { + storeCalls := 0 + adapter := newConfigExchangeStoreAdapter(&mockConfigStoreForExchange{ + saveRIExchangeRecordFunc: func(context.Context, *config.RIExchangeRecord) error { + storeCalls++ + return nil + }, + reserveRIExchangeFunc: func(context.Context, *config.RIExchangeRecord, string, string) (string, error) { + storeCalls++ + return "0", nil + }, + }) + + for _, accountID := range []string{ + "", + "unknown", // resolveAccountID's STS-failure placeholder + "12345678901", + "1234567890123", + "3f2504e0-4f89-11d3-9a0c-0305e82c3301", // Azure subscription GUID + "cudly-prod-0042", // GCP project ID + "123456789012\n", + "١٢٣٤٥٦٧٨٩٠١٢", // 12 non-ASCII digits + } { + t.Run(fmt.Sprintf("%q", accountID), func(t *testing.T) { + saveErr := adapter.SaveRIExchangeRecord(context.Background(), &exchange.ExchangeRecord{AccountID: accountID}) + testutil.AssertError(t, saveErr) + testutil.AssertContains(t, saveErr.Error(), "not a 12-digit AWS account ID") + + _, reserveErr := adapter.ReserveRIExchange(context.Background(), &exchange.ExchangeRecord{AccountID: accountID}, "20", "10") + testutil.AssertError(t, reserveErr) + testutil.AssertContains(t, reserveErr.Error(), "not a 12-digit AWS account ID") + }) + } + testutil.AssertEqual(t, 0, storeCalls) +} + func TestSendExchangeNotification_NoEmailSender(t *testing.T) { app := &Application{Email: nil} result := &exchange.AutoExchangeResult{Mode: "auto"} @@ -463,7 +508,7 @@ func TestExecuteRIExchangeReshape_NoConvertibleRIs(t *testing.T) { lookupOffering: func(ctx context.Context, _ exchange.LookupOfferingParams) (string, error) { return "", nil }, - accountID: "123456789", + accountID: "123456789012", region: "us-east-1", } @@ -590,7 +635,7 @@ func TestExecuteRIExchangeReshape_ManualMode(t *testing.T) { lookupOffering: func(ctx context.Context, _ exchange.LookupOfferingParams) (string, error) { return "offering-123", nil }, - accountID: "123456789", + accountID: "123456789012", region: "us-east-1", } @@ -709,7 +754,7 @@ func TestExecuteRIExchangeReshape_AutoMode(t *testing.T) { lookupOffering: func(ctx context.Context, _ exchange.LookupOfferingParams) (string, error) { return "offering-456", nil }, - accountID: "123456789", + accountID: "123456789012", region: "us-east-1", } @@ -824,7 +869,7 @@ func TestExecuteRIExchangeReshape_DailyCapHitMidRun(t *testing.T) { lookupOffering: func(ctx context.Context, _ exchange.LookupOfferingParams) (string, error) { return "offering-cap-test", nil }, - accountID: "123456789", + accountID: "123456789012", region: "us-east-1", }