[HYPERSHELL-122] feat(gcp) Provision a GCP managed cluster for multi-cloud validation - #229
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:
Comment |
Amber reviewStatus: Complete VerdictCOMMENT — This is a clean, additive multi-cloud validation change (a GCP OSD kustomize overlay, a deploy runbook, and an e2e script) with no production Go changes, so the core reconciler/error-handling conventions do not apply. The overlay itself is fine, but it is not self-contained: the accompanying runbook requires a long series of imperative I reviewed against CLAUDE.md, the security spec, and the control-plane conventions spec. The changes are deployment/test/documentation only; no Findings[Major] The committed overlay does not produce a working deployment on its own. [Minor] [Minor] The new e2e script is not covered by CI shell-syntax linting. [Minor] Verbatim duplication of the canonical controller ClusterRole. Cross-PR coordinationTwo open pull requests create material coordination requirements with this one:
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
COMMENT — This is a clean, additive multi-cloud validation change (a GCP OSD kustomize overlay, a deploy runbook, and an e2e script) with no production Go changes, so the core reconciler/error-handling conventions do not apply. The overlay itself is fine, but it is not self-contained: the accompanying runbook requires a long series of imperative oc set image :latest / oc patch steps to reach a working state, and there are two cross-PR coordination items that need a maintainer decision before or as this merges.
I reviewed against CLAUDE.md, the security spec, and the control-plane conventions spec. The changes are deployment/test/documentation only; no panic(), error-wrapping, IsNotFound, or pod-spec SecurityContext concerns are in scope. The controller-clusterrbac.yaml is byte-identical to the already-merged IBM overlay, so its (broad, privileged-SCC-granting) rules are an intentional, pre-existing pattern rather than a new grant introduced here.
Findings
[Major] The committed overlay does not produce a working deployment on its own.
components/api-server/deploy/gcp/kustomization.yaml pins the base images and sets only GATEWAY_INGRESS_MODE/GATEWAY_API_BASE_DOMAIN/HYPERSHELL_DATABASE_IMAGE, but skills/deploy/gcp-cluster/SKILL.md Step 6.1 states the "pinned digest images lack OIDC and CNPG support" and instructs the operator to immediately override them with oc set image ...:latest, plus imperative oc patch/oc set env for the OIDC command, JWK URL, RBAC flags, Keycloak admin secret, etc. That means oc kustomize deploy/gcp | oc apply alone yields a non-functional stack, and the real configuration lives as imperative runbook steps rather than in version-controlled config. This works against "Separate configuration from code" and reproducibility. Consider baking the OIDC env/command and a working image reference into the overlay (or a dedicated gcp patch file) so the deploy is declarative. Confidence: High.
[Minor] HYPERSHELL_DATABASE_IMAGE=docker.io/library/postgres:18 is likely dead config on this path. The PR body, skill, and e2e all describe GCP using CNPG-managed per-gateway databases (provider=cnpg), where the standalone-postgres image is not used. Either document that this is only a fallback for provider=local, or drop it to avoid implying the GCP path uses a standalone postgres. Confidence: Medium.
[Minor] The new e2e script is not covered by CI shell-syntax linting. .github/workflows/lint.yml lint-pr-test only runs bash -n components/pr-test/e2e-openshell.sh; the new e2e-openshell-gcp.sh (like e2e-openshell-roks.sh) is not checked. Per "Register every component in CI", consider globbing components/pr-test/*.sh so new scripts get a bash -n gate. Confidence: High.
[Minor] Verbatim duplication of the canonical controller ClusterRole. deploy/gcp/controller-clusterrbac.yaml is an exact copy of deploy/ibm/controller-clusterrbac.yaml (which itself mirrors deploy/base/controller-rbac.yaml). The grant is now maintained in three places and will drift; the PR body already acknowledges this is temporary until the deploy/openshift base grows the ClusterRole. See also the Cross-PR section. Confidence: High.
Cross-PR coordination
Two open pull requests create material coordination requirements with this one:
-
#223 (remove Fleet entity and
fleet_idacross the stack). This PR's newe2e-openshell-gcp.shandgcp-cluster/SKILL.mdare built entirely on the Fleet data model: theyPOST /fleets,GET /fleets, and passfleet_idwhen creating managed clusters, releases, databases, and gateways. #223 deletes the/fleetsendpoint and removesfleet_id(including from the gateway create request). These are incompatible data-model assumptions. Maintainers need to decide merge order and ownership: if #223 lands first, this PR's script and runbook must be updated to drop all fleet usage; if this lands first, #223's author must also update these new GCP artifacts. -
#227 (GatewayProfile namespace quota enforcement). #227 extends the canonical controller RBAC in
deploy/ibm/controller-clusterrbac.yamlwithresourcequotas/limitranges. This PR adds a byte-identical copy of that same canonical grant asdeploy/gcp/controller-clusterrbac.yamlwithout those verbs. Because the grant is duplicated per-overlay, whichever PR merges second must add the quota rules to the other overlay; otherwise quota enforcement will work on IBM but silently fail on GCP. Maintainers should coordinate keeping the GCP overlay in sync (ideally by promoting a single shared ClusterRole).
Findings Summary (ordered by severity, highest first)
- [Major] Committed overlay is not self-contained; a working deploy requires imperative
:latest/OIDC/CNPG overrides from the runbook — Config/Code Separation (kustomization.yaml L19-41; SKILL.md Step 6.1) - [Minor]
HYPERSHELL_DATABASE_IMAGEunused under the documented CNPG path — Spec Consistency (kustomization.yaml L40-41) - [Minor] New e2e script not wired into
lint-pr-testbash -n— CI Registration (e2e-openshell-gcp.sh L1) - [Minor] Canonical controller ClusterRole duplicated verbatim into a third overlay; drift risk — Maintainability (controller-clusterrbac.yaml L1-12)
Convention Checklist
| Convention | Result |
|---|---|
| Image references consistent across manifests | Fail (overlay images require manual :latest override to function) |
| Configuration separate from code | Fail (working config lives in imperative runbook steps) |
| Register every component in CI | Fail (new e2e script not lint-gated) |
| Conventional commit message | Pass |
| No secrets committed (test creds only) | Pass |
| Controller ClusterRole matches canonical grant | Pass (identical to IBM overlay) |
| - name: controller | ||
| env: | ||
| - name: GATEWAY_INGRESS_MODE | ||
| value: route |
There was a problem hiding this comment.
[Major] This overlay is not self-contained. It pins the base images and only sets these three env vars, but skills/deploy/gcp-cluster/SKILL.md Step 6.1 says the pinned digests "lack OIDC and CNPG support" and requires the operator to override them with oc set image ...:latest plus imperative oc patch/oc set env for the OIDC command, JWK URL, RBAC flags, and Keycloak admin secret. As a result oc kustomize deploy/gcp | oc apply alone yields a non-functional stack, and the real config lives in runbook steps rather than version control. Consider baking the OIDC env/command and a working image reference into the overlay so the deploy is declarative and reproducible.
| - name: GATEWAY_API_BASE_DOMAIN | ||
| value: apps.hypershell-gcp.u0zc.p2.openshiftapps.com | ||
| - name: HYPERSHELL_DATABASE_IMAGE | ||
| value: docker.io/library/postgres:18 |
There was a problem hiding this comment.
[Minor] The PR, skill, and e2e all describe the GCP path using CNPG-managed databases (provider=cnpg), where this standalone-postgres image isn't consulted. Please document that this only applies to a provider=local fallback, or drop it to avoid implying GCP uses standalone postgres.
| # whole tenant gateways: it creates per-tenant namespaces, Deployments, | ||
| # Services, Secrets, NetworkPolicies, and -- in Route ingress mode -- OpenShift | ||
| # Routes, all cluster-wide. This mirrors deploy/base/controller-rbac.yaml (the | ||
| # GitOps tree's canonical grant), bound to the hypershell/hypershell-controller |
There was a problem hiding this comment.
[Minor] This file is a byte-identical copy of deploy/ibm/controller-clusterrbac.yaml, which itself mirrors deploy/base/controller-rbac.yaml — the same canonical grant is now maintained in three places and will drift. Note that PR #227 already extends the IBM copy with resourcequotas/limitranges; this copy would silently lack them. Prefer promoting a single shared ClusterRole rather than a third copy.
| @@ -0,0 +1,957 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
[Minor] .github/workflows/lint.yml lint-pr-test only runs bash -n components/pr-test/e2e-openshell.sh, so this new script (like e2e-openshell-roks.sh) gets no CI syntax gate. Per "Register every component in CI", consider globbing components/pr-test/*.sh so new scripts are checked.
…cloud validation Add a GCP (OpenShift Dedicated) deployment path so HyperShell deployment and reconciliation can be validated on a second cloud target. - deploy/gcp kustomize overlay extending deploy/openshift: Route ingress mode on the OSD-managed wildcard, direct pulls from public registries (no image mirroring), CNPG-managed databases. - controller-clusterrbac.yaml: cluster-scoped controller RBAC for full tenant reconciliation (namespaces, workloads, Routes, cert-manager, Gateway API, SCC, sandboxes). - gcp-cluster deploy skill documenting end-to-end provisioning, OIDC wiring, teardown, and troubleshooting. - e2e-openshell-gcp.sh covering JWT auth, gateway provisioning, CNPG DB, sandbox lifecycle, and developer RBAC on the GCP target. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Amber reviewStatus: Complete VerdictREQUEST_CHANGES. This PR adds a thorough, well-structured GCP OSD deployment runbook, but its own body advertises four deliverables (the SummaryThe change is documentation-only: it adds a single 650-line skill, Findings[Major] Runbook depends on artifacts that are not in this PR. The body's "What's included" lists a [Minor/Security] Fixed credential values embedded in copy-paste commands. [Minor/Security] Broad privilege grants presented without blast-radius caveats. A Cross-PR coordinationTwo in-flight changes require a maintainer decision or a defined merge order with this runbook:
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
REQUEST_CHANGES. This PR adds a thorough, well-structured GCP OSD deployment runbook, but its own body advertises four deliverables (the deploy/gcp overlay, controller-clusterrbac.yaml, and e2e-openshell-gcp.sh) that are not actually in the diff, so the runbook is not executable as written. Fix the missing-artifact gap (and the security/coordination notes below) before merge.
Summary
The change is documentation-only: it adds a single 650-line skill, skills/deploy/gcp-cluster/SKILL.md. No Go, manifests, or scripts are included, so the Go/reconciler/container conventions are not directly exercised. The core problem is completeness and accuracy: the runbook depends on files the PR claims to include but does not.
Findings
[Major] Runbook depends on artifacts that are not in this PR. The body's "What's included" lists a components/api-server/deploy/gcp/ overlay, controller-clusterrbac.yaml, and components/pr-test/e2e-openshell-gcp.sh. The diff adds only SKILL.md. Verified in the tree: deploy/gcp does not exist (only ibm, kind, openshift); the only controller-clusterrbac.yaml is under deploy/ibm/; and pr-test contains only e2e-openshell.sh and e2e-openshell-roks.sh. Consequently the key commands fail for any reader — oc kustomize deploy/gcp | oc apply -f - (Step 6.1) and bash components/pr-test/e2e-openshell-gcp.sh (Step 10). Either add the overlay/RBAC/e2e artifacts to this PR, or restructure the runbook so it does not depend on files that have not landed and adjust the PR description to match the actual contents. (Inline: L207, L214, L502.)
[Minor/Security] Fixed credential values embedded in copy-paste commands. client-secret=control-plane-secret (L255-259) and username=admin/password=admin (L173-176, L408-411) are presented as ready-to-run. For a runbook that positions GCP OSD as a validated Cloud Hub, mark these explicitly as validation-only throwaway values and direct readers to generate and rotate real secrets. (Inline: L259.)
[Minor/Security] Broad privilege grants presented without blast-radius caveats. A cluster-admin ServiceAccount with a long-lived (non-expiring) token stored in a Secret (Step 7) and privileged SCC for the CNPG manager (Step 6.2). Call out the risk and scope down where feasible. (Inline: L358.)
Cross-PR coordination
Two in-flight changes require a maintainer decision or a defined merge order with this runbook:
-
Fleet /
fleet_idremoval (#223). This runbook's provisioning flow (Step 8.1) creates a Fleet viaPOST /fleetsand passesfleet_idinto managed_clusters, gateway_releases, managed_databases, and the Gateway. #223 removes the Fleet entity andfleet_idend to end (spec, OpenAPI/SDK, gRPC, CLI, deploy/e2e), making those resources top-level with RBAC-based tenancy and deleting the/fleetsroutes. These are incompatible data-model assumptions: if #223 merges first, every resource-creation call in this runbook (and the referenced e2e script) becomes invalid. Maintainers should decide merge order and who rewrites the GCP provisioning steps (and the e2e suite) to the post-Fleet model. -
development_oidcfor the OpenShift overlay (#217). This runbook works around the JWT/developmentclobber by manually patchingAPI_ENV=development_oidconto the api-server viaoc patch(Step 6.1) and documents the exact failure modes in its troubleshooting matrix, while proposing adeploy/gcpoverlay that extendsdeploy/openshift. #217 fixes the same issue at the basedeploy/openshiftoverlay by settingAPI_ENV=development_oidc(and re-declaring the provisioner env). If #217 lands, the GCP overlay would inherit the fix and the manualoc patch/ documented workaround here becomes redundant or conflicting. Maintainers/owners should align on building the GCP overlay on top of #217 rather than re-patching, to avoid two competing sources of truth for the same auth configuration.
Findings Summary (ordered by severity, highest first)
- [Major] Runbook depends on overlay, controller RBAC, and e2e script not present in the PR; key commands fail as written - Spec/Docs Completeness (L207, L214, L502)
- [Minor] Fixed credential values embedded in runnable commands without validation-only caveat - Security (L259)
- [Minor] cluster-admin SA long-lived token and privileged SCC grant without blast-radius caveat - Security (L358)
Convention Checklist
| Convention | Result |
|---|---|
| PR contents match PR description | Fail |
| Referenced files/commands resolve in-repo | Fail |
| No credentials normalized as defaults in docs | Fail |
| Config kept separate from code (overlay-based) | Pass |
| Conventional commit message | Pass |
|
|
||
| ```bash | ||
| cd components/api-server | ||
| oc kustomize deploy/gcp | oc apply -f - |
There was a problem hiding this comment.
[Major] This step runs oc kustomize deploy/gcp, but components/api-server/deploy/gcp/ does not exist in this PR (only deploy/ibm, deploy/kind, deploy/openshift are present). The PR body lists this overlay under "What's included," but the diff adds only this SKILL.md. As written, the runbook fails at this command for anyone who follows it. Either add the deploy/gcp overlay to this PR or change the instructions to not depend on an overlay that has not landed. Confidence: High.
|
|
||
| - `GATEWAY_INGRESS_MODE=route` - use OpenShift Routes, not Gateway API | ||
| - `GATEWAY_API_BASE_DOMAIN=apps.<cluster>.<id>.openshiftapps.com` | ||
| - `controller-clusterrbac.yaml` - cluster-wide RBAC for tenant reconciliation |
There was a problem hiding this comment.
[Major] controller-clusterrbac.yaml is referenced as part of the deploy/gcp overlay, but the only controller-clusterrbac.yaml in the tree is under deploy/ibm/. This file is not delivered by this PR, so the cluster-scoped controller RBAC the runbook depends on is missing. Confidence: High.
| ## Step 10: Run the e2e test suite | ||
|
|
||
| ```bash | ||
| GATEWAY_NAME=angel SKIP_CLEANUP=1 bash components/pr-test/e2e-openshell-gcp.sh |
There was a problem hiding this comment.
[Major] The runbook invokes components/pr-test/e2e-openshell-gcp.sh, but that script is not in the repository (only e2e-openshell.sh and e2e-openshell-roks.sh exist). The reference test run below is presented as reproducible, yet the command cannot be executed from this PR. Add the script or drop/soften the reproducibility claim. Confidence: High.
| --from-literal=client-id=hypershell-control-plane \ | ||
| --from-literal=client-secret=control-plane-secret \ | ||
| --from-literal=realm=hypershell \ | ||
| --from-literal=server-url=http://keycloak-service.keycloak.svc.cluster.local:8080 |
There was a problem hiding this comment.
[Minor/Security] The runbook bakes fixed credential values into copy-paste commands (client-secret=control-plane-secret here; username=admin/password=admin in Steps 5.1 and 8). For a doc that positions GCP OSD as a validated Cloud Hub target, please label these explicitly as throwaway validation-only values and point readers to generate/rotate real secrets, so the runbook does not normalize shipping default credentials. Confidence: Medium.
|
|
||
| ```bash | ||
| oc -n hypershell create sa cluster-agent | ||
| oc adm policy add-cluster-role-to-user cluster-admin -z cluster-agent -n hypershell |
There was a problem hiding this comment.
[Minor/Security] Step 7 provisions a ServiceAccount with cluster-admin plus a long-lived, non-expiring token stored in a Secret, and Step 6.2 grants the CNPG manager the privileged SCC. These are broad grants. Consider calling out the blast radius and, where possible, scoping down (the controller RBAC is described as cluster-scoped for tenant reconciliation, but cluster-admin for the agent identity is stronger than that). Confidence: Medium.
|
|
||
| ```bash | ||
| # Fleet | ||
| FLEET=$(curl -sk -X POST "$API/fleets" -H 'Content-Type: application/json' \ |
There was a problem hiding this comment.
[Coordination] Step 8.1 creates a Fleet via POST /fleets and threads fleet_id into managed_clusters, gateway_releases, managed_databases, and the Gateway. A separate in-flight change removes the Fleet entity and fleet_id end to end (making these top-level resources with RBAC-based tenancy). If that change lands first, every resource-creation call in this runbook becomes invalid. See the Cross-PR coordination section in the top-level review. Confidence: High.
2 participants
Summary
Adds a GCP (OpenShift Dedicated) deployment path so HyperShell deployment and reconciliation can be validated on a second cloud target, satisfying HYPERSHELL-122. The GCP OSD environment provides an additional target for validating the managed-cluster architecture and end-to-end workflows beyond the existing ROKS (IBM) path.
Why GCP OSD
Unlike ROKS, GCP OSD workers have full egress to public registries, so no image mirroring is required and OperatorHub/Gateway API work out of the box. This overlay uses Route ingress mode on the OSD-managed
*.apps.<cluster>.<id>.openshiftapps.comwildcard to avoid custom DNS/TLS/wildcard-cert setup, with passthrough Routes preserving the gateway's per-tenant TLS end to end.What's included
components/api-server/deploy/gcp/kustomize overlay extendingdeploy/openshift:GATEWAY_INGRESS_MODE=routeon the OSD wildcard domaincontroller-clusterrbac.yaml— cluster-scoped controller RBAC for full tenant reconciliation (namespaces, workloads, Routes +routes/custom-host, cert-manager, Gateway API, privileged SCCuse, agent sandboxes). Mirrors the GitOps tree's canonical grant, carried in this overlay until thedeploy/openshiftbase grows the same ClusterRole.skills/deploy/gcp-cluster/SKILL.md— end-to-end deploy runbook: cluster verification, cert-manager + agent-sandbox + Keycloak install, OIDC wiring, CNPG operator setup, managed-cluster identity, tenant gateway provisioning, teardown, and a troubleshooting matrix. Environment-specific config is documented and kept separate from application code.components/pr-test/e2e-openshell-gcp.sh— end-to-end suite proving the full path (JWT-authenticated API → control plane → CNPG DB → gateway provisioning → openshell CLI → sandbox lifecycle → developer RBAC).Validation
Validated end to end on
hypershell-gcp(OSD 4.22.9, openshell 0.0.109) on 2026-08-21: Keycloak realm, JWT-authenticated API, CNPG database cluster, tenant gateway with per-gateway OIDC console, TLS verified via passthrough Route. The e2e suite reports 23 passed, 0 failed (see SKILL.md reference run).Acceptance criteria mapping
deploy/openshiftbase) noted inline for future Jira issues🤖 Generated with Claude Code