Skip to content

clusterd: remove unused CTP server-FQDN validation - #36876

Closed
jasonhernandez wants to merge 1 commit into
mainfrom
jason/distroless-ctp-rip-fqdn
Closed

clusterd: remove unused CTP server-FQDN validation#36876
jasonhernandez wants to merge 1 commit into
mainfrom
jason/distroless-ctp-rip-fqdn

Conversation

@jasonhernandez

@jasonhernandez jasonhernandez commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Removes the CTP server_fqdn handshake check that CLUSTERD_GRPC_HOST fed. Despite the name it has nothing to do with gRPC (which is now only persist pubsub). In the CTP handshake, clusterd advertised its FQDN and the controller compared it against the address it dialed, failing on mismatch (transport.rs::handshake). That check is:

  • optional — only fires when both sides set a value, so it's already a no-op when the host is unset;
  • narrow — guards only against reaching a misrouted/stale replica (DNS / pod-IP reuse);
  • misnamed, which is what prompted the review question on the previous PR.

The distroless migration removes entrypoint.sh, which set CLUSTERD_GRPC_HOST via hostname --fqdn. Rather than re-plumb that, this removes the feature: drops --grpc-host/CLUSTERD_GRPC_HOST, the server_fqdn field from the CTP Hello, the host_from_address helper, and the test_handshake_fqdn_mismatch test.

This is rebased to a single commit on main and stands alone: it touches only clusterd/service, not entrypoint.sh or any Dockerfile, so it can merge ahead of the distroless image PR (#36099).

Notes

  • Wire change: Hello loses a field. CTP version-gates the handshake (mismatched versions fail and reconnect), so this is safe across a release boundary.
  • test_metrics byte-count bounds were loosened (the handshake shrank).

This is the "rip it out" answer to the FQDN question; it makes #36100 (in-process resolve) unnecessary.

Test plan

  • cargo check -p mz-clusterd -p mz-service -p mz-compute-client -p mz-storage-controller (rustc 1.96.0)
  • cargo test -p mz-service --test transport — green across 10 consecutive runs
  • Confirm controller↔replica CTP connects in a k8s/kind cluster — verified via cloudtest on kind: test_smoke (test_wait: default cluster u1 replica reached Ready, i.e. the CTP handshake succeeded; test_sql) and test_testdrive (multi-replica: c1 with r1/r2, c2 at replication factor 2, Kafka source, materialized views) all pass. Confirmed the deployed clusterd binary no longer exposes --grpc-host.

🤖 Generated with Claude Code

@jasonhernandez
jasonhernandez force-pushed the jason/distroless-ctp-rip-fqdn branch from d27b55c to 7e976bd Compare June 2, 2026 20:24
jasonhernandez added a commit that referenced this pull request Jun 2, 2026
Distroless containers run the binary directly as PID 1 (no tini/shell). On
Linux, PID 1 ignores signals with a SIG_DFL disposition, so SIGTERM from
Kubernetes pod termination would be silently dropped. Install an explicit
termination-signal handler in clusterd (environmentd already has one), and
derive CLUSTERD_PROCESS from the StatefulSet ordinal in-process (previously
done by the entrypoint.sh that distroless removes).

Minimal distroless-lifecycle change. #36876 stacks on this to remove the
now-unused CTP server-FQDN validation.

Part of SEC-236 distroless migration.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@jasonhernandez
jasonhernandez force-pushed the jason/distroless-ctp-rip-fqdn branch from 7e976bd to d5356e4 Compare June 2, 2026 20:34
@jasonhernandez
jasonhernandez changed the base branch from main to jason/distroless-orchestrator-fqdn June 2, 2026 20:34
@jasonhernandez jasonhernandez changed the title clusterd: SIGTERM handler + rip out CTP server-FQDN validation (Variant C) clusterd: remove unused CTP server-FQDN validation Jun 2, 2026
@jasonhernandez
jasonhernandez force-pushed the jason/distroless-orchestrator-fqdn branch from 2efe3f1 to ec0fd3c Compare July 1, 2026 18:40
@jasonhernandez

Copy link
Copy Markdown
Contributor Author

🔗 Distroless migration — coordination

Removes the unused CTP server-FQDN check, whose only supplier (CLUSTERD_GRPC_HOST, set by the shell entrypoint) goes away with the distroless migration (#36099).

Stacks on #36872. Off the critical path: a missing CLUSTERD_GRPC_HOST already makes the check a no-op (it only fires when both sides set a value), so this is pure dead-code cleanup and can land any time after #36872 rather than being gated on the cutover.

Part of the set: #36099 (image), #36101 (UID/GID gating), #36872 (clusterd process ordinal). Supersedes the FQDN half of the closed #36100.

@jasonhernandez
jasonhernandez force-pushed the jason/distroless-ctp-rip-fqdn branch 2 times, most recently from aa3c4f0 to 1a72ee3 Compare July 1, 2026 21:51
@jasonhernandez
jasonhernandez force-pushed the jason/distroless-orchestrator-fqdn branch from ec0fd3c to 0931afb Compare July 1, 2026 22:26
@jasonhernandez
jasonhernandez force-pushed the jason/distroless-ctp-rip-fqdn branch from 1a72ee3 to 75fd2b5 Compare July 1, 2026 22:26
jasonhernandez added a commit that referenced this pull request Jul 3, 2026
…36872)

## Summary

Distroless clusterd has no shell entrypoint to set `CLUSTERD_PROCESS`
(the process ordinal), which `entrypoint.sh` derived from the pod
hostname before the distroless migration (#36099) removes it. clusterd
now derives it itself when running under Kubernetes and the value is
unset: the ordinal is the last `-`-delimited segment of the StatefulSet
pod name, matching how orchestrator-kubernetes parses the process id
back out of pod names.

No signal handling is added. tini (added to the distroless image in
#36099) runs as PID 1 and forwards signals, so clusterd is a child
process and SIGTERM takes its default action rather than being ignored
by PID 1.

Part of the distroless migration: #36099 (image), #36101 (UID/GID
gating), #36876 (removes the unused CTP FQDN check, stacks on this).
Supersedes the closed #36100.

## Merge order

Land before or with #36099. Without it, distroless clusterd has no
process ordinal and won't start. Safe to land first: on the current
Ubuntu images `entrypoint.sh` still sets `CLUSTERD_PROCESS`, and the
in-process derivation only runs when it is unset.

## Test plan

- [x] `cargo check -p mz-clusterd`
- [ ] Confirm a distroless clusterd pod in k8s picks up its ordinal and
joins the cluster

Co-authored-by: Jason Hernandez <7144515+jasonhernandez@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Base automatically changed from jason/distroless-orchestrator-fqdn to main July 3, 2026 00:14
Stacks on #36872 (SIGTERM handler). Removes the optional CTP `server_fqdn`
handshake check: clusterd advertised its FQDN (via CLUSTERD_GRPC_HOST, set by
the now-removed entrypoint.sh) and the controller compared it to the address
it dialed. The check only fired when the value was set, is unrelated to gRPC
despite the name, and guards only against reaching a misrouted replica.

Drops `--grpc-host`/`CLUSTERD_GRPC_HOST`, the `server_fqdn` field from the CTP
`Hello`, the `host_from_address` helper, and the `test_handshake_fqdn_mismatch`
test. CTP version-gates the handshake, so dropping the field is fine across a
release boundary.

Part of SEC-236 distroless migration.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@jasonhernandez
jasonhernandez force-pushed the jason/distroless-ctp-rip-fqdn branch from 75fd2b5 to 07f25fa Compare July 7, 2026 19:12
@jasonhernandez
jasonhernandez marked this pull request as ready for review July 7, 2026 21:07
@jasonhernandez
jasonhernandez requested a review from a team as a code owner July 7, 2026 21:07

@def- def- left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the QA LLM review:

MEDIUM - Removing the CTP host check leaves no process-identity validation for partitioned replica connections

This PR removes --grpc-host/CLUSTERD_GRPC_HOST, stops passing a server identity into transport::serve, and reduces the CTP Hello handshake to only a semver check (src/service/src/transport.rs:395-431). After this change, a controller that dials a stale or misrouted replica address accepts any clusterd process running the same Materialize semver.

That is a real correctness contract in this code path, not just defense in depth. Managed Kubernetes replicas build an ordered address list from StatefulSet pod DNS names (src/orchestrator-kubernetes/src/lib.rs:688-698). The controller then constructs a Partitioned client from that ordered list, and the connection index determines which command shard is sent and how responses are reassembled (src/service/src/client.rs:141-148, :158-164). Compute makes this especially sensitive because all commands except Hello and UpdateConfiguration are sent only to shard 0 (src/compute-client/src/service.rs:368-379).

Before this PR, managed clusterd pods set CLUSTERD_GRPC_HOST from hostname --fqdn, so the handshake rejected a connection where the dialed host did not match the server's own FQDN. That caught the exact stale-DNS or pod-IP-reuse case called out in the PR description. After this PR, the same stale address can silently connect to a different clusterd process of the same semver. The new connection also cancels any existing CTP connection on that process (src/service/src/transport.rs:144-151), so the failure mode can corrupt the controller's partition mapping and disrupt the correctly connected replica at the same time.

The Kubernetes orchestrator explicitly uses zero termination grace and documents that duplicate pod identities and delayed network packets are part of the model (src/orchestrator-kubernetes/src/lib.rs:1207-1228). In that environment, pod DNS and pod-IP reuse are not hypothetical enough to remove the only peer identity check without replacing it. A safer fix is to keep the check until CTP carries an explicit controller-assigned process identity, preferably including at least cluster, replica, process ordinal, and generation, and rejects peers whose identity does not match the address slot being connected.

jasonhernandez added a commit that referenced this pull request Jul 13, 2026
…ge (#37526)

Migrate the `environmentd` and `clusterd` container images from Ubuntu
to a distroless base, shrinking the image and its attack
surface.<br>This is a lighter approach compared to
#36099 / #36101 /
#36876 which attempted to entirely remove the
shell. I've closed those.

<br>We keep a shell, but it becomes busybox and we aim for pure POSIX /
sh compatibility instead of implicitly depending on bash.

## Base image

* Uses `gcr.io/distroless/cc-debian13:debug-nonroot`. The `debug`
variant ships a busybox shell at `/busybox/sh`. Keeping a shell lets
`entrypoint.sh` and `kubectl exec` continue to work. This is a
deliberate, incremental step off Ubuntu; dropping the shell is a
possible future hardening.
* Runs as a `materialize` user with uid/gid **999**, created in a Debian
builder stage and copied into the final image (distroless has no shell
to run `useradd`). uid/gid 999 matches the pod securityContext the
orchestrator already applies (`runAsUser`/`runAsGroup`/`fsGroup = 999`),
so nothing on the orchestrator side changes and existing uid-999
persistent volumes need no re-chown.

## Runtime dependencies copied in

The distroless base is minimal, so the image explicitly provides what
the binaries need at runtime:

* **static** `tini` as PID 1, to forward signals and reap zombies (e.g.
ssh-tunnel subprocesses).
* **static** `ssh`, since distroless ships none and SSH tunnels require
it. (busybox includes some SSH features but we prefer to use openssh
built with aws-lc for eventual FIPS support + stronger cryptographic
foundations)
* `libfdb_c.so` (FoundationDB) and `liblzma.so.5` (xz), both dynamically
linked by the binaries. Without `liblzma` they abort at startup with
`error while loading shared libraries`.
* a `nsswitch.conf` using the `files`/`dns` backends, which avoids a
glibc `compat`-backend segfault in the statically-linked `ssh`.

This might look ugly, but we know what we're shipping vs. the entire
Ubuntu userspace!

## Entrypoint

`entrypoint.sh` (for both binaries) is POSIX `sh`: `#!/bin/sh`, `set
-eu`, and `hostname -f` (works under both busybox and GNU). The
Dockerfiles invoke it as `/busybox/sh entrypoint.sh`, since distroless
has no `/bin/sh`. Under Kubernetes it advertises the pod FQDN for the
CTP peer check and derives the process ordinal from the StatefulSet pod
name.

## Scope

Container layer only, no product-code changes: image definitions, the
two entrypoints, cloudtest adjustments (pod processes are signalled via
`kubectl exec … sh -c 'kill'`), and dependabot coverage for the new base
images.

## Test plan

- [X] Standalone image on the pinned base runs as uid 999 with
`HOME=/home/materialize`; the entrypoint executes end-to-end and
`hostname -f` resolves.
- [X] CI nightly cloudtest / orchestratord / k8s-recovery jobs pass:
managed replicas come up healthy as uid 999 and the CTP check passes
against the in-cluster pod FQDN. (One `Cloudtest` shard flakes
independently of this change; it also fails on `main`.)

---------

Co-authored-by: Jason Hernandez <7144515+jasonhernandez@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants