From 2ca9522f9aa5c684451dbf072c2b797e86bbb483 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Sun, 23 Aug 2026 22:32:17 -0700 Subject: [PATCH] Fix two defects a production-clone dress rehearsal exposed Streamed a clone of a production store into the smoke VM - 3.6 GB, 12,361 settings, 6 accounts across 9 domains - and migrated it 0.15.5 -> 0.16.14 with the tool's own phases. The migration succeeded. Two defects surfaced that no smaller instance could have shown, plus one finding worth recording. 1. Account roles broke on production-shaped names. v0.16 stores an account as a local part plus a domain reference: a v0.15 account named "john@example.net" becomes name "john" with a domainId. The generator passed the full address and the server rejected it outright ("Invalid email local part"), failing the apply. The smoke instance used bare usernames - alice, bob - and never exercised this. Fixed to use the local part. And because local parts are unique only within a domain - postmaster@a.com and postmaster@b.com both become "postmaster" - an ambiguous one is now refused with a warning rather than risking an upsert that grants Admin to the wrong account. Verified on the clone: the one admin came out with roles {"@type": "Admin"} and the other five accounts untouched. 2. Cutover's health check conflated liveness with credentials. A config fallback-admin does not survive the migration - v0.16's config is a store pointer, so the old [authentication.fallback-admin] block simply ceases to exist - so the credentials supplied for the pre-migration instance came back 401 on the migrated one, and the check reported the service as never having answered. It had answered; it was up and serving on all ten ports. Liveness and credentials are now separate: any response proves the service is up, and credentials that stopped working are a warning that names this cause. Also recorded: a failed apply leaves the store in bootstrap mode, where only Bootstrap objects are accessible. A half-applied plan is not a partially configured server but an unusable one. Timing, which is the other reason to rehearse: the recovery-mode conversion of that 3.6 GB store took 2 seconds. A migration window is dominated by waiting and verification, not data volume. No production data in this commit; fixtures use example.net and the shapes involved. --- ARCHITECTURE.md | 35 ++++++++++++++ internal/applyplan/roles.go | 83 ++++++++++++++++++++++++++------ internal/applyplan/roles_test.go | 50 +++++++++++++++++++ internal/cutover/cutover.go | 18 ++++++- internal/cutover/cutover_test.go | 40 ++++++++++++--- internal/stalwartapi/task.go | 40 +++++++++++++++ 6 files changed, 245 insertions(+), 21 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index d5cc8bb..1076b72 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -794,6 +794,41 @@ happens to need them. `preflight.DeploymentKind` is a type alias for them and refusing. - **Nothing prevents concurrent runs.** Two invocations against the same run-id would both proceed; there's no lock file or equivalent. +- **A full dress rehearsal has been run against a clone of production** - + the real 3.6 GB store, 12,361 settings, 6 accounts across 9 domains, + streamed into the smoke VM and migrated 0.15.5 -> 0.16.14 with the tool's + own phases. It succeeded, and the timings are the useful part: the + recovery-mode conversion of that store took **2 seconds**, and the whole + sequence from service-stop to service-start was seconds of work. A + migration window is dominated by waiting and verification, not by data + volume - worth knowing before scheduling one around store size. + + Four things it found that smaller instances could not: + + - **Account roles broke on production-shaped names.** v0.16 stores an + account as a local part plus a domain reference; the generator was + passing v0.15's full address and the server rejected it ("Invalid email + local part"). The smoke instance used bare usernames and never + exercised it. Fixed - and because local parts are unique only within a + domain, an ambiguous one is now refused with a warning rather than + risking Admin landing on the wrong account. + - **A failed apply leaves the store in bootstrap mode.** After a partial + apply the instance answers every management call with "The server is in + bootstrap mode. Only the 'Bootstrap' object type can be accessed until + the bootstrap process is complete." So a half-applied plan is not a + partially configured server, it is an unusable one, which raises the + stakes on apply failures considerably. + - **A config fallback-admin does not survive the migration.** v0.16's + config is a store pointer, so an `[authentication.fallback-admin]` + block in the old config.toml simply ceases to exist. The credentials an + operator supplies for the pre-migration instance therefore stop working + on the migrated one, and cutover's health check - which authenticated - + failed a cutover that had actually succeeded. Liveness and credentials + are now separate questions: any response proves the service is up, and + credentials that no longer work are a warning naming this cause. + - `tenant-admin` has no v0.16 equivalent and is reported as unrestorable + rather than silently dropped. + - **`rehearse` (ยง4.9) is designed but not built.** The command is still `run --dry-run` with the old sandbox-cloning shape. Building it is mostly deletion: the dump, convert and report pieces already exist and work diff --git a/internal/applyplan/roles.go b/internal/applyplan/roles.go index 207b319..bc0547b 100644 --- a/internal/applyplan/roles.go +++ b/internal/applyplan/roles.go @@ -32,11 +32,27 @@ import ( // own "@type" as well. func AccountRoleOperations(principals []backup.Principal) (ops []Operation, covered []string, warnings []string, err error) { type change struct { - name string - variant string + localPart string + domain string + variant string + source string } var changes []change + // v0.16 stores an account as a local part plus a domain reference, not + // as the full address v0.15 uses for its principal name: a v0.15 + // account named "john@example.net" becomes name "john" with a domainId. + // Passing the full address is rejected outright ("Invalid email local + // part"), which is how this was found - on a production clone, where + // accounts are named by address. The smoke instance used bare names and + // never exercised it. + localPartCount := map[string]int{} + for _, p := range principals { + if strings.EqualFold(p.Type, "individual") { + localPartCount[localPart(p)]++ + } + } + for _, p := range principals { if !strings.EqualFold(p.Type, "individual") { continue // domains and groups don't carry these roles @@ -50,30 +66,69 @@ func AccountRoleOperations(principals []backup.Principal) (ops []Operation, cove // touch every account to no effect. continue } - changes = append(changes, change{name: p.Name, variant: variant}) + local := localPart(p) + // Local parts are unique only within a domain, so an upsert matched + // on name alone could land on a different account that happens to + // share it - postmaster@a.com and postmaster@b.com both become + // "postmaster". Granting Admin to the wrong account is worse than + // granting it to none, so an ambiguous name is refused and + // reported rather than guessed at. + if localPartCount[local] > 1 { + warnings = append(warnings, fmt.Sprintf( + "account %q: %d accounts share the local part %q, so this role cannot be restored unambiguously - grant it by hand after migrating", + p.Name, localPartCount[local], local)) + continue + } + changes = append(changes, change{localPart: local, domain: domainOf(p), variant: variant, source: p.Name}) } - sort.Slice(changes, func(i, j int) bool { return changes[i].name < changes[j].name }) + sort.Slice(changes, func(i, j int) bool { return changes[i].localPart < changes[j].localPart }) for _, c := range changes { + value := map[string]any{ + // Account is a multi-variant object: without its own @type the + // upsert is rejected outright. + "@type": "User", + "name": c.localPart, + "roles": map[string]any{"@type": c.variant}, + } ops = append(ops, Operation{ Type: "upsert", Object: "Account", MatchOn: []string{"name"}, - Value: map[string]map[string]any{ - "role-" + c.name: { - // Account is a multi-variant object: without its own - // @type the upsert is rejected outright. - "@type": "User", - "name": c.name, - "roles": map[string]any{"@type": c.variant}, - }, - }, + Value: map[string]map[string]any{"role-" + c.localPart: value}, }) - covered = append(covered, "principal:"+c.name+":roles") + covered = append(covered, "principal:"+c.source+":roles") } return ops, covered, warnings, nil } +// localPart is the account name v0.16 will hold: the part before "@" when +// the v0.15 principal is named by address, otherwise the name as-is. +func localPart(p backup.Principal) string { + name := p.Name + if name == "" && len(p.Emails) > 0 { + name = p.Emails[0] + } + if at := strings.Index(name, "@"); at > 0 { + return name[:at] + } + return name +} + +// domainOf reports the domain an account belongs to, for messages. +func domainOf(p backup.Principal) string { + name := p.Name + if at := strings.Index(name, "@"); at > 0 && at+1 < len(name) { + return name[at+1:] + } + if len(p.Emails) > 0 { + if at := strings.Index(p.Emails[0], "@"); at > 0 && at+1 < len(p.Emails[0]) { + return p.Emails[0][at+1:] + } + } + return "" +} + // roleVariant maps a v0.15 role list onto v0.16's x:UserRoles variant. // // v0.15 carries a list of role names; v0.16 carries one variant. Where an diff --git a/internal/applyplan/roles_test.go b/internal/applyplan/roles_test.go index 9284221..8cecaa9 100644 --- a/internal/applyplan/roles_test.go +++ b/internal/applyplan/roles_test.go @@ -111,3 +111,53 @@ func TestAccountRolesReportsRolesItCannotMap(t *testing.T) { t.Errorf("warnings = %v, want the unmappable role named", warnings) } } + +// Found by a dress rehearsal against a clone of production, where accounts +// are named by address. v0.16 stores an account as a local part plus a +// domain reference, and rejects a full address outright ("Invalid email +// local part"). The smoke instance used bare names and never exercised it. +func TestAccountRolesUsesTheLocalPartOfAnEmailStyleName(t *testing.T) { + ops, _, warnings, err := AccountRoleOperations([]backup.Principal{ + {Type: "individual", Name: "john@linuxexperts.net", Emails: []string{"john@linuxexperts.net"}, Roles: []string{"admin"}}, + }) + if err != nil { + t.Fatal(err) + } + if len(ops) != 1 { + t.Fatalf("generated %d op(s), want 1", len(ops)) + } + v := ops[0].Value["role-john"] + if v == nil { + t.Fatalf("operation not keyed by local part: %+v", ops[0].Value) + } + if v["name"] != "john" { + t.Errorf("name = %v, want the local part \"john\" - the full address is rejected by the server", v["name"]) + } + if len(warnings) != 0 { + t.Errorf("unexpected warnings: %v", warnings) + } +} + +// Local parts are unique only within a domain. Granting Admin to the wrong +// account is worse than granting it to none. +func TestAccountRolesRefusesAnAmbiguousLocalPart(t *testing.T) { + ops, covered, warnings, err := AccountRoleOperations([]backup.Principal{ + {Type: "individual", Name: "postmaster@one.example", Roles: []string{"admin"}}, + {Type: "individual", Name: "postmaster@two.example", Roles: []string{"user"}}, + }) + if err != nil { + t.Fatal(err) + } + if len(ops) != 0 { + t.Errorf("generated %d op(s) for an ambiguous local part, want 0 - it could land on the wrong account", len(ops)) + } + if len(covered) != 0 { + t.Errorf("covered = %v, want none", covered) + } + if len(warnings) != 1 || !strings.Contains(warnings[0], "share the local part") { + t.Errorf("warnings = %v, want one explaining the ambiguity", warnings) + } + if !strings.Contains(warnings[0], "by hand") { + t.Errorf("warning %q should tell the operator what to do instead", warnings[0]) + } +} diff --git a/internal/cutover/cutover.go b/internal/cutover/cutover.go index ca85681..a663d39 100644 --- a/internal/cutover/cutover.go +++ b/internal/cutover/cutover.go @@ -340,9 +340,25 @@ func Run(ctx context.Context, store *checkpoint.Store, rs *checkpoint.RunState, }, nil } client := newClient(opts) - if err := client.WaitForPing(ctx, healthTimeout); err != nil { + // Liveness first, and on its own. Any response - 401 included - + // proves the service is up and routing. + if err := client.WaitForResponse(ctx, healthTimeout); err != nil { return checkpoint.StepOutcome{}, fmt.Errorf("the migrated service started but never answered at %s within %s: %w", opts.AdminURL, healthTimeout, err) } + // Credentials are a separate question, and failing them is not a + // failed cutover. A config fallback-admin does not survive into + // v0.16 - its config is just a store pointer, so the old + // [authentication.fallback-admin] block is gone - so the + // credentials that worked before the migration routinely stop + // working after it, on an instance that is otherwise fine. + if err := client.Ping(ctx); err != nil { + return checkpoint.StepOutcome{ + Verdict: string(StatusWarn), + Detail: fmt.Sprintf("migrated instance is up and answering at %s, but these admin credentials no longer work: %v. "+ + "If they were a config fallback-admin, that does not survive the migration - v0.16 keeps its config in the store. "+ + "Authenticate as an account that exists in the directory instead", opts.AdminURL, err), + }, nil + } return checkpoint.StepOutcome{Detail: fmt.Sprintf("migrated instance answered an authenticated JMAP session request at %s", opts.AdminURL)}, nil }); err != nil { return report, err diff --git a/internal/cutover/cutover_test.go b/internal/cutover/cutover_test.go index e85fc74..01a495a 100644 --- a/internal/cutover/cutover_test.go +++ b/internal/cutover/cutover_test.go @@ -231,16 +231,13 @@ func TestRunResumesWithoutRedoingCompletedSteps(t *testing.T) { func TestRunFailsWhenTheMigratedServiceNeverAnswers(t *testing.T) { store, rs, opts := migratedRun(t) - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusBadGateway) - })) - defer srv.Close() - opts.AdminURL = srv.URL + // Nothing listening at all: a closed port, not an error response. + opts.AdminURL = "http://127.0.0.1:1" opts.HealthTimeout = 300 * time.Millisecond report, err := Run(context.Background(), store, rs, opts) if err == nil { - t.Fatal("Run: want failure when the started service never answers, got nil") + t.Fatal("Run: want failure when nothing answers at all, got nil") } if !strings.Contains(err.Error(), "never answered") { t.Errorf("error %q should distinguish 'started but not answering' from 'failed to start'", err) @@ -250,6 +247,37 @@ func TestRunFailsWhenTheMigratedServiceNeverAnswers(t *testing.T) { } } +// A 401 proves the service is up and routing. Treating it as unhealthy +// failed a cutover that had actually succeeded: the credentials supplied +// for the pre-migration instance were a config fallback-admin, which does +// not survive into v0.16. +func TestRunWarnsRatherThanFailsWhenCredentialsStopWorking(t *testing.T) { + store, rs, opts := migratedRun(t) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + })) + defer srv.Close() + opts.AdminURL = srv.URL + opts.HealthTimeout = 2 * time.Second + + report, err := Run(context.Background(), store, rs, opts) + if err != nil { + t.Fatalf("a service that is up but rejects these credentials is not a failed cutover: %v\n%s", err, report) + } + var warned bool + for _, res := range report.Results { + if res.Name == "wait-healthy" { + warned = res.Status == StatusWarn + if !strings.Contains(res.Detail, "fallback-admin") { + t.Errorf("warning %q should name the likely cause", res.Detail) + } + } + } + if !warned { + t.Errorf("wait-healthy should warn, not fail or pass silently:\n%s", report) + } +} + // Rolling back a migration that completed successfully, because a counter // didn't get rebuilt, would be worse than a stale counter. func TestRunWarnsRatherThanFailsWhenQuotaRecalculationFails(t *testing.T) { diff --git a/internal/stalwartapi/task.go b/internal/stalwartapi/task.go index 0e428fd..0a6c842 100644 --- a/internal/stalwartapi/task.go +++ b/internal/stalwartapi/task.go @@ -7,6 +7,7 @@ import ( "context" "encoding/json" "fmt" + "net/http" "sort" "strings" "time" @@ -287,3 +288,42 @@ func (c *Client) WaitForPing(ctx context.Context, timeout time.Duration) error { } } } + +// WaitForResponse polls until the instance answers an HTTP request at all, +// whatever the status, or timeout elapses. +// +// This is the liveness question, and it is deliberately separate from +// WaitForPing's "and my credentials work". A 401 proves the server is up, +// listening and routing - which is exactly what a caller waiting for a +// restarted service needs to know. Conflating the two failed a cutover +// that had in fact succeeded: the credentials supplied for the +// pre-migration instance were a config fallback-admin, which does not +// survive into v0.16 (its config is a store pointer, so the old +// [authentication.fallback-admin] block is simply gone), so every poll came +// back 401 and the phase reported the service as never having answered. +func (c *Client) WaitForResponse(ctx context.Context, timeout time.Duration) error { + url := strings.TrimRight(c.BaseURL, "/") + "/.well-known/jmap" + deadline := time.Now().Add(timeout) + var lastErr error + for { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return err + } + req.SetBasicAuth(c.Username, c.Password) + resp, err := c.httpClient().Do(req) + if err == nil { + resp.Body.Close() + return nil + } + lastErr = err + if !time.Now().Before(deadline) { + return fmt.Errorf("stalwartapi: %s did not respond within %s: %w", url, timeout, lastErr) + } + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(500 * time.Millisecond): + } + } +}