diff --git a/cmd/proxyctl/main.go b/cmd/proxyctl/main.go index 3bb34d9..791e142 100644 --- a/cmd/proxyctl/main.go +++ b/cmd/proxyctl/main.go @@ -197,7 +197,16 @@ func runMaintenanceCommand(args []string, input io.Reader, output io.Writer, sto if args[0] != "reset" { return errors.New("invalid maintenance command") } - fmt.Fprint(output, "Reset every managed Target and restore recorded original state? [y/N] ") + readOnlyState, readOnlyErr := store.ReadOnlyLoad() + if readOnlyErr != nil { + return fmt.Errorf("inspect Profile state before reset confirmation: %w", readOnlyErr) + } + warning := app.NewMaintenance(readOnlyState, targets, nil, nil).ResetConfirmation(context.Background()) + fmt.Fprintln(output, "Reset every managed Target and restore recorded original state?") + if warning != "" { + fmt.Fprintln(output, warning) + } + fmt.Fprint(output, "Continue? [y/N] ") answer, _ := bufio.NewReader(input).ReadString('\n') answer = strings.ToLower(strings.TrimSpace(answer)) if answer != "y" && answer != "yes" { @@ -222,8 +231,15 @@ func runMaintenanceCommand(args []string, input io.Reader, output io.Writer, sto fmt.Fprintf(output, "%s: skipped (%s) — %s\n", item.TargetID, item.Inspection.State, profile.RedactText(item.Inspection.Evidence)) continue } - if item.Err != nil { - fmt.Fprintf(output, "%s: failed (%s) — %s; fresh inspection: %s\n", item.TargetID, item.Inspection.State, profile.RedactText(item.Err.Error()), profile.RedactText(item.Inspection.Evidence)) + if item.OperationErr != nil { + fmt.Fprintf(output, "%s: failed (%s) — %s; fresh inspection: %s\n", item.TargetID, item.Inspection.State, profile.RedactText(item.OperationErr.Error()), profile.RedactText(item.Inspection.Evidence)) + if item.LogErr != nil { + fmt.Fprintf(output, "%s: operation logging also failed — %s\n", item.TargetID, profile.RedactText(item.LogErr.Error())) + } + continue + } + if item.LogErr != nil { + fmt.Fprintf(output, "%s: reset (%s) — %s; operation logging failed — %s\n", item.TargetID, item.Inspection.State, profile.RedactText(item.Inspection.Evidence), profile.RedactText(item.LogErr.Error())) continue } fmt.Fprintf(output, "%s: reset (%s) — %s\n", item.TargetID, item.Inspection.State, profile.RedactText(item.Inspection.Evidence)) diff --git a/cmd/proxyctl/maintenance_test.go b/cmd/proxyctl/maintenance_test.go index f1707d7..9ccf39a 100644 --- a/cmd/proxyctl/maintenance_test.go +++ b/cmd/proxyctl/maintenance_test.go @@ -12,6 +12,7 @@ import ( "strings" "testing" + "proxyctl/internal/app" "proxyctl/internal/gnome" "proxyctl/internal/operationlock" "proxyctl/internal/oplog" @@ -29,11 +30,16 @@ func (failingIdentityEnvironment) LookupUser(string) (profile.UserIdentity, erro } func (failingIdentityEnvironment) LookupEnv(string) (string, bool) { return "", false } +type failingOperationLogger struct{ err error } + +func (l failingOperationLogger) Write(app.LogEntry) error { return l.err } + type processFakeTarget struct { - id string - state target.State - err error - calls int + id string + state target.State + err error + calls int + confirmation string } func (f *processFakeTarget) ID() string { return f.id } @@ -41,7 +47,7 @@ func (f *processFakeTarget) Description() target.Description { return target.Des func (f *processFakeTarget) Availability(context.Context) target.Availability { return target.Availability{Available: true, Evidence: "fake executable found"} } -func (f *processFakeTarget) Confirmation(bool) string { return "" } +func (f *processFakeTarget) Confirmation(bool) string { return f.confirmation } func (f *processFakeTarget) Inspect(context.Context, *profile.Profile) target.Inspection { return target.Inspection{State: f.state, Evidence: "fake evidence for http://alice:secret@proxy.test"} } @@ -217,14 +223,21 @@ func TestCommandsRunAsRealProcessesInsideIsolatedUserState(t *testing.T) { if !strings.Contains(string(doctorOutput), "Profile validity") || (doctorErr != nil && !strings.Contains(string(doctorOutput), "[FAIL]")) { t.Fatalf("doctor process err=%v output=%q", doctorErr, doctorOutput) } + resetMarker := filepath.Join(root, "home", "reset-mutation-marker") resetOutput, err := invoke("no\n", "reset") - if err != nil || !strings.Contains(string(resetOutput), "Reset cancelled") { + if err != nil || !strings.Contains(string(resetOutput), "Reset cancelled") || !strings.Contains(string(resetOutput), "Docker will restart immediately") { t.Fatalf("reset cancellation process err=%v output=%q", err, resetOutput) } + if _, statErr := os.Stat(resetMarker); !errors.Is(statErr, os.ErrNotExist) { + t.Fatalf("cancelled real process mutated reset fixture: %v", statErr) + } resetOutput, err = invoke("yes\n", "reset") - if err != nil || !strings.Contains(string(resetOutput), "shell: skipped (Disabled)") { + if err != nil || !strings.Contains(string(resetOutput), "shell: skipped (Disabled)") || !strings.Contains(string(resetOutput), "docker-daemon-test: reset (Disabled)") { t.Fatalf("confirmed reset process err=%v output=%q", err, resetOutput) } + if data, readErr := os.ReadFile(resetMarker); readErr != nil || string(data) != "reset\n" { + t.Fatalf("confirmed real process marker err=%v data=%q", readErr, data) + } installOutput, err := invoke("", "shell", "install") if err != nil || !strings.Contains(string(installOutput), "installed") { t.Fatalf("shell install process err=%v output=%q", err, installOutput) @@ -359,6 +372,47 @@ func TestResetSuppressesSavedGNOMEPasswordFromOperationCLIAndLogsWithoutActivePr } } +func TestResetPromptIncludesEveryManagedTargetWarningAndCancellationDoesNotMutate(t *testing.T) { + root := t.TempDir() + config := filepath.Join(root, "profiles.json") + writeConfig(t, config, `{"profiles":[]}`) + store := profile.NewFileStore(config) + daemon := &processFakeTarget{id: "docker-daemon", state: target.Drifted, confirmation: "Docker will restart immediately and may disrupt running containers and other work."} + other := &processFakeTarget{id: "other", state: target.Enabled, confirmation: "Other managed warning."} + external := &processFakeTarget{id: "external", state: target.External, confirmation: "must not appear"} + var output bytes.Buffer + + err := runMaintenanceCommand([]string{"reset"}, strings.NewReader("no\n"), &output, store, []target.Target{daemon, other, external}, operationlock.New(filepath.Join(root, "operation.lock")), nil, nil) + if err != nil { + t.Fatal(err) + } + text := output.String() + for _, want := range []string{"docker-daemon: Docker will restart immediately", "other: Other managed warning", "Reset cancelled"} { + if !strings.Contains(text, want) { + t.Errorf("reset output missing %q: %q", want, text) + } + } + if strings.Contains(text, "must not appear") || daemon.calls+other.calls+external.calls != 0 { + t.Fatalf("cancel output=%q calls=%d/%d/%d", text, daemon.calls, other.calls, external.calls) + } +} + +func TestResetCommandReportsCompletedTargetTruthWhenOperationLoggingFails(t *testing.T) { + root := t.TempDir() + config := filepath.Join(root, "profiles.json") + writeConfig(t, config, `{"profiles":[]}`) + managed := &processFakeTarget{id: "managed", state: target.Enabled} + var output bytes.Buffer + + err := runMaintenanceCommand([]string{"reset"}, strings.NewReader("yes\n"), &output, profile.NewFileStore(config), []target.Target{managed}, operationlock.New(filepath.Join(root, "operation.lock")), failingOperationLogger{err: errors.New("log device full")}, nil) + if err == nil || !strings.Contains(err.Error(), "log device full") { + t.Fatalf("logger failure err=%v output=%q", err, output.String()) + } + if managed.calls != 1 || managed.state != target.Disabled || !strings.Contains(output.String(), "managed: reset (Disabled)") || !strings.Contains(output.String(), "operation logging failed") { + t.Fatalf("logger failure truth output=%q calls=%d state=%s", output.String(), managed.calls, managed.state) + } +} + func TestResetCommandCancellationAndTruthfulPartialFailureWithFakeTargets(t *testing.T) { root := t.TempDir() config := filepath.Join(root, "xdg-config", "proxyctl", "profiles.json") diff --git a/cmd/proxyctl/process_testenv.go b/cmd/proxyctl/process_testenv.go index 3e5f493..88a3810 100644 --- a/cmd/proxyctl/process_testenv.go +++ b/cmd/proxyctl/process_testenv.go @@ -3,21 +3,47 @@ package main import ( + "context" "errors" "os" "os/user" + "path/filepath" "strconv" "proxyctl/internal/profile" "proxyctl/internal/target" ) +type processResetTarget struct{ marker string } + +func (t processResetTarget) ID() string { return "docker-daemon-test" } +func (t processResetTarget) Description() target.Description { + return target.Description{Name: "Docker daemon"} +} +func (t processResetTarget) Availability(context.Context) target.Availability { + return target.Availability{Available: true, Evidence: "isolated process fixture"} +} +func (t processResetTarget) Inspect(context.Context, *profile.Profile) target.Inspection { + if _, err := os.Stat(t.marker); err == nil { + return target.Inspection{State: target.Disabled, Evidence: "isolated managed configuration removed"} + } + return target.Inspection{State: target.Drifted, Evidence: "isolated recovery metadata exists"} +} +func (t processResetTarget) Confirmation(bool) string { + return "Docker will restart immediately. This may disrupt running containers and other work." +} +func (t processResetTarget) SetDesired(context.Context, bool, *profile.Profile) target.OperationResult { + err := os.WriteFile(t.marker, []byte("reset\n"), 0o600) + return target.OperationResult{Inspection: t.Inspect(context.Background(), nil), Err: err} +} + // This file is compiled only into the process-smoke binary. It gives that real // executable an identity whose home is inside the test sandbox and omits // production system Targets, so no command can reach developer or host state. func init() { processProductionTargets = func(profile.IdentityEnvironment) ([]target.Target, error) { - return []target.Target{}, nil + home := os.Getenv("PROXYCTL_PROCESS_TEST_HOME") + return []target.Target{processResetTarget{marker: filepath.Join(home, "reset-mutation-marker")}}, nil } processCurrentUser = func() (profile.UserIdentity, error) { home := os.Getenv("PROXYCTL_PROCESS_TEST_HOME") diff --git a/internal/app/maintenance.go b/internal/app/maintenance.go index 133da03..07ceb6f 100644 --- a/internal/app/maintenance.go +++ b/internal/app/maintenance.go @@ -94,10 +94,12 @@ func (m *Maintenance) Doctor(ctx context.Context, applicationChecks []target.Che } type ResetTargetResult struct { - TargetID string - Inspection target.Inspection - Skipped bool - Err error + TargetID string + Inspection target.Inspection + Skipped bool + OperationErr error + LogErr error + Err error } type ResetResult struct { Cancelled bool @@ -105,6 +107,23 @@ type ResetResult struct { Err error } +// ResetConfirmation performs only read-only inspection and returns every +// warning required by Targets that a reset would attempt to disable. +func (m *Maintenance) ResetConfirmation(ctx context.Context) string { + active := activeFromState(m.state) + var warnings []string + for _, adapter := range m.targets { + inspection := adapter.Inspect(ctx, active) + if inspection.State != target.Enabled && inspection.State != target.Drifted { + continue + } + if warning := adapter.Confirmation(false); warning != "" { + warnings = append(warnings, adapter.ID()+": "+warning) + } + } + return strings.Join(warnings, "\n") +} + func (m *Maintenance) Reset(ctx context.Context, confirmed bool) ResetResult { if !confirmed { return ResetResult{Cancelled: true} @@ -138,10 +157,18 @@ func (m *Maintenance) Reset(ctx context.Context, confirmed bool) ResetResult { result.Err = errors.Join(result.Err, fmt.Errorf("reset %s Target: %w", adapter.ID(), redactError(operation.Err))) } detail = RedactLogDetail(detail) + operationErr := redactError(operation.Err) + var logErr error if m.logger != nil { - _ = m.logger.Write(LogEntry{Time: time.Now(), Operation: "reset", Target: adapter.ID(), Result: outcome, Detail: detail}) + if err := m.logger.Write(LogEntry{Time: time.Now(), Operation: "reset", Target: adapter.ID(), Result: outcome, Detail: detail}); err != nil { + logErr = redactError(err) + result.Err = errors.Join(result.Err, fmt.Errorf("record reset %s Target operation log: %w", adapter.ID(), logErr)) + } } - result.Targets = append(result.Targets, ResetTargetResult{TargetID: adapter.ID(), Inspection: finalInspection, Err: redactError(operation.Err)}) + result.Targets = append(result.Targets, ResetTargetResult{ + TargetID: adapter.ID(), Inspection: finalInspection, + OperationErr: operationErr, LogErr: logErr, Err: errors.Join(operationErr, logErr), + }) } return result } diff --git a/internal/app/maintenance_test.go b/internal/app/maintenance_test.go index d4022a6..d4b52ca 100644 --- a/internal/app/maintenance_test.go +++ b/internal/app/maintenance_test.go @@ -19,6 +19,7 @@ type maintenanceTarget struct { checks []target.Check disableErr error disableCalls int + confirmation string } func (f *maintenanceTarget) ID() string { return f.id } @@ -26,7 +27,7 @@ func (f *maintenanceTarget) Description() target.Description { return target.Description{Name: strings.ToUpper(f.id)} } func (f *maintenanceTarget) Availability(context.Context) target.Availability { return f.available } -func (f *maintenanceTarget) Confirmation(bool) string { return "" } +func (f *maintenanceTarget) Confirmation(bool) string { return f.confirmation } func (f *maintenanceTarget) Inspect(context.Context, *profile.Profile) target.Inspection { return f.inspection } @@ -73,11 +74,14 @@ func (l *recordingLock) Lock(context.Context) (func(), error) { return func() { l.held-- }, nil } -type recordingLog struct{ entries []app.LogEntry } +type recordingLog struct { + entries []app.LogEntry + err error +} func (l *recordingLog) Write(entry app.LogEntry) error { l.entries = append(l.entries, entry) - return nil + return l.err } func TestDoctorReportsReadOnlyChecksEvidenceAndRedactsCredentials(t *testing.T) { @@ -149,6 +153,41 @@ func TestResetFailureUsesFreshInspectionForResultAndRedactedLog(t *testing.T) { } } +func TestResetConfirmationAggregatesWarningsOnlyForManagedCandidatesWithoutMutation(t *testing.T) { + managed := &maintenanceTarget{id: "managed", inspection: target.Inspection{State: target.Enabled, Evidence: "managed"}} + drifted := &maintenanceTarget{id: "drifted", inspection: target.Inspection{State: target.Drifted, Evidence: "recovery metadata remains"}} + external := &maintenanceTarget{id: "external", inspection: target.Inspection{State: target.External, Evidence: "administrator owned"}} + managed.confirmation = "managed warning" + drifted.confirmation = "drifted warning" + external.confirmation = "must not be shown" + maintenance := app.NewMaintenance(profile.State{}, []target.Target{managed, drifted, external}, &recordingLock{}, nil) + + confirmation := maintenance.ResetConfirmation(context.Background()) + for _, want := range []string{"managed: managed warning", "drifted: drifted warning"} { + if !strings.Contains(confirmation, want) { + t.Errorf("confirmation missing %q: %q", want, confirmation) + } + } + if strings.Contains(confirmation, "must not be shown") || managed.disableCalls+drifted.disableCalls+external.disableCalls != 0 { + t.Fatalf("confirmation=%q mutation calls=%d/%d/%d", confirmation, managed.disableCalls, drifted.disableCalls, external.disableCalls) + } +} + +func TestResetLoggerFailureIsReportedWithoutUndoingCompletedConfiguration(t *testing.T) { + adapter := &maintenanceTarget{id: "managed", inspection: target.Inspection{State: target.Enabled, Evidence: "managed"}} + logger := &recordingLog{err: errors.New("operation log disk full")} + + result := app.NewMaintenance(profile.State{}, []target.Target{adapter}, &recordingLock{}, logger).Reset(context.Background(), true) + + if result.Err == nil || !strings.Contains(result.Err.Error(), "operation log disk full") || len(result.Targets) != 1 { + t.Fatalf("reset result = %#v", result) + } + item := result.Targets[0] + if item.Inspection.State != target.Disabled || item.OperationErr != nil || item.LogErr == nil || item.Err == nil || adapter.disableCalls != 1 { + t.Fatalf("logger failure outcome=%#v disable calls=%d", item, adapter.disableCalls) + } +} + func TestResetRequiresConfirmationAndReportsTruthfulPartialResults(t *testing.T) { good := &maintenanceTarget{id: "good", available: target.Availability{Available: true}, inspection: target.Inspection{State: target.Enabled, Evidence: "managed"}} failure := errors.New("remove https://alice:secret@proxy.example failed: permission denied") diff --git a/internal/dockerclient/dockerclient.go b/internal/dockerclient/dockerclient.go index 2e4a997..1c386d1 100644 --- a/internal/dockerclient/dockerclient.go +++ b/internal/dockerclient/dockerclient.go @@ -358,6 +358,19 @@ func (a *Adapter) disable() error { if err != nil { return err } + if err := validateManagedProxyStrings(defaults); err != nil { + return err + } + currentFingerprint := fingerprintCurrent(config) + if currentFingerprint == fingerprintOriginal(metadata) { + if err := a.files.Remove(a.configuration.RecoveryFile); err != nil && !errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf("clean Docker client recovery metadata for already-restored proxy defaults: %w", err) + } + return nil + } + if currentFingerprint != metadata.Fingerprint { + return errors.New("Docker client managed proxy defaults changed after proxyctl applied them; refusing to overwrite current configuration or discard recovery metadata") + } for _, key := range managedKeys { prior := metadata.Original[key] if prior.Present { @@ -533,6 +546,19 @@ func fingerprintDefaults(values map[string]string) string { return fingerprintSnapshot(snapshot) } +func fingerprintOriginal(metadata recovery) string { + snapshot := make(map[string]managedValue, len(managedKeys)) + for _, key := range managedKeys { + prior := metadata.Original[key] + value := managedValue{Present: prior.Present} + if prior.Present && json.Unmarshal(prior.Value, &value.Value) == nil { + value.Valid = true + } + snapshot[key] = value + } + return fingerprintSnapshot(snapshot) +} + func fingerprintCurrent(config map[string]json.RawMessage) string { snapshot := make(map[string]managedValue, len(managedKeys)) proxies, _, proxyErr := childObject(config, "proxies") diff --git a/internal/dockerclient/dockerclient_test.go b/internal/dockerclient/dockerclient_test.go index 2268713..5866403 100644 --- a/internal/dockerclient/dockerclient_test.go +++ b/internal/dockerclient/dockerclient_test.go @@ -13,6 +13,7 @@ import ( "testing" "time" + "proxyctl/internal/app" "proxyctl/internal/dockerclient" "proxyctl/internal/profile" "proxyctl/internal/target" @@ -202,6 +203,56 @@ func TestEnableSafelyMergesAndDisableRestoresPriorDefaults(t *testing.T) { } } +func TestResetDriftedDockerClientRejectsAdministratorManagedKeyEditsWithoutWritesOrCleanup(t *testing.T) { + files := &countingFS{FileSystem: dockerclient.OSFileSystem{}} + a := newAdapterWithFS(t, files) + writeConfig(t, a.ConfigPath(), `{"auths":{"keep":{}},"proxies":{"default":{"httpProxy":"http://prior.example:3128"}}}`) + if result := a.SetDesired(context.Background(), true, activeProfile()); result.Err != nil { + t.Fatal(result.Err) + } + updateDefaultKey(t, a.ConfigPath(), "httpProxy", json.RawMessage(`"http://administrator.example:3128"`)) + configBefore := readFile(t, a.ConfigPath()) + metadataBefore := readFile(t, a.RecoveryPath()) + writesBefore := files.writes + + reset := app.NewMaintenance(profile.State{Profiles: []profile.Profile{*activeProfile()}, ActiveProfile: activeProfile().Name}, []target.Target{a}, immediateLock{}, nil).Reset(context.Background(), true) + if reset.Err == nil || len(reset.Targets) != 1 || reset.Targets[0].Inspection.State != target.Drifted || reset.Targets[0].Err == nil { + t.Fatalf("conflicting reset = %#v", reset) + } + if !strings.Contains(reset.Targets[0].Err.Error(), "changed after proxyctl applied") || files.writes != writesBefore { + t.Fatalf("conflict outcome=%#v writes=%d->%d", reset.Targets[0], writesBefore, files.writes) + } + if got := readFile(t, a.ConfigPath()); got != configBefore { + t.Fatalf("conflict rewrote config:\n%s", got) + } + if got := readFile(t, a.RecoveryPath()); got != metadataBefore { + t.Fatalf("conflict changed metadata:\n%s", got) + } +} + +func TestResetDriftedDockerClientAlreadyAtOriginalOnlyCleansMetadata(t *testing.T) { + files := &countingFS{FileSystem: dockerclient.OSFileSystem{}} + a := newAdapterWithFS(t, files) + original := `{"auths":{"keep":{}},"proxies":{"default":{"httpProxy":"http://prior.example:3128","custom":{"keep":true}}}}` + writeConfig(t, a.ConfigPath(), original) + if result := a.SetDesired(context.Background(), true, activeProfile()); result.Err != nil { + t.Fatal(result.Err) + } + writeConfig(t, a.ConfigPath(), original) + writesBefore := files.writes + + reset := app.NewMaintenance(profile.State{Profiles: []profile.Profile{*activeProfile()}, ActiveProfile: activeProfile().Name}, []target.Target{a}, immediateLock{}, nil).Reset(context.Background(), true) + if reset.Err != nil || len(reset.Targets) != 1 || reset.Targets[0].Inspection.State != target.External { + t.Fatalf("already-original reset = %#v", reset) + } + if files.writes != writesBefore || readFile(t, a.ConfigPath()) != original { + t.Fatalf("already-original reset wrote config: writes=%d->%d", writesBefore, files.writes) + } + if _, err := os.Stat(a.RecoveryPath()); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("recovery metadata remains: %v", err) + } +} + func TestInspectionDistinguishesAllStatesAndManagedValueEditsDrift(t *testing.T) { a := newAdapter(t, nil) assertState(t, a, target.Disabled) @@ -563,6 +614,10 @@ func assertState(t *testing.T, a *dockerclient.Adapter, want target.State) { } } +type immediateLock struct{} + +func (immediateLock) Lock(context.Context) (func(), error) { return func() {}, nil } + type failingFS struct { dockerclient.FileSystem permission, failConfig, leak, restoreLeak, failRecoveryOnce bool diff --git a/internal/dockerdaemon/dockerdaemon.go b/internal/dockerdaemon/dockerdaemon.go index 894925d..0201908 100644 --- a/internal/dockerdaemon/dockerdaemon.go +++ b/internal/dockerdaemon/dockerdaemon.go @@ -348,6 +348,12 @@ func (a *Adapter) disable(ctx context.Context) error { alreadyOriginal := metadata.OriginalExists && currentErr == nil && fingerprint(current) == fingerprint(metadata.Original) alreadyAbsent := !metadata.OriginalExists && errors.Is(currentErr, fs.ErrNotExist) if alreadyOriginal || alreadyAbsent { + // Recovery metadata proves proxyctl previously changed the effective + // daemon configuration. Even when the file has already been restored, + // complete the service effects before relinquishing that recovery record. + if err := a.reloadRestartInspect(ctx); err != nil { + return err + } if err := a.files.Remove(a.configuration.RecoveryPath); err != nil && !errors.Is(err, fs.ErrNotExist) { return fmt.Errorf("clean Docker daemon recovery metadata for already-restored configuration: %w", err) } diff --git a/internal/dockerdaemon/dockerdaemon_test.go b/internal/dockerdaemon/dockerdaemon_test.go index 249c68e..596ccc8 100644 --- a/internal/dockerdaemon/dockerdaemon_test.go +++ b/internal/dockerdaemon/dockerdaemon_test.go @@ -23,6 +23,7 @@ type recorder struct{ events []string } type fakeRunner struct { recorder *recorder show string + active string fail map[string]error } @@ -36,7 +37,7 @@ func (r *fakeRunner) Run(_ context.Context, name string, args ...string) ([]byte return []byte(r.show), nil } if strings.Contains(command, " is-active ") { - return []byte("active\n"), nil + return []byte(r.active), nil } return nil, nil } @@ -73,7 +74,7 @@ func fixture(t *testing.T) (*dockerdaemon.Adapter, *recorder, string, string, *f daemonPath := filepath.Join(directory, "etc", "docker", "daemon.json") recoveryPath := filepath.Join(directory, "state", "docker-daemon.json") record := &recorder{} - runner := &fakeRunner{recorder: record, show: "LoadState=loaded\nFragmentPath=/lib/systemd/system/docker.service\n", fail: map[string]error{}} + runner := &fakeRunner{recorder: record, show: "LoadState=loaded\nFragmentPath=/lib/systemd/system/docker.service\n", active: "active\n", fail: map[string]error{}} adapter := dockerdaemon.New(dockerdaemon.Configuration{DaemonConfigPath: daemonPath, RecoveryPath: recoveryPath, TrustedRoot: directory}, recordingFS{recorder: record}, runner) return adapter, record, daemonPath, recoveryPath, runner } @@ -429,9 +430,75 @@ func TestDockerDaemonDisableAndResetRejectDriftWithoutMutatingDaemonOrRecovery(t } } -func TestDockerDaemonResetCleansAlreadyRestoredRecoveryWithoutDaemonWriteOrSystemctl(t *testing.T) { +func TestDockerDaemonResetAlreadyRestoredReloadsRestartsAndInspectsBeforeMetadataCleanup(t *testing.T) { a, record, daemonPath, recoveryPath, _ := fixture(t) original := []byte("{\n \"log-level\": \"debug\",\n \"proxies\": {\"http-proxy\": \"http://prior.example:3128\"}\n}\n") + prepareAlreadyRestoredDaemon(t, a, daemonPath, original) + record.events = nil + + reset := app.NewMaintenance(profile.State{Profiles: []profile.Profile{*activeProfile()}, ActiveProfile: activeProfile().Name}, []target.Target{a}, immediateLock{}, nil).Reset(context.Background(), true) + if reset.Err != nil || len(reset.Targets) != 1 || reset.Targets[0].Err != nil || reset.Targets[0].Inspection.State != target.External { + t.Fatalf("already-restored reset = %#v", reset) + } + assertSubsequence(t, record.events, []string{"systemctl daemon-reload", "systemctl restart docker.service", "systemctl is-active docker.service", "remove docker-daemon.json"}) + if contains(record.events, "write daemon.json") { + t.Fatalf("already-restored reset rewrote daemon config: %#v", record.events) + } + if got, err := os.ReadFile(daemonPath); err != nil || !reflect.DeepEqual(got, original) { + t.Fatalf("already-restored daemon configuration changed to %q: %v", got, err) + } + if _, err := os.Stat(recoveryPath); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("recovery metadata remains: %v", err) + } +} + +func TestDockerDaemonAlreadyRestoredPreservesMetadataAndReportsFreshDriftOnEveryServiceFailure(t *testing.T) { + failures := []struct { + name string + command string + active string + }{ + {name: "daemon reload command", command: "systemctl daemon-reload"}, + {name: "restart command", command: "systemctl restart docker.service"}, + {name: "active inspection command", command: "systemctl is-active docker.service"}, + {name: "inactive service state", active: "failed\n"}, + } + for _, failure := range failures { + t.Run(failure.name, func(t *testing.T) { + a, record, daemonPath, recoveryPath, runner := fixture(t) + original := []byte("{\n \"log-level\": \"debug\"\n}\n") + prepareAlreadyRestoredDaemon(t, a, daemonPath, original) + metadataBefore, err := os.ReadFile(recoveryPath) + if err != nil { + t.Fatal(err) + } + if failure.command != "" { + runner.fail[failure.command] = errors.New("service failure") + } + if failure.active != "" { + runner.active = failure.active + } + record.events = nil + + reset := app.NewMaintenance(profile.State{Profiles: []profile.Profile{*activeProfile()}, ActiveProfile: activeProfile().Name}, []target.Target{a}, immediateLock{}, nil).Reset(context.Background(), true) + if reset.Err == nil || len(reset.Targets) != 1 || reset.Targets[0].Inspection.State != target.Drifted { + t.Fatalf("failed already-restored reset = %#v", reset) + } + if contains(record.events, "write daemon.json") || contains(record.events, "remove docker-daemon.json") { + t.Fatalf("service failure mutated configuration or metadata: %#v", record.events) + } + if got, err := os.ReadFile(daemonPath); err != nil || !reflect.DeepEqual(got, original) { + t.Fatalf("daemon config changed to %q: %v", got, err) + } + if got, err := os.ReadFile(recoveryPath); err != nil || !reflect.DeepEqual(got, metadataBefore) { + t.Fatalf("metadata changed to %q: %v", got, err) + } + }) + } +} + +func prepareAlreadyRestoredDaemon(t *testing.T, a *dockerdaemon.Adapter, daemonPath string, original []byte) { + t.Helper() if err := os.MkdirAll(filepath.Dir(daemonPath), 0o700); err != nil { t.Fatal(err) } @@ -444,35 +511,6 @@ func TestDockerDaemonResetCleansAlreadyRestoredRecoveryWithoutDaemonWriteOrSyste if err := os.WriteFile(daemonPath, original, 0o640); err != nil { t.Fatal(err) } - record.events = nil - - reset := app.NewMaintenance(profile.State{Profiles: []profile.Profile{*activeProfile()}, ActiveProfile: activeProfile().Name}, []target.Target{a}, immediateLock{}, nil).Reset(context.Background(), true) - if reset.Err != nil || len(reset.Targets) != 1 || reset.Targets[0].Err != nil || reset.Targets[0].Inspection.State != target.External { - t.Fatalf("already-restored reset = %#v", reset) - } - for _, event := range record.events { - if event == "write daemon.json" || strings.Contains(event, "daemon-reload") || strings.Contains(event, " restart ") { - t.Fatalf("already-restored reset changed daemon or systemd state: %#v", record.events) - } - } - if !contains(record.events, "remove docker-daemon.json") { - t.Fatalf("already-restored reset did not clean recovery metadata: %#v", record.events) - } - if got, err := os.ReadFile(daemonPath); err != nil || !reflect.DeepEqual(got, original) { - t.Fatalf("already-restored daemon configuration changed to %q: %v", got, err) - } - if info, err := os.Stat(daemonPath); err != nil || info.Mode().Perm() != 0o640 { - t.Fatalf("already-restored daemon metadata changed: info=%v err=%v", info, err) - } - if _, err := os.Stat(recoveryPath); !errors.Is(err, os.ErrNotExist) { - t.Fatalf("recovery metadata remains: %v", err) - } - - record.events = nil - retry := a.SetDesired(context.Background(), false, nil) - if retry.Err != nil || retry.Inspection.State != target.External || len(mutationEvents(record.events)) != 0 { - t.Fatalf("already-restored cleanup retry = %#v events=%#v", retry, record.events) - } } func TestDockerDaemonPreservesAndRestoresExistingConfiguration(t *testing.T) {