Add ProxyAdminService with member-aware responses - #250
Conversation
liam-lowe
changed the title
7ae10e4 to
27537ff
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| name: lint, breaking and generation drift - pull request | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v5 |
There was a problem hiding this comment.
Opengrep — security.gha.unpinned-action (WARNING)
Unpinned action reference actions/checkout@v5: this uses: resolves a mutable ref (tag or branch), so the code that runs in CI can change without this line changing. A compromised upstream can repoint the tag and execute arbitrary code with access to this repository's secrets and GITHUB_TOKEN (tj-actions/changed-files, March 2025). Pin to the full 40-character commit SHA with the resolved version in a trailing comment, e.g. uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2. Prefer deputy pin --ecosystems github-actions, which resolves the ref, writes a version comment that reflects the most specific ref actually pointing at that commit, and verifies the SHA is reachable from a real branch upstream. That last check matters: pinning alone does not detect imposter or dangling commits, and this rule only sees the shape of the ref, never its provenance. Reusable workflow calls (owner/repo/.github/workflows/x.yml@ref) run with the same trust as actions and are pinned the same way. Not reported, by campaign policy: temporalio/* refs (first-party, pinned by internal process), local ./ actions, self-repository $/ refs (resolve to the running commit, so they are already pin-equivalent), and docker:// images (pinned by digest as a separate ecosystem).
Fixed in 3b226c3
Fixed in c50f001
Fixed in 8c28d08
Fixed in 79ec5ce
| fetch-depth: 0 | ||
|
|
||
| - name: Install buf | ||
| uses: bufbuild/buf-setup-action@v1 |
There was a problem hiding this comment.
Opengrep — security.gha.unpinned-action (WARNING)
Unpinned action reference bufbuild/buf-setup-action@v1: this uses: resolves a mutable ref (tag or branch), so the code that runs in CI can change without this line changing. A compromised upstream can repoint the tag and execute arbitrary code with access to this repository's secrets and GITHUB_TOKEN (tj-actions/changed-files, March 2025). Pin to the full 40-character commit SHA with the resolved version in a trailing comment, e.g. uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2. Prefer deputy pin --ecosystems github-actions, which resolves the ref, writes a version comment that reflects the most specific ref actually pointing at that commit, and verifies the SHA is reachable from a real branch upstream. That last check matters: pinning alone does not detect imposter or dangling commits, and this rule only sees the shape of the ref, never its provenance. Reusable workflow calls (owner/repo/.github/workflows/x.yml@ref) run with the same trust as actions and are pinned the same way. Not reported, by campaign policy: temporalio/* refs (first-party, pinned by internal process), local ./ actions, self-repository $/ refs (resolve to the running commit, so they are already pin-equivalent), and docker:// images (pinned by digest as a separate ecosystem).
Fixed in 3b226c3
Fixed in c50f001
Fixed in 8c28d08
Fixed in 79ec5ce
3b226c3 to
c50f001
Compare
liam-lowe
changed the title
s2s-proxy.mergedConfig only wrote clusterConnections back into the merged result, so every other top-level configOverride key was silently discarded. Setting configOverride.metrics or configOverride.profiling rendered the default from files/default.yaml with no error. Merge the remaining top-level keys with mergeOverwrite. The source is deepCopy'd because sprig mutates its first argument and the clusterConnections loop above already writes defaults into .Values in place. The deployment test worked around this by nesting metrics under clusterConnections[0], but ClusterConnConfig has no metrics field and config loading uses KnownFields(true), so that config would be rejected by the binary. Move it to the top level and drop the dead per-connection lookup in parsedPorts that allowed it.
c50f001 to
8c28d08
Compare
proxy/debug.go mapped session states to strings inline. The admin API and a metrics gauge both need the same mapping, and three copies of a switch over MuxSessionState is three places to miss when a fourth state appears. CountSessions also exposes the breakdown itself, which nothing did before: num_muxes_active is the size of the session map and mux_connection_active is per session, so a session failing its health check but still in the map reads as healthy in both. DesiredMuxCount is exported alongside it because the sessions a manager holds mean nothing without the count it was configured to hold, and a caller reporting that ratio should not carry its own copy of the default.
The schema, the generated stubs, the make target and the drift workflow are one unit: CI regenerates and diffs api/, so a change to any of them without the others fails the gate. Requests carry no routing fields. Scope and target travel as gRPC metadata, because an interceptor cannot read a request field without reflection, so with fields every listener's limits would have to be re-checked inside every handler and any RPC added later would be exposed until someone remembered.
Each discovery provider gets its own typed block, selected by name. Every layered configuration tool in this stack deep-merges and cannot delete keys, so switching provider through a Helm override leaves the previous provider's block behind and it has to be inert. A single flat options map could not be switched at all under strict decoding. ProxyAdmin is a value rather than a pointer because nil and an empty listen address already mean the same thing. The counterparty method list is deliberately not resolved through auth.AccessControl, whose IsAllowed returns true for an empty list. That fail-open default is a compatibility promise the replication ACL depends on, and repeating it here would make the natural spelling of "off" the widest possible setting.
Serve answers one RPC at whatever scope the caller asked for and the listener allows: this process, this deployment, or once across a mux to another organization. Endpoints supply four functions and inherit deadline budgeting, discovery, concurrency capping, dial lifecycle and the unreachable roster. Three properties are structural rather than conventions each handler has to remember. Serve applies the narrowing View last and refuses to answer a counterparty that has none. The peer listener refuses to forward, which is what bounds a group call to a single round of fan-out. And a method outside the admin service passes through untouched, because on a mux this interceptor is installed server-wide and also sees every replication call. Merge takes members as a slice rather than a pre-folded value, so an endpoint whose aggregate is not a sum can still be expressed: shard ownership is disjoint across pods, and a configuration check wants to know whether the members agree rather than what they add up to. Self is recognized after the call, not before it. A DNS record carries no identity, so every discovered address is dialed, including this pod's own, and a reply bearing our own id is discarded.
Wires the admin plane into the proxy: each cluster connection describes its own mux and local cluster state, one instance of the service answers on every listener, and the merge folds the members' answers into one. NewProxy gains an identity and an error return. The identity is supplied rather than read from config, because a value in the shared config file would be identical on every replica and deduplication by id would collapse the whole deployment into a single member. The error return replaces a Fatal-then- continue that silently dropped a cluster connection when the log component was disabled. Three listeners, three roles. The loopback operator listener is trusted local access. The peer listener serves sibling pods and refuses to forward. The mux listener is reached by another organization, so it serves only listed methods and its answer is built field by field rather than by deleting fields, so a field added to the proto later is not served across an organizational boundary until someone adds it there. That withholds this deployment's shape, and it withholds LocalCluster, whose shard count and failover version increment are the raw values adminservice.DescribeCluster rewrites for that same caller. Peer TLS does not reuse encryption.GetServerTLSConfig, which sets RequireAnyClientCert and replaces VerifyPeerCertificate with a function that logs the subject and returns nil, so it never checks the chain. On a listener bound to the pod network the certificate is the only thing distinguishing a sibling from anything else that can reach it. A cached probe answers whether this proxy can reach the Temporal cluster it fronts, which no mux state can. It calls the frontend directly rather than through the proxy's own DescribeCluster handler, and it runs on a timer because a two second dial inside a two second member budget would turn a slow frontend into a member that looks unreachable.
The admin API answers an operator who asks. Alerting needs the same facts without one, and the chart leaves that API's listener on loopback. The session state breakdown is what nothing exported before. mux_connection_active is set to 1 on every observer tick until the session's lifetime ends and num_muxes_active is the size of the session map, so a session failing its ping but still in the map reads as healthy in both, and their label sets do not join in PromQL. The target gauge is the denominator that makes the breakdown alertable: a session that never established was never added to the manager, so the sessions held say nothing on their own about how many were meant to exist. The sampler reads mux.CountSessions, the same function the admin API reads, so the metric and the endpoint cannot disagree. It samples before waiting rather than after, unlike the mux observer, so a connection that lives less than one interval is not invisible. Sampling faster would not detect a failure faster: a session is marked errored by its own health check, which pings about once a minute. The admin listeners had no gRPC metrics at all. They reuse the registered server metrics rather than declaring a second collector, because a second grpcprom.ServerMetrics under the same namespace and subsystem panics at process start whether or not its label list matches. Metrics wrap the admin interceptor so its rejections land in grpc_code. Also registers GRPCIntraProxyClientMetrics, which was constructed and used but never registered, so intra-proxy client metrics were collected and never scraped.
The operator listener ships enabled on loopback, so a query needs only an exec or a port-forward rather than a config change. The peer listener stays commented out. It is the part that lets one pod answer for the whole deployment, and it binds the pod network, so it should be a decision rather than a default. When dns discovery is selected the name defaults to this release's headless Service. That Service already publishes one A record per endpoint, so nothing about it changes; without the default every install would hand-write a cluster-internal DNS name, which is the kind of configuration alignment this endpoint exists to avoid. POD_NAME comes through the downward API so each pod names itself in an aggregated response. Two members reporting the same id are indistinguishable from one member answering twice. The configmap tests cover the case the config shape was designed around: switching discovery provider through an override leaves the previous provider's block behind, and the binary must still accept it under strict decoding.
8c28d08 to
79ec5ce
Compare
1 participant
ProxyAdminServiceanswersDescribeClusterConnectionsfor a whole proxy deployment rather than for whichever pod received the call, and a cross-cluster query asks the far proxy to do the same for its own.A proxy runs as a Deployment with N pods, so a one-pod answer is incomplete. It is also non-deterministic:
MultiClientConnload balances across mux sessions (transport/grpcutil/grpc.go:21), so a cross-cluster query lands on an arbitrary far pod and two identical calls can disagree.Eight commits, meant to be read in order. The merge squashes them, so they are for review rather than for
main's history. The first five are independent units (a shared helper, the proto, config, the engine); the sixth is the proxy wiring; the last two are metrics and the chart. Every commit builds and passesmake lintandmake teston its own, verified withgit rebase --exec.Routing lives in metadata, not request fields
DescribeClusterConnectionsRequestis empty. Two headers control how far a call travels:s2s-proxy-scope:memberorgroup. Absent meansgroup.s2s-proxy-target: a cluster connection name, forwarded once to that connection's peer proxy.An interceptor cannot read a request field without reflection. With per-message fields, each listener's limits have to be re-checked inside every handler, and any RPC added later is exposed until someone remembers. One interceptor now covers every RPC the service will ever have.
Three listeners, three roles
:60610.0.0.0:9234The peer listener refusing to forward is what bounds a group call to a single round of fan-out. The mux interceptor is installed server-wide and also sees replication traffic, so anything outside the admin service passes through untouched; there is a test for exactly that.
Access control
A compile-time table keyed by the generated method constants carries two independent booleans per RPC: whether another organization may call it on us, and whether we may put it to them. Both default to false, so a new RPC is neither served across an organizational boundary nor sent to one until someone decides it should be.
Operators can narrow the counterparty side further with
aclPolicy.allowedMethods.proxyAdmin. Absent means the compile-time ceiling; an empty list serves nothing, which is how you decline to answer one counterparty. A name outside the ceiling is a startup error, so configuration can only narrow. Note this differs from the siblingadminServicelist, whose empty value means allow everything: that fail-open default is a compatibility promise the replication ACL depends on, and repeating it here would make the natural spelling of "off" the widest possible setting.Two bugs this fixes in the earlier draft of the branch
responding: 0. The chart default leaves the peer block off, which leftDialnil, andfanOuttook its first early return with the count never set. Four early-return paths shared it.proxy.member_id, per-pod versions and sibling pod addresses in place, and kept each connection'slocal_cluster, whoseHistoryShardCountandFailoverVersionIncrementare read raw from the frontend. Those are the valuesproxy/adminservice.go:148-162rewrites before answering that same counterparty'sDescribeCluster, so the admin endpoint served the untranslated numbers around the existing translation. The response is now built field by field, so a field added to the proto later is not exposed until someone adds it there.Known gap, not addressed here
Counterparty controls are defence in depth rather than access control, because the mux does not authenticate the caller.
encryption.GetServerTLSConfigsetsClientAuth = tls.RequireAnyClientCertand replacesVerifyPeerCertificatewith a function that logs the subject and returns nil, so a counterparty is authenticated only by presenting some certificate andremoteCAPathis decorative on that listener. Fixing it touches the replication transport and needs a coordinated rollout, so it is filed separately. The peer listener does verify, and there is a test that a certificate from another CA is refused.Also deferred: authentication on the operator listener (loopback by documentation today), a per-connection outbound ACL, and a configured session count on
ClusterConnectionMemberso the RPC reports the same denominator ascluster_connection_mux_sessions_target.Metrics
The session state breakdown is the gap nothing else fills:
mux_connection_activeis set to 1 on every observer tick until the session's lifetime ends andnum_muxes_activeis the size of the session map, so a session failing its ping but still in the map reads as healthy in both, and their label sets do not join in PromQL.cluster_connection_mux_sessions{config_name,state}and..._target(the denominator; a session that never established was never added, so sessions held says nothing about how many were meant to exist)cluster_connection_local_cluster_reachable{config_name}build_info{version}The sampler reads
mux.CountSessions, the same function the admin API reads, so the metric and the endpoint cannot disagree. Also registersGRPCIntraProxyClientMetrics, which was constructed and used but never registered, so those metrics were collected and never scraped.Testing
make lint(0 issues),make test,make bins,make helm-test(9/9),buf lintwith no generated-code drift,example.yamlregenerated, andgo test -raceover the prober and fan-out. Each of the eight commits was verified independently withgit rebase --exec.New tests, and the behaviour each pins:
adminplane/serve_test.go: fan-out against a fake discovery and dialer, no network. The roster invariant across all four early-return paths, budget exhaustion, the concurrency cap, self-dedupe including when the ID function is absent, and the fail-closed guard on a missingView.adminplane/role_test.go: the listener role matrix, the compile-time ceiling, operator narrowing including the empty-list off switch, and that a non-admin method passes through a counterparty listener untouched.proxy/proxyadmin_tls_test.go: a peer certificate signed by another CA is refused, which is the claimpeerServerTLSConfigexists to make. Uses an in-process CA and leaf, because the committed fixtures are two self-signed leaves that each trust the other and cannot express "wrong CA".proxy/local_cluster_probe_test.go,transport/mux/state_test.go,proxy/connection_metrics_test.go: previously uncovered.Four of the security-relevant tests were checked by reverting the code they guard and confirming they fail.