diff --git a/README.md b/README.md index cf6a1bb..9b91748 100644 --- a/README.md +++ b/README.md @@ -159,19 +159,6 @@ Most commonly set: Full key list in `internal/config/config.go`. -> **Migration note.** The auto-generated default moved from -> `~/.config/banger/ssh/id_ed25519` to -> `~/.local/state/banger/ssh/id_ed25519`. If you have the old path -> hardcoded in `config.toml`, either keep it (banger preserves the -> directory when `ssh_key_path` points inside it) or unset the key -> and banger will manage the new default for you. The first time the -> daemon starts against a new key, any already-running guest VMs -> still carry the previous fingerprint in their `authorized_keys`. -> Stop-and-start each VM (`banger vm stop && banger vm start -> `, or `vm restart`) to let the start-path reprovision the -> work disk with the new key. Fresh VMs and `--rm` flows are -> unaffected. - ### `vm_defaults` — sizing for new VMs Every `vm run` / `vm create` prints a `spec:` line up front showing diff --git a/docs/advanced.md b/docs/advanced.md index 1535c5b..90dee38 100644 --- a/docs/advanced.md +++ b/docs/advanced.md @@ -73,9 +73,9 @@ banger vm workspace prepare ./other-repo --guest-path /root/repo Default guest path is `/root/repo`; default mode is a shallow metadata copy plus a tracked-files overlay. Untracked files are skipped by default — pass `--include-untracked` to ship untracked -non-ignored files too (the old behaviour). Pass `--dry-run` to list -the exact file set without touching the guest. For repositories with -submodules, pass `--mode full_copy`. +non-ignored files too. Pass `--dry-run` to list the exact file set +without touching the guest. For repositories with submodules, pass +`--mode full_copy`. ## Inspecting boot failures diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index f539b6f..38df3f3 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -111,15 +111,6 @@ func TestKernelCommandExposesSubcommands(t *testing.T) { } } -func TestLegacyRemovedCommandIsRejected(t *testing.T) { - cmd := NewBangerCommand() - cmd.SetArgs([]string{"tui"}) - err := cmd.Execute() - if err == nil || !strings.Contains(err.Error(), "unknown command \"tui\"") { - t.Fatalf("Execute() error = %v, want unknown legacy command", err) - } -} - func TestDoctorCommandPrintsReportAndFailsOnHardFailures(t *testing.T) { d := defaultDeps() d.doctor = func(context.Context) (system.Report, error) { @@ -1125,9 +1116,9 @@ func TestVMRunPreflightRejectsSubmodules(t *testing.T) { repoRoot := t.TempDir() // Stub the CLI's repo-inspector with a scripted runner. Per-deps - // injection means this test no longer mutates any package global, - // so t.Parallel() is safe to add here in the future without - // worrying about racing another test's fake runner. + // injection keeps this test free of package globals, so t.Parallel + // is safe to add here in the future without racing another test's + // fake runner. d.repoInspector = &workspace.Inspector{ Runner: func(ctx context.Context, name string, args ...string) ([]byte, error) { t.Helper() @@ -1711,10 +1702,6 @@ func TestVMRunToolingHarnessScriptUsesMiseOnly(t *testing.T) { } } -// The shallow-repo-copy + checkout-script paths used to live in the -// CLI. They now live in internal/daemon/workspace and are exercised -// by that package's tests; no need to duplicate here. - func TestVMRunGuestDirIsFixed(t *testing.T) { if got := vmRunGuestDir(); got != "/root/repo" { t.Fatalf("vmRunGuestDir() = %q, want /root/repo", got) diff --git a/internal/cli/commands_ssh_config.go b/internal/cli/commands_ssh_config.go index 0bdd1b8..5ce5553 100644 --- a/internal/cli/commands_ssh_config.go +++ b/internal/cli/commands_ssh_config.go @@ -12,8 +12,7 @@ import ( // newSSHConfigCommand exposes the opt-in ergonomics for `ssh .vm`. // Default mode prints current status + the exact Include line the user // can paste into ~/.ssh/config themselves. --install does the include -// for them inside a marker-fenced block; --uninstall reverses it (also -// cleans up any legacy inline block from pre-opt-in builds). +// for them inside a marker-fenced block; --uninstall reverses it. func newSSHConfigCommand() *cobra.Command { var ( install bool diff --git a/internal/cli/vm_run.go b/internal/cli/vm_run.go index e3a049f..3c8d60d 100644 --- a/internal/cli/vm_run.go +++ b/internal/cli/vm_run.go @@ -256,8 +256,8 @@ func vmRunToolingHarnessLogPath(repoName string) string { // startVMRunToolingHarness uploads + launches the mise bootstrap // script inside the guest. repoRoot / repoName both come from the -// daemon's workspace.prepare RPC response — the CLI no longer does -// its own git inspection. +// daemon's workspace.prepare RPC response so the CLI doesn't have +// to re-inspect the git tree. func (d *deps) startVMRunToolingHarness(ctx context.Context, client vmRunGuestClient, repoRoot, repoName string, progress *vmRunProgressRenderer) error { if progress != nil { progress.render("starting guest tooling bootstrap") diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 96ff7bf..c95cc54 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -41,9 +41,9 @@ func TestLoadDefaultsResolveFirecrackerAndGenerateSSHKey(t *testing.T) { t.Fatalf("stat %s: %v", path, err) } } - legacyKey := filepath.Join(configDir, "ssh", "id_ed25519") - if _, err := os.Stat(legacyKey); err == nil { - t.Fatalf("key was also generated at legacy path %s; config.Load must not write under ConfigDir/ssh anymore", legacyKey) + forbiddenKey := filepath.Join(configDir, "ssh", "id_ed25519") + if _, err := os.Stat(forbiddenKey); err == nil { + t.Fatalf("key was also generated at %s; config.Load must not write under ConfigDir/ssh", forbiddenKey) } if cfg.DefaultImageName != "debian-bookworm" { t.Fatalf("DefaultImageName = %q, want debian-bookworm", cfg.DefaultImageName) @@ -72,8 +72,8 @@ func TestLoadSSHKeyPathExpandsHomeAnchored(t *testing.T) { // TestLoadNormalizesAbsoluteSSHKeyPath pins filepath.Clean behaviour // for configured paths: trailing slashes and duplicate slashes are -// flattened so downstream comparisons (e.g. sameDirOrParent) don't -// see two spellings for the same path. +// flattened so downstream path comparisons don't see two spellings +// for the same path. func TestLoadNormalizesAbsoluteSSHKeyPath(t *testing.T) { cases := []struct { name string diff --git a/internal/daemon/doctor_test.go b/internal/daemon/doctor_test.go index dbf0639..047333b 100644 --- a/internal/daemon/doctor_test.go +++ b/internal/daemon/doctor_test.go @@ -178,9 +178,9 @@ func TestDoctorReport_IncludesEveryDefaultCapability(t *testing.T) { report := d.doctorReport(context.Background(), nil, false) // Every registered capability that implements doctorCapability must - // contribute a check. Pre-v0.1 the defaults are work-disk, dns, nat. - // If a capability is added later it should either extend this list - // or register its own check name — either way, the assertion makes + // contribute a check. Current defaults: work-disk, dns, nat. If a + // capability is added later it should either extend this list or + // register its own check name — either way, the assertion makes // the contract visible. for _, name := range []string{ "feature /root work disk", diff --git a/internal/daemon/ssh_client_config.go b/internal/daemon/ssh_client_config.go index 063e7e7..455cb6a 100644 --- a/internal/daemon/ssh_client_config.go +++ b/internal/daemon/ssh_client_config.go @@ -12,19 +12,9 @@ import ( "banger/internal/paths" ) -// Marker sentinels. -// -// vmSSHConfigIncludeBegin / vmSSHConfigIncludeEnd used to wrap the full -// Host *.vm stanza when banger wrote directly into ~/.ssh/config. -// We keep the sentinel strings only so uninstall can find and remove -// legacy blocks on systems that upgraded from that behaviour. -// -// The new opt-in flow writes a short Include block with its own marker -// pair; the daemon itself no longer touches ~/.ssh/config at all. +// Marker sentinels that fence the `Include` block banger writes into +// ~/.ssh/config when the user runs `banger ssh-config --install`. const ( - vmSSHConfigIncludeBegin = "# BEGIN BANGER MANAGED VM SSH" - vmSSHConfigIncludeEnd = "# END BANGER MANAGED VM SSH" - bangerSSHIncludeBegin = "# BEGIN BANGER SSH INCLUDE" bangerSSHIncludeEnd = "# END BANGER SSH INCLUDE" ) @@ -78,11 +68,6 @@ func (d *Daemon) ensureVMSSHClientConfig() { // // The file lives in the banger config dir so users who manage their // SSH config declaratively can decide how (or whether) to pull it in. -// A narrow migration step also runs here: the pre-opt-in daemon -// wrote a sibling file at $ConfigDir/ssh/ssh_config. Remove only -// that specific legacy file, then remove the enclosing directory -// only if it's empty — never os.RemoveAll, because the user may -// have pointed ssh_key_path at a key under that directory. func syncVMSSHClientConfig(layout paths.Layout, keyPath string) error { keyPath = strings.TrimSpace(keyPath) if keyPath == "" { @@ -96,79 +81,12 @@ func syncVMSSHClientConfig(layout paths.Layout, keyPath string) error { return err } block := renderManagedVMSSHBlock(keyPath, layout.KnownHostsPath) - if err := writeTextFileIfChanged(target, block, 0o644); err != nil { - return err - } - - cleanupLegacySSHConfigDir(layout, keyPath) - return nil -} - -// cleanupLegacySSHConfigDir removes the pre-opt-in sibling file at -// $ConfigDir/ssh/ssh_config and, if the directory is then empty, the -// directory itself. Skips the whole operation when ssh_key_path -// resolves under that directory — users who explicitly configured a -// key there must not have the enclosing dir yanked out from under -// them. All errors are swallowed: this is best-effort migration, not -// a hard failure mode. -func cleanupLegacySSHConfigDir(layout paths.Layout, keyPath string) { - legacyDir := filepath.Join(layout.ConfigDir, "ssh") - if sameDirOrParent(legacyDir, keyPath) { - return - } - _ = os.Remove(filepath.Join(legacyDir, "ssh_config")) - // Remove the dir only if it's now empty. os.Remove returns - // ENOTEMPTY when it isn't, which is the signal we want. - _ = os.Remove(legacyDir) -} - -// sameDirOrParent reports whether dir contains path (or equals it) -// after resolving symlinks. Used to gate destructive cleanup against -// a configured key that lives inside the cleanup target — either -// directly or via a symlinked spelling of the same physical -// location. Lexical comparison alone would miss the symlink case -// and let the scrub delete a user key aliased through an symlinked -// directory. -func sameDirOrParent(dir, path string) bool { - if strings.TrimSpace(dir) == "" || strings.TrimSpace(path) == "" { - return false - } - absDir, err := resolvePathForComparison(dir) - if err != nil { - return false - } - absPath, err := resolvePathForComparison(path) - if err != nil { - return false - } - rel, err := filepath.Rel(absDir, absPath) - if err != nil { - return false - } - // filepath.Rel returns "../..." when absPath is outside absDir. - // A path inside (or equal to) the dir starts with "." or a - // non-".." prefix. - return rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) -} - -// resolvePathForComparison returns an absolute, symlink-resolved -// version of p. Falls back to filepath.Abs when EvalSymlinks errors -// — typically because p refers to a file or directory that doesn't -// exist yet, which is fine for comparison purposes: two non-existent -// paths compared lexically is the best we can do and matches the -// pre-symlink-aware behaviour. -func resolvePathForComparison(p string) (string, error) { - if resolved, err := filepath.EvalSymlinks(p); err == nil { - return resolved, nil - } - return filepath.Abs(p) + return writeTextFileIfChanged(target, block, 0o644) } // InstallUserSSHInclude adds an `Include ` line // to ~/.ssh/config inside a banger-owned marker block. Idempotent: -// running it twice leaves a single block. Also strips any legacy -// inline `Host *.vm` banger block left over from the pre-opt-in -// era so the user ends up with the Include-only layout. +// running it twice leaves a single block. func InstallUserSSHInclude(layout paths.Layout) error { bangerConfig := BangerSSHConfigPath(layout) if bangerConfig == "" { @@ -182,21 +100,17 @@ func InstallUserSSHInclude(layout paths.Layout) error { if err != nil { return err } - stripped, err := removeManagedBlock(existing, vmSSHConfigIncludeBegin, vmSSHConfigIncludeEnd) - if err != nil { - return err - } block := renderBangerSSHIncludeBlock(bangerConfig) - updated, err := upsertManagedBlock(stripped, bangerSSHIncludeBegin, bangerSSHIncludeEnd, block) + updated, err := upsertManagedBlock(existing, bangerSSHIncludeBegin, bangerSSHIncludeEnd, block) if err != nil { return err } return writeTextFileIfChanged(userConfigPath, updated, 0o600) } -// UninstallUserSSHInclude removes the Include block (and any legacy -// inline Host *.vm block) from ~/.ssh/config. Idempotent: missing -// file or missing block is a no-op. +// UninstallUserSSHInclude removes the Include block from +// ~/.ssh/config. Idempotent: missing file or missing block is a +// no-op. func UninstallUserSSHInclude() error { userConfigPath, err := userSSHConfigPath() if err != nil { @@ -213,16 +127,12 @@ func UninstallUserSSHInclude() error { if err != nil { return err } - updated, err = removeManagedBlock(updated, vmSSHConfigIncludeBegin, vmSSHConfigIncludeEnd) - if err != nil { - return err - } return writeTextFileIfChanged(userConfigPath, updated, 0o600) } -// UserSSHIncludeInstalled reports whether ~/.ssh/config contains -// either the new Include block or a legacy inline banger block. -// Used by `ssh-config` (status readout) and `doctor`. +// UserSSHIncludeInstalled reports whether ~/.ssh/config contains the +// banger Include block. Used by `ssh-config` (status readout) and +// `doctor`. func UserSSHIncludeInstalled() (bool, error) { userConfigPath, err := userSSHConfigPath() if err != nil { @@ -232,13 +142,7 @@ func UserSSHIncludeInstalled() (bool, error) { if err != nil { return false, err } - if strings.Contains(existing, bangerSSHIncludeBegin) { - return true, nil - } - if strings.Contains(existing, vmSSHConfigIncludeBegin) { - return true, nil - } - return false, nil + return strings.Contains(existing, bangerSSHIncludeBegin), nil } func userSSHConfigPath() (string, error) { diff --git a/internal/daemon/ssh_client_config_test.go b/internal/daemon/ssh_client_config_test.go index 8c16569..907665d 100644 --- a/internal/daemon/ssh_client_config_test.go +++ b/internal/daemon/ssh_client_config_test.go @@ -9,200 +9,6 @@ import ( "banger/internal/paths" ) -// TestSameDirOrParentHandlesSymlinks guards against a drift where -// sameDirOrParent (the gate that protects a user key under the -// legacy dir from the cleanup scrub) compares lexical paths and -// misses symlink aliasing. -// -// Scenario: user configured ssh_key_path at a path that lands inside -// ConfigDir/ssh via a symlink (e.g. ConfigDir is itself symlinked, -// or the user maintains a symlink alias for their key tree). The -// gate must resolve both sides to the same physical location and -// refuse to scrub. -func TestSameDirOrParentHandlesSymlinks(t *testing.T) { - physical := t.TempDir() - realDir := filepath.Join(physical, "real-ssh") - if err := os.Mkdir(realDir, 0o700); err != nil { - t.Fatalf("Mkdir: %v", err) - } - realKey := filepath.Join(realDir, "id_ed25519") - if err := os.WriteFile(realKey, []byte("PRIVATE"), 0o600); err != nil { - t.Fatalf("WriteFile: %v", err) - } - - // A symlink that aliases the whole real-ssh directory. The user - // configured ssh_key_path via this alias, but sameDirOrParent is - // called with the canonical (realDir) legacyDir path. - aliasDir := filepath.Join(physical, "alias-ssh") - if err := os.Symlink(realDir, aliasDir); err != nil { - t.Skipf("symlink unsupported on this filesystem: %v", err) - } - aliasKey := filepath.Join(aliasDir, "id_ed25519") - - if !sameDirOrParent(realDir, aliasKey) { - t.Fatalf("sameDirOrParent(%q, %q) = false; symlinked key was not recognised as inside the dir — cleanup would delete it", realDir, aliasKey) - } - - // Reverse direction: dir provided as a symlink, key as canonical. - if !sameDirOrParent(aliasDir, realKey) { - t.Fatalf("sameDirOrParent(%q, %q) = false; reverse symlink direction also missed", aliasDir, realKey) - } - - // Negative: a key in a completely unrelated directory must not - // be reported inside either spelling of the legacy dir. - outside := filepath.Join(t.TempDir(), "other", "id_ed25519") - if err := os.MkdirAll(filepath.Dir(outside), 0o700); err != nil { - t.Fatalf("MkdirAll: %v", err) - } - if err := os.WriteFile(outside, []byte("UNRELATED"), 0o600); err != nil { - t.Fatalf("WriteFile: %v", err) - } - if sameDirOrParent(realDir, outside) { - t.Fatalf("sameDirOrParent(%q, %q) = true; unrelated dir incorrectly flagged as inside", realDir, outside) - } -} - -// A user-configured ssh_key_path that happens to live under the -// legacy $ConfigDir/ssh directory must survive the pre-opt-in -// migration cleanup. The old code did os.RemoveAll on the whole -// directory, which nuked the key. Pin the narrower behavior so a -// future refactor can't re-broaden the scrub. -func TestSyncVMSSHClientConfigPreservesUserKeyInLegacyDir(t *testing.T) { - homeDir := t.TempDir() - t.Setenv("HOME", homeDir) - - configDir := filepath.Join(homeDir, ".config", "banger") - legacyDir := filepath.Join(configDir, "ssh") - if err := os.MkdirAll(legacyDir, 0o700); err != nil { - t.Fatalf("MkdirAll: %v", err) - } - userKey := filepath.Join(legacyDir, "id_ed25519") - if err := os.WriteFile(userKey, []byte("PRIVATE"), 0o600); err != nil { - t.Fatalf("WriteFile: %v", err) - } - // A stale ssh_config under the same dir from pre-opt-in era. - legacyConfig := filepath.Join(legacyDir, "ssh_config") - if err := os.WriteFile(legacyConfig, []byte("stale"), 0o644); err != nil { - t.Fatalf("WriteFile: %v", err) - } - - layout := paths.Layout{ - ConfigDir: configDir, - KnownHostsPath: filepath.Join(homeDir, ".local", "state", "banger", "ssh", "known_hosts"), - } - if err := syncVMSSHClientConfig(layout, userKey); err != nil { - t.Fatalf("syncVMSSHClientConfig: %v", err) - } - - // The configured key must survive. - if _, err := os.Stat(userKey); err != nil { - t.Fatalf("user-configured key disappeared: %v", err) - } - // Enclosing directory must also survive (it contains the key). - if _, err := os.Stat(legacyDir); err != nil { - t.Fatalf("legacy dir removed despite containing the configured key: %v", err) - } - // The stale legacy ssh_config file can still be gone in this - // case — the user's key isn't ssh_config, so cleaning up the - // sibling file is fine. We don't assert either way, since the - // gate is "don't delete the user's key" not "always delete the - // sibling file." -} - -// With ssh_key_path configured outside ConfigDir/ssh, the legacy -// migration step should scrub the old sibling file and then the -// (now-empty) directory — no os.RemoveAll on anything still in use. -func TestSyncVMSSHClientConfigNarrowsCleanupToLegacyFile(t *testing.T) { - homeDir := t.TempDir() - t.Setenv("HOME", homeDir) - - configDir := filepath.Join(homeDir, ".config", "banger") - legacyDir := filepath.Join(configDir, "ssh") - if err := os.MkdirAll(legacyDir, 0o700); err != nil { - t.Fatalf("MkdirAll: %v", err) - } - // Simulate the pre-opt-in leftover: just the ssh_config file. - legacyConfig := filepath.Join(legacyDir, "ssh_config") - if err := os.WriteFile(legacyConfig, []byte("stale"), 0o644); err != nil { - t.Fatalf("WriteFile: %v", err) - } - - // ssh_key_path lives in the state dir (the new default location). - stateDir := filepath.Join(homeDir, ".local", "state", "banger", "ssh") - if err := os.MkdirAll(stateDir, 0o700); err != nil { - t.Fatalf("MkdirAll: %v", err) - } - userKey := filepath.Join(stateDir, "id_ed25519") - if err := os.WriteFile(userKey, []byte("PRIVATE"), 0o600); err != nil { - t.Fatalf("WriteFile: %v", err) - } - - layout := paths.Layout{ - ConfigDir: configDir, - KnownHostsPath: filepath.Join(homeDir, ".local", "state", "banger", "ssh", "known_hosts"), - } - if err := syncVMSSHClientConfig(layout, userKey); err != nil { - t.Fatalf("syncVMSSHClientConfig: %v", err) - } - - // Legacy ssh_config file: gone. - if _, err := os.Stat(legacyConfig); !os.IsNotExist(err) { - t.Fatalf("legacy ssh_config survived cleanup: %v", err) - } - // Legacy dir: gone, since it was empty after the file removal. - if _, err := os.Stat(legacyDir); !os.IsNotExist(err) { - t.Fatalf("legacy dir survived cleanup when empty: %v", err) - } - // User's key: untouched. - if _, err := os.Stat(userKey); err != nil { - t.Fatalf("user key disappeared: %v", err) - } -} - -// If the legacy dir contains UNEXPECTED files (not ssh_config, not -// the configured key), leave the dir alone. os.Remove on a non- -// empty dir errors with ENOTEMPTY, which we swallow. Regression -// guard so the cleanup can never escalate to recursive deletion. -func TestSyncVMSSHClientConfigLeavesUnexpectedLegacyContents(t *testing.T) { - homeDir := t.TempDir() - t.Setenv("HOME", homeDir) - - configDir := filepath.Join(homeDir, ".config", "banger") - legacyDir := filepath.Join(configDir, "ssh") - if err := os.MkdirAll(legacyDir, 0o700); err != nil { - t.Fatalf("MkdirAll: %v", err) - } - // A user-managed file we have no business removing. - userFile := filepath.Join(legacyDir, "my-other-thing") - if err := os.WriteFile(userFile, []byte("mine"), 0o600); err != nil { - t.Fatalf("WriteFile: %v", err) - } - - layout := paths.Layout{ - ConfigDir: configDir, - KnownHostsPath: filepath.Join(homeDir, ".local", "state", "banger", "ssh", "known_hosts"), - } - // ssh_key_path lives elsewhere; cleanup would otherwise proceed. - stateKey := filepath.Join(homeDir, ".local", "state", "banger", "ssh", "id_ed25519") - if err := os.MkdirAll(filepath.Dir(stateKey), 0o700); err != nil { - t.Fatalf("MkdirAll: %v", err) - } - if err := os.WriteFile(stateKey, []byte("PRIVATE"), 0o600); err != nil { - t.Fatalf("WriteFile: %v", err) - } - - if err := syncVMSSHClientConfig(layout, stateKey); err != nil { - t.Fatalf("syncVMSSHClientConfig: %v", err) - } - - if _, err := os.Stat(userFile); err != nil { - t.Fatalf("user-managed legacy-dir file disappeared: %v", err) - } - if _, err := os.Stat(legacyDir); err != nil { - t.Fatalf("legacy dir vanished despite non-empty contents: %v", err) - } -} - // Under the opt-in contract the daemon writes its own ssh_config file // and never touches ~/.ssh/config on its own. func TestSyncVMSSHClientConfigWritesBangerFileOnly(t *testing.T) { @@ -240,17 +46,6 @@ func TestSyncVMSSHClientConfigWritesBangerFileOnly(t *testing.T) { if _, err := os.Stat(filepath.Join(homeDir, ".ssh", "config")); !os.IsNotExist(err) { t.Fatalf("~/.ssh/config should be untouched; stat err = %v", err) } - - // Regression: the legacy posture (strict no + /dev/null) must not - // reappear in the banger file. - for _, must := range []string{ - "StrictHostKeyChecking no", - "UserKnownHostsFile /dev/null", - } { - if strings.Contains(string(bangerConfig), must) { - t.Fatalf("banger ssh_config leaked legacy posture %q:\n%s", must, bangerConfig) - } - } } func TestInstallUserSSHIncludeAddsIncludeBlock(t *testing.T) { @@ -307,64 +102,7 @@ func TestInstallUserSSHIncludeIsIdempotent(t *testing.T) { } } -func TestInstallUserSSHIncludeMigratesLegacyInlineBlock(t *testing.T) { - homeDir := t.TempDir() - t.Setenv("HOME", homeDir) - - layout := paths.Layout{ConfigDir: filepath.Join(homeDir, ".config", "banger")} - if err := os.MkdirAll(layout.ConfigDir, 0o755); err != nil { - t.Fatalf("MkdirAll: %v", err) - } - if err := os.WriteFile(BangerSSHConfigPath(layout), []byte("Host *.vm\n"), 0o644); err != nil { - t.Fatalf("WriteFile: %v", err) - } - - sshDir := filepath.Join(homeDir, ".ssh") - if err := os.MkdirAll(sshDir, 0o700); err != nil { - t.Fatalf("MkdirAll(.ssh): %v", err) - } - legacy := strings.Join([]string{ - "ServerAliveInterval 120", - "", - vmSSHConfigIncludeBegin, - "Host *.vm", - " User root", - " IdentityFile /some/old/key", - vmSSHConfigIncludeEnd, - "", - "Host other", - " HostName 192.0.2.5", - "", - }, "\n") - if err := os.WriteFile(filepath.Join(sshDir, "config"), []byte(legacy), 0o600); err != nil { - t.Fatalf("seed legacy config: %v", err) - } - - if err := InstallUserSSHInclude(layout); err != nil { - t.Fatalf("InstallUserSSHInclude: %v", err) - } - got, err := os.ReadFile(filepath.Join(sshDir, "config")) - if err != nil { - t.Fatalf("ReadFile: %v", err) - } - gotStr := string(got) - // Legacy inline block must be gone. - if strings.Contains(gotStr, vmSSHConfigIncludeBegin) { - t.Fatalf("legacy inline block survived:\n%s", gotStr) - } - // New Include block must be present. - if !strings.Contains(gotStr, bangerSSHIncludeBegin) { - t.Fatalf("new include block missing:\n%s", gotStr) - } - // Unrelated stanzas must be preserved. - for _, want := range []string{"ServerAliveInterval 120", "Host other"} { - if !strings.Contains(gotStr, want) { - t.Fatalf("user config lost unrelated entry %q:\n%s", want, gotStr) - } - } -} - -func TestUninstallUserSSHIncludeRemovesBothMarkerBlocks(t *testing.T) { +func TestUninstallUserSSHIncludeRemovesIncludeBlock(t *testing.T) { homeDir := t.TempDir() t.Setenv("HOME", homeDir) @@ -376,10 +114,6 @@ func TestUninstallUserSSHIncludeRemovesBothMarkerBlocks(t *testing.T) { "Host keep", " HostName 198.51.100.1", "", - vmSSHConfigIncludeBegin, - "Host *.vm", - vmSSHConfigIncludeEnd, - "", bangerSSHIncludeBegin, "Include /tmp/banger-ssh-config", bangerSSHIncludeEnd, @@ -397,10 +131,8 @@ func TestUninstallUserSSHIncludeRemovesBothMarkerBlocks(t *testing.T) { t.Fatalf("ReadFile: %v", err) } gotStr := string(got) - for _, banned := range []string{vmSSHConfigIncludeBegin, bangerSSHIncludeBegin} { - if strings.Contains(gotStr, banned) { - t.Fatalf("residue of %q:\n%s", banned, gotStr) - } + if strings.Contains(gotStr, bangerSSHIncludeBegin) { + t.Fatalf("begin marker survived uninstall:\n%s", gotStr) } if !strings.Contains(gotStr, "Host keep") { t.Fatalf("lost unrelated entry:\n%s", gotStr) @@ -419,7 +151,7 @@ func TestUninstallUserSSHIncludeIsNoOpWhenMissing(t *testing.T) { } } -func TestUserSSHIncludeInstalledDetectsBothMarkers(t *testing.T) { +func TestUserSSHIncludeInstalledDetectsMarker(t *testing.T) { for _, tc := range []struct { name string seed string @@ -427,8 +159,7 @@ func TestUserSSHIncludeInstalledDetectsBothMarkers(t *testing.T) { }{ {"missing file", "", false}, {"unrelated only", "Host other\n HostName 1.2.3.4\n", false}, - {"legacy marker", vmSSHConfigIncludeBegin + "\nHost *.vm\n" + vmSSHConfigIncludeEnd + "\n", true}, - {"new marker", bangerSSHIncludeBegin + "\nInclude /tmp/banger\n" + bangerSSHIncludeEnd + "\n", true}, + {"installed", bangerSSHIncludeBegin + "\nInclude /tmp/banger\n" + bangerSSHIncludeEnd + "\n", true}, } { t.Run(tc.name, func(t *testing.T) { homeDir := t.TempDir() diff --git a/internal/daemon/vm.go b/internal/daemon/vm.go index ec9b1be..3bdff67 100644 --- a/internal/daemon/vm.go +++ b/internal/daemon/vm.go @@ -13,6 +13,7 @@ import ( "banger/internal/model" "banger/internal/namegen" "banger/internal/system" + "banger/internal/vmdns" ) // Cross-service constants. Kept in vm.go because both lifecycle @@ -46,18 +47,11 @@ func (s *VMService) rebuildDNS(ctx context.Context) error { if strings.TrimSpace(vm.Runtime.GuestIP) == "" { continue } - records[vmDNSRecordName(vm.Name)] = vm.Runtime.GuestIP + records[vmdns.RecordName(vm.Name)] = vm.Runtime.GuestIP } return s.net.replaceDNS(records) } -// vmDNSRecordName is a small indirection so the dns-record-name -// helper is not directly pulled into every file that used to import -// vmdns for this one call. Equivalent to vmdns.RecordName. -func vmDNSRecordName(name string) string { - return strings.ToLower(strings.TrimSpace(name)) + ".vm" -} - // cleanupRuntime tears down the host-side state for a VM: firecracker // process, DM snapshot, capabilities, tap, sockets. Lives on VMService // because it reaches into handles (VMService-owned); the capability diff --git a/internal/daemon/vm_disk.go b/internal/daemon/vm_disk.go index 276f577..7cb53ee 100644 --- a/internal/daemon/vm_disk.go +++ b/internal/daemon/vm_disk.go @@ -172,13 +172,6 @@ func (s *VMService) ensureWorkDisk(ctx context.Context, vm *model.VMRecord, imag // Pins the lookup path so the banger-written file always wins, // regardless of distro default ($HOME/.ssh/authorized_keys) and // regardless of any per-image weirdness. -// -// Previously this file also contained `LogLevel DEBUG3` and -// `StrictModes no`. DEBUG3 was a leftover from debugging the -// first-boot flow and flooded journald in normal use. StrictModes no -// was a workaround for perm drift on /root inside the work disk; the -// real fix — normalising /root permissions at provisioning time — is -// in ensureAuthorizedKeyOnWorkDisk / seedAuthorizedKeyOnExt4Image. func sshdGuestConfig() string { return strings.Join([]string{ "PermitRootLogin prohibit-password", diff --git a/internal/daemon/vm_handles.go b/internal/daemon/vm_handles.go index b628cd0..c52c938 100644 --- a/internal/daemon/vm_handles.go +++ b/internal/daemon/vm_handles.go @@ -123,8 +123,8 @@ func (s *VMService) setVMHandlesInMemory(vmID string, h model.VMHandles) { } // vmHandles returns the cached handles for vm (zero-value if no -// entry). Call sites that previously read `vm.Runtime.{PID,...}` -// should read through this instead. +// entry). The in-process handle cache is the authoritative source +// for PID / loops / dm-name — VMRecord.Runtime holds only paths. func (s *VMService) vmHandles(vmID string) model.VMHandles { if s == nil { return model.VMHandles{} diff --git a/internal/daemon/vm_test.go b/internal/daemon/vm_test.go index 7acd0c1..e131e94 100644 --- a/internal/daemon/vm_test.go +++ b/internal/daemon/vm_test.go @@ -798,8 +798,8 @@ func TestEnsureAuthorizedKeyOnWorkDiskRepairsNestedRootLayout(t *testing.T) { if err := os.WriteFile(filepath.Join(nestedHome, ".bashrc"), []byte("export TEST_PROMPT=1\n"), 0o644); err != nil { t.Fatalf("WriteFile(.bashrc): %v", err) } - legacyKey := "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAILEgacykey legacy@test\n" - if err := os.WriteFile(filepath.Join(nestedHome, ".ssh", "authorized_keys"), []byte(legacyKey), 0o600); err != nil { + existingKey := "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAILEgacykey existing@test\n" + if err := os.WriteFile(filepath.Join(nestedHome, ".ssh", "authorized_keys"), []byte(existingKey), 0o600); err != nil { t.Fatalf("WriteFile(authorized_keys): %v", err) } @@ -838,8 +838,8 @@ func TestEnsureAuthorizedKeyOnWorkDiskRepairsNestedRootLayout(t *testing.T) { t.Fatalf("ReadFile(authorized_keys): %v", err) } content := string(data) - if !strings.Contains(content, strings.TrimSpace(legacyKey)) { - t.Fatalf("authorized_keys missing legacy key: %q", content) + if !strings.Contains(content, strings.TrimSpace(existingKey)) { + t.Fatalf("authorized_keys missing pre-existing key: %q", content) } if !strings.Contains(content, "ssh-rsa ") { t.Fatalf("authorized_keys missing managed key: %q", content) diff --git a/internal/daemon/workspace_test.go b/internal/daemon/workspace_test.go index b2d08f3..e98477d 100644 --- a/internal/daemon/workspace_test.go +++ b/internal/daemon/workspace_test.go @@ -456,8 +456,9 @@ func TestPrepareVMWorkspace_ReleasesVMLockDuringGuestIO(t *testing.T) { } // TestPrepareVMWorkspace_SerialisesConcurrentPreparesOnSameVM asserts -// the workspaceLocks scope: two concurrent prepares on the same VM do -// NOT interleave, even though they no longer take the core VM mutex. +// the workspaceLocks scope: two concurrent prepares on the same VM +// serialise via workspaceLocks even though they don't hold the core +// VM mutex, so a lifecycle op (stop/delete) isn't blocked. func TestPrepareVMWorkspace_SerialisesConcurrentPreparesOnSameVM(t *testing.T) { t.Parallel() ctx := context.Background() diff --git a/internal/store/migrations.go b/internal/store/migrations.go index 2490942..059ec7e 100644 --- a/internal/store/migrations.go +++ b/internal/store/migrations.go @@ -24,15 +24,10 @@ type migration struct { // entries — installed DBs key off the id column. var migrations = []migration{ {id: 1, name: "baseline", up: migrateBaseline}, - {id: 2, name: "drop_dead_image_columns", up: migrateDropDeadImageColumns}, } // runMigrations ensures schema_migrations exists, then applies every -// migration whose id hasn't been recorded yet, in id order. Existing -// dev databases (schema set up by the pre-versioning inline migrate() -// helper) see the baseline SQL as a no-op because every statement is -// `CREATE TABLE IF NOT EXISTS`; the row that records id=1 is what -// brings them into the new system. +// migration whose id hasn't been recorded yet, in id order. func runMigrations(db *sql.DB) error { if _, err := db.Exec(`CREATE TABLE IF NOT EXISTS schema_migrations ( id INTEGER PRIMARY KEY, @@ -105,11 +100,7 @@ func applyMigration(db *sql.DB, m migration) error { return tx.Commit() } -// migrateBaseline captures the schema as it stood when the versioned -// migration system was introduced. Uses IF NOT EXISTS on every object -// so existing dev databases — whose tables were set up by the old -// inline migrate() — pass through cleanly and only the -// schema_migrations row gets added. +// migrateBaseline creates the full current schema. func migrateBaseline(tx *sql.Tx) error { stmts := []string{ `CREATE TABLE IF NOT EXISTS images ( @@ -122,7 +113,6 @@ func migrateBaseline(tx *sql.Tx) error { kernel_path TEXT NOT NULL, initrd_path TEXT, modules_dir TEXT, - packages_path TEXT, build_size TEXT, seeded_ssh_public_key_fingerprint TEXT, docker INTEGER NOT NULL DEFAULT 0, @@ -149,99 +139,5 @@ func migrateBaseline(tx *sql.Tx) error { return err } } - // Columns added to the images table across the pre-versioning - // lifetime of the project. New installs get them from the CREATE - // TABLE above; upgraders from an ancient snapshot (pre- - // ensureColumnExists) pick them up here. Idempotent either way. - for _, col := range []struct{ table, name, typ string }{ - {"images", "work_seed_path", "TEXT"}, - {"images", "seeded_ssh_public_key_fingerprint", "TEXT"}, - } { - if err := addColumnIfMissing(tx, col.table, col.name, col.typ); err != nil { - return err - } - } return nil } - -// migrateDropDeadImageColumns removes image-table columns that the -// store never reads or writes. `packages_path` was introduced for a -// build pipeline that no longer exists; the baseline migration still -// creates it for historical fidelity, and this migration drops it on -// new installs + any upgrader that still carries it. Idempotent via -// dropColumnIfExists so running the migration twice (or against a -// DB where the column was already gone) is a no-op. -func migrateDropDeadImageColumns(tx *sql.Tx) error { - return dropColumnIfExists(tx, "images", "packages_path") -} - -// dropColumnIfExists is SQLite's "ALTER TABLE DROP COLUMN IF EXISTS" -// (which the dialect lacks) as a library function. modernc.org/sqlite -// bundles SQLite 3.42+, which supports plain DROP COLUMN — we add the -// existence guard so the statement is idempotent across repeat runs -// and legacy DBs that never had the column in the first place. -func dropColumnIfExists(tx *sql.Tx, table, column string) error { - rows, err := tx.Query(fmt.Sprintf("PRAGMA table_info(%s)", table)) - if err != nil { - return err - } - defer rows.Close() - var found bool - for rows.Next() { - var ( - cid int - name string - valueType string - notNull int - defaultV sql.NullString - pk int - ) - if err := rows.Scan(&cid, &name, &valueType, ¬Null, &defaultV, &pk); err != nil { - return err - } - if name == column { - found = true - } - } - if err := rows.Err(); err != nil { - return err - } - if !found { - return nil - } - _, err = tx.Exec(fmt.Sprintf("ALTER TABLE %s DROP COLUMN %s", table, column)) - return err -} - -// addColumnIfMissing is SQLite's "ALTER TABLE ADD COLUMN IF NOT EXISTS" -// (which the dialect lacks) as a library function. Used inside -// migrations when a column needs to survive a database that went -// through some historical path where the column was added later. -func addColumnIfMissing(tx *sql.Tx, table, column, columnType string) error { - rows, err := tx.Query(fmt.Sprintf("PRAGMA table_info(%s)", table)) - if err != nil { - return err - } - defer rows.Close() - for rows.Next() { - var ( - cid int - name string - valueType string - notNull int - defaultV sql.NullString - pk int - ) - if err := rows.Scan(&cid, &name, &valueType, ¬Null, &defaultV, &pk); err != nil { - return err - } - if name == column { - return nil - } - } - if err := rows.Err(); err != nil { - return err - } - _, err = tx.Exec(fmt.Sprintf("ALTER TABLE %s ADD COLUMN %s %s", table, column, columnType)) - return err -} diff --git a/internal/store/migrations_test.go b/internal/store/migrations_test.go index fd0ba71..32bcb1a 100644 --- a/internal/store/migrations_test.go +++ b/internal/store/migrations_test.go @@ -141,98 +141,18 @@ func TestApplyMigrationRollsBackOnBodyError(t *testing.T) { } } -// TestMigrateDropDeadImageColumns_AcrossInstallPaths verifies the -// drop-column migration is correct on both paths it can land on: -// a fresh install (baseline created the column, migration 2 drops -// it) and a legacy DB that somehow lost or never had the column -// (migration 2 is a no-op). Runs migrations end-to-end so the -// invariant-check is the real system, not the helper in isolation. -func TestMigrateDropDeadImageColumns_AcrossInstallPaths(t *testing.T) { - hasColumn := func(t *testing.T, db *sql.DB, table, column string) bool { - t.Helper() - rows, err := db.Query("PRAGMA table_info(" + table + ")") - if err != nil { - t.Fatalf("PRAGMA table_info: %v", err) - } - defer rows.Close() - for rows.Next() { - var ( - cid int - name string - valueType string - notNull int - defaultV sql.NullString - pk int - ) - if err := rows.Scan(&cid, &name, &valueType, ¬Null, &defaultV, &pk); err != nil { - t.Fatalf("scan table_info row: %v", err) - } - if name == column { - return true - } - } - if err := rows.Err(); err != nil { - t.Fatalf("rows.Err: %v", err) - } - return false - } - - t.Run("fresh install drops packages_path", func(t *testing.T) { - db := openRawDB(t) - if err := runMigrations(db); err != nil { - t.Fatalf("runMigrations: %v", err) - } - if hasColumn(t, db, "images", "packages_path") { - t.Fatal("packages_path column survived migration 2 on fresh install") - } - }) - - t.Run("legacy DB without column is a no-op", func(t *testing.T) { - db := openRawDB(t) - // Simulate a DB whose baseline was applied against a modified - // schema that never had packages_path: seed schema_migrations, - // run baseline, drop the column out-of-band, then run - // runMigrations and expect migration 2 to succeed regardless. - if _, err := db.Exec(`CREATE TABLE IF NOT EXISTS schema_migrations ( - id INTEGER PRIMARY KEY, - name TEXT NOT NULL, - applied_at TEXT NOT NULL - )`); err != nil { - t.Fatalf("seed schema_migrations: %v", err) - } - if err := applyMigration(db, migrations[0]); err != nil { - t.Fatalf("apply baseline: %v", err) - } - if _, err := db.Exec("ALTER TABLE images DROP COLUMN packages_path"); err != nil { - t.Fatalf("pre-drop packages_path: %v", err) - } - if err := runMigrations(db); err != nil { - t.Fatalf("runMigrations after manual pre-drop: %v", err) - } - if hasColumn(t, db, "images", "packages_path") { - t.Fatal("packages_path reappeared after runMigrations") - } - }) -} - // TestOpenReadOnlyDoesNotRunMigrations pins the doctor contract: -// OpenReadOnly must not mutate the DB. We create a DB without the -// schema_migrations row for migration 2 present (simulating a -// daemon-not-yet-run state), open it read-only, and confirm no row -// was added and no column dropped. +// OpenReadOnly must not mutate the DB. Seed a DB whose baseline +// migration row has been forcibly removed (simulating a "behind" +// state), open it read-only, and confirm nothing was re-applied. func TestOpenReadOnlyDoesNotRunMigrations(t *testing.T) { path := filepath.Join(t.TempDir(), "state.db") - // Seed the file by running full Open once, then roll migration 2 - // backwards manually so the DB is "behind" current code. full, err := Open(path) if err != nil { t.Fatalf("Open: %v", err) } - if _, err := full.db.Exec("ALTER TABLE images ADD COLUMN packages_path TEXT"); err != nil { - t.Fatalf("re-add packages_path: %v", err) - } - if _, err := full.db.Exec("DELETE FROM schema_migrations WHERE id = 2"); err != nil { - t.Fatalf("remove migration 2 marker: %v", err) + if _, err := full.db.Exec("DELETE FROM schema_migrations WHERE id = 1"); err != nil { + t.Fatalf("remove baseline marker: %v", err) } _ = full.Close() @@ -242,39 +162,12 @@ func TestOpenReadOnlyDoesNotRunMigrations(t *testing.T) { } defer ro.Close() - // Migration 2 marker must still be absent; packages_path must - // still exist. var migCount int - if err := ro.db.QueryRow("SELECT COUNT(*) FROM schema_migrations WHERE id = 2").Scan(&migCount); err != nil { + if err := ro.db.QueryRow("SELECT COUNT(*) FROM schema_migrations WHERE id = 1").Scan(&migCount); err != nil { t.Fatalf("query schema_migrations: %v", err) } if migCount != 0 { - t.Fatal("OpenReadOnly recorded migration 2 — the open path mutated the DB") - } - rows, err := ro.db.Query("PRAGMA table_info(images)") - if err != nil { - t.Fatalf("PRAGMA table_info: %v", err) - } - defer rows.Close() - var sawColumn bool - for rows.Next() { - var ( - cid int - name string - valueType string - notNull int - defaultV sql.NullString - pk int - ) - if err := rows.Scan(&cid, &name, &valueType, ¬Null, &defaultV, &pk); err != nil { - t.Fatalf("scan: %v", err) - } - if name == "packages_path" { - sawColumn = true - } - } - if !sawColumn { - t.Fatal("packages_path disappeared — OpenReadOnly ran the drop migration") + t.Fatal("OpenReadOnly re-recorded a migration row — the open path mutated the DB") } } @@ -351,72 +244,6 @@ func TestRunMigrationsIgnoresUnknownAppliedIDs(t *testing.T) { } } -// TestDropColumnIfExistsIsIdempotent pins the "run twice, no harm" -// property. A daemon that restarts after a successful migration 2 -// on a fresh install shouldn't fail because the column is already -// gone. migrateDropDeadImageColumns calls dropColumnIfExists, which -// must silently succeed when the column is absent. -func TestDropColumnIfExistsIsIdempotent(t *testing.T) { - db := openRawDB(t) - // Set up a tiny table with a known column we're going to drop. - if _, err := db.Exec(`CREATE TABLE throwaway (keeper TEXT, victim TEXT)`); err != nil { - t.Fatalf("CREATE: %v", err) - } - - run := func(label string) error { - tx, err := db.Begin() - if err != nil { - t.Fatalf("%s Begin: %v", label, err) - } - if err := dropColumnIfExists(tx, "throwaway", "victim"); err != nil { - _ = tx.Rollback() - return err - } - return tx.Commit() - } - - if err := run("first"); err != nil { - t.Fatalf("first dropColumnIfExists: %v", err) - } - // Second call against a table that no longer has the column. - if err := run("second"); err != nil { - t.Fatalf("second dropColumnIfExists (column already gone): %v", err) - } - - // The keeper column must still be there; victim is gone. - rows, err := db.Query("PRAGMA table_info(throwaway)") - if err != nil { - t.Fatalf("PRAGMA: %v", err) - } - defer rows.Close() - var haveKeeper, haveVictim bool - for rows.Next() { - var ( - cid int - name string - valueType string - notNull int - defaultV sql.NullString - pk int - ) - if err := rows.Scan(&cid, &name, &valueType, ¬Null, &defaultV, &pk); err != nil { - t.Fatalf("scan: %v", err) - } - switch name { - case "keeper": - haveKeeper = true - case "victim": - haveVictim = true - } - } - if !haveKeeper { - t.Fatal("keeper column disappeared — dropColumnIfExists is too aggressive") - } - if haveVictim { - t.Fatal("victim column survived — dropColumnIfExists didn't actually drop") - } -} - func TestRunMigrationsRejectsDuplicateID(t *testing.T) { db := openRawDB(t) orig := migrations