[HYPERSHELL-297] Filter gateways by cluster_id for managed-cluster pull model - #242
[HYPERSHELL-297] Filter gateways by cluster_id for managed-cluster pull model#242markturansky wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This PR cleanly implements the managed-cluster pull model by threading a cluster_id filter from control-plane config through the seed/watch/health/sandbox-count reconcilers and adding a matching optional proto field with server-side filtering. The design is well-reasoned and well-tested; my two Major items are about (a) validating the request-supplied cluster_id before interpolating it into a search filter and (b) the in-code "security boundary" language overstating a guarantee the server does not yet enforce.
Strengths
- The four gateway-list call sites are de-duplicated onto one shared
listAllGateways(ctx, client, clusterID)helper, and the pagination test was correctly updated (250→1200) to reflect the merged helper'sgatewayListPageSize=500. - Per-reconciler filtering semantics are deliberate and documented: namespace GC is intentionally left unfiltered (a superset only ever protects globally-unique namespaces), while health and sandbox-count are scoped so a spoke never stamps or zeroes a foreign gateway.
- Proto change is additive/non-breaking (
optionalfields, new field numbers only), andgateways.pb.gois a faithful regeneration — no hand edits. - Test changes are additive signature updates plus new coverage (
TestSeedGateways_ThreadsClusterID,TestListAllGateways_ClusterIDFilter); no pre-existing assertion was flipped to remove a guarantee.
Findings
[Major] Unvalidated request input interpolated into a search filter — Security / Input Validation (grpc_handler.go:243)
listArgs.Search = fmt.Sprintf("cluster_id = '%s'", clusterID) builds a search-DSL string from req.GetClusterId() with no validation. security.spec.md requires validating all user input (K8s DNS label / KSUID) and preventing injection. The realistic blast radius is bounded — this internal gRPC ListGateways already returns the full fleet when unfiltered (so it is not a confidentiality escalation), and the rh-trex search DSL scopes to the gateways table — but a cluster_id containing a single quote can still break the parse or broaden the caller's own filter. Please validate cluster_id (e.g. grpcutil.ValidateStringField / KSUID check) before interpolation, mirroring how the REST path only inlines already-validated IDs in visibilitySearchFilter. Confidence: Medium.
[Major] "Security boundary" comments overstate an unenforced guarantee — Architecture / Clarity (grpc_handler.go:241, 272)
The comments describe the cluster_id filter as "the security boundary for the pull model," but the api-server does not authenticate that the caller owns the claimed cluster_id: ListGateways/WatchGateways apply no per-caller RBAC, so any control-plane can pass any cluster_id. This is a cooperative scoping filter, not an enforced boundary. The PR body acknowledges deferring the remote gRPC TLS+OIDC dial, so the caveat is understood — please soften the in-code wording (e.g. "cooperative scoping; enforcement pending caller-identity binding") so a future maintainer does not rely on it as a trust boundary. Confidence: High.
[Minor] optionalClusterID duplicated across packages — Maintainability (reconciler.go:1901, watcher.go:369)
The same helper is defined in both reconciler and watcher. This is legal (distinct packages) and low-risk, but a single shared helper would avoid drift. Confidence: High.
Cross-PR coordination
A concurrently proposed control-plane world-synchronization specification defines a periodic "complete inventory" pass that every API-backed reconciler consumes and explicitly builds on the existing Gateway list seed. This PR redefines that seed/list to be cluster_id-scoped server-side for a spoke, so for a managed cluster the "complete inventory" of gateways becomes "complete inventory of this cluster's gateways." The maintainers must decide how that spec's completeness/inventory-watermark contract and its inventory-driven orphan-cleanup composes with per-cluster scoping — and whether the world-sync gateway pass should itself be cluster_id-filtered, consistent with this PR's deliberate choice to leave namespace GC unfiltered. This needs a design decision so the two efforts agree on whether "world" for a spoke means the whole fleet or its own slice.
Findings Summary (ordered by severity, highest first)
- [Major] Unvalidated
cluster_idinterpolated into search filter - Security / Input Validation (grpc_handler.go:243) - [Major] "Security boundary" comments overstate an unenforced guarantee - Architecture / Clarity (grpc_handler.go:241, 272)
- [Minor]
optionalClusterIDduplicated across packages - Maintainability (reconciler.go:1901, watcher.go:369)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound handled for 404 scenarios |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (K8s DNS labels, KSUID) | Fail |
| Reconcile pattern (not create-or-skip) | Pass |
| OpenAPI/proto client not manually edited | Pass |
| Config separate from code | Pass |
| Test Diff Scrutiny (no silently removed guarantees) | Pass |
| Conventional commit message | Pass |
| // server-side keeps foreign gateways off the wire entirely (see | ||
| // WatchGateways, where the same filter is a security boundary). | ||
| if clusterID := req.GetClusterId(); clusterID != "" { | ||
| listArgs.Search = fmt.Sprintf("cluster_id = '%s'", clusterID) |
There was a problem hiding this comment.
[Major] Validate cluster_id before interpolation — Security / Input Validation
listArgs.Search = fmt.Sprintf("cluster_id = '%s'", clusterID) interpolates request-supplied input into a search-DSL string with no validation. security.spec.md requires validating all user input and preventing injection. Blast radius is bounded (this internal handler already lists the full fleet when unfiltered, and the DSL scopes to the gateways table), but a value containing a single quote can break the parse or broaden the filter. Validate cluster_id (KSUID / grpcutil.ValidateStringField) before building the filter, as the REST path does by only inlining already-validated IDs in visibilitySearchFilter.
| // The broker fans EVERY gateway out to EVERY subscriber, so without this a | ||
| // spoke would receive (and could act on) other clusters' gateways. Filtering | ||
| // here is therefore the security boundary for the pull model. | ||
| clusterFilter := req.GetClusterId() |
There was a problem hiding this comment.
[Major] "Security boundary" overstates an unenforced guarantee — Architecture / Clarity
The server does not authenticate that the caller owns the claimed cluster_id: ListGateways/WatchGateways apply no per-caller RBAC, so any control-plane can pass any cluster_id. This is cooperative scoping, not an enforced boundary (the PR body defers the TLS+OIDC dial). Please soften the wording here and at line 241 so a future maintainer doesn't rely on it as a trust boundary, and track the enforcement follow-up.
| // cluster_id field: an empty identity becomes nil (no server-side filter, the | ||
| // single-cluster default), a non-empty one is sent so the api-server scopes the | ||
| // list/watch to that cluster. | ||
| func optionalClusterID(clusterID string) *string { |
There was a problem hiding this comment.
[Minor] Duplicated helper — Maintainability
optionalClusterID is defined identically here and in reconciler.go:1901. Distinct packages so it compiles fine, but a single shared helper would avoid drift.
…ll model A managed-cluster spoke deploys only the control-plane; it dials the hub api-server over gRPC, watches for gateways whose cluster_id matches its own identity, and provisions them locally. Because the event broker fans every gateway out to every subscriber, scoping a spoke to its own cluster is a security boundary, so the filter is applied server-side. api-server: - Add optional cluster_id to ListGatewaysRequest and WatchGatewaysRequest. - ListGateways filters via a cluster_id = '...' search when set. - WatchGateways skips events whose loaded gateway's cluster_id does not match, and (when a filter is set) skips deletes it cannot attribute to a cluster. control-plane: - Add Config.ClusterID from HYPERSHELL_CLUSTER_ID (empty = handle all gateways). - Thread it through the gateway watch and its seed lists. - De-duplicate the two listAllGateways helpers into one taking a clusterID. - Health and sandbox-count reconcilers filter by cluster_id so a spoke never stamps or zeroes a foreign cluster's gateway; the namespace GC stays unfiltered on purpose (its live set only ever protects namespaces, which are globally unique, so a superset can never cause a wrong reap). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
8e81745 to
cd3075e
Compare
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This is a carefully-reasoned, well-tested implementation of per-cluster gateway scoping: the proto change is non-breaking, the four control-plane list call sites are correctly de-duplicated onto one shared helper, and the per-reconciler filtering table (filter watch/seed/health/sandbox, leave namespace GC unfiltered) is sound. Two issues are worth addressing before merge — one input-validation/comment-accuracy issue on the server-side filter, and one contradictory doc comment about whether the filter is a trust boundary.
Major
1. cluster_id is interpolated into the search DSL and the "validate" step does not do what the comment claims — grpc_handler.go:248-251, Security / Input Validation
ListGateways builds the filter as listArgs.Search = fmt.Sprintf("cluster_id = '%s'", clusterID) and the comment states this is safe because ValidateStringField guards against "a value containing a quote." That is not accurate: grpcutil.ValidateStringField only enforces required and max length (255) — it does not reject single quotes or any TSL/search metacharacters. A cluster_id such as x' or name = 'y would pass validation and be spliced into the TSL expression, either failing the parse (BadRequest) or injecting an extra predicate that broadens the result set.
Mitigating facts: the downstream TSL→squirrel path parameterizes literal values, so this is not raw SQL injection, and the handler itself documents that this scoping is "cooperative … not an enforced trust boundary," so a caller broadening its own filter is not a privilege escalation today. That keeps it out of Blocker territory, but the guarantee the comment asserts does not exist. Please either (a) validate cluster_id against a strict charset (K8s DNS-label / KSUID) as security.spec.md prescribes for user input, or (b) build the predicate without string interpolation, and correct the comment so it no longer claims quote-safety it does not provide. The same value flows from operator config (HYPERSHELL_CLUSTER_ID) unvalidated, so a strict charset check would also protect the control-plane's own lists.
Minor
2. Contradictory documentation on whether the filter is a security boundary — gateways.proto:143-148 vs grpc_handler.go:277-283, Docs / Consistency
The proto comment calls WatchGatewaysRequest.cluster_id "the security boundary that keeps a spoke's stream scoped to its own cluster," while the handler comment states it is "cooperative scoping, not an enforced trust boundary … any control-plane could pass any cluster_id." These directly contradict each other. Align the wording (the handler's framing is the accurate one) so a future reader does not treat the filter as an authenticated boundary.
Cross-PR coordination
An open specification change defines periodic "world synchronization" requiring each cycle to obtain the complete inventory from the API server for every enabled reconciler and to perform orphan cleanup only from a complete inventory. This PR makes the control plane's gateway listing cluster-scoped for seed/watch/health/sandbox while deliberately leaving namespace GC unfiltered. These two efforts make opposite assumptions about what "the world" is for a managed-cluster spoke, and both govern gateway inventory listing and orphan cleanup. The maintainers should decide, and coordinate between that spec PR (#185) and this one, whether world-sync's "complete inventory" means the global fleet or the caller's cluster_id slice, so the spec's cleanup-from-complete-inventory rule stays consistent with this PR's per-reconciler filtering (especially the intentionally-unfiltered namespace GC).
Findings Summary (ordered by severity, highest first)
- [Major]
cluster_idinterpolated into search DSL;ValidateStringFielddoes not reject quotes as the comment claims — Security / Input Validation (grpc_handler.go L248-L251) - [Minor] Proto vs handler comments contradict on whether the filter is a security/trust boundary — Docs / Consistency (gateways.proto L148, grpc_handler.go L277-L283)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound / not-found handled |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (K8s DNS labels, safe interpolation) | Fail |
| Reconcile pattern (not create-or-skip) | Pass |
| OpenAPI/proto stubs regenerated, not hand-edited | Pass |
| Test diff scrutiny (modified assertions justified) | Pass |
| Conventional commit message | Pass |
| if err := grpcutil.ValidateStringField("cluster_id", clusterID, false); err != nil { | ||
| return nil, err | ||
| } | ||
| listArgs.Search = fmt.Sprintf("cluster_id = '%s'", clusterID) |
There was a problem hiding this comment.
ValidateStringField("cluster_id", ...) here only checks required + max length (255) — it does not reject single quotes or search metacharacters, so the surrounding comment's claim that it guards against "a value containing a quote" is inaccurate. fmt.Sprintf("cluster_id = '%s'", clusterID) then splices the raw value into the TSL search string; an input like x' or name = 'y parses as an extra predicate and broadens the filter. The TSL→squirrel path parameterizes literals so this is not raw SQL injection, and this scoping is explicitly cooperative (not an enforced trust boundary), which keeps it below Blocker — but please validate cluster_id against a strict charset (K8s DNS-label / KSUID, per security.spec.md) or build the predicate without string interpolation, and fix the comment so it no longer asserts quote-safety it does not provide. Note the same value reaches here unvalidated from HYPERSHELL_CLUSTER_ID.
| // cluster_id, when set, restricts the stream to gateways assigned to that | ||
| // managed cluster (see ListGatewaysRequest.cluster_id). Because the event | ||
| // broker fans every gateway out to every subscriber, this filter is the | ||
| // security boundary that keeps a spoke's stream scoped to its own cluster. |
There was a problem hiding this comment.
This calls the filter "the security boundary that keeps a spoke's stream scoped to its own cluster," but the WatchGateways handler comment (grpc_handler.go L277-L283) states it is "cooperative scoping, not an enforced trust boundary … any control-plane could pass any cluster_id." These contradict. Please align the wording with the handler's (accurate) framing so future readers don't rely on this as an authenticated boundary.
2 participants
Summary
Implements the MVP of the managed-cluster pull model (HYPERSHELL-297). A managed cluster (spoke) deploys only the control-plane, dials the hub api-server over gRPC, and provisions only the gateways whose
cluster_idmatches its ownHYPERSHELL_CLUSTER_ID. Kubeconfigs stay on each spoke — nothing is centralized in a hub.Changes
api-server
gateways.proto: addoptional string cluster_idtoListGatewaysRequestandWatchGatewaysRequest(non-breaking, regeneratedgateways.pb.go).plugins/gateways/grpc_handler.go:ListGatewaysapplies a server-sidecluster_id = '...'search filter;WatchGatewaysskips fan-out events whose gatewaycluster_iddoesn't match the subscriber's filter (a security boundary — the broker fans every gateway to every subscriber). Unattributable deletes are skipped when a filter is set.control-plane
config: newConfig.ClusterIDfromHYPERSHELL_CLUSTER_ID(empty = handle all gateways, single-cluster default).listAllGateways(ctx, client, clusterID)helper.main.go: logs the cluster mode at startup and wirescfg.ClusterIDinto the reconcilers.Per-reconciler filtering semantics
Testing
TestSeedGateways_ThreadsClusterID,TestListAllGateways_ClusterIDFilter, plus call-site updates.go build,go vet, and all control-plane package tests pass for both components.cluster_id=hub-local, spoke (nshypershell-mc1) scopedcluster_id=mc1, both against one standalone api-server. Created two gateways (hub-local,mc1); each control plane seeded and provisioned only its own gateway's namespace and ignored the other's.Notes / follow-ups (out of scope)
ManagedDatabasehas nocluster_id, so both controllers still reconcile all databases.GetManagedClusterByName, ManagedCluster heartbeat/last_seen,cluster_idonManagedDatabase, spec updates to the pull model.🤖 Generated with Claude Code