amadan
registry, cli: unit-backed and unit-named is refused at every door
Review IMPORTANT 1 and 2. unitRail refused a route that was both unit-backed and unit-named, but only from one side and only on one verb, so two orders reached the forbidden state anyway: `carlos add -backing unit -unit X` minted it outright, and `carlos route -unit X` then `-backing unit` walked into it. Either way the dispatch honours the name and the carlos-app@<host> unit is never cycled by adoption again — issue #110 recreated for an ordinary tenant. runAdd now refuses the combination in unitRail's own words, and backingRail takes the target value so it can refuse the move TO unit backing on a stamped row while leaving the move away from it legal (a row that somehow reached the state must have a way back out). Minors: unitNameRe bounds the stem (first char [A-Za-z0-9@_], ≤64 before .service), so `-x.service` — a name that reads as a flag in any argv built from this column — and `..service` are refused, with cases added at the bound and one over it. Route.Unit's doc comment no longer claims Upsert validates: Upsert validates no column at all (SetCaps's rule, rails at the door), so the convention is that a full-row caller has already run ValidUnitName, which `carlos add -unit` does at the flag.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4 files changed,
+138
−28
cli.go+29 −2cli_test.go+72 −8internal/registry/registry.go+16 −4internal/registry/registry_test.go+21 −14
diff --git a/cli.go b/cli.go| index 246cb681b..e49017e48 100644 |
| --- a/cli.go |
| +++ b/cli.go |
| @@ -99,6 +99,18 @@ func runAdd(args []string) { |
| if !registry.ValidUnitName(*unit) { |
| log.Fatalf("add %s: unit %q: want a systemd service unit name (e.g. console.service)", host, *unit) |
| } |
| + // The same combination `carlos route -unit` refuses (unitRail), refused at |
| + // the OTHER door onto these two columns — and this is the door that could |
| + // mint the row from nothing. A route that is both unit-backed and |
| + // unit-named is two answers to one question, and the dispatch resolves it |
| + // by honouring the name: the carlos-app@<host> unit would then never be |
| + // cycled by adoption again, which is issue #110 recreated for an ordinary |
| + // tenant. Same wording as unitRail's, so an operator who meets it at one |
| + // door recognises it at the other. |
| + if *unit != "" && *backing == registry.BackingUnit { |
| + log.Fatalf("add %s: -backing unit with -unit %s: a unit-backed route's process is the carlos-app@%s unit the platform derives and already restarts, "+ |
| + "so a second unit name here would be two answers to one question — drop one of the two flags", host, *unit, host) |
| + } |
| if *socket == "" && *addr == "" && *kind == "instance" { |
| if *backing == registry.BackingUnit { |
| // The carlos-app@<host>.socket unit's path — enable that unit |
| @@ -1190,6 +1202,10 @@ func runRouteHibernate(r *registry.Registry, rt registry.Route, on, dryRun bool) |
| // single-route axes. |
| // - kind != "instance" is refused: only an instance has a process for |
| // anything to back. A service or static route has no ExecStart to move. |
| +// - a UNIT-STAMPED row may not be moved TO unit backing. That is the same |
| +// forbidden state unitRail guards from the other side, reached in the |
| +// other order (`-unit X` first, `-backing unit` second), and it has to be |
| +// refused at both ends or the pair is only half-guarded. |
| // - the value is "" or BackingUnit, checked at the flag before the |
| // registry is even opened. |
| // |
| @@ -1199,7 +1215,7 @@ func runRouteHibernate(r *registry.Registry, rt registry.Route, on, dryRun bool) |
| // and it prints what the next adopt pass will do with the new value so the |
| // operator can decide whether they are ready for it. |
| func runRouteBacking(r *registry.Registry, rt registry.Route, backing string, dryRun bool) { |
| - if err := backingRail(rt); err != nil { |
| + if err := backingRail(rt, backing); err != nil { |
| log.Fatalf("route: %s: %v", rt.Host, err) |
| } |
| prefix := "" |
| @@ -1238,7 +1254,18 @@ func runRouteBacking(r *registry.Registry, rt registry.Route, backing string, dr |
| // rows — it is meaningless. A platform alias is a second name for one |
| // process, and a second carlos-app@ unit under it would claim ports and a |
| // state directory the real route already owns. |
| -func backingRail(rt registry.Route) error { |
| +// |
| +// It takes the TARGET value as well as the row because one of its refusals |
| +// is directional: moving a unit-stamped row TO unit backing creates the state |
| +// unitRail forbids from the other side, while moving one AWAY from unit |
| +// backing is always fine (it can only reduce the number of things claiming to |
| +// own the process). Refusing both directions would strand any row that |
| +// somehow reached the forbidden state with no way back out. |
| +func backingRail(rt registry.Route, backing string) error { |
| + if backing == registry.BackingUnit && rt.Unit != "" { |
| + return fmt.Errorf("route names the unit %s — a unit-backed route's process is the carlos-app@%s unit the platform derives and already restarts, "+ |
| + "so a second unit name would be two answers to one question. Clear the stamp first: `carlos route -host %s -unit -`", rt.Unit, rt.Host, rt.Host) |
| + } |
| if rt.Platform { |
| return fmt.Errorf("Platform route — a platform alias is a second name for one process, so a unit of its own would " + |
| "claim ports and a state directory the real route already holds; and the domains reconciler rebuilds these rows " + |
diff --git a/cli_test.go b/cli_test.go| index 6528698f2..714015d05 100644 |
| --- a/cli_test.go |
| +++ b/cli_test.go |
| @@ -413,27 +413,56 @@ func TestBackingRailRefusals(t *testing.T) { |
| } |
| for _, c := range cases { |
| t.Run(c.name, func(t *testing.T) { |
| - err := backingRail(c.rt) |
| - if err == nil { |
| - t.Fatalf("backingRail(%+v) accepted; want a refusal", c.rt) |
| - } |
| - if !strings.Contains(err.Error(), c.says) { |
| - t.Fatalf("refusal %q does not say %q", err, c.says) |
| + // Row-shape refusals hold whichever value is being written, so |
| + // both targets are exercised. |
| + for _, target := range []string{registry.BackingUnit, ""} { |
| + err := backingRail(c.rt, target) |
| + if err == nil { |
| + t.Fatalf("backingRail(%+v, %q) accepted; want a refusal", c.rt, target) |
| + } |
| + if !strings.Contains(err.Error(), c.says) { |
| + t.Fatalf("refusal %q does not say %q", err, c.says) |
| + } |
| } |
| }) |
| } |
| // Both directions of the migration are the same verb on the same row, so |
| // both must clear the rail from either starting value. |
| - if err := backingRail(instance); err != nil { |
| + if err := backingRail(instance, registry.BackingUnit); err != nil { |
| t.Fatalf("backingRail(exec-backed instance) = %v, want accepted", err) |
| } |
| unit := instance |
| unit.Backing = registry.BackingUnit |
| - if err := backingRail(unit); err != nil { |
| + if err := backingRail(unit, ""); err != nil { |
| t.Fatalf("backingRail(unit-backed instance) = %v, want accepted", err) |
| } |
| } |
| +// The pair rail, from backingRail's side: `carlos route -unit X` followed by |
| +// `carlos route -backing unit` reaches exactly the state unitRail forbids in |
| +// the other order, and a rail guarded at one end only is not guarded. The |
| +// refusal is DIRECTIONAL — moving a stamped row AWAY from unit backing stays |
| +// legal, or a row that somehow reached the forbidden state would have no way |
| +// back out. |
| +func TestBackingRailRefusesUnitBackingOnAStampedRow(t *testing.T) { |
| + stamped := registry.Route{ |
| + Host: "console.carloku.com", Kind: "instance", App: "console", |
| + Addr: "127.0.0.1:8090", Unit: "console.service", |
| + } |
| + err := backingRail(stamped, registry.BackingUnit) |
| + if err == nil { |
| + t.Fatal("backingRail(unit-stamped row, backing=unit) accepted; want a refusal") |
| + } |
| + // The operator has to be told which flag to undo, or the refusal is a |
| + // dead end. |
| + if !strings.Contains(err.Error(), "-unit -") { |
| + t.Errorf("refusal %q does not name the clear command", err) |
| + } |
| + if err := backingRail(stamped, ""); err != nil { |
| + t.Fatalf("backingRail(unit-stamped row, backing=\"\") = %v, want accepted — the way out must stay open", err) |
| + } |
| +} |
| + |
| // TestUnitRailRefusals: `carlos route -unit` is the stamp issue #110 needed, |
| // and its one structural refusal is a unit-backed route — that route's |
| // process is the carlos-app@<host> unit the platform derives and already |
| @@ -585,6 +614,41 @@ func TestRouteUnitStampsAnExistingRoute(t *testing.T) { |
| t.Fatalf("Unit = %q after -unit -, want it cleared", got) |
| } |
| + // The pair rail through the real binary, in both orders. Neither door may |
| + // mint a row that is unit-BACKED and unit-NAMED at once: the dispatch |
| + // honours the name, so the carlos-app@<host> unit would never be cycled |
| + // by adoption again — issue #110 recreated for an ordinary tenant. |
| + out, err = run("add", "-registry", regPath, "-app", "kass", "-backing", "unit", |
| + "-unit", "bespoke.service", "-db", filepath.Join(tmp, "t.db"), "pair.example.com") |
| + if err == nil { |
| + t.Fatalf("add -backing unit -unit bespoke.service: want non-zero exit, got 0\n%s", out) |
| + } |
| + if !strings.Contains(out, "carlos-app@") { |
| + t.Errorf("add refusal = %q, want it to name the unit that already owns the process", out) |
| + } |
| + if out, err := run("add", "-registry", regPath, "-app", "kass", "-backing", "unit", |
| + "-db", filepath.Join(tmp, "t.db"), "tenant.example.com"); err != nil { |
| + t.Fatalf("add unit tenant: %v\n%s", err, out) |
| + } |
| + out, err = run("route", "-registry", regPath, "-host", "tenant.example.com", "-unit", "bespoke.service") |
| + if err == nil { |
| + t.Fatalf("route -unit on a unit-backed route: want non-zero exit, got 0\n%s", out) |
| + } |
| + if out, err := run("route", "-registry", regPath, "-host", "console.carloku.com", |
| + "-unit", "console.service"); err != nil { |
| + t.Fatalf("re-stamp console: %v\n%s", err, out) |
| + } |
| + out, err = run("route", "-registry", regPath, "-host", "console.carloku.com", "-backing", "unit") |
| + if err == nil { |
| + t.Fatalf("route -backing unit on a unit-stamped route: want non-zero exit, got 0\n%s", out) |
| + } |
| + if !strings.Contains(out, "-unit -") { |
| + t.Errorf("backing refusal = %q, want it to name the clear command", out) |
| + } |
| + if out, err := run("route", "-registry", regPath, "-host", "console.carloku.com", "-unit", "-"); err != nil { |
| + t.Fatalf("clear the stamp again: %v\n%s", err, out) |
| + } |
| + |
| // The exclusions: each of these is a command that would otherwise do part |
| // of what was typed and report success. |
| for _, c := range []struct { |
diff --git a/internal/registry/registry.go b/internal/registry/registry.go| index aa87db238..fc5bf7520 100644 |
| --- a/internal/registry/registry.go |
| +++ b/internal/registry/registry.go |
| @@ -120,9 +120,13 @@ type Route struct { |
| // X-Carlos-Version header — and cycled nothing, so the old process went |
| // on serving a build the header already claimed was live (issue #110). |
| // |
| - // Written only by SetUnit / `carlos add -unit` / `carlos route -unit`, |
| - // each of which validates the name (ValidUnitName); Upsert preserves a |
| - // non-empty existing value — see Environment's comment for why. |
| + // Written by SetUnit, which validates the name, and by Upsert, which does |
| + // not — Upsert validates no column at all (SetCaps's rule: the rails live |
| + // at the door), so the convention is that a full-row caller has already |
| + // run ValidUnitName. `carlos add -unit` is the only such caller today and |
| + // does exactly that, at the flag, before the registry is opened. Upsert |
| + // preserves a non-empty existing value — see Environment's comment for |
| + // why. |
| // |
| // Deliberately NOT in Fingerprint(): unit changes who restarts the |
| // process, not what the edge serves. |
| @@ -208,10 +212,18 @@ func ValidTunnelName(name string) bool { |
| // the '@' of a template instance, so `console.service` and |
| // `titogo-canary@demo.service` both pass and a typo does not. |
| // |
| +// The FIRST character is narrower than the rest, and bounded like |
| +// ValidTunnelName's 32 (64 here, because unit names legitimately carry a |
| +// prefix and an instance): a leading '-' is how a name gets mistaken for a |
| +// flag by anything that ever builds an argv from this column, and a leading |
| +// '.' admits `..service`, `..`-shaped names and other things that read as |
| +// paths rather than units. Neither is a name anybody means to type, and both |
| +// are cheaper to refuse than to reason about later. |
| +// |
| // Lives here rather than in the CLI for the same reason ValidTunnelName does: |
| // `unit` is this package's column, and this is the shape SetUnit enforces on |
| // every write, not merely the shape one door happens to check. |
| -var unitNameRe = regexp.MustCompile(`^[A-Za-z0-9@_.\-]+\.service$`) |
| +var unitNameRe = regexp.MustCompile(`^[A-Za-z0-9@_][A-Za-z0-9@_.\-]{0,63}\.service$`) |
| // ValidUnitName reports whether name is usable as a route's Unit binding. |
| // Unlike ValidTunnelName, "" is VALID here: it is the column's default and |
diff --git a/internal/registry/registry_test.go b/internal/registry/registry_test.go| index d027979ad..dc6a24ff2 100644 |
| --- a/internal/registry/registry_test.go |
| +++ b/internal/registry/registry_test.go |
| @@ -1505,22 +1505,29 @@ func TestSetUnitRefusesAMalformedName(t *testing.T) { |
| func TestValidUnitName(t *testing.T) { |
| valid := []string{ |
| - "", // the column's default: no unit |
| - "console.service", // |
| - "keymail-relay.service", // hyphens |
| - "titogo-canary@demo.service", // template instance |
| - "carlos_app.service", // underscore |
| - "a.b.c.service", // dots |
| + "", // the column's default: no unit |
| + "console.service", // |
| + "keymail-relay.service", // hyphens, just not leading |
| + "titogo-canary@demo.service", // template instance |
| + "carlos_app.service", // underscore |
| + "a.b.c.service", // dots, just not leading |
| + "a.service", // one-character stem |
| + strings.Repeat("a", 64) + ".service", // the length bound itself |
| } |
| invalid := []string{ |
| - "console", // no suffix |
| - "console.servcie", // the typo this rail is for |
| - "carlos-adopt.timer", // a unit, but not one that owns a process |
| - "console.socket", // |
| - "has space.service", // space |
| - "../../etc/x.service", // path traversal |
| - "console.service; reboot", // shell-shaped, and refused as a shape |
| - ".service", // suffix alone |
| + "console", // no suffix |
| + "console.servcie", // the typo this rail is for |
| + "carlos-adopt.timer", // a unit, but not one that owns a process |
| + "console.socket", // |
| + "has space.service", // space |
| + "../../etc/x.service", // path traversal |
| + "console.service; reboot", // shell-shaped, and refused as a shape |
| + ".service", // suffix alone |
| + "-x.service", // leading dash: reads as a flag in any argv built from this column |
| + "..service", // dot-only stem |
| + "...service", // |
| + ".x.service", // leading dot |
| + strings.Repeat("a", 65) + ".service", // one over the bound |
| } |
| for _, n := range valid { |
| if !ValidUnitName(n) { |