diff --git a/cmd/stalwart-migrate/run.go b/cmd/stalwart-migrate/run.go index 63b7e7d..b7bb6c8 100644 --- a/cmd/stalwart-migrate/run.go +++ b/cmd/stalwart-migrate/run.go @@ -196,21 +196,48 @@ func runRun(args []string) (err error) { } fmt.Printf("cloned verified backup into sandbox: %s\n", sandboxDataDir) + unmigratedPath := filepath.Join(runWorkDir, "unmigrated.txt") if _, err := store.RunStep(rs, checkpoint.PhaseStage, "convert-settings", func() (checkpoint.StepOutcome, error) { if err := backup.RunSettingsConvert(ctx, backup.SettingsConvertOptions{ PythonPath: *pythonPath, ScriptPath: scriptDest, SettingsPath: settingsPath, PrincipalsPath: principalsPath, ConfigPath: sandboxConfigPath, OutputPath: sandboxExportPath, PatchPaths: map[string]string{*dataDir: sandboxDataDir}, + // Without this the script writes unmigrated.txt into whatever + // directory this command was launched from - or fails outright + // if that isn't writable. + WorkDir: runWorkDir, }); err != nil { return checkpoint.StepOutcome{}, err } - return checkpoint.StepOutcome{Detail: fmt.Sprintf("generated %s and %s, patched to point at the sandbox", sandboxConfigPath, sandboxExportPath)}, nil + detail := fmt.Sprintf("generated %s and %s, patched to point at the sandbox", sandboxConfigPath, sandboxExportPath) + if report, err := backup.ReadUnmigratedReport(unmigratedPath); err == nil && report != nil && report.TotalKeys > 0 { + detail += fmt.Sprintf("; %d setting(s) were NOT migrated", report.TotalKeys) + } + return checkpoint.StepOutcome{Detail: detail, Extra: unmigratedPath}, nil }); err != nil { return fmt.Errorf("convert settings: %w", err) } fmt.Println("generated sandbox config.json and export.json") + // What the converter could NOT carry over matters more than what it + // could: against a real instance this is the overwhelming majority of + // the configuration, including the listeners, and an operator who + // doesn't read it will bring up a server that answers on no ports. + unmigrated, err := backup.ReadUnmigratedReport(unmigratedPath) + if err != nil { + fmt.Fprintf(os.Stderr, "warning: couldn't read the unmigrated-settings report: %v\n", err) + } else if unmigrated != nil && unmigrated.TotalKeys > 0 { + if sum, size, hashErr := backup.HashFile(unmigratedPath); hashErr == nil { + rs.RecordArtifact("unmigrated-settings", checkpoint.Artifact{Path: unmigratedPath, SHA256: sum, SizeBytes: size}) + if saveErr := store.Save(rs); saveErr != nil { + fmt.Fprintf(os.Stderr, "warning: couldn't record the unmigrated-settings artifact: %v\n", saveErr) + } + } + fmt.Printf("\n !! %s\n", unmigrated.Summary(10)) + fmt.Println(" These do not carry over. Recreate them on the migrated instance before it serves mail.") + } + fmt.Println("\n--- recovery-mode migration (against the sandbox) ---") listenURL := fmt.Sprintf("http://127.0.0.1:%d/", *recoveryPort) recReport, err := recovery.Run(ctx, store, rs, recovery.Options{ diff --git a/internal/backup/backup.go b/internal/backup/backup.go index ab05b8f..8bf98ce 100644 --- a/internal/backup/backup.go +++ b/internal/backup/backup.go @@ -91,7 +91,7 @@ func Run(ctx context.Context, store *checkpoint.Store, rs *checkpoint.RunState, if err != nil { return checkpoint.StepOutcome{}, err } - sum, size, err := hashFile(preserved) + sum, size, err := HashFile(preserved) if err != nil { return checkpoint.StepOutcome{}, fmt.Errorf("backup: hash preserved binary %s: %w", preserved, err) } @@ -146,7 +146,7 @@ func Run(ctx context.Context, store *checkpoint.Store, rs *checkpoint.RunState, if err := RunPgDump(ctx, opts.SQL); err != nil { return checkpoint.StepOutcome{}, err } - sum, size, err := hashFile(opts.SQL.OutPath) + sum, size, err := HashFile(opts.SQL.OutPath) if err != nil { return checkpoint.StepOutcome{}, err } @@ -161,7 +161,7 @@ func Run(ctx context.Context, store *checkpoint.Store, rs *checkpoint.RunState, if err := RunMySQLDump(ctx, opts.SQL); err != nil { return checkpoint.StepOutcome{}, err } - sum, size, err := hashFile(opts.SQL.OutPath) + sum, size, err := HashFile(opts.SQL.OutPath) if err != nil { return checkpoint.StepOutcome{}, err } @@ -214,11 +214,11 @@ func Run(ctx context.Context, store *checkpoint.Store, rs *checkpoint.RunState, }); err != nil { return checkpoint.StepOutcome{}, err } - settingsSum, settingsSize, err := hashFile(opts.SettingsDumpPath) + settingsSum, settingsSize, err := HashFile(opts.SettingsDumpPath) if err != nil { return checkpoint.StepOutcome{}, err } - principalsSum, principalsSize, err := hashFile(opts.PrincipalsDumpPath) + principalsSum, principalsSize, err := HashFile(opts.PrincipalsDumpPath) if err != nil { return checkpoint.StepOutcome{}, err } @@ -247,7 +247,7 @@ func Run(ctx context.Context, store *checkpoint.Store, rs *checkpoint.RunState, } var totalSize int64 for i, f := range files { - sum, size, err := hashFile(f) + sum, size, err := HashFile(f) if err != nil { return checkpoint.StepOutcome{}, err } diff --git a/internal/backup/hash.go b/internal/backup/hash.go index f87e0ab..0f5d702 100644 --- a/internal/backup/hash.go +++ b/internal/backup/hash.go @@ -11,9 +11,9 @@ import ( "os" ) -// hashFile returns a file's SHA256 and size, for recording as a +// HashFile returns a file's SHA256 and size, for recording as a // checkpoint.Artifact. -func hashFile(path string) (sha256Hex string, size int64, err error) { +func HashFile(path string) (sha256Hex string, size int64, err error) { f, err := os.Open(path) if err != nil { return "", 0, fmt.Errorf("backup: hash %s: %w", path, err) diff --git a/internal/backup/settingsdump.go b/internal/backup/settingsdump.go index d8c8f75..a7aef8e 100644 --- a/internal/backup/settingsdump.go +++ b/internal/backup/settingsdump.go @@ -12,7 +12,9 @@ import ( "net/http" "os" "os/exec" + "regexp" "sort" + "strconv" "strings" "time" ) @@ -135,6 +137,14 @@ type SettingsConvertOptions struct { // config.json's contents directly, which would require depending on its // exact schema. PatchPaths map[string]string + + // WorkDir is where the script runs. It matters more than it looks: + // migrate_v016.py writes its unmigrated.txt report into the current + // working directory, so without this the convert either fails outright + // (an unwritable CWD - which is what happens running as a service from + // /) or silently drops the single most important output of the whole + // migration wherever the operator happened to be standing. + WorkDir string } // RunSettingsConvert runs migrate_v016.py's convert subcommand. @@ -159,9 +169,97 @@ func RunSettingsConvert(ctx context.Context, o SettingsConvertOptions) error { args = append(args, "--patch-paths", strings.Join(pairs, ",")) } cmd := exec.CommandContext(ctx, python, args...) + if o.WorkDir != "" { + if err := os.MkdirAll(o.WorkDir, 0o750); err != nil { + return fmt.Errorf("backup: create convert working directory %s: %w", o.WorkDir, err) + } + cmd.Dir = o.WorkDir + } out, err := cmd.CombinedOutput() if err != nil { return fmt.Errorf("backup: migrate_v016.py convert failed: %w (output: %s)", err, out) } return nil } + +// UnmigratedPrefix is one group of v0.15 settings the conversion did not +// carry over, as reported by migrate_v016.py's unmigrated.txt. +type UnmigratedPrefix struct { + Prefix string + Keys int +} + +// UnmigratedReport summarizes what a conversion left behind. +// +// This is not a footnote. Against a real production instance - 12,401 +// settings - Stalwart's own converter migrated 219 of them, 1.8%, and left +// 12,182 for the operator to recreate by hand: spam-filter rules, DNSBLs, +// trusted-domain and URL-redirector lookups, queue scheduling and TLS +// settings, and server.listener itself, which is why a freshly migrated +// instance answers on none of the ports the old one did. A migration that +// reported success while silently discarding this would be worse than one +// that failed. +type UnmigratedReport struct { + Path string + TotalKeys int + Prefixes []UnmigratedPrefix +} + +// Summary renders the report for an operator, largest groups first. +func (r *UnmigratedReport) Summary(maxPrefixes int) string { + if r == nil || r.TotalKeys == 0 { + return "no unmigrated settings were reported" + } + var b strings.Builder + fmt.Fprintf(&b, "%d v0.15 setting(s) were NOT migrated and must be recreated by hand (full list: %s)", r.TotalKeys, r.Path) + prefixes := r.Prefixes + if len(prefixes) > maxPrefixes { + prefixes = prefixes[:maxPrefixes] + } + for _, p := range prefixes { + fmt.Fprintf(&b, "\n %-32s %d keys", p.Prefix, p.Keys) + } + if len(r.Prefixes) > len(prefixes) { + fmt.Fprintf(&b, "\n ... and %d more prefix(es)", len(r.Prefixes)-len(prefixes)) + } + return b.String() +} + +// unmigratedPattern matches the report's per-prefix lines, e.g. +// " spam-filter.rule 424 keys". +var unmigratedPattern = regexp.MustCompile(`^\s+(\S+)\s+(\d+) keys\s*$`) + +// ReadUnmigratedReport parses the unmigrated.txt migrate_v016.py writes +// beside its output. A missing file is not an error - an older script, or +// a conversion with nothing left over, simply won't produce one - so +// callers get a nil report rather than a failure. +func ReadUnmigratedReport(path string) (*UnmigratedReport, error) { + data, err := os.ReadFile(path) + if os.IsNotExist(err) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("backup: read %s: %w", path, err) + } + report := &UnmigratedReport{Path: path} + for _, line := range strings.Split(string(data), "\n") { + if m := unmigratedPattern.FindStringSubmatch(line); m != nil { + n, convErr := strconv.Atoi(m[2]) + if convErr != nil { + continue + } + report.Prefixes = append(report.Prefixes, UnmigratedPrefix{Prefix: m[1], Keys: n}) + continue + } + if strings.HasPrefix(strings.TrimSpace(line), "Total unmigrated keys:") { + fields := strings.Fields(line) + if len(fields) >= 4 { + if n, convErr := strconv.Atoi(fields[3]); convErr == nil { + report.TotalKeys = n + } + } + } + } + sort.Slice(report.Prefixes, func(i, j int) bool { return report.Prefixes[i].Keys > report.Prefixes[j].Keys }) + return report, nil +} diff --git a/internal/backup/settingsdump_test.go b/internal/backup/settingsdump_test.go index c562b2e..9916c6a 100644 --- a/internal/backup/settingsdump_test.go +++ b/internal/backup/settingsdump_test.go @@ -176,3 +176,89 @@ func TestRunSettingsConvertPropagatesFailure(t *testing.T) { t.Errorf("error = %v, want it to include the script's stderr", err) } } + +// The report this parses is the most consequential output of a real +// migration: against a production instance with 12,401 settings, +// migrate_v016.py migrated 219 of them and listed the other 12,182 here. +// Losing or ignoring this file means bringing up a server that answers on +// no ports, since server.listener is among the settings that don't carry. +const sampleUnmigrated = `# Unmigrated v0.15 settings + +These v0.15 settings were not migrated by the script and must be +reviewed manually. + +Total unmigrated keys: 12182 across 69 prefixes. + + server.blocked-ip 8547 keys + lookup.url-redirectors 1076 keys + spam-filter.rule 424 keys + server.listener 26 keys + asn.expires 1 keys +` + +func TestReadUnmigratedReportParsesTotalsAndPrefixes(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "unmigrated.txt") + if err := os.WriteFile(path, []byte(sampleUnmigrated), 0o640); err != nil { + t.Fatal(err) + } + + report, err := ReadUnmigratedReport(path) + if err != nil { + t.Fatal(err) + } + if report.TotalKeys != 12182 { + t.Errorf("TotalKeys = %d, want 12182", report.TotalKeys) + } + if len(report.Prefixes) != 5 { + t.Fatalf("parsed %d prefixes, want 5", len(report.Prefixes)) + } + // Largest first, so the summary leads with what matters most. + if report.Prefixes[0].Prefix != "server.blocked-ip" || report.Prefixes[0].Keys != 8547 { + t.Errorf("first prefix = %+v, want server.blocked-ip 8547", report.Prefixes[0]) + } + + summary := report.Summary(3) + if !strings.Contains(summary, "12182") || !strings.Contains(summary, "must be recreated by hand") { + t.Errorf("summary should lead with the scale of the problem:\n%s", summary) + } + if !strings.Contains(summary, "and 2 more prefix(es)") { + t.Errorf("summary should say how much it elided:\n%s", summary) + } +} + +// An older script, or a conversion with nothing left over, writes no file. +// That is not an error. +func TestReadUnmigratedReportTreatsMissingFileAsNoReport(t *testing.T) { + report, err := ReadUnmigratedReport(filepath.Join(t.TempDir(), "absent.txt")) + if err != nil { + t.Errorf("missing report should not be an error: %v", err) + } + if report != nil { + t.Errorf("report = %+v, want nil", report) + } +} + +// migrate_v016.py writes unmigrated.txt into its working directory, so the +// convert has to run somewhere writable that the caller knows about. +func TestRunSettingsConvertRunsInTheGivenWorkDir(t *testing.T) { + dir := t.TempDir() + workDir := filepath.Join(dir, "work") + log := argsFile(t, dir) + withFakeExecutable(t, "python3", fakeScriptLoggingArgs(log, "pwd >> "+log+"\ntouch unmigrated.txt")) + + err := RunSettingsConvert(context.Background(), SettingsConvertOptions{ + ScriptPath: "/tmp/migrate_v016.py", SettingsPath: "/tmp/s.json", PrincipalsPath: "/tmp/p.json", + ConfigPath: filepath.Join(dir, "c.json"), OutputPath: filepath.Join(dir, "e.json"), + WorkDir: workDir, + }) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(readArgsFile(t, log), workDir) { + t.Errorf("script did not run in WorkDir; log:\n%s", readArgsFile(t, log)) + } + if _, err := os.Stat(filepath.Join(workDir, "unmigrated.txt")); err != nil { + t.Errorf("unmigrated.txt should land in WorkDir, not the caller's cwd: %v", err) + } +}