diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 99dec3c..d5cc8bb 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -599,6 +599,31 @@ happens to need them. `preflight.DeploymentKind` is a type alias for script is Stalwart's, not ours — need a policy for what happens when it changes upstream (re-vendor + re-test before bumping the pin, never silently float to `main`). +- **Most of the "unmigrated" settings are not work at all - measured, then + classified.** The raw figure from a production instance was 12,182 + settings, which reads as an impossible amount of manual reconstruction. + Snapshotting a migrated v0.16.14 store showed why it is misleading: + 8,547 of them are `server.blocked-ip`, runtime auto-ban state that + repopulates itself; 3,337 are stock spam-filter and lookup data v0.16 + ships its own copies of (2,084 `MemoryLookupKey`, 66 `SpamRule`, 18 + `SpamDnsblServer` were already present after migration); ~224 had already + been carried by another route, DKIM included, as `DkimSignature` objects + with their private keys. That leaves **~293 keys genuinely needing a + human**. + + `backup.UnmigratedReport.Classify` encodes this, and the rehearsal reports + the categorised view rather than the raw count. Every rule was checked + against a migrated instance rather than inferred, and an unrecognized + prefix defaults to "needs review" - assuming an unknown setting is safe to + ignore is the wrong default. + + This also retired a planned pair of generators. v0.15's spam rules are + `stwt_rbl_senderscore_ip`; v0.16's are `STWT_RBL_SENDERSCORE_IP` - the + same stock set, already installed. Generating them from v0.15 would + duplicate every rule and revert a year of upstream updates, so the + lookup and spam-filter generators were deliberately not written. The + targets worth generating are the small site-specific groups: + `queue.schedule`, `queue.tls`, `session.auth`, `server.auto-ban`. - **Settings apply-plan (§4.3): started, and the critical path.** `internal/applyplan` covers `server.listener` — verified end to end by applying a generated plan to a real 0.16.14 instance and reading back all diff --git a/README.md b/README.md index 3751c7e..002ca10 100644 --- a/README.md +++ b/README.md @@ -124,10 +124,18 @@ stalwart-cli apply --file /runs//supplement.json \ --url https://mail.example.com ``` -Expect the worklist to be long. Measured against a real production instance, -`migrate_v016.py` carried **219 of 12,401 settings — 1.8%**. The rest, -including `server.listener`, has to be rebuilt by hand; until it is, a -migrated instance answers on no ports at all. Both outputs are preserved +The worklist is long but mostly not work, and `rehearse` says which is +which. Measured against a real production instance, `migrate_v016.py` +carried **219 of 12,401 settings**. Of the 12,182 it left: + +- **8,547** are runtime auto-ban state that repopulates itself +- **3,337** are stock spam-filter and lookup data v0.16 ships its own copies + of — restoring v0.15's would revert a year of upstream updates +- **~224** were already carried another way, DKIM signatures included +- **~293** genuinely need your eyes + +`server.listener` is in that third group only because this tool regenerates +it for you; without that a migrated instance answers on no ports at all. Both outputs are preserved under `/runs//` (`export.json` and `unmigrated.txt`) even though the rest of the scratch directory is cleaned up, because they are the conclusions. diff --git a/cmd/stalwart-migrate/rehearse.go b/cmd/stalwart-migrate/rehearse.go index 1d7296b..a9bc139 100644 --- a/cmd/stalwart-migrate/rehearse.go +++ b/cmd/stalwart-migrate/rehearse.go @@ -212,9 +212,10 @@ func runRehearse(args []string) (err error) { rs.RecordArtifact("unmigrated-settings", checkpoint.Artifact{Path: keptWorklist, SHA256: sum, SizeBytes: size}) } } - fmt.Printf("\n !! %s\n", unmigrated.Summary(10)) - fmt.Println(" These do not carry over on their own. See the supplemental plan below for the part") - fmt.Println(" this tool can rebuild for you.") + // Categorised rather than counted: the raw total is alarming and + // misleading. Most of it either rebuilds itself or ships with + // v0.16 - see backup.Disposition. + fmt.Printf("\n !! %s\n", unmigrated.Classify().Summary(keptWorklist)) } // Generate what we can of the gap (ARCHITECTURE.md §4.3). This is diff --git a/internal/backup/settingsdump_test.go b/internal/backup/settingsdump_test.go index 9916c6a..005baac 100644 --- a/internal/backup/settingsdump_test.go +++ b/internal/backup/settingsdump_test.go @@ -262,3 +262,118 @@ func TestRunSettingsConvertRunsInTheGivenWorkDir(t *testing.T) { t.Errorf("unmigrated.txt should land in WorkDir, not the caller's cwd: %v", err) } } + +// The numbers here are the real ones from a production instance, because +// the point of classifying is what it does to those numbers: 12,182 reads +// as impossible, and is mostly nothing to do. +const productionUnmigrated = `# Unmigrated v0.15 settings + +Total unmigrated keys: 12182 across 69 prefixes. + + server.blocked-ip 8547 keys + lookup.url-redirectors 1076 keys + lookup.trusted-domains 828 keys + spam-filter.list 537 keys + spam-filter.rule 424 keys + spam-filter.dnsbl 292 keys + lookup.surbl-hashbl 180 keys + queue.schedule 41 keys + server.listener 26 keys + signature.rsa-example.com 22 keys + server.auto-ban 16 keys + session.auth 14 keys +` + +func classifyProduction(t *testing.T) *ClassifiedReport { + t.Helper() + path := filepath.Join(t.TempDir(), "unmigrated.txt") + if err := os.WriteFile(path, []byte(productionUnmigrated), 0o640); err != nil { + t.Fatal(err) + } + report, err := ReadUnmigratedReport(path) + if err != nil { + t.Fatal(err) + } + return report.Classify() +} + +func TestClassifySeparatesWorkFromNoise(t *testing.T) { + c := classifyProduction(t) + + // Runtime state: auto-ban repopulates it. + if got := c.Counts[DispositionRegenerates]; got != 8547 { + t.Errorf("regenerates = %d, want 8547 (server.blocked-ip)", got) + } + // Stock data v0.16 ships: restoring v0.15's would revert it. + if got := c.Counts[DispositionShipped]; got != 1076+828+537+424+292+180 { + t.Errorf("shipped = %d, want the stock spam/lookup groups", got) + } + // Carried by another route. + if got := c.Counts[DispositionCarried]; got != 22+26 { + t.Errorf("carried = %d, want the signature and listener groups", got) + } + // What's actually left for a human. + if got := c.Counts[DispositionReview]; got != 41+16+14 { + t.Errorf("needs review = %d, want %d", got, 41+16+14) + } +} + +// server.blocked-ip is runtime state; server.auto-ban sitting right next to +// it is configuration. A shortest-prefix match would get this wrong. +func TestClassifyPrefersTheMoreSpecificRule(t *testing.T) { + c := classifyProduction(t) + for _, g := range c.Groups { + switch g.Prefix { + case "server.blocked-ip": + if g.Disposition != DispositionRegenerates { + t.Errorf("server.blocked-ip = %q, want regenerates", g.Disposition) + } + case "server.auto-ban": + if g.Disposition != DispositionReview { + t.Errorf("server.auto-ban = %q, want review - it is configuration, not runtime state", g.Disposition) + } + case "server.listener": + if g.Disposition != DispositionCarried { + t.Errorf("server.listener = %q, want carried - the apply plan regenerates it", g.Disposition) + } + } + } +} + +func TestClassifyReviewListIsTheActualWorklist(t *testing.T) { + review := classifyProduction(t).NeedsReview() + if len(review) != 3 { + t.Fatalf("review groups = %d, want 3", len(review)) + } + if review[0].Prefix != "queue.schedule" { + t.Errorf("first review group = %q, want the largest (queue.schedule)", review[0].Prefix) + } + for _, g := range review { + if g.Disposition != DispositionReview { + t.Errorf("%s is in the review list with disposition %q", g.Prefix, g.Disposition) + } + } +} + +func TestClassifySummaryLeadsWithWhatMatters(t *testing.T) { + summary := classifyProduction(t).Summary("/var/lib/stalwart-migrator/runs/x/unmigrated.txt") + if !strings.Contains(summary, "NEED YOUR REVIEW") { + t.Errorf("summary should call out the review bucket:\n%s", summary) + } + if !strings.Contains(summary, "71 needing review are work") { + t.Errorf("summary should say how much is actually work:\n%s", summary) + } + if !strings.Contains(summary, "would revert them") { + t.Errorf("summary should warn against restoring stock data:\n%s", summary) + } +} + +func TestClassifyUnknownPrefixesDefaultToReview(t *testing.T) { + d, note := classifyPrefix("something.nobody.has.seen") + if d != DispositionReview { + t.Errorf("unknown prefix = %q, want review - guessing that an unknown setting is safe to ignore is the wrong default", d) + } + if note != "" { + t.Errorf("note = %q, want empty for an unclassified prefix", note) + } +} diff --git a/internal/backup/unmigrated_classify.go b/internal/backup/unmigrated_classify.go new file mode 100644 index 0000000..cc9e787 --- /dev/null +++ b/internal/backup/unmigrated_classify.go @@ -0,0 +1,176 @@ +// SPDX-FileCopyrightText: 2026 LINUXexpert-org +// SPDX-License-Identifier: GPL-3.0-or-later + +package backup + +import ( + "fmt" + "sort" + "strings" +) + +// Disposition is what an operator actually has to do about a group of +// settings migrate_v016.py didn't carry over. +// +// The raw count is misleading on its own, and alarmingly so. A real +// production instance reported 12,182 unmigrated settings, which reads as +// an impossible amount of manual work. Categorised, 8,547 of them +// repopulate themselves, 3,337 ship with v0.16, ~198 had already been +// carried by another mechanism, and roughly 290 genuinely needed a human. +// The difference between those two framings is the difference between a +// migration nobody attempts and an afternoon's review. +type Disposition string + +const ( + // DispositionRegenerates: runtime state, not configuration. Nothing to + // do; it rebuilds itself in normal operation. + DispositionRegenerates Disposition = "regenerates" + // DispositionShipped: v0.16 provides its own copy, and restoring + // v0.15's would duplicate it or replace newer definitions with older + // ones. + DispositionShipped Disposition = "shipped with v0.16" + // DispositionCarried: the substance came across by another route even + // though these particular keys are listed as unmigrated. + DispositionCarried Disposition = "already carried" + // DispositionReview: genuinely site-specific. This is the real work. + DispositionReview Disposition = "needs review" +) + +// dispositionRule maps a setting prefix to what to do about it. +// +// Every entry here was checked against a v0.16.14 instance migrated from a +// real v0.15.5 - by snapshotting the migrated store and counting what was +// already in it - not inferred from documentation. Rules are matched +// longest-prefix-first, because "server.blocked-ip" is runtime state while +// "server.auto-ban" right next to it is configuration. +type dispositionRule struct { + prefix string + disposition Disposition + note string +} + +var dispositionRules = []dispositionRule{ + {"server.blocked-ip", DispositionRegenerates, + "auto-ban repopulates these from live traffic; a migrated instance simply starts with an empty list"}, + + {"lookup.url-redirectors", DispositionShipped, + "v0.16 ships this list itself (observed as MemoryLookupKey/SpamUrlRedirector entries in a migrated store)"}, + {"lookup.trusted-domains", DispositionShipped, + "v0.16 ships this list itself (MemoryLookupKey/SpamTrustedDomain)"}, + {"lookup.surbl-hashbl", DispositionShipped, + "v0.16 ships this list itself (MemoryLookupKey)"}, + {"spam-filter.rule", DispositionShipped, + "v0.16 ships its own rules - 66 SpamRule objects were present in a migrated store, the same stock set under v0.16's naming (stwt_x -> STWT_X). Restoring v0.15's would duplicate them and revert a year of updates"}, + {"spam-filter.dnsbl", DispositionShipped, + "v0.16 ships its own DNSBL servers (18 SpamDnsblServer objects present after migration)"}, + {"spam-filter.list", DispositionShipped, + "stock lists (file extensions and similar) that v0.16 provides itself"}, + + {"signature.", DispositionCarried, + "DKIM: carried across as DkimSignature objects, private keys included - verified on a migrated instance"}, + {"server.listener", DispositionCarried, + "regenerated by this tool's own apply plan (see the supplement)"}, +} + +// Classification is one group of unmigrated settings with its disposition. +type Classification struct { + Prefix string + Keys int + Disposition Disposition + Note string +} + +// ClassifiedReport is an UnmigratedReport with each group's disposition +// resolved, and totals per disposition. +type ClassifiedReport struct { + TotalKeys int + Groups []Classification + Counts map[Disposition]int +} + +// Classify resolves what to do about each group in the report. +func (r *UnmigratedReport) Classify() *ClassifiedReport { + out := &ClassifiedReport{Counts: map[Disposition]int{}} + if r == nil { + return out + } + out.TotalKeys = r.TotalKeys + for _, p := range r.Prefixes { + disposition, note := classifyPrefix(p.Prefix) + out.Groups = append(out.Groups, Classification{ + Prefix: p.Prefix, Keys: p.Keys, Disposition: disposition, Note: note, + }) + out.Counts[disposition] += p.Keys + } + sort.Slice(out.Groups, func(i, j int) bool { return out.Groups[i].Keys > out.Groups[j].Keys }) + return out +} + +func classifyPrefix(prefix string) (Disposition, string) { + best := -1 + var disposition Disposition = DispositionReview + var note string + for _, rule := range dispositionRules { + if !strings.HasPrefix(prefix, rule.prefix) { + continue + } + if len(rule.prefix) > best { + best, disposition, note = len(rule.prefix), rule.disposition, rule.note + } + } + return disposition, note +} + +// NeedsReview returns just the groups a human has to deal with, largest +// first - the actual worklist, as opposed to the raw count. +func (c *ClassifiedReport) NeedsReview() []Classification { + var out []Classification + for _, g := range c.Groups { + if g.Disposition == DispositionReview { + out = append(out, g) + } + } + return out +} + +// Summary renders the categorised report, leading with what needs a human +// rather than with the total. +func (c *ClassifiedReport) Summary(worklistPath string) string { + if c == nil || c.TotalKeys == 0 { + return "no unmigrated settings were reported" + } + var b strings.Builder + fmt.Fprintf(&b, "%d setting(s) were not carried over by migrate_v016.py. What they actually are:\n", c.TotalKeys) + + for _, d := range []Disposition{DispositionRegenerates, DispositionShipped, DispositionCarried, DispositionReview} { + n := c.Counts[d] + if n == 0 { + continue + } + label := string(d) + if d == DispositionReview { + label = "NEED YOUR REVIEW" + } + fmt.Fprintf(&b, "\n %6d %-18s", n, label) + groups := []string{} + for _, g := range c.Groups { + if g.Disposition == d && g.Keys > 0 { + groups = append(groups, fmt.Sprintf("%s (%d)", g.Prefix, g.Keys)) + } + } + shown := groups + if len(shown) > 6 { + shown = shown[:6] + } + fmt.Fprintf(&b, " %s", strings.Join(shown, ", ")) + if len(groups) > len(shown) { + fmt.Fprintf(&b, ", +%d more", len(groups)-len(shown)) + } + } + + review := c.Counts[DispositionReview] + fmt.Fprintf(&b, "\n\n Only the %d needing review are work. The rest either rebuild themselves,\n", review) + fmt.Fprintf(&b, " ship with v0.16 (restoring v0.15's copies would revert them), or came\n") + fmt.Fprintf(&b, " across by another route. Full key list: %s", worklistPath) + return b.String() +}