P3 follow-up: editable target dir, conditional --no-ownership, UK lint
Three small follow-ups from review:
1. Restore target is now operator-editable. Default value is the
literal '\$HOME/rm-restore/<job-id>/' (agent expands \$HOME at
run time using os.UserHomeDir(); also handles \${HOME} and ~/
prefixes). Operator can replace with any absolute path.
- ui_restore.go validates the input is either absolute or starts
with one of the recognised prefixes; other env-var refs (\$PATH
etc.) are deliberately rejected so operator paths can't pick up
arbitrary agent env values.
- host_restore.html replaces the read-only mono-text display with
a real <input>; help text spells out that \$HOME resolves
agent-side and <job-id> is substituted on dispatch.
- install.sh + the systemd unit prep /root/rm-restore so the
default works under the sandbox: ReadWritePaths gains a soft
'-/root/rm-restore' entry (the '-' makes the bind-mount soft-fail
if missing, but install.sh pre-creates it root-owned 0700).
2. --no-ownership flag now gated on restic version. The flag was
added in restic 0.17 and 0.16 rejects it. Previously dropped it
wholesale — that meant new-dir restores silently preserved
ownership against design intent on 0.17+. Now the agent threads
its detected restic version (sysinfo already collects it) through
runner.Config -> restic.Env, and RunRestore appends --no-ownership
only when AtLeastVersion(0, 17) returns true. 0.16 hosts still
restore with original uid/gid; help text in the wizard explicitly
notes this. The previous 'Original ownership is preserved' copy
was wrong for new-dir mode and is corrected.
3. golangci-lint misspell locale switched US -> UK and the codebase
swept (73 corrections, mostly behaviour/serialise/recognise/honour).
Wire-format ErrorCode 'unauthorized' -> 'unauthorised' is a tiny
contract change but the agent doesn't parse those codes today and
no external API consumers exist yet. Tests passed before + after.
Tests:
- internal/restic/version_test.go covers Env.AtLeastVersion across
edge cases (empty, exact match, patch above, minor below, non-
numeric) and expandHome on \$HOME / \${HOME} / ~/, plus
pass-through for absolute paths and refusal of other env vars.
- ui_restore_test updated: TargetDir now starts '\$HOME/rm-restore/'
with the job_id substituted into the placeholder.
Live verified on the smoke env: default target restored to
/root/rm-restore/<job-id>/ as the agent's expanded \$HOME (2 files,
14 bytes); custom override '/tmp/custom-restore/<job-id>/' restored
into the agent's PrivateTmp namespace (1 file, 6 bytes); both jobs
'succeeded', exit 0.
This commit is contained in:
@@ -32,7 +32,7 @@ type LsEntry struct {
|
||||
//
|
||||
// The first emitted line is restic's "snapshot" preamble (struct_type
|
||||
// = "snapshot") which we discard. Subsequent lines are nodes; we
|
||||
// match on path equal to dirPath + "/" + name (with normalization so
|
||||
// match on path equal to dirPath + "/" + name (with normalisation so
|
||||
// trailing slashes don't break the comparison).
|
||||
//
|
||||
// dirPath="" or "/" lists the snapshot root.
|
||||
|
||||
+51
-10
@@ -7,7 +7,9 @@ import (
|
||||
"errors"
|
||||
"fmt"
|
||||
"io"
|
||||
"os"
|
||||
"os/exec"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
)
|
||||
|
||||
@@ -63,17 +65,26 @@ func (e Env) RunRestore(ctx context.Context, snapshotID string, paths []string,
|
||||
target := targetDir
|
||||
if inPlace {
|
||||
target = "/"
|
||||
} else {
|
||||
// Expand $HOME / ${HOME} / leading ~/ in the operator-supplied
|
||||
// path, using the agent's own HOME (which under the systemd
|
||||
// unit is the agent user's home — typically /root for the
|
||||
// User=root unit). The expansion runs agent-side so the
|
||||
// operator can specify a portable default like
|
||||
// $HOME/rm-restore/<job-id>/ in the wizard without the server
|
||||
// needing to know which user the agent runs as.
|
||||
target = expandHome(target)
|
||||
}
|
||||
args = append(args, "--target", target)
|
||||
// NOTE: restic added --no-ownership in 0.17. Older versions reject
|
||||
// the flag with "unknown flag: --no-ownership" before doing any
|
||||
// work. Since the agent runs as root in the systemd unit, files
|
||||
// land under /var/restic-restore with their original uid/gid
|
||||
// either way — the original "cp without sudo" rationale doesn't
|
||||
// hold (operators copying from /var/restic-restore need sudo
|
||||
// regardless because the parent dir is root-owned). Drop the flag
|
||||
// entirely until we drop 0.16 support; revisit if a non-root
|
||||
// agent deployment requirement comes back.
|
||||
// --no-ownership was added in restic 0.17. Older versions reject
|
||||
// the flag with "unknown flag: --no-ownership". For new-dir
|
||||
// restores we want the files owned by the agent user (operator
|
||||
// can cp them without juggling chown), so pass the flag iff the
|
||||
// running restic supports it. In-place restores always preserve
|
||||
// ownership — that's the whole point of in-place.
|
||||
if !inPlace && e.AtLeastVersion(0, 17) {
|
||||
args = append(args, "--no-ownership")
|
||||
}
|
||||
for _, p := range paths {
|
||||
args = append(args, "--include", p)
|
||||
}
|
||||
@@ -119,7 +130,7 @@ func (e Env) RunRestore(ctx context.Context, snapshotID string, paths []string,
|
||||
// stdout — but unlike backup we include the raw status JSON in
|
||||
// log.stream too because restore is short and the live log audience
|
||||
// genuinely benefits from the per-file traffic. Actually — we mirror
|
||||
// backup's behavior and DROP raw status lines from log.stream
|
||||
// backup's behaviour and DROP raw status lines from log.stream
|
||||
// (they'd drown the log on a fast restore); the progress envelope
|
||||
// covers them.
|
||||
func pumpRestoreStdout(r io.Reader, handle LineHandler, summary **RestoreSummary) error {
|
||||
@@ -168,6 +179,36 @@ func pumpRestoreStdout(r io.Reader, handle LineHandler, summary **RestoreSummary
|
||||
return scanner.Err()
|
||||
}
|
||||
|
||||
// expandHome rewrites $HOME, ${HOME}, or a leading ~/ in p to the
|
||||
// agent process's home directory. Other env-var references are left
|
||||
// untouched on purpose (operator-supplied paths shouldn't be able to
|
||||
// pick up arbitrary agent env values like $PATH or $RESTIC_PASSWORD).
|
||||
// Returns p unchanged if HOME can't be resolved.
|
||||
func expandHome(p string) string {
|
||||
if p == "" {
|
||||
return p
|
||||
}
|
||||
home, err := os.UserHomeDir()
|
||||
if err != nil || home == "" {
|
||||
return p
|
||||
}
|
||||
switch {
|
||||
case strings.HasPrefix(p, "$HOME/"):
|
||||
return filepath.Join(home, p[len("$HOME/"):])
|
||||
case p == "$HOME":
|
||||
return home
|
||||
case strings.HasPrefix(p, "${HOME}/"):
|
||||
return filepath.Join(home, p[len("${HOME}/"):])
|
||||
case p == "${HOME}":
|
||||
return home
|
||||
case strings.HasPrefix(p, "~/"):
|
||||
return filepath.Join(home, p[2:])
|
||||
case p == "~":
|
||||
return home
|
||||
}
|
||||
return p
|
||||
}
|
||||
|
||||
// RunDiff executes `restic diff --json <a> <b>` and forwards every
|
||||
// line to handle as stdout. Restic emits per-line "change" objects
|
||||
// plus a final "statistics" object; we don't parse them server-side —
|
||||
|
||||
@@ -15,7 +15,7 @@ import (
|
||||
"time"
|
||||
)
|
||||
|
||||
// Locate resolves the path to the restic binary. Honor an explicit
|
||||
// Locate resolves the path to the restic binary. Honour an explicit
|
||||
// override if provided, else fall back to PATH.
|
||||
func Locate(override string) (string, error) {
|
||||
if override != "" {
|
||||
@@ -42,6 +42,7 @@ func Locate(override string) (string, error) {
|
||||
// in this package ever needs to *log* a URL, use RedactURL.
|
||||
type Env struct {
|
||||
Bin string // path to restic binary
|
||||
Version string // e.g. "0.17.1"; empty if unknown
|
||||
RepoURL string // RESTIC_REPOSITORY (no embedded creds)
|
||||
RepoUsername string // optional HTTP basic-auth user for rest: URLs
|
||||
RepoPassword string // doubles as RESTIC_PASSWORD and (for rest:) HTTP basic-auth password
|
||||
@@ -55,6 +56,45 @@ type Env struct {
|
||||
LimitDownloadKBps int
|
||||
}
|
||||
|
||||
// AtLeastVersion reports whether e.Version >= the given major/minor.
|
||||
// Comparison is best-effort: empty / unparseable versions return false
|
||||
// (callers stay on the conservative path). Patch level is ignored.
|
||||
func (e Env) AtLeastVersion(major, minor int) bool {
|
||||
v := strings.TrimSpace(e.Version)
|
||||
if v == "" {
|
||||
return false
|
||||
}
|
||||
parts := strings.SplitN(v, ".", 3)
|
||||
if len(parts) < 2 {
|
||||
return false
|
||||
}
|
||||
maj, err1 := atoi(parts[0])
|
||||
min, err2 := atoi(parts[1])
|
||||
if err1 != nil || err2 != nil {
|
||||
return false
|
||||
}
|
||||
if maj != major {
|
||||
return maj > major
|
||||
}
|
||||
return min >= minor
|
||||
}
|
||||
|
||||
// atoi is strconv.Atoi without dragging the import into a file that
|
||||
// only needs it for one helper.
|
||||
func atoi(s string) (int, error) {
|
||||
n := 0
|
||||
if len(s) == 0 {
|
||||
return 0, fmt.Errorf("empty")
|
||||
}
|
||||
for _, r := range s {
|
||||
if r < '0' || r > '9' {
|
||||
return 0, fmt.Errorf("not a digit: %q", r)
|
||||
}
|
||||
n = n*10 + int(r-'0')
|
||||
}
|
||||
return n, nil
|
||||
}
|
||||
|
||||
// globalArgs returns restic's pre-subcommand global flags derived
|
||||
// from the Env. Currently just bandwidth caps.
|
||||
func (e Env) globalArgs() []string {
|
||||
@@ -69,8 +109,8 @@ func (e Env) globalArgs() []string {
|
||||
}
|
||||
|
||||
// resticCmd builds an exec.Cmd with bandwidth-limit globals prefixed
|
||||
// before the supplied subcommand args. Centralizing this so every
|
||||
// command (backup/forget/prune/check/unlock/init/stats) honors
|
||||
// before the supplied subcommand args. Centralising this so every
|
||||
// command (backup/forget/prune/check/unlock/init/stats) honours
|
||||
// the caps without each call site having to remember.
|
||||
//
|
||||
// Cancellation: by default exec.CommandContext sends SIGKILL when
|
||||
@@ -142,7 +182,7 @@ type BackupSummary struct {
|
||||
}
|
||||
|
||||
// LineHandler receives every stdout/stderr line. event is non-nil
|
||||
// when the line is a recognized JSON status; raw always carries the
|
||||
// when the line is a recognised JSON status; raw always carries the
|
||||
// original text (so we can also tee to job_logs as `stdout`).
|
||||
type LineHandler func(stream string, raw string, event any)
|
||||
|
||||
@@ -282,7 +322,7 @@ func (e Env) RunInit(ctx context.Context, handle LineHandler) error {
|
||||
|
||||
// Sniff for "config file already exists" on stderr; if we see it
|
||||
// we'll treat the non-zero exit as a soft success — running init
|
||||
// against an already-initialized repo is a no-op semantically,
|
||||
// against an already-initialised repo is a no-op semantically,
|
||||
// not a failure. Wraps the caller's handle so the line still
|
||||
// gets streamed verbatim to the operator-facing log.
|
||||
alreadyInited := false
|
||||
@@ -298,7 +338,7 @@ func (e Env) RunInit(ctx context.Context, handle LineHandler) error {
|
||||
if err := runWithPump(cmd, sniff); err != nil {
|
||||
if alreadyInited {
|
||||
if handle != nil {
|
||||
handle("event", "repo already initialized — treating as success", nil)
|
||||
handle("event", "repo already initialised — treating as success", nil)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
@@ -394,7 +434,7 @@ func (e Env) RunStats(ctx context.Context, handle LineHandler) (*RepoStats, erro
|
||||
return out, nil
|
||||
}
|
||||
|
||||
// CheckResult summarizes a `restic check` invocation. LockPresent is
|
||||
// CheckResult summarises a `restic check` invocation. LockPresent is
|
||||
// true if the stderr stream contained a stale-lock signal (caller is
|
||||
// expected to surface this in the UI so the operator can run unlock).
|
||||
// ErrorsFound is true if check exited with a non-zero status (errors
|
||||
@@ -406,7 +446,7 @@ type CheckResult struct {
|
||||
|
||||
// RunCheck executes `restic check` with optional --read-data-subset.
|
||||
// subsetPct of 0 omits the flag (full data check); >0 passes
|
||||
// --read-data-subset N%. Returns a CheckResult summarizing what was
|
||||
// --read-data-subset N%. Returns a CheckResult summarising what was
|
||||
// sniffed from stderr; the result is set even if check itself
|
||||
// returns an error (so the caller can persist last_check_status).
|
||||
func (e Env) RunCheck(ctx context.Context, subsetPct int, handle LineHandler) (CheckResult, error) {
|
||||
|
||||
@@ -0,0 +1,64 @@
|
||||
package restic
|
||||
|
||||
import (
|
||||
"path/filepath"
|
||||
"testing"
|
||||
)
|
||||
|
||||
func TestEnvAtLeastVersion(t *testing.T) {
|
||||
t.Parallel()
|
||||
cases := []struct {
|
||||
ver string
|
||||
major int
|
||||
minor int
|
||||
want bool
|
||||
shortDesc string
|
||||
}{
|
||||
{"0.17.0", 0, 17, true, "exact match"},
|
||||
{"0.17.1", 0, 17, true, "patch above"},
|
||||
{"0.18.0", 0, 17, true, "minor above"},
|
||||
{"1.0.0", 0, 17, true, "major above"},
|
||||
{"0.16.4", 0, 17, false, "minor below"},
|
||||
{"0.16", 0, 17, false, "two-part minor below"},
|
||||
{"", 0, 17, false, "empty"},
|
||||
{"v0.17", 0, 17, false, "prefixed v rejected"},
|
||||
{"unknown", 0, 17, false, "non-numeric rejected"},
|
||||
}
|
||||
for _, c := range cases {
|
||||
got := Env{Version: c.ver}.AtLeastVersion(c.major, c.minor)
|
||||
if got != c.want {
|
||||
t.Errorf("AtLeastVersion(%q, %d, %d): got %v want %v · %s",
|
||||
c.ver, c.major, c.minor, got, c.want, c.shortDesc)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestExpandHome(t *testing.T) {
|
||||
// Not parallel — t.Setenv on HOME would race with sibling tests.
|
||||
tmp := t.TempDir()
|
||||
t.Setenv("HOME", tmp)
|
||||
|
||||
cases := []struct {
|
||||
in, want string
|
||||
}{
|
||||
{"$HOME/rm-restore/job-1/", filepath.Join(tmp, "rm-restore/job-1")},
|
||||
{"${HOME}/rm-restore/job-2/", filepath.Join(tmp, "rm-restore/job-2")},
|
||||
{"~/rm-restore/job-3/", filepath.Join(tmp, "rm-restore/job-3")},
|
||||
{"$HOME", tmp},
|
||||
{"~", tmp},
|
||||
{"/var/lib/x/y", "/var/lib/x/y"}, // absolute path passes through
|
||||
{"", ""},
|
||||
{"$PATH/foo", "$PATH/foo"}, // other env vars not expanded
|
||||
}
|
||||
for _, c := range cases {
|
||||
got := expandHome(c.in)
|
||||
if got != c.want {
|
||||
t.Errorf("expandHome(%q): got %q want %q", c.in, got, c.want)
|
||||
}
|
||||
}
|
||||
|
||||
// Sanity: an absolute path always passes through regardless of HOME.
|
||||
if got := expandHome("/abs"); got != "/abs" {
|
||||
t.Errorf("expandHome(/abs): got %q", got)
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user