amadan
Merge branch 'worktree-config-write-restart-notice' into main
12 files changed,
+1381
−84
apimode.go+46 −8apimode_test.go+9 −3appconfig_cli.go+237 −23appconfig_restart_test.go+142 −0cli.go+17 −2docs/site/cli.md+11 −0internal/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
CI passed — run details
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..a23bf26c1 100644 |
| --- a/appconfig_cli.go |
| +++ b/appconfig_cli.go |
| @@ -8,15 +8,154 @@ 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\n", c.warn("note:"), line) |
| + } |
| + for _, h := range a.pending.Unresolved { |
| + fmt.Fprintf(w, "%s %s\n", c.warn("note:"), fmt.Sprintf(restartCopyUnresolved, h)) |
| + } |
| + // Once, under the list, rather than on every line: the caveat is about |
| + // the LIST — what this box could see — so repeating it per host would be |
| + // three copies of one sentence saying nothing more the third time. |
| + fmt.Fprintf(w, "%s\n", c.muted(restartCopyScope)) |
| +} |
| + |
| +// 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 ( |
| + // Carries its own "secrets set:" prefix, so its one call site prints it |
| + // bare rather than prefixing it again. |
| + secretsCopyEmpty = "secrets set: %s: empty value: nothing stored. To remove a secret use `carlos secrets unset`; for a deliberately empty value use `carlos env set`. (A command substitution that produced nothing is the usual cause.)" |
| + restartCopyPlain = "%s has the file but is still running the old values. carlos routes to this process without owning it. Restart to fix." |
| + restartCopyUnit = "%s has the file but is still running the old values. carlos restart --app %s cycles %s." |
| + restartCopyUnresolved = "could not tell whether %s picked this up. Its config bundle would not load." |
| + restartCopyUnknown = "could not check which routes are still running the old values. This console cannot see the route registry." |
| + restartCopyScope = "(this machine's routes only)" |
| +) |
| + |
| const envUsage = `usage: carlos env <command> |
| commands: |
| @@ -203,7 +342,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 +365,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 +420,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 +555,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 +595,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 +615,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 |
| @@ -559,9 +733,15 @@ func runEnvSync(args []string) { |
| log.Fatal(err) |
| } |
| } |
| - fmt.Printf("%s for %s — exec-backed instances converge within seconds; "+ |
| - "unit-backed routes within seconds where the box's carlos-adopt.path trigger is armed, "+ |
| - "else on the next hourly root pass\n", colorFor(os.Stdout).ok("config sync requested"), appName) |
| + // The files, not the processes. The old wording promised that |
| + // "exec-backed instances converge within seconds", which is true of the |
| + // file and false of the program reading it for exactly the route shape |
| + // discussion #16 was filed about — and this is the command people run |
| + // while waiting for a change to take effect, so overstating it sends |
| + // them off to debug the wrong thing. |
| + fmt.Printf("%s for %s the files land within seconds; programs carlos owns restart themselves, "+ |
| + "and anything else keeps its old values until you restart it\n", |
| + colorFor(os.Stdout).ok("config sync requested"), appName) |
| } |
| const secretsUsage = `usage: carlos secrets <command> |
| @@ -656,7 +836,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 +846,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.Fatal(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 +903,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 +925,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 +955,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..7ebdff591 |
| --- /dev/null |
| +++ b/appconfig_restart_test.go |
| @@ -0,0 +1,142 @@ |
| +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) |
| + } |
| + } |
| + // One line per host, plus the single trailing scope caveat. |
| + if n := strings.Count(got, "\n"); n != 3 { |
| + t.Fatalf("output has %d lines, want one per host plus one caveat:\n%s", n, got) |
| + } |
| + if n := strings.Count(got, restartCopyScope); n != 1 { |
| + t.Fatalf("scope caveat appears %d times, want exactly once:\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/docs/site/cli.md b/docs/site/cli.md| index aef13a492..22561c166 100644 |
| --- a/docs/site/cli.md |
| +++ b/docs/site/cli.md |
| @@ -343,6 +343,12 @@ seconds and refuses such a row by name. `carlos instances create` takes |
| `--environment` too, which is how a route is born on the right bundle rather |
| than moved onto it. |
| +Some routes are ones carlos proxies to without owning the process behind them. |
| +Setting config for one of those writes the file and stops there. The program |
| +carries on with the old values until its own supervisor restarts it. `set` and |
| +`unset` say so, naming the routes it applies to, and say when they could not |
| +work it out rather than staying quiet about it. |
| + |
| ### carlos secrets |
| The same shape as `carlos env`, sealed. `list` prints key names and never |
| @@ -356,6 +362,11 @@ Sealing uses a public key; the private half lives on the box that decrypts |
| them and never leaves it, in either mode. `carlos secrets genkey` mints the |
| pair, and it is always local to your terminal. |
| +An empty value is refused rather than stored. A command substitution that |
| +produced nothing looks exactly like a real value at the point you type it, and |
| +a stored empty secret usually surfaces much later as a feature quietly not |
| +working. Use `carlos env set` for a value that is meant to be empty. |
| + |
| ### carlos instances |
| An app's instances are the places it actually runs. `carlos instances enable` |
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..922ec2c3d 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`. 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") |
| + } |
| +} |