diff --git a/.github/workflows/codecov.yml b/.github/workflows/codecov.yml index e484934..7be8d0d 100644 --- a/.github/workflows/codecov.yml +++ b/.github/workflows/codecov.yml @@ -16,7 +16,7 @@ jobs: - name: Setup Go uses: actions/setup-go@v6 with: - go-version: '1.25' + go-version-file: 'go.mod' - name: Run tests with coverage run: | diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 5ccfa4d..73f72f2 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -47,7 +47,7 @@ jobs: - name: Set up Go uses: actions/setup-go@v6 with: - go-version: "1.25" + go-version-file: 'go.mod' - name: Show Go version run: go version diff --git a/.github/workflows/security-ultimate.yml b/.github/workflows/security-ultimate.yml index 8f33247..8fdf8fe 100644 --- a/.github/workflows/security-ultimate.yml +++ b/.github/workflows/security-ultimate.yml @@ -26,10 +26,10 @@ jobs: ######################################## # GO 1.25 — MAIN TOOLCHAIN ######################################## - - name: Set up Go 1.25 + - name: Set up Go (from go.mod) uses: actions/setup-go@v6 with: - go-version: "1.25" + go-version-file: 'go.mod' - name: Show Go version run: go version diff --git a/cmd/proxsave/main.go b/cmd/proxsave/main.go index 55ebca6..3c9ade0 100644 --- a/cmd/proxsave/main.go +++ b/cmd/proxsave/main.go @@ -36,7 +36,7 @@ import ( const ( defaultLegacyEnvPath = "/opt/proxsave/env/backup.env" legacyEnvFallbackPath = "/opt/proxmox-backup/env/backup.env" - goRuntimeMinVersion = "1.25.4" + goRuntimeMinVersion = "1.25.5" networkPreflightTimeout = 2 * time.Second bytesPerMegabyte int64 = 1024 * 1024 defaultDirPerm = 0o755 diff --git a/cmd/proxsave/runtime_helpers.go b/cmd/proxsave/runtime_helpers.go index 498b1d1..4ea42be 100644 --- a/cmd/proxsave/runtime_helpers.go +++ b/cmd/proxsave/runtime_helpers.go @@ -700,29 +700,58 @@ func migrateLegacyCronEntries(ctx context.Context, baseDir, execPath string, boo updatedLines := make([]string, 0, len(lines)+1) - containsAny := func(line string) bool { + correctPaths := []string{strings.TrimSpace(newCommandToken)} + if execPath != "" && execPath != newCommandToken { + correctPaths = append(correctPaths, execPath) + } + + containsLegacy := func(line string) bool { if strings.Contains(line, "proxmox-backup.sh") { return true } - if strings.Contains(line, "proxmox-backup") || strings.Contains(line, "proxsave") { - return true - } for _, p := range legacyPaths { - if strings.Contains(line, p) { + if p != "" && strings.Contains(line, p) { + return true + } + } + return false + } + + containsCorrectPath := func(line string) bool { + for _, p := range correctPaths { + if p != "" && strings.Contains(line, p) { return true } } return false } + containsBinaryReference := func(line string) bool { + if strings.Contains(line, "proxsave") || strings.Contains(line, "proxmox-backup") { + return true + } + return false + } + + hasCurrentEntry := false + for _, line := range lines { trimmed := strings.TrimSpace(line) if trimmed == "" || strings.HasPrefix(trimmed, "#") { updatedLines = append(updatedLines, line) continue } - if containsAny(line) { - // Skip all existing proxmox-backup entries (bash or go) to recreate cleanly. + if containsLegacy(line) { + // Remove cron entries that still reference the legacy Bash script paths. + continue + } + if containsCorrectPath(line) { + hasCurrentEntry = true + updatedLines = append(updatedLines, line) + continue + } + if containsBinaryReference(line) { + // Remove proxsave/proxmox-backup entries that point to outdated binaries. continue } updatedLines = append(updatedLines, line) @@ -732,9 +761,11 @@ func migrateLegacyCronEntries(ctx context.Context, baseDir, execPath string, boo if schedule == "" { schedule = "0 2 * * *" } - // Always append a fresh default entry pointing to the Go binary (or fallback). - defaultLine := fmt.Sprintf("%s %s", schedule, newCommandToken) - updatedLines = append(updatedLines, defaultLine) + // Append a fresh default entry pointing to the Go binary (or fallback) if one doesn't exist already. + if !hasCurrentEntry { + defaultLine := fmt.Sprintf("%s %s", schedule, newCommandToken) + updatedLines = append(updatedLines, defaultLine) + } newCron := strings.Join(updatedLines, "\n") + "\n" if err := writeCron(newCron); err != nil { @@ -742,7 +773,11 @@ func migrateLegacyCronEntries(ctx context.Context, baseDir, execPath string, boo return } - bootstrap.Debug("Recreated cron entry for proxsave at 02:00: %s", newCommandToken) + if hasCurrentEntry { + bootstrap.Debug("Existing cron entry already targets %s; no changes made.", newCommandToken) + } else { + bootstrap.Debug("Recreated cron entry for proxsave at schedule %s: %s", schedule, newCommandToken) + } } func logBootstrapWarning(bootstrap *logging.BootstrapLogger, format string, args ...interface{}) { diff --git a/docs/CLI_REFERENCE.md b/docs/CLI_REFERENCE.md index 9221c8c..8bb4f87 100644 --- a/docs/CLI_REFERENCE.md +++ b/docs/CLI_REFERENCE.md @@ -182,7 +182,7 @@ Some interactive commands support two interface modes: 7. Atomically replaces current binary (write to .tmp, then rename) 8. Updates symlinks in `/usr/local/bin/` (proxsave, proxmox-backup) 9. Cleans up legacy Bash script symlinks -10. Migrates cron entries and fixes file permissions +10. Migrates cron entries (legacy entries are replaced, existing ones using the Go binary are preserved) and fixes file permissions **Post-upgrade steps**: 1. Configuration file automatically compatible with new version @@ -196,7 +196,7 @@ Some interactive commands support two interface modes: - **No configuration changes**: `backup.env` is never modified during `--upgrade` - **Platform support**: Linux only (amd64, arm64) - **Incompatible flags**: Cannot use with `--install` or `--new-install` -- **Automatic maintenance**: Symlinks, cron, and permissions updated automatically +- **Automatic maintenance**: Symlinks, cron (without touching entries already pointing to proxsave/proxmox-backup), and permissions updated automatically - **Safe replacement**: Old binary is replaced atomically (no backup created) - **Separate config upgrade**: Use `--upgrade-config` separately to update configuration diff --git a/docs/INSTALL.md b/docs/INSTALL.md index 3b31a1a..1d76095 100644 --- a/docs/INSTALL.md +++ b/docs/INSTALL.md @@ -93,7 +93,7 @@ The `--upgrade` command: - ✅ Atomically replaces current binary - ✅ Updates symlinks (`/usr/local/bin/proxsave`, `/usr/local/bin/proxmox-backup`) - ✅ Cleans up legacy Bash script symlinks -- ✅ Migrates cron entries to new binary +- ✅ Migrates cron entries to new binary while preserving entries that already point to the Go executable - ✅ Fixes file permissions - ❌ **Does NOT modify** your `backup.env` configuration @@ -416,4 +416,4 @@ Run first backup | **Code size** | 20,370 lines | ~3,000 lines Go code | | **Memory usage** | Higher (multiple processes) | Lower (single binary) | | **Maintenance** | Archived, critical fixes only | Active development | -| **Compatibility** | Can coexist with Go version | Can coexist with Bash version | \ No newline at end of file +| **Compatibility** | Can coexist with Go version | Can coexist with Bash version | diff --git a/embed.go b/embed.go index f18cb15..b493a4c 100644 --- a/embed.go +++ b/embed.go @@ -68,6 +68,3 @@ func collectEmbeddedDocs() ([]DocAsset, error) { }) return assets, nil } - -// Dummy symbol so that Go Report Card recognizes this package as valid. -const _goreportcardFix = true diff --git a/go.mod b/go.mod index 9600a6b..4a31a28 100644 --- a/go.mod +++ b/go.mod @@ -2,7 +2,7 @@ module github.com/tis24dev/proxsave go 1.25 -toolchain go1.25.4 +toolchain go1.25.5 require ( filippo.io/age v1.2.1 diff --git a/internal/orchestrator/.backup.lock b/internal/orchestrator/.backup.lock index 90ea944..2241ce2 100644 --- a/internal/orchestrator/.backup.lock +++ b/internal/orchestrator/.backup.lock @@ -1,3 +1,3 @@ -pid=230863 +pid=324942 host=pbs1 -time=2025-12-02T16:24:15+01:00 +time=2025-12-02T20:27:27+01:00 diff --git a/internal/orchestrator/additional_helpers_test.go b/internal/orchestrator/additional_helpers_test.go index 9f8feb3..9395268 100644 --- a/internal/orchestrator/additional_helpers_test.go +++ b/internal/orchestrator/additional_helpers_test.go @@ -5,6 +5,7 @@ import ( "bufio" "bytes" "context" + "encoding/json" "errors" "fmt" "io" @@ -791,6 +792,141 @@ func TestEnsureTempRegistrySuccess(t *testing.T) { } } +func TestCleanupPreviousExecutionArtifacts(t *testing.T) { + logger := logging.New(types.LogLevelDebug, false) + o := &Orchestrator{ + logger: logger, + } + + logDir := t.TempDir() + o.logPath = logDir + + statsFile := filepath.Join(logDir, "backup-stats-old.json") + cpuProfile := filepath.Join(logDir, "cpu-test.pprof") + if err := os.WriteFile(statsFile, []byte("stats"), 0o640); err != nil { + t.Fatalf("write stats file: %v", err) + } + if err := os.WriteFile(cpuProfile, []byte("cpu"), 0o640); err != nil { + t.Fatalf("write cpu profile: %v", err) + } + + heapDir := filepath.Join("/tmp", "proxsave") + if err := os.MkdirAll(heapDir, 0o755); err != nil { + t.Fatalf("prepare heap dir: %v", err) + } + heapFile := filepath.Join(heapDir, fmt.Sprintf("heap-%d.pprof", time.Now().UnixNano())) + if err := os.WriteFile(heapFile, []byte("heap"), 0o640); err != nil { + t.Fatalf("write heap profile: %v", err) + } + t.Cleanup(func() { + if err := os.Remove(heapFile); err != nil && !os.IsNotExist(err) { + t.Fatalf("cleanup heap file: %v", err) + } + }) + + registryDir := t.TempDir() + registryPath := filepath.Join(registryDir, "registry.json") + reg, err := NewTempDirRegistry(logger, registryPath) + if err != nil { + t.Fatalf("NewTempDirRegistry: %v", err) + } + o.tempRegistry = reg + + orphanDir := filepath.Join(registryDir, "orphan-temp") + if err := os.MkdirAll(orphanDir, 0o755); err != nil { + t.Fatalf("create orphan dir: %v", err) + } + if err := reg.Register(orphanDir); err != nil { + t.Fatalf("register orphan dir: %v", err) + } + makeRegistryEntriesStale(t, registryPath) + + returned := o.cleanupPreviousExecutionArtifacts() + if returned != reg { + t.Fatalf("cleanup should return configured registry instance") + } + + if _, err := os.Stat(statsFile); !os.IsNotExist(err) { + t.Fatalf("stats file should be removed, got err=%v", err) + } + if _, err := os.Stat(cpuProfile); !os.IsNotExist(err) { + t.Fatalf("cpu profile should be removed, got err=%v", err) + } + if _, err := os.Stat(heapFile); !os.IsNotExist(err) { + t.Fatalf("heap profile should be removed, got err=%v", err) + } + if _, err := os.Stat(orphanDir); !os.IsNotExist(err) { + t.Fatalf("orphan directory should be removed, got err=%v", err) + } +} + +func makeRegistryEntriesStale(t *testing.T, registryPath string) { + t.Helper() + data, err := os.ReadFile(registryPath) + if err != nil { + t.Fatalf("read registry: %v", err) + } + var entries []tempDirRecord + if err := json.Unmarshal(data, &entries); err != nil { + t.Fatalf("parse registry: %v", err) + } + staleTime := time.Now().Add(-2 * tempDirCleanupAge) + for i := range entries { + entries[i].CreatedAt = staleTime + } + content, err := json.Marshal(entries) + if err != nil { + t.Fatalf("marshal registry: %v", err) + } + if err := os.WriteFile(registryPath, content, 0o640); err != nil { + t.Fatalf("write registry: %v", err) + } +} + +func TestCopyFileUsesProvidedFS(t *testing.T) { + fs := NewFakeFS() + src := "src/config.txt" + dest := "dest/clone.txt" + if err := fs.AddFile(src, []byte("payload")); err != nil { + t.Fatalf("add src: %v", err) + } + if err := fs.AddDir("dest"); err != nil { + t.Fatalf("mkdir dest: %v", err) + } + + if err := copyFile(fs, src, dest); err != nil { + t.Fatalf("copyFile returned error: %v", err) + } + + data, err := fs.ReadFile(dest) + if err != nil { + t.Fatalf("read dest: %v", err) + } + if string(data) != "payload" { + t.Fatalf("copied content mismatch: %q", data) + } +} + +func TestCopyFileFallsBackToOSFS(t *testing.T) { + tempDir := t.TempDir() + src := filepath.Join(tempDir, "src.txt") + dest := filepath.Join(tempDir, "dest.txt") + if err := os.WriteFile(src, []byte("data"), 0o640); err != nil { + t.Fatalf("write src: %v", err) + } + + if err := copyFile(nil, src, dest); err != nil { + t.Fatalf("copyFile fallback error: %v", err) + } + data, err := os.ReadFile(dest) + if err != nil { + t.Fatalf("read dest: %v", err) + } + if string(data) != "data" { + t.Fatalf("dest content mismatch: %q", data) + } +} + func TestShowRestoreModeMenu(t *testing.T) { logger := logging.New(types.LogLevelError, false) old := os.Stdin diff --git a/internal/orchestrator/decrypt_tui.go b/internal/orchestrator/decrypt_tui.go index 8ef8f61..21015b0 100644 --- a/internal/orchestrator/decrypt_tui.go +++ b/internal/orchestrator/decrypt_tui.go @@ -34,6 +34,11 @@ const ( pathActionCancel = "cancel" ) +var ( + promptOverwriteActionFunc = promptOverwriteAction + promptNewPathInputFunc = promptNewPathInput +) + // RunDecryptWorkflowTUI runs the decrypt workflow using a TUI flow. func RunDecryptWorkflowTUI(ctx context.Context, cfg *config.Config, logger *logging.Logger, version, configPath, buildSig string) error { if cfg == nil { @@ -513,7 +518,7 @@ func ensureWritablePathTUI(path, description, configPath, buildSig string) (stri return "", fmt.Errorf("stat %s: %w", current, err) } - action, err := promptOverwriteAction(current, description, failureMessage, configPath, buildSig) + action, err := promptOverwriteActionFunc(current, description, failureMessage, configPath, buildSig) if err != nil { return "", err } @@ -527,7 +532,7 @@ func ensureWritablePathTUI(path, description, configPath, buildSig string) (stri } return current, nil case pathActionNew: - newPath, err := promptNewPathInput(current, configPath, buildSig) + newPath, err := promptNewPathInputFunc(current, configPath, buildSig) if err != nil { if errors.Is(err, ErrDecryptAborted) { return "", ErrDecryptAborted diff --git a/internal/orchestrator/decrypt_tui_test.go b/internal/orchestrator/decrypt_tui_test.go index 9faccb9..6dc8068 100644 --- a/internal/orchestrator/decrypt_tui_test.go +++ b/internal/orchestrator/decrypt_tui_test.go @@ -1,11 +1,18 @@ package orchestrator import ( + "context" + "errors" + "os" "path/filepath" "testing" "time" + "github.com/rivo/tview" + "github.com/tis24dev/proxsave/internal/backup" + "github.com/tis24dev/proxsave/internal/logging" + "github.com/tis24dev/proxsave/internal/types" ) func TestNormalizeProxmoxVersion(t *testing.T) { @@ -76,3 +83,206 @@ func TestEnsureWritablePathTUI_ReturnsCleanMissingPath(t *testing.T) { t.Fatalf("ensureWritablePathTUI path=%q, want %q", path, target) } } + +func TestEnsureWritablePathTUIOverwriteExisting(t *testing.T) { + tmp := t.TempDir() + target := filepath.Join(tmp, "existing.tar") + if err := os.WriteFile(target, []byte("payload"), 0o640); err != nil { + t.Fatalf("write existing file: %v", err) + } + + restore := stubPromptOverwriteAction(func(path, desc, failure, configPath, buildSig string) (string, error) { + if failure != "" { + t.Fatalf("unexpected failure message: %s", failure) + } + return pathActionOverwrite, nil + }) + defer restore() + + got, err := ensureWritablePathTUI(target, "archive", "cfg", "sig") + if err != nil { + t.Fatalf("ensureWritablePathTUI error: %v", err) + } + if got != target { + t.Fatalf("path = %q, want %q", got, target) + } + if _, err := os.Stat(target); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("existing file should be removed, stat err=%v", err) + } +} + +func TestEnsureWritablePathTUINewPath(t *testing.T) { + tmp := t.TempDir() + existing := filepath.Join(tmp, "current.tar") + if err := os.WriteFile(existing, []byte("payload"), 0o640); err != nil { + t.Fatalf("write existing file: %v", err) + } + nextPath := filepath.Join(tmp, "new.tar") + + var promptCalls int + restorePrompt := stubPromptOverwriteAction(func(path, desc, failure, configPath, buildSig string) (string, error) { + promptCalls++ + if failure != "" { + t.Fatalf("unexpected failure message: %s", failure) + } + return pathActionNew, nil + }) + defer restorePrompt() + + restoreNew := stubPromptNewPath(func(current, configPath, buildSig string) (string, error) { + if filepath.Clean(current) != filepath.Clean(existing) { + t.Fatalf("promptNewPath received %q, want %q", current, existing) + } + return nextPath, nil + }) + defer restoreNew() + + got, err := ensureWritablePathTUI(existing, "bundle", "cfg", "sig") + if err != nil { + t.Fatalf("ensureWritablePathTUI error: %v", err) + } + if got != filepath.Clean(nextPath) { + t.Fatalf("path=%q, want %q", got, nextPath) + } + if promptCalls != 1 { + t.Fatalf("expected 1 prompt call, got %d", promptCalls) + } +} + +func TestEnsureWritablePathTUIAbortOnCancel(t *testing.T) { + path := mustCreateExistingFile(t) + restore := stubPromptOverwriteAction(func(path, desc, failure, configPath, buildSig string) (string, error) { + return pathActionCancel, nil + }) + defer restore() + + if _, err := ensureWritablePathTUI(path, "bundle", "cfg", "sig"); !errors.Is(err, ErrDecryptAborted) { + t.Fatalf("expected ErrDecryptAborted, got %v", err) + } +} + +func TestEnsureWritablePathTUIPropagatesPromptErrors(t *testing.T) { + path := mustCreateExistingFile(t) + wantErr := errors.New("boom") + restore := stubPromptOverwriteAction(func(path, desc, failure, configPath, buildSig string) (string, error) { + return "", wantErr + }) + defer restore() + + if _, err := ensureWritablePathTUI(path, "bundle", "cfg", "sig"); !errors.Is(err, wantErr) { + t.Fatalf("expected %v, got %v", wantErr, err) + } +} + +func TestEnsureWritablePathTUINewPathAbort(t *testing.T) { + path := mustCreateExistingFile(t) + restorePrompt := stubPromptOverwriteAction(func(path, desc, failure, configPath, buildSig string) (string, error) { + return pathActionNew, nil + }) + defer restorePrompt() + + restoreNew := stubPromptNewPath(func(current, configPath, buildSig string) (string, error) { + return "", ErrDecryptAborted + }) + defer restoreNew() + + if _, err := ensureWritablePathTUI(path, "bundle", "cfg", "sig"); !errors.Is(err, ErrDecryptAborted) { + t.Fatalf("expected ErrDecryptAborted, got %v", err) + } +} + +func TestPreparePlainBundleTUICopiesRawArtifacts(t *testing.T) { + logger := logging.New(types.LogLevelError, false) + tmp := t.TempDir() + rawArchive := filepath.Join(tmp, "backup.tar") + rawMetadata := rawArchive + ".metadata" + rawChecksum := rawArchive + ".sha256" + + if err := os.WriteFile(rawArchive, []byte("payload-data"), 0o640); err != nil { + t.Fatalf("write archive: %v", err) + } + if err := os.WriteFile(rawMetadata, []byte(`{"manifest":true}`), 0o640); err != nil { + t.Fatalf("write metadata: %v", err) + } + if err := os.WriteFile(rawChecksum, []byte("checksum backup.tar\n"), 0o640); err != nil { + t.Fatalf("write checksum: %v", err) + } + + cand := &decryptCandidate{ + Manifest: &backup.Manifest{ + ArchivePath: rawArchive, + EncryptionMode: "none", + CreatedAt: time.Now(), + Hostname: "node1", + }, + Source: sourceRaw, + RawArchivePath: rawArchive, + RawMetadataPath: rawMetadata, + RawChecksumPath: rawChecksum, + DisplayBase: "test-backup", + } + + ctx := context.Background() + prepared, err := preparePlainBundleTUI(ctx, cand, "1.0.0", logger, "cfg", "sig") + if err != nil { + t.Fatalf("preparePlainBundleTUI error: %v", err) + } + defer prepared.Cleanup() + + if prepared.ArchivePath == "" { + t.Fatalf("expected archive path to be set") + } + if prepared.Manifest.EncryptionMode != "none" { + t.Fatalf("expected manifest encryption mode none, got %s", prepared.Manifest.EncryptionMode) + } + if prepared.Manifest.ScriptVersion != "1.0.0" { + t.Fatalf("expected script version to propagate, got %s", prepared.Manifest.ScriptVersion) + } + if _, err := os.Stat(prepared.ArchivePath); err != nil { + t.Fatalf("expected staged archive to exist: %v", err) + } + if prepared.Checksum == "" { + t.Fatalf("expected checksum to be computed") + } +} + +func TestPreparePlainBundleTUIRejectsInvalidCandidate(t *testing.T) { + logger := logging.New(types.LogLevelError, false) + ctx := context.Background() + if _, err := preparePlainBundleTUI(ctx, nil, "", logger, "cfg", "sig"); err == nil { + t.Fatalf("expected error for nil candidate") + } +} + +func TestBuildWizardPageReturnsFlex(t *testing.T) { + content := tview.NewBox() + page := buildWizardPage("Title", "/etc/proxsave/backup.env", "sig", content) + if page == nil { + t.Fatalf("expected non-nil page") + } + if _, ok := page.(*tview.Flex); !ok { + t.Fatalf("expected *tview.Flex, got %T", page) + } +} + +func stubPromptOverwriteAction(fn func(path, description, failureMessage, configPath, buildSig string) (string, error)) func() { + orig := promptOverwriteActionFunc + promptOverwriteActionFunc = fn + return func() { promptOverwriteActionFunc = orig } +} + +func stubPromptNewPath(fn func(current, configPath, buildSig string) (string, error)) func() { + orig := promptNewPathInputFunc + promptNewPathInputFunc = fn + return func() { promptNewPathInputFunc = orig } +} + +func mustCreateExistingFile(t *testing.T) string { + t.Helper() + tmp := t.TempDir() + path := filepath.Join(tmp, "existing.dat") + if err := os.WriteFile(path, []byte("data"), 0o640); err != nil { + t.Fatalf("write %s: %v", path, err) + } + return path +} diff --git a/internal/orchestrator/directory_recreation.go b/internal/orchestrator/directory_recreation.go index 43c3fdd..06b7460 100644 --- a/internal/orchestrator/directory_recreation.go +++ b/internal/orchestrator/directory_recreation.go @@ -10,10 +10,14 @@ import ( "github.com/tis24dev/proxsave/internal/logging" ) +var ( + storageCfgPath = "/etc/pve/storage.cfg" + datastoreCfgPath = "/etc/proxmox-backup/datastore.cfg" + zpoolCachePath = "/etc/zfs/zpool.cache" +) + // RecreateStorageDirectories parses storage.cfg and recreates storage directories (PVE) func RecreateStorageDirectories(logger *logging.Logger) error { - storageCfgPath := "/etc/pve/storage.cfg" - // Check if file exists if _, err := os.Stat(storageCfgPath); err != nil { if os.IsNotExist(err) { @@ -50,8 +54,8 @@ func RecreateStorageDirectories(logger *logging.Logger) error { if strings.Contains(line, ":") && !strings.Contains(line, "=") { parts := strings.Fields(line) if len(parts) >= 2 { - currentType = parts[0] - currentStorage = parts[1] + currentType = strings.TrimSuffix(parts[0], ":") + currentStorage = strings.TrimSuffix(parts[1], ":") currentPath = "" } continue @@ -134,8 +138,6 @@ func createPVEStorageStructure(basePath, storageType string, logger *logging.Log // RecreateDatastoreDirectories parses datastore.cfg and recreates datastore directories (PBS) func RecreateDatastoreDirectories(logger *logging.Logger) error { - datastoreCfgPath := "/etc/proxmox-backup/datastore.cfg" - // Check if file exists if _, err := os.Stat(datastoreCfgPath); err != nil { if os.IsNotExist(err) { @@ -171,7 +173,7 @@ func RecreateDatastoreDirectories(logger *logging.Logger) error { if strings.HasPrefix(line, "datastore:") { parts := strings.Fields(line) if len(parts) >= 2 { - currentDatastore = parts[1] + currentDatastore = strings.TrimSuffix(parts[1], ":") currentPath = "" } continue @@ -252,7 +254,7 @@ func createPBSDatastoreStructure(basePath, datastoreName string, logger *logging // isLikelyZFSMountPoint checks if a path is likely a ZFS mount point func isLikelyZFSMountPoint(path string, logger *logging.Logger) bool { // Check if /etc/zfs/zpool.cache exists (indicates ZFS is used on this system) - if _, err := os.Stat("/etc/zfs/zpool.cache"); err != nil { + if _, err := os.Stat(zpoolCachePath); err != nil { // No ZFS on this system return false } @@ -261,8 +263,8 @@ func isLikelyZFSMountPoint(path string, logger *logging.Logger) bool { // PBS datastores on ZFS are typically under /mnt/ or use "backup" in the name pathLower := strings.ToLower(path) if strings.HasPrefix(pathLower, "/mnt/") || - strings.Contains(pathLower, "backup") || - strings.Contains(pathLower, "datastore") { + strings.Contains(pathLower, "backup") || + strings.Contains(pathLower, "datastore") { logger.Debug("Path %s matches ZFS mount point pattern", path) return true } diff --git a/internal/orchestrator/directory_recreation_test.go b/internal/orchestrator/directory_recreation_test.go new file mode 100644 index 0000000..d5b53e5 --- /dev/null +++ b/internal/orchestrator/directory_recreation_test.go @@ -0,0 +1,191 @@ +package orchestrator + +import ( + "fmt" + "io" + "os" + "path/filepath" + "testing" + + "github.com/tis24dev/proxsave/internal/logging" + "github.com/tis24dev/proxsave/internal/types" +) + +func newDirTestLogger() *logging.Logger { + logger := logging.New(types.LogLevelDebug, false) + logger.SetOutput(io.Discard) + return logger +} + +func overridePath(t *testing.T, target *string, filename string) (string, func()) { + t.Helper() + dir := t.TempDir() + path := filepath.Join(dir, filename) + prev := *target + *target = path + return path, func() { + *target = prev + } +} + +func writeFile(t *testing.T, path, content string) { + t.Helper() + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatalf("write %s: %v", path, err) + } +} + +func TestRecreateStorageDirectoriesCreatesStructure(t *testing.T) { + logger := newDirTestLogger() + baseDir := filepath.Join(t.TempDir(), "local") + cfg := fmt.Sprintf("dir: local\n path %s\n", baseDir) + cfgPath, restore := overridePath(t, &storageCfgPath, "storage.cfg") + defer restore() + writeFile(t, cfgPath, cfg) + + if err := RecreateStorageDirectories(logger); err != nil { + t.Fatalf("RecreateStorageDirectories error: %v", err) + } + + for _, sub := range []string{"dump", "images", "template", "snippets", "private"} { + if _, err := os.Stat(filepath.Join(baseDir, sub)); err != nil { + t.Fatalf("expected subdir %s to exist: %v", sub, err) + } + } +} + +func TestCreatePVEStorageStructureHandlesVariousTypes(t *testing.T) { + logger := newDirTestLogger() + baseNFS := filepath.Join(t.TempDir(), "nfs") + if err := createPVEStorageStructure(baseNFS, "nfs", logger); err != nil { + t.Fatalf("createPVEStorageStructure(nfs): %v", err) + } + for _, sub := range []string{"dump", "images", "template"} { + if _, err := os.Stat(filepath.Join(baseNFS, sub)); err != nil { + t.Fatalf("expected nfs subdir %s: %v", sub, err) + } + } + + baseOther := filepath.Join(t.TempDir(), "rbd") + if err := createPVEStorageStructure(baseOther, "rbd", logger); err != nil { + t.Fatalf("createPVEStorageStructure(rbd): %v", err) + } + if _, err := os.Stat(baseOther); err != nil { + t.Fatalf("expected base directory for other type: %v", err) + } +} + +func TestRecreateDatastoreDirectoriesCreatesStructure(t *testing.T) { + logger := newDirTestLogger() + baseDir := filepath.Join(t.TempDir(), "datastore1") + cfg := fmt.Sprintf("datastore: backup\n path %s\n", baseDir) + cfgPath, restore := overridePath(t, &datastoreCfgPath, "datastore.cfg") + defer restore() + writeFile(t, cfgPath, cfg) + + cachePath, cacheRestore := overridePath(t, &zpoolCachePath, "zpool.cache") + defer cacheRestore() + // Ensure cache path does not exist to simulate non-ZFS environment + if err := os.RemoveAll(cachePath); err != nil && !os.IsNotExist(err) { + t.Fatalf("cleanup cache path: %v", err) + } + + if err := RecreateDatastoreDirectories(logger); err != nil { + t.Fatalf("RecreateDatastoreDirectories error: %v", err) + } + + for _, sub := range []string{".chunks", ".lock"} { + if _, err := os.Stat(filepath.Join(baseDir, sub)); err != nil { + t.Fatalf("expected datastore subdir %s: %v", sub, err) + } + } +} + +func TestRecreateDatastoreDirectoriesSkipsZFSMountPoints(t *testing.T) { + logger := newDirTestLogger() + baseDir := filepath.Join(t.TempDir(), "backup-ds") + cfg := fmt.Sprintf("datastore: ds\n path %s\n", baseDir) + cfgPath, restore := overridePath(t, &datastoreCfgPath, "datastore.cfg") + defer restore() + writeFile(t, cfgPath, cfg) + + cachePath, cacheRestore := overridePath(t, &zpoolCachePath, "zpool.cache") + defer cacheRestore() + writeFile(t, cachePath, "cache") + + if err := RecreateDatastoreDirectories(logger); err != nil { + t.Fatalf("RecreateDatastoreDirectories zfs scenario: %v", err) + } + if _, err := os.Stat(filepath.Join(baseDir, ".chunks")); !os.IsNotExist(err) { + t.Fatalf("expected ZFS path to be skipped, got err=%v", err) + } +} + +func TestIsLikelyZFSMountPointRequiresCache(t *testing.T) { + logger := newDirTestLogger() + cachePath, restore := overridePath(t, &zpoolCachePath, "zpool.cache") + defer restore() + + // Without cache file the function should return false even for matching paths. + if isLikelyZFSMountPoint("/mnt/pbs", logger) { + t.Fatalf("expected false when cache file is missing") + } + + writeFile(t, cachePath, "cache") + if !isLikelyZFSMountPoint("/mnt/pbs", logger) { + t.Fatalf("expected true when cache exists and path matches patterns") + } +} + +func TestSetDatastoreOwnershipNoop(t *testing.T) { + logger := newDirTestLogger() + if err := setDatastoreOwnership(t.TempDir(), logger); err != nil { + t.Fatalf("setDatastoreOwnership returned error: %v", err) + } +} + +func TestRecreateDirectoriesFromConfigRoutes(t *testing.T) { + logger := newTestLogger() + + t.Run("PVE", func(t *testing.T) { + baseDir := filepath.Join(t.TempDir(), "local") + cfg := fmt.Sprintf("dir: local\n path %s\n", baseDir) + cfgPath, restore := overridePath(t, &storageCfgPath, "storage.cfg") + t.Cleanup(restore) + writeFile(t, cfgPath, cfg) + + if err := RecreateDirectoriesFromConfig(SystemTypePVE, logger); err != nil { + t.Fatalf("RecreateDirectoriesFromConfig PVE: %v", err) + } + if _, err := os.Stat(filepath.Join(baseDir, "images")); err != nil { + t.Fatalf("expected PVE directories to be created: %v", err) + } + }) + + t.Run("PBS", func(t *testing.T) { + baseDir := filepath.Join(t.TempDir(), "data") + cfg := fmt.Sprintf("datastore: main\n path %s\n", baseDir) + cfgPath, restore := overridePath(t, &datastoreCfgPath, "datastore.cfg") + t.Cleanup(restore) + writeFile(t, cfgPath, cfg) + + cachePath, cacheRestore := overridePath(t, &zpoolCachePath, "zpool.cache") + t.Cleanup(cacheRestore) + if err := os.RemoveAll(cachePath); err != nil && !os.IsNotExist(err) { + t.Fatalf("cleanup cache path: %v", err) + } + + if err := RecreateDirectoriesFromConfig(SystemTypePBS, logger); err != nil { + t.Fatalf("RecreateDirectoriesFromConfig PBS: %v", err) + } + if _, err := os.Stat(filepath.Join(baseDir, ".chunks")); err != nil { + t.Fatalf("expected PBS directories to be created: %v", err) + } + }) + + t.Run("Unknown", func(t *testing.T) { + if err := RecreateDirectoriesFromConfig(SystemTypeUnknown, logger); err != nil { + t.Fatalf("RecreateDirectoriesFromConfig unknown: %v", err) + } + }) +} diff --git a/internal/orchestrator/orchestrator_test.go b/internal/orchestrator/orchestrator_test.go index 71b4fc1..002fea0 100644 --- a/internal/orchestrator/orchestrator_test.go +++ b/internal/orchestrator/orchestrator_test.go @@ -1,9 +1,11 @@ package orchestrator import ( + "bytes" "context" "os" "path/filepath" + "strings" "testing" "time" @@ -127,7 +129,7 @@ func TestOrchestrator_SetIdentity(t *testing.T) { // TestOrchestrator_SetBackupConfig tests SetBackupConfig func TestOrchestrator_SetBackupConfig(t *testing.T) { logger := logging.New(types.LogLevelInfo, false) - + orch := New(logger, false) backupPath := "/tmp/backups" @@ -372,13 +374,13 @@ func TestApplyCollectorOverridesCopiesConfig(t *testing.T) { CustomBackupPaths: []string{"/etc", "/var/lib"}, BackupBlacklist: []string{"/tmp"}, - ConfigPath: "/etc/proxsave/backup.env", - PVEConfigPath: "/etc/pve", - PBSConfigPath: "/etc/proxmox-backup", - PVEClusterPath: "/etc/pve/corosync.conf", + ConfigPath: "/etc/proxsave/backup.env", + PVEConfigPath: "/etc/pve", + PBSConfigPath: "/etc/proxmox-backup", + PVEClusterPath: "/etc/pve/corosync.conf", CorosyncConfigPath: "/etc/corosync/corosync.conf", - VzdumpConfigPath: "/etc/vzdump.conf", - PBSDatastorePaths: []string{"/mnt/pbs1", "/mnt/pbs2"}, + VzdumpConfigPath: "/etc/vzdump.conf", + PBSDatastorePaths: []string{"/mnt/pbs1", "/mnt/pbs2"}, PBSRepository: "pbs@pam!token", PBSPassword: "secret", @@ -439,3 +441,55 @@ func TestApplyCollectorOverridesCopiesConfig(t *testing.T) { t.Fatalf("PBS auth fields not copied correctly") } } + +func TestLogStepWritesStepMessage(t *testing.T) { + logger := logging.New(types.LogLevelInfo, false) + var buf bytes.Buffer + logger.SetOutput(&buf) + + o := &Orchestrator{logger: logger} + o.logStep(3, "Processing %s", "configs") + + out := buf.String() + if !strings.Contains(out, "STEP") { + t.Fatalf("expected STEP label in output, got: %s", out) + } + if !strings.Contains(out, "Processing configs") { + t.Fatalf("expected formatted message, got: %s", out) + } +} + +func TestLogStepHandlesNilLogger(t *testing.T) { + o := &Orchestrator{} + defer func() { + if r := recover(); r != nil { + t.Fatalf("logStep should not panic without logger: %v", r) + } + }() + o.logStep(1, "noop") +} + +func TestSaveStatsReportNilStats(t *testing.T) { + logger := logging.New(types.LogLevelInfo, false) + orch := New(logger, false) + + if err := orch.SaveStatsReport(nil); err == nil { + t.Fatalf("expected error when stats is nil") + } +} + +func TestSaveStatsReportSkipsWithoutLogPath(t *testing.T) { + logger := logging.New(types.LogLevelInfo, false) + orch := New(logger, false) + + stats := &BackupStats{ + Timestamp: time.Now(), + } + + if err := orch.SaveStatsReport(stats); err != nil { + t.Fatalf("SaveStatsReport should skip without log path: %v", err) + } + if stats.ReportPath != "" { + t.Fatalf("ReportPath should remain empty when log path is unset, got %s", stats.ReportPath) + } +} diff --git a/internal/orchestrator/restore.go b/internal/orchestrator/restore.go index b4ea6db..477dd2e 100644 --- a/internal/orchestrator/restore.go +++ b/internal/orchestrator/restore.go @@ -442,10 +442,6 @@ func unmountEtcPVE(ctx context.Context, logger *logging.Logger) error { return nil } -func runCommand(ctx context.Context, logger *logging.Logger, name string, args ...string) error { - return runCommandWithTimeout(ctx, logger, 0, name, args...) -} - func runCommandWithTimeout(ctx context.Context, logger *logging.Logger, timeout time.Duration, name string, args ...string) error { return execCommand(ctx, logger, timeout, name, args...) } diff --git a/internal/orchestrator/restore_tui.go b/internal/orchestrator/restore_tui.go index 620d990..0d0c77f 100644 --- a/internal/orchestrator/restore_tui.go +++ b/internal/orchestrator/restore_tui.go @@ -29,6 +29,7 @@ const ( ) var errRestoreBackToMode = errors.New("restore mode back") +var promptYesNoTUIFunc = promptYesNoTUI // RunRestoreWorkflowTUI runs the restore workflow using a TUI flow. func RunRestoreWorkflowTUI(ctx context.Context, cfg *config.Config, logger *logging.Logger, version, configPath, buildSig string) error { @@ -217,6 +218,13 @@ func RunRestoreWorkflowTUI(ctx context.Context, cfg *config.Config, logger *logg logger.Info("Preparing PBS system for restore: stopping proxmox-backup services") if err := stopPBSServices(ctx, logger); err != nil { logger.Warning("Unable to stop PBS services automatically: %v", err) + cont, perr := promptContinueWithPBSServicesTUI(configPath, buildSig) + if perr != nil { + return perr + } + if !cont { + return ErrRestoreAborted + } logger.Warning("Continuing restore with PBS services still running") } else { pbsServicesStopped = true @@ -832,7 +840,7 @@ func selectCategoriesTUI(available []Category, systemType SystemType, configPath func promptCompatibilityTUI(configPath, buildSig string, compatErr error) (bool, error) { message := fmt.Sprintf("Compatibility check reported:\n\n[red]%v[white]\n\nContinuing may cause system instability.\n\nDo you want to continue anyway?", compatErr) - return promptYesNoTUI( + return promptYesNoTUIFunc( "Compatibility warning", configPath, buildSig, @@ -844,7 +852,7 @@ func promptCompatibilityTUI(configPath, buildSig string, compatErr error) (bool, func promptContinueWithoutSafetyBackupTUI(configPath, buildSig string, cause error) (bool, error) { message := fmt.Sprintf("Failed to create safety backup:\n\n[red]%v[white]\n\nWithout a safety backup, it will be harder to rollback changes.\n\nContinue without safety backup?", cause) - return promptYesNoTUI( + return promptYesNoTUIFunc( "Safety backup failed", configPath, buildSig, @@ -856,7 +864,7 @@ func promptContinueWithoutSafetyBackupTUI(configPath, buildSig string, cause err func promptContinueWithPBSServicesTUI(configPath, buildSig string) (bool, error) { message := "Unable to stop Proxmox Backup Server services automatically.\n\nContinuing the restore while services are running may lead to inconsistent state.\n\nContinue restore with PBS services still running?" - return promptYesNoTUI( + return promptYesNoTUIFunc( "PBS services running", configPath, buildSig, @@ -1182,7 +1190,7 @@ func promptYesNoTUI(title, configPath, buildSig, message, yesLabel, noLabel stri func confirmOverwriteTUI(configPath, buildSig string) (bool, error) { message := "This operation will overwrite existing configuration files on this system.\n\nAre you sure you want to proceed with the restore?" - return promptYesNoTUI( + return promptYesNoTUIFunc( "Confirm overwrite", configPath, buildSig, diff --git a/internal/orchestrator/restore_tui_test.go b/internal/orchestrator/restore_tui_test.go index ee61e9d..d7f1523 100644 --- a/internal/orchestrator/restore_tui_test.go +++ b/internal/orchestrator/restore_tui_test.go @@ -1,8 +1,11 @@ package orchestrator import ( + "errors" "strings" "testing" + + "github.com/rivo/tview" ) func TestFilterAndSortCategoriesForSystem(t *testing.T) { @@ -68,3 +71,101 @@ func TestBuildRestorePlanText(t *testing.T) { t.Fatalf("missing warning text") } } + +func TestBuildRestoreWizardPageReturnsFlex(t *testing.T) { + content := tview.NewBox() + page := buildRestoreWizardPage("Title", "/etc/proxsave/backup.env", "sig", content) + if page == nil { + t.Fatalf("expected non-nil page") + } + if _, ok := page.(*tview.Flex); !ok { + t.Fatalf("expected *tview.Flex, got %T", page) + } +} + +func TestPromptCompatibilityTUIUsesWarningText(t *testing.T) { + restore := stubPromptYesNo(func(title, configPath, buildSig, message, yesLabel, noLabel string) (bool, error) { + if title != "Compatibility warning" { + t.Fatalf("unexpected title %q", title) + } + if !strings.Contains(message, "boom") { + t.Fatalf("missing error text: %s", message) + } + if yesLabel != "Continue anyway" || noLabel != "Abort restore" { + t.Fatalf("unexpected button labels %q/%q", yesLabel, noLabel) + } + return true, nil + }) + defer restore() + + ok, err := promptCompatibilityTUI("cfg", "sig", errors.New("boom")) + if err != nil || !ok { + t.Fatalf("promptCompatibilityTUI returned %v, %v", ok, err) + } +} + +func TestPromptContinueWithoutSafetyBackupTUI(t *testing.T) { + restore := stubPromptYesNo(func(title, configPath, buildSig, message, yesLabel, noLabel string) (bool, error) { + if title != "Safety backup failed" { + t.Fatalf("unexpected title %q", title) + } + if !strings.Contains(message, "failure") { + t.Fatalf("missing cause text: %s", message) + } + return false, nil + }) + defer restore() + + ok, err := promptContinueWithoutSafetyBackupTUI("cfg", "sig", errors.New("failure")) + if err != nil { + t.Fatalf("promptContinueWithoutSafetyBackupTUI error: %v", err) + } + if ok { + t.Fatalf("expected false decision") + } +} + +func TestPromptContinueWithPBSServicesTUI(t *testing.T) { + restore := stubPromptYesNo(func(title, configPath, buildSig, message, yesLabel, noLabel string) (bool, error) { + if title != "PBS services running" { + t.Fatalf("unexpected title %q", title) + } + if yesLabel != "Continue restore" || noLabel != "Abort restore" { + t.Fatalf("unexpected button labels %q/%q", yesLabel, noLabel) + } + return true, nil + }) + defer restore() + + ok, err := promptContinueWithPBSServicesTUI("cfg", "sig") + if err != nil || !ok { + t.Fatalf("promptContinueWithPBSServicesTUI returned %v, %v", ok, err) + } +} + +func TestConfirmOverwriteTUI(t *testing.T) { + restore := stubPromptYesNo(func(title, configPath, buildSig, message, yesLabel, noLabel string) (bool, error) { + if title != "Confirm overwrite" { + t.Fatalf("unexpected title %q", title) + } + if yesLabel != "Overwrite and restore" || noLabel != "Cancel" { + t.Fatalf("unexpected labels %q/%q", yesLabel, noLabel) + } + if !strings.Contains(message, "overwrite existing configuration files") { + t.Fatalf("missing warning text: %s", message) + } + return true, nil + }) + defer restore() + + ok, err := confirmOverwriteTUI("cfg", "sig") + if err != nil || !ok { + t.Fatalf("confirmOverwriteTUI returned %v, %v", ok, err) + } +} + +func stubPromptYesNo(fn func(title, configPath, buildSig, message, yesLabel, noLabel string) (bool, error)) func() { + orig := promptYesNoTUIFunc + promptYesNoTUIFunc = fn + return func() { promptYesNoTUIFunc = orig } +} diff --git a/internal/orchestrator/securemem_test.go b/internal/orchestrator/securemem_test.go new file mode 100644 index 0000000..d5b08e1 --- /dev/null +++ b/internal/orchestrator/securemem_test.go @@ -0,0 +1,23 @@ +package orchestrator + +import "testing" + +func TestZeroBytesClearsSlice(t *testing.T) { + data := []byte{1, 2, 3, 4} + zeroBytes(data) + for i, b := range data { + if b != 0 { + t.Fatalf("byte %d not zeroed: %d", i, b) + } + } + zeroBytes(nil) // should not panic +} + +func TestResetStringClearsValue(t *testing.T) { + value := "secret" + resetString(&value) + if value != "" { + t.Fatalf("expected string to be reset, got %q", value) + } + resetString(nil) // should not panic +}