diff --git a/README.md b/README.md index 3a7e52f..cce5528 100644 --- a/README.md +++ b/README.md @@ -39,8 +39,11 @@ was down for **6 seconds** end to end. Plan the window around verification, not data volume. After cutover, `run` compares the migrated instance against the snapshot -preflight took, and fails the command if an account or a domain that existed -before is missing from it. The service is left running either way — by that +preflight took, and fails the command if an account that existed before is +missing from it. A domain that no longer appears is reported as a warning +rather than a failure: the two versions do not agree on what counts as a +domain — principals on one side, `Domain` objects on the other — and failing +a migration over that difference would abort runs that lost nothing. The service is left running either way — by that point the store has been migrated in place, so stopping it would not undo anything; your recovery point is the way back. `report ` prints the same finding again later. Where preflight had no admin URL to snapshot from, diff --git a/cmd/stalwart-migrate/report.go b/cmd/stalwart-migrate/report.go index 48d405b..be78594 100644 --- a/cmd/stalwart-migrate/report.go +++ b/cmd/stalwart-migrate/report.go @@ -76,6 +76,8 @@ func reportFromSteps(rs *checkpoint.RunState) validate.Report { status = validate.StatusFail case step.Verdict == string(validate.StatusSkip): status = validate.StatusSkip + case step.Verdict == string(validate.StatusWarn): + status = validate.StatusWarn case step.Status != checkpoint.StepDone: // Recorded but never completed: the run stopped partway. status = validate.StatusFail diff --git a/internal/stalwartapi/management.go b/internal/stalwartapi/management.go index 80165d2..f96c7b7 100644 --- a/internal/stalwartapi/management.go +++ b/internal/stalwartapi/management.go @@ -185,14 +185,22 @@ func (c *Client) accountSnapshotJMAP(ctx context.Context) (*Snapshot, error) { if err != nil { return nil, fmt.Errorf("stalwartapi: resolve domain names (an unresolved id would make every domain look missing): %w", err) } + // Every domain the server holds, not only those some account calls its + // primary. Deriving the list from accounts dropped alias domains and + // domains with no accounts, so comparing it against a "before" list + // that did include them reported losses that had not happened. domainSet := map[string]bool{} + for _, name := range domainNames { + if name != "" { + domainSet[name] = true + } + } for _, a := range accounts { if a.DomainID == "" { continue } if name, ok := domainNames[a.DomainID]; ok && name != "" { - domainSet[name] = true - continue + continue // already counted above } // Keep the raw id rather than dropping the domain entirely: a // domain that can't be named is still a domain that exists. diff --git a/internal/stalwartapi/principal.go b/internal/stalwartapi/principal.go index 18f07a1..24cbd94 100644 --- a/internal/stalwartapi/principal.go +++ b/internal/stalwartapi/principal.go @@ -180,12 +180,21 @@ func (c *Client) principalSnapshotREST(ctx context.Context) (*Snapshot, error) { domainSet[d.Name] = true } } - // Fall back to the domains implied by account addresses if the - // instance has no explicit domain principals. - for _, p := range individuals { - for _, email := range p.Emails { - if at := strings.LastIndex(email, "@"); at >= 0 && at+1 < len(email) { - domainSet[email[at+1:]] = true + // Fall back to the domains implied by account addresses only when the + // instance declares no domain principals at all. + // + // This loop used to run unconditionally, which quietly inflated the + // list with every alias domain. That matters because this snapshot is + // the "before" side of the post-migration comparison, and the 0.16 side + // reports the domains the server actually holds: an alias domain that + // was never its own principal would be present here, absent there, and + // reported as lost by a migration that lost nothing. + if len(domainSet) == 0 { + for _, p := range individuals { + for _, email := range p.Emails { + if at := strings.LastIndex(email, "@"); at >= 0 && at+1 < len(email) { + domainSet[email[at+1:]] = true + } } } } diff --git a/internal/stalwartapi/principal_test.go b/internal/stalwartapi/principal_test.go index 9357163..3e2f585 100644 --- a/internal/stalwartapi/principal_test.go +++ b/internal/stalwartapi/principal_test.go @@ -356,3 +356,55 @@ func TestTenantNamesReportsTenantPrincipals(t *testing.T) { t.Error("TenantNames returned nil without an error; want a (possibly empty) list") } } + +// The "before" side of the post-migration comparison. It used to add every +// domain appearing in any account's address on top of the domain principals, +// so an instance with three declared domains and accounts aliased across nine +// reported nine - and the 0.16 side, which lists the domains the server +// actually holds, then looked as though six had been lost by a migration that +// lost nothing. INBUXA is exactly that shape. +func TestRESTSnapshotDoesNotInflateDomainsWithAliases(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + typ := r.URL.Query().Get("types") + w.Header().Set("content-type", "application/json") + switch typ { + case "domain": + _, _ = w.Write([]byte(`{"data":{"items":[{"type":"domain","name":"example.org"}],"total":1}}`)) + default: + _, _ = w.Write([]byte(`{"data":{"items":[{"type":"individual","name":"ann@example.org",` + + `"emails":["ann@example.org","ann@alias.example","ann@other.example"],"usedQuota":1}],"total":1}}`)) + } + })) + defer srv.Close() + + snap, err := (&Client{BaseURL: srv.URL, Username: "admin", Password: "pw", HTTPClient: srv.Client()}).principalSnapshotREST(context.Background()) + if err != nil { + t.Fatalf("principalSnapshotREST: %v", err) + } + if len(snap.Domains) != 1 || snap.Domains[0] != "example.org" { + t.Fatalf("Domains = %v, want just the declared domain principal", snap.Domains) + } +} + +// The fallback still exists for an instance that declares none, where an +// address is the only evidence a domain is in use. +func TestRESTSnapshotFallsBackToAddressesWhenNoDomainPrincipals(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("content-type", "application/json") + if r.URL.Query().Get("types") == "domain" { + _, _ = w.Write([]byte(`{"data":{"items":[],"total":0}}`)) + return + } + _, _ = w.Write([]byte(`{"data":{"items":[{"type":"individual","name":"ann@example.org",` + + `"emails":["ann@example.org"],"usedQuota":1}],"total":1}}`)) + })) + defer srv.Close() + + snap, err := (&Client{BaseURL: srv.URL, Username: "admin", Password: "pw", HTTPClient: srv.Client()}).principalSnapshotREST(context.Background()) + if err != nil { + t.Fatalf("principalSnapshotREST: %v", err) + } + if len(snap.Domains) != 1 || snap.Domains[0] != "example.org" { + t.Fatalf("Domains = %v, want the domain implied by the address", snap.Domains) + } +} diff --git a/internal/validate/content_integrity.go b/internal/validate/content_integrity.go index 2787c17..719ad99 100644 --- a/internal/validate/content_integrity.go +++ b/internal/validate/content_integrity.go @@ -43,11 +43,25 @@ type ContentIntegrityResult struct { MessageCountsCompared bool // false when the source version could not report counts } -// OK reports whether everything this comparison was able to check matched. -// Read it together with MessageCountsCompared: OK with that false means -// "the directory survived", not "no mail was lost". +// OK reports whether everything that must match did: no account and no mail +// went missing. Read it together with MessageCountsCompared: OK with that +// false means "the directory survived", not "no mail was lost". +// +// Domains are deliberately not part of this. What the two versions call a +// domain differs across the 0.15/0.16 boundary — principals on one side, +// Domain objects on the other, with aliases and account-less domains +// counted differently — and we have already been caught once reporting a +// migration that lost nothing as having lost domains. A disagreement there +// is worth showing an operator; it is not worth failing a migration over, +// where a missing account is. func (r ContentIntegrityResult) OK() bool { - return len(r.MissingAccounts) == 0 && len(r.MessageCountMismatches) == 0 && len(r.MissingDomains) == 0 + return len(r.MissingAccounts) == 0 && len(r.MessageCountMismatches) == 0 +} + +// DomainsOK reports whether every domain seen before the migration is still +// listed after it. +func (r ContentIntegrityResult) DomainsOK() bool { + return len(r.MissingDomains) == 0 } func (r ContentIntegrityResult) String() string { @@ -59,7 +73,10 @@ func (r ContentIntegrityResult) String() string { "(this migration's source version reports no per-mailbox counts, so no-data-loss is NOT verified here - "+ "only that every account and domain survived)", r.AccountsChecked) } - if r.OK() { + // Everything below is a finding, so return early only when there is + // nothing at all to report - domains included, even though they no + // longer fail the run. A warning nobody can read is not a warning. + if r.OK() && r.DomainsOK() { if r.MessageCountsCompared { b.WriteString(", all message counts match") } diff --git a/internal/validate/live.go b/internal/validate/live.go index c347ca9..824bfa5 100644 --- a/internal/validate/live.go +++ b/internal/validate/live.go @@ -71,11 +71,16 @@ func RunLive(ctx context.Context, store *checkpoint.Store, rs *checkpoint.RunSta if err != nil { return checkpoint.StepOutcome{}, err } - if !r.OK() { + switch { + case !r.OK(): // Recorded as a completed step with a failing verdict rather // than an error: the comparison ran, and its answer is the // finding. An error here would read as "we could not look". return checkpoint.StepOutcome{Verdict: string(StatusFail), Detail: r.String()}, nil + case !r.DomainsOK(): + // The two versions disagree about what counts as a domain, so + // this is reported rather than treated as data loss. + return checkpoint.StepOutcome{Verdict: string(StatusWarn), Detail: r.String()}, nil } return checkpoint.StepOutcome{Detail: r.String()}, nil }) @@ -88,8 +93,11 @@ func RunLive(ctx context.Context, store *checkpoint.Store, rs *checkpoint.RunSta } status := StatusOK - if outcome.Verdict == string(StatusFail) { + switch outcome.Verdict { + case string(StatusFail): status = StatusFail + case string(StatusWarn): + status = StatusWarn } report.Results = append(report.Results, CheckResult{Name: "content-integrity", Status: status, Detail: outcome.Detail}) return report, nil diff --git a/internal/validate/live_test.go b/internal/validate/live_test.go index b632e6e..b965d7b 100644 --- a/internal/validate/live_test.go +++ b/internal/validate/live_test.go @@ -101,7 +101,40 @@ func TestRunLiveFailsWhenAnAccountIsMissing(t *testing.T) { } } -func TestRunLiveFailsWhenADomainIsMissing(t *testing.T) { +func TestRunLiveWarnsButDoesNotBlockWhenADomainIsMissing(t *testing.T) { + // What the two versions call a domain differs across the 0.15/0.16 + // boundary - principals on one side, Domain objects on the other, with + // aliases counted differently - and INBUXA's own before-list was + // inflated with alias domains that the after-list structurally cannot + // contain. Failing the migration on that would abort a run that lost + // nothing, so it is reported and not treated as data loss. + srv := fakeInstance(t, []string{"example.org"}, map[string]float64{"ann@example.org": 10}) + defer srv.Close() + + store, rs := newRun(t) + report, err := RunLive(context.Background(), store, rs, LiveOptions{ + AdminURL: srv.URL, AdminUser: "admin", AdminPassword: "pw", HTTPClient: srv.Client(), + Before: &checkpoint.PreflightSnapshot{ + Domains: []string{"example.org", "alias.example"}, + UsedQuota: map[string]int64{"ann@example.org": 1}, + }, + }) + if err != nil { + t.Fatalf("RunLive: %v", err) + } + if report.Blocking() { + t.Fatalf("a domain-only difference must not abort the migration, got: %s", report.String()) + } + if got := report.Results[0].Status; got != StatusWarn { + t.Fatalf("status = %q, want %q", got, StatusWarn) + } + if !strings.Contains(report.Results[0].Detail, "alias.example") { + t.Fatalf("the operator still needs to be told which domain, got %q", report.Results[0].Detail) + } +} + +func TestRunLiveStillBlocksWhenAnAccountAndADomainAreMissing(t *testing.T) { + // A lost account is a lost account, whatever the domain list says. srv := fakeInstance(t, []string{"example.org"}, map[string]float64{"ann@example.org": 10}) defer srv.Close() @@ -109,15 +142,12 @@ func TestRunLiveFailsWhenADomainIsMissing(t *testing.T) { report, _ := RunLive(context.Background(), store, rs, LiveOptions{ AdminURL: srv.URL, AdminUser: "admin", AdminPassword: "pw", HTTPClient: srv.Client(), Before: &checkpoint.PreflightSnapshot{ - Domains: []string{"example.org", "vanished.example"}, - UsedQuota: map[string]int64{"ann@example.org": 1}, + Domains: []string{"example.org", "alias.example"}, + UsedQuota: map[string]int64{"ann@example.org": 1, "bob@example.org": 2}, }, }) if !report.Blocking() { - t.Fatalf("a missing domain must block, got: %s", report.String()) - } - if !strings.Contains(report.Results[0].Detail, "vanished.example") { - t.Fatalf("the report should name the missing domain, got %q", report.Results[0].Detail) + t.Fatalf("a missing account must still block, got: %s", report.String()) } } diff --git a/internal/validate/report.go b/internal/validate/report.go index 7eda2e8..1523769 100644 --- a/internal/validate/report.go +++ b/internal/validate/report.go @@ -18,6 +18,9 @@ const ( // are different answers, and reporting the second as the first is the // failure mode ARCHITECTURE.md §4.7 warns about. StatusSkip Status = "skip" + // StatusWarn is a finding worth an operator's attention that is not + // worth failing a migration over. + StatusWarn Status = "warn" ) type CheckResult struct {