amadan

carlos / platform Public

relay: fold Addrs in the delivery check, state the master exposure in the docs, restart the relay on update, adopted only for tcp-exec-file, no empty meter samples (final review)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev 95ac196fce0c93f851231c33003af5ad691edcb7 parent 6af806d
9 files changed, +232 −33
  • docs/site/cli.md +24 −0
  • internal/console/apirelay.go +2 −1
  • internal/console/apirelay_test.go +72 −0
  • internal/console/relayops.go +40 −1
  • internal/console/relayops_test.go +8 −1
  • internal/host/systemapply.go +18 −8
  • internal/host/systemapply_test.go +24 −21
  • internal/relay/meter.go +12 −1
  • internal/relay/meter_test.go +32 −0

CI passed — run details

diff --git a/docs/site/cli.md b/docs/site/cli.md
index f82a76c93..2d732cee8 100644
--- a/docs/site/cli.md
+++ b/docs/site/cli.md
@@ -701,6 +701,30 @@ a master to that secret issues nothing on its own — `rotate` is what moves
the region onto it, and it refuses until every live relay there reports the
new key id, so a rotation cannot mint credentials nothing can verify.
+Know what that console copy exposes before you stage it. The console holds
+every region's master as sealed config so it can derive an app's keys.
+Sealed config is decryptable on every host, so a compromised host can read
+every region's master and, with a master, mint credentials for any app in
+that region, present and future, until the operator rotates. Per-region
+masters mean a relay box holds only its own region's, and rotating one
+region recovers that region.
+
+Four console variables turn the relay on for a deployment.
+`CARLOS_RELAY_REGIONS` is a comma-separated list of the regions that run
+one, in the order an app hands them to the browser; unset means this
+deployment has no relay, and `enable` refuses with a sentence naming the
+variable. `CARLOS_RELAY_OPERATORS` is the addresses allowed to run
+`rotate`. `CARLOS_RELAY_MASTERS_<REGION>` carries the masters themselves as
+`<kid>:<key>[,<kid>:<key>]`, the region uppercased with hyphens as
+underscores. `CARLOS_PLATFORM_DOMAIN` is the domain under which
+`relay.<region>.` resolves; unset, it defaults to the apps domain with its
+first label dropped, which is an inference to verify against your own DNS
+rather than assume. The rest is infrastructure: the `A` record for
+`relay.<region>.<domain>` and the security group's `relay = true` are staged
+in the deployment's Terraform, region by region. Arm platform boxes only —
+a fleet box is the customer's own hardware, and a master staged there mints
+credentials for every app in the region.
+
`serve` is what `carlos-relay.service` execs. The unit only starts on a box
that has a masters file, so a box with no key never listens; a box with one
answers STUN to anyone and TURN to credentials an enabled app minted. It
diff --git a/internal/console/apirelay.go b/internal/console/apirelay.go
index 2a152c026..18d8c8fda 100644
--- a/internal/console/apirelay.go
+++ b/internal/console/apirelay.go
@@ -103,7 +103,8 @@ func relayDoorError(w http.ResponseWriter, verb string, err error) {
jsonError(w, http.StatusServiceUnavailable, err.Error())
case errors.Is(err, errRelayUnknownHost), errors.Is(err, errRelayNotEnabled):
jsonError(w, http.StatusNotFound, err.Error())
- case errors.Is(err, errRelayUndelivered), relayLooksLikeClientError(err):
+ case errors.Is(err, errRelayUndelivered), errors.Is(err, errRelayNotMemberRoute),
+ relayLooksLikeClientError(err):
jsonError(w, http.StatusBadRequest, err.Error())
default:
log.Printf("console: api relay %s: %v", verb, err)
diff --git a/internal/console/apirelay_test.go b/internal/console/apirelay_test.go
index 64b245b4e..f54ab92c5 100644
--- a/internal/console/apirelay_test.go
+++ b/internal/console/apirelay_test.go
@@ -21,6 +21,7 @@ import (
"time"
"github.com/carlosframework/platform/internal/appconfig"
+ "github.com/carlosframework/platform/internal/provision"
"github.com/carlosframework/platform/internal/registry"
"github.com/carlosframework/platform/internal/relay"
)
@@ -226,6 +227,77 @@ func TestAPIRelayAdoptedUnknownHostIs404(t *testing.T) {
}
}
+// TestAPIRelayAdoptedRefusesARouteThePlatformRestarts: the member verb
+// exists for the ONE route shape this platform cannot restart. A unit or
+// socket-exec route is bounced by the config sync itself, so its cutover is
+// an observation — the ack that sync writes, naming what the host actually
+// rendered. A member's word here would stamp the identities the console
+// ISSUED over that observation, and a route whose restart quietly failed
+// would then read as cut over and let the old master be retired under it.
+func TestAPIRelayAdoptedRefusesARouteThePlatformRestarts(t *testing.T) {
+ fx, _ := relayFixture(t, "code-relay-adopted-shape")
+ token := cliToken(t, fx, "alice@keymail.dev", "code-relay-adopted-shape-2")
+ fx.addRoute(t, registry.Route{Host: "s.titogo.oncarlos.com", App: "titogo",
+ Kind: "service", Socket: "/run/titogo-s.sock", Status: "ready"})
+ if status, out := postJSON(t, relayURL(fx, "titogo"), map[string]any{}, token); status != http.StatusOK {
+ t.Fatalf("enable = %d %v, want 200", status, out)
+ }
+
+ status, out := postJSON(t, relayURL(fx, "titogo")+"/adopted",
+ map[string]any{"host": "s.titogo.oncarlos.com"}, token)
+ if status != http.StatusBadRequest {
+ t.Fatalf("adopted for a socket-exec route = %d %v, want 400", status, out)
+ }
+ if msg := str(t, out, "error"); !strings.Contains(msg,
+ "the platform restarts this route itself; its adoption is observed, not declared") {
+ t.Fatalf("error = %q, want the reason in those words", msg)
+ }
+ if rec := relayRecord(t, fx, "titogo"); rec != nil && len(rec.Cutovers) != 0 {
+ t.Fatalf("a refused ack wrote a cutover: %+v", rec.Cutovers)
+ }
+}
+
+// TestAPIRelayMultiUpstreamRouteIsRefusedThenReadsAsTCPExecFile: a record
+// names its upstream EITHER as the legacy Addr or as the Addrs set, never
+// both (provision.ValidateShape), so a multi-upstream external route has an
+// empty Addr. Read unfolded, it looked like a route with no address at all —
+// which the delivery check reads as deliverable — so the enable spec §5 step
+// 2 exists to refuse went through, status called the route "unknown", and
+// `carlos relay adopted` accepted it. Both halves are pinned here: the
+// refusal names the host, and once the bundle opts in, the same route reads
+// as the tcp-exec-file it is.
+func TestAPIRelayMultiUpstreamRouteIsRefusedThenReadsAsTCPExecFile(t *testing.T) {
+ fx, _ := relayFixture(t, "code-relay-addrs")
+ ctx := context.Background()
+ token := cliToken(t, fx, "alice@keymail.dev", "code-relay-addrs-2")
+ if err := provision.CreateRecord(ctx, fx.rel, provision.Record{
+ Schema: 1, Host: "m.titogo.oncarlos.com", Slug: "m", App: "titogo",
+ Account: fx.sqid, Kind: "service", Channel: "stable",
+ Addrs: []string{"10.0.0.5:8080", "10.0.0.6:8080"},
+ CreatedAt: time.Now().UTC(),
+ }); err != nil {
+ t.Fatalf("seed a multi-upstream record: %v", err)
+ }
+
+ status, out := postJSON(t, relayURL(fx, "titogo"), map[string]any{}, token)
+ if status != http.StatusBadRequest {
+ t.Fatalf("enable over a multi-upstream route with no file delivery = %d %v, want 400", status, out)
+ }
+ if msg := str(t, out, "error"); !strings.Contains(msg, "m.titogo.oncarlos.com") {
+ t.Fatalf("error = %q, want the host named", msg)
+ }
+
+ if err := appconfig.SetExecDelivery(ctx, fx.rel, fx.sqid, "titogo", appconfig.ExecDeliveryFile); err != nil {
+ t.Fatal(err)
+ }
+ if status, out := postJSON(t, relayURL(fx, "titogo"), map[string]any{}, token); status != http.StatusOK {
+ t.Fatalf("enable after opting into file delivery = %d %v, want 200", status, out)
+ }
+ _, out = getJSON(t, relayURL(fx, "titogo"), token)
+ wantRoute(t, relayRouteRows(t, out), "m.titogo.oncarlos.com", "tcp-exec-file",
+ "written, awaiting delivery")
+}
+
// ---- the gate ----------------------------------------------------------------
// TestAPIRelayGateIsByteIdenticalWithTheStore: every app-scoped relay door
diff --git a/internal/console/relayops.go b/internal/console/relayops.go
index 8de68407b..3c26d297f 100644
--- a/internal/console/relayops.go
+++ b/internal/console/relayops.go
@@ -130,6 +130,13 @@ var (
// errRelayUnknownHost is `carlos relay adopted --host` naming something
// that is not a route of this app — the door's 404.
errRelayUnknownHost = errors.New("no such route on this app")
+ // errRelayNotMemberRoute is `carlos relay adopted --host` naming a route
+ // the config sync restarts itself. Spec §5, step 5 gives a route exactly
+ // one way to establish its cutover, decided by shape: a unit or
+ // socket-exec route's cutover is the ack the config sync writes after it
+ // restarted the process, and a member's word would override an observed
+ // fact with a claim.
+ errRelayNotMemberRoute = errors.New("this route's cutover is observed, not declared")
)
// relayLease is control/relay-writer.json.
@@ -802,7 +809,21 @@ func (s *Server) relayAppRoutes(ctx context.Context, acct, app string) ([]relayR
seen := map[string]bool{}
for _, rec := range recs {
seen[rec.Host] = true
- out = append(out, relayRoute{Host: rec.Host, Kind: rec.Kind, Addr: rec.Addr,
+ // Fold Addrs the way the registry branch does (routeAddr, below). A
+ // record picks ONE upstream shape — the legacy Addr or the Addrs set,
+ // never both (provision.ValidateShape) — so a multi-upstream external
+ // route carries Addr == "", and "no address" is exactly what the
+ // delivery check reads as deliverable. Left unfolded, such a route
+ // slips past the refusal enable exists to make, shows as shape
+ // "unknown" in status, and can be acknowledged with `carlos relay
+ // adopted`. What every caller here asks is "is there a process at the
+ // far end this platform never spawned", and the first upstream
+ // answers that as well as five.
+ addr := rec.Addr
+ if all := rec.AddrsOrDefault(); len(all) > 0 {
+ addr = all[0]
+ }
+ out = append(out, relayRoute{Host: rec.Host, Kind: rec.Kind, Addr: addr,
Unit: rec.Unit, Environment: rec.Environment})
}
if s.cfg.Registry == "" {
@@ -1099,6 +1120,24 @@ func (s *Server) relayAdopted(ctx context.Context, acct *Account, app, host, by
return nil, fmt.Errorf("%s has no config delivery, so no relay key has been written for it to adopt — "+
"opt it into file delivery with `carlos env exec-delivery file` or move it to a unit: %w", host, errRelayUndelivered)
}
+ // And the route must be the ONE shape whose cutover a member establishes.
+ // Spec §5, step 5: a unit or socket-exec route is restarted by the config
+ // sync, so its cutover is the ack that sync writes — an observed fact,
+ // with the identities the host actually rendered. Accepting a member's
+ // word for such a route would stamp the identities the console ISSUED
+ // over that observation, and a route whose restart silently failed would
+ // read as cut over. The member verb exists only for the process this
+ // platform cannot restart.
+ switch shape := relayRouteShape(routes[i]); shape {
+ case relayShapeFile: // the one shape a member acknowledges
+ case relayShapeUnit, relayShapeSocket:
+ return nil, fmt.Errorf("%s is a %s route: the platform restarts this route itself; its adoption is observed, not declared — "+
+ "`carlos relay status` reports it as adopted once the config sync acknowledges the write: %w",
+ host, shape, errRelayNotMemberRoute)
+ default:
+ return nil, fmt.Errorf("%s has no unit, no socket and no address, so this console cannot say what would run a relay key, "+
+ "let alone that it has: %w", host, errRelayNotMemberRoute)
+ }
cut := relay.RouteCutover{
Host: host,
Suffixes: maps.Clone(rec.IssuedSuffixes),
diff --git a/internal/console/relayops_test.go b/internal/console/relayops_test.go
index 8044cfe01..603151c2e 100644
--- a/internal/console/relayops_test.go
+++ b/internal/console/relayops_test.go
@@ -490,12 +490,19 @@ func TestRelayEnableRefusesRouteWithoutDelivery(t *testing.T) {
// an ack given before a later rotation cannot count for it — and a host that
// is not a route of this app is refused rather than appended, which is what
// keeps the record bounded.
+//
+// The route is a tcp-exec-file one because that is the only shape a member
+// may acknowledge at all: anything the config sync restarts itself has its
+// cutover observed (TestAPIRelayAdoptedRefusesARouteThePlatformRestarts).
func TestRelayAdoptedRecordsTheIssuedIdentitiesAndRefusesAStranger(t *testing.T) {
fx, _ := relayFixture(t, "code-relay-adopted")
ctx := context.Background()
acct := fx.account(t)
fx.addRoute(t, registry.Route{Host: "titogo.oncarlos.com", App: "titogo",
- Kind: "instance", Status: "ready", Addr: "10.0.0.9:8080"})
+ Kind: "service", Status: "ready", Addr: "10.0.0.9:8080"})
+ if err := appconfig.SetExecDelivery(ctx, fx.rel, fx.sqid, "titogo", appconfig.ExecDeliveryFile); err != nil {
+ t.Fatal(err)
+ }
rec, err := fx.srv.relayEnable(ctx, acct, "titogo", nil, "alice@keymail.dev")
if err != nil {
diff --git a/internal/host/systemapply.go b/internal/host/systemapply.go
index ab64a3b35..f09681f5c 100644
--- a/internal/host/systemapply.go
+++ b/internal/host/systemapply.go
@@ -575,16 +575,26 @@ func (a *systemApplier) restartEdge() error {
// restartRelay mirrors restartEdge for the relay (relay spec §3), with one
// difference that decides its shape: the relay is optional. A box with no
-// /etc/carlos/relay-masters has the unit installed but inactive, because its
-// ConditionPathExists failed, and `try-restart` on it is a no-op that may
-// still report an error. That is not a failure of the update — the edge, the
-// binary and the gate are all fine — so the error is logged and swallowed,
-// and try-restart (not restart) is what keeps an unarmed box from being
-// handed a relay it never asked for.
+// /etc/carlos/relay-masters must not be handed a relay it never asked for.
+//
+// What keeps that promise is the unit's own ConditionPathExists, not the
+// verb: on an unarmed box `restart` starts nothing, because the condition
+// fails and systemd records the unit as skipped — which is exactly what
+// bootstrap already relies on when it starts the unit outright. So the verb
+// here is `restart`, the same one restartEdge uses, and `try-restart` is
+// wrong for a reason that only shows up on a bad day: reset-failed turns a
+// crash-looped relay from failed into inactive, and try-restart does nothing
+// at all to an inactive unit. A relay that died at boot would then stay down
+// until the NEXT platform update happened to run arm(), with the box's
+// heartbeat the only sign.
+//
+// The error is still logged and swallowed. A relay that will not start is not
+// a failure of the update — the edge, the binary and the gate are all fine —
+// and `carlos relay status` is where a dead region is meant to be read.
func (a *systemApplier) restartRelay() {
a.ops.systemctl("reset-failed", systemRelayUnit)
- if _, err := a.ops.systemctl("try-restart", systemRelayUnit); err != nil {
- a.ops.logf("system apply: relay try-restart: %v", err)
+ if _, err := a.ops.systemctl("restart", systemRelayUnit); err != nil {
+ a.ops.logf("system apply: relay restart: %v", err)
}
}
diff --git a/internal/host/systemapply_test.go b/internal/host/systemapply_test.go
index e227ef89f..bf642a8a0 100644
--- a/internal/host/systemapply_test.go
+++ b/internal/host/systemapply_test.go
@@ -693,9 +693,12 @@ func TestApplyRollsBackWhenTheGateFails(t *testing.T) {
// The relay rides the platform binary, so a self-update has to bounce it
// onto the new build the way it bounces the edge — after the edge, and with
-// try-restart rather than restart, because on a box with no region master
-// the unit is inactive by its own ConditionPathExists and there is nothing
-// to restart (relay spec §3).
+// reset-failed then restart. `restart`, not `try-restart`: an unarmed box is
+// kept relay-less by the unit's own ConditionPathExists (bootstrap starts
+// the unit outright and relies on the same thing), while try-restart after a
+// reset-failed cannot revive a relay that crash-looped at boot, because
+// reset-failed has just made it inactive and try-restart skips an inactive
+// unit (relay spec §3).
func TestApplyRestartsTheRelayAfterTheEdge(t *testing.T) {
f, _ := applyHappy(t)
if err := f.a.Run(context.Background(), false); err != nil {
@@ -704,29 +707,29 @@ func TestApplyRestartsTheRelayAfterTheEdge(t *testing.T) {
joined := strings.Join(f.calls, "\n")
edge := strings.Index(joined, "systemctl restart carlos-edge")
reset := strings.Index(joined, "systemctl reset-failed carlos-relay")
- try := strings.Index(joined, "systemctl try-restart carlos-relay")
+ restart := strings.Index(joined, "systemctl restart carlos-relay")
switch {
- case reset < 0 || try < 0:
+ case reset < 0 || restart < 0:
t.Fatalf("relay never restarted:\n%s", joined)
- case !(edge < reset && reset < try):
- t.Fatalf("want restart carlos-edge, then reset-failed then try-restart carlos-relay:\n%s", joined)
+ case !(edge < reset && reset < restart):
+ t.Fatalf("want restart carlos-edge, then reset-failed then restart carlos-relay:\n%s", joined)
}
- // `restart` would start the relay on a box whose masters file the
- // operator never staged. try-restart is the whole point.
- if strings.Contains(joined, "systemctl restart carlos-relay") {
- t.Errorf("relay restarted rather than try-restarted:\n%s", joined)
+ // try-restart is the bug this test now guards against: it would leave a
+ // failed relay down until the next update.
+ if strings.Contains(joined, "systemctl try-restart carlos-relay") {
+ t.Errorf("relay try-restarted, which cannot revive a failed unit:\n%s", joined)
}
}
// A relay that cannot be restarted is not a failed update. The commonest
-// case is the ordinary one — a box with no masters file, whose unit is
-// inactive and whose try-restart systemd may well answer with an error —
-// and the edge, the binary and the gate are all fine.
+// case is the ordinary one — a box with no masters file, whose unit systemd
+// skips on its condition and may well answer about — and the edge, the
+// binary and the gate are all fine.
func TestApplyRelayRestartFailureDoesNotFailTheUpdate(t *testing.T) {
f, req := applyHappy(t)
orig := f.a.ops.systemctl
f.a.ops.systemctl = func(args ...string) (string, error) {
- if args[0] == "try-restart" && args[1] == systemRelayUnit {
+ if args[0] == "restart" && args[1] == systemRelayUnit {
orig(args...) // still recorded
return "", errors.New("Unit carlos-relay.service is not loaded")
}
@@ -739,8 +742,8 @@ func TestApplyRelayRestartFailureDoesNotFailTheUpdate(t *testing.T) {
if r == nil || r.Outcome != systemOutcomeApplied || !r.matches("stable", "v2", req.PromotedAt) {
t.Fatalf("result = %+v", r)
}
- if !f.called("systemctl try-restart " + systemRelayUnit) {
- t.Fatalf("try-restart never attempted:\n%s", strings.Join(f.calls, "\n"))
+ if !f.called("systemctl restart " + systemRelayUnit) {
+ t.Fatalf("restart never attempted:\n%s", strings.Join(f.calls, "\n"))
}
}
@@ -764,15 +767,15 @@ func TestApplyRollbackRestartsTheRelay(t *testing.T) {
}
joined := strings.Join(f.calls, "\n")
edge := strings.LastIndex(joined, "systemctl restart carlos-edge")
- try := strings.LastIndex(joined, "systemctl try-restart carlos-relay")
- if try < 0 || try < edge {
+ relay := strings.LastIndex(joined, "systemctl restart carlos-relay")
+ if relay < 0 || relay < edge {
t.Fatalf("the rollback left the relay on the withdrawn build:\n%s", joined)
}
// Twice over the whole run: once for the apply's restart, once for the
// rollback's — not just the apply's, which would pass a LastIndex check
// by accident if the rollback did nothing.
- if n := strings.Count(joined, "systemctl try-restart carlos-relay"); n != 2 {
- t.Errorf("relay try-restarted %d times, want 2 (apply then rollback):\n%s", n, joined)
+ if n := strings.Count(joined, "systemctl restart carlos-relay"); n != 2 {
+ t.Errorf("relay restarted %d times, want 2 (apply then rollback):\n%s", n, joined)
}
}
diff --git a/internal/relay/meter.go b/internal/relay/meter.go
index 6bc67a32c..4a3f907da 100644
--- a/internal/relay/meter.go
+++ b/internal/relay/meter.go
@@ -136,7 +136,9 @@ func (m *Meter) SetAllocsAll(live map[Identity]int) {
// allocations — is dropped from the sample and from the meter's live set,
// so a burst of short-lived apps does not grow m.cur without bound. An app
// with open allocations is kept (with zero deltas) even while idle, so its
-// allocation gauge keeps being reported.
+// allocation gauge keeps being reported. And a sample left naming no app at
+// all is not written: an idle relay writes nothing rather than a minute-by-
+// minute record of having nothing to say.
func (m *Meter) Flush(ctx context.Context, s release.Store) error {
m.mu.Lock()
now := m.now()
@@ -150,6 +152,15 @@ func (m *Meter) Flush(ctx context.Context, s release.Store) error {
m.cur[id] = &MeterApp{Allocs: a.Allocs} // deltas reset, gauge kept
}
m.mu.Unlock()
+ // Nothing to say, so say nothing. Almost every relay box is idle almost
+ // all of the time, and a sample with no apps in it is a read, a write and
+ // a row of JSON that carries no information — 1,440 of them a day per
+ // box, growing the hourly object a reader has to walk and the CAS chain
+ // that guards it. An hour in which nothing happened is best represented
+ // by an hour with no object.
+ if len(sample.Apps) == 0 {
+ return nil
+ }
hour := metrics.HourOf(now)
key := MeterKey(m.box, hour)
var lastErr error
diff --git a/internal/relay/meter_test.go b/internal/relay/meter_test.go
index 03c7813dc..6c38af22c 100644
--- a/internal/relay/meter_test.go
+++ b/internal/relay/meter_test.go
@@ -137,6 +137,38 @@ func TestMeterEvictsDeadApps(t *testing.T) {
}
}
+// TestMeterFlushOfAnIdleRelayWritesNothing: a sample naming no app carries
+// no information, and a relay box is idle almost all of the time — left
+// unguarded, the flush loop writes 1,440 empty rows a day per box, each one
+// a read, a write, a CAS and a row every reader of the hour has to walk. An
+// hour in which nothing happened is best represented by no object at all.
+func TestMeterFlushOfAnIdleRelayWritesNothing(t *testing.T) {
+ s := newMemStore(t)
+ now := time.Date(2026, 9, 7, 12, 30, 0, 0, time.UTC)
+ m := NewMeter("box1", func() time.Time { return now }, nil)
+ for i := 0; i < 2; i++ {
+ if err := m.Flush(context.Background(), s); err != nil {
+ t.Fatalf("flush %d: %v", i, err)
+ }
+ }
+ keys, err := s.List(context.Background(), "metrics/relay/box1/")
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(keys) != 0 {
+ t.Fatalf("an idle relay wrote %v, want nothing", keys)
+ }
+ // And the first byte that IS forwarded still lands, so this is a skip,
+ // not a mute.
+ m.Add(Identity{"abc", "meet"}, 1, 0, 0)
+ if err := m.Flush(context.Background(), s); err != nil {
+ t.Fatal(err)
+ }
+ if keys, _ := s.List(context.Background(), "metrics/relay/box1/"); len(keys) != 1 {
+ t.Fatalf("after traffic, keys = %v, want the hour's one chunk", keys)
+ }
+}
+
// TestSetAllocsAllZeroesDepartedApps is the leak test. SetAllocs alone can
// only ever raise a gauge, because the ids that need lowering are precisely
// the ones a census of live allocations no longer names — so an app whose