Merge pull request #2 from LINUXexpert-org/docker-refuse-before-stopping

Refuse a Docker deployment before stopping anything

cmd/stalwart-migrate/rehearse.go

@@ -113,7 +113,7 @@ func runRehearse(args []string) (err error) {
 		BinaryPath: *binaryPath, ConfigPath: *configPath, DataDir: *dataDir, ContainerName: *containerName,
 		AdminURL: *adminURL, AdminUser: *adminUser, AdminPassword: *adminPassword,
 		TargetVersion: *targetVersion, MinFreeMultiple: *minFree, HTTPClient: httpClient,
-		CLIPath: *stalwartCLI, PythonPath: *pythonPath, ToolCheckAdvisory: true,
+		CLIPath: *stalwartCLI, PythonPath: *pythonPath, ToolCheckAdvisory: true, DeploymentCheckAdvisory: true,
 	})
 	pfReport, err := checker.Run(ctx, store, rs)
 	fmt.Print(pfReport.String())

cmd/stalwart-migrate/run.go

@@ -253,6 +253,41 @@ func runRun(args []string) (err error) {
 	}
 	fmt.Println(controller.Target(), "stopped")
 
+	// Mail is down from here, and every return below is a return with it
+	// still down. Registered after the stop rather than before, so it only
+	// ever restarts something this tool actually stopped.
+	//
+	// It does not pretend to have recovered the migration: a run that
+	// aborted midway is still part-migrated and still needs --resume or the
+	// operator's recovery point. What it prevents is the narrower and worse
+	// outcome of the tool exiting on an error it could see coming while the
+	// server it stopped stays stopped.
+	defer func() {
+		if err == nil {
+			return
+		}
+		// The run's context may already be cancelled - that can be why we
+		// are here - and a cancelled context cannot start anything.
+		restartCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 3*time.Minute)
+		defer cancel()
+		active, activeErr := controller.Active(restartCtx)
+		if activeErr == nil && active {
+			return
+		}
+		fmt.Println("\n--- this run failed with", controller.Target(), "stopped: starting it again ---")
+		if startErr := controller.Start(restartCtx); startErr != nil {
+			fmt.Printf("could not start %s: %v\n", controller.Target(), startErr)
+			fmt.Println("MAIL IS STILL DOWN - start it by hand before anything else.")
+			return
+		}
+		if waitErr := service.WaitFor(restartCtx, controller, true, 2*time.Minute); waitErr != nil {
+			fmt.Printf("%s was asked to start but did not come up: %v\n", controller.Target(), waitErr)
+			fmt.Println("MAIL IS STILL DOWN - check it by hand before anything else.")
+			return
+		}
+		fmt.Println(controller.Target(), "is running again. The migration itself did not complete - see the error below.")
+	}()
+
 	if p.CrossesMajorBoundary {
 		fmt.Println("\n--- convert ---")
 		if _, err := store.RunStep(rs, checkpoint.PhaseStage, "convert-settings", func() (checkpoint.StepOutcome, error) {

internal/preflight/checks.go

@@ -44,7 +44,15 @@ type Options struct {
 	// about to be used and a missing one means stopping a mail server to
 	// find out.
 	ToolCheckAdvisory bool
-	HTTPClient        *http.Client
+	// DeploymentCheckAdvisory downgrades the deployment-kind check from
+	// blocking to advisory, for the same reason as ToolCheckAdvisory.
+	// `rehearse` sets it: it never stops the service or cuts over, so a
+	// deployment this tool cannot cut over is still worth rehearsing
+	// against - the reconnaissance is exactly what tells an operator what
+	// the manual path involves. `run` leaves it false, because there the
+	// alternative is finding out after mail is already down.
+	DeploymentCheckAdvisory bool
+	HTTPClient              *http.Client
 }
 
 // Checker runs the preflight checks described in ARCHITECTURE.md §4.1.
@@ -186,6 +194,20 @@ func (c *Checker) Run(ctx context.Context, store *checkpoint.Store, rs *checkpoi
 
 	deploymentOutcome, err := runCheck("deployment-kind", func() (CheckResult, string) {
 		kind := DetectDeploymentKind(ctx, c.opts.ContainerName)
+		// Docker has to fail here rather than later. Cutover refuses this
+		// deployment - recreating a container from a new image is not
+		// swapping a binary and rewriting a unit, and this tool does not
+		// automate it - but cutover runs after the service has been
+		// stopped. Refusing there means refusing with mail already down,
+		// which is how a migration attempt turned into an outage.
+		if kind == DeploymentDocker && !c.opts.DeploymentCheckAdvisory {
+			return CheckResult{
+				Status: StatusFail,
+				Detail: "detected deployment kind: docker - this tool cannot cut over a container. " +
+					"Migrating one means pulling the new image and recreating the container, which has to be done by hand; " +
+					"`rehearse` still works and will tell you what the migration involves",
+			}, string(kind)
+		}
 		status := StatusOK
 		if kind == DeploymentUnknown {
 			status = StatusWarn

internal/preflight/checks_test.go

@@ -450,3 +450,95 @@ func TestAdminAccountKindWithoutASnapshot(t *testing.T) {
 		t.Fatalf("status = %q, want a warning when there was nothing to look in", res.Status)
 	}
 }
+
+// withFakeDocker puts a `docker` on PATH that answers `inspect` successfully,
+// so DetectDeploymentKind sees a container without one being involved.
+//
+// It skips rather than fails if this host has a real stalwart systemd unit:
+// detection checks those paths first and would win, and a test that depends
+// on the host *not* having Stalwart installed is a test that fails for a
+// reason unrelated to what it is checking.
+func withFakeDocker(t *testing.T) {
+	t.Helper()
+	for _, p := range systemdUnitPaths {
+		if _, err := os.Stat(p); err == nil {
+			t.Skipf("host has %s, which detection prefers over docker", p)
+		}
+	}
+	dir := t.TempDir()
+	script := "#!/bin/sh\ncase \"$1\" in inspect) exit 0 ;; esac\nexit 1\n"
+	if err := os.WriteFile(filepath.Join(dir, "docker"), []byte(script), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH"))
+}
+
+// dockerPreflight runs a minimal but real preflight against a fake 0.15.5
+// install with a fake docker on PATH.
+func dockerPreflight(t *testing.T, advisory bool) Report {
+	t.Helper()
+	withFakeDocker(t)
+
+	counterPath := filepath.Join(t.TempDir(), "invocations")
+	binaryPath := writeFakeBinary(t, "0.15.5", counterPath)
+	configPath := filepath.Join(t.TempDir(), "config.toml")
+	if err := os.WriteFile(configPath, []byte("[server]\nhostname = \"mail.example.com\"\n"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	withFakeGithub(t, func(w http.ResponseWriter, r *http.Request) {
+		json.NewEncoder(w).Encode(Release{TagName: "v0.16.14"})
+	})
+	store := checkpoint.NewStore(t.TempDir())
+	rs, err := store.Create("", "latest")
+	if err != nil {
+		t.Fatal(err)
+	}
+	// ToolCheckAdvisory is on in both cases so the deployment flag is the
+	// only thing that varies: whether stalwart-cli happens to be installed
+	// on the machine running the tests is not what this is testing.
+	report, err := New(Options{
+		BinaryPath: binaryPath, ConfigPath: configPath, DataDir: t.TempDir(),
+		TargetVersion: "latest", ToolCheckAdvisory: true, DeploymentCheckAdvisory: advisory,
+	}).Run(context.Background(), store, rs)
+	if err != nil {
+		t.Fatalf("Run: %v", err)
+	}
+	return report
+}
+
+// A container has to be refused here, in preflight, and not later. Cutover
+// already refuses it -- but cutover runs after the service has been stopped,
+// so refusing there refuses with mail down, which turned an attempted
+// migration into an outage.
+func TestPreflightBlocksADockerDeployment(t *testing.T) {
+	report := dockerPreflight(t, false)
+	if !report.Blocking() {
+		t.Fatalf("expected a blocking report for a docker deployment, got:\n%s", report.String())
+	}
+	var found bool
+	for _, res := range report.Results {
+		if res.Name == "deployment-kind" {
+			found = true
+			if res.Status != StatusFail {
+				t.Errorf("deployment-kind status = %q, want %q", res.Status, StatusFail)
+			}
+			if !strings.Contains(res.Detail, "docker") {
+				t.Errorf("deployment-kind detail does not mention docker: %q", res.Detail)
+			}
+		}
+	}
+	if !found {
+		t.Fatalf("no deployment-kind result in report:\n%s", report.String())
+	}
+}
+
+// rehearse is read-only: it never stops anything and never cuts over, so it
+// has to keep working against a container. Telling an operator what the
+// migration involves is most useful precisely when the tool cannot do it for
+// them.
+func TestRehearseStillRunsAgainstADockerDeployment(t *testing.T) {
+	report := dockerPreflight(t, true)
+	if report.Blocking() {
+		t.Fatalf("advisory mode should not block on docker, got:\n%s", report.String())
+	}
+}