From 7892019d9e7469c2e3a7c4c980734f7c63b64fa9 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 12 Oct 2026 23:53:06 +0200 Subject: [PATCH] Copy each store test's database from a template migrated once, and hash test bus passwords at bcrypt's least cost Running ~90 migrations into a fresh database per store test, and bcrypt at the default cost under the race detector, were most of the controller's check (novox/hq issue 591). --- internal/identity/fortest.go | 15 +-- internal/inventory/bususers.go | 21 ++++- internal/inventory/bususers_test.go | 10 ++ internal/inventory/fortest.go | 17 ++-- internal/licences/fortest.go | 15 +-- internal/store/fortest.go | 138 ++++++++++++++++++++++++++++ 6 files changed, 183 insertions(+), 33 deletions(-) create mode 100644 internal/store/fortest.go diff --git a/internal/identity/fortest.go b/internal/identity/fortest.go index 9542c279..0dbeab61 100644 --- a/internal/identity/fortest.go +++ b/internal/identity/fortest.go @@ -29,14 +29,13 @@ func ForTest(t *testing.T) *Identity { if len(name) > 60 { name = name[:60] } - conn, err := pgx.Connect(t.Context(), admin) + // Created already migrated, a copy of a template migrated once (novox/hq issue 591); the Migrate + // below then finds nothing pending, which it does only if the copy's record matches every migration. + migrations, err := Migrations() if err != nil { - t.Fatalf("cannot reach the test PostgreSQL: %v", err) + t.Fatal(err) } - if _, err := conn.Exec(t.Context(), "create database "+name); err != nil { - t.Fatalf("cannot create %s: %v", name, err) - } - conn.Close(t.Context()) + store.CreateMigratedForTest(t, admin, name, migrations) cut := strings.LastIndex(admin, "/") t.Setenv(store.Variable(Name), admin[:cut]+"/"+name+"?sslmode=disable") @@ -57,10 +56,6 @@ func ForTest(t *testing.T) *Identity { if err := ident.Ready(t.Context(), 20*time.Second); err != nil { t.Fatal(err) } - migrations, err := Migrations() - if err != nil { - t.Fatal(err) - } if _, err := ident.store.Migrate(t.Context(), migrations); err != nil { t.Fatal(err) } diff --git a/internal/inventory/bususers.go b/internal/inventory/bususers.go index b0f661f5..e5ef9dc4 100644 --- a/internal/inventory/bususers.go +++ b/internal/inventory/bususers.go @@ -6,6 +6,7 @@ import ( "encoding/base64" "errors" "fmt" + "sync/atomic" "github.com/jackc/pgx/v5" "golang.org/x/crypto/bcrypt" @@ -23,6 +24,20 @@ import ( // access change would blank every module's password if the mesh kept nothing (design 25 §4, and the // migration beside this). +// busPasswordCostForTest, when set, is the bcrypt cost a bus password is hashed at instead of the +// library's default. Only ForTest sets it, to bcrypt's least (novox/hq issue 591): at the default +// cost one hash takes most of a second under the race detector, and a hash at the least cost is the +// same kind of hash, checked by the same comparison and by the bus the same way. +var busPasswordCostForTest atomic.Int32 + +// busPasswordCost is the bcrypt cost a bus password is hashed at: the library's default, but in a test. +func busPasswordCost() int { + if c := busPasswordCostForTest.Load(); c != 0 { + return int(c) + } + return bcrypt.DefaultCost +} + // BusUser is one user of the bus, as the mesh records it. type BusUser struct { Username string @@ -67,7 +82,7 @@ func (i *Inventory) RecordBusPassword(ctx context.Context, u BusUser, password s if password == "" { return errors.New("a bus user needs a password") } - hash, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.DefaultCost) + hash, err := bcrypt.GenerateFromPassword([]byte(password), busPasswordCost()) if err != nil { return fmt.Errorf("cannot hash a bus password: %w", err) } @@ -101,7 +116,7 @@ func (i *Inventory) MintBusPassword(ctx context.Context, u BusUser) (string, err // The cost the server will pay on every connection. Left at the library's default rather than // raised: a node reconnecting after a network blip pays it, and the mesh's own links reconnect // far more often than a person logs in anywhere. - hash, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.DefaultCost) + hash, err := bcrypt.GenerateFromPassword([]byte(password), busPasswordCost()) if err != nil { return "", fmt.Errorf("cannot hash a bus password: %w", err) } @@ -183,7 +198,7 @@ func (i *Inventory) SeedBusUser(ctx context.Context, u BusUser, password string) if u.Username == "" || u.Kind == "" || password == "" { return errors.New("a bus user needs a username, a kind and the credential it is using") } - hash, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.DefaultCost) + hash, err := bcrypt.GenerateFromPassword([]byte(password), busPasswordCost()) if err != nil { return fmt.Errorf("cannot hash a bus password: %w", err) } diff --git a/internal/inventory/bususers_test.go b/internal/inventory/bususers_test.go index 199b6600..f330f93c 100644 --- a/internal/inventory/bususers_test.go +++ b/internal/inventory/bususers_test.go @@ -158,3 +158,13 @@ func TestACredentialTheMeshDidNotMintIsRecordedOnceAndNotOverwritten(t *testing. t.Fatal("a restart put the bootstrap credential back over a rotated one") } } + +// Outside a test a bus password is hashed at bcrypt's default cost: only ForTest lowers it (novox/hq issue 591). +func TestABusPasswordIsHashedAtTheDefaultCostOutsideATest(t *testing.T) { + was := busPasswordCostForTest.Load() + t.Cleanup(func() { busPasswordCostForTest.Store(was) }) + busPasswordCostForTest.Store(0) + if got := busPasswordCost(); got != bcrypt.DefaultCost { + t.Fatalf("a bus password is hashed at cost %d, not bcrypt's default %d", got, bcrypt.DefaultCost) + } +} diff --git a/internal/inventory/fortest.go b/internal/inventory/fortest.go index 74d58805..77aec9cc 100644 --- a/internal/inventory/fortest.go +++ b/internal/inventory/fortest.go @@ -9,6 +9,7 @@ import ( "time" "github.com/jackc/pgx/v5" + "golang.org/x/crypto/bcrypt" "github.com/novox/mesh-controller/internal/store" ) @@ -30,14 +31,14 @@ func ForTest(t *testing.T) *Inventory { name = name[:60] } - conn, err := pgx.Connect(t.Context(), admin) + // Created already migrated, a copy of a template migrated once (novox/hq issue 591); the Migrate + // below then finds nothing pending, which it does only if the copy's record matches every migration. + migrations, err := Migrations() if err != nil { - t.Fatalf("cannot reach the test PostgreSQL: %v", err) + t.Fatal(err) } - if _, err := conn.Exec(t.Context(), "create database "+name); err != nil { - t.Fatalf("cannot create %s: %v", name, err) - } - conn.Close(t.Context()) + store.CreateMigratedForTest(t, admin, name, migrations) + busPasswordCostForTest.Store(int32(bcrypt.MinCost)) cut := strings.LastIndex(admin, "/") t.Setenv(store.Variable(Name), admin[:cut]+"/"+name+"?sslmode=disable") @@ -59,10 +60,6 @@ func ForTest(t *testing.T) *Inventory { t.Fatal(err) } - migrations, err := Migrations() - if err != nil { - t.Fatal(err) - } if _, err := inv.store.Migrate(t.Context(), migrations); err != nil { t.Fatal(err) } diff --git a/internal/licences/fortest.go b/internal/licences/fortest.go index 193376cc..2bbf72c8 100644 --- a/internal/licences/fortest.go +++ b/internal/licences/fortest.go @@ -32,14 +32,13 @@ func ForTest(t *testing.T) *Licences { name = name[:60] } - conn, err := pgx.Connect(t.Context(), admin) + // Created already migrated, a copy of a template migrated once (novox/hq issue 591); the Migrate + // below then finds nothing pending, which it does only if the copy's record matches every migration. + migrations, err := Migrations() if err != nil { - t.Fatalf("cannot reach the test PostgreSQL: %v", err) + t.Fatal(err) } - if _, err := conn.Exec(t.Context(), "create database "+name); err != nil { - t.Fatalf("cannot create %s: %v", name, err) - } - conn.Close(t.Context()) + store.CreateMigratedForTest(t, admin, name, migrations) cut := strings.LastIndex(admin, "/") t.Setenv(store.Variable(Name), admin[:cut]+"/"+name+"?sslmode=disable") @@ -61,10 +60,6 @@ func ForTest(t *testing.T) *Licences { t.Fatal(err) } - migrations, err := Migrations() - if err != nil { - t.Fatal(err) - } if _, err := held.store.Migrate(t.Context(), migrations); err != nil { t.Fatal(err) } diff --git a/internal/store/fortest.go b/internal/store/fortest.go new file mode 100644 index 00000000..9727c82b --- /dev/null +++ b/internal/store/fortest.go @@ -0,0 +1,138 @@ +package store + +import ( + "context" + "crypto/sha256" + "encoding/binary" + "encoding/hex" + "errors" + "fmt" + "strings" + "sync" + "testing" + "time" + + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgconn" + "github.com/jackc/pgx/v5/pgxpool" +) + +// CreateMigratedForTest creates the database name on the test PostgreSQL that admin reaches, holding +// every one of migrations already applied and recorded, as a fresh database migrated by Migrate would. +// +// **A copy of a template, migrated once** (novox/hq issue 591): a store test used to create an empty +// database and run every migration into it, some 90 for the inventory, and with hundreds of store tests +// that alone was most of the controller's check. The template is migrated by Migrate itself, once for +// each set of migrations, and every test's database is copied from it by PostgreSQL. The caller still +// runs Migrate on the copy, which finds nothing pending only if the copy's record matches the migrations +// exactly, so a template that disagreed with them would fail the test rather than pass it unseen. +// +// The template is named after a digest of the migrations, so a changed or added migration is a new +// template, never an old one reused. Packages tested at once in separate processes share one: it is +// built under an advisory lock, and a template a crashed build left half done is not marked a template +// and is built again. It takes a *testing.T, so nothing that is not a test can call it. +func CreateMigratedForTest(t *testing.T, admin, name string, migrations []Migration) { + t.Helper() + template, err := templateFor(admin, migrations) + if err != nil { + t.Fatalf("cannot raise the migrated template: %v", err) + } + conn, err := pgx.Connect(t.Context(), admin) + if err != nil { + t.Fatalf("cannot reach the test PostgreSQL: %v", err) + } + defer conn.Close(context.Background()) + // A copy is refused while anything is connected to its template; the template allows no connection + // once built, so only the builder's last connection closing can still be in the way. + for deadline := time.Now().Add(10 * time.Second); ; { + _, err = conn.Exec(t.Context(), "create database "+name+" template "+template) + var pgErr *pgconn.PgError + if err == nil || !errors.As(err, &pgErr) || pgErr.Code != "55006" || time.Now().After(deadline) { + break + } + time.Sleep(20 * time.Millisecond) + } + if err != nil { + t.Fatalf("cannot create %s from %s: %v", name, template, err) + } +} + +// templates are the templates this process has raised or found, by name. +var templates sync.Map // name → *templateOnce + +type templateOnce struct { + once sync.Once + err error +} + +// templateFor is the name of the template holding migrations, raised if no process has yet. +func templateFor(admin string, migrations []Migration) (string, error) { + digest := sha256.New() + for _, m := range migrations { + fmt.Fprintf(digest, "%d %s %s\n", m.Number, m.Name, m.Checksum) + } + sum := digest.Sum(nil) + name := "tpl_" + hex.EncodeToString(sum[:12]) + held, _ := templates.LoadOrStore(name, &templateOnce{}) + once := held.(*templateOnce) + once.once.Do(func() { + // Not the asking test's context: the template outlives that test, and is raised once for all. + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + once.err = raiseTemplate(ctx, admin, name, int64(binary.BigEndian.Uint64(sum[12:20])), migrations) + }) + return name, once.err +} + +// raiseTemplate builds the template name under the advisory lock key, unless it stands built. +func raiseTemplate(ctx context.Context, admin, name string, key int64, migrations []Migration) error { + conn, err := pgx.Connect(ctx, admin) + if err != nil { + return err + } + defer conn.Close(context.Background()) + if _, err := conn.Exec(ctx, `select pg_advisory_lock($1)`, key); err != nil { + return err + } + defer func() { _, _ = conn.Exec(context.Background(), `select pg_advisory_unlock($1)`, key) }() + + var built bool + if err := conn.QueryRow(ctx, `select exists (select 1 from pg_database where datname = $1 and datistemplate)`, + name).Scan(&built); err != nil { + return err + } + if built { + return nil + } + if _, err := conn.Exec(ctx, "drop database if exists "+name+" with (force)"); err != nil { + return err + } + if _, err := conn.Exec(ctx, "create database "+name); err != nil { + return err + } + cut := strings.LastIndex(admin, "/") + rest := "" + if q := strings.Index(admin[cut:], "?"); q >= 0 { + rest = admin[cut+q:] + } + pool, err := pgxpool.New(ctx, admin[:cut]+"/"+name+rest) + if err != nil { + return err + } + _, err = (&Store{context: "template", pool: pool}).Migrate(ctx, migrations) + pool.Close() + if err != nil { + return err + } + // Marked a template only once whole, and closed to connections, so a copy is never refused for one. + for _, sql := range []string{ + "alter database " + name + " with allow_connections false", + "select pg_terminate_backend(pid) from pg_stat_activity where datname = '" + name + "'", + "alter database " + name + " with is_template true", + } { + if _, err := conn.Exec(ctx, sql); err != nil { + return err + } + } + return nil +}