diff --git a/.github/wiki/Multi-App-Isolation.md b/.github/wiki/Multi-App-Isolation.md index 8b64bf5e2..4dea13bd0 100644 --- a/.github/wiki/Multi-App-Isolation.md +++ b/.github/wiki/Multi-App-Isolation.md @@ -9,7 +9,7 @@ nself init --template multi-app . --force # overlay the template files The overlay replaces the `.env.example` and `nself.yml` that `nself init` wrote with the template's own. Keep your own values in `.env` and `.env.local`. -`--dry-run` lists what it would write: `README.md`, `manifest.json`, `nself.yml`, `.env.example`, `migrations/001_multi_app.sql`, `hasura/metadata.json` and `hasura/metadata/databases/default/tables/tables.yaml` (the layout `nself db reconcile` reads; both hold the same tables). No plugins are required, and there is no UI starter. +`--dry-run` lists what it would write: `README.md`, `manifest.json`, `nself.yml`, `.env.example`, `migrations/001_multi_app.sql` and `hasura/metadata/databases/default/tables/tables.yaml` (the layout `nself db reconcile` reads). No plugins are required, and there is no UI starter. ## The model diff --git a/.github/wiki/Shared-Proxy.md b/.github/wiki/Shared-Proxy.md index 81df57653..0879090f8 100644 --- a/.github/wiki/Shared-Proxy.md +++ b/.github/wiki/Shared-Proxy.md @@ -61,8 +61,8 @@ A fronted project publishes no port 80 or 443 and runs no Nginx of its own. The |---|---| | `nself start` in a fronted project | After the stack is healthy: connect the owner's Nginx to `_network` (no change when already connected), run `nginx -t`, then `nginx -s reload`. | | `nself start` in the owner | Does the same for every registration whose network exists. A registered project that is stopped gives one warning per project; the owner still starts. | -| `nself stop`, `restart`, `start --fresh`, `reset`, `uninstall` in a fronted project | Before compose takes the project down, the owner's Nginx is disconnected from `_network`. Compose cannot remove a network while a foreign container is attached, so without this step `nself stop` would fail. | -| `nself uninstall` in a fronted project | Also removes the project's files and registration from the owner and reloads the owner's Nginx. If that cannot be done (for example a registration cannot be read, `nginx/shared/quarantine` or a certificate directory is a symlink, or the owner's Nginx cannot be looked up), uninstall returns `E506` and does not report the files as removed. | +| `nself stop`, `restart`, `start --fresh`, `reset`, `uninstall` in a fronted project | Before compose takes the project down, the owner's Nginx is disconnected from `_network`. Compose cannot remove a network while a foreign container is attached, so without this step `nself stop` would fail. The owner lock is held from the disconnect until compose has finished, so an owner `nself start` cannot reattach the network in between. | +| `nself uninstall` in a fronted project | Also removes the project's files and registration from the owner and reloads the owner's Nginx. If that cannot be done (for example a registration cannot be read, `nginx/shared/quarantine` or a certificate directory is a symlink, or the owner's Nginx cannot be looked up), uninstall returns `E506` and does not report the files as removed. If only the reload failed, the debt is recorded in `nginx/shared/.reload-pending`; run `nself uninstall` again or `nself start` in the owner and the reload is retried. | A fronted `nself start` returns `E506` when the registrations under the owner cannot be read (one invalid registration file of any project is enough), because its domains would not be served. The owner's own start only warns. Before a compose down, the same condition is a warning that names the cause; the down continues. @@ -72,7 +72,7 @@ If the owner's Nginx is not running, the fronted project starts and stops anyway ### Restarting the owner's Nginx -A restarted or recreated Nginx container loses its extra networks. Run `nself start` in the owner afterwards: it reattaches every registered, running project. +A restarted or recreated Nginx container loses its extra networks. `nself restart nginx` in the owner reattaches every registered, running project once the container is back; for a container you recreated by other means, run `nself start` in the owner. If a start attach fails after it stopped a crash-looping Nginx, the owner's Nginx is started again. ### A failed `nginx -t` (E506) diff --git a/cmd/commands/debt_d0353_test.go b/cmd/commands/debt_d0353_test.go index ff058e6f4..b88fe1efc 100644 --- a/cmd/commands/debt_d0353_test.go +++ b/cmd/commands/debt_d0353_test.go @@ -107,7 +107,7 @@ func TestD0353ComposeCascadeDoesNotChangeHash(t *testing.T) { } _ = os.Setenv("D0353_VALUE", "dev-conflict") if operation == "restart" { - err = restartServices(context.Background(), comp, dir, []string{"hasura"}, false) + err = restartServices(context.Background(), comp, dir, nil, []string{"hasura"}, false) } else { err = startRemainingServicesPhase(context.Background(), comp, dir, cfg) } diff --git a/cmd/commands/restart.go b/cmd/commands/restart.go index e91b67d4d..8ee1cbbf9 100644 --- a/cmd/commands/restart.go +++ b/cmd/commands/restart.go @@ -140,7 +140,7 @@ func runRestartFlow(cmd *cobra.Command, args []string) error { } } else if len(services) > 0 { // Specific services: restart only those - if err := restartServices(ctx, comp, workdir, services, verbose); err != nil { + if err := restartServices(ctx, comp, workdir, cfg, services, verbose); err != nil { return err } } else if smart { diff --git a/cmd/commands/restart_strategies.go b/cmd/commands/restart_strategies.go index 67bb499b6..524f5971e 100644 --- a/cmd/commands/restart_strategies.go +++ b/cmd/commands/restart_strategies.go @@ -21,6 +21,7 @@ import ( "fmt" "os" "path/filepath" + "slices" "time" "github.com/nself-org/cli/internal/build" @@ -76,7 +77,7 @@ func restartFull(ctx context.Context, comp *docker.Compose, workdir string, cfg } // restartServices restarts only the specified services. -func restartServices(ctx context.Context, comp *docker.Compose, workdir string, services []string, verbose bool) error { +func restartServices(ctx context.Context, comp *docker.Compose, workdir string, cfg *config.Config, services []string, verbose bool) error { if verbose { ui.Info(fmt.Sprintf("Restarting services: %v", services)) } @@ -101,6 +102,11 @@ func restartServices(ctx context.Context, comp *docker.Compose, workdir string, } ui.Success(fmt.Sprintf("Restarted: %v", services)) + // A recreated nginx container loses the networks the shared proxy attached + // to it (D-0931): attach again, as `nself start` does. + if slices.Contains(services, "nginx") { + return afterStartSharedProxy(ctx, cfg, workdir) + } return nil } diff --git a/cmd/commands/start_shared_proxy.go b/cmd/commands/start_shared_proxy.go index 9e3e9a65d..d0c0c411a 100644 --- a/cmd/commands/start_shared_proxy.go +++ b/cmd/commands/start_shared_proxy.go @@ -24,6 +24,7 @@ import ( "context" "errors" "fmt" + "path/filepath" "slices" "github.com/nself-org/cli/internal/config" @@ -36,8 +37,9 @@ import ( var ( spLocate = sharedproxy.Locate spAttach = sharedproxy.Attach - spDetach = sharedproxy.Detach + spDetach = sharedproxy.DetachHolding spLeave = sharedproxy.Leave + spFlush = sharedproxy.ReloadIfPending ) func init() { @@ -61,6 +63,7 @@ func afterStartSharedProxy(ctx context.Context, cfg *config.Config, workdir stri } switch site.Role { case sharedproxy.RoleOwner: + flushPendingReload(ctx, workdir) attachAllRegistered(ctx, workdir, site) return nil case sharedproxy.RoleFronted: @@ -72,6 +75,14 @@ func afterStartSharedProxy(ctx context.Context, cfg *config.Config, workdir stri return nil } +// flushPendingReload pays the owner reload that a fronted project's failed +// uninstall left owed (D-0931). A failure is a warning: the owner still starts. +func flushPendingReload(ctx context.Context, ownerDir string) { + if _, err := spFlush(ctx, ownerDir); err != nil { + ui.Warn(fmt.Sprintf("Shared proxy: a removed project's files may still be served; the owner's nginx could not be reloaded: %v", err)) + } +} + func attachAllRegistered(ctx context.Context, ownerDir string, site sharedproxy.Site) { if len(site.Set.Projects) == 0 { return @@ -111,22 +122,26 @@ func warnOutcome(out sharedproxy.Outcome, err error) { // beforeStopSharedProxy is the compose pre-down hook: a fronted project // disconnects the owner's nginx from its network before its network is removed. -func beforeStopSharedProxy(ctx context.Context, workdir string) { +// The owner lock is held until the returned function runs after the down, so a +// concurrent owner start cannot reattach the network in between (D-0929). +func beforeStopSharedProxy(ctx context.Context, workdir string) func() { site, err := spLocate(workdir) if err != nil { ui.Warn(fmt.Sprintf("Shared proxy: registrations are not readable, so the owner's nginx was not disconnected and the network may stay in use: %v", err)) - return + return nil } if site.Role != sharedproxy.RoleFronted { - return + return nil } - switch err := spDetach(ctx, site.OwnerDir, *site.Reg); { + release, err := spDetach(ctx, site.OwnerDir, *site.Reg) + switch { case err == nil: case errors.Is(err, sharedproxy.ErrOwnerNotRunning): ui.Warn("Shared proxy: the owner's nginx is not running; nothing to disconnect") default: ui.Warn(fmt.Sprintf("Shared proxy: %v", err)) } + return release } // leaveSharedProxy is `nself uninstall` in a fronted project (review R2b): it @@ -140,6 +155,14 @@ func leaveSharedProxy(ctx context.Context, workdir string) error { return errs.New("E506", "cannot leave the shared proxy: its registrations are not readable").WithWhy(err.Error()) } if site.Role != sharedproxy.RoleFronted { + // A retry after a failed reload: the registration is already gone, but + // the parent owner may still owe a reload (D-0931). + if parent := filepath.Dir(workdir); sharedproxy.ReloadPending(parent) { + if _, err := spFlush(ctx, parent); err != nil { + ui.Warn(fmt.Sprintf("Shared proxy: reloading %s: %v", parent, err)) + return fmt.Errorf("reloading the shared proxy at %s: %w", parent, err) + } + } return nil } if _, err := spLeave(ctx, site.OwnerDir, site.Reg.Project); err != nil { diff --git a/cmd/commands/start_shared_proxy_pending_test.go b/cmd/commands/start_shared_proxy_pending_test.go new file mode 100644 index 000000000..7b4ee60d9 --- /dev/null +++ b/cmd/commands/start_shared_proxy_pending_test.go @@ -0,0 +1,108 @@ +package commands + +// Tests for D-0931 in start_shared_proxy.go and restart_strategies.go: a +// targeted `restart nginx` reattaches, the owner's start pays a reload a +// failed uninstall left owed, and an uninstall retry pays it too. + +import ( + "context" + "errors" + "os" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/nself-org/cli/internal/docker" + "github.com/nself-org/cli/internal/sharedproxy" +) + +func stubFlush(t *testing.T, err error) *[]string { + t.Helper() + var calls []string + orig := spFlush + t.Cleanup(func() { spFlush = orig }) + spFlush = func(_ context.Context, ownerDir string) (bool, error) { + calls = append(calls, ownerDir) + return true, err + } + return &calls +} + +func TestRestartServicesReattachesNginx(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("fake docker is a POSIX shell script") + } + ctx := context.Background() + owner, _, _ := proxyLayout(t) + bin := t.TempDir() + if err := os.WriteFile(filepath.Join(bin, "docker"), []byte("#!/bin/sh\nexit 0\n"), 0o755); err != nil { + t.Fatal(err) + } + comp := &docker.Compose{DockerPath: filepath.Join(bin, "docker")} + stubFlush(t, nil) + + calls := stubAttach(t, sharedproxy.Outcome{}, nil) + if err := restartServices(ctx, comp, owner, nil, []string{"hasura"}, false); err != nil || len(*calls) != 0 { + t.Fatalf("restarting another service must not attach: err %v calls %v", err, *calls) + } + if err := restartServices(ctx, comp, owner, nil, []string{"nginx"}, false); err != nil { + t.Fatal(err) + } + if len(*calls) != 1 || (*calls)[0].owner != owner || strings.Join((*calls)[0].projects, ",") != "b,c" { + t.Errorf("restart nginx must reattach every registration, got %+v", *calls) + } +} + +func TestOwnerStartPaysAPendingReload(t *testing.T) { + ctx := context.Background() + owner, _, _ := proxyLayout(t) + stubAttach(t, sharedproxy.Outcome{}, nil) + flushed := stubFlush(t, nil) + if err := afterStartSharedProxy(ctx, nil, owner); err != nil { + t.Fatal(err) + } + if len(*flushed) != 1 || (*flushed)[0] != owner { + t.Errorf("the owner's start must flush the owed reload, got %v", *flushed) + } + // A failed flush warns; the start still succeeds. + stubFlush(t, errors.New("boom")) + out := captureStderr(t, func() { _ = afterStartSharedProxy(ctx, nil, owner) }) + if !strings.Contains(out, "could not be reloaded") { + t.Errorf("want a warning, got %q", out) + } +} + +func TestUninstallRetryPaysAPendingReload(t *testing.T) { + ctx := context.Background() + owner, b, _ := proxyLayout(t) + pending := filepath.Join(sharedproxy.DirOf(owner), sharedproxy.ReloadPendingFile) + + // b's registration is gone, as after a leave whose reload failed. + if err := os.Remove(filepath.Join(sharedproxy.DirOf(owner), sharedproxy.FileName(b.Project))); err != nil { + t.Fatal(err) + } + t.Run("nothing owed", func(t *testing.T) { + flushed := stubFlush(t, nil) + if err := leaveSharedProxy(ctx, b.ProjectDir); err != nil || len(*flushed) != 0 { + t.Fatalf("err %v flushed %v", err, *flushed) + } + }) + if err := os.WriteFile(pending, nil, 0o600); err != nil { + t.Fatal(err) + } + t.Run("owed and paid", func(t *testing.T) { + flushed := stubFlush(t, nil) + if err := leaveSharedProxy(ctx, b.ProjectDir); err != nil || len(*flushed) != 1 || (*flushed)[0] != owner { + t.Fatalf("err %v flushed %v", err, *flushed) + } + }) + t.Run("owed and still failing", func(t *testing.T) { + stubFlush(t, errors.New("reload boom")) + var err error + captureStderr(t, func() { err = leaveSharedProxy(ctx, b.ProjectDir) }) + if err == nil { + t.Fatal("a retry that cannot reload must report the incomplete removal") + } + }) +} diff --git a/cmd/commands/start_shared_proxy_test.go b/cmd/commands/start_shared_proxy_test.go index 1af63108e..d43a1892e 100644 --- a/cmd/commands/start_shared_proxy_test.go +++ b/cmd/commands/start_shared_proxy_test.go @@ -149,16 +149,24 @@ func TestSharedProxyDetachOnDown(t *testing.T) { var detached []string var detachErr error + held := 0 // locks taken by the stub and not yet released orig := spDetach t.Cleanup(func() { spDetach = orig }) - spDetach = func(_ context.Context, ownerDir string, regs ...sharedproxy.Registration) error { + spDetach = func(_ context.Context, ownerDir string, regs ...sharedproxy.Registration) (func(), error) { if data, _ := os.ReadFile(order); len(data) != 0 { t.Errorf("detach ran after docker: %q", data) } for _, r := range regs { detached = append(detached, ownerDir+"|"+r.Project) } - return detachErr + held++ + return func() { + held-- + // D-0929: the lock outlives the compose down. + if data, _ := os.ReadFile(order); !strings.HasPrefix(string(data), "docker compose down") { + t.Errorf("the owner lock was released before docker down ran: %q", data) + } + }, detachErr } // Every compose-down path of a fronted project detaches first. @@ -174,6 +182,9 @@ func TestSharedProxyDetachOnDown(t *testing.T) { if data, _ := os.ReadFile(order); !strings.HasPrefix(string(data), "docker compose down") { t.Errorf("docker down did not run after the detach: %q", data) } + if held != 0 { + t.Errorf("down %+v left the owner lock held (%d)", opts, held) + } } // The owner's own down, and a project outside any proxy, detach nothing. @@ -195,6 +206,9 @@ func TestSharedProxyDetachOnDown(t *testing.T) { if err := comp.ComposeDown(context.Background(), b.ProjectDir, docker.DownOptions{}); err != nil { t.Errorf("detach error %v must not fail the down: %v", e, err) } + if held != 0 { + t.Errorf("detach error %v left the owner lock held (%d)", e, held) + } } } diff --git a/cmd/commands/start_shared_proxy_unreadable_test.go b/cmd/commands/start_shared_proxy_unreadable_test.go index ec08dea85..74a487ef6 100644 --- a/cmd/commands/start_shared_proxy_unreadable_test.go +++ b/cmd/commands/start_shared_proxy_unreadable_test.go @@ -64,8 +64,11 @@ func TestUnreadableRegistrations(t *testing.T) { detached := false orig := spDetach t.Cleanup(func() { spDetach = orig }) - spDetach = func(context.Context, string, ...sharedproxy.Registration) error { detached = true; return nil } - out := captureStderr(t, func() { beforeStopSharedProxy(ctx, b.ProjectDir) }) + spDetach = func(context.Context, string, ...sharedproxy.Registration) (func(), error) { + detached = true + return func() {}, nil + } + out := captureStderr(t, func() { _ = beforeStopSharedProxy(ctx, b.ProjectDir) }) if detached || !strings.Contains(out, "not readable") || !strings.Contains(out, "zzz.json") { t.Fatalf("detached=%v, stderr %q: want a warning naming the unreadable file", detached, out) } diff --git a/internal/docker/compose.go b/internal/docker/compose.go index 72f801f06..62590133f 100644 --- a/internal/docker/compose.go +++ b/internal/docker/compose.go @@ -74,7 +74,9 @@ func (c *Compose) ComposeUp(ctx context.Context, workdir string, services ...str // ComposeDown runs `docker compose down` with the given options in the given workdir. func (c *Compose) ComposeDown(ctx context.Context, workdir string, opts DownOptions) error { - RunPreDownHooks(ctx, workdir) // the shared proxy detaches here (compose_hooks.go) + // The shared proxy detaches here and keeps its lock until the down is done + // (compose_hooks.go), so an owner attach cannot slip in between. + defer RunPreDownHooks(ctx, workdir)() args := c.buildBaseArgs() args = append(args, "down") if opts.RemoveVolumes { diff --git a/internal/docker/compose_hooks.go b/internal/docker/compose_hooks.go index 109f4d73c..921f74ebf 100644 --- a/internal/docker/compose_hooks.go +++ b/internal/docker/compose_hooks.go @@ -11,15 +11,18 @@ package docker // Outputs: RegisterPreDownHook returns the function that unregisters the hook. // Constraints: a hook cannot fail the down (it reports through its own // channel); hooks run in registration order, once per ComposeDown call, before -// docker is invoked; safe for concurrent use. +// docker is invoked; safe for concurrent use. A hook may return a release +// function: ComposeDown calls it after docker returns, so a hook can hold a +// lock across its own work and the down (D-0929). Releases run in reverse order. import ( "context" "sync" ) -// PreDownHook runs before `docker compose down` in workdir. -type PreDownHook func(ctx context.Context, workdir string) +// PreDownHook runs before `docker compose down` in workdir. The function it +// returns, if not nil, runs once the down has finished. +type PreDownHook func(ctx context.Context, workdir string) (release func()) type preDownEntry struct { id int @@ -52,15 +55,24 @@ func RegisterPreDownHook(h PreDownHook) (unregister func()) { } } -// RunPreDownHooks runs every registered hook for workdir. -func RunPreDownHooks(ctx context.Context, workdir string) { +// RunPreDownHooks runs every registered hook for workdir and returns a +// function that releases whatever they hold. Call it once the down is done. +func RunPreDownHooks(ctx context.Context, workdir string) (release func()) { preDownMu.Lock() hooks := make([]PreDownHook, 0, len(preDownHooks)) for _, e := range preDownHooks { hooks = append(hooks, e.hook) } preDownMu.Unlock() + var releases []func() for _, h := range hooks { - h(ctx, workdir) + if r := h(ctx, workdir); r != nil { + releases = append(releases, r) + } + } + return func() { + for i := len(releases) - 1; i >= 0; i-- { + releases[i]() + } } } diff --git a/internal/docker/compose_hooks_test.go b/internal/docker/compose_hooks_test.go index 1f79ecfa9..f8cf17111 100644 --- a/internal/docker/compose_hooks_test.go +++ b/internal/docker/compose_hooks_test.go @@ -24,16 +24,26 @@ func TestPreDownHook(t *testing.T) { c := &Compose{DockerPath: bin} var got []string - unregister := RegisterPreDownHook(func(_ context.Context, workdir string) { + released := 0 + unregister := RegisterPreDownHook(func(_ context.Context, workdir string) func() { got = append(got, workdir) b, _ := os.ReadFile(order) if len(b) != 0 { t.Errorf("the hook ran after docker: %q", b) } + return func() { + released++ + if b, _ := os.ReadFile(order); !strings.HasPrefix(string(b), "docker compose down") { + t.Errorf("the hook was released before docker finished: %q", b) + } + } }) if err := c.ComposeDown(context.Background(), dir, DownOptions{RemoveOrphans: true}); err != nil { t.Fatal(err) } + if released != 1 { + t.Errorf("release ran %d times after the down, want 1 (D-0929)", released) + } if !slices.Equal(got, []string{dir}) { t.Errorf("hook calls = %v, want one call for %s", got, dir) } diff --git a/internal/doctor/dogfood_checks.go b/internal/doctor/dogfood_checks.go index 5301257fd..6e7f1952f 100644 --- a/internal/doctor/dogfood_checks.go +++ b/internal/doctor/dogfood_checks.go @@ -103,5 +103,5 @@ func checkLicenseVaultPath() CheckResult { } return CheckResult{Section: "dogfood", Name: checkName, Status: "warn", Message: "no license env var set; some Licensed plugin checks may skip", - FixCmd: "source ~/.claude/vault.env # or set NSELF_PLUGIN_LICENSE_KEY"} + FixCmd: "# load your secrets file, or set NSELF_PLUGIN_LICENSE_KEY"} } diff --git a/internal/doctor/dogfood_privatepath_test.go b/internal/doctor/dogfood_privatepath_test.go new file mode 100644 index 000000000..5ab5a30d2 --- /dev/null +++ b/internal/doctor/dogfood_privatepath_test.go @@ -0,0 +1,25 @@ +package doctor + +import ( + "os" + "strings" + "testing" +) + +// TestNoOperatorPrivatePathAdvice (D-0938): product text that only advises the +// user must not point public-repo users at an operator-private secrets path. +// check_ci_vault_sync.go is exempt: it is the one place that reads the file. +func TestNoOperatorPrivatePathAdvice(t *testing.T) { + for _, p := range []string{ + "dogfood_checks.go", + "../../scripts/ops/credential-rotation-check.sh", + } { + b, err := os.ReadFile(p) + if err != nil { + t.Fatalf("read %s: %v", p, err) + } + if strings.Contains(string(b), "vault.env") { + t.Errorf("%s mentions the operator-private vault.env path", p) + } + } +} diff --git a/internal/doctor/isolation_rules_test.go b/internal/doctor/isolation_rules_test.go index 77ad8337e..61f79f71e 100644 --- a/internal/doctor/isolation_rules_test.go +++ b/internal/doctor/isolation_rules_test.go @@ -8,6 +8,8 @@ import ( "sort" "strings" "testing" + + "gopkg.in/yaml.v3" ) // isoFixture is one testdata/isolation/*.json file: a live export_metadata @@ -98,9 +100,19 @@ func TestIsolationRules(t *testing.T) { } // TestIsolationTemplateFile keeps the guard in step with the shipped template: -// the real multi-app metadata.json must produce no finding. +// the real multi-app tables.yaml (the one table metadata the template ships, +// D-0917) must produce no finding. It is wrapped into the export_metadata +// shape the guard parses. func TestIsolationTemplateFile(t *testing.T) { - b, err := os.ReadFile(filepath.Join("..", "templates", "clone", "multi-app", "hasura", "metadata.json")) + raw, err := os.ReadFile(filepath.Join("..", "templates", "clone", "multi-app", "hasura", "metadata", "databases", "default", "tables", "tables.yaml")) + if err != nil { + t.Fatal(err) + } + var tables []any + if err := yaml.Unmarshal(raw, &tables); err != nil { + t.Fatal(err) + } + b, err := json.Marshal(map[string]any{"sources": []any{map[string]any{"name": "default", "tables": tables}}}) if err != nil { t.Fatal(err) } diff --git a/internal/maintenance/runner_posix.go b/internal/maintenance/runner_posix.go index 13f720f97..5a3bab145 100644 --- a/internal/maintenance/runner_posix.go +++ b/internal/maintenance/runner_posix.go @@ -3,6 +3,7 @@ package maintenance import ( + "errors" "fmt" "io/fs" "os" @@ -83,7 +84,16 @@ func reclaimRunnerWork(root string, dryRun bool) (bytes int64, reclaimed []Recla workDir := filepath.Join(root, "_work") entries, err := os.ReadDir(workDir) if err != nil { - return 0, nil, nil // no _work dir yet (fresh install) — nothing to do + if errors.Is(err, fs.ErrNotExist) { + return 0, nil, nil // no _work dir yet (fresh install) — nothing to do + } + // Unreadable (EACCES, ENOTDIR, EIO) is not "empty": report it so a + // cleanup that reclaimed nothing is not mistaken for a healthy no-op. + skipped = append(skipped, SkipEntry{ + Path: workDir, + Reason: fmt.Sprintf("cannot read _work: %v", err), + }) + return 0, nil, skipped } for _, e := range entries { diff --git a/internal/maintenance/runner_posix_test.go b/internal/maintenance/runner_posix_test.go index 0aa62d493..d2c99e04d 100644 --- a/internal/maintenance/runner_posix_test.go +++ b/internal/maintenance/runner_posix_test.go @@ -444,3 +444,31 @@ func TestPressureThresholdStaysBelowDoctorHostLimit(t *testing.T) { DefaultPressureThreshold, doctorHostDiskUsedLimit) } } + +// TestReclaimRunnerWork_UnreadableWorkDirIsReported guards D-0061: an _work +// path that cannot be read (anything but "does not exist") used to return the +// same empty result as a healthy no-op. It must surface a SkipEntry instead. +func TestReclaimRunnerWork_UnreadableWorkDirIsReported(t *testing.T) { + root := t.TempDir() + // _work is a file, so ReadDir fails with ENOTDIR rather than ENOENT. + if err := os.WriteFile(filepath.Join(root, "_work"), []byte("x"), 0o600); err != nil { + t.Fatal(err) + } + + _, reclaimed, skipped := reclaimRunnerWork(root, false) + if len(reclaimed) != 0 { + t.Fatalf("reclaimed = %v; want none", reclaimed) + } + if len(skipped) != 1 || skipped[0].Path != filepath.Join(root, "_work") { + t.Fatalf("skipped = %+v; want one entry for the unreadable _work dir", skipped) + } +} + +// TestReclaimRunnerWork_MissingWorkDirIsSilent keeps the legitimate case: a +// fresh runner install has no _work yet, which is nothing to report. +func TestReclaimRunnerWork_MissingWorkDirIsSilent(t *testing.T) { + _, reclaimed, skipped := reclaimRunnerWork(t.TempDir(), false) + if len(reclaimed) != 0 || len(skipped) != 0 { + t.Fatalf("got reclaimed=%v skipped=%v; want both empty for a missing _work", reclaimed, skipped) + } +} diff --git a/internal/plugin/err_to_zero_test.go b/internal/plugin/err_to_zero_test.go new file mode 100644 index 000000000..0e42af7b1 --- /dev/null +++ b/internal/plugin/err_to_zero_test.go @@ -0,0 +1,50 @@ +//go:build !windows + +package plugin + +import ( + "context" + "net" + "os" + "path/filepath" + "testing" + + "github.com/nself-org/cli/internal/config" +) + +// TestGetSchemaVersion_PsqlFailureIsAnError guards D-0061: a failed psql query +// used to read as "schema version 0", so a broken database looked like a fresh +// one and the caller re-ran schema provisioning against it. +func TestGetSchemaVersion_PsqlFailureIsAnError(t *testing.T) { + dir := t.TempDir() + stub := "#!/bin/sh\necho 'Error response from daemon: No such container' >&2\nexit 1\n" + if err := os.WriteFile(filepath.Join(dir, "docker"), []byte(stub), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + + v, err := getSchemaVersion(context.Background(), &config.Config{ProjectName: "demo"}, "cron") + if err == nil { + t.Fatalf("getSchemaVersion = %d, nil; want an error when psql fails", v) + } +} + +// TestHealth_UnreachablePluginReturnsDialError guards D-0061: an unreachable +// plugin used to return (false, nil), so the start loop could only report a +// generic "timed out" instead of the dial error it had actually seen. +func TestHealth_UnreachablePluginReturnsDialError(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + port := ln.Addr().(*net.TCPAddr).Port + _ = ln.Close() // nothing listens on port any more + + ok, err := health(context.Background(), "gone", port) + if ok { + t.Fatal("health reported ok for an unreachable plugin") + } + if err == nil { + t.Fatal("health returned a nil error for an unreachable plugin; want the dial error") + } +} diff --git a/internal/plugin/installer_platform_test.go b/internal/plugin/installer_platform_test.go index 298540449..6f14c41bc 100644 --- a/internal/plugin/installer_platform_test.go +++ b/internal/plugin/installer_platform_test.go @@ -54,6 +54,7 @@ type platRegistry struct { sourceHits, binaryHits int32 sourceSig any platformSigs any + binaryMissing bool // the platform release asset 404s, forcing the source fallback } func (p *platRegistry) serve(t *testing.T) { @@ -62,6 +63,10 @@ func (p *platRegistry) serve(t *testing.T) { switch { case strings.Contains(r.URL.Path, "/releases/download/"): atomic.AddInt32(&p.binaryHits, 1) + if p.binaryMissing { + http.NotFound(w, r) + return + } _, _ = w.Write(p.binary) case strings.HasSuffix(r.URL.Path, "/tarball"): atomic.AddInt32(&p.sourceHits, 1) @@ -221,6 +226,54 @@ func TestInstallBinaryPackageNoSourceFallback(t *testing.T) { }) } +// TestInstallSourceFallbackRefusedE121 (D-1415): when the platform package +// 404s, the validly signed source package must not install a command plugin +// whose entry advertises platform_signatures for this platform, whether or not +// that source tarball holds a binary, and the dev bypass does not open it. +func TestInstallSourceFallbackRefusedE121(t *testing.T) { + compattest.Both(t, func(t *testing.T) { + for name, withBinary := range map[string]bool{"source without binary": false, "source with binary": true} { + t.Run(name, func(t *testing.T) { + p, dir, err := platCase(t, func(p *platRegistry, signer signing.Signer, _ string) { + p.binaryMissing = true + if withBinary { + p.source = binTarball(t, "# smuggled\n") + p.sourceSig = sigObj(signaturetest.Sign(t, signer, sigPlugin, "1.0.0", sha256Hex(p.source))) + } + }) + if codeOf(err) != "E121" || !dirEmpty(t, dir) { + t.Fatalf("want E121 and an empty plugin dir, got %v", err) + } + if atomic.LoadInt32(&p.binaryHits) == 0 || atomic.LoadInt32(&p.sourceHits) == 0 { + t.Fatalf("the test did not exercise the fallback: binary=%d source=%d", p.binaryHits, p.sourceHits) + } + if _, statErr := os.Stat(filepath.Join(dir, "bin", "nself-"+sigPlugin)); statErr == nil { + t.Fatal("a binary was published from the source fallback") + } + }) + } + }) +} + +// TestInstallSourceFallbackWithoutPlatformSignaturesInstalls: the refusal only +// applies to an entry that advertises platform_signatures; a plugin that never +// shipped them keeps installing from its signed source package. +func TestInstallSourceFallbackWithoutPlatformSignaturesInstalls(t *testing.T) { + compattest.Both(t, func(t *testing.T) { + _, dir, err := platCase(t, func(p *platRegistry, signer signing.Signer, _ string) { + p.binaryMissing, p.platformSigs = true, nil + p.source = binTarball(t, "") + p.sourceSig = sigObj(signaturetest.Sign(t, signer, sigPlugin, "1.0.0", sha256Hex(p.source))) + }) + if err != nil { + t.Fatalf("a signed source package for an entry with no platform_signatures must install: %v", err) + } + if _, err := os.Stat(filepath.Join(dir, sigPlugin, "plugin.json")); err != nil { + t.Fatalf("not extracted: %v", err) + } + }) +} + // TestInstallBinaryPackageBypassNeverSkips: the dev bypass skips an ABSENT // platform signature, never a present one, and a registry that strips // release_signature does not change that. diff --git a/internal/plugin/installer_signature.go b/internal/plugin/installer_signature.go index a8a2dd838..e10a8c377 100644 --- a/internal/plugin/installer_signature.go +++ b/internal/plugin/installer_signature.go @@ -5,7 +5,7 @@ package plugin // Purpose: verify the registry's release_signature (hq PLUG D7, contract plugins.release-signature v1) against the keys compiled into the CLI, for both tiers, after the tarball checksum (E124) and before anything is extracted. Split out of installer_locked.go (299-line ratchet). // Inputs: the resolved registry manifest and whether the entry is licensed (paid). // Outputs: nil (verified, or a one-line warning printed), or a coded error: E121 bad signature, E122 unknown key, E123 revoked key, E107 unsigned where signatures are required. -// Constraints: the release_signature covers the SOURCE tarball's sha256 (manifest.Checksum). A binary (platform) package on a pinned tier installs only when platform_signatures[] verifies over the sha256 computed here from the downloaded archive (contract v1.1, P7-PLUG-84; never the registry's checksum field); absent or invalid is E121, with no source-signature fallback. The dev bypass never skips a present signature on a pinned tier. The manifest's author_public_key is never read for trust. --allow-unsigned is honoured for local path or link installs only; registry installs pass local=false and cannot opt out. Pure policy lives in verify/signature.Decide. +// Constraints: the release_signature covers the SOURCE tarball's sha256 (manifest.Checksum). A binary (platform) package on a pinned tier installs only when platform_signatures[] verifies over the sha256 computed here from the downloaded archive (contract v1.1, P7-PLUG-84; never the registry's checksum field); absent or invalid is E121, with no source-signature fallback; a command plugin whose entry advertises platform_signatures for this platform refuses a source artifact with E121 (D-1415). The dev bypass never skips a present signature on a pinned tier. The manifest's author_public_key is never read for trust. --allow-unsigned is honoured for local path or link installs only; registry installs pass local=false and cannot opt out. Pure policy lives in verify/signature.Decide. import ( "crypto/sha256" @@ -57,6 +57,11 @@ func verifyReleaseSignature(manifest *PluginManifest, paid, local bool, artifact if pinned && artifactKind != "" && artifactKind != ArtifactKindSource { return verifyPlatformSignature(manifest, tier, artifactKind, archivePath, bypass) } + if pinned { + if err := refuseSourceFallback(manifest); err != nil { + return err + } + } if bypass && (!pinned || manifest.ReleaseSignature == nil) { return nil } @@ -81,6 +86,28 @@ func verifyReleaseSignature(manifest *PluginManifest, paid, local bool, artifact return nil } +// refuseSourceFallback closes the source-package fallback for a command plugin +// whose entry advertises a platform signature for this platform (D-1415). The +// registry field that picks the download (repository) is unsigned, so it can +// make the platform package 404 and push the install onto the source tarball; +// that tarball is covered only by release_signature, and linkCLIBinary would +// publish any binary it holds. An entry that advertises platform_signatures +// for this platform promises a signed platform package, so a source artifact is +// E121. The dev bypass does not apply: the signature entry is present. +func refuseSourceFallback(manifest *PluginManifest) error { + if len(cliBinaryNames(manifest.Name, manifest)) == 0 { + return nil + } + plat, err := PlatformArch() + if err != nil { + return nil + } + if _, advertised := manifest.PlatformSignatures[plat]; !advertised { + return nil + } + return errs.Newf("E121", "plugin %q: the %s platform package was unavailable and the source package carries no platform signature; refusing the source fallback", manifest.Name, plat) +} + // verifyPlatformSignature checks the platform signature of a binary package // on a pinned tier. Only the platform the CLI selected (artifactKind, from // PlatformArch()) is ever looked up. The bypass skips an ABSENT entry, never a diff --git a/internal/plugin/runtime.go b/internal/plugin/runtime.go index 073aef3fe..f701c4c96 100644 --- a/internal/plugin/runtime.go +++ b/internal/plugin/runtime.go @@ -190,7 +190,7 @@ func Status(name string) (*PluginStatus, error) { // health performs an HTTP health check against a plugin by sending a GET // request to http://localhost:{port}/health. It returns true if the response -// status is 200 OK. +// status is 200 OK, and a non-nil error when the plugin cannot be reached. func health(ctx context.Context, name string, port int) (bool, error) { url := fmt.Sprintf("http://localhost:%d/health", port) @@ -202,7 +202,9 @@ func health(ctx context.Context, name string, port int) (bool, error) { client := &http.Client{Timeout: 5 * time.Second} resp, err := client.Do(req) if err != nil { - return false, nil + // Unreachable is "not healthy", but the dial error is the reason; the + // start loop keeps it as lastErr instead of a generic timeout. + return false, fmt.Errorf("health check for %s: %w", name, err) } defer func() { _ = resp.Body.Close() }() diff --git a/internal/plugin/schema.go b/internal/plugin/schema.go index d0e48c53c..2aea4e8df 100644 --- a/internal/plugin/schema.go +++ b/internal/plugin/schema.go @@ -125,8 +125,11 @@ func getSchemaVersion(ctx context.Context, cfg *config.Config, pluginName string ) out, err := queryPSQL(ctx, cfg, sql) if err != nil { - // Table may not exist yet on a fresh instance; treat as version 0. - return 0, nil + // Not "version 0": a failed query means the version is unknown, and + // callers act on 0 by re-running schema provisioning. The version + // table is created (checked) before this is called, so a missing + // table is not an expected failure here. + return 0, fmt.Errorf("reading schema version for %q: %w", pluginName, err) } if out == "" { return 0, nil diff --git a/internal/sharedproxy/attach.go b/internal/sharedproxy/attach.go index bf0152060..e81ca5e0b 100644 --- a/internal/sharedproxy/attach.go +++ b/internal/sharedproxy/attach.go @@ -22,7 +22,8 @@ package sharedproxy // test is repeated (once more per further project the output names), and when // it still fails every connection made by this call is undone. On every failed // exit the files this call restored from quarantine go back into it, so a -// conf known to fail `nginx -t` never stays in nginx/sites. The public entry +// conf known to fail `nginx -t` never stays in nginx/sites. A restarting owner +// this call stopped is started again on any failed exit. The public entry // points (Attach, Detach, ...) hold the owner lock (lock.go); the bodies are // lower case. All docker calls go through internal/docker. diff --git a/internal/sharedproxy/attach_flow.go b/internal/sharedproxy/attach_flow.go index d527cac34..c25bb2266 100644 --- a/internal/sharedproxy/attach_flow.go +++ b/internal/sharedproxy/attach_flow.go @@ -30,6 +30,7 @@ type attachRun struct { out Outcome connected []Registration // connected by this call, for the rollback restored []Registration // Files = what this call took out of quarantine + stopped bool // this call stopped a restarting owner nginx } // undo disconnects every network this call connected. @@ -84,9 +85,30 @@ func attach(ctx context.Context, ownerDir, self string, regs []Registration) (Ou err = errors.Join(err, qerr) } } + if err != nil { + if serr := a.startOwnerBack(); serr != nil { + err = errors.Join(err, serr) + } + } return a.out, err } +// startOwnerBack starts the owner's nginx again when this call stopped it (a +// restarting owner) and the attach then failed before it was started: the +// owner is left as it was found, never stopped by a failed attach (D-0931). +func (a *attachRun) startOwnerBack() error { + if !a.stopped { + return nil + } + if st, err := docker.InspectState(a.ctx, a.nginx); err == nil && st == "running" { + return nil + } + if err := docker.StartContainer(a.ctx, a.nginx); err != nil { + return e506("the attach failed and the owner nginx "+a.nginx+" could not be started again; run nself start in the owner", err, "") + } + return nil +} + // run does the work. settled is true when the error it returns leaves a // consistent disk (the broken project(s) are quarantined and nginx reloaded). func (a *attachRun) run(regs []Registration) (settled bool, err error) { @@ -112,6 +134,7 @@ func (a *attachRun) run(regs []Registration) (settled bool, err error) { if err := docker.StopContainer(a.ctx, nginx); err != nil { return false, e506("cannot stop the restarting owner nginx "+nginx, err, "") } + a.stopped = true } before, err := docker.ContainerNetworks(a.ctx, nginx) if err != nil { diff --git a/internal/sharedproxy/hold_pending_test.go b/internal/sharedproxy/hold_pending_test.go new file mode 100644 index 000000000..2c374d769 --- /dev/null +++ b/internal/sharedproxy/hold_pending_test.go @@ -0,0 +1,151 @@ +package sharedproxy + +// Tests for D-0929 (DetachHolding keeps the owner lock) and D-0931 (a leave +// whose reload failed is retried; a failed attach starts a restarting owner +// back; the reload debt survives until paid). + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +func TestDetachHoldingKeepsTheLockUntilReleased(t *testing.T) { + ctx := context.Background() + f := newProxyFixture(t, "b_network") + f.owner0() + b := f.project("b") + if _, err := Attach(ctx, f.owner, "b", []Registration{b}); err != nil { + t.Fatal(err) + } + oldWait, oldPoll := lockWait, lockPoll + lockWait, lockPoll = 80*time.Millisecond, time.Millisecond + t.Cleanup(func() { lockWait, lockPoll = oldWait, oldPoll }) + + release, err := DetachHolding(ctx, f.owner, b) + if err != nil { + t.Fatal(err) + } + if strings.Contains(f.nets(), "b_network") { + t.Error("DetachHolding must disconnect the network") + } + // An owner attach arriving between the detach and the compose down waits. + if _, err := Attach(ctx, f.owner, "", []Registration{b}); !isE506(err) { + t.Fatalf("an attach while the detach lock is held must time out, got %v", err) + } + if strings.Contains(f.nets(), "b_network") { + t.Error("the owner reattached the network while the lock was held") + } + release() + if _, err := Attach(ctx, f.owner, "", []Registration{b}); err != nil { + t.Fatalf("after release the attach must work: %v", err) + } +} + +func TestLeaveReloadFailureIsRetried(t *testing.T) { + ctx := context.Background() + f := newProxyFixture(t, "b_network") + f.owner0() + f.project("b") + marker := filepath.Join(DirOf(f.owner), ReloadPendingFile) + + variantDocker(t, reloadFails, `echo "reload boom" >&2; exit 1`) + if _, err := Leave(ctx, f.owner, "b"); !isE506(err) { + t.Fatalf("want the reload failure, got %v", err) + } + if _, err := os.Stat(marker); err != nil || !ReloadPending(f.owner) { + t.Fatalf("a failed reload must leave the debt recorded: %v", err) + } + + // The retry finds no registration but pays the reload, then forgets the debt. + newProxyDockerOnPath(t, f) + before := f.reloads() + if _, err := Leave(ctx, f.owner, "b"); err != nil { + t.Fatalf("retry: %v", err) + } + if f.reloads() != before+1 { + t.Errorf("the retry must reload the owner once (reloads %d -> %d)", before, f.reloads()) + } + if ReloadPending(f.owner) { + t.Error("a paid reload must clear the marker") + } + // Nothing owed: a leave of an unknown project is a no-op again. + if _, err := Leave(ctx, f.owner, "b"); err != nil || f.reloads() != before+1 { + t.Errorf("no-op expected, got err %v reloads %d", err, f.reloads()) + } +} + +func TestReloadIfPending(t *testing.T) { + ctx := context.Background() + f := newProxyFixture(t, "b_network") + f.owner0() + f.project("b") + if ran, err := ReloadIfPending(ctx, f.owner); ran || err != nil || f.reloads() != 0 { + t.Fatalf("nothing owed: ran=%v err=%v reloads=%d", ran, err, f.reloads()) + } + variantDocker(t, reloadFails, `echo "reload boom" >&2; exit 1`) + _, _ = Leave(ctx, f.owner, "b") + if ran, err := ReloadIfPending(ctx, f.owner); !ran || !isE506(err) || !ReloadPending(f.owner) { + t.Fatalf("a failing reload keeps the debt: ran=%v err=%v", ran, err) + } + newProxyDockerOnPath(t, f) + if ran, err := ReloadIfPending(ctx, f.owner); !ran || err != nil || ReloadPending(f.owner) { + t.Fatalf("paying the debt: ran=%v err=%v", ran, err) + } +} + +func TestLeaveWithAStoppedOwnerOwesNothing(t *testing.T) { + f := newProxyFixture(t, "b_network") + f.owner0() + f.project("b") + f.write(filepath.Join(f.fake, "stopped"), "") + if _, err := Leave(context.Background(), f.owner, "b"); err != nil { + t.Fatal(err) + } + if ReloadPending(f.owner) { + t.Error("a stopped owner reads its files afresh on start: no reload is owed") + } +} + +func TestFailedAttachStartsARestartingOwnerBack(t *testing.T) { + ctx := context.Background() + f := newProxyFixture(t, "b_network") + f.owner0() + b := f.project("b") + f.setState("restarting") + failingConnectDocker(t, "b_network") + if _, err := Attach(ctx, f.owner, "", []Registration{b}); !isE506(err) { + t.Fatalf("want the connect failure, got %v", err) + } + if got := f.state(); got != "running" { + t.Errorf("owner state after a failed attach = %q, want running", got) + } +} + +func TestFailedAttachLeavesAnExitedOwnerAlone(t *testing.T) { + ctx := context.Background() + f := newProxyFixture(t, "b_network") + f.owner0() + b := f.project("b") + f.setState("exited") + failingConnectDocker(t, "b_network") + if _, err := Attach(ctx, f.owner, "", []Registration{b}); !isE506(err) { + t.Fatalf("want the connect failure, got %v", err) + } + if got := f.state(); got != "exited" { + t.Errorf("an owner this call did not stop must stay as found, state = %q", got) + } +} + +// newProxyDockerOnPath puts the plain fake docker back on PATH. +func newProxyDockerOnPath(t *testing.T, _ *proxyFixture) { + t.Helper() + bin := t.TempDir() + if err := os.WriteFile(filepath.Join(bin, "docker"), []byte(fakeProxyDocker), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", bin+string(os.PathListSeparator)+os.Getenv("PATH")) +} diff --git a/internal/sharedproxy/leave.go b/internal/sharedproxy/leave.go index 1711ae83b..247cfecc0 100644 --- a/internal/sharedproxy/leave.go +++ b/internal/sharedproxy/leave.go @@ -44,8 +44,9 @@ func removeQuarantine(ownerDir, project string, remove bool) error { return nil } -// leave detaches, unregisters and reloads. A project with no registration is a -// no-op. Leave is the locked entry. +// leave detaches, unregisters and reloads. A project with no registration owes +// nothing, unless an earlier leave could not reload (D-0931): then it pays that +// reload. Leave is the locked entry. func leave(ctx context.Context, ownerDir, project string) ([]string, error) { set, err := Load(DirOf(ownerDir), ownerDir) if err != nil { @@ -53,6 +54,9 @@ func leave(ctx context.Context, ownerDir, project string) ([]string, error) { } reg := set.Find(project) if reg == nil { + if ReloadPending(ownerDir) { + return nil, settleReload(ctx, ownerDir) + } return nil, nil } // Checked first: a refused quarantine path leaves registration and files alone. @@ -67,6 +71,11 @@ func leave(ctx context.Context, ownerDir, project string) ([]string, error) { if err != nil { return removed, err } + // The registration is gone, so a retry cannot tell a reload is owed: record + // it until the owner has reloaded (or is not running). + if err := markReloadPending(ownerDir); err != nil { + detachErr = errors.Join(detachErr, err) + } if err := removeQuarantine(ownerDir, project, true); err != nil { return removed, err } @@ -74,6 +83,7 @@ func leave(ctx context.Context, ownerDir, project string) ([]string, error) { switch { case errors.Is(nerr, ErrOwnerNotRunning): // Warning level: the files are gone and a stopped owner serves nothing. + clearReloadPending(ownerDir) case nerr != nil: // The files are gone but nginx still serves them; say so. return removed, errors.Join(ownerErr(nerr), detachErr) @@ -85,6 +95,7 @@ func leave(ctx context.Context, ownerDir, project string) ([]string, error) { if err := reload(ctx, nginx, &o, func() {}); err != nil { return removed, err } + clearReloadPending(ownerDir) } return removed, detachErr } diff --git a/internal/sharedproxy/lock.go b/internal/sharedproxy/lock.go index 54eb7a469..d408697c7 100644 --- a/internal/sharedproxy/lock.go +++ b/internal/sharedproxy/lock.go @@ -130,6 +130,19 @@ func Detach(ctx context.Context, ownerDir string, regs ...Registration) error { return err } +// DetachHolding takes the owner lock, detaches regs and returns with the lock +// still held: release hands it back. The caller removes the networks (compose +// down) before releasing, so an owner attach cannot reconnect nginx between the +// detach and the removal (D-0929). release is never nil; on a lock error it +// does nothing, and on a detach error the lock is still held. +func DetachHolding(ctx context.Context, ownerDir string, regs ...Registration) (release func(), err error) { + release, err = Lock(ctx, ownerDir) + if err != nil { + return release, err + } + return release, detach(ctx, ownerDir, regs...) +} + // Leave is leave under the owner lock. func Leave(ctx context.Context, ownerDir, project string) ([]string, error) { return locked(ctx, ownerDir, func() ([]string, error) { return leave(ctx, ownerDir, project) }) diff --git a/internal/sharedproxy/pending.go b/internal/sharedproxy/pending.go new file mode 100644 index 000000000..743977cc6 --- /dev/null +++ b/internal/sharedproxy/pending.go @@ -0,0 +1,112 @@ +package sharedproxy + +// pending.go — remember that the owner's nginx still has to reload after a leave. +// +// Purpose: D-0931. Leave removes the project's registration and files and then +// reloads the owner. If that reload fails, a retry finds no registration, so +// the registration cannot say the reload is owed and the running nginx would +// keep serving the removed project until its next reload. A marker file in +// nginx/shared records the debt: Leave writes it once the registration is +// gone, clears it when the owner reloaded (or is not running, so it reads its +// configuration afresh on start), and ReloadPending/ReloadIfPending pay it. +// Inputs: the owner dir. +// Outputs: a bool, or the error of the reload. +// Constraints: the marker is a regular file inside nginx/shared, written and +// removed through an os.Root on the owner dir; a symlink on the way is an E506 +// error. Load ignores it (it reads only `*.json`). Not a registration: it +// never lists files, so Unregister and Quarantine leave it alone. + +import ( + "context" + "errors" + "os" + "path/filepath" +) + +// ReloadPendingFile is the marker's name inside nginx/shared/. +const ReloadPendingFile = ".reload-pending" + +func pendingRel(ownerDir string) (string, error) { + return ownerRel(ownerDir, filepath.Join(DirOf(ownerDir), ReloadPendingFile)) +} + +// ReloadPending reports whether a leave still owes the owner a reload. +func ReloadPending(ownerDir string) bool { + _, err := os.Lstat(filepath.Join(DirOf(ownerDir), ReloadPendingFile)) + return err == nil +} + +// markReloadPending writes the marker. +func markReloadPending(ownerDir string) error { + root, err := os.OpenRoot(ownerDir) + if err != nil { + return e506("cannot open the shared proxy owner dir "+ownerDir, err, "") + } + defer func() { _ = root.Close() }() + rel, err := pendingRel(ownerDir) + if err != nil { + return e506("cannot record the pending reload", err, "") + } + if err := plainDirs(root, filepath.Dir(rel)); err != nil { + return e506("cannot record the pending reload", err, "") + } + f, err := root.OpenFile(rel, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o600) + if err != nil { + return e506("cannot record the pending reload", err, "") + } + return f.Close() +} + +// clearReloadPending removes the marker; a missing one is not an error. +func clearReloadPending(ownerDir string) { + root, err := os.OpenRoot(ownerDir) + if err != nil { + return + } + defer func() { _ = root.Close() }() + rel, err := pendingRel(ownerDir) + if err != nil || plainDirs(root, filepath.Dir(rel)) != nil { + return + } + _ = root.Remove(rel) +} + +// reloadOwner tests and reloads the owner's nginx. A stopped owner is success: +// it reads the files fresh when it starts. +func reloadOwner(ctx context.Context, ownerDir string) error { + nginx, nerr := OwnerNginx(ctx, ownerDir) + switch { + case errors.Is(nerr, ErrOwnerNotRunning): + return nil + case nerr != nil: + return ownerErr(nerr) + } + if _, stderr, terr := nginxTest(ctx, nginx); terr != nil { + return e506("the owner's nginx -t fails; it was not reloaded", terr, stderr) + } + var o Outcome + return reload(ctx, nginx, &o, func() {}) +} + +// settleReload runs reloadOwner and clears the marker when it succeeded. +func settleReload(ctx context.Context, ownerDir string) error { + err := reloadOwner(ctx, ownerDir) + if err == nil { + clearReloadPending(ownerDir) + } + return err +} + +// ReloadIfPending pays a reload a failed leave owed, under the owner lock. It +// is a no-op (false, nil) when none is owed. +func ReloadIfPending(ctx context.Context, ownerDir string) (bool, error) { + if !ReloadPending(ownerDir) { + return false, nil + } + return locked(ctx, ownerDir, func() (bool, error) { + if !ReloadPending(ownerDir) { // paid while this call waited for the lock + return false, nil + } + return true, settleReload(ctx, ownerDir) + }) +} diff --git a/internal/ssl/trust.go b/internal/ssl/trust.go index 0e89fcdc8..5a499530d 100644 --- a/internal/ssl/trust.go +++ b/internal/ssl/trust.go @@ -223,17 +223,9 @@ func installCALinux(caPath string) error { // ── Windows ─────────────────────────────────────────────────────────────────── func isCAInstalledWindows(caPath string) (bool, error) { - // certutil -store Root lists all trusted root certs. We check for exit 0 - // with the thumbprint. Simple approach: just check if certutil exits 0. - ctx, cancel := context.WithTimeout(context.Background(), mkcertCATimeout) - defer cancel() - out, err := sysexec.Stdout(ctx, "certutil", "-store", "Root") - if err != nil { - return false, nil - } - // Read the cert to get its subject for matching. - _ = out - // On Windows, we treat it as not installed if we can't verify; InstallCA is idempotent. + // Verification is not implemented on Windows: report "not installed" so + // the caller runs installCAWindows, which is idempotent (certutil + // -addstore Root). The constant false is deliberate, not a swallowed error. return false, nil } diff --git a/internal/templates/clone/clone_test.go b/internal/templates/clone/clone_test.go index 072c0e912..58ce84f4f 100644 --- a/internal/templates/clone/clone_test.go +++ b/internal/templates/clone/clone_test.go @@ -171,28 +171,33 @@ func maDecode(t *testing.T, raw json.RawMessage) interface{} { return v } -// maTables loads the shipped hasura/metadata.json as table name -> fields. +// maTables loads the shipped tables.yaml (the file `nself db reconcile` +// reads) as table name -> fields. func maTables(t *testing.T) map[string]map[string]json.RawMessage { t.Helper() - data, err := templateFS.ReadFile("multi-app/hasura/metadata.json") + data, err := templateFS.ReadFile("multi-app/hasura/metadata/databases/default/tables/tables.yaml") if err != nil { t.Fatal(err) } - var doc struct { - Sources []struct { - Tables []map[string]json.RawMessage `json:"tables"` - } `json:"sources"` - } - if err := json.Unmarshal(data, &doc); err != nil { + var doc []map[string]interface{} + if err := yaml.Unmarshal(data, &doc); err != nil { t.Fatal(err) } out := map[string]map[string]json.RawMessage{} - for _, tb := range doc.Sources[0].Tables { + for _, tb := range doc { + fields := map[string]json.RawMessage{} + for k, v := range tb { + raw, err := json.Marshal(v) + if err != nil { + t.Fatal(err) + } + fields[k] = raw + } var ref struct{ Name string } - if err := json.Unmarshal(tb["table"], &ref); err != nil { + if err := json.Unmarshal(fields["table"], &ref); err != nil { t.Fatal(err) } - out[ref.Name] = tb + out[ref.Name] = fields } return out } @@ -213,7 +218,7 @@ func TestMultiAppFiles(t *testing.T) { if err != nil { t.Fatal(err) } - want := map[string]bool{".env.example": true, "README.md": true, "hasura/metadata.json": true, + want := map[string]bool{".env.example": true, "README.md": true, "hasura/metadata/databases/default/tables/tables.yaml": true, "manifest.json": true, "migrations/001_multi_app.sql": true, "nself.yml": true} if len(got) != len(want) { @@ -302,24 +307,22 @@ func TestMultiAppNoSpoofing(t *testing.T) { } } -// TestMultiAppMetadataInSync: the YAML that `nself db reconcile` reads and the -// required hasura/metadata.json describe the same tables. +// TestMultiAppMetadataInSync (D-0917): tables.yaml, which `nself db reconcile` +// reads, is the only table metadata the template ships. A hasura/metadata.json +// would be read only by replace_metadata (`nself db hasura metadata apply`), +// which untracks the hasura-auth tables, so it must not come back. func TestMultiAppMetadataInSync(t *testing.T) { - raw, err := templateFS.ReadFile("multi-app/hasura/metadata/databases/default/tables/tables.yaml") - if err != nil { - t.Fatal(err) + if _, err := templateFS.ReadFile("multi-app/hasura/metadata.json"); err == nil { + t.Error("multi-app ships hasura/metadata.json; tables.yaml is the one source") } - var fromYAML interface{} - if err := yaml.Unmarshal(raw, &fromYAML); err != nil { - t.Fatal(err) + tables := maTables(t) + if len(tables) != 3 { + t.Errorf("tables.yaml describes %d tables, want notes, projects, app_members", len(tables)) } - jsonTables := []map[string]json.RawMessage{} for _, name := range []string{"notes", "projects", "app_members"} { - jsonTables = append(jsonTables, maTables(t)[name]) - } - b, _ := json.Marshal(jsonTables) - if !reflect.DeepEqual(fromYAML, maDecode(t, b)) { - t.Errorf("tables.yaml and metadata.json differ:\nyaml: %v\njson: %s", fromYAML, b) + if tables[name] == nil { + t.Errorf("tables.yaml has no %s table", name) + } } } diff --git a/internal/templates/clone/multi-app/README.md b/internal/templates/clone/multi-app/README.md index 7ccb7d5fb..8a35b664c 100644 --- a/internal/templates/clone/multi-app/README.md +++ b/internal/templates/clone/multi-app/README.md @@ -57,8 +57,7 @@ nself db reconcile --apply # pushes it Reconcile sends targeted per-object calls and never replaces the whole Hasura metadata document, so hasura-auth's own tables stay tracked. Do not run `nself db hasura metadata apply` here. The metadata lives in -`hasura/metadata/databases/default/tables/tables.yaml`; `hasura/metadata.json` -holds the same tables. +`hasura/metadata/databases/default/tables/tables.yaml`. Add a member with the admin secret: diff --git a/internal/templates/clone/multi-app/hasura/metadata.json b/internal/templates/clone/multi-app/hasura/metadata.json deleted file mode 100644 index 988aae358..000000000 --- a/internal/templates/clone/multi-app/hasura/metadata.json +++ /dev/null @@ -1,93 +0,0 @@ -{ - "version": 3, - "sources": [ - { - "name": "default", - "tables": [ - { - "table": {"schema": "public", "name": "notes"}, - "select_permissions": [ - { - "role": "user", - "permission": { - "columns": ["id", "source_account_id", "title", "created_at"], - "filter": {"_and": [{"source_account_id": {"_eq": "X-Hasura-Source-Account-Id"}}, {"_exists": {"_table": {"schema": "public", "name": "app_members"}, "_where": {"user_id": {"_eq": "X-Hasura-User-Id"}, "source_account_id": {"_eq": "X-Hasura-Source-Account-Id"}}}}]} - } - } - ], - "insert_permissions": [ - { - "role": "user", - "permission": { - "check": {"_and": [{"source_account_id": {"_eq": "X-Hasura-Source-Account-Id"}}, {"_exists": {"_table": {"schema": "public", "name": "app_members"}, "_where": {"user_id": {"_eq": "X-Hasura-User-Id"}, "source_account_id": {"_eq": "X-Hasura-Source-Account-Id"}}}}]}, - "columns": ["title"], - "set": {"source_account_id": "X-Hasura-Source-Account-Id"} - } - } - ], - "update_permissions": [ - { - "role": "user", - "permission": { - "columns": ["title"], - "filter": {"_and": [{"source_account_id": {"_eq": "X-Hasura-Source-Account-Id"}}, {"_exists": {"_table": {"schema": "public", "name": "app_members"}, "_where": {"user_id": {"_eq": "X-Hasura-User-Id"}, "source_account_id": {"_eq": "X-Hasura-Source-Account-Id"}}}}]}, - "check": {"_and": [{"source_account_id": {"_eq": "X-Hasura-Source-Account-Id"}}, {"_exists": {"_table": {"schema": "public", "name": "app_members"}, "_where": {"user_id": {"_eq": "X-Hasura-User-Id"}, "source_account_id": {"_eq": "X-Hasura-Source-Account-Id"}}}}]} - } - } - ], - "delete_permissions": [ - { - "role": "user", - "permission": { - "filter": {"_and": [{"source_account_id": {"_eq": "X-Hasura-Source-Account-Id"}}, {"_exists": {"_table": {"schema": "public", "name": "app_members"}, "_where": {"user_id": {"_eq": "X-Hasura-User-Id"}, "source_account_id": {"_eq": "X-Hasura-Source-Account-Id"}}}}]} - } - } - ] - }, - { - "table": {"schema": "public", "name": "projects"}, - "select_permissions": [ - { - "role": "user", - "permission": { - "columns": ["id", "source_account_id", "title", "name", "created_at"], - "filter": {"_and": [{"source_account_id": {"_eq": "X-Hasura-Source-Account-Id"}}, {"_exists": {"_table": {"schema": "public", "name": "app_members"}, "_where": {"user_id": {"_eq": "X-Hasura-User-Id"}, "source_account_id": {"_eq": "X-Hasura-Source-Account-Id"}}}}]} - } - } - ], - "insert_permissions": [ - { - "role": "user", - "permission": { - "check": {"_and": [{"source_account_id": {"_eq": "X-Hasura-Source-Account-Id"}}, {"_exists": {"_table": {"schema": "public", "name": "app_members"}, "_where": {"user_id": {"_eq": "X-Hasura-User-Id"}, "source_account_id": {"_eq": "X-Hasura-Source-Account-Id"}}}}]}, - "columns": ["title", "name"], - "set": {"source_account_id": "X-Hasura-Source-Account-Id"} - } - } - ], - "update_permissions": [ - { - "role": "user", - "permission": { - "columns": ["title", "name"], - "filter": {"_and": [{"source_account_id": {"_eq": "X-Hasura-Source-Account-Id"}}, {"_exists": {"_table": {"schema": "public", "name": "app_members"}, "_where": {"user_id": {"_eq": "X-Hasura-User-Id"}, "source_account_id": {"_eq": "X-Hasura-Source-Account-Id"}}}}]}, - "check": {"_and": [{"source_account_id": {"_eq": "X-Hasura-Source-Account-Id"}}, {"_exists": {"_table": {"schema": "public", "name": "app_members"}, "_where": {"user_id": {"_eq": "X-Hasura-User-Id"}, "source_account_id": {"_eq": "X-Hasura-Source-Account-Id"}}}}]} - } - } - ], - "delete_permissions": [ - { - "role": "user", - "permission": { - "filter": {"_and": [{"source_account_id": {"_eq": "X-Hasura-Source-Account-Id"}}, {"_exists": {"_table": {"schema": "public", "name": "app_members"}, "_where": {"user_id": {"_eq": "X-Hasura-User-Id"}, "source_account_id": {"_eq": "X-Hasura-Source-Account-Id"}}}}]} - } - } - ] - }, - { - "table": {"schema": "public", "name": "app_members"} - } - ] - } - ] -} diff --git a/internal/templates/clone/multi-app/hasura/metadata/databases/default/tables/tables.yaml b/internal/templates/clone/multi-app/hasura/metadata/databases/default/tables/tables.yaml index be40c7c23..1b3061bd4 100644 --- a/internal/templates/clone/multi-app/hasura/metadata/databases/default/tables/tables.yaml +++ b/internal/templates/clone/multi-app/hasura/metadata/databases/default/tables/tables.yaml @@ -1,4 +1,4 @@ -# Hasura table metadata for nself db reconcile. Keep in sync with hasura/metadata.json. +# Hasura table metadata for nself db reconcile. - table: schema: "public" name: "notes" diff --git a/scripts/e2e/isolation-guard.sh b/scripts/e2e/isolation-guard.sh index a2e359197..5a947238f 100755 --- a/scripts/e2e/isolation-guard.sh +++ b/scripts/e2e/isolation-guard.sh @@ -45,7 +45,7 @@ cd "$DIR" nself init --non-interactive --name "$NAME" --no-pgvector >>"$LOG" 2>&1 || fail 'init' owned=1 nself init --template multi-app . --force >"$W/overlay.out" 2>>"$LOG" || fail 'template overlay' -for f in README.md manifest.json nself.yml .env.example migrations/001_multi_app.sql hasura/metadata.json hasura/metadata/databases/default/tables/tables.yaml; do +for f in README.md manifest.json nself.yml .env.example migrations/001_multi_app.sql hasura/metadata/databases/default/tables/tables.yaml; do test -f "$f" || fail "overlay did not write $f" done test ! -e flutter || fail 'overlay wrote a flutter/ folder' diff --git a/scripts/e2e/multi-app-isolation.sh b/scripts/e2e/multi-app-isolation.sh index db98f18bb..68be997be 100755 --- a/scripts/e2e/multi-app-isolation.sh +++ b/scripts/e2e/multi-app-isolation.sh @@ -46,7 +46,7 @@ cd "$DIR" nself init --non-interactive --name "$NAME" --no-pgvector >>"$LOG" 2>&1 || fail 'init' owned=1 nself init --template multi-app . --force >"$W/overlay.out" 2>>"$LOG" || fail 'template overlay' -for f in README.md manifest.json nself.yml .env.example migrations/001_multi_app.sql hasura/metadata.json hasura/metadata/databases/default/tables/tables.yaml; do +for f in README.md manifest.json nself.yml .env.example migrations/001_multi_app.sql hasura/metadata/databases/default/tables/tables.yaml; do test -f "$f" || fail "overlay did not write $f" done test ! -e flutter || fail 'overlay wrote a flutter/ folder' diff --git a/scripts/ops/credential-rotation-check.sh b/scripts/ops/credential-rotation-check.sh index c29342abd..c997d760d 100755 --- a/scripts/ops/credential-rotation-check.sh +++ b/scripts/ops/credential-rotation-check.sh @@ -98,7 +98,7 @@ Summary: 1. Verify backup admin is available (see [\`bus-factor.md\`](../docs/operations/bus-factor.md)). 2. Generate new credential at vendor console (parallel to existing). -3. Stage as \`${credential}_NEW\` in \`~/.claude/vault.env\`. +3. Stage as \`${credential}_NEW\` in your credential store. 4. Roll dependents (Vercel env, GH Actions secrets, Hetzner \`.env.secrets\`, CRD config). 5. Verify new credential works via a real action. 6. Revoke old credential at vendor. @@ -107,7 +107,7 @@ Summary: ## Vault Variable -Update in \`~/.claude/vault.env\`: +Update in your secrets file: \`\`\`bash ${credential}=