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", }