docs(control-plane): specify periodic world synchronization - #185
docs(control-plane): specify periodic world synchronization#185jhjaggars 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: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Amber reviewStatus: Complete VerdictThis is a well-constructed, security-conscious spec-only PR that adds a coherent Control Plane World Synchronization specification (periodic inventory, revision-aware queues, fail-closed orphan cleanup) and pairs it with a RoleBinding→Keycloak projection recovery model. Overall assessment: COMMENT — no blockers, but one Major spec-consistency issue (the Amber Analysis The change is documentation/spec only (6 files, +374/-18): a new Findings[Major] Role-bridge change diverges from [Minor] Union mapping is a changed guarantee with a migration dependency (spec consistency) [Minor] Two versioning primitives introduced across in-flight specs (design consistency) Cross-PR coordinationI reviewed all currently open PRs against #216, #214, #212, #211, #210, #209, #208, #207, #206, #201, #200, #194, #189, #188, #185 (this), #182, #179, #151, #150, #148, #135, #109, #75, #73. Material conflicts requiring a maintainer decision:
Adjacent, not a material conflict:
No other open PR shows a logical, structural, or ordering conflict with #185. UI PRs (#214/#209/#208/#210), dependency bumps (#188/#189/#135/#75/#73), and deploy/console/e2e/auth PRs (#216/#211/#212/#182/#201/#194/#150/#148/#109/#206) are in unrelated areas. Findings Summary (ordered by severity, highest first)
Convention Checklist
|
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This is a well-constructed, security-conscious spec-only PR that adds a coherent Control Plane World Synchronization specification (periodic inventory, revision-aware queues, fail-closed orphan cleanup) and pairs it with a RoleBinding→Keycloak projection recovery model. Overall assessment: COMMENT — no blockers, but one Major spec-consistency issue (the gateway:owner role-bridge change diverges from rbac-enforcement.spec.md) and material cross-PR coordination with #200 and #151 should be resolved before/at merge.
Amber Analysis
The change is documentation/spec only (6 files, +374/-18): a new control-plane-world-sync.spec.md, index registration, and aligned edits to control-plane.spec.md, data-model.spec.md, openshell-gateway-database.spec.md, and openshell-gateway-keycloak.spec.md. The spec quality is high and it explicitly bakes in HyperShell security conventions (no secrets in responses, ownership-labeled cleanup, complete-inventory-before-delete, destructive GC disabled by default, capped backoff). My main concerns are (1) the Keycloak role-bridge semantics now contradict the authoritative RBAC spec that is not updated in this PR, and (2) this spec overlaps heavily with two other in-flight control-plane specs.
Findings
[Major] Role-bridge change diverges from rbac-enforcement.spec.md (spec consistency)
This PR changes gateway:owner to map to both openshell-admin and openshell-user and replaces "highest-privilege wins" with a union rule (keycloak spec L142, L149). However specs/security/rbac-enforcement.spec.md — which security.spec.md cites as the canonical OIDC role bridge — is not touched and still states the single-role mapping (gateway:owner → openshell-admin) at L108, L118-119, and L517. Two specs now disagree on the bridge contract. Additionally, within the modified file, L17 still says "a gateway:owner binding on gw-1 results in an openshell-admin Keycloak client role assignment" (single role). Update rbac-enforcement.spec.md and keycloak L17 to the union model, or revert the mapping change here.
[Minor] Union mapping is a changed guarantee with a migration dependency (spec consistency)
Moving gateway:owner from {openshell-admin} to {openshell-admin, openshell-user} flips the pre-existing "Gateway owner receives admin role" scenario from asserting a single role to asserting two. The stated rationale ("the gateway enforces those roles independently") is a contract assumption that should be verified against the gateway's actual authorization behavior. Existing owner users provisioned under the old mapping will only regain full access once the new periodic projection runs and backfills openshell-user; the PR should note this backfill dependency explicitly so operators know convergence relies on the world-sync projection (or on the ADDED path re-running).
[Minor] Two versioning primitives introduced across in-flight specs (design consistency)
This PR mandates a new resource_revision transport/concurrency primitive on list/watch snapshots and migrating reconcileQueue[T] version accessors off updated_at (world-sync spec L76, L154; data-model L8). PR #151 concurrently introduces generation/observed_generation domain fields for the Gateway convergence gate. These are complementary but need an explicit, agreed relationship so implementers don't build overlapping revision mechanisms. See Cross-PR section.
Cross-PR coordination
I reviewed all currently open PRs against main (as of 2026-08-27):
#216, #214, #212, #211, #210, #209, #208, #207, #206, #201, #200, #194, #189, #188, #185 (this), #182, #179, #151, #150, #148, #135, #109, #75, #73.
Material conflicts requiring a maintainer decision:
-
#200 "docs: define control plane reconciliation contract" — overlapping/competing canonical spec. #200 adds
specs/standards/control-plane/reconciliation-contract.spec.mdrequiring "initial listing, periodic resync, idempotent operations, per-resource serialization, bounded retries, durable finalization, fail-closed destructive actions" — the same behavior domain this PR defines as "world synchronization" (periodic inventory, per-ID serialization, revision-aware queues, bounded backoff, complete-inventory-before-cleanup). Both PRs also editcontrol-plane.spec.md(Reconciler / Status Synchronization / Design Decisions — #200 changes the Design Decision row to "gRPC watch streams with periodic resync"; #185 adds "Periodic inventory in addition to watch streams"), and both depend on/editwatch-delete-events.spec.mdandopenshell-gateway-namespace-gc.spec.md. Decision needed: is world-sync a subordinate detail spec under #200's reconciliation contract, or a standalone peer? Which is canonical, and in what order should they merge? Theircontrol-plane.spec.mdedits will not cleanly coexist without a chosen hierarchy. -
#151 "gate gateway re-provisioning on desired-state convergence" — competing convergence primitive + same requirement rewrite. #151 adds
generation/observed_generationto the Gateway data model and rewrites the samecontrol-plane.spec.md"Status Synchronization" requirement this PR touches, keying the provisioning gate onobserved_generation == generation. This PR instead addsresource_revision/inventory-watermark metadata and keys queue coalescing on it, and separately constrains how periodic sync may repair drift without forcing phase or fighting the health reconciler. Decision needed: confirmresource_revision(queue ordering/coalescing) andgeneration/observed_generation(drift gate) are intended to coexist rather than be redundant, and reconcile the two competing edits to the Status Synchronization requirement and the Gateway drift-repair semantics. -
#179 "reconcile existing Keycloak clients on gated gateways" — same problem space, same file. #179 is a hotfix that reconciles existing Keycloak clients past the phase gate (Device Authorization Grant attribute) and edits
openshell-gateway-keycloak.spec.md, which this PR also substantially rewrites. Both address "the phase gate prevents Keycloak reconciliation on existing gateways"; #185 proposes periodic world-sync projection as the general mechanism, #179 is a targeted repair. Decision needed: ensure both converge on the same client-ID derivation (<name>-<id>) and the "control plane is sole writer of bridge-owned roles" model, and sequence the keycloak-spec edits so they don't clobber each other. #179's body already flags a #151 interaction.
Adjacent, not a material conflict:
- #207 "reconcile-to-request trace correlation" extends the gRPC
ObjectReference/snapshot withtraceparent/tracestate; this PR requires addingresource_revision(+ watermark) to the same list/watch snapshot contract. Both are additive transport metadata and can coexist; only note is that concrete proto field numbering (out of scope for this spec-only PR) will need coordination at implementation time.
No other open PR shows a logical, structural, or ordering conflict with #185. UI PRs (#214/#209/#208/#210), dependency bumps (#188/#189/#135/#75/#73), and deploy/console/e2e/auth PRs (#216/#211/#212/#182/#201/#194/#150/#148/#109/#206) are in unrelated areas.
Findings Summary (ordered by severity, highest first)
- [Major] Role-bridge change (
gateway:owner→ both roles / union rule) contradicts unmodifiedrbac-enforcement.spec.md(L108, L118-119, L517) and keycloak L17 — Spec Consistency (keycloak L142, L149) - [Minor] Changed owner-role guarantee depends on periodic projection backfill; verify the "gateway enforces both roles independently" assumption — Spec Consistency (keycloak L149)
- [Minor]
resource_revisionvs #151'sgeneration/observed_generation— clarify coexistence — Design Consistency (data-model L8; world-sync L76, L154)
Convention Checklist
| Convention | Result |
|---|---|
| No secrets in logs or responses (spec asserts secret-free inventory/RPCs) | Pass |
| Reconcile, not create-or-skip (queues replace create-or-skip/active-map) | Pass |
Fail-closed destructive actions (orphan GC default false, complete-inventory + grace + recheck) |
Pass |
| Input/ownership validation before deletion (exact ownership labels, no prefix inference) | Pass |
| Configuration separate from code (env-driven interval/kill switch/grace period) | Pass |
| Cross-spec consistency (role bridge) | Fail |
| Spec index registration for new specs | Pass |
| | HyperShell Role | Keycloak Client Role | Scope | | ||
| |---|---|---| | ||
| | `gateway:owner` | `openshell-admin` | The specific bound gateway | | ||
| | `gateway:owner` | `openshell-admin` **and** `openshell-user` | The specific bound gateway | |
There was a problem hiding this comment.
[Major] Role bridge diverges from the canonical RBAC spec. This table now maps gateway:owner to both openshell-admin and openshell-user, but specs/security/rbac-enforcement.spec.md (cited by security.spec.md as the authoritative OIDC role bridge) is not updated in this PR and still documents the single-role mapping at L108, L118-119, and L517. Two specs now disagree. Please update rbac-enforcement.spec.md (and L17 of this file, which still says an owner binding yields only openshell-admin) to the union model, or revert this change here.
| ### Effective Role Resolution | ||
|
|
||
| When a user has multiple RoleBindings on the same gateway, the **highest-privilege** Keycloak client role wins. A user with both `gateway:viewer` (→ `openshell-user`) and `gateway:owner` (→ `openshell-admin`) on the same gateway SHALL have the `openshell-admin` client role. | ||
| For each `(user, gateway)` pair, the control plane SHALL compute the effective Keycloak role set as the union of mappings for every active RoleBinding. `gateway:owner` is the highest-privilege HyperShell role and maps to both `openshell-admin` and `openshell-user`, because the gateway enforces those roles independently; a concurrent `gateway:viewer` binding adds no further role. Periodic RoleBinding projection SHALL use this same effective-role-set calculation before it diffs the two bridge-owned Keycloak role member lists. |
There was a problem hiding this comment.
[Minor] Changed guarantee + assumption to verify. Moving gateway:owner from {openshell-admin} to {openshell-admin, openshell-user} flips the pre-existing "owner receives admin role" scenario from one role to two. The rationale "the gateway enforces those roles independently" is a contract assumption — please verify it against the gateway's actual authorization behavior. Also note explicitly that existing owners provisioned under the old mapping only regain openshell-user once the new periodic projection (or a re-run of the ADDED path) backfills it; correctness depends on that projection running.
| ## Overview | ||
|
|
||
| The HyperShell API server provides a control plane for deploying and managing distributed API gateways across multiple Kubernetes clusters and cloud providers. | ||
| The HyperShell API server provides a control plane for deploying and managing distributed API gateways across multiple Kubernetes clusters and cloud providers. Control-plane list and watch snapshots additionally carry API-assigned resource revisions and inventory watermarks as defined in the [Control Plane World Synchronization specification](./control-plane-world-sync.spec.md); these are transport/concurrency metadata rather than persisted domain fields. |
There was a problem hiding this comment.
[Minor] Versioning-primitive coordination. This PR adds resource_revision/inventory-watermark transport metadata and (in the world-sync spec) migrates reconcileQueue[T] off updated_at. PR #151 concurrently adds generation/observed_generation domain fields for the Gateway convergence gate. These are complementary but should be explicitly reconciled so implementers don't build two overlapping revision mechanisms — see the Cross-PR coordination section of the top-level review.
| ## Overview | ||
|
|
||
| The HyperShell control plane is a Go service that watches the API server via gRPC streaming RPCs and reconciles the desired state (Fleet resources in the database) into actual Kubernetes resources across managed clusters. It follows the informer-reconciler pattern without depending on controller-runtime. | ||
| The HyperShell control plane is a Go service that watches the API server via gRPC streaming RPCs and reconciles the desired state (Fleet resources in the database) into actual Kubernetes resources across managed clusters. It follows the informer-reconciler pattern without depending on controller-runtime. Periodic inventory and reconciliation behavior is defined in the [Control Plane World Synchronization specification](./control-plane-world-sync.spec.md). |
There was a problem hiding this comment.
Cross-PR overlap. This Overview now points to the new world-sync spec, but PR #200 (reconciliation-contract.spec.md) and PR #151 both edit this same file's Reconciler / Status Synchronization / Design Decisions sections and cover the same "periodic resync" behavior domain. Maintainers should decide the canonical hierarchy and merge order before these edits collide (details in the top-level Cross-PR coordination section).
2 participants
Summary
Safety and compatibility
Validation
make checkgit diff --check