[HYPERSHELL-44] feat: Support OpenShift development (openshift-up/down/swap) - #232
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 |
squizzi
changed the title
squizzi
changed the title
Amber reviewStatus: Stopped The pull request head changed before Amber posted the review. A later job can review the new head. |
5e564e0 to
afbb851
Compare
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This PR adds a well-structured OpenShift local-dev lifecycle (driver model, ephemeral namespace isolation, per-env prefixed RBAC, Keycloak companion namespace, and component swaps) with genuinely thorough spec coverage and a 62-case bash test suite. Two issues block merge: a committed git conflict marker in deploy/hub/kustomization.yaml that produces an invalid manifest, and seeding/spec logic that still assumes the now-removed Fleet entity, which will fail against current main.
Blocker
deploy/hub/kustomization.yaml contains unresolved git conflict markers. Lines 14, 15, and 74 hold <<<<<<< Updated upstream, =======, and >>>>>>> Stashed changes — the residue of a git stash pop. This file is no longer valid YAML, so any kustomize build deploy/hub (the GitOps fleet-hub base) fails outright. Remove the markers and keep the intended content (the patches: block that deletes the bundled CNPG cluster and Keycloak resources appears to be the intended side).
Major
Seeding, spec, and reconcile ledger still depend on the removed Fleet entity. seed_via_api issues POST /api/hypershell/v1/fleets and threads fleet_id into every ManagedCluster/GatewayRelease/ManagedDatabase/Gateway payload (scripts/cluster/drivers/openshift.sh ~L791–L900). Current main removed the Fleet entity and dropped fleet_id from all resources (migrations drop the columns; the CLAUDE.md domain model states all resources are top-level). After a rebase onto main, the /fleets GET/POST returns 404 and the fleet_id fields are invalid, so automatic seeding will not complete. specs/platform/openshift-development.spec.md ("seed a Fleet, a ManagedCluster, ...") and the skills/RECONCILE.md summary carry the same stale assumption. Rebase and remove Fleet from the seed flow, payloads, spec scenario, and ledger.
Minor
rewrite_doc rewrites every hypershell-system occurrence, not just namespace fields (scripts/cluster/rewrite-namespaces.py L83: doc.replace(PLATFORM_NS, platform_ns)). For the known overlay this is safe, but any value that merely contains the substring (an image path, arg, or annotation) would also be rewritten. Consider constraining the replacement to namespace:/metadata.name fields as the keycloak-namespace path already does.
Hardcoded dev credentials on command lines. create_bootstrap_secrets/add_keycloak_redirect_uri/seed_via_api use fixed admin/admin, control-plane-secret, provisioner-secret and pass tokens via curl -d password=... / podman login -p <token> (visible in the process table). This matches the existing Kind dev flow and is dev-only, but keep it strictly out of any non-local path.
Cross-PR coordination
Two open pull requests need a maintainer decision before or alongside this merge:
-
#217 solves the same OpenShift + Keycloak JWT 401 problem this PR addresses, but declaratively: it bakes
API_ENV=development_oidcintodeploy/openshift/kustomization.yaml, whereas this PR sets the same variable imperatively at deploy time in the driver (configure_oidc_from_routesrunsoc set env ... API_ENV=development_oidc). Both also editdeploy/openshift/kustomization.yaml. Maintainers should pick a single source of truth forAPI_ENV(overlay vs. driver) and sequence the merges so the two mechanisms do not diverge. -
#212 edits the same
specs/platform/openshift-development.spec.mdseeding scenario and theskills/RECONCILE.mdcoverage ledger, and it removesFleetfrom that scenario to match the post-Fleet data model — the opposite of this PR, which keepsFleet. This is a conflicting data-model assumption on a shared spec requirement, not just file overlap. Maintainers must reconcile the Fleet assumption and decide merge order so the spec and ledger converge on the top-level (no-Fleet) model.
Findings Summary (ordered by severity, highest first)
- [Blocker] Unresolved git conflict markers in
deploy/hub/kustomization.yamlproduce an invalid manifest - Correctness / Build (L14, L15, L74) - [Major] Seeding, spec, and RECONCILE ledger still use the removed
Fleetentity/fleet_id; breaks against currentmain- Data Model / Spec Consistency - [Minor] Blanket
hypershell-systemstring replace inrewrite-namespaces.pycan rewrite non-namespace values - Robustness (L83) - [Minor] Hardcoded dev credentials/tokens passed on command lines - Security (dev-only)
Convention Checklist
| Convention | Result |
|---|---|
| Input validated (RFC 1123 DNS labels, reserved-name refusal) | Pass |
| Restricted SecurityContext preserved (UIDs stripped for restricted SCC) | Pass |
| Image references consistent across manifests/overlay | Pass |
| No secrets written to logs | Pass |
| Manifests are valid / buildable | Fail |
| Consistent with current data model (no Fleet) | Fail |
| Conventional commit messages | Pass |
afbb851 to
c456363
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 well-structured, well-tested addition of an OpenShift ephemeral-namespace development path built on a clean Kind/OpenShift driver abstraction; the shell logic is unusually careful (RFC 1123 validation, reserved-namespace refusal, per-environment ownership labels, env-scoped --prune, and a real lib_test.sh unit suite). No blockers or convention-critical defects were found in the changed shell/manifest/spec code; the notes below are minor hardening and hygiene items plus one cross-PR coordination point.
Strengths
- Driver seam (
scripts/cluster/lib.sh+drivers/*.sh) keeps existingmake kind-*behavior intact while adding OpenShift with a single new driver file. - Input validation is solid:
validate_rfc1123_label,sanitize_dns_label,is_reserved_cluster_namespace, and foreign/owned-namespace refusal all guard destructive operations. - Best-effort paths (labeling, seeding, redirect-URI registration)
warnrather than silently swallow, and fatal RBAC/infra gapsexit. rewrite-namespaces.pyis accompanied by unit assertions covering namespace rewrite, cluster-scoped prefixing, and system: roleRef preservation.
Findings
- [Minor] (Security)
login_internal_registrypasses the OpenShift token via-p "${token}"on the container-engine command line (scripts/cluster/drivers/openshift.sh:1149), which exposes the token in the process list /psoutput. Prefer--password-stdin. - [Minor] (Config vs. code)
API_ENV=development_oidcis applied imperatively withoc set env(scripts/cluster/drivers/openshift.sh:623) instead of being carried declaratively by thedeploy/openshiftoverlay. Declarative config aligns better with the "separate configuration from code" convention and with thedeploy/kindoverlay. See Cross-PR coordination. - [Minor] (Docs) The PR description "Highlights" still says
openshift-up"seeds Fleet/ManagedCluster/GatewayRelease/ManagedDatabase/Gateway". Fleet was removed from the stack onmain, and the code correctly no longer seeds a Fleet (seed_via_api). Please update the description so it doesn't reference a removed entity. - [Minor] (CI) The new
make openshift-testtarget runsscripts/cluster/lib_test.sh, but the suite does not appear to be wired into CI, so these guarantees won't run automatically on future changes. Consider registering it (/maintain-ci).
Cross-PR coordination
Coordination is required with the open pull request that sets API_ENV=development_oidc for the OpenShift api-server declaratively in deploy/openshift/kustomization.yaml (the one titled "fix(deploy): use development_oidc env for OpenShift JWT auth"). This PR solves the same JWT/OIDC problem imperatively via oc set env after apply, and it also edits deploy/openshift/kustomization.yaml. These are competing solutions to one problem: maintainers should decide whether the overlay owns API_ENV declaratively (in which case this PR's oc set env becomes redundant and should be dropped) or whether the imperative script path is intentional, and sequence the merges accordingly to avoid one silently overriding the other.
Findings Summary (ordered by severity, highest first):
- [Minor] Registry login exposes token via
-pon the command line - Security (openshift.sh:1149) - [Minor]
API_ENVset imperatively rather than in the overlay - Config/Code Separation (openshift.sh:623) - [Minor] PR description references removed
Fleetentity - Docs Consistency - [Minor] New
openshift-testshell suite not registered in CI - CI Coverage
Convention Checklist:
| Convention | Result |
|---|---|
| Input validated (RFC 1123 DNS labels, reserved namespaces) | Pass |
| No secrets in logs or error messages | Pass |
| Secret references, not inline secret values in manifests | Pass |
| Never silently swallow partial failures | Pass |
| Separate configuration from code | Minor concern |
| Image references consistent across manifests | Pass |
| Register new components/tests in CI | Minor concern |
| Conventional commit messages | Pass |
| local user token | ||
| user="$(oc_cli whoami)" | ||
| token="$(oc_cli whoami -t)" | ||
| if ${CONTAINER_ENGINE} login -u "${user}" -p "${token}" "${OPENSHIFT_PUSH_REGISTRY}" >/dev/null; then |
There was a problem hiding this comment.
[Minor] (Security) The token is passed via -p "${token}", which exposes it in the process list (ps) and can land in shell history/audit logs on shared machines. Prefer stdin:
printf '%s' "${token}" | ${CONTAINER_ENGINE} login -u "${user}" --password-stdin "${OPENSHIFT_PUSH_REGISTRY}" >/dev/null| "KC_HOSTNAME=${OPENSHIFT_KC_HOSTNAME}" >/dev/null | ||
|
|
||
| info "Configuring API server JWT environment" | ||
| oc_cli set env deployment/hypershell-api-server -n "${OPENSHIFT_NAMESPACE}" -c api-server \ |
There was a problem hiding this comment.
[Minor] (Config vs. code) API_ENV=development_oidc is applied imperatively here rather than declaratively in the deploy/openshift overlay. Setting it in the overlay keeps configuration out of the lifecycle script (matching the deploy/kind overlay) and avoids drift on plain kustomize build. See the Cross-PR coordination note in the top-level review - an open PR sets this same value declaratively in deploy/openshift/kustomization.yaml, so a decision is needed on which layer owns it.
e1483b2 to
c991637
Compare
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
REQUEST_CHANGES — this is a well-structured, thoroughly self-tested OpenShift local-dev workflow, but it broadens a security-critical RBAC grant in the shared base overlay (all environments), which needs to be re-scoped or explicitly signed off before merge. The dev tooling itself is solid; the blocking concern is the bind verb widening, with a few minor security/CI follow-ups.
Summary
The PR introduces an OpenShift ephemeral-namespace development workflow (make openshift-up/down/status, component swaps) via a clean driver abstraction under scripts/cluster/, with a genuinely impressive shell/python test suite (lib_test.sh, rewrite-namespaces.py). Most of the change is dev-only tooling and specs; the one change that reaches production is the controller RBAC broadening in deploy/base/controller-rbac.yaml, which is the reason for REQUEST_CHANGES.
Findings
[Major] Unrestricted bind verb in shared base RBAC — deploy/base/controller-rbac.yaml:20
The controller ClusterRole now grants bind on all roles/rolebindings/clusterroles/clusterrolebindings, replacing the previous narrowly-scoped hypershell-controller-scc-bind ClusterRole (which limited bind to resourceNames: [system:openshift:scc:privileged]). This lets the controller SA bind any ClusterRole — including cluster-admin — defeating Kubernetes escalation prevention, and it applies to the base overlay consumed by every environment. Re-scope bind to the specific SCC ClusterRole(s) the runtime SCC reconciliation needs, or justify matching an existing stage posture so a maintainer can sign off. See inline comment.
[Minor] Registry login passes a bearer token via argv — scripts/cluster/drivers/openshift.sh:1202
Use --password-stdin instead of -p "${token}" to keep the session token out of the process list.
[Minor] Default admin/admin Keycloak exposed via a network Route — deploy/openshift/keycloak-route.yaml
Unlike the localhost-only Kind flow, this exposes the bundled Keycloak (seeded admin/admin) on cluster DNS. Document that openshift-up must target a private cluster and/or rotate the admin credential.
[Minor] New shell/python test harness not run in CI — Makefile:496
make openshift-test and scripts/cluster/ aren't covered by any workflow or shellcheck job. The PR calls CI automation out of scope; please track a follow-up so the suite guards the code.
[Minor] Stale PR description — The "Highlights" and "Scope" text say openshift-up "seeds Fleet/ManagedCluster/…", but Fleet was removed from the stack and this branch's own history drops it from the seed (seed_via_api creates ManagedCluster/GatewayRelease/ManagedDatabase/Gateway only). Update the description to avoid implying a Fleet resource still exists.
What looks good
- Driver seam (
load_cluster_driver+ per-driver function contract) is clean and keepsmake kind-*behavior intact byexec-ing the existingscripts/kind/entrypoints. - Namespace/DNS-label validation (
validate_rfc1123_label,sanitize_dns_label, reserved-namespace guard) and the foreign/owned-namespace refusal logic are careful and well-tested. - Secrets are created as K8s Secret references and piped to
oc applyvia stdin (not logged); Keycloak client redirect URIs avoid wildcards. - The
rewrite-namespaces.pytoken-boundary handling (not rewriting image repos / substrings) and cluster-scoped name prefixing are backed by real assertions inlib_test.sh.
Cross-PR coordination
Another open pull request implements the same OpenShift JWT-auth fix as this one — setting API_ENV=development_oidc for the api-server — but places it declaratively in the deploy/openshift overlay's kustomization patch, whereas this PR applies it imperatively at runtime via oc set env in configure_oidc_from_routes and leaves the overlay itself without that setting. Maintainers should pick a single source of truth: if the overlay owns API_ENV=development_oidc, this PR's runtime oc set env for API_ENV becomes redundant, and non-script consumers (e.g. hub/stage) of deploy/openshift still need the declarative value. There is also an ordering dependency: that PR notes the overlay's JSON6902 add on the env array replaces the whole array (re-declaring HYPERSHELL_SERVICE_ACCOUNT_PROVISIONER_ADDR), and this PR renders that same overlay — so whichever lands first determines whether the provisioner address survives the render. Both PRs also edit deploy/openshift/kustomization.yaml. Please coordinate the placement and merge order with that PR's owner.
Convention Checklist
| Convention | Result |
|---|---|
| No secrets in logs or responses | Pass |
| Secrets stored as K8s Secret references | Pass |
| Input validated (RFC 1123 / reserved namespaces) | Pass |
| Restricted/least-privilege RBAC | Fail |
| Image references consistent across the stack | Pass |
| Reconcile (update-or-create) pattern | Pass |
| Component/tests registered in CI | Fail |
| Conventional commit messages | Pass |
Findings Summary (ordered by severity, highest first):
- [Major] Unrestricted
bindverb added to shared base controller RBAC (privilege escalation) — Security / Least Privilege (controller-rbac.yaml:20) - [Minor] Registry login passes bearer token via argv — Security (openshift.sh:1202)
- [Minor] Default admin/admin Keycloak exposed via network Route — Security (keycloak-route.yaml)
- [Minor] OpenShift test harness not wired into CI — CI / Convention (Makefile:496)
- [Minor] Stale PR description references removed Fleet entity — Docs
| - apiGroups: ["rbac.authorization.k8s.io"] | ||
| resources: ["roles", "rolebindings", "clusterroles", "clusterrolebindings"] | ||
| verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] | ||
| verbs: ["bind", "get", "list", "watch", "create", "update", "patch", "delete"] |
There was a problem hiding this comment.
[Major] Least-privilege regression in shared base RBAC. This adds an unrestricted bind verb on roles, rolebindings, clusterroles, clusterrolebindings. Combined with the existing create on clusterrolebindings, the controller ServiceAccount can now bind any ClusterRole (including cluster-admin) to any subject, which removes Kubernetes' escalation-prevention guard. Previously this capability was expressed narrowly in deploy/openshift/scc.yaml as a dedicated ClusterRole scoped with resourceNames: ["system:openshift:scc:privileged"] and verbs: ["bind"] — this PR deletes that scoped role and replaces it with a broad grant that lands in the base overlay used by all environments (kind, hub, production), not just OpenShift dev.
Please restore a resourceNames-scoped bind rule limited to exactly the SCC ClusterRole(s) the controller must bind at runtime (as the removed hypershell-controller-scc-bind role did), rather than granting bind on all RBAC resources. If the intent is to match an existing stage posture, please call that out explicitly so a maintainer can sign off on the broadened grant. Confidence: High (that this broadens the grant); Medium (that the broad grant is strictly required).
| local user token | ||
| user="$(oc_cli whoami)" | ||
| token="$(oc_cli whoami -t)" | ||
| if ${CONTAINER_ENGINE} login -u "${user}" -p "${token}" "${OPENSHIFT_PUSH_REGISTRY}" >/dev/null; then |
There was a problem hiding this comment.
[Minor] Bearer token passed on the command line. oc whoami -t returns a live session token and it is passed via -p "${token}", which exposes it in the process list (ps, /proc) to other local users for the duration of the login. Prefer --password-stdin:
printf '%s' "${token}" | ${CONTAINER_ENGINE} login -u "${user}" --password-stdin "${OPENSHIFT_PUSH_REGISTRY}" >/dev/nullConfidence: High.
| @@ -0,0 +1,14 @@ | |||
| apiVersion: route.openshift.io/v1 | |||
There was a problem hiding this comment.
[Minor] Public exposure of default admin/admin credentials. Unlike the Kind flow (localhost only), this Route makes the bundled Keycloak reachable on cluster DNS, and the seeded realm ships with admin/admin (see deploy/base/keycloak/keycloak.yaml) which the scripts also rely on. On a shared or internet-reachable OpenShift cluster, anyone who can resolve the Route can log in as admin. Consider documenting that openshift-up must target a private/trusted cluster, and/or rotating the admin credential post-bootstrap. Confidence: Medium.
| @CLUSTER_DRIVER=openshift scripts/cluster/swap.sh down web-console | ||
|
|
||
| .PHONY: openshift-test | ||
| openshift-test: |
There was a problem hiding this comment.
[Minor] New test harness is not wired into CI. make openshift-test runs the 650-line scripts/cluster/lib_test.sh (plus the rewrite-namespaces.py logic), but no workflow in .github/workflows/ invokes it, and scripts/cluster/ isn't covered by any lint/shellcheck job. Regressions in the driver/rewriter logic won't be caught. The PR body lists "CI automation" as out of scope, so a follow-up is fine — but please track adding this target (and shellcheck for scripts/cluster/) to CI so the suite actually guards the code. Confidence: High.
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This PR does two things well: it fixes a genuine multi-tenant safety bug by scoping gateway-namespace garbage collection to a per-instance identity label (hypershell.redhat.io/instance=<HYPERSHELL_NAMESPACE>), and it lands a substantial OpenShift local-dev lifecycle. The control-plane change is well-factored, correctly threaded through every call site, and backed by thorough table-driven and scenario tests, so I'm approving in spirit; the two Major items below are worth a maintainer decision before merge rather than hard blockers.
Strengths
- Instance-scoped GC selector plus the "empty instance aborts the sweep" guard is exactly the right failure mode: it refuses to list-by-generic-labels rather than risk reaping another instance's live namespaces.
HYPERSHELL_NAMESPACEmoved from a hardcoded literal to the downward API (metadata.namespace), which removes the copy-a-static-value footgun that the whole feature depends on.- The delete-driven path deliberately still handles legacy unlabeled namespaces (keyed to a live Gateway in this instance's API server), and that distinction is called out in both code and spec.
- Errors are wrapped with context, no
panic(), reconcile-not-create-or-skip viaEnsureManagedNamespace, and the new behaviors are covered by targeted tests (foreign instance, unlabeled legacy, empty-instance abort, past-grace foreign retention).
Findings
[Major] Pre-existing orphaned legacy namespaces are now permanently un-reapable (no backfill). Test Diff Scrutiny / Migration
Before this change, periodic GC reaped gateway namespaces by name prefix, which "keeps pre-existing orphaned gateway namespaces eligible for periodic GC without a label migration" (the prior doc comment). After this change, IsGatewayNamespaceForGC requires the instance label, and the new spec scenario "Unlabeled legacy namespace is not swept" makes that permanent. For an already-orphaned legacy namespace (its Gateway was deleted while the control plane was down — the exact case periodic GC was built to recover), no code path ever stamps the instance label: EnsureManagedNamespace only runs on the reconcile path for a live Gateway, and the delete-driven path never fires because there is no delete event. Such namespaces now leak forever. This is the "optional -> required without a fallback/backfill" pattern: the old value (no instance label) can already exist in a running environment. Please add either a one-time backfill (stamp the instance label on unlabeled openshell-* namespaces at startup, then let normal GC handle orphans) or a documented manual cleanup step in the migration notes. Confidence: Medium.
[Major] bind verb broadened from the privileged SCC only to all (cluster)roles. Security / Least Privilege
The removed deploy/openshift/scc.yaml block granted bind narrowly (resourceNames: ["system:openshift:scc:privileged"]) with an explicit comment about Kubernetes escalation prevention. This PR instead adds bind to the main controller rule over ["roles","rolebindings","clusterroles","clusterrolebindings"] with no resourceNames. Combined with the pre-existing create/update/patch/delete on rolebindings/clusterrolebindings, the controller SA can now bind any ClusterRole (including cluster-admin) to any subject. Please scope the bind grant to the specific ClusterRole(s) the runtime reconcileOpenShiftSCC actually binds (the privileged SCC), restoring least privilege. Confidence: Medium.
Cross-PR coordination
- #200 edits the same
specs/platform/openshell-gateway-namespace-gc.spec.mdand reframes periodic GC as "a safety net for legacy, force-deleted, or otherwise untracked namespaces" plus a durable "deleting-state finalization" model. That directly contradicts this PR, which makes periodic GC ignore unlabeled/legacy namespaces and keeps an event-driven delete model. Maintainers must decide the single canonical namespace-GC/deletion contract and the merge order, because the two specs cannot both be true as written. - #217 solves the same OpenShift JWT problem as this PR — the API server needs
API_ENV=development_oidcor JWT auth silently 401s — but via a declarativedeploy/openshift/kustomization.yamloverlay patch, whereas this PR applies it imperatively withoc set envinscripts/cluster/drivers/openshift.sh. These are competing solutions to one problem touching the same OpenShift auth config. A decision is needed on where OIDC/API_ENVconfiguration lives (overlay vs. script) so the two do not diverge or override each other. - #194 restructures
ReconcileGatewayfor Helm-based gateway deployment while keeping thenamespaceExists/createNamespacehelpers and a two-label namespace scheme (--create-namespace=false, labelsmanaged-by+managed=trueonly). This PR deletes those helpers in favor ofEnsureManagedNamespaceand makes thehypershell.redhat.io/instancelabel mandatory for GC ownership. The two are incompatible: whichever merges second must adopt the other's namespace-creation API and labeling contract, or Helm-created namespaces will lack the instance label and be excluded from GC. Maintainers should align the namespace-ownership model and the shared helper API and set a merge order.
Findings Summary (ordered by severity, highest first)
- [Major] Pre-existing orphaned legacy namespaces are permanently un-reapable with no backfill/migration - Test Diff Scrutiny / Migration (namespace.go L108; spec L185)
- [Major]
bindverb broadened from privileged-SCC-only to all (cluster)roles - Security / Least Privilege (controller-rbac.yaml L20)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound handled for 404 scenarios |
Pass |
| Reconcile pattern (not create-or-skip) | Pass |
| Proper context propagation | Pass |
| Restricted RBAC / least privilege | Fail |
| Migration/backfill for tightened precondition | Fail |
| Test diff scrutiny (modified assertions justified) | Pass |
| Conventional commit messages | Pass |
| - apiGroups: ["rbac.authorization.k8s.io"] | ||
| resources: ["roles", "rolebindings", "clusterroles", "clusterrolebindings"] | ||
| verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] | ||
| verbs: ["bind", "get", "list", "watch", "create", "update", "patch", "delete"] |
There was a problem hiding this comment.
[Major] Least privilege: bind is now cluster-wide.
The removed deploy/openshift/scc.yaml scoped bind to resourceNames: ["system:openshift:scc:privileged"] specifically to satisfy Kubernetes escalation prevention without granting broad binding power. Adding bind here over roles/rolebindings/clusterroles/clusterrolebindings with no resourceNames lets the controller SA bind any ClusterRole (e.g. cluster-admin) to any subject, since it already has create/update/delete on (cluster)rolebindings.
Consider a dedicated rule scoped to the exact ClusterRole(s) reconcileOpenShiftSCC binds:
- apiGroups: ["rbac.authorization.k8s.io"]
resources: ["clusterroles"]
resourceNames: ["system:openshift:scc:privileged"]
verbs: ["bind"]Confidence: Medium.
| // owned by a different instance, or lacking this instance's identity label, are | ||
| // never eligible: another HyperShell's live gateways would otherwise look | ||
| // orphaned because they are absent from this instance's API server. | ||
| func IsGatewayNamespaceForGC(ns *corev1.Namespace, instance string) bool { |
There was a problem hiding this comment.
[Major] Legacy orphans can no longer be reaped, and nothing backfills the label.
Requiring the instance label here (via IsManagedNamespace) means an already-orphaned legacy namespace - two management labels, no instance label, and no live Gateway (the missed-delete case periodic GC was built for) - is now never swept. EnsureManagedNamespace only stamps the label on the reconcile path of a live Gateway, and the delete-driven path never fires for an orphan, so these namespaces leak permanently.
This is the optional→required transition without a fallback for pre-existing data. Please add a one-time backfill (stamp instance on unlabeled openshell-* namespaces at startup, then let GC proceed) or document a manual cleanup in the migration notes. Confidence: Medium.
There was a problem hiding this comment.
We might need to manually migrate the labels or run a label migration on first run.
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT — This is a well-engineered PR: the control-plane change moves gateway/database namespaces from generic management labels to a per-instance identity label (hypershell.redhat.io/instance sourced from the pod's own namespace via the downward API), so two HyperShells on one cluster no longer treat each other's gateways as orphans. Error handling, context propagation, and test coverage are strong, but the legacy-namespace backfill reintroduces a cross-instance data-loss window that the maintainers should decide on before merge.
Summary
The instance-label design is sound and defensively coded (empty-instance guards, hasManagementLabels defense-in-depth, foreign-instance refusal, additive test coverage). My main concern is BackfillInstanceLabels: to make pre-label orphans visible again it claims every unlabeled openshell-* namespace on the cluster for the sweeping instance, which can capture and later reap another live instance's namespaces during a migration — the exact isolation the rest of the PR establishes. The remaining items are lower severity.
Blocking / Major
- [Major] Legacy backfill can claim and reap another instance's live gateway namespaces.
BackfillInstanceLabels(components/control-plane/internal/gateway/namespace.go:82) stamps this instance onto any unlabeledopenshell-*namespace cluster-wide, andreconcileOncethen treats it as an orphan because it isn't in this instance's live Gateway set. In a cluster running two HyperShells whose gateway namespaces predate the instance label, the first instance to run the new code claims the other's namespaces; the other instance can no longer re-adopt them (EnsureManagedNamespacerefuses a foreign label) and they are reaped after the grace period.TestReconcileOnce_OnlySweepsThisInstancedemonstrates the mechanism: the unlabeled legacy namespace is claimed and stampedgc-eligible-sincein the same sweep. This directly undermines the multi-instance isolation the PR is built to provide. Please decide on a safer migration (e.g. only backfill namespaces confirmed to back one of this instance's live Gateways, gate the backfill behind single-instance detection, or make it opt-in). This is a Blocker if multiple HyperShells per cluster with pre-existing gateways is a supported deployment.
Minor
- [Minor]
bindon the privileged SCC moved into base RBAC. Thebindverb onsystem:openshift:scc:privilegedmoved from the OpenShift overlay intodeploy/base/controller-rbac.yaml:20-24, so every overlay (kind, hub, base) now grants the controller ClusterRole this privilege. It is inert where the SCC ClusterRole doesn't exist, but confirm the broadened base RBAC surface is intentional rather than overlay-scoped. - [Minor] Empty-instance selector produces an invalid label value.
ManagedNamespaceSelector("")returnshypershell.redhat.io/instance=__no-such-instance__(components/control-plane/internal/gateway/namespace.go:60); leading/trailing underscores are not a valid label value, so aListwith it would 400 rather than match nothing. It's currently unreachable becausereconcileOnceaborts on an empty instance first, but a real empty-set selector or an explicit error would be less fragile.
Test Diff Scrutiny
TestIsGatewayNamespaceForGC flips the legacy-unlabeled case from GC-eligible (true) to not-eligible (false) — a removed guarantee. It is acceptable here because the guarantee is restored through a documented fallback (BackfillInstanceLabels stamps the instance so the sweep can still reap the orphan), but that same fallback is the source of the Major finding above. The other test edits are mechanical signature updates plus additive cases.
Positives
- No
panic(); errors wrapped withfmt.Errorf("context: %w", …)and aggregated viaerrors.Join. - Instance identity sourced from the downward API (
metadata.namespace) instead of a hardcoded value. EnsureManagedNamespaceis a proper update-or-create reconcile with create-race handling; no create-or-skip.- No secrets logged; dev-tooling scripts use
set -euo pipefailand keep tokens out of logs.
Cross-PR coordination
- #217 proposes the identical
deploy/openshift/kustomization.yamlfix that this PR also contains: switching the OpenShift overlay toAPI_ENV=development_oidcand re-declaringHYPERSHELL_SERVICE_ACCOUNT_PROVISIONER_ADDRso--enable-jwt=trueis not clobbered. These are duplicate solutions to the same OpenShift JWT-auth defect in the same file. Maintainers should decide which PR owns the fix and coordinate merge order; whichever merges second must drop its redundant copy to avoid a conflicting/no-op change.
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 |
| Reconcile (update-or-create), not create-or-skip | Pass |
| Proper context propagation | Pass |
| SecurityContext / least-privilege RBAC | See Minor (base SCC bind) |
| Multi-instance isolation preserved | Fail (legacy backfill window) |
| Conventional commit messages | Pass |
Findings Summary (ordered by severity, highest first):
- [Major] Legacy backfill claims/reaps other instances' live gateway namespaces during migration - Reconciliation / Data loss (namespace.go:82, reconciler/namespace.go:146)
- [Minor]
bindon privileged SCC broadened to base RBAC - Security / RBAC (controller-rbac.yaml:20-24) - [Minor] Empty-instance selector emits an invalid label value - Robustness (namespace.go:60)
| // different instance label, and never claims ManagedDatabase namespaces. An | ||
| // empty instance is a configuration error: unlabeled namespaces must not be | ||
| // claimed without an identity. | ||
| func BackfillInstanceLabels(ctx context.Context, client kubernetes.Interface, instance string) error { |
There was a problem hiding this comment.
[Major] Cross-instance data-loss window in the legacy backfill.
This claims every unlabeled openshell-* namespace on the cluster for instance, and reconcileOnce then marks any that aren't in this instance's live Gateway set as gc-eligible-since in the same sweep (see TestReconcileOnce_OnlySweepsThisInstance). In a cluster running two HyperShells whose gateway namespaces predate the instance label, the first instance to run this code claims the other's namespaces; the other instance can no longer re-adopt them (EnsureManagedNamespace refuses a foreign label) and they get reaped after the grace period.
This contradicts the multi-instance isolation the rest of the PR establishes. Consider only backfilling namespaces confirmed to back one of this instance's live Gateways, gating the backfill behind single-instance detection, or making it opt-in. Blocker if multiple HyperShells per cluster with pre-existing gateways is a supported deployment.
There was a problem hiding this comment.
Sure, but at the end of the day reaping the gateways is the goal and while it's a bit messy it won't matter from that point onward, it's not less messy then if someone manually went in and called delete.
| // labels alone would treat every other HyperShell on the cluster as an orphan. | ||
| func ManagedNamespaceSelector(instance string) string { | ||
| if instance == "" { | ||
| return InstanceLabel + "=__no-such-instance__" |
There was a problem hiding this comment.
[Minor] __no-such-instance__ has leading/trailing underscores, which is not a valid Kubernetes label value, so a List using this selector would return HTTP 400 rather than match nothing. It's currently unreachable (reconcileOnce aborts on an empty instance first), but returning an explicit error or an empty result set would be less fragile than relying on the caller's guard.
| - apiGroups: ["rbac.authorization.k8s.io"] | ||
| resources: ["clusterroles"] | ||
| resourceNames: ["system:openshift:scc:privileged"] | ||
| verbs: ["bind"] |
There was a problem hiding this comment.
[Minor] bind on system:openshift:scc:privileged now lives in base RBAC, so every overlay (kind, hub, base) grants the controller ClusterRole this privilege rather than only the OpenShift overlay. It's inert where the SCC ClusterRole doesn't exist, but please confirm broadening the base RBAC surface is intended rather than keeping it overlay-scoped.
6d74262 to
6014ae9
Compare
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
Solid, defensively-written change that gives gateway/DB namespaces a per-instance ownership identity (hypershell.redhat.io/instance) so two HyperShells on one cluster stop reaping each other's namespaces, plus a complete OpenShift local-dev lifecycle. The instance-label migration is handled correctly (backfill of legacy unlabeled namespaces, foreign labels never overwritten, empty-instance sweeps aborted), so I have no blocking findings; the notes below are minor plus cross-PR coordination.
What I checked
- Instance-ownership model (
internal/gateway/namespace.go,internal/reconciler/namespace.go):IsManagedNamespace/IsGatewayNamespaceForGC/DeleteManagedNamespacenow require the caller's instance identity; the GC sweep aborts on an empty identity andManagedNamespaceSelector("")returns a match-nothing selector plus an error, so a dropped identity cannot escalate into a cluster-wide sweep. Good defense-in-depth. - Migration/backfill: existing namespaces created before this PR carry the two management labels but no instance label.
BackfillInstanceLabelsclaims those legacyopenshell-*(non-openshell-db-*) namespaces at the start of each sweep, never overwrites a foreign instance label, and never claims ManagedDatabase namespaces. The gateway-delete path still deletes legacy unlabeled namespaces because it is keyed to a Gateway from this instance's API server. This is the fallback path the test-diff-scrutiny rule requires, so themanagedNShelper now stampingInstanceLabeland the tightened assertions are justified rather than a silently removed guarantee. - Test diff: assertion changes in pre-existing tests are backed by the backfill path; new tests cover multi-instance isolation, live-unlabeled retention, and the DB-namespace exclusion.
NewManagedDatabaseReconcilercallers updated consistently. - Error handling: errors wrapped with
%w,IsNotFound/IsAlreadyExistshandled,errors.Joinfor aggregate failures, nopanic(), no secret values logged. - Shell tooling (
scripts/cluster/*): RFC1123 validation, reserved/system-namespace refusal, and foreign-environment refusal guard every destructiveoc delete project;lib_test.shexercises them. RBACbindis scoped to exactlysystem:openshift:scc:privileged, and SCC bindings are correctly narrowed from ClusterRoleBinding to namespace-scoped RoleBinding for ephemeral environments.
Findings
[Minor] Provisioner dial FQDN hardcodes hypershell-system while the controller identity is now dynamic — deploy/base/api-server.yaml sets HYPERSHELL_SERVICE_ACCOUNT_PROVISIONER_ADDR to hypershell-controller.hypershell-system.svc.cluster.local:9443, but the controller's HYPERSHELL_NAMESPACE is now sourced from the downward API (metadata.namespace) and drives the instance label. In the base overlay both land in hypershell-system, so this is consistent today and the OpenShift overlay rewrites the segment; the coupling is only a hazard if the base is ever applied to a different namespace. Consider a comment cross-referencing the rewrite, or templating the namespace segment, so the two never silently diverge. Confidence: Medium.
Cross-PR coordination
- #217 implements the same fix as this PR — setting
API_ENV=development_oidcindeploy/openshift/kustomization.yamlto stop the OpenShift+Keycloak JWT 401 login loop. This PR also adds that exact env var to the same overlay with the same rationale. This is a duplicate solution in one file; maintainers should decide which one lands and drop or rebase the other so the two do not collide. - #194 re-architects gateway provisioning onto the upstream OpenShell Helm chart and, in the same
internal/gateway/reconciler.goblock this PR rewrites, keeps the oldnamespaceExists/createNamespacepath that this PR deletes in favor ofEnsureManagedNamespace(instance). These are competing refactors of the same namespace-creation code, and more importantly a design conflict: namespaces created via #194's retained path (or by Helm) would not carry this PR'shypershell.redhat.io/instanceownership label that GC now depends on, so #194's gateway namespaces would be treated as legacy/unlabeled. Maintainers need to decide a merge order and whether Helm-created namespaces must stamp the instance label. - #212 (same author) introduces a parallel driver abstraction under
tests/e2e/drivers/{kind,openshift}.shwhile this PR introducesscripts/cluster/drivers/{kind,openshift}.sh, and both substantially edit the shared specsspecs/platform/openshift-development.spec.md,specs/platform/e2e-testing.spec.md,specs/platform/local-development.spec.md, andskills/RECONCILE.md. This PR explicitly defers the E2E driver, which #212 delivers, so the two must land in a defined order and reconcile the overlapping spec/driver-model text; maintainers should confirm the sequence and whether the two driver abstractions are meant to be unified.
Findings Summary (ordered by severity, highest first):
- [Minor] Provisioner dial FQDN hardcodes
hypershell-systemwhile the controller instance identity is now dynamic - Config Coupling (deploy/base/api-server.yaml L96)
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 (RFC1123 labels, reserved-namespace guards) | Pass |
| Reconcile (update-or-create) pattern used | Pass |
| Test diff: modified assertions have migration/fallback | Pass |
| Image references consistent across manifests | Pass |
| Conventional commit messages | Pass |
| # hypershell-system to OPENSHIFT_NAMESPACE. | ||
| - name: HYPERSHELL_SERVICE_ACCOUNT_PROVISIONER_ADDR | ||
| value: "hypershell-controller:9443" | ||
| value: "hypershell-controller.hypershell-system.svc.cluster.local:9443" |
There was a problem hiding this comment.
[Minor] FQDN namespace segment is hardcoded while the controller identity is now dynamic.
This dials hypershell-controller.hypershell-system.svc.cluster.local, but the controller's HYPERSHELL_NAMESPACE (which now drives the hypershell.redhat.io/instance label) is sourced from the downward API (metadata.namespace) in deploy/base/controller.yaml. In the base overlay both resolve to hypershell-system, so this is consistent today and the OpenShift overlay rewrites the segment. The coupling only bites if base is ever applied to a different namespace. Consider cross-referencing the rewrite in the comment or templating the namespace segment so the dial target and the controller's own namespace/instance identity can't silently diverge.
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT. This is a large, carefully documented change that makes control-plane namespace ownership multi-instance-safe (instance-label scoping), converts the controller's background goroutines from fail-fast to supervised-restart, and adds a full OpenShift local-dev lifecycle. The Go changes are clean (no panic(), wrapped errors, IsNotFound handled, thorough tests with documented legacy fallback), so my findings are minor; the more consequential items are cross-PR coordination decisions the maintainers should make before merge.
What's good
- The optional→required tightening on namespace ownership (management labels now also require
hypershell.redhat.io/instance) is handled correctly:DeleteManagedNamespacestill reaps legacy unlabeled namespaces on the delete-driven path, periodic GC ignores them until manually labeled, andEnsureManagedNamespacerestamps live namespaces on the next reconcile. Test diffs cover foreign-instance, unlabeled-legacy, and empty-instance cases — the changed assertions are a deliberate, documented contract change with a fallback, not a silent test fixup. - Empty-instance guards abort the GC sweep and produce a match-nothing (but still valid) label selector, avoiding both an HTTP 400 and a cluster-wide reap.
runSupervisedcorrectly returns onctx.Err()and theWaitGroupmakes shutdown wait for components to observe cancellation.
Findings
[Minor] Persistent per-watch failures are now only visible in logs (observability).
components/control-plane/cmd/hypershell-controller/main.go — moving from fail-fast (os.Exit(1)) to runSupervised is a good resilience win, but a watch stream (e.g. Gateway watch) that fails permanently — for example a durable RBAC/permission error — will now spin forever emitting only WARN ... restarting, silently stopping reconciliation with no CrashLoopBackOff and no health/metric signal. The provisioner is covered by the new TCP liveness probe, but the watch/reconciler loops are not. Consider surfacing a per-component restart counter/metric or a readiness signal so a stuck component is observable.
[Minor] bind on the privileged SCC moved into the base ClusterRole.
deploy/base/controller-rbac.yaml — the bind verb on system:openshift:scc:privileged previously lived in the OpenShift overlay (scc.yaml); it now sits in the base controller ClusterRole and therefore applies to every overlay (including Kind, where that ClusterRole doesn't exist). It's harmless (RBAC rules over non-existent resources are inert), but it broadens the base permission surface. Confirm this is intentional rather than overlay-scoped.
[Minor] Local-dev OpenShift bootstrap uses static credentials against the caller's current kube-context.
scripts/cluster/drivers/openshift.sh (create_bootstrap_secrets, seeding/redirect curls use admin/admin) — acceptable for local development and consistent with kind-up, but this driver acts on whatever context the user has selected. The foreign-namespace / reserved-name guards mitigate accidental teardown; a similar up-front confirmation of the target cluster/context would reduce the risk of seeding weak dev secrets into an unintended cluster.
[Minor] PR description references seeding a Fleet.
The body lists "seeds Fleet/ManagedCluster/GatewayRelease/ManagedDatabase/Gateway", but Fleet was removed from the stack and the scripts correctly do not seed one. Please update the description to avoid confusion.
Cross-PR coordination
- #217 implements exactly the fix this PR already contains: setting
API_ENV=development_oidcindeploy/openshift/kustomization.yamlto stop thedevelopmentenvironment from clobbering--enable-jwt=true(the OpenShift+Keycloak 401 loop). These are duplicate solutions to the same problem in the same file. The maintainers should decide which one lands; if this PR merges, #217 becomes redundant and should be closed or reduced, and vice-versa. - #194 restructures the same control-plane gateway-provisioning layer this PR modifies — it replaces the gateway deployment path in
internal/gateway/reconciler.gowith a Helm-based install and reworks OpenShift SCC binding, while this PR introduces the instance-label namespace-ownership contract (EnsureManagedNamespace, instance-scopedIsManagedNamespace/IsGatewayNamespaceForGC) and adds platform NetworkPolicies. Whichever merges second must carry the other's model (instance-label ownership must survive the Helm rewrite; the "no NetworkPolicies for gateways" decision in #194 must be reconciled with this PR's NetworkPolicy usage). A merge-order and integration decision is needed.
Findings Summary (ordered by severity, highest first)
- [Minor] Permanent per-watch/reconciler failures now surface only as WARN logs, with no health/metric signal — Observability (main.go L54, L61)
- [Minor]
bindon privileged SCC moved from the OpenShift overlay into the base ClusterRole — RBAC scope (controller-rbac.yaml L21-24) - [Minor] Local-dev OpenShift bootstrap seeds static credentials against the caller's active kube-context — Security (dev tooling) (openshift.sh L440)
- [Minor] PR description mentions seeding a removed
Fleetentity — Docs
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 error messages | Pass |
| Input validated (instance identity, label selectors) | Pass |
| Reconcile (update-or-create) pattern used | Pass |
| Proper context propagation | Pass |
| Test diff scrutiny (optional→required has fallback + tests) | Pass |
| SecurityContext on pod specs unchanged/preserved | Pass |
| Image references consistent across manifests | Pass |
| Conventional commit messages | Pass |
| if err == nil || ctx.Err() != nil { | ||
| return | ||
| } | ||
| log.Printf("WARN %s exited with error, restarting in %s: %v", name, backoff, err) |
There was a problem hiding this comment.
[Minor] Observability: this is a solid resilience change over the old os.Exit(1), but a component that fails permanently (e.g. a watch stream hitting a durable RBAC error) will now loop here forever emitting only WARN ... restarting — reconciliation stops silently with no CrashLoopBackOff and no health/metric signal. The provisioner is covered by the new TCP liveness probe, but the watch/reconciler loops are not. Consider a per-component restart counter/metric or readiness signal so a stuck component is observable.
| - apiGroups: ["rbac.authorization.k8s.io"] | ||
| resources: ["clusterroles"] | ||
| resourceNames: ["system:openshift:scc:privileged"] | ||
| verbs: ["bind"] |
There was a problem hiding this comment.
[Minor] RBAC scope: bind on system:openshift:scc:privileged previously lived only in the OpenShift overlay (scc.yaml). Moving it into the base ClusterRole applies it to every overlay including Kind (where the referenced ClusterRole doesn't exist). It's inert there, but it broadens the base permission surface — please confirm this is intentional rather than overlay-scoped.
| apply_sandbox_scc || true | ||
| } | ||
|
|
||
| create_bootstrap_secrets() { |
There was a problem hiding this comment.
[Minor] Dev tooling security: these static credentials (control-plane-secret, provisioner-secret, and the admin/admin curls) are fine for local dev and match kind-up, but this driver acts on whatever kube-context the caller has selected. The foreign-namespace / reserved-name guards mitigate teardown mistakes; an up-front confirmation of the target cluster/context would similarly reduce the risk of seeding weak dev secrets into an unintended cluster.
5cbfd9b to
86bf08a
Compare
Amber reviewStatus: Stopped The pull request head changed before Amber posted the review. A later job can review the new head. |
86bf08a to
59f69a0
Compare
Amber reviewStatus: Stopped The pull request head changed before Amber posted the review. A later job can review the new head. |
squizzi
left a comment
squizzi
left a comment
There was a problem hiding this comment.
This PR substantially improves OpenShift local development, and the focused lifecycle, dependency-pin, and control-plane tests pass. However, the teardown ownership check currently permits deletion of a pre-existing unlabeled OpenShift project, which is unsafe for a command intended to clean up only its own ephemeral environment.
Overall assessment: REQUEST_CHANGES
Findings Summary (ordered by severity, highest first):
- [Blocker]
openshift-downtreats an unlabeled existing project as verified and permanently deletes it - Safety (L1068, L1168)
Convention Checklist (omit conventions not applicable to the diff):
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
| No secrets in logs or responses | Pass |
| Input validated | Pass |
| SecurityContext on all pod specs | Pass |
| Reconcile pattern used (not create-or-skip) | Pass |
| Image references consistent across manifests | Pass |
| Conventional commit messages | Pass |
| Modified test assertions scrutinized | Pass |
| local owned env_id | ||
| owned="$(namespace_label_value "${ns}" "${OWNED_LABEL}")" | ||
| env_id="$(namespace_label_value "${ns}" "${ENV_LABEL}")" | ||
| if [[ "${owned}" == "true" && -n "${env_id}" && -n "${OPENSHIFT_ENVIRONMENT_ID:-}" && "${env_id}" != "${OPENSHIFT_ENVIRONMENT_ID}" ]]; then |
There was a problem hiding this comment.
[Blocker] Unsafe deletion of an unowned project
verify_owned_namespace only rejects a namespace when it already has both ownership labels and belongs to another environment. For a normal pre-existing project with no HyperShell labels, it returns successfully; cluster_down then marks it eligible and calls oc delete project at L1191. Since the default namespace is the caller’s current oc project, running make openshift-down after selecting an unrelated project can delete that entire project and its unrelated workloads.
Require both hypershell.redhat.io/owned=true and a nonempty environment ID before allowing project deletion (and verify the platform/Keycloak IDs match). For unlabeled or partially labeled namespaces, refuse the project deletion; at most offer an explicitly confirmed, label-scoped cleanup path. Confidence: High (98%).
1866227 to
f0d2328
Compare
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
Assessment: COMMENT. This is a large, well-engineered PR whose control-plane core (instance-scoped namespace ownership + GC, supervised background components) is careful and thoroughly unit-tested, with a genuine migration/fallback story for the tightened namespace contract. I found no Blocker or Critical issues; the notes below are robustness and hygiene items, plus cross-PR coordination that maintainers should decide before merge order.
I'm Amber. Two-sentence summary: the namespace-ownership refactor correctly moves gateway GC from a name-prefix sweep to an instance-scoped model (hypershell.redhat.io/instance=<HYPERSHELL_NAMESPACE>), and — importantly for a contract change — it keeps live gateways backfilled via EnsureManagedNamespace, keeps the delete-driven path able to remove legacy unlabeled namespaces, and deliberately leaves pre-existing orphans untouched rather than over-reaping. The main risks are a couple of silent-failure/robustness edges in the new supervisor and dev tooling, and coordination with other in-flight control-plane/deploy work.
Test Diff Scrutiny
The modified assertions in namespace_test.go (both packages) flip IsManagedNamespace/IsGatewayNamespaceForGC from "two labels ⇒ managed" to "two labels + matching instance ⇒ managed". This is a tightened contract, but it is handled correctly per project standards: (a) EnsureManagedNamespace backfills the instance label on the next reconcile of any live gateway, (b) the delete-driven path still removes legacy unlabeled namespaces, and (c) new tests explicitly cover legacy-unlabeled, foreign-instance, and empty-instance cases. No removed guarantee is left without a fallback. Good.
Findings
[Minor] runSupervised treats an early nil return as a clean stop, silently disabling that component — components/control-plane/cmd/hypershell-controller/main.go:58. If a watch/reconciler fn ever returns nil before ctx is cancelled (unexpected clean stream end), the goroutine returns and is never restarted while the process keeps running with that watch silently gone. The functions are documented to return ctx.Err(), so this is unlikely, but it runs counter to "never silently swallow partial failures." Consider treating err == nil && ctx.Err() == nil as an anomaly worth logging and restarting.
[Minor] Post-upgrade orphans predating the instance label are no longer auto-reaped — components/control-plane/internal/reconciler/namespace.go:48. This is intentional and documented (safe over reaping), but operators upgrading a running environment will find pre-existing orphaned openshell-* namespaces stay until manually labeled. Worth a one-line runbook/upgrade note so the change in GC coverage isn't surprising.
[Minor] Fragile JSON6902 whole-array replace of the api-server env — deploy/openshift/kustomization.yaml:97. Re-declaring the base env inline (POD_NAMESPACE, provisioner addr, DB_SSLMODE) means a future base-manifest env change silently won't reach this overlay. The comment acknowledges it; a follow-up using a targeted add per element (or a documented sync check) would be more durable.
[Minor] Hard-coded dev bootstrap credentials — scripts/cluster/drivers/openshift.sh:454. control-plane-secret, provisioner-secret, and admin/admin are fine for ephemeral local-dev namespaces, but please keep an explicit guard/note that this driver must never target a shared or persistent environment, since these are predictable literals.
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 (selector/instance identity) | Pass |
| SecurityContext / probes on pod specs | Pass |
| Reconcile (update-or-create) pattern | Pass |
| Status/error paths not silently swallowed | Partial (see runSupervised) |
| Test diff scrutiny (contract change has fallback/backfill) | Pass |
| Image references consistent (multi-arch Dockerfile pins + CI check) | Pass |
| Conventional commits | Pass |
Findings Summary (highest severity first)
- [Minor]
runSupervisedsilently stops a component on earlynilreturn - Reliability / partial-failure (main.go:58) - [Minor] Pre-instance-label orphans no longer auto-reaped after upgrade - Operability (reconciler/namespace.go:48)
- [Minor] JSON6902 whole-array env replace is drift-prone - Config maintainability (kustomization.yaml:97)
- [Minor] Hard-coded predictable dev bootstrap secrets - Security hygiene (dev-only) (openshift.sh:454)
Cross-PR coordination
The following require a maintainer decision or a defined merge order:
-
#217 proposes the exact
deploy/openshift/kustomization.yamlfix (API_ENV=development_oidc, plus re-declaring the whole env array including the service-account provisioner address) that this PR already contains in full in the same file. This is a duplicate solution: one of the two must be dropped or rebased, and maintainers should decide which carries the change so the other does not land a redundant/conflicting overlay edit. -
#194 re-implements the gateway deployment path (Helm-based
ReconcileGateway, rewriting the sameinternal/gateway/reconciler.goandinternal/reconciler/reconciler.gonamespace-creation code this PR changes). This PR introduces the instance-label ownership model (EnsureManagedNamespacestampinghypershell.redhat.io/instance) that gateway GC now depends on. Whichever merges second must preserve that ownership stamping in the new deployment path, or GC will stop recognizing gateway namespaces. Maintainers should agree the ownership model and sequence the two changes. -
#185 specifies control-plane world synchronization and orphan cleanup and explicitly builds on the namespace-GC contract in
openshell-gateway-namespace-gc.spec.md— the contract this PR rewrites from a two-label/name-prefix sweep to an instance-scoped (hypershell.redhat.io/instance) sweep. Its ownership tables assume the prior model and use a separatemanaged-database-idlabel scheme, while this PR also stamps the instance label onto ManagedDatabase namespaces viaEnsureManagedNamespace. The canonical ownership-labeling model and spec wording must be reconciled, and the specs sequenced, so implementation and specification agree.
| backoff := time.Second | ||
| for { | ||
| err := fn(ctx) | ||
| if err == nil || ctx.Err() != nil { |
There was a problem hiding this comment.
[Minor] Early nil return silently stops the component. If any supervised fn returns nil before ctx is cancelled (e.g. a watch stream ends cleanly without a ctx error), runSupervised returns and that goroutine is never restarted while the rest of the process keeps running — the watch/reconciler is silently gone. The functions are documented to return ctx.Err(), so this is unlikely, but it runs against the "never silently swallow partial failures" convention. Consider logging and restarting when err == nil && ctx.Err() == nil.
| // this control-plane instance created but that no longer have a live Gateway in | ||
| // this instance's API server. Other HyperShell instances on the same cluster are | ||
| // ignored: the sweep selects on hypershell.redhat.io/instance=<HYPERSHELL_NAMESPACE>. | ||
| // Legacy gateway namespaces that predate the instance label must be labeled |
There was a problem hiding this comment.
[Minor] Upgrade behavior note. Instance-scoped GC means gateway namespaces created before the instance label existed (two management labels, no instance label) that are already orphaned (no live Gateway to trigger EnsureManagedNamespace backfill) will never be auto-reaped again — they must be labeled manually. This is the safe direction and is documented in the spec, but an explicit upgrade/runbook line would help operators who relied on the old name-prefix sweep to clear stale openshell-* orphans.
| # deploy/kind overlay and the gitops jwt-enforce component. | ||
| - name: API_ENV | ||
| value: "development_oidc" | ||
| # Re-declare the base env: this JSON6902 add REPLACES the whole env |
There was a problem hiding this comment.
[Minor] Drift-prone overlay. This JSON6902 add replaces the whole env array, so the base manifest's env (POD_NAMESPACE, provisioner address, DB_SSLMODE) has to be re-declared here and will silently diverge if the base changes. The comment acknowledges it; a targeted per-element add or a documented sync check would be more durable against future base-manifest edits.
| -n "${OPENSHIFT_NAMESPACE}" \ | ||
| --from-literal=api-service.issuerUrl="${kc_svc}/realms/hypershell" \ | ||
| --from-literal=api-service.clientId="hypershell-control-plane" \ | ||
| --from-literal=api-service.clientSecret="control-plane-secret" \ |
There was a problem hiding this comment.
[Minor] Predictable dev bootstrap secrets. control-plane-secret / provisioner-secret (and the admin/admin Keycloak login used later) are acceptable for ephemeral local-dev namespaces, but they are fixed literals. Please keep the guard/note that this driver must only ever target throwaway dev namespaces and never a shared or persistent cluster.
f0d2328 to
935baba
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 large, carefully engineered PR that adds an OpenShift local-dev lifecycle and, along the way, refactors control-plane namespace ownership to be instance-scoped (multi-HyperShell-safe) and hardens the controller's goroutine supervision. The Go changes are well-tested and defensive; my main substantive concern is a garbage-collection coverage gap for namespaces that predate the new instance label, plus two cross-PR coordination items maintainers should decide on before merge.
What I looked at
internal/gateway/namespace.go/reconciler.go: thecreateNamespace/namespaceExistspath is replaced byEnsureManagedNamespace(instance), and GC selection/ownership now keys on a newhypershell.redhat.io/instancelabel. Well-scoped, with strong table tests and additive test cases.cmd/hypershell-controller/main.go:errCh+os.Exit(1)supervision replaced with per-componentrunSupervisedunder aWaitGroup. Good isolation; one edge case noted inline.- Deploy manifests:
HYPERSHELL_NAMESPACE/POD_NAMESPACEfrom the downward API, cluster-local FQDN for the provisioner dial target, SCC bindings converted to namespacedRoleBindings, and thebindverb on the privileged SCC ClusterRole folded into the main controller ClusterRole (functionally equivalent to the removed dedicated bind ClusterRole — no new privilege). - Multi-arch Dockerfiles: per-arch digest pins are consistent across api-server/control-plane/cli; web-console uses its own base set as expected.
- Shell tooling (
scripts/cluster/*):set -euo pipefail, RFC1123 validation, reserved-namespace refusal, and a 955-line test suite. Deletion guards look sound.
Findings
[Major] GC no longer reaps pre-existing orphaned gateway namespaces.
ManagedNamespaceSelector/IsGatewayNamespaceForGC now require the hypershell.redhat.io/instance label. Live gateways get re-labeled on the next reconcile via EnsureManagedNamespace, and the delete-driven path still removes legacy 2-label namespaces — but a namespace that is already orphaned at upgrade time (no live Gateway to trigger re-labeling) carries only the two management labels and will therefore never match the sweep. Under the previous code these were exactly the namespaces GC existed to reap. The IsGatewayNamespaceForGC test flip (legacy 2-label namespace true -> false) confirms this is a removed guarantee. It is documented (spec says label manually), but relying on a manual runbook for resource reclamation in already-running environments is fragile. Recommend a one-time startup backfill that stamps the instance label onto namespaces this instance already owns (or an explicit, tested migration step) so orphans predating the change remain reclaimable automatically. Confidence: Medium-High.
[Minor] runSupervised silently stops a component that returns nil before ctx is done.
if err == nil || ctx.Err() != nil { return } means a watch/reconciler Run that returns nil while the context is still live is treated as normal completion: that component stops and is never restarted, and — unlike the old errCh+cancel() model — the process keeps running degraded. Every fn here is expected to block until ctx cancellation, so a nil return before then is really an unexpected exit. Consider restarting (or at least logging WARN) when err == nil && ctx.Err() == nil. Confidence: Medium.
[Minor] Controller liveness/readiness are implicitly coupled to the provisioner being enabled.
The new tcpSocket probes target the provisioner port (9443). If HYPERSHELL_SERVICE_ACCOUNT_PROVISIONER_ADDR is ever empty, the provisioner goroutine is not launched, nothing listens on 9443, and the container crashloops on liveness. The base manifest always sets the address so this is currently consistent, but the coupling is implicit; a comment tying the probe to the provisioner being enabled would prevent a future footgun. Confidence: Medium.
Cross-PR coordination
Two open pull requests require a maintainer decision or a defined merge order with this one:
- #217 makes the same
deploy/openshift/kustomization.yamlchange this PR includes (settingAPI_ENV=development_oidc, addingPOD_NAMESPACE, and the cluster-local provisioner address). This is a duplicate solution to the same OpenShift+Keycloak JWT/401 problem in the same file. Maintainers should decide which one lands: if #217 merges first, this PR must drop that portion of the overlay change; if this PR merges first, #217 becomes redundant and should be closed or reduced. - #194 restructures the same
ReconcileGatewayfunction and the same namespace-creation seam ininternal/gateway/reconciler.go(it moves gateway deployment to the upstream OpenShell Helm chart and keepscreateNamespace), and it also handles the OpenShift SCC binding in the gateway namespace. This PR instead removescreateNamespace/namespaceExists/CreateManagedNamespace/NamespaceExistsin favor of an instance-scopedEnsureManagedNamespace, and reworks the SCC RBAC. These are competing designs over the same reconciler ownership boundary. A decision is needed on merge order and on how the new instance-ownership/labeling model integrates into the Helm-based deploy path, since whichever merges second will have to re-implement its gateway-reconciler changes on top of the other.
Findings Summary (ordered by severity, highest first)
- [Major] GC no longer reaps orphaned namespaces that predate the instance label; no automatic backfill - Reconciliation / Migration (reconciler/namespace.go L46-L55, gateway/namespace.go L111)
- [Minor]
runSupervisedsilently stops a component on a nil return before ctx cancel - Reliability (main.go L58) - [Minor] Liveness/readiness probes implicitly assume the provisioner is enabled - Observability (deploy/base/controller.yaml L63)
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 error messages | Pass |
| Input validated (namespace/DNS labels) | Pass |
| SecurityContext / SCC on pod specs | Pass |
| Reconcile (update-or-create) pattern | Pass |
| Image references consistent across manifests | Pass |
| Proper context propagation | Pass |
| Test Diff Scrutiny (modified assertions justified) | See Major |
Add openshift-* targets for supporting an OpenShift development workflow similar to how kind-* Makefile targets work. This will allow users to deploy onto a target OpenShift cluster. Relates to HYPERSHELL-44 Assisted-by: Cursor Grok 4.6
Assisted-by: Cursor Grok 4.6 Signed-off-by: Kyle Squizzato <kysquizz@redhat.com>
A shared hypershell-e2e ClusterRole would let one environment's overlay patch break every other e2e env. Prefix ClusterRoles and ClusterRoleBindings per project instead, and delete only this env's objects on teardown. Assisted-by: Cursor Grok 4.6 Signed-off-by: Kyle Squizzato <kysquizz@redhat.com>
…write POST /fleets 404s on current main, so seed the same top-level resources kind-up does. Rewrite hypershell-system only as a DNS label so hyphenated image names stay intact while in-cluster DNS still maps. Assisted-by: Cursor Grok 4.6 Signed-off-by: Kyle Squizzato <kysquizz@redhat.com>
…nts creation Assisted-by: Cursor Grok 4.6 Signed-off-by: Kyle Squizzato <kysquizz@redhat.com>
Periodic GC listed every hypershell-managed namespace on the cluster, so an e2e controller treated stage gateways as orphans. Stamp hypershell.redhat.io/instance from the controller pod's own namespace and only reap namespaces that carry that identity. Assisted-by: Cursor Grok 4.6 Signed-off-by: Kyle Squizzato <kysquizz@redhat.com>
…dev auth Periodic GC backfills hypershell.redhat.io/instance on leftover unlabeled openshell-* namespaces so missed-delete orphans are not invisible. OpenShift JWT now comes from the overlay (API_ENV=development_oidc), and a Forbidden prefixed ClusterRole falls back to binding the existing hypershell-controller. Assisted-by: Cursor Grok 4.6
Signed-off-by: Kyle Squizzato <kysquizz@redhat.com>
Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Kyle Squizzato <kysquizz@redhat.com>
Signed-off-by: Kyle Squizzato <kysquizz@redhat.com>
Assisted-by: Claude Sonnet 5 <noreply@anthrophic.com> Signed-off-by: Kyle Squizzato <kysquizz@redhat.com>
Fail closed during teardown so the lifecycle cannot delete an unrelated selected OpenShift project. Assisted-by: GPT-5.6 Terra Signed-off-by: Kyle Squizzato <kysquizz@redhat.com>
e322023 to
b121cea
Compare
…aces The namespace GC selector requires hypershell.redhat.io/instance, but a live gateway in a steady-state phase (Running/Provisioning/Degraded) is skipped by the reconciler's phase gate on restart, so EnsureManagedNamespace never runs and its legacy namespace never gains the label. Add a one-shot startup backfill, driven from this instance's Gateway inventory, that stamps the instance label onto namespaces it already owns -- never creating a namespace, never touching one lacking both management labels, and never overwriting a foreign instance's label, so it stays safe on a shared cluster. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Kyle Squizzato <kysquizz@redhat.com>
b121cea to
869021d
Compare
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This PR delivers an OpenShift local-dev lifecycle plus a well-tested control-plane change that makes gateway-namespace ownership and GC instance-scoped (via hypershell.redhat.io/instance = HYPERSHELL_NAMESPACE) with a one-shot startup backfill for legacy namespaces. The engineering quality is high — proper error wrapping, no panics, aggregated per-namespace failures, refusal to act without an instance identity, a graceful supervisor/wg.Wait shutdown, and thorough table-driven tests — but there is one deliberate behavioral narrowing of GC and a couple of manifest/RBAC scope points worth a maintainer decision.
Strengths
BackfillInstanceLabels/BackfillInstanceLabelare DB-driven, idempotent, and never claim namespaces lacking the two management labels or already carrying a foreign instance label — safe on shared clusters.EnsureManagedNamespacereplaces the old create-or-skip + ad-hoc label-update paths with a single reconcile that refuses to adopt a foreign instance's namespace and errors on an empty identity.supervisor.Run+sync.WaitGroupconverts the previous "first goroutine error exits the process" model into per-component restart-with-backoff plus a clean drain on shutdown. Good defensive improvement.- Test coverage is genuinely strong: foreign-instance, unlabeled-legacy, empty-identity, create-race, and no-op paths are all asserted.
Findings
[Major] Periodic GC no longer reaps untracked/legacy orphan gateway namespaces — IsGatewayNamespaceForGC now requires this instance's label, and reconcileOnce selects only on it. The prior behavior explicitly kept pre-existing orphaned gateway namespaces GC-eligible by name prefix "without a label migration." The startup backfill closes the gap only for namespaces whose Gateway row still exists in this instance's API server; a namespace orphaned before it was ever labeled (missed delete + no Gateway row, both generic management labels, no instance label) is now invisible to the sweep and requires manual cleanup. This is a documented, deliberate trade-off for shared-cluster safety, and it is the correct safety posture — but it removes an automated guarantee that could matter for environments that already hold such orphans. Please confirm the trade-off and ensure operator/runbook guidance exists for the manual-cleanup residue. (See components/control-plane/internal/gateway/namespace.go, components/control-plane/internal/reconciler/namespace.go.)
[Minor] Privileged-SCC bind grant moved from the OpenShift overlay into the base controller ClusterRole — deploy/base/controller-rbac.yaml now grants bind on system:openshift:scc:privileged to the controller in deploy/base, so every deployment (including Kind) carries it. On non-OpenShift clusters the target ClusterRole is absent, so the grant is inert, but this widens the base RBAC surface for a privileged-SCC capability that was previously OpenShift-only. Confirm this is intentional; otherwise keep it in the OpenShift overlay.
[Minor] Controller liveness/readiness/startup probes are hard-coupled to the optional provisioner port (9443) — deploy/base/controller.yaml adds tcpSocket probes on provisioner. As the inline comment concedes, if HYPERSHELL_SERVICE_ACCOUNT_PROVISIONER_BIND_ADDRESS is ever empty the provisioner does not bind and the controller crash-loops on probe failure. This is a latent foot-gun in the base manifest; consider gating the probes with the provisioner feature or documenting the override requirement more prominently than a YAML comment.
Cross-PR coordination
The following require maintainer coordination or a design decision:
-
#242 (managed-cluster pull model,
cluster_idfiltering): #242 changes the package-levellistAllGatewayssignature tolistAllGateways(ctx, client, clusterID), while this PR's newbackfill.gocalls the existing two-argumentlistAllGateways(ctx, client). Whichever merges second breaks compilation of the other. More substantively, the two PRs introduce two different scoping/ownership keys for "which gateways/namespaces does this control plane own" — this PR keys ownership on the instance label (HYPERSHELL_NAMESPACE), #242 keys the gateway inventory onHYPERSHELL_CLUSTER_ID. The GC "live set" and the startup backfill both derive from the gateway list, so maintainers need to decide how instance-label ownership andcluster_idfiltering compose (e.g. whether the backfill/GC list must also becluster_id-scoped) and set a merge order. -
#217 (
development_oidcOpenShift JWT fix): This PR includes the samedeploy/openshift/kustomization.yamlfix as #217 (identicalAPI_ENV=development_oidcblock and near-verbatim rationale comment), so they are duplicate solutions to the same 401 bug. They also diverge on the provisioner dial target: this PR rewrites it to the cluster-local FQDNhypershell-controller.$(POD_NAMESPACE).svc.cluster.local:9443(arguing grpc-go's resolver ignores kube-DNS search domains), whereas #217 keeps the short namehypershell-controller:9443. Maintainers must pick one address and decide which PR carries the overlay fix / merge order to avoid one clobbering the other. -
#200 (control-plane reconciliation contract): Both PRs edit
specs/platform/openshell-gateway-namespace-gc.spec.mdwith conflicting intent. #200 specifies the periodic GC as a "safety net for legacy, force-deleted, or otherwise untracked namespaces" that reaps orphans with no active/deleting Gateway; this PR narrows the periodic GC so untracked/legacy (unlabeled) namespaces are explicitly not reaped and require manual cleanup. Maintainers must reconcile whether the periodic sweep remains the safety net for untracked legacy namespaces or becomes strictly instance-scoped.
Findings Summary (ordered by severity, highest first)
- [Major] Periodic GC no longer reaps untracked/legacy orphan gateway namespaces; only backfilled/labeled ones are swept — Spec Consistency / Test Diff Scrutiny
- [Minor] Privileged-SCC
bindgrant widened from OpenShift overlay into base controller ClusterRole — Security / RBAC - [Minor] Controller probes hard-coupled to the optional provisioner port; provisioner-disabled overlays crash-loop — Reliability / Manifests
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 / refuses empty instance identity | Pass |
| Reconcile pattern (not create-or-skip) | Pass |
| Status/error paths propagate; partial failures aggregated | Pass |
| Proper context propagation | Pass |
| SecurityContext on pod specs | Pass |
| Restricted RBAC scope | Review |
| Test Diff Scrutiny (modified assertions justified + fallback) | Review |
| Conventional commit messages | Pass |
| // owned by a different instance, or lacking this instance's identity label, are | ||
| // never eligible: another HyperShell's live gateways would otherwise look | ||
| // orphaned because they are absent from this instance's API server. | ||
| func IsGatewayNamespaceForGC(ns *corev1.Namespace, instance string) bool { |
There was a problem hiding this comment.
[Major] Removed guarantee: legacy/untracked orphan gateway namespaces are no longer GC-eligible.
IsGatewayNamespaceForGC now requires this instance's label, so a gateway namespace that carries only the two generic management labels (no instance label) is excluded from the periodic sweep. The previous implementation deliberately kept such pre-existing orphans reapable by name prefix "without a label migration."
The startup BackfillInstanceLabels recovers only namespaces whose Gateway row still exists in this instance's API server. A namespace orphaned before it was ever labeled (missed delete + no Gateway row) is now invisible to GC and leaks until manually cleaned up.
This is a sound shared-cluster safety posture and it is documented, but it is an automated-guarantee removal for environments that may already hold such orphans. Please confirm the trade-off and ensure runbook/manual-cleanup guidance exists.
There was a problem hiding this comment.
We need to manually set labels on some legacy stuff, there's no way to do this in a happy way that doesn't result in deleted gateways and we're in a place where we don't have a ton of gateways so we can label them manually and get away with it.
| - apiGroups: ["rbac.authorization.k8s.io"] | ||
| resources: ["clusterroles"] | ||
| resourceNames: ["system:openshift:scc:privileged"] | ||
| verbs: ["bind"] |
There was a problem hiding this comment.
[Minor] Privileged-SCC bind moved into the base controller ClusterRole.
This bind on system:openshift:scc:privileged previously lived in the OpenShift overlay (deploy/openshift/scc.yaml). Placing it in deploy/base means every deployment (including Kind) carries the grant. It is inert where the target ClusterRole is absent, but it widens the base RBAC surface for a privileged-SCC capability. Confirm this is intentional; otherwise keep it overlay-scoped.
| # If HYPERSHELL_SERVICE_ACCOUNT_PROVISIONER_BIND_ADDRESS is ever empty, | ||
| # the provisioner is disabled, nothing binds port 9443, and the controller will fail probes. | ||
| # Overlays that disable the provisioner must also override or remove these probes. | ||
| startupProbe: |
There was a problem hiding this comment.
[Minor] Probes hard-coupled to the optional provisioner port.
As the inline comment concedes, if HYPERSHELL_SERVICE_ACCOUNT_PROVISIONER_BIND_ADDRESS is empty, nothing binds 9443 and the controller crash-loops on these tcpSocket probes. Encoding that foot-gun in the base manifest via a comment is fragile — consider gating the probes with the provisioner feature, or making the coupling impossible to miss for overlays that disable it.
2 participants
What
Implement OpenShift as a development platform against Kind. Users can target a kubernetes context to a desired OpenShift cluster and run
make openshift-up/down/swapwith ephemeral namespace isolation, shared cluster-scoped RBAC, and component swaps via the internal registry.Highlights
scripts/cluster/): Kind wraps existingscripts/kind/with thin dispatcher; new OpenShift driver handles ephemeral lifecyclemake openshift-up: Deploys to ephemeral namespace group (OPENSHIFT_NAMESPACE+${name}-keycloak), validates shared Gateway, seeds Fleet/ManagedCluster/GatewayRelease/ManagedDatabase/Gateway, prints Routesmake openshift-down/openshift-teardown: Deletes environment projects (or HyperShell resources if forbidden); refuses foreign namespaces and reserved names; labels optionalmake openshift-api-server-upetc): Build, push immutable commit+namespace image to internal registry, per-namespace.openshift-swaps/tracking, preserved across reconcile${OPENSHIFT_NAMESPACE}-keycloak): Separate lifecycle, cross-namespace NetworkPolicy allows platform pods to reach JWKS/Admin APIhypershell-e2e, per-environment prefixed ClusterRoleBindings; fails fast if missing and cannot be createdScope
Local-dev lifecycle complete (
make openshift-up/down/status, component swaps). Out of scope: E2E driver completion, CI automation,pr-testconsolidation, environment access handoff, overlay drift CI. Seespecs/platform/openshift-development.spec.mdandskills/RECONCILE.mdfor coverage.