service: CTP controller-assigned process identity (RFC, alt to #36876)#37515
Draft
jasonhernandez wants to merge 1 commit into
Draft
service: CTP controller-assigned process identity (RFC, alt to #36876)#37515jasonhernandez wants to merge 1 commit into
jasonhernandez wants to merge 1 commit into
Conversation
The CTP handshake previously carried the server's DNS FQDN and rejected a connection when the dialed host and the replica's advertised FQDN disagreed. The `--grpc-host` flag that fed it is vestigial (the command transport is CTP, not gRPC). Replace the mechanism with an opaque, controller-assigned process identity carried in the same handshake slot. The compare logic is unchanged: both sides carry an `Option<String>`, and the handshake rejects the connection only when both are present and differ. `None` on either side skips the check, preserving current behavior for the local orchestrator and tests. Wiring: - transport: rename `Hello.server_fqdn` to `peer_identity`; thread an explicit `expected_identity` through `Client::connect`; `connect_partitioned` now takes `(address, expected_identity)` pairs. - clusterd: replace `--grpc-host`/`GRPC_HOST` with `--ctp-identity`/ `CTP_IDENTITY`. - controller: expect each replica process to advertise `<cluster_id>/<replica_id>/<ordinal>` via `ClusterReplicaLocation. ctl_identities`, and pass the replica-scoped prefix to clusterd, which appends its own ordinal. The ordinal cannot be baked into the launch arg because Kubernetes StatefulSet pods share one arg template. This catches wrong-replica and wrong-ordinal misrouting. It does not yet catch a same-identity stale pod, because generation/incarnation is intentionally not part of the identity yet (open design question). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
43caa52 to
f2930d2
Compare
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft / RFC. Alternative to #36876 for the CTP host-check question, per review discussion.
Problem
The distroless migration (#36099) deletes
clusterd/ci/entrypoint.sh, which setCLUSTERD_GRPC_HOST=$(hostname --fqdn). That value fed the CTP handshake'sserver_fqdncheck (src/service/src/transport.rs), which rejects a controller connection whose dialed host does not match the replica's own FQDN. So #36099 silently disables that guard on Kubernetes, and #36876 proposed removing it outright.Review (thanks @def-) pushed back: the check is a real correctness guard, not just defense in depth. The controller builds an ordered replica address list and the
Partitionedclient uses the connection index to route command shards (compute sends all commands to shard 0), so a connection that lands on the wrong process can corrupt the partition mapping, and the new connection cancels the legitimate one. Kubernetes runs replicas with zero termination grace and reused pod identities, so stale DNS / pod-IP reuse are realistic.Why not just re-plumb the FQDN
We could feed the pod FQDN back to distroless clusterd (downward API / an arg) and keep the existing check. But the mechanism is vestigial: the command transport is CTP (a custom bincode-over-TCP protocol), not gRPC. The
--grpc-hostname is a leftover from when it was gRPC. Since we own CTP, we can carry a proper controller-assigned identity instead of a DNS hostname.This change
Replace the
server_fqdnstring in the CTPHellowith a controller-assigned identity token, verified per address slot. The handshake compare logic is unchanged (symmetric: reject when both sides present a value and they differ). Only the value and how each side obtains it change:<cluster_id>/<replica_id>/<ordinal>and expects that exact value for the corresponding address slot (src/controller/src/clusters.rs).src/clusterd/src/lib.rs). The controller passes the replica-scoped prefix<cluster_id>/<replica_id>via--ctp-identity; clusterd appends its own--processordinal. (The ordinal cannot be baked into a per-pod arg because Kubernetes StatefulSet pods share one arg template.)ClusterReplicaLocationcarries actl_identitieslist parallel toctl_addrs, so the expected identity flows from launch to connect.--grpc-host/CLUSTERD_GRPC_HOSTis replaced by--ctp-identity/CTP_IDENTITY.Option:Noneon either side disables the check, preserving current behavior for the process orchestrator and unmanaged replicas.This restores the guard for managed replicas and retires the misleading
grpcnaming.Open design questions (the point of the draft)
<cluster>/<replica>/<ordinal>with no incarnation component. It catches wrong-replica and wrong-ordinal misrouting, but not a same-identity stale pod (an old incarnation of the same replica/ordinal reachable at a reused IP). @def-'s suggestion was to include a generation. Closing this needs a durable incarnation source that both the controller's expected value and the pod's advertised value derive from. Where should that live (replica config generation? a controller-assigned per-launch epoch)?Test plan
cargo check --workspacecargo test -p mz-service --test transport(incl. renamedtest_handshake_identity_mismatch)🤖 Generated with Claude Code