amadan
config writes: say what got the file and will never read it (WIP: copy pending review)
Discussion #16: `carlos secrets set` and `carlos env set` succeed, the merged bundle is materialized to /data/<host>.config.env, and `secrets list` confirms delivery — while the process on the other end goes on serving the old values until somebody restarts it by hand. Nothing in the loop says so. The reporter's symptom was an OAuth provider silently not offered, and they had it written down twice in their own runbook, in bold, with "nothing will remind you" beside it. This is the cheapest of their three asks: say it in the output. It is NOT a request to have carlos own the restart, and this does not — a TCP-addr exec route's lifecycle stays the app's. appconfig.AwaitingRestart answers, for one write, which of a box's routes were handed a file nothing will bounce. It sits beside execAddrCandidate and resolves the exec_delivery opt-in through the same resolveExecDelivery that SyncExec's checkOptedIn now uses, so the notice and the sweep cannot disagree about who gets a file. Three states, not two: hosts delivered to, hosts nothing could be determined about (their bundle would not read, or the bundle they are bound to is missing), and — at the console and CLI layer — "no claim was made" for a console that predates the field. Rendering "could not tell" as "all clear" is the bug being fixed; reproducing it inside the fix would be a poor joke. A default-bundle write is narrowed by the keys it touched: a bound route whose own bundle shadows every one of them resolves to a byte-identical manifest, syncExecHost writes nothing, and naming it would send someone to restart a process for a value it was never going to see. Route.Unit is carried through because it decides the REMEDY. An addr route naming its own unit (issue #110's consoles) is the one addr shape `carlos restart` can cycle; for a plain one the agent's restart leg skips it and the activator has no child for it, so pointing an operator at that command would be a second silent failure answering the first. Also the aside from the same report: `secrets set` stored an empty value without complaint. Values arrive as "$(op read ...)", `op read` on a missing field prints nothing and exits 0, so a typo delivered two empty strings and reported success. Refused now on both sides of the seal — in the CLI on the plaintext, and at the console door on the envelope's LENGTH, since AES-GCM is length-preserving and an envelope at exactly ephLen+nonceLen+16 sealed nothing. The door check is ordered after the structural ones so a 93-byte run of garbage is still told its ephemeral key is invalid rather than that it seals an empty value. accountFlagsWithRegistry exists so the five config verbs read the registry --registry actually names, not defaultRegistry(), which would answer "which routes did this reach" from the wrong box. WIP: the six user-facing strings are TODO(copy-review) placeholders pending Paul's review; tests assert structure, never wording.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CpFEzjVAS2unze59dFTLZN
11 files changed,
+1351
−81
apimode.go+46 −8apimode_test.go+9 −3appconfig_cli.go+222 −20appconfig_restart_test.go+138 −0cli.go+17 −2internal/appconfig/awaitingrestart_test.go+263 −0internal/appconfig/crypto.go+31 −0internal/appconfig/crypto_test.go+58 −5internal/appconfig/syncexec.go+274 −42internal/console/apiconfig.go+85 −1internal/console/apiconfigrestart_test.go+208 −0
diff --git a/apimode.go b/apimode.go| index 91d190316..0267d6b43 100644 |
| --- a/apimode.go |
| +++ b/apimode.go |
| @@ -1897,22 +1897,60 @@ type apiConfigApply struct { |
| // the server's count of unsets ACTUALLY present, never the argument count — |
| // the same rule unsetVars already follows for direct mode (a count of |
| // arguments reads as success for a typo'd key). |
| -func (s *apiSession) configApply(ctx context.Context, a apiAcct, app string, req apiConfigApply) (int, error) { |
| +func (s *apiSession) configApply(ctx context.Context, a apiAcct, app string, req apiConfigApply) (apiConfigApplyResult, error) { |
| status, body, err := s.putJSON(ctx, apiPath(a, app, "/config"), req) |
| if err != nil { |
| - return 0, err |
| + return apiConfigApplyResult{}, err |
| } |
| if status != http.StatusOK { |
| - return 0, apiErrorFromBody(status, body) |
| + return apiConfigApplyResult{}, apiErrorFromBody(status, body) |
| } |
| var res struct { |
| - Result string `json:"result"` |
| - Removed int `json:"removed"` |
| + Result string `json:"result"` |
| + Removed int `json:"removed"` |
| + Restart *apiConfigRestart `json:"restart"` |
| } |
| if err := json.Unmarshal(body, &res); err != nil { |
| - return 0, fmt.Errorf("config: unreadable response: %w", err) |
| - } |
| - return res.Removed, nil |
| + return apiConfigApplyResult{}, fmt.Errorf("config: unreadable response: %w", err) |
| + } |
| + return apiConfigApplyResult{Removed: res.Removed, Restart: res.Restart}, nil |
| +} |
| + |
| +// apiConfigApplyResult is one config write's answer — a struct rather than |
| +// a bare count for the reason apiPromoteResult is one: a second thing worth |
| +// returning arrived, and growing a third return value would have meant |
| +// touching every call site to ignore it. |
| +type apiConfigApplyResult struct { |
| + // Removed is how many vars the unset half ACTUALLY removed, which is |
| + // not necessarily how many were asked for. |
| + Removed int |
| + // Restart is what the write landed on that will not read it, and it |
| + // has three states, all meaningful. NIL means the console never |
| + // mentioned it — a console older than this field, whose silence must |
| + // not be rendered as "nothing to restart", because it is not a claim |
| + // at all (the same precedent apiRetentionResult.Eligible sets for a |
| + // console that could not compute a number). Present with Known false |
| + // means the console looked and could not tell. Present with Known true |
| + // means these are the routes, and an empty Hosts there really does mean |
| + // none. |
| + Restart *apiConfigRestart |
| +} |
| + |
| +// apiConfigRestart mirrors the console's own restart notice across the wire |
| +// (internal/console/apiconfig.go). Additive: an older console omits it |
| +// entirely and a newer one may add fields this binary ignores. |
| +type apiConfigRestart struct { |
| + Known bool `json:"known"` |
| + Hosts []apiConfigRestartHost `json:"hosts"` |
| + Unresolved []string `json:"unresolved"` |
| +} |
| + |
| +// apiConfigRestartHost is one delivered-to route. Unit decides which remedy |
| +// the renderer offers, so it is carried rather than derived: a plain |
| +// TCP-addr route is one `carlos restart` cannot cycle at all. |
| +type apiConfigRestartHost struct { |
| + Host string `json:"host"` |
| + Unit string `json:"unit"` |
| } |
| // configSync asks the console to touch app's generation object — the remote |
diff --git a/apimode_test.go b/apimode_test.go| index 6a79e8dfe..c459e9004 100644 |
| --- a/apimode_test.go |
| +++ b/apimode_test.go |
| @@ -2873,7 +2873,7 @@ func TestAPISessionConfigApplyUnsetBody(t *testing.T) { |
| defer ts.Close() |
| s := newTestSession(ts.URL) |
| - removed, err := s.configApply(context.Background(), apiAcct{Seg: "acct"}, "app", apiConfigApply{ |
| + res, err := s.configApply(context.Background(), apiAcct{Seg: "acct"}, "app", apiConfigApply{ |
| Environment: "", |
| Unset: []string{"OLD_KEY", "ALSO_MISSING"}, |
| }) |
| @@ -2885,8 +2885,14 @@ func TestAPISessionConfigApplyUnsetBody(t *testing.T) { |
| } |
| // removed is the server's ACTUAL count, never the argument count — two |
| // keys were asked for, only one was really removed. |
| - if removed != 1 { |
| - t.Fatalf("removed = %d, want 1 (the server's count, not len(Unset)==2)", removed) |
| + if res.Removed != 1 { |
| + t.Fatalf("removed = %d, want 1 (the server's count, not len(Unset)==2)", res.Removed) |
| + } |
| + // A response with no "restart" key at all leaves Restart nil — the |
| + // "console said nothing" state, which the CLI renders as silence rather |
| + // than as "nothing needs restarting". |
| + if res.Restart != nil { |
| + t.Fatalf("Restart = %+v, want nil for a response that omits the field", res.Restart) |
| } |
| } |
diff --git a/appconfig_cli.go b/appconfig_cli.go| index 1722a6980..043fe8599 100644 |
| --- a/appconfig_cli.go |
| +++ b/appconfig_cli.go |
| @@ -8,15 +8,148 @@ import ( |
| "crypto/ecdh" |
| "flag" |
| "fmt" |
| + "io" |
| "log" |
| "os" |
| "sort" |
| "strings" |
| "github.com/carlosframework/platform/internal/appconfig" |
| + "github.com/carlosframework/platform/internal/registry" |
| "github.com/carlosframework/platform/internal/release" |
| ) |
| +// restartAnswer is what one config write can be said to have left behind |
| +// on the box, and it exists to keep THREE outcomes apart where a bool or an |
| +// empty slice would keep two. |
| +// |
| +// A nil *restartAnswer means "no claim was made" — an older console that |
| +// does not send the field, or a direct-mode run with no registry anywhere |
| +// near it. Nothing is printed: silence about a question nobody was in a |
| +// position to ask is honest, where a "could not check" line printed on |
| +// every laptop-side `env set` would be noise that teaches people to skip |
| +// the line that matters. |
| +// |
| +// known == false means the opposite: someone WAS in a position to look and |
| +// could not. That gets said out loud. Conflating it with "all clear" is |
| +// discussion #16's bug, and reproducing it inside the fix would be a poor |
| +// joke. |
| +type restartAnswer struct { |
| + known bool |
| + pending appconfig.RestartPending |
| +} |
| + |
| +// restartFromAPI converts the console's answer, preserving the nil case. |
| +func restartFromAPI(r *apiConfigRestart) *restartAnswer { |
| + if r == nil { |
| + return nil |
| + } |
| + out := &restartAnswer{known: r.Known} |
| + out.pending.Unresolved = r.Unresolved |
| + for _, h := range r.Hosts { |
| + out.pending.Hosts = append(out.pending.Hosts, appconfig.RestartHost{Host: h.Host, Unit: h.Unit}) |
| + } |
| + return out |
| +} |
| + |
| +// localRestartAnswer is the direct-mode (on-box, CARLOS_DEPLOYMENT_BUCKET) |
| +// half: the same question asked of this box's own registry file. |
| +// |
| +// A registry that is not THERE returns nil rather than known:false. Direct |
| +// mode is equally the mode an operator runs from a laptop against the |
| +// bucket, where no registry exists, none is expected, and there is no box |
| +// in the picture to have routes on. A registry that exists but will not |
| +// open or list is the other thing entirely — a box that should be able to |
| +// answer and cannot — and that one says so. |
| +// |
| +// Deliberately os.Stat before registry.Open: Open CREATES an empty database |
| +// at a path that has none (see SyncExec's own note on this), so opening |
| +// optimistically would answer "no routes, nothing to restart" with complete |
| +// confidence, having just minted the empty file it read that from. |
| +func localRestartAnswer(ctx context.Context, s release.Store, regPath, acct, app, env string, keys []string) *restartAnswer { |
| + path := regPath |
| + if _, err := os.Stat(path); err != nil { |
| + return nil |
| + } |
| + reg, err := registry.Open(path) |
| + if err != nil { |
| + return &restartAnswer{} |
| + } |
| + defer reg.Close() |
| + routes, err := reg.List() |
| + if err != nil { |
| + return &restartAnswer{} |
| + } |
| + return &restartAnswer{known: true, pending: appconfig.AwaitingRestart(ctx, s, routes, acct, app, env, keys)} |
| +} |
| + |
| +// printRestartNotice writes the line discussion #16 was filed for: the |
| +// config write landed, and here is what is still running without it. |
| +// |
| +// One renderer for all four writing verbs (env set/unset, secrets |
| +// set/unset) and both modes, because the thing being reported is a property |
| +// of the write rather than of the verb — and because four hand-written |
| +// variants of one sentence is how they end up disagreeing about which |
| +// routes count. |
| +func printRestartNotice(w io.Writer, app string, a *restartAnswer) { |
| + if a == nil { |
| + return |
| + } |
| + c := colorFor(w) |
| + if !a.known { |
| + fmt.Fprintf(w, "%s %s\n", c.warn("note:"), restartCopyUnknown) |
| + return |
| + } |
| + if a.pending.Empty() { |
| + return |
| + } |
| + for _, h := range a.pending.Hosts { |
| + line := fmt.Sprintf(restartCopyPlain, h.Host) |
| + if h.Unit != "" { |
| + line = fmt.Sprintf(restartCopyUnit, h.Host, app, h.Unit) |
| + } |
| + fmt.Fprintf(w, "%s %s %s\n", c.warn("note:"), line, c.muted(restartCopyScope)) |
| + } |
| + for _, h := range a.pending.Unresolved { |
| + fmt.Fprintf(w, "%s %s\n", c.warn("note:"), fmt.Sprintf(restartCopyUnresolved, h)) |
| + } |
| +} |
| + |
| +// checkNoEmptyValues refuses a `secrets set` carrying any empty value, and |
| +// is a returned error rather than an inline log.Fatalf so that it can be |
| +// tested for what it says as well as for whether it fires. |
| +// |
| +// Keys are named, values never — parseKV's rule, for the same reason: a |
| +// mistyped `secrets set` argument IS a secret, and this text lands in stderr |
| +// and in shell scrollback. All of them at once, sorted, because a caller who |
| +// mistyped one field name has usually mistyped several and Go's map order |
| +// would otherwise pick a different one to complain about each run. |
| +func checkNoEmptyValues(kv map[string]string) error { |
| + var empty []string |
| + for k, v := range kv { |
| + if v == "" { |
| + empty = append(empty, k) |
| + } |
| + } |
| + if len(empty) == 0 { |
| + return nil |
| + } |
| + sort.Strings(empty) |
| + return fmt.Errorf(secretsCopyEmpty, strings.Join(empty, ", ")) |
| +} |
| + |
| +// The notice's words, in one block because they are one message in four |
| +// shapes and were reviewed as one. The verb sites format these; they do not |
| +// write their own. |
| +const ( |
| + secretsCopyEmpty = "TODO(copy-review) %s: empty" |
| + restartCopyPlain = "TODO(copy-review) %s: plain" |
| + restartCopyUnit = "TODO(copy-review) %s %s %s: unit" |
| + restartCopyUnresolved = "TODO(copy-review) %s: unresolved" |
| + restartCopyUnknown = "TODO(copy-review): unknown" |
| + restartCopyScope = "TODO(copy-review): scope" |
| +) |
| + |
| const envUsage = `usage: carlos env <command> |
| commands: |
| @@ -203,7 +336,7 @@ func runEnvSet(args []string) { |
| fs := flag.NewFlagSet("env set", flag.ExitOnError) |
| app := appFlag(fs, "") |
| environment := environmentFlag(fs, "named environment bundle to set in (omit for the default bundle)") |
| - acctRaw, acctIDRaw, acctOf := accountFlags(fs) |
| + acctRaw, acctIDRaw, regPath, acctOf := accountFlagsWithRegistry(fs) |
| consoleFlag := fs.String("console", "", "console origin to work through (API mode only; default: the only logged-in one)") |
| fs.Usage = flagUsage(fs, "") |
| fs.Parse(args) |
| @@ -226,28 +359,44 @@ func runEnvSet(args []string) { |
| for k, v := range kv { |
| set[k] = appconfig.Var{Kind: "env", Value: v} |
| } |
| - if _, err := sess.configApply(context.Background(), acct, app2, |
| - apiConfigApply{Environment: *environment, Set: set}); err != nil { |
| + res, err := sess.configApply(context.Background(), acct, app2, |
| + apiConfigApply{Environment: *environment, Set: set}) |
| + if err != nil { |
| log.Fatal(err) |
| } |
| fmt.Printf("%s %d env var(s) for %s (%s)\n", colorFor(os.Stdout).ok("set"), len(kv), app2, bundleLabel(*environment)) |
| + printRestartNotice(os.Stdout, app2, restartFromAPI(res.Restart)) |
| return |
| } |
| if *app == "" { |
| log.Fatal(usage) |
| } |
| - if err := saveEnvVars(context.Background(), openStore(), acctOf(), *app, *environment, kv); err != nil { |
| + store := openStore() |
| + if err := saveEnvVars(context.Background(), store, acctOf(), *app, *environment, kv); err != nil { |
| log.Fatal(err) |
| } |
| fmt.Printf("%s %d env var(s) for %s (%s)\n", colorFor(os.Stdout).ok("set"), len(kv), *app, bundleLabel(*environment)) |
| + printRestartNotice(os.Stdout, *app, localRestartAnswer(context.Background(), store, *regPath, acctOf(), *app, *environment, mapKeys(kv))) |
| +} |
| + |
| +// mapKeys is the key set of a KEY=value map, for the writtenKeys argument |
| +// AwaitingRestart narrows on. Sorted, so a notice is deterministic across |
| +// runs where Go's map order is not. |
| +func mapKeys(kv map[string]string) []string { |
| + out := make([]string, 0, len(kv)) |
| + for k := range kv { |
| + out = append(out, k) |
| + } |
| + sort.Strings(out) |
| + return out |
| } |
| func runEnvUnset(args []string) { |
| fs := flag.NewFlagSet("env unset", flag.ExitOnError) |
| app := appFlag(fs, "") |
| environment := environmentFlag(fs, "named environment bundle to remove from (omit for the default bundle)") |
| - acctRaw, acctIDRaw, acctOf := accountFlags(fs) |
| + acctRaw, acctIDRaw, regPath, acctOf := accountFlagsWithRegistry(fs) |
| consoleFlag := fs.String("console", "", "console origin to work through (API mode only; default: the only logged-in one)") |
| fs.Usage = flagUsage(fs, "") |
| fs.Parse(args) |
| @@ -265,23 +414,31 @@ func runEnvUnset(args []string) { |
| printUsing(os.Stdout, sess) |
| acct := resolvedAccount(sess, acctRaw, acctIDRaw) |
| app2 := resolvedApp(sess, acct, *app, usage) |
| - removed, err := sess.configApply(context.Background(), acct, app2, |
| + res, err := sess.configApply(context.Background(), acct, app2, |
| apiConfigApply{Environment: *environment, Unset: fs.Args()}) |
| if err != nil { |
| log.Fatal(err) |
| } |
| - fmt.Printf("%s %d of %d var(s) for %s (%s)\n", colorFor(os.Stdout).ok("unset"), removed, fs.NArg(), app2, bundleLabel(*environment)) |
| + fmt.Printf("%s %d of %d var(s) for %s (%s)\n", colorFor(os.Stdout).ok("unset"), res.Removed, fs.NArg(), app2, bundleLabel(*environment)) |
| + printRestartNotice(os.Stdout, app2, restartFromAPI(res.Restart)) |
| return |
| } |
| if *app == "" { |
| log.Fatal(usage) |
| } |
| - removed, err := unsetVars(context.Background(), openStore(), acctOf(), *app, *environment, fs.Args()) |
| + store := openStore() |
| + removed, err := unsetVars(context.Background(), store, acctOf(), *app, *environment, fs.Args()) |
| if err != nil { |
| log.Fatal(err) |
| } |
| fmt.Printf("%s %d of %d var(s) for %s (%s)\n", colorFor(os.Stdout).ok("unset"), removed, fs.NArg(), *app, bundleLabel(*environment)) |
| + // Gated on removed: an unset that removed nothing moved no file, so |
| + // there is nothing for anyone to restart over — the same test the |
| + // console door applies before it computes its own notice. |
| + if removed > 0 { |
| + printRestartNotice(os.Stdout, *app, localRestartAnswer(context.Background(), store, *regPath, acctOf(), *app, *environment, fs.Args())) |
| + } |
| } |
| func runEnvList(args []string) { |
| @@ -392,7 +549,7 @@ func runEnvExecDelivery(args []string) { |
| app := appFlag(fs, "") |
| environment := environmentFlag(fs, |
| "named environment bundle to set the flag in (omit for the app's default bundle)") |
| - acctRaw, acctIDRaw, acctOf := accountFlags(fs) |
| + acctRaw, acctIDRaw, regPath, acctOf := accountFlagsWithRegistry(fs) |
| consoleFlag := fs.String("console", "", "console origin to work through (API mode only; default: the only logged-in one)") |
| fs.Usage = flagUsage(fs, "") |
| fs.Parse(args) |
| @@ -432,6 +589,12 @@ func runEnvExecDelivery(args []string) { |
| log.Fatal(err) |
| } |
| appName := *app |
| + // Turning delivery ON is the write that creates a route's file for the |
| + // first time, with the same never-bounced process on the other end of |
| + // it, so it carries the same notice as a var write. No keys are passed: |
| + // there is nothing to shadow here, and the file appears (or does not) |
| + // regardless of which vars the bundle happens to hold. |
| + var restart *restartAnswer |
| if sess != nil { |
| printUsing(os.Stdout, sess) |
| acct := resolvedAccount(sess, acctRaw, acctIDRaw) |
| @@ -446,23 +609,28 @@ func runEnvExecDelivery(args []string) { |
| if value != "" { |
| wire = value |
| } |
| - if _, err := sess.configApply(context.Background(), acct, appName, |
| - apiConfigApply{Environment: *environment, ExecDelivery: &wire}); err != nil { |
| + res, err := sess.configApply(context.Background(), acct, appName, |
| + apiConfigApply{Environment: *environment, ExecDelivery: &wire}) |
| + if err != nil { |
| log.Fatal(err) |
| } |
| + restart = restartFromAPI(res.Restart) |
| } else { |
| if *app == "" { |
| log.Fatal(usage) |
| } |
| - if err := appconfig.SetExecDeliveryAt(context.Background(), openStore(), acctOf(), *app, *environment, value); err != nil { |
| + store := openStore() |
| + if err := appconfig.SetExecDeliveryAt(context.Background(), store, acctOf(), *app, *environment, value); err != nil { |
| log.Fatal(err) |
| } |
| + restart = localRestartAnswer(context.Background(), store, *regPath, acctOf(), *app, *environment, nil) |
| } |
| word := value |
| if value == "" { |
| word = cleared |
| } |
| fmt.Printf("%s %s for %s (%s)\n", colorFor(os.Stdout).ok("exec-delivery"), word, appName, bundleLabel(*environment)) |
| + printRestartNotice(os.Stdout, appName, restart) |
| } |
| // bundleAppName renders --app for a refusal printed BEFORE the app has been |
| @@ -656,7 +824,7 @@ func runSecretsSet(args []string) { |
| fs := flag.NewFlagSet("secrets set", flag.ExitOnError) |
| app := appFlag(fs, "") |
| environment := environmentFlag(fs, "named environment bundle to set in (omit for the default bundle)") |
| - acctRaw, acctIDRaw, acctOf := accountFlags(fs) |
| + acctRaw, acctIDRaw, regPath, acctOf := accountFlagsWithRegistry(fs) |
| consoleFlag := fs.String("console", "", "console origin to work through (API mode only; default: the only logged-in one)") |
| fs.Usage = flagUsage(fs, "") |
| fs.Parse(args) |
| @@ -666,6 +834,30 @@ func runSecretsSet(args []string) { |
| if err != nil { |
| log.Fatalf("secrets set: %v", err) |
| } |
| + // Empty values first, and as a group: they are the likeliest thing to |
| + // be wrong about a `secrets set` invocation and the cheapest to catch, |
| + // and a caller who mistyped one field name has usually mistyped |
| + // several. Discussion #16 — values arrive as "$(op read ...)", `op |
| + // read` on a field that does not exist prints nothing and exits 0, so |
| + // the shell hands us an empty string, we seal it, store it, and report |
| + // success. The symptom lands hours later somewhere else entirely: with |
| + // both halves of a credential pair empty the app reads "not |
| + // configured" and silently offers no OAuth provider; with one empty it |
| + // refuses to boot. |
| + // |
| + // Keys only, never values — parseKV's rule above, for the same reason: |
| + // a mistyped `secrets set` argument IS a secret, and this text lands in |
| + // stderr and shell scrollback. |
| + // |
| + // The console door refuses these too (appconfig.CheckEnvelope, which |
| + // reads emptiness off the envelope's length). This check exists in |
| + // front of it because it is the ONLY one in direct mode, because it |
| + // fires before the network round-trip that fetches the sealing key, |
| + // and because it can name every offending key at once where the door, |
| + // which validates var by var, names the first. |
| + if err := checkNoEmptyValues(kv); err != nil { |
| + log.Fatalf("secrets set: %v", err) |
| + } |
| // Validate every value while the plaintext is still in hand, before |
| // either branch below seals anything — after Seal the value is opaque |
| // forever (`secrets list` never shows values), so a bad one would only |
| @@ -699,11 +891,13 @@ func runSecretsSet(args []string) { |
| } |
| set[k] = appconfig.Var{Kind: "secret", Envelope: env} |
| } |
| - if _, err := sess.configApply(context.Background(), acct, app2, |
| - apiConfigApply{Environment: *environment, Set: set}); err != nil { |
| + res, err := sess.configApply(context.Background(), acct, app2, |
| + apiConfigApply{Environment: *environment, Set: set}) |
| + if err != nil { |
| log.Fatal(err) |
| } |
| fmt.Printf("%s %d secret(s) for %s (%s)\n", colorFor(os.Stdout).ok("set"), len(kv), app2, bundleLabel(*environment)) |
| + printRestartNotice(os.Stdout, app2, restartFromAPI(res.Restart)) |
| return |
| } |
| @@ -719,17 +913,19 @@ func runSecretsSet(args []string) { |
| } |
| sealed[k] = env |
| } |
| - if err := saveSecrets(context.Background(), openStore(), acctOf(), *app, *environment, sealed); err != nil { |
| + store := openStore() |
| + if err := saveSecrets(context.Background(), store, acctOf(), *app, *environment, sealed); err != nil { |
| log.Fatal(err) |
| } |
| fmt.Printf("%s %d secret(s) for %s (%s)\n", colorFor(os.Stdout).ok("set"), len(kv), *app, bundleLabel(*environment)) |
| + printRestartNotice(os.Stdout, *app, localRestartAnswer(context.Background(), store, *regPath, acctOf(), *app, *environment, mapKeys(kv))) |
| } |
| func runSecretsUnset(args []string) { |
| fs := flag.NewFlagSet("secrets unset", flag.ExitOnError) |
| app := appFlag(fs, "") |
| environment := environmentFlag(fs, "named environment bundle to remove from (omit for the default bundle)") |
| - acctRaw, acctIDRaw, acctOf := accountFlags(fs) |
| + acctRaw, acctIDRaw, regPath, acctOf := accountFlagsWithRegistry(fs) |
| consoleFlag := fs.String("console", "", "console origin to work through (API mode only; default: the only logged-in one)") |
| fs.Usage = flagUsage(fs, "") |
| fs.Parse(args) |
| @@ -747,23 +943,29 @@ func runSecretsUnset(args []string) { |
| printUsing(os.Stdout, sess) |
| acct := resolvedAccount(sess, acctRaw, acctIDRaw) |
| app2 := resolvedApp(sess, acct, *app, usage) |
| - removed, err := sess.configApply(context.Background(), acct, app2, |
| + res, err := sess.configApply(context.Background(), acct, app2, |
| apiConfigApply{Environment: *environment, Unset: fs.Args()}) |
| if err != nil { |
| log.Fatal(err) |
| } |
| - fmt.Printf("%s %d of %d secret(s) for %s (%s)\n", colorFor(os.Stdout).ok("unset"), removed, fs.NArg(), app2, bundleLabel(*environment)) |
| + fmt.Printf("%s %d of %d secret(s) for %s (%s)\n", colorFor(os.Stdout).ok("unset"), res.Removed, fs.NArg(), app2, bundleLabel(*environment)) |
| + printRestartNotice(os.Stdout, app2, restartFromAPI(res.Restart)) |
| return |
| } |
| if *app == "" { |
| log.Fatal(usage) |
| } |
| - removed, err := unsetVars(context.Background(), openStore(), acctOf(), *app, *environment, fs.Args()) |
| + store := openStore() |
| + removed, err := unsetVars(context.Background(), store, acctOf(), *app, *environment, fs.Args()) |
| if err != nil { |
| log.Fatal(err) |
| } |
| fmt.Printf("%s %d of %d secret(s) for %s (%s)\n", colorFor(os.Stdout).ok("unset"), removed, fs.NArg(), *app, bundleLabel(*environment)) |
| + // Gated on removed, exactly as `env unset`'s direct branch is. |
| + if removed > 0 { |
| + printRestartNotice(os.Stdout, *app, localRestartAnswer(context.Background(), store, *regPath, acctOf(), *app, *environment, fs.Args())) |
| + } |
| } |
| // printSecretsList renders `secrets list`'s output — header line, then |
diff --git a/appconfig_restart_test.go b/appconfig_restart_test.go| new file mode 100644 |
| index 000000000..92f10cd58 |
| --- /dev/null |
| +++ b/appconfig_restart_test.go |
| @@ -0,0 +1,138 @@ |
| +package main |
| + |
| +// printRestartNotice's three states (discussion #16). The assertions are on |
| +// STRUCTURE — which hosts are named, which remedy is offered, whether |
| +// anything is said at all — never on the exact sentences, which are reviewed |
| +// copy and change without changing what the notice means. |
| + |
| +import ( |
| + "bytes" |
| + "strings" |
| + "testing" |
| + |
| + "github.com/carlosframework/platform/internal/appconfig" |
| +) |
| + |
| +func TestPrintRestartNoticeSaysNothingWhenNoClaimWasMade(t *testing.T) { |
| + var b bytes.Buffer |
| + printRestartNotice(&b, "relay", nil) |
| + if b.Len() != 0 { |
| + t.Fatalf("printed %q for a nil answer, want silence — an older console said nothing, which is not a claim that nothing needs restarting", b.String()) |
| + } |
| +} |
| + |
| +func TestPrintRestartNoticeSaysNothingWhenNothingPending(t *testing.T) { |
| + var b bytes.Buffer |
| + printRestartNotice(&b, "relay", &restartAnswer{known: true}) |
| + if b.Len() != 0 { |
| + t.Fatalf("printed %q when the answer is a genuine none", b.String()) |
| + } |
| +} |
| + |
| +// The one that matters: a console that LOOKED and could not tell must say |
| +// so, because staying quiet here is the failure being fixed. |
| +func TestPrintRestartNoticeSpeaksUpWhenItCouldNotTell(t *testing.T) { |
| + var b bytes.Buffer |
| + printRestartNotice(&b, "relay", &restartAnswer{known: false}) |
| + if b.Len() == 0 { |
| + t.Fatal("printed nothing for known:false — 'could not check' must not render as 'all clear'") |
| + } |
| +} |
| + |
| +func TestPrintRestartNoticeNamesEachHost(t *testing.T) { |
| + var b bytes.Buffer |
| + printRestartNotice(&b, "relay", &restartAnswer{known: true, pending: appconfig.RestartPending{ |
| + Hosts: []appconfig.RestartHost{ |
| + {Host: "one.example.com"}, |
| + {Host: "two.example.com"}, |
| + }, |
| + }}) |
| + got := b.String() |
| + for _, want := range []string{"one.example.com", "two.example.com"} { |
| + if !strings.Contains(got, want) { |
| + t.Fatalf("output %q does not name %q", got, want) |
| + } |
| + } |
| + if n := strings.Count(got, "\n"); n != 2 { |
| + t.Fatalf("output has %d lines, want one per host:\n%s", n, got) |
| + } |
| +} |
| + |
| +// The remedy split. A route whose unit carlos knows gets pointed at a |
| +// command; a route without one must NOT be, because `carlos restart` is a |
| +// documented no-op for that shape and sending someone to run it would be a |
| +// second silent failure answering the first. |
| +func TestPrintRestartNoticeOffersTheRightRemedy(t *testing.T) { |
| + var withUnit bytes.Buffer |
| + printRestartNotice(&withUnit, "relay", &restartAnswer{known: true, pending: appconfig.RestartPending{ |
| + Hosts: []appconfig.RestartHost{{Host: "console.example.com", Unit: "console.service"}}, |
| + }}) |
| + if !strings.Contains(withUnit.String(), "console.service") { |
| + t.Fatalf("output %q does not name the unit that can be cycled", withUnit.String()) |
| + } |
| + |
| + var plain bytes.Buffer |
| + printRestartNotice(&plain, "relay", &restartAnswer{known: true, pending: appconfig.RestartPending{ |
| + Hosts: []appconfig.RestartHost{{Host: "connect.example.com"}}, |
| + }}) |
| + if strings.Contains(plain.String(), "carlos restart") { |
| + t.Fatalf("output %q offers `carlos restart` for a route it cannot cycle", plain.String()) |
| + } |
| +} |
| + |
| +func TestPrintRestartNoticeNamesUnresolvedHosts(t *testing.T) { |
| + var b bytes.Buffer |
| + printRestartNotice(&b, "relay", &restartAnswer{known: true, pending: appconfig.RestartPending{ |
| + Unresolved: []string{"ghost.example.com"}, |
| + }}) |
| + if !strings.Contains(b.String(), "ghost.example.com") { |
| + t.Fatalf("output %q does not name the host it could not resolve", b.String()) |
| + } |
| +} |
| + |
| +// checkNoEmptyValues: the report's aside, on the CLI side of the seal. This |
| +// is the only gate in direct mode, and it fires before the network |
| +// round-trip that fetches the sealing key. |
| +func TestCheckNoEmptyValues(t *testing.T) { |
| + if err := checkNoEmptyValues(map[string]string{"A": "1", "B": "2"}); err != nil { |
| + t.Fatalf("refused a pair of real values: %v", err) |
| + } |
| + err := checkNoEmptyValues(map[string]string{"CLIENT_ID": "", "CLIENT_SECRET": "", "FINE": "v"}) |
| + if err == nil { |
| + t.Fatal("accepted empty values") |
| + } |
| + // Every offending key, not just the first — a mistyped `op read` field |
| + // usually spoils several at once, and Go's map order would otherwise |
| + // name a different one each run. |
| + for _, want := range []string{"CLIENT_ID", "CLIENT_SECRET"} { |
| + if !strings.Contains(err.Error(), want) { |
| + t.Fatalf("error %q does not name %q", err, want) |
| + } |
| + } |
| + if strings.Contains(err.Error(), "FINE") { |
| + t.Fatalf("error %q names a key that was fine", err) |
| + } |
| + // Sorted, so the message is the same on every run. |
| + if i, j := strings.Index(err.Error(), "CLIENT_ID"), strings.Index(err.Error(), "CLIENT_SECRET"); i > j { |
| + t.Fatalf("keys are not sorted in %q", err) |
| + } |
| +} |
| + |
| +// restartFromAPI's nil-preservation is the wire half of the same three |
| +// states: a console that omits the field must not be turned into one that |
| +// claims there is nothing to do. |
| +func TestRestartFromAPIPreservesNil(t *testing.T) { |
| + if got := restartFromAPI(nil); got != nil { |
| + t.Fatalf("restartFromAPI(nil) = %+v, want nil", got) |
| + } |
| + got := restartFromAPI(&apiConfigRestart{ |
| + Known: true, |
| + Hosts: []apiConfigRestartHost{{Host: "h.example.com", Unit: "u.service"}}, |
| + }) |
| + if got == nil || !got.known { |
| + t.Fatalf("restartFromAPI dropped a present answer: %+v", got) |
| + } |
| + if len(got.pending.Hosts) != 1 || got.pending.Hosts[0].Unit != "u.service" { |
| + t.Fatalf("hosts = %+v, want the unit carried through", got.pending.Hosts) |
| + } |
| +} |
diff --git a/cli.go b/cli.go| index e20eac7df..0652d5ac7 100644 |
| --- a/cli.go |
| +++ b/cli.go |
| @@ -1643,14 +1643,29 @@ func routeVersionCheck(ctx context.Context, store release.Store, pub ed25519.Pub |
| // string-coupled to this function's internal flag name the way |
| // fs.Lookup("account") would be. |
| func accountFlags(fs *flag.FlagSet) (raw, rawID *string, resolve func() string) { |
| + raw, rawID, _, resolve = accountFlagsWithRegistry(fs) |
| + return raw, rawID, resolve |
| +} |
| + |
| +// accountFlagsWithRegistry is accountFlags for the handful of verbs that |
| +// need the registry PATH as well as the account it resolves — not the flag's |
| +// default, but whatever --registry actually says. |
| +// |
| +// It exists so that those verbs cannot end up reading a different database |
| +// than the one their own --account was resolved against. A verb that called |
| +// defaultRegistry() directly would do exactly that the moment anyone passed |
| +// --registry, and would do it silently, which for `env set`'s restart notice |
| +// (appconfig_cli.go) would mean answering "which routes did this reach" from |
| +// the wrong box's registry. |
| +func accountFlagsWithRegistry(fs *flag.FlagSet) (raw, rawID, regPath *string, resolve func() string) { |
| // Empty default, two meanings by mode: API mode resolves it to the |
| // caller's own account (resolvedAccount, apimode.go); direct mode's |
| // resolveAccount reads "" as ops exactly as it always has, so box and |
| // script behavior is unchanged. |
| acct := fs.String("account", "", "account (sqid or name) the app belongs to (default: your only account, or the configured/chosen one; ops in direct mode)") |
| acctID := fs.String("account-id", "", "account sqid, matched as a sqid ONLY — supersedes --account; this is the flag an ambiguity error tells you to use") |
| - regPath := fs.String("registry", defaultRegistry(), "registry database (resolves --account)") |
| - return acct, acctID, func() string { return resolveAccount(*regPath, *acct, *acctID) } |
| + reg := fs.String("registry", defaultRegistry(), "registry database (resolves --account)") |
| + return acct, acctID, reg, func() string { return resolveAccount(*reg, *acct, *acctID) } |
| } |
| func openStore() release.Store { |
diff --git a/internal/appconfig/awaitingrestart_test.go b/internal/appconfig/awaitingrestart_test.go| new file mode 100644 |
| index 000000000..501ed1d92 |
| --- /dev/null |
| +++ b/internal/appconfig/awaitingrestart_test.go |
| @@ -0,0 +1,263 @@ |
| +package appconfig |
| + |
| +import ( |
| + "encoding/json" |
| + "errors" |
| + "testing" |
| + |
| + "github.com/carlosframework/platform/internal/registry" |
| +) |
| + |
| +// addrRoute is the shape this whole feature is about: an instance carlos |
| +// proxies to over TCP and neither spawns nor owns — the one SyncExec writes |
| +// a file for and deliberately never bounces. |
| +func addrRoute(host, app, acct, env string) registry.Route { |
| + return registry.Route{ |
| + Host: host, Kind: "instance", App: app, Account: acct, |
| + Status: "ready", Addr: "127.0.0.1:8080", Environment: env, |
| + } |
| +} |
| + |
| +// TestAwaitingRestartNamesOptedInAddrRoute is the base case, and the one |
| +// the notice exists for: config written, file delivered, process none the |
| +// wiser. |
| +func TestAwaitingRestartNamesOptedInAddrRoute(t *testing.T) { |
| + ctx, s, _, _, _, _ := execKit(t) |
| + if err := SetExecDelivery(ctx, s, "bab", "relay", ExecDeliveryFile); err != nil { |
| + t.Fatalf("SetExecDelivery: %v", err) |
| + } |
| + routes := []registry.Route{addrRoute("connect.example.com", "relay", "bab", "")} |
| + got := AwaitingRestart(ctx, s, routes, "bab", "relay", "", []string{"CLIENT_ID"}) |
| + if len(got.Hosts) != 1 || got.Hosts[0].Host != "connect.example.com" { |
| + t.Fatalf("Hosts = %+v, want just connect.example.com", got.Hosts) |
| + } |
| + if got.Hosts[0].Unit != "" { |
| + t.Fatalf("Unit = %q, want empty for a route carlos cannot restart", got.Hosts[0].Unit) |
| + } |
| + if len(got.Unresolved) != 0 { |
| + t.Fatalf("Unresolved = %v, want none", got.Unresolved) |
| + } |
| +} |
| + |
| +// TestAwaitingRestartSkipsUnoptedAndSocketRoutes pins the two shapes that |
| +// must never appear in the notice, for opposite reasons: an un-opted addr |
| +// route never gets a file at all, and a socket route gets one AND gets |
| +// bounced, so telling someone to restart it by hand would be noise. |
| +func TestAwaitingRestartSkipsUnoptedAndSocketRoutes(t *testing.T) { |
| + ctx, s, _, _, _, _ := execKit(t) |
| + if err := SetExecDelivery(ctx, s, "bab", "relay", ExecDeliveryFile); err != nil { |
| + t.Fatalf("SetExecDelivery: %v", err) |
| + } |
| + socket := registry.Route{ |
| + Host: "sock.example.com", Kind: "instance", App: "relay", Account: "bab", |
| + Status: "ready", Socket: "/run/carlos/sock.sock", |
| + } |
| + unit := registry.Route{ |
| + Host: "unit.example.com", Kind: "instance", App: "relay", Account: "bab", |
| + Status: "ready", Addr: "127.0.0.1:9090", Backing: registry.BackingUnit, |
| + } |
| + got := AwaitingRestart(ctx, s, []registry.Route{socket, unit}, "bab", "relay", "", []string{"K"}) |
| + if !got.Empty() { |
| + t.Fatalf("got %+v, want nothing: a socket route is bounced and a unit route is the root pass's", got) |
| + } |
| + |
| + // The same addr route, with the app never opted in: no file was ever |
| + // written for it, so there is nothing to restart over. |
| + ctx2, s2, _, _, _, _ := execKit(t) |
| + got = AwaitingRestart(ctx2, s2, []registry.Route{addrRoute("connect.example.com", "relay", "bab", "")}, |
| + "bab", "relay", "", []string{"K"}) |
| + if !got.Empty() { |
| + t.Fatalf("got %+v, want nothing for an app that never opted in", got) |
| + } |
| +} |
| + |
| +// TestAwaitingRestartCarriesUnit pins the field that decides which remedy |
| +// the CLI offers. An addr route naming its own unit (issue #110's consoles) |
| +// is the one addr shape `carlos restart` can actually cycle. |
| +func TestAwaitingRestartCarriesUnit(t *testing.T) { |
| + ctx, s, _, _, _, _ := execKit(t) |
| + if err := SetExecDelivery(ctx, s, "bab", "relay", ExecDeliveryFile); err != nil { |
| + t.Fatalf("SetExecDelivery: %v", err) |
| + } |
| + rt := addrRoute("console.example.com", "relay", "bab", "") |
| + rt.Unit = "console.service" |
| + got := AwaitingRestart(ctx, s, []registry.Route{rt}, "bab", "relay", "", nil) |
| + if len(got.Hosts) != 1 || got.Hosts[0].Unit != "console.service" { |
| + t.Fatalf("Hosts = %+v, want console.service carried through", got.Hosts) |
| + } |
| +} |
| + |
| +// TestAwaitingRestartDefaultWriteReachesBoundRoute: a named bundle is an |
| +// overlay ON the default, not a replacement for it, so a default-bundle |
| +// write does reach a route bound to an environment — for any key that |
| +// environment does not itself state. |
| +func TestAwaitingRestartDefaultWriteReachesBoundRoute(t *testing.T) { |
| + ctx, s, _, _, _, _ := execKit(t) |
| + if err := SetExecDelivery(ctx, s, "bab", "relay", ExecDeliveryFile); err != nil { |
| + t.Fatalf("SetExecDelivery: %v", err) |
| + } |
| + if _, err := ApplyEnvironment(ctx, s, "bab", "relay", "prod", |
| + map[string]Var{"OWN_KEY": {Kind: "env", Value: "v"}}, nil); err != nil { |
| + t.Fatalf("ApplyEnvironment: %v", err) |
| + } |
| + routes := []registry.Route{addrRoute("connect.example.com", "relay", "bab", "prod")} |
| + |
| + // A key the bound bundle does NOT state: the merge changes, the file |
| + // changes, the process is behind. |
| + got := AwaitingRestart(ctx, s, routes, "bab", "relay", "", []string{"OTHER_KEY"}) |
| + if len(got.Hosts) != 1 { |
| + t.Fatalf("Hosts = %+v, want the bound route for an unshadowed key", got.Hosts) |
| + } |
| + |
| + // A key it DOES state: the overlay wins, the merged manifest is |
| + // byte-identical, syncExecHost writes nothing. Naming the route here |
| + // would send someone to restart a process for a value it was never |
| + // going to see. |
| + got = AwaitingRestart(ctx, s, routes, "bab", "relay", "", []string{"OWN_KEY"}) |
| + if !got.Empty() { |
| + t.Fatalf("got %+v, want nothing: OWN_KEY is shadowed by the bound bundle", got) |
| + } |
| + |
| + // Mixed: one shadowed, one not — still affected. |
| + got = AwaitingRestart(ctx, s, routes, "bab", "relay", "", []string{"OWN_KEY", "OTHER_KEY"}) |
| + if len(got.Hosts) != 1 { |
| + t.Fatalf("Hosts = %+v, want the route when any key is unshadowed", got.Hosts) |
| + } |
| +} |
| + |
| +// TestAwaitingRestartNamedWriteOnlyReachesItsOwnRoutes: the other |
| +// direction. A write to one environment must not name routes bound to |
| +// another, or to none. |
| +func TestAwaitingRestartNamedWriteOnlyReachesItsOwnRoutes(t *testing.T) { |
| + ctx, s, _, _, _, _ := execKit(t) |
| + if err := SetExecDelivery(ctx, s, "bab", "relay", ExecDeliveryFile); err != nil { |
| + t.Fatalf("SetExecDelivery: %v", err) |
| + } |
| + for _, env := range []string{"prod", "staging"} { |
| + if _, err := ApplyEnvironment(ctx, s, "bab", "relay", env, |
| + map[string]Var{"K": {Kind: "env", Value: env}}, nil); err != nil { |
| + t.Fatalf("ApplyEnvironment(%s): %v", env, err) |
| + } |
| + } |
| + routes := []registry.Route{ |
| + addrRoute("prod.example.com", "relay", "bab", "prod"), |
| + addrRoute("staging.example.com", "relay", "bab", "staging"), |
| + addrRoute("plain.example.com", "relay", "bab", ""), |
| + } |
| + got := AwaitingRestart(ctx, s, routes, "bab", "relay", "prod", []string{"K"}) |
| + if len(got.Hosts) != 1 || got.Hosts[0].Host != "prod.example.com" { |
| + t.Fatalf("Hosts = %+v, want only prod.example.com", got.Hosts) |
| + } |
| +} |
| + |
| +// TestAwaitingRestartOtherAppsAndAccounts is the tenant boundary: routes |
| +// belong to one app of one account, and a notice that leaked a neighbour's |
| +// hostname would be a disclosure, not a nicety. |
| +func TestAwaitingRestartOtherAppsAndAccounts(t *testing.T) { |
| + ctx, s, _, _, _, _ := execKit(t) |
| + for _, app := range []string{"relay", "other"} { |
| + if err := SetExecDelivery(ctx, s, "bab", app, ExecDeliveryFile); err != nil { |
| + t.Fatalf("SetExecDelivery(%s): %v", app, err) |
| + } |
| + } |
| + if err := SetExecDelivery(ctx, s, "zzz", "relay", ExecDeliveryFile); err != nil { |
| + t.Fatalf("SetExecDelivery(zzz): %v", err) |
| + } |
| + routes := []registry.Route{ |
| + addrRoute("mine.example.com", "relay", "bab", ""), |
| + addrRoute("otherapp.example.com", "other", "bab", ""), |
| + addrRoute("otheracct.example.com", "relay", "zzz", ""), |
| + } |
| + got := AwaitingRestart(ctx, s, routes, "bab", "relay", "", nil) |
| + if len(got.Hosts) != 1 || got.Hosts[0].Host != "mine.example.com" { |
| + t.Fatalf("Hosts = %+v, want only this account's route for this app", got.Hosts) |
| + } |
| +} |
| + |
| +// TestAwaitingRestartUnresolvedOnMissingBoundBundle: a route bound to a |
| +// bundle that does not exist resolves to an ERROR, not to the app default — |
| +// LoadMerged refuses it and SyncExec leaves the file exactly as it was. |
| +// Nothing was delivered, so "delivered" would be a lie; but the route is |
| +// not fine either, so it is not a skip. |
| +func TestAwaitingRestartUnresolvedOnMissingBoundBundle(t *testing.T) { |
| + ctx, s, _, _, _, _ := execKit(t) |
| + if err := SetExecDelivery(ctx, s, "bab", "relay", ExecDeliveryFile); err != nil { |
| + t.Fatalf("SetExecDelivery: %v", err) |
| + } |
| + routes := []registry.Route{addrRoute("connect.example.com", "relay", "bab", "ghost")} |
| + got := AwaitingRestart(ctx, s, routes, "bab", "relay", "", nil) |
| + if len(got.Hosts) != 0 { |
| + t.Fatalf("Hosts = %+v, want none: nothing was delivered", got.Hosts) |
| + } |
| + if len(got.Unresolved) != 1 || got.Unresolved[0] != "connect.example.com" { |
| + t.Fatalf("Unresolved = %v, want connect.example.com", got.Unresolved) |
| + } |
| +} |
| + |
| +// TestAwaitingRestartUnresolvedOnFetchFailure is the same distinction for a |
| +// transient blip. A read that failed is not a determination either way, and |
| +// reporting it as "nothing to restart" would be discussion #16's own bug |
| +// reappearing inside its fix. |
| +func TestAwaitingRestartUnresolvedOnFetchFailure(t *testing.T) { |
| + ctx, s, _, _, _, _ := execKit(t) |
| + if err := SetExecDelivery(ctx, s, "bab", "relay", ExecDeliveryFile); err != nil { |
| + t.Fatalf("SetExecDelivery: %v", err) |
| + } |
| + blip := &failingKeyStore{Store: s, failKey: Key("bab", "relay"), err: errors.New("503 slow down")} |
| + routes := []registry.Route{addrRoute("connect.example.com", "relay", "bab", "")} |
| + got := AwaitingRestart(ctx, blip, routes, "bab", "relay", "", nil) |
| + if len(got.Hosts) != 0 { |
| + t.Fatalf("Hosts = %+v, want none when the bundle would not read", got.Hosts) |
| + } |
| + if len(got.Unresolved) != 1 || got.Unresolved[0] != "connect.example.com" { |
| + t.Fatalf("Unresolved = %v, want connect.example.com", got.Unresolved) |
| + } |
| +} |
| + |
| +// TestAwaitingRestartUnknownExecDeliveryIsOff pins the additive-discipline |
| +// case: a value a NEWER carlos wrote decides "not opted in" here, exactly as |
| +// it does in SyncExec, so the notice and the sweep agree about who gets a |
| +// file even across versions. |
| +func TestAwaitingRestartUnknownExecDeliveryIsOff(t *testing.T) { |
| + ctx, s, _, _, _, _ := execKit(t) |
| + // Written straight to the bucket rather than through a setter: the |
| + // setters refuse this value by design, and the case being pinned is a |
| + // manifest a FUTURE carlos wrote that this binary has to read without |
| + // choking on. |
| + raw, err := json.Marshal(&Manifest{Schema: 1, ExecDelivery: "socket-someday"}) |
| + if err != nil { |
| + t.Fatalf("marshal: %v", err) |
| + } |
| + if _, err := s.Put(ctx, Key("bab", "relay"), raw, ""); err != nil { |
| + t.Fatalf("Put: %v", err) |
| + } |
| + routes := []registry.Route{addrRoute("connect.example.com", "relay", "bab", "")} |
| + if got := AwaitingRestart(ctx, s, routes, "bab", "relay", "", nil); !got.Empty() { |
| + t.Fatalf("got %+v, want nothing for an exec_delivery value this binary does not understand", got) |
| + } |
| +} |
| + |
| +// TestAwaitingRestartSorted pins determinism: registry order is not stable, |
| +// and a notice whose lines reshuffle between identical runs is one people |
| +// stop reading. |
| +func TestAwaitingRestartSorted(t *testing.T) { |
| + ctx, s, _, _, _, _ := execKit(t) |
| + if err := SetExecDelivery(ctx, s, "bab", "relay", ExecDeliveryFile); err != nil { |
| + t.Fatalf("SetExecDelivery: %v", err) |
| + } |
| + routes := []registry.Route{ |
| + addrRoute("zebra.example.com", "relay", "bab", ""), |
| + addrRoute("alpha.example.com", "relay", "bab", ""), |
| + addrRoute("middle.example.com", "relay", "bab", ""), |
| + } |
| + got := AwaitingRestart(ctx, s, routes, "bab", "relay", "", nil) |
| + want := []string{"alpha.example.com", "middle.example.com", "zebra.example.com"} |
| + if len(got.Hosts) != len(want) { |
| + t.Fatalf("Hosts = %+v, want %d", got.Hosts, len(want)) |
| + } |
| + for i, w := range want { |
| + if got.Hosts[i].Host != w { |
| + t.Fatalf("Hosts[%d] = %q, want %q", i, got.Hosts[i].Host, w) |
| + } |
| + } |
| +} |
diff --git a/internal/appconfig/crypto.go b/internal/appconfig/crypto.go| index aa2630762..c6705d879 100644 |
| --- a/internal/appconfig/crypto.go |
| +++ b/internal/appconfig/crypto.go |
| @@ -172,6 +172,37 @@ func CheckEnvelope(envelopeB64 string) error { |
| if _, err := ecdh.P256().NewPublicKey(raw[:ephLen]); err != nil { |
| return errors.New("envelope has no valid ephemeral key") |
| } |
| + // LAST, and deliberately after the structural checks: this one is about |
| + // the VALUE inside a well-formed envelope, where every check above is |
| + // about whether there is a well-formed envelope at all. Run earlier it |
| + // would answer "seals an empty value" for a 93-byte run of garbage that |
| + // seals nothing — naming a defect the caller does not have, which is |
| + // the one thing every refusal here must not do. |
| + // |
| + // EXACTLY the floor means the plaintext was empty: AES-GCM is |
| + // length-preserving, so an envelope carries len(plaintext) bytes beyond |
| + // the ephemeral point, the nonce and the tag. That makes emptiness the |
| + // one property of a secret's VALUE this side of the seal can read |
| + // without a key — and reading it is worth doing, because an empty |
| + // secret is almost always a substitution that produced nothing rather |
| + // than a value someone meant. Discussion #16: values arriving as |
| + // "$(op read ...)" where a mistyped field name returns empty, sealed |
| + // and stored and reported as set, surfacing hours later as an OAuth |
| + // provider silently not offered. |
| + // |
| + // Enforced HERE as well as in `carlos secrets set` for the reason the |
| + // environment-name check above it is: a door must not depend on its |
| + // client's manners. The CLI's check is on the plaintext, before Seal |
| + // and before any network; this one is the same rule expressed on the |
| + // far side of the seal, and it also covers an older CLI that has not |
| + // got the first one. |
| + // |
| + // A caller who genuinely wants an empty value wants an env var, not a |
| + // secret: `carlos env set KEY=` stores one, and it is not sealed |
| + // because there is nothing in it to protect. |
| + if len(raw) == minLen { |
| + return errors.New("envelope seals an empty value — to remove a secret use `carlos secrets unset`, and for a deliberately empty value use `carlos env set`") |
| + } |
| return nil |
| } |
diff --git a/internal/appconfig/crypto_test.go b/internal/appconfig/crypto_test.go| index ad8461397..c18237c79 100644 |
| --- a/internal/appconfig/crypto_test.go |
| +++ b/internal/appconfig/crypto_test.go |
| @@ -115,10 +115,17 @@ func TestOpenRejectsTamperedCiphertext(t *testing.T) { |
| } |
| // TestCheckEnvelopeAcceptsSealed pins the property the console door relies |
| -// on: anything Seal produces passes the structural check, including the |
| -// shortest legal envelope (empty plaintext, which is still a full GCM tag). |
| -// If these two ever diverge, the door starts refusing envelopes the box |
| -// could have opened. |
| +// on: anything Seal produces passes the structural check. If these two ever |
| +// diverge, the door starts refusing envelopes the box could have opened. |
| +// |
| +// Empty plaintext is the ONE deliberate exception, and it is the next test |
| +// rather than a case here. It is not a structural refusal — the envelope is |
| +// perfectly well formed and the box could open it and find "" — it is a |
| +// refusal of the VALUE, added because an empty secret is nearly always a |
| +// substitution that produced nothing (discussion #16). Keeping the two |
| +// tests apart keeps that distinction where the next person will see it: the |
| +// invariant this test protects is still "Seal's output is structurally |
| +// valid", not "the door takes everything". |
| func TestCheckEnvelopeAcceptsSealed(t *testing.T) { |
| _, pub, err := GenerateKey() |
| if err != nil { |
| @@ -128,7 +135,7 @@ func TestCheckEnvelopeAcceptsSealed(t *testing.T) { |
| if err != nil { |
| t.Fatalf("ParsePublicKey: %v", err) |
| } |
| - for _, plaintext := range [][]byte{[]byte("s3kr1t"), {}} { |
| + for _, plaintext := range [][]byte{[]byte("s3kr1t"), []byte("x")} { |
| envelope, err := Seal(pubKey, plaintext) |
| if err != nil { |
| t.Fatalf("Seal(%q): %v", plaintext, err) |
| @@ -139,6 +146,52 @@ func TestCheckEnvelopeAcceptsSealed(t *testing.T) { |
| } |
| } |
| +// TestCheckEnvelopeRefusesEmptyValue pins the length-read the refusal rests |
| +// on, because it is the whole mechanism and it is not obvious: AES-GCM is |
| +// length-preserving, so an envelope of EXACTLY ephLen+nonceLen+16 sealed an |
| +// empty plaintext and one byte longer did not. That is the only fact about a |
| +// secret's value this side of the seal can know without a key, and if it |
| +// ever stopped being true this refusal would start rejecting real |
| +// one-character secrets — hence the boundary is asserted from both sides, |
| +// against real Seal output rather than against a hand-built buffer. |
| +func TestCheckEnvelopeRefusesEmptyValue(t *testing.T) { |
| + _, pub, err := GenerateKey() |
| + if err != nil { |
| + t.Fatalf("GenerateKey: %v", err) |
| + } |
| + pubKey, err := ParsePublicKey(pub, "test") |
| + if err != nil { |
| + t.Fatalf("ParsePublicKey: %v", err) |
| + } |
| + empty, err := Seal(pubKey, []byte{}) |
| + if err != nil { |
| + t.Fatalf("Seal(empty): %v", err) |
| + } |
| + err = CheckEnvelope(empty) |
| + if err == nil { |
| + t.Fatal("CheckEnvelope accepted an envelope sealing an empty value") |
| + } |
| + if !strings.Contains(err.Error(), "empty value") { |
| + t.Fatalf("error %q does not name the defect (want it to mention %q)", err, "empty value") |
| + } |
| + // The refusal points somewhere useful rather than just saying no: the |
| + // two things an operator might actually have meant. |
| + for _, want := range []string{"secrets unset", "env set"} { |
| + if !strings.Contains(err.Error(), want) { |
| + t.Fatalf("error %q does not offer %q", err, want) |
| + } |
| + } |
| + // One byte over the floor is a real secret and must survive — the |
| + // shortest value this check must never eat. |
| + oneByte, err := Seal(pubKey, []byte("x")) |
| + if err != nil { |
| + t.Fatalf("Seal(x): %v", err) |
| + } |
| + if err := CheckEnvelope(oneByte); err != nil { |
| + t.Fatalf("CheckEnvelope rejected a one-byte secret: %v", err) |
| + } |
| +} |
| + |
| // TestCheckEnvelopeRefusals walks the structural defects the door has to |
| // catch at SET time, and asserts on the one thing every refusal must have in |
| // common besides being an error: it names the DEFECT and never echoes the |
diff --git a/internal/appconfig/syncexec.go b/internal/appconfig/syncexec.go| index 2987f1f5e..0f0f8e5b6 100644 |
| --- a/internal/appconfig/syncexec.go |
| +++ b/internal/appconfig/syncexec.go |
| @@ -161,46 +161,38 @@ func SyncExec(ctx context.Context, reg *registry.Registry, s release.Store, priv |
| if c, done := optedIn[k]; done { |
| return c |
| } |
| - var c optedInRes |
| // The route's OWN bundle first, and only when it STATES something: |
| // a named bundle silent on the field inherits the app default, |
| // exactly as a key it does not set inherits the default bundle's |
| - // value. Absence is inheritance, never "off" — an operator who has |
| - // never mentioned exec_delivery in a bundle has not asked for |
| - // delivery to stop. |
| - decided := false |
| + // value. That PRECEDENCE now lives in resolveExecDelivery, which |
| + // AwaitingRestart resolves through too, so the notice an operator |
| + // reads and the file this sweep writes can never disagree about |
| + // who is opted in. What stays here is what THIS caller owes the |
| + // two non-verdict outcomes: a loud line in the sweep's log, once |
| + // per bundle per pass, and every file left exactly as it was. |
| + var eb envRes |
| if env != "" { |
| - eb := loadEnvBundle(acct, app, env) |
| - switch { |
| - case eb.err != nil: |
| + eb = loadEnvBundle(acct, app, env) |
| + if eb.err != nil { |
| // NOT a determination — the same contract the default |
| // bundle's fetch-failure branch below gives: say so loudly, |
| // leave every file alone, retry next pass. |
| fmt.Printf(" %s/%s: exec_delivery unresolved for environment %q (config fetch failed), leaving this bundle's TCP-addr route file(s) as-is: %v\n", acct, app, env, eb.err) |
| - c, decided = optedInRes{v: false, known: false}, true |
| - case eb.found && eb.m.ExecDelivery == ExecDeliveryFile: |
| - c, decided = optedInRes{v: true, known: true}, true |
| - case eb.found && eb.m.ExecDelivery != "": |
| - // A value THIS binary doesn't understand, in a bundle that |
| - // does state one: a statement, so it decides — treated as |
| - // not opted in, loudly, once per bundle per pass, never a |
| - // hard failure (additive discipline, AGENTS.md). |
| - fmt.Printf(" %s/%s: config: exec_delivery %q in environment %q is not a value this binary understands — ignoring it (not opted in); upgrade carlos if this is expected\n", acct, app, eb.m.ExecDelivery, env) |
| - c, decided = optedInRes{v: false, known: true}, true |
| + c := optedInRes{v: false, known: false} |
| + optedIn[k] = c |
| + return c |
| } |
| // found-but-silent, and absent, both fall through to the app |
| - // default. A bundle bound but MISSING is a defect the route's |
| - // own resolution reports (LoadMerged refuses it) and its file is |
| - // left as-is there — the verdict reached here never gets to a |
| - // write in that case. |
| - } |
| - if decided { |
| - optedIn[k] = c |
| - return c |
| - } |
| - def := loadDefault(acct, app) |
| - switch { |
| - case def.err != nil: |
| + // default inside resolveExecDelivery. A bundle bound but |
| + // MISSING is a defect the route's own resolution reports |
| + // (LoadMerged refuses it) and its file is left as-is there — |
| + // the verdict reached here never gets to a write in that case. |
| + } |
| + v, err := resolveExecDelivery(eb.m, eb.found, func() (*Manifest, bool, error) { |
| + d := loadDefault(acct, app) |
| + return d.m, d.found, d.err |
| + }) |
| + if err != nil { |
| // NOT a determination: this app may well be opted in and this |
| // box just failed to see it this pass — indistinguishable here |
| // from any other route's ordinary fetch-failure blip. Silence |
| @@ -222,22 +214,25 @@ func SyncExec(ctx context.Context, reg *registry.Registry, s release.Store, priv |
| // the materialization loop, or reap; its file is untouched by |
| // construction, not because this function decided to spare |
| // it. Next pass retries from scratch. |
| - fmt.Printf(" %s/%s: exec_delivery unresolved (config fetch failed), leaving this app's TCP-addr route file(s) as-is: %v\n", acct, app, def.err) |
| - c = optedInRes{v: false, known: false} |
| - case !def.found: |
| - c = optedInRes{v: false, known: true} |
| - case def.m.ExecDelivery == ExecDeliveryFile: |
| - c = optedInRes{v: true, known: true} |
| - case def.m.ExecDelivery == "": |
| - c = optedInRes{v: false, known: true} |
| - default: |
| + fmt.Printf(" %s/%s: exec_delivery unresolved (config fetch failed), leaving this app's TCP-addr route file(s) as-is: %v\n", acct, app, err) |
| + c := optedInRes{v: false, known: false} |
| + optedIn[k] = c |
| + return c |
| + } |
| + if v.Unknown != "" { |
| // A manifest a NEWER carlos wrote with a value THIS binary |
| // doesn't understand must not brick this box's sync (additive |
| // discipline, AGENTS.md) — ignore it, loudly, once, and treat |
| - // the app as not opted in rather than refusing the whole pass. |
| - fmt.Printf(" %s/%s: config: exec_delivery %q in the default bundle is not a value this binary understands — ignoring it (not opted in); upgrade carlos if this is expected\n", acct, app, def.m.ExecDelivery) |
| - c = optedInRes{v: false, known: true} |
| + // the bundle as not opted in rather than refusing the whole |
| + // pass. Which bundle said it is named, because "upgrade |
| + // carlos" is useless advice without knowing where to look. |
| + where := "the default bundle" |
| + if v.FromEnvironment { |
| + where = fmt.Sprintf("environment %q", env) |
| + } |
| + fmt.Printf(" %s/%s: config: exec_delivery %q in %s is not a value this binary understands — ignoring it (not opted in); upgrade carlos if this is expected\n", acct, app, v.Unknown, where) |
| } |
| + c := optedInRes{v: v.On, known: true} |
| optedIn[k] = c |
| return c |
| } |
| @@ -557,6 +552,243 @@ func ExecConfigApps(routes []registry.Route) [][2]string { |
| return out |
| } |
| +// execDeliveryVerdict is the DECIDED half of exec_delivery resolution |
| +// (issue #195): given a route's bound bundle and its app's default one, |
| +// what does the flag say? Pure and log-free, so that the two callers who |
| +// need the answer — SyncExec's checkOptedIn, which writes the files, and |
| +// AwaitingRestart, which tells an operator a file was written that nothing |
| +// has read — reach it through one function rather than through two |
| +// readings of one rule. A notice that disagreed with the sweep about who |
| +// gets a file would be worse than no notice: it would name the wrong |
| +// route, in the exact place an operator went looking for certainty. |
| +// |
| +// Fetch FAILURES are deliberately not modelled here. They are not |
| +// determinations (see checkOptedIn's def.err branch), and the two callers |
| +// owe them different things — a loud log line and a left-alone file on one |
| +// side, a named "could not tell" host on the other. A caller that could |
| +// not read a bundle must not call this. |
| +type execDeliveryVerdict struct { |
| + // On is the only question SyncExec asks: does this bundle get files? |
| + On bool |
| + // Unknown is non-empty when the DECIDING bundle stated a value this |
| + // binary does not understand — decided "off", but for a reason worth |
| + // saying out loud (additive discipline, AGENTS.md: a manifest a newer |
| + // carlos wrote must not brick an older box, and must not pass silently |
| + // either). Carried out rather than logged in place, because the two |
| + // callers word it differently and print it to different places. |
| + Unknown string |
| + // FromEnvironment records WHICH bundle decided, so a caller's message |
| + // can name it. False means the app default decided — including the |
| + // common case where a named bundle was silent and inherited. |
| + FromEnvironment bool |
| +} |
| + |
| +// resolveExecDelivery applies the #195 precedence: the route's OWN bundle |
| +// first, and only when it STATES something; the app default otherwise. |
| +// Absence is inheritance, never "off" — an operator who has never |
| +// mentioned exec_delivery in a bundle has not asked for delivery to stop. |
| +// |
| +// loadDefault is a thunk rather than a value because the default bundle |
| +// must not be FETCHED when the named one already decided: eager fetching |
| +// would let an unrelated blip on the default's read turn a decided verdict |
| +// into an unresolved one, and would cost a bucket read per call on the |
| +// path where the answer was already in hand. |
| +func resolveExecDelivery(envM *Manifest, envFound bool, loadDefault func() (*Manifest, bool, error)) (execDeliveryVerdict, error) { |
| + if envFound && envM.ExecDelivery == ExecDeliveryFile { |
| + return execDeliveryVerdict{On: true, FromEnvironment: true}, nil |
| + } |
| + if envFound && envM.ExecDelivery != "" { |
| + return execDeliveryVerdict{Unknown: envM.ExecDelivery, FromEnvironment: true}, nil |
| + } |
| + defM, defFound, err := loadDefault() |
| + if err != nil { |
| + return execDeliveryVerdict{}, err |
| + } |
| + switch { |
| + case !defFound: |
| + return execDeliveryVerdict{}, nil |
| + case defM.ExecDelivery == ExecDeliveryFile: |
| + return execDeliveryVerdict{On: true}, nil |
| + case defM.ExecDelivery == "": |
| + return execDeliveryVerdict{}, nil |
| + default: |
| + return execDeliveryVerdict{Unknown: defM.ExecDelivery}, nil |
| + } |
| +} |
| + |
| +// RestartHost names one route a config write delivered a file to and that |
| +// nothing bounced. Unit is carried because it decides the REMEDY, and the |
| +// two remedies are not interchangeable: an addr route naming its own unit |
| +// (issue #110's consoles) is the one addr shape `carlos restart` can |
| +// actually cycle, where a plain one reaches nothing at all — the agent's |
| +// restart leg skips it (internal/host/restart.go, the rt.Unit != "" test) |
| +// and the activator it would otherwise ask has no child for a process |
| +// carlos never spawned. Telling an operator to run a command that is a |
| +// documented no-op for their route would be a second silent failure |
| +// answering the first. |
| +type RestartHost struct { |
| + Host string |
| + Unit string |
| +} |
| + |
| +// RestartPending is what a config write left behind on ONE box: files that |
| +// landed for processes carlos does not own, and therefore does not restart. |
| +// |
| +// Unresolved is the honest half and the reason this is not a plain |
| +// []string. A route whose exec_delivery could not be resolved this call — |
| +// its bundle unreadable, or the bundle it is bound to missing outright — |
| +// is a route nothing can say "delivered" or "not delivered" about. Dropping |
| +// it silently would commit the very conflation this notice exists to end: |
| +// "nothing needs a restart" and "I could not tell" rendered identically is |
| +// how discussion #16 got filed in the first place. |
| +type RestartPending struct { |
| + Hosts []RestartHost |
| + Unresolved []string |
| +} |
| + |
| +// Empty reports whether there is nothing to say — no host delivered to and |
| +// none unresolved. The renderers' one test, so "say nothing" is one |
| +// judgement rather than four. |
| +func (r RestartPending) Empty() bool { return len(r.Hosts) == 0 && len(r.Unresolved) == 0 } |
| + |
| +// AwaitingRestart answers, for a config write to app's envName bundle |
| +// ("" = the default), which of this box's routes were handed a file that |
| +// nothing will make their process read. It is the reporting half of the |
| +// ownership line SyncExec draws: carlos writes these files and |
| +// deliberately never bounces the processes behind them, so the only thing |
| +// that can close the loop is telling the person who just ran the command. |
| +// See discussion #16 — `secrets set` reported success, `secrets list` |
| +// confirmed the key, and the value reached nothing for want of one line. |
| +// |
| +// keys are the var names the write touched (set and unset together), used |
| +// ONLY to skip routes whose merged config the write cannot have changed — |
| +// see the shadowing test below. An empty slice disables that narrowing, |
| +// which is what a write with no vars in it (turning exec_delivery on) |
| +// needs: there is nothing to shadow, and the file appears regardless. |
| +// |
| +// Eligibility is execAddrCandidate plus the opt-in, which is exactly |
| +// SyncExec's inScope for the addr shape — reached through the same |
| +// predicate and the same resolveExecDelivery, from the same file, for the |
| +// same reason ExecConfigApps is exported from here. |
| +// |
| +// SCOPE, and it is a real limit: routes is whatever the caller could see, |
| +// which is one box's registry (the #145 blind spot internal/console/ |
| +// apiroutes.go documents). This function cannot know what it was not |
| +// shown, so its callers say what they looked at rather than letting a |
| +// short list read as a complete one. |
| +func AwaitingRestart(ctx context.Context, s release.Store, routes []registry.Route, acct, app, envName string, keys []string) RestartPending { |
| + var out RestartPending |
| + // Both caches exist for the same reason SyncExec's do: a box with many |
| + // TCP-addr routes on one bundle must not cost more than one read for |
| + // it. Unlike SyncExec's, they live for one call. |
| + var ( |
| + defM *Manifest |
| + defFound bool |
| + defErr error |
| + defDone bool |
| + ) |
| + loadDefault := func() (*Manifest, bool, error) { |
| + if !defDone { |
| + defM, defFound, defErr = getIfExists(ctx, s, acct, app) |
| + defDone = true |
| + } |
| + return defM, defFound, defErr |
| + } |
| + type envRes struct { |
| + m *Manifest |
| + found bool |
| + err error |
| + } |
| + envBundles := map[string]envRes{} |
| + loadEnv := func(env string) envRes { |
| + if e, done := envBundles[env]; done { |
| + return e |
| + } |
| + m, found, err := getIfExistsAt(ctx, s, EnvironmentKey(acct, app, env)) |
| + e := envRes{m: m, found: found, err: err} |
| + envBundles[env] = e |
| + return e |
| + } |
| + for _, rt := range routes { |
| + if !execAddrCandidate(rt) || rt.App != app || release.RouteAccount(rt) != acct { |
| + continue |
| + } |
| + // Which routes a write REACHES. A named bundle is read by the |
| + // routes bound to it and by nothing else. The default bundle is |
| + // read by every route of the app, bound or not, because a named |
| + // bundle is an overlay ON it (environments.go, LoadMerged) rather |
| + // than a replacement for it. |
| + if envName != "" && rt.Environment != envName { |
| + continue |
| + } |
| + eb := envRes{} |
| + if rt.Environment != "" { |
| + eb = loadEnv(rt.Environment) |
| + if eb.err != nil { |
| + out.Unresolved = append(out.Unresolved, rt.Host) |
| + continue |
| + } |
| + // A route bound to a bundle that does not exist resolves to an |
| + // ERROR, not to the app default: LoadMerged refuses it, and |
| + // SyncExec leaves the route's file exactly as it was. Nothing |
| + // was delivered, so "delivered" would be a lie — but neither |
| + // is this route fine, so it is not a skip either. |
| + if !eb.found { |
| + out.Unresolved = append(out.Unresolved, rt.Host) |
| + continue |
| + } |
| + } |
| + v, err := resolveExecDelivery(eb.m, eb.found, loadDefault) |
| + if err != nil { |
| + out.Unresolved = append(out.Unresolved, rt.Host) |
| + continue |
| + } |
| + // Not opted in — including the Unknown case, which decides "off" |
| + // — means no file was ever written for this route, by this write |
| + // or any other. Nothing to say. |
| + if !v.On { |
| + continue |
| + } |
| + // The shadowing test, and the reason keys is a parameter: a write |
| + // to the DEFAULT bundle changes what a bound route resolves to |
| + // only for keys that route's own bundle does not state. Where every |
| + // touched key is shadowed, the merged manifest is byte-identical, |
| + // syncExecHost's bytes.Equal writes nothing, and naming the route |
| + // would send someone to restart a process for a value it was never |
| + // going to see. |
| + // |
| + // Only for a default-bundle write reaching a BOUND route: a write |
| + // to the route's own bundle is never shadowed, and an unbound |
| + // route has nothing above it. |
| + if envName == "" && rt.Environment != "" && len(keys) > 0 && allShadowed(keys, eb.m) { |
| + continue |
| + } |
| + out.Hosts = append(out.Hosts, RestartHost{Host: rt.Host, Unit: rt.Unit}) |
| + } |
| + // Sorted so a caller's output — and any test pinning it — is stable |
| + // across passes, where registry order is not. Same discipline as |
| + // ExecConfigApps'. |
| + sort.Slice(out.Hosts, func(i, j int) bool { return out.Hosts[i].Host < out.Hosts[j].Host }) |
| + sort.Strings(out.Unresolved) |
| + return out |
| +} |
| + |
| +// allShadowed reports whether every one of keys is STATED by m — the test |
| +// that decides whether a default-bundle write can have changed what a route |
| +// bound to m actually resolves to. An unset key counts the same as a set |
| +// one: removing a key the overlay shadows changes the merge by nothing. |
| +func allShadowed(keys []string, m *Manifest) bool { |
| + if m == nil { |
| + return false |
| + } |
| + for _, k := range keys { |
| + if _, ok := m.Vars[k]; !ok { |
| + return false |
| + } |
| + } |
| + return true |
| +} |
| + |
| // execAddrConfigHeader is prepended to a TCP-addr (issue #18) route's |
| // <host>.config.env — and ONLY that shape. RenderEnv itself stays |
| // header-free, deliberately: it is shared by sync.go's unit path AND this |
diff --git a/internal/console/apiconfig.go b/internal/console/apiconfig.go| index 722b97bb3..24d7911d4 100644 |
| --- a/internal/console/apiconfig.go |
| +++ b/internal/console/apiconfig.go |
| @@ -469,7 +469,91 @@ func (s *Server) handleAPIConfigPut(w http.ResponseWriter, r *http.Request) { |
| s.audit(acct, app, addr, "exec-delivery", "exec-delivery "+*req.ExecDelivery+" ("+bundle+") (cli)") |
| s.bumpSerial(ctx) |
| } |
| - writeJSON(w, http.StatusOK, map[string]any{"result": "ok", "removed": removed}) |
| + out := map[string]any{"result": "ok", "removed": removed} |
| + // Whether this write landed on a process that will never read it |
| + // (discussion #16). Computed AFTER both writes, because it describes |
| + // what they left behind, and only when something actually changed: an |
| + // unset of keys that were not there removed nothing and moved no file, |
| + // so there is nothing for anyone to restart over. |
| + if len(sets) > 0 || removed > 0 || req.ExecDelivery != nil { |
| + out["restart"] = s.configRestartNotice(ctx, acct.Sqid, app, envName, writtenKeys(sets, req.Unset)) |
| + } |
| + writeJSON(w, http.StatusOK, out) |
| +} |
| + |
| +// apiConfigRestart is the config door's answer to "did that reach |
| +// anything?" — the wire half of appconfig.AwaitingRestart. |
| +// |
| +// Known is the field that carries the whole point. A console with no |
| +// registry, or one whose registry will not open, has not looked and must |
| +// not answer with an empty list: "nothing needs restarting" and "I could |
| +// not tell" rendering identically is the failure discussion #16 reported, |
| +// and answering it with a second instance of itself would be worse than |
| +// saying nothing. The CLI renders the two differently, and a client that |
| +// predates the field sees no `restart` key at all — see apimode.go, where |
| +// the field is a pointer for exactly that third state. |
| +type apiConfigRestart struct { |
| + Known bool `json:"known"` |
| + // Hosts got a file that nothing will bounce. Unit, when set, is the |
| + // systemd unit that owns the process — the difference between an |
| + // operator who can be pointed at a command and one who has to reach |
| + // for their own supervisor. |
| + Hosts []apiConfigRestartHost `json:"hosts,omitempty"` |
| + // Unresolved got neither a yes nor a no: their bundle would not read, |
| + // or the bundle they are bound to is missing. |
| + Unresolved []string `json:"unresolved,omitempty"` |
| +} |
| + |
| +type apiConfigRestartHost struct { |
| + Host string `json:"host"` |
| + Unit string `json:"unit,omitempty"` |
| +} |
| + |
| +// configRestartNotice resolves the notice for one config write, or reports |
| +// that it could not. |
| +// |
| +// SCOPE: the console reads its OWN box's registry, so in a multi-box fleet |
| +// the other boxes' routes are not represented here — the same blind spot |
| +// handleAPIRoutes documents at the top of apiroutes.go, and internal/ |
| +// fleetapi (#145) is the eventual fix. The CLI says what it looked at |
| +// rather than letting a short list read as a complete one. |
| +func (s *Server) configRestartNotice(ctx context.Context, acct, app, envName string, keys []string) apiConfigRestart { |
| + if s.cfg.Registry == "" { |
| + // Opening "" would hand sqlite a throwaway temp DB and answer |
| + // "no routes" having created a file to say it — handleAPIRoutes' |
| + // twin guard, and the same reasoning. |
| + return apiConfigRestart{} |
| + } |
| + reg, err := openRegistry(s.cfg.Registry) |
| + if err != nil { |
| + log.Printf("console: api config: registry %s: %v", s.cfg.Registry, err) |
| + return apiConfigRestart{} |
| + } |
| + routes, err := reg.List() |
| + if err != nil { |
| + log.Printf("console: api config: registry list: %v", err) |
| + return apiConfigRestart{} |
| + } |
| + p := appconfig.AwaitingRestart(ctx, s.cfg.Releases, routes, acct, app, envName, keys) |
| + out := apiConfigRestart{Known: true, Unresolved: p.Unresolved} |
| + for _, h := range p.Hosts { |
| + out.Hosts = append(out.Hosts, apiConfigRestartHost{Host: h.Host, Unit: h.Unit}) |
| + } |
| + return out |
| +} |
| + |
| +// writtenKeys is the var names one config write touched, set and unset |
| +// together — what AwaitingRestart needs to tell whether a default-bundle |
| +// write can have changed anything a BOUND route resolves to. Sets and |
| +// unsets are one list because the shadowing question does not care which |
| +// it was: a key the route's own bundle states is a key this write did not |
| +// move for that route, whichever way it moved elsewhere. |
| +func writtenKeys(sets map[string]appconfig.Var, unset []string) []string { |
| + out := make([]string, 0, len(sets)+len(unset)) |
| + for k := range sets { |
| + out = append(out, k) |
| + } |
| + return append(out, unset...) |
| } |
| // handleAPIConfigSync answers `carlos env sync`: touch the app's generation |
diff --git a/internal/console/apiconfigrestart_test.go b/internal/console/apiconfigrestart_test.go| new file mode 100644 |
| index 000000000..4cd98f43b |
| --- /dev/null |
| +++ b/internal/console/apiconfigrestart_test.go |
| @@ -0,0 +1,208 @@ |
| +package console |
| + |
| +// The config door's restart notice (discussion #16): a write that lands on |
| +// a process carlos does not own comes back naming it, so the CLI can say so |
| +// instead of reporting a bare success. And the empty-secret refusal from the |
| +// same report, which this door can enforce without a key because AES-GCM is |
| +// length-preserving. |
| + |
| +import ( |
| + "context" |
| + "net/http" |
| + "strings" |
| + "testing" |
| + |
| + "github.com/carlosframework/platform/internal/appconfig" |
| + "github.com/carlosframework/platform/internal/release" |
| +) |
| + |
| +// restartOf pulls the `restart` object out of a config PUT's response. |
| +// found==false means the door said nothing about it at all, which is a |
| +// distinct answer from an empty one and is asserted as such below. |
| +func restartOf(t *testing.T, out map[string]any) (map[string]any, bool) { |
| + t.Helper() |
| + r, ok := out["restart"].(map[string]any) |
| + return r, ok |
| +} |
| + |
| +// restartHosts lists the hostnames in a restart object, in order. |
| +func restartHosts(t *testing.T, r map[string]any) []string { |
| + t.Helper() |
| + raw, _ := r["hosts"].([]any) |
| + var out []string |
| + for _, h := range raw { |
| + m, _ := h.(map[string]any) |
| + out = append(out, str(t, m, "host")) |
| + } |
| + return out |
| +} |
| + |
| +// optIn turns exec_delivery on for an app's default bundle — the opt-in that |
| +// makes a TCP-addr route eligible for a delivered file in the first place. |
| +func optIn(t *testing.T, fx *consoleFixture, app string) { |
| + t.Helper() |
| + if err := appconfig.SetExecDelivery(context.Background(), fx.rel, release.OpsAccount, app, appconfig.ExecDeliveryFile); err != nil { |
| + t.Fatalf("SetExecDelivery: %v", err) |
| + } |
| +} |
| + |
| +// TestAPIConfigPutNamesRoutesAwaitingRestart is the door's half of the |
| +// report. routesFixture's connect.go.tito.io is exactly the shape that was |
| +// filed: a hand-added TCP-addr relay bound to the "relay" environment, with |
| +// no instance record anywhere and nothing that will ever bounce it. |
| +func TestAPIConfigPutNamesRoutesAwaitingRestart(t *testing.T) { |
| + fx, token := routesFixture(t, "code-restart-named") |
| + optIn(t, fx, "titogo") |
| + |
| + status, out := putJSON(t, configURL(fx, "titogo"), map[string]any{ |
| + "environment": "relay", |
| + "set": map[string]any{"LOG_LEVEL": map[string]any{"kind": "env", "value": "debug"}}, |
| + }, token) |
| + if status != http.StatusOK { |
| + t.Fatalf("PUT = %d %v", status, out) |
| + } |
| + r, ok := restartOf(t, out) |
| + if !ok { |
| + t.Fatalf("response carries no restart object: %v", out) |
| + } |
| + if r["known"] != true { |
| + t.Fatalf("known = %v, want true — this console has a registry and read it", r["known"]) |
| + } |
| + got := restartHosts(t, r) |
| + if len(got) != 1 || got[0] != "connect.go.tito.io" { |
| + t.Fatalf("hosts = %v, want just connect.go.tito.io", got) |
| + } |
| +} |
| + |
| +// TestAPIConfigPutRestartUnknownWithoutRegistry is the field that carries |
| +// the whole point. A console that cannot see a registry has not looked, and |
| +// must not answer with an empty list: "nothing needs restarting" and "I |
| +// could not tell" rendering identically IS the bug being fixed. |
| +func TestAPIConfigPutRestartUnknownWithoutRegistry(t *testing.T) { |
| + fx, token := routesFixture(t, "code-restart-noreg") |
| + optIn(t, fx, "titogo") |
| + fx.srv.cfg.Registry = "" |
| + |
| + status, out := putJSON(t, configURL(fx, "titogo"), map[string]any{ |
| + "environment": "relay", |
| + "set": map[string]any{"LOG_LEVEL": map[string]any{"kind": "env", "value": "debug"}}, |
| + }, token) |
| + if status != http.StatusOK { |
| + t.Fatalf("PUT = %d %v", status, out) |
| + } |
| + r, ok := restartOf(t, out) |
| + if !ok { |
| + t.Fatalf("response carries no restart object: %v", out) |
| + } |
| + if r["known"] != false { |
| + t.Fatalf("known = %v, want false for a console with no registry", r["known"]) |
| + } |
| + if hosts := restartHosts(t, r); len(hosts) != 0 { |
| + t.Fatalf("hosts = %v, want none alongside known:false", hosts) |
| + } |
| +} |
| + |
| +// TestAPIConfigPutRestartSilentForUnoptedApp: the notice appears only where |
| +// a file is actually delivered. An app that never opted into exec delivery |
| +// has routes, and they get nothing, so there is nothing to restart over — |
| +// and this is a genuine "none", not a "could not tell". |
| +func TestAPIConfigPutRestartSilentForUnoptedApp(t *testing.T) { |
| + fx, token := routesFixture(t, "code-restart-unopted") |
| + |
| + status, out := putJSON(t, configURL(fx, "titogo"), map[string]any{ |
| + "environment": "relay", |
| + "set": map[string]any{"LOG_LEVEL": map[string]any{"kind": "env", "value": "debug"}}, |
| + }, token) |
| + if status != http.StatusOK { |
| + t.Fatalf("PUT = %d %v", status, out) |
| + } |
| + r, ok := restartOf(t, out) |
| + if !ok { |
| + t.Fatalf("response carries no restart object: %v", out) |
| + } |
| + if r["known"] != true { |
| + t.Fatalf("known = %v, want true — the registry read fine, the answer is genuinely none", r["known"]) |
| + } |
| + if hosts := restartHosts(t, r); len(hosts) != 0 { |
| + t.Fatalf("hosts = %v, want none for an app that never opted in", hosts) |
| + } |
| +} |
| + |
| +// TestAPIConfigPutRestartAbsentWhenNothingChanged: an unset that removed |
| +// nothing moved no file, so the door does not speculate about restarts. |
| +func TestAPIConfigPutRestartAbsentWhenNothingChanged(t *testing.T) { |
| + fx, token := routesFixture(t, "code-restart-noop") |
| + optIn(t, fx, "titogo") |
| + // The bundle has to exist: unsetting from one that does not is a 409 |
| + // refusal, which is a different answer than "removed nothing". |
| + if status, out := putJSON(t, configURL(fx, "titogo"), map[string]any{ |
| + "environment": "relay", |
| + "set": map[string]any{"REAL_KEY": map[string]any{"kind": "env", "value": "v"}}, |
| + }, token); status != http.StatusOK { |
| + t.Fatalf("seed PUT = %d %v", status, out) |
| + } |
| + |
| + status, out := putJSON(t, configURL(fx, "titogo"), map[string]any{ |
| + "environment": "relay", |
| + "unset": []string{"NEVER_SET"}, |
| + }, token) |
| + if status != http.StatusOK { |
| + t.Fatalf("PUT = %d %v", status, out) |
| + } |
| + if r, ok := restartOf(t, out); ok { |
| + t.Fatalf("restart = %v, want the field absent when the write removed nothing", r) |
| + } |
| +} |
| + |
| +// TestAPIConfigPutRefusesEmptySecret pins the door's half of the empty-value |
| +// refusal (the report's aside). The console holds no private key, so it |
| +// reads emptiness the only way it can: AES-GCM is length-preserving, so an |
| +// envelope at exactly the floor sealed nothing. |
| +// |
| +// The refusal must also leave the bucket untouched — a rejected write that |
| +// still stored something would be worse than the silence it replaces. |
| +func TestAPIConfigPutRefusesEmptySecret(t *testing.T) { |
| + fx, token := configFixture(t, "code-config-empty-secret") |
| + _, _, envelope := sealedFor(t, "") |
| + |
| + status, out := putJSON(t, configURL(fx, "titogo"), map[string]any{ |
| + "set": map[string]any{"CLIENT_SECRET": map[string]any{"kind": "secret", "envelope": envelope}}, |
| + }, token) |
| + if status != http.StatusBadRequest { |
| + t.Fatalf("PUT = %d %v, want 400", status, out) |
| + } |
| + msg := str(t, out, "error") |
| + for _, want := range []string{"empty value", "secrets unset", "env set"} { |
| + if !strings.Contains(msg, want) { |
| + t.Fatalf("refusal %q does not mention %q", msg, want) |
| + } |
| + } |
| + if strings.Contains(msg, envelope) { |
| + t.Fatalf("refusal echoes the envelope back: %q", msg) |
| + } |
| + if _, found := storedManifest(t, fx, "titogo"); found { |
| + t.Fatal("a refused empty secret still wrote a manifest") |
| + } |
| +} |
| + |
| +// TestAPIConfigPutAcceptsOneByteSecret is the boundary from the other side: |
| +// the shortest real secret is one byte longer than an empty one, and the |
| +// length read must never eat it. |
| +func TestAPIConfigPutAcceptsOneByteSecret(t *testing.T) { |
| + fx, token := configFixture(t, "code-config-one-byte") |
| + _, _, envelope := sealedFor(t, "x") |
| + |
| + status, out := putJSON(t, configURL(fx, "titogo"), map[string]any{ |
| + "set": map[string]any{"CLIENT_SECRET": map[string]any{"kind": "secret", "envelope": envelope}}, |
| + }, token) |
| + if status != http.StatusOK { |
| + t.Fatalf("PUT = %d %v, want 200 for a one-byte secret", status, out) |
| + } |
| + m, found := storedManifest(t, fx, "titogo") |
| + if !found { |
| + t.Fatal("a one-byte secret was accepted but stored nothing") |
| + } |
| + if m.Vars["CLIENT_SECRET"].Envelope != envelope { |
| + t.Fatal("stored envelope is not the one that was sent") |
| + } |
| +} |