diff --git a/.github/wiki/error-codes.md b/.github/wiki/error-codes.md index 90ca2329e..9285ccc50 100644 --- a/.github/wiki/error-codes.md +++ b/.github/wiki/error-codes.md @@ -265,6 +265,8 @@ Change-plan (E450) and operation-lock (E460) codes. The other codes in this bloc | E453 | 1 | Plan id cannot bind plugin changes | The plan installs or removes plugins, and what those change is only known after they run, so a plan id cannot describe the final render. | Run nself build --yes without --plan-id (the render after the plugin changes is printed and held), or install or remove the plugins first. | | E454 | 1 | Plan id cannot bind certificate or trust effects | Certificate bytes are generated at runtime and host trust state can change independently; artifact byte confirmation excludes these effects. | Run nself build and confirm at the prompt (or with --yes, without --plan-id), or provision certificates and host trust first. | | E455 | 1 | Stack does not match nself.lock.yaml | nself start --frozen found a service image, an installed plugin version or checksum, or the CLI minor that differs from the project lock, or the project has no lock. | Run nself build to regenerate the stack from the lock, or accept the change by running nself build --write-lock and committing the new nself.lock.yaml. | +| E456 | 1 | Update source unavailable | nself update --plan or --yes could not get an answer from the CLI release lookup or the plugin registry, so it cannot build a plan. | Retry in a few minutes. Nothing was changed. | +| E457 | 1 | Update source rate limited | The CLI release lookup or the plugin registry answered nself update --plan or --yes with HTTP 429. | Retry in a few minutes. Nothing was changed. | | E460 | 1 | Project operation lock held | Another nself command is changing this project and holds its operation lock. | Wait for the running nself command to finish, or stop it, then run this command again. | | E465 | 1 | Image unavailable | The locked image could not be pulled from its upstream or digest-identical mirror. | Run nself doctor images, then check registry access and the locked digest. | | E470 | 2 | ACME issuance failed | The ACME client could not obtain a certificate from the certificate authority. | Check that the names resolve to this server and that port 80 or the DNS credential is reachable, then run the command that failed again. A certificate that is already managed can be tested with nself config trust ssl renew --dry-run. | diff --git a/cmd/commands/deploy_apply_v15_test.go b/cmd/commands/deploy_apply_v15_test.go index 231b1c883..2949fad9d 100644 --- a/cmd/commands/deploy_apply_v15_test.go +++ b/cmd/commands/deploy_apply_v15_test.go @@ -645,6 +645,24 @@ func TestRollbackWithoutBackupHostTar(t *testing.T) { } } +// D-0541: a rollback whose record cannot be saved must say so, not exit 0. +func TestRollbackReportsUnsavedRecord(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root ignores directory permissions") + } + proj, _, _, _ := twoHostProd(t) + rollbackRecord(t, proj, "deploy@app.example.test", "deploy@app2.example.test") + dir := filepath.Dir(filepath.Join(proj, deploy.DeployStateFile)) + if err := os.Chmod(dir, 0o500); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(dir, 0o755) }) + handled, err := rollbackRemoteV15(yesCmd(), proj, "prod") + if !handled || err == nil || !strings.Contains(err.Error(), "rolled back but the record was not saved") { + t.Fatalf("handled=%v err=%v, want the unsaved record reported", handled, err) + } +} + const rollbackSignalEnv = "NSELF_DEPLOY_ROLLBACK_SIGNAL_HELPER" // TestRollbackInterruptHelper is the child process; it does nothing unless re-executed. diff --git a/cmd/commands/deploy_lifecycle_rollback.go b/cmd/commands/deploy_lifecycle_rollback.go index aea3ec7d5..04144e680 100644 --- a/cmd/commands/deploy_lifecycle_rollback.go +++ b/cmd/commands/deploy_lifecycle_rollback.go @@ -108,6 +108,8 @@ func rollbackRemoteV15(cmd *cobra.Command, workdir, target string) (bool, error) if !manualDB { st.BackupID = "" } - _ = deploy.SaveDeployState(workdir, st) + if err := deploy.SaveDeployState(workdir, st); err != nil { + return true, fmt.Errorf("deploy rollback: the hosts were rolled back but the record was not saved (%w); %s still shows this deploy as not rolled back, so a repeat rollback restores the files from the same tar again", err, deploy.DeployStateFile) + } return true, nil } diff --git a/cmd/commands/ssl_certbot_hooks.go b/cmd/commands/ssl_certbot_hooks.go index 766834318..15f8afcef 100644 --- a/cmd/commands/ssl_certbot_hooks.go +++ b/cmd/commands/ssl_certbot_hooks.go @@ -15,6 +15,7 @@ import ( "errors" "fmt" "os" + "os/user" "path/filepath" "runtime" "slices" @@ -76,9 +77,65 @@ func certbotHookFiles(dir string) (confs, scripts []string, err error) { return confs, scripts, nil } +// certbotUserConfigs lists the per-user cli.ini files certbot reads besides +// /cli.ini: own is $XDG_CONFIG_HOME/letsencrypt and ~/.config/letsencrypt of +// the user running nself, root is root's (a root certbot scheduler reads that +// one). Missing ones are harmless. A variable so tests do not read the real home. +var certbotUserConfigs = func() (own, root []string) { + add := func(list *[]string, base string) { + if p := filepath.Join(base, "letsencrypt", "cli.ini"); !slices.Contains(own, p) && !slices.Contains(root, p) { + *list = append(*list, p) + } + } + if x := os.Getenv("XDG_CONFIG_HOME"); filepath.IsAbs(x) { + add(&own, x) + } + if h, err := os.UserHomeDir(); err == nil { + add(&own, filepath.Join(h, ".config")) + } + if u, err := user.Lookup("root"); err == nil && u.HomeDir != "" { + add(&root, filepath.Join(u.HomeDir, ".config")) + } + return own, root +} + +// userCertbotConfs returns the per-user cli.ini files that exist. Only absence +// is tolerated: a denied stat (a non-root caller and root's home) is returned, +// so the caller refuses; root's cli.ini may hold the hook a root certbot +// scheduler replays and cannot be shown clean. +func userCertbotConfs() ([]string, error) { + var found []string + own, root := certbotUserConfigs() + for _, p := range append(slices.Clone(own), root...) { + st, err := os.Stat(p) + switch { + case errors.Is(err, os.ErrNotExist): + case err != nil: + return nil, err + case st.Mode().IsRegular(): + found = append(found, p) + } + } + return found, nil +} + // certbotHooks lists the certbot hooks under dir that name one of sslDirs. -// A missing tree has none. +// A missing tree has none. The per-user cli.ini files (certbotUserConfigs) are +// scanned too, whatever dir is (D-0595). func certbotHooks(dir string, sslDirs ...string) ([]string, error) { + confs, scripts, err := certbotHookFiles(dir) + if err != nil { + return nil, err + } + perUser, err := userCertbotConfs() + if err != nil { + return nil, err + } + for _, p := range perUser { + if !slices.Contains(confs, p) { + confs = append(confs, p) + } + } var needles []string for _, d := range sslDirs { needles = append(needles, filepath.Clean(d)) @@ -86,10 +143,6 @@ func certbotHooks(dir string, sslDirs ...string) ([]string, error) { needles = append(needles, real) } } - confs, scripts, err := certbotHookFiles(dir) - if err != nil { - return nil, err - } var hits []string for _, p := range append(confs, scripts...) { data, err := os.ReadFile(p) //nolint:gosec // certbot's tree, read only diff --git a/cmd/commands/ssl_certbot_retire_test.go b/cmd/commands/ssl_certbot_retire_test.go index 4b7ecb8bf..d9af356c1 100644 --- a/cmd/commands/ssl_certbot_retire_test.go +++ b/cmd/commands/ssl_certbot_retire_test.go @@ -14,6 +14,7 @@ import ( "os" "path/filepath" "runtime" + "slices" "strings" "testing" ) @@ -26,6 +27,7 @@ var realSystemctlRun = systemctlRun func init() { systemctlRun = func(context.Context, ...string) (string, error) { return "", errNoSystemctl } certbotDir = filepath.Join(os.TempDir(), "nself-test-no-letsencrypt") + certbotUserConfigs = func() ([]string, []string) { return nil, nil } // never the real home } // useCertbotDir points certbotDir at a fresh tree for the test. @@ -292,3 +294,59 @@ func TestRetireCertbotSystemdDownHint(t *testing.T) { t.Fatalf("no systemctl: err = %v", err) } } + +// D-0595: a copy hook in the per-user cli.ini (XDG_CONFIG_HOME, the caller's +// ~/.config, root's ~/.config) is found; the scan names the file and line. +func TestCertbotHooksPerUserConfig(t *testing.T) { + sslDir := filepath.Join(t.TempDir(), "proj", "ssl") + le := t.TempDir() // an empty certbot tree: only the per-user files can hit + xdg, home, rootHome := t.TempDir(), t.TempDir(), t.TempDir() + conf := func(base string) string { return filepath.Join(base, "letsencrypt", "cli.ini") } + old := certbotUserConfigs + t.Cleanup(func() { certbotUserConfigs = old }) + certbotUserConfigs = func() ([]string, []string) { + return []string{conf(xdg), conf(filepath.Join(home, ".config"))}, []string{conf(filepath.Join(rootHome, ".config"))} + } + if hits, err := certbotHooks(le, sslDir); err != nil || len(hits) != 0 { + t.Fatalf("no per-user files: hits=%v err=%v", hits, err) + } + for _, p := range []string{conf(xdg), conf(filepath.Join(home, ".config")), conf(filepath.Join(rootHome, ".config"))} { + acmeWriteFile(t, p, []byte("email = a@example.test\ndeploy-hook = cp x "+sslDir+"/\n")) + hits, err := certbotHooks(le, sslDir) + if want := p + ":2"; err != nil || !slices.Contains(hits, want) { + t.Fatalf("%s: hits=%v err=%v, want %s", p, hits, err, want) + } + if err := os.Remove(p); err != nil { + t.Fatal(err) + } + } + // A per-user hook that names another dir is not a hit. + acmeWriteFile(t, conf(xdg), []byte("deploy-hook = cp x /srv/other/ssl/\n")) + if hits, err := certbotHooks(le, sslDir); err != nil || len(hits) != 0 { + t.Fatalf("unrelated hook: hits=%v err=%v", hits, err) + } +} + +// D-0595: root's cli.ini may hold the copy hook a root scheduler replays. A +// caller that may not enter root's home cannot show it clean, so the scan +// returns the permission error (the caller refuses with E151) instead of +// skipping the file. +func TestCertbotHooksRootConfigDenied(t *testing.T) { + if os.Geteuid() == 0 || runtime.GOOS == "windows" { + t.Skip("permission bits do not deny root or windows") + } + sslDir := filepath.Join(t.TempDir(), "proj", "ssl") + rootHome := t.TempDir() + cfg := filepath.Join(rootHome, ".config", "letsencrypt", "cli.ini") + acmeWriteFile(t, cfg, []byte("deploy-hook = cp x "+sslDir+"/\n")) + if err := os.Chmod(rootHome, 0); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(rootHome, 0o700) }) + old := certbotUserConfigs + t.Cleanup(func() { certbotUserConfigs = old }) + certbotUserConfigs = func() ([]string, []string) { return nil, []string{cfg} } + if hits, err := certbotHooks(t.TempDir(), sslDir); !errors.Is(err, os.ErrPermission) { + t.Fatalf("denied root cli.ini: hits=%v err=%v, want permission error", hits, err) + } +} diff --git a/cmd/commands/update_plan.go b/cmd/commands/update_plan.go index cb8cc7f9d..876a55529 100644 --- a/cmd/commands/update_plan.go +++ b/cmd/commands/update_plan.go @@ -9,8 +9,8 @@ package commands // Inputs: the project's nself.lock.yaml, the release lookup (GitHub), the // plugin registry, and the flags --plan, --yes, --cli, --version, --json. // Outputs: a table and step list, or the v1 envelope with UpdateResult. -// Constraints: a source that cannot be reached (including HTTP 429) is a coded -// error, never an empty plan; a pin the source no longer offers is a row with +// Constraints: a source that cannot be reached is a coded error (E456; HTTP 429 +// is E457, update_source_failure.go), never an empty plan; a pin the source no longer offers is a row with // resolvable=false. The sources are reached through updateDeps so tests run on // fixtures. The steps that need a running stack belong to `update stack`. @@ -19,7 +19,6 @@ import ( "fmt" "io" "os" - "regexp" "strings" "github.com/nself-org/cli/internal/errs" @@ -66,19 +65,6 @@ func runUpdatePlanMode(cmd *cobra.Command) (handled bool, err error) { return true, runUpdateYes(cmd, newUpdateDeps()) } -// httpStatusRE pulls the status out of fetchReleaseFromURL's error text. -var httpStatusRE = regexp.MustCompile(`status (\d{3})`) - -// sourceFailure codes a source that could not answer. 429 gets its own words. -func sourceFailure(source string, err error) error { - why := "The source did not answer: " + err.Error() - if m := httpStatusRE.FindStringSubmatch(err.Error()); m != nil && m[1] == "429" { - why = "The source rate-limited this request (HTTP 429)." - } - return errs.Wrap("E400", "update plan: "+source+" is unavailable", err). - WithWhy(why).WithFix("Retry in a few minutes. Nothing was changed.") -} - // loadProjectLock returns the project dir and its lock. func loadProjectLock() (string, lock.Lock, error) { dir, err := resolveProjectDir() diff --git a/cmd/commands/update_plan_hasura.go b/cmd/commands/update_plan_hasura.go index ee24f9fa9..3e49a9b4e 100644 --- a/cmd/commands/update_plan_hasura.go +++ b/cmd/commands/update_plan_hasura.go @@ -41,19 +41,22 @@ func hasuraCandidate(pin string) plan.Candidate { // hasuraPinOrder compares a hasura pin with the candidate entry: >0 when the pin is a // newer release, <0 when older, 0 when equal. known is false for a pin without a // parseable tag whose digest is neither lock entry's (the lock cannot name the release). -// A digest-only pin of an entry orders as that entry's release. +// A pin's digest is what the daemon runs, so a digest one of the entries owns +// orders as that entry's release, whatever its tag says (a hand-edited +// tag@digest pin that disagrees must not defeat the downgrade guard, D-1154); +// the tag decides only when the digest is absent or not in the lock. func hasuraPinOrder(pin string, cand, v14, cur compose.Ref) (order int, known bool) { base, digest, _ := strings.Cut(pin, "@") - if i := strings.LastIndex(base, ":"); i > strings.LastIndex(base, "/") { - if c, ok := compareHasuraVersion(base[i+1:], cand.Version); ok { - return c, true - } - } for _, e := range []compose.Ref{v14, cur} { if digest != "" && digest == e.IndexDigest { return compareHasuraVersion(e.Version, cand.Version) } } + if i := strings.LastIndex(base, ":"); i > strings.LastIndex(base, "/") { + if c, ok := compareHasuraVersion(base[i+1:], cand.Version); ok { + return c, true + } + } return 0, false } diff --git a/cmd/commands/update_plan_hasura_test.go b/cmd/commands/update_plan_hasura_test.go index 921e53e3f..2957b587a 100644 --- a/cmd/commands/update_plan_hasura_test.go +++ b/cmd/commands/update_plan_hasura_test.go @@ -37,6 +37,9 @@ func TestUpdatePlanHasuraRow(t *testing.T) { {"v1.5 digest-only v2.44.0 pin moves with a one-way note", true, "docker.io/hasura/graphql-engine@" + v14.IndexDigest, cur.String(), "one-way"}, {"v1.4 digest-only v2.44.0 pin stays", false, "docker.io/hasura/graphql-engine@" + v14.IndexDigest, v14.String(), ""}, {"v1.5 unknown digest-only pin is left as is", true, "docker.io/hasura/graphql-engine@sha256:" + strings.Repeat("cd", 32), "", "does not know"}, + {"v1.4 trusts the v2.51.0 digest over a v2.44.0 tag", false, "docker.io/hasura/graphql-engine:" + v14.Version + "@" + cur.IndexDigest, "", "backup restored"}, + {"v1.5 trusts the v2.44.0 digest over a v2.51.0 tag", true, "docker.io/hasura/graphql-engine:" + cur.Version + "@" + v14.IndexDigest, cur.String(), "one-way"}, + {"v1.5 trusts the tag when the digest is unknown", true, newer, "", "backup restored"}, {"v1.4 unknown digest-only pin is left as is", false, "docker.io/hasura/graphql-engine@sha256:" + strings.Repeat("cd", 32), "", "does not know"}, } { t.Run(tc.name, func(t *testing.T) { diff --git a/cmd/commands/update_plan_sources.go b/cmd/commands/update_plan_sources.go index 78e3edbaa..9ca477895 100644 --- a/cmd/commands/update_plan_sources.go +++ b/cmd/commands/update_plan_sources.go @@ -40,7 +40,7 @@ func gatherPlan(ctx context.Context, d updateDeps, l lock.Lock, target string) ( switch { case err == nil: c.CLI = plan.Candidate{Value: tag} - case strings.Contains(err.Error(), "status 404"): + case httpStatusOf(err) == http.StatusNotFound: c.CLI = plan.Candidate{Reason: "no such release (HTTP 404)"} default: return plan.Plan{}, sourceFailure("the CLI release lookup", err) diff --git a/cmd/commands/update_plan_test.go b/cmd/commands/update_plan_test.go index ff68d008e..bb7053dae 100644 --- a/cmd/commands/update_plan_test.go +++ b/cmd/commands/update_plan_test.go @@ -282,7 +282,7 @@ func TestUpdatePlanRateLimited(t *testing.T) { if err == nil { t.Fatalf("a 429 produced a plan instead of an error:\n%s", out) } - if d := errs.Describe(err); d == nil || d.Code != "E400" || errs.ExitCodeFor(err) == 0 { + if d := errs.Describe(err); d == nil || d.Code != "E457" || errs.ExitCodeFor(err) == 0 { t.Fatalf("error is not coded and non-zero: %v (%+v)", err, d) } if !strings.Contains(err.Error(), "429") && !strings.Contains(fmt.Sprint(errs.Describe(err)), "429") { @@ -302,8 +302,8 @@ func TestUpdatePlanRateLimited(t *testing.T) { return d } t.Cleanup(func() { newUpdateDeps = old }) - if _, _, err := runUpdate(t, "--plan"); err == nil || errs.Describe(err).Code != "E400" { - t.Fatalf("registry failure = %v, want E400", err) + if _, _, err := runUpdate(t, "--plan"); err == nil || errs.Describe(err).Code != "E457" { + t.Fatalf("registry failure = %v, want E457", err) } } @@ -448,3 +448,27 @@ func TestUpdatePlanRealPathIsReadOnly(t *testing.T) { t.Fatal("update --plan created or changed a project file") } } + +// D-0560: the code follows the typed status, not the words of the error text. +func TestUpdatePlanSourceFailureCodes(t *testing.T) { + for _, tc := range []struct { + name string + err error + code string + }{ + {"typed 429", &releaseStatusError{Status: 429}, "E457"}, + {"wrapped typed 429", fmt.Errorf("lookup: %w", &releaseStatusError{Status: 429}), "E457"}, + {"typed 500", &releaseStatusError{Status: 500}, "E456"}, + {"typed 403 whose wording mentions 429", fmt.Errorf("x: %w (was status 429 once)", &releaseStatusError{Status: 403}), "E456"}, + {"no status", errors.New("connection refused"), "E456"}, + } { + t.Run(tc.name, func(t *testing.T) { + if d := errs.Describe(sourceFailure("the CLI release lookup", tc.err)); d == nil || d.Code != tc.code { + t.Fatalf("code = %+v, want %s", d, tc.code) + } + }) + } + if got := httpStatusOf(&releaseStatusError{Status: 404}); got != 404 { + t.Fatalf("httpStatusOf = %d, want 404", got) + } +} diff --git a/cmd/commands/update_release_api.go b/cmd/commands/update_release_api.go index f9e7f9cfc..be6cec0b3 100644 --- a/cmd/commands/update_release_api.go +++ b/cmd/commands/update_release_api.go @@ -24,6 +24,14 @@ import ( "github.com/nself-org/cli/internal/ui" ) +// releaseStatusError is a release lookup the API answered with a non-200 status. +// Callers read Status with errors.As; the text stays what it always was (D-0560). +type releaseStatusError struct{ Status int } + +func (e *releaseStatusError) Error() string { + return fmt.Sprintf("GitHub API returned status %d", e.Status) +} + func fetchLatestRelease() (tag string, url string, err error) { return fetchReleaseFromURL(githubLatestReleaseURL) } @@ -43,7 +51,7 @@ func fetchReleaseFromURL(apiURL string) (tag string, htmlURL string, err error) defer func() { _ = resp.Body.Close() }() if resp.StatusCode != http.StatusOK { - return "", "", fmt.Errorf("GitHub API returned status %d", resp.StatusCode) + return "", "", &releaseStatusError{Status: resp.StatusCode} } body, err := io.ReadAll(resp.Body) diff --git a/cmd/commands/update_source_failure.go b/cmd/commands/update_source_failure.go new file mode 100644 index 000000000..e5a865171 --- /dev/null +++ b/cmd/commands/update_source_failure.go @@ -0,0 +1,48 @@ +package commands + +// Purpose: how `nself update --plan|--yes` codes a source that could not answer +// (D-0560): the HTTP status is read from the typed releaseStatusError, not from +// the words of an error, so a rate limit and a plain outage have their own codes. +// Inputs: the error a source returned. +// Outputs: an E456 (unavailable) or E457 (rate limited) coded error. +// Constraints: only the plugin registry fetch still words its status as text, so +// that is the one place the text fallback applies. + +import ( + "errors" + "net/http" + "regexp" + "strconv" + + "github.com/nself-org/cli/internal/errs" +) + +// httpStatusRE pulls the status out of an untyped error's text: only the plugin +// registry fetch still words its status that way. +var httpStatusRE = regexp.MustCompile(`status (\d{3})`) + +// httpStatusOf is the HTTP status a source answered with, 0 when it did not +// answer with one. The release lookup carries it typed; other errors fall back +// to their text. +func httpStatusOf(err error) int { + var se *releaseStatusError + if errors.As(err, &se) { + return se.Status + } + if m := httpStatusRE.FindStringSubmatch(err.Error()); m != nil { + n, _ := strconv.Atoi(m[1]) + return n + } + return 0 +} + +// sourceFailure codes a source that could not answer: E457 for a rate limit +// (HTTP 429), E456 for anything else. +func sourceFailure(source string, err error) error { + if httpStatusOf(err) == http.StatusTooManyRequests { + return errs.Wrap("E457", "update plan: "+source+" is rate limited", err). + WithWhy("The source rate-limited this request (HTTP 429).").WithFix("Retry in a few minutes. Nothing was changed.") + } + return errs.Wrap("E456", "update plan: "+source+" is unavailable", err). + WithWhy("The source did not answer: " + err.Error()).WithFix("Retry in a few minutes. Nothing was changed.") +} diff --git a/internal/errs/codes_lock.go b/internal/errs/codes_lock.go index 5006e3e30..ffd687f3a 100644 --- a/internal/errs/codes_lock.go +++ b/internal/errs/codes_lock.go @@ -1,6 +1,6 @@ -// Registry fragment: project lock codes (E455-E459, P7-LIVE-09). E456-E459 are -// reserved for this Ticket's owner Epic and stay unregistered until a producer -// exists. Registered from init() through Register; see codes.go for the rules +// Registry fragment: project lock codes (E455-E459, P7-LIVE-09). E456 and E457 +// are the update-plan source failures (D-0560); E458-E459 are reserved for this +// Ticket's owner Epic and stay unregistered until a producer exists. Registered from init() through Register; see codes.go for the rules // and codes_blocks.go for the allocation. package errs @@ -15,5 +15,23 @@ func init() { DocsPath: "reference/error-codes#e455", Exit: 1, }, + CodeEntry{ + Code: "E456", + Category: "reconcile", + Summary: "Update source unavailable", + DefaultWhy: "nself update --plan or --yes could not get an answer from the CLI release lookup or the plugin registry, so it cannot build a plan.", + DefaultFix: "Retry in a few minutes. Nothing was changed.", + DocsPath: "reference/error-codes#e456", + Exit: 1, + }, + CodeEntry{ + Code: "E457", + Category: "reconcile", + Summary: "Update source rate limited", + DefaultWhy: "The CLI release lookup or the plugin registry answered nself update --plan or --yes with HTTP 429.", + DefaultFix: "Retry in a few minutes. Nothing was changed.", + DocsPath: "reference/error-codes#e457", + Exit: 1, + }, ) }