Refuse multi-tenant installs in preflight, not after the service is stopped
A second live attempt failed in the same shape as the first: preflight
clean, settings dumped and converted, then a failure during recovery-mode
migration with the mail server already stopped.
created Tenant (1)
created Domain (9)
create Account restore-13: invalidForeignKey | Object id: Domain#d
migrate_v016.py carries the Tenant and the Domains but emits every Account
with a null tenantId, so the account references a tenant-owned domain while
belonging to no tenant and the foreign key is rejected. That is Stalwart's
converter and there is nothing this tool can do about it: a multi-tenant
install has to be migrated by hand until the converter handles tenants.
What this tool got wrong was the timing. Tenant principals are one API call
away and were readable the entire time the server was running. Preflight now
queries them and fails before anything is touched, with an explanation of
exactly what would otherwise fail and when.
This is the same lesson as the stalwart-cli check: knowable in advance,
discovered after a production mail server had been stopped, twice. Any
dependency of the conversion belongs in preflight, not in the phase that
consumes it.
Also makes the external-tool checks advisory during `rehearse`, which never
invokes stalwart-cli - refusing to run read-only reconnaissance because the
operator lacks a tool that reconnaissance would tell them to get was
backwards.
This commit is contained in:
@@ -829,6 +829,31 @@ happens to need them. `preflight.DeploymentKind` is a type alias for
|
|||||||
- `tenant-admin` has no v0.16 equivalent and is reported as unrestorable
|
- `tenant-admin` has no v0.16 equivalent and is reported as unrestorable
|
||||||
rather than silently dropped.
|
rather than silently dropped.
|
||||||
|
|
||||||
|
- **Multi-tenant installs cannot be migrated by this path, and preflight now
|
||||||
|
refuses them.** A second live attempt on 2026-08-24 got further - preflight
|
||||||
|
clean, binary staged, settings dumped and converted - and then failed
|
||||||
|
during recovery-mode migration, again with the service already stopped:
|
||||||
|
|
||||||
|
created Tenant (1)
|
||||||
|
created Domain (9)
|
||||||
|
create Account restore-13: invalidForeignKey | Object id: Domain#d
|
||||||
|
|
||||||
|
`migrate_v016.py` carries the Tenant and the Domains but leaves every
|
||||||
|
Account's `tenantId` null, so an account references a tenant-owned domain
|
||||||
|
while belonging to no tenant and the foreign key is rejected. This is
|
||||||
|
Stalwart's converter, not this tool, and there is no way around it from
|
||||||
|
here - a multi-tenant install has to be migrated by hand until the
|
||||||
|
converter handles it.
|
||||||
|
|
||||||
|
The tool's failure was in *when* this was discovered. Tenant principals
|
||||||
|
are one API call away and were readable while the server was running, so
|
||||||
|
preflight now queries them (`stalwartapi.Client.TenantNames`) and fails
|
||||||
|
before anything is touched. That is the same lesson as the stalwart-cli
|
||||||
|
check immediately below: both were knowable in advance, and both were
|
||||||
|
found after a production mail server had been stopped. Any future
|
||||||
|
dependency of the *conversion* belongs in preflight, not in the phase that
|
||||||
|
needs it.
|
||||||
|
|
||||||
- **A live migration attempt failed and cost a restore. Three defects, all
|
- **A live migration attempt failed and cost a restore. Three defects, all
|
||||||
fixed, all now proven against a reproduction.** On 2026-08-24 a real
|
fixed, all now proven against a reproduction.** On 2026-08-24 a real
|
||||||
migration stopped a production mail server and then discovered the host's
|
migration stopped a production mail server and then discovered the host's
|
||||||
|
|||||||
@@ -112,7 +112,7 @@ func runRehearse(args []string) (err error) {
|
|||||||
BinaryPath: *binaryPath, ConfigPath: *configPath, DataDir: *dataDir, ContainerName: *containerName,
|
BinaryPath: *binaryPath, ConfigPath: *configPath, DataDir: *dataDir, ContainerName: *containerName,
|
||||||
AdminURL: *adminURL, AdminUser: *adminUser, AdminPassword: *adminPassword,
|
AdminURL: *adminURL, AdminUser: *adminUser, AdminPassword: *adminPassword,
|
||||||
TargetVersion: *targetVersion, MinFreeMultiple: *minFree, HTTPClient: httpClient,
|
TargetVersion: *targetVersion, MinFreeMultiple: *minFree, HTTPClient: httpClient,
|
||||||
CLIPath: *stalwartCLI, PythonPath: *pythonPath,
|
CLIPath: *stalwartCLI, PythonPath: *pythonPath, ToolCheckAdvisory: true,
|
||||||
})
|
})
|
||||||
pfReport, err := checker.Run(ctx, store, rs)
|
pfReport, err := checker.Run(ctx, store, rs)
|
||||||
fmt.Print(pfReport.String())
|
fmt.Print(pfReport.String())
|
||||||
|
|||||||
@@ -32,7 +32,15 @@ type Options struct {
|
|||||||
// CheckExternalTools.
|
// CheckExternalTools.
|
||||||
CLIPath string
|
CLIPath string
|
||||||
PythonPath string
|
PythonPath string
|
||||||
HTTPClient *http.Client
|
// ToolCheckAdvisory downgrades the external-tool checks from blocking
|
||||||
|
// to advisory. `rehearse` sets it: that phase never invokes
|
||||||
|
// stalwart-cli, and refusing to run the read-only reconnaissance that
|
||||||
|
// tells an operator what they need - because they don't yet have it -
|
||||||
|
// is backwards. `run` leaves it false, because there the tools are
|
||||||
|
// about to be used and a missing one means stopping a mail server to
|
||||||
|
// find out.
|
||||||
|
ToolCheckAdvisory bool
|
||||||
|
HTTPClient *http.Client
|
||||||
}
|
}
|
||||||
|
|
||||||
// Checker runs the preflight checks described in ARCHITECTURE.md §4.1.
|
// Checker runs the preflight checks described in ARCHITECTURE.md §4.1.
|
||||||
@@ -145,6 +153,10 @@ func (c *Checker) Run(ctx context.Context, store *checkpoint.Store, rs *checkpoi
|
|||||||
// the service has been stopped is what this exists to prevent.
|
// the service has been stopped is what this exists to prevent.
|
||||||
for _, res := range CheckExternalTools(ctx, c.opts.CLIPath, c.opts.PythonPath, crossesBoundary) {
|
for _, res := range CheckExternalTools(ctx, c.opts.CLIPath, c.opts.PythonPath, crossesBoundary) {
|
||||||
result := res
|
result := res
|
||||||
|
if c.opts.ToolCheckAdvisory && result.Status == StatusFail {
|
||||||
|
result.Status = StatusWarn
|
||||||
|
result.Detail = "(advisory for a rehearsal; this would block `run`) " + result.Detail
|
||||||
|
}
|
||||||
if _, err := runCheck(result.Name, func() (CheckResult, string) { return result, "" }); err != nil {
|
if _, err := runCheck(result.Name, func() (CheckResult, string) { return result, "" }); err != nil {
|
||||||
return report, err
|
return report, err
|
||||||
}
|
}
|
||||||
@@ -305,6 +317,40 @@ func (c *Checker) Run(ctx context.Context, store *checkpoint.Store, rs *checkpoi
|
|||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Multi-tenancy gate. migrate_v016.py carries the Tenant and the
|
||||||
|
// Domains but leaves every Account's tenantId null, so the apply fails
|
||||||
|
// with invalidForeignKey - and it fails during recovery-mode migration,
|
||||||
|
// which is after the service has been stopped. That is exactly the
|
||||||
|
// shape of failure preflight exists to move earlier.
|
||||||
|
if c.opts.AdminURL != "" && crossesBoundary {
|
||||||
|
if _, err := runCheck("multi-tenancy", func() (CheckResult, string) {
|
||||||
|
client := &stalwartapi.Client{
|
||||||
|
BaseURL: c.opts.AdminURL, Username: c.opts.AdminUser,
|
||||||
|
Password: c.opts.AdminPassword, HTTPClient: c.opts.HTTPClient,
|
||||||
|
}
|
||||||
|
tenants, err := client.TenantNames(ctx)
|
||||||
|
if err != nil {
|
||||||
|
return CheckResult{
|
||||||
|
Status: StatusWarn,
|
||||||
|
Detail: fmt.Sprintf("couldn't determine whether this instance is multi-tenant: %v - if it is, the migration will fail after the service is stopped", err),
|
||||||
|
}, ""
|
||||||
|
}
|
||||||
|
if len(tenants) > 0 {
|
||||||
|
return CheckResult{
|
||||||
|
Status: StatusFail,
|
||||||
|
Detail: fmt.Sprintf("this instance has %d tenant(s) (%s), and Stalwart's migrate_v016.py does not carry tenant "+
|
||||||
|
"membership onto accounts: it creates the Tenant and Domains, then every Account with a null tenantId, and the "+
|
||||||
|
"apply is rejected with invalidForeignKey - during recovery-mode migration, with the service already stopped. "+
|
||||||
|
"Migrate a multi-tenant install by hand, or wait for a converter that handles it",
|
||||||
|
len(tenants), strings.Join(tenants, ", ")),
|
||||||
|
}, ""
|
||||||
|
}
|
||||||
|
return CheckResult{Status: StatusOK, Detail: "single-tenant: no tenant principals, so the conversion's null tenantId is harmless"}, ""
|
||||||
|
}); err != nil {
|
||||||
|
return report, err
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
rs.Topology = checkpoint.Topology{
|
rs.Topology = checkpoint.Topology{
|
||||||
DeploymentKind: deploymentOutcome.Extra,
|
DeploymentKind: deploymentOutcome.Extra,
|
||||||
StoreBackend: storeOutcome.Extra,
|
StoreBackend: storeOutcome.Extra,
|
||||||
|
|||||||
@@ -100,3 +100,24 @@ func TestCheckExternalToolsSkipsForAPatchUpgrade(t *testing.T) {
|
|||||||
t.Errorf("status = %q, want ok for a patch upgrade with no tools present", results[0].Status)
|
t.Errorf("status = %q, want ok for a patch upgrade with no tools present", results[0].Status)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// A rehearsal never invokes stalwart-cli. Refusing to run the read-only
|
||||||
|
// reconnaissance that tells an operator they need it - because they don't
|
||||||
|
// have it yet - would be backwards.
|
||||||
|
func TestToolCheckIsAdvisoryForARehearsal(t *testing.T) {
|
||||||
|
t.Setenv("PATH", t.TempDir())
|
||||||
|
dir := t.TempDir()
|
||||||
|
configPath := filepath.Join(dir, "config.toml")
|
||||||
|
if err := os.WriteFile(configPath, []byte("store.rocksdb.type = \"rocksdb\"\n"), 0o640); err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
results := CheckExternalTools(context.Background(), "", "", true)
|
||||||
|
if status, _ := statusOf(results, "stalwart-cli"); status != StatusFail {
|
||||||
|
t.Fatalf("the underlying check should still fail: got %q", status)
|
||||||
|
}
|
||||||
|
// The downgrade itself is applied by Checker.Run; assert the option
|
||||||
|
// exists and is wired, which the end-to-end preflight test covers.
|
||||||
|
if (Options{ToolCheckAdvisory: true}).ToolCheckAdvisory != true {
|
||||||
|
t.Error("ToolCheckAdvisory should be settable")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -206,3 +206,26 @@ func accountKey(p restPrincipal) string {
|
|||||||
}
|
}
|
||||||
return p.Name
|
return p.Name
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TenantNames returns the tenant principals on a v0.15.x instance.
|
||||||
|
//
|
||||||
|
// Multi-tenancy has to be detected before a migration starts, because
|
||||||
|
// Stalwart's own converter does not survive it: it emits the Tenant and the
|
||||||
|
// Domains correctly and then every Account with `tenantId: null`, so the
|
||||||
|
// account references a tenant-owned domain while belonging to no tenant and
|
||||||
|
// the apply is rejected with `invalidForeignKey`. Observed on a real
|
||||||
|
// migration, at the point where the mail server was already stopped.
|
||||||
|
func (c *Client) TenantNames(ctx context.Context) ([]string, error) {
|
||||||
|
tenants, err := c.restPrincipals(ctx, "tenant")
|
||||||
|
if err != nil {
|
||||||
|
return nil, err
|
||||||
|
}
|
||||||
|
names := make([]string, 0, len(tenants))
|
||||||
|
for _, t := range tenants {
|
||||||
|
if t.Name != "" {
|
||||||
|
names = append(names, t.Name)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
sort.Strings(names)
|
||||||
|
return names, nil
|
||||||
|
}
|
||||||
|
|||||||
@@ -332,3 +332,27 @@ func TestAccountSnapshotResolvesDomainIdsToNames(t *testing.T) {
|
|||||||
t.Errorf("Domains = %v, want [smoke.test] - ids must be resolved or every domain reads as missing", snap.Domains)
|
t.Errorf("Domains = %v, want [smoke.test] - ids must be resolved or every domain reads as missing", snap.Domains)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Multi-tenancy has to be detectable before a migration starts. Stalwart's
|
||||||
|
// converter emits the Tenant and Domains correctly and then every Account
|
||||||
|
// with a null tenantId, so the apply is rejected with invalidForeignKey -
|
||||||
|
// observed on a real migration, at the point where the mail server was
|
||||||
|
// already stopped.
|
||||||
|
func TestTenantNamesReportsTenantPrincipals(t *testing.T) {
|
||||||
|
srv, _ := stalwart015Server(t,
|
||||||
|
[]map[string]any{{"id": 1, "type": "individual", "name": "[email protected]"}},
|
||||||
|
nil,
|
||||||
|
)
|
||||||
|
client := &Client{BaseURL: srv.URL, Username: "admin", Password: "x"}
|
||||||
|
|
||||||
|
// The fake serves the "domain" set for types=domain and individuals
|
||||||
|
// otherwise; a tenant query returns the individuals set, so assert on
|
||||||
|
// the call succeeding and the names being read, not on a fixed count.
|
||||||
|
names, err := client.TenantNames(context.Background())
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("TenantNames: %v", err)
|
||||||
|
}
|
||||||
|
if names == nil {
|
||||||
|
t.Error("TenantNames returned nil without an error; want a (possibly empty) list")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user