diff --git a/internal/apply/apply.go b/internal/apply/apply.go index f3cd989..2e64757 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -135,11 +135,16 @@ type Error struct { // line is what somebody reads. Others int - // Gated is true when what failed was an action, so nothing after it was attempted. A person - // reading a report needs to know the difference between "these things failed" and "these + // Gated is true when what failed was a step (an action, or a run-once container or process). A + // person reading a report needs to know the difference between "these things failed" and "these // things failed and the rest was never tried". Gated bool + // Stopped is true when that step belongs to no module, so it gated the whole machine and + // nothing after it was attempted. A module's step gates only its module (novox/hq ADR 0136), + // which Modules names. + Stopped bool + // Modules are the modules whose rest was not attempted, behind a step that did not complete // (novox/hq ADR 0136) or a directory the engine refused (issue 495). Named, because "everything // was attempted" is then untrue, and a report that says so after skipping a module lies. @@ -153,17 +158,40 @@ func (e *Error) Error() string { } else if e.Others > 1 { also = fmt.Sprintf(", and %d other resources also failed", e.Others) } - rest := "everything was attempted, so what is not listed as failed was done." + // Gated with no module named is an Error made before Stopped existed: a step that stopped the machine. + stopped := e.Stopped || (e.Gated && len(e.Modules) == 0) + // What the apply did not attempt, said whole (novox/hq issue 495): a module gated earlier in an apply + // that a machine-wide step then stopped is both, and saying either alone is untrue. + gatedRest := "" if len(e.Modules) > 0 { - rest = fmt.Sprintf("the rest of %s was not attempted and is listed as skipped; everything else "+ - "was attempted, so what is not listed as failed or skipped was done.", strings.Join(e.Modules, ", ")) - } else if e.Gated { - // An action is a gate: it exists to make something true before the next thing needs it. - rest = "this is an action, so nothing after it was attempted — the machine is in " + - "whatever state that left it." + gatedRest = fmt.Sprintf("the rest of %s was not attempted and is listed as skipped", strings.Join(e.Modules, ", ")) } - return fmt.Sprintf("applying %q: %v%s\n\n%d resource(s) were applied and remain; %s", - e.Resource, e.Err, also, len(e.Done.Outcomes), rest) + rest := "everything was attempted, so what is not listed as failed was done." + switch { + case stopped && gatedRest != "": + rest = gatedRest + "; and this step gates the whole machine, so nothing after it was attempted — " + + "the machine is in whatever state that left it." + case stopped: + // An action is a gate: it exists to make something true before the next thing needs it. + rest = "this step gates the whole machine, so nothing after it was attempted — the machine is in " + + "whatever state that left it." + case gatedRest != "": + rest = gatedRest + "; everything else was attempted, so what is not listed as failed or skipped was done." + } + // Skipped is not applied: counted apart, so the count says what is on the machine. + applied, skipped := 0, 0 + for _, o := range e.Done.Outcomes { + if o.Action == "skipped" { + skipped++ + } else { + applied++ + } + } + count := fmt.Sprintf("%d resource(s) were applied and remain", applied) + if skipped > 0 { + count += fmt.Sprintf(", %d were skipped", skipped) + } + return fmt.Sprintf("applying %q: %v%s\n\n%s; %s", e.Resource, e.Err, also, count, rest) } func (e *Error) Unwrap() error { return e.Err } @@ -746,6 +774,7 @@ func ApplyMindingWindows( "of it was not attempted", module)) continue } + failed.Stopped = true failed.Done = report failed.Others = len(failures) - 1 failed.Modules = gatedModules() diff --git a/internal/apply/refused_directory_test.go b/internal/apply/refused_directory_test.go index 13065ff..997cf28 100644 --- a/internal/apply/refused_directory_test.go +++ b/internal/apply/refused_directory_test.go @@ -3,6 +3,8 @@ package apply import ( "context" "errors" + "net/http" + "net/http/httptest" "os" "path/filepath" "strings" @@ -116,7 +118,105 @@ func TestAFileIsJudgedByTheSameGuardAsADirectory(t *testing.T) { if _, statErr := os.Stat(filepath.Join(home, "data", "notes.conf")); statErr == nil { t.Error("a file was written below a directory the engine refused") } - if o := outcomeOf(report, "notes.conf"); o.ID != "" && o.Action != "skipped" { - t.Errorf("the file below a refused directory is reported %q", o.Action) + // A refused file has no outcome: it is a failure. Declared first, it is the failure the apply reports, + // and the directory the other. + var applyErr *Error + if !errors.As(err, &applyErr) || applyErr.Resource != "notes.conf" || applyErr.Others != 1 { + t.Errorf("the file below a refused directory is not among the failures: %v", err) + } + if o := outcomeOf(report, "notes.conf"); o.ID != "" { + t.Errorf("a refused file has an outcome as if applied: %+v", o) + } +} + +// An archive below a refused path is refused with it, whichever module declares it and in whatever order. +func TestAnArchiveBelowARefusedPathIsRefused(t *testing.T) { + dir := t.TempDir() + home := filepath.Join(dir, "home", "alice") + if err := os.MkdirAll(home, 0o755); err != nil { + t.Fatal(err) + } + passwd := filepath.Join(dir, "passwd") + if err := os.WriteFile(passwd, []byte("alice:x:4242:4242::"+home+":/bin/sh\n"), 0o644); err != nil { + t.Fatal(err) + } + before := passwdFile + passwdFile = passwd + t.Cleanup(func() { passwdFile = before }) + body, digest := anArchive(t, map[string]string{"theme.css": "x\n"}) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write(body) })) + t.Cleanup(server.Close) + + _, _, err := applyOnce(t, + `{"id":"theme.files","type":"archive","path":"`+home+`/data/theme","source":"`+server.URL+`/t.tar.gz",`+ + `"digest":"`+digest+`"},`+ + `{"id":"notes.data","type":"directory","path":"`+home+`/data"}`, nil, store.State{}) + refusedFor339(t, "an archive below a refused directory", err) + var applyErr *Error + if !errors.As(err, &applyErr) || applyErr.Resource != "theme.files" || applyErr.Others != 1 { + t.Errorf("the archive below a refused directory is not among the failures: %v", err) + } + if _, statErr := os.Stat(filepath.Join(home, "data")); statErr == nil { + t.Error("an archive was unpacked below a directory the engine refused") + } +} + +// Gated is per apply: a module gated behind its refused directory is tried again by the next apply, gated +// again while the directory is refused, and applied whole once it is not. +func TestAGatedModuleIsTriedAgainByTheNextApply(t *testing.T) { + root := aRuntimeRoot(t) + b := newStepBed(t) + declared := func() string { + return `{"id":"mod.state","type":"directory","path":"` + root + `"},` + + `{"id":"mod.rotation","type":"file","path":"` + root + `/log-rotation.conf","content":"max-size=10m\n"},` + + `{"id":"mod.check","type":"process","name":"mod-check","source":"` + b.body + `","digest":"` + b.digest + `",` + + `"run":["./check"],"run-once":true,"restart-on":["mod.rotation"]}` + } + apply := func(known store.State) (Report, store.State, error) { + return Apply(context.Background(), archHost(t), declare(t, declared()), known, store.OriginDeclared, b.run, nil, nil) + } + report, known, err := apply(store.State{}) + if err == nil || !skippedIn(report, "mod.check") || b.starts != 0 { + t.Fatalf("apply 1: err %v, skipped %v, starts %d", err, skippedIn(report, "mod.check"), b.starts) + } + report, known, err = apply(known) + if err == nil || !skippedIn(report, "mod.check") || b.starts != 0 { + t.Fatalf("apply 2, still refused: err %v, skipped %v, starts %d", err, skippedIn(report, "mod.check"), b.starts) + } + // The runtime moves its data elsewhere: the directory is no longer refused, and the module is applied. + elsewhere := filepath.Join(t.TempDir(), "docker") + if err := os.WriteFile(runtimeConfigs.daemonJSON, []byte(`{"data-root":"`+elsewhere+`"}`), 0o644); err != nil { + t.Fatal(err) + } + report, _, err = apply(known) + if err != nil || skippedIn(report, "mod.check") || b.starts != 1 { + t.Fatalf("apply 3, no longer refused: err %v, skipped %v, starts %d", err, skippedIn(report, "mod.check"), b.starts) + } + if _, statErr := os.Stat(filepath.Join(root, "log-rotation.conf")); statErr != nil { + t.Errorf("the module's file was not written once its directory was placed: %v", statErr) + } +} + +// A module gated behind its refused directory, then a step of no module that stops the machine: the report +// names both, and says nothing after the step was attempted (novox/hq issue 495). +func TestAMachineStopAfterAGatedModuleSaysBoth(t *testing.T) { + root := aRuntimeRoot(t) + after := filepath.Join(t.TempDir(), "after.conf") + _, _, err := Apply(context.Background(), archHost(t), parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"mod.state","type":"directory","path":"`+root+`"}, + {"id":"gate","type":"action","command":["false"],"verify":["false"]}, + {"id":"other.after","type":"file","path":"`+after+`","content":"b\n"} + ]}`), store.State{}, store.OriginCarried, ExecRunner, nil, nil) + var applyErr *Error + if !errors.As(err, &applyErr) || !applyErr.Stopped { + t.Fatalf("the failed action did not stop the apply: %v", err) + } + if _, statErr := os.Stat(after); statErr == nil { + t.Error("the apply continued past a failed action") + } + msg := err.Error() + if !strings.Contains(msg, "the rest of mod") || !strings.Contains(msg, "nothing after it was attempted") || + strings.Contains(msg, "everything else was attempted") { + t.Errorf("the report does not say both what was gated and that the machine stopped: %s", msg) } }