[HYPERSHELL-259] feat: reconcile gateway version for CLI installation - #210
[HYPERSHELL-259] feat: reconcile gateway version for CLI installation#210jsell-rh wants to merge 14 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
jsell-rh
changed the title
Amber reviewStatus: Complete VerdictThis is a clean, well-tested web-console change that adds a gateway-version-matched OpenShell CLI install command, reorders the Connection tab so installation precedes the combined registration + provider block, and removes the service-account shortcut - all reflected in the spec and covered by additive tests. I recommend it for merge after two Minor cleanups; the main action item is cross-PR coordination with #208, which restructures the same Connection tab and still consumes the pre-rename Amber Analysis The refactor of Findings[Minor] Translator description contradicts the rendered order - Content / i18n [Minor] Cross-PR coordinationI reviewed the other open PRs in
No material conflict with the other open PRs. The dashboard PRs (#209/#214) and the deps/i18n PRs touch Findings Summary (ordered by severity, highest first)
Convention Checklist
|
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This is a clean, well-tested web-console change that adds a gateway-version-matched OpenShell CLI install command, reorders the Connection tab so installation precedes the combined registration + provider block, and removes the service-account shortcut - all reflected in the spec and covered by additive tests. I recommend it for merge after two Minor cleanups; the main action item is cross-PR coordination with #208, which restructures the same Connection tab and still consumes the pre-rename EditableCommand/buildSetupScript API this PR replaces.
Amber Analysis
The refactor of EditableCommand -> CommandBlock (making marker/label/value props optional so a static command can reuse the same highlighted, copyable block) is a good consolidation, and the user-supplied endpoint is correctly passed through shellArgument() before it lands in the generated command (with a test that injects a space). Test diff scrutiny came back clean: the modified assertions track real behavior changes (prereq copy, step ordering, service-account link removal) and are all mirrored in the spec and PR description, not silent contract removals - and the endpoint-undefined readiness case that moved out of gateway-connection-steps.test.tsx is still covered by the unit test in gateway-connections.test.ts.
Findings
[Minor] Translator description contradicts the rendered order - Content / i18n
connectionInstallPrereq's description says the note is "shown after gateway registration and before provider setup" (messages.ts:102, en.json:156), but the component renders the prerequisite alert (and its install command) before the combined registration+provider block, and the test asserts exactly that (installationIndex < registrationIndex). The description gives translators the wrong placement context; align it with the actual "before the one-time setup commands" order the spec mandates.
[Minor] install.sh is fetched from main while the CLI version is pinned - Supply chain (Confidence: Low)
buildOpenShellInstallCommand pins OPENSHELL_VERSION to the gateway's reported version but pulls the installer itself from .../NVIDIA/OpenShell/main/install.sh (gateway-connections.ts:80-81,104). A future breaking change to install.sh on main could desync from the pinned version. This matches the command verbatim added to architecture.spec.md in this same PR, so it is intentional - flagging only so maintainers consciously accept pulling the installer from an unpinned ref.
Cross-PR coordination
I reviewed the other open PRs in openshift-online/hypershell (#216, #214, #212, #211, #209, #208, #207, #206, #201, #200, #194, #189, #188, #185, #182, #179, #151, #150, #148, #135, #109, #75, #73). One material conflict:
- #208 "[HYPERSHELL-129] feat(web-console): Adding section to gateway UI with instructions for sandbox connecting" - This is a genuine design/interface conflict, not just a file-merge overlap. Both PRs restructure the same Connection tab and edit the same shared files (
editable-command.tsx,gateway-connection-steps.tsx,gateway-connections.ts,gateway-connection-steps.test.tsx,gateway-connections.test.ts,messages.ts,locales/en.json). Critically, #210 renames the shared exportsEditableCommand -> CommandBlockandbuildSetupScript -> buildOneTimeSetupScript, while #208 still imports and uses the oldEditableCommandandbuildSetupScriptto add its "Connect to a sandbox" step. Whichever merges first, the other must rebase onto the renamed component API and the reordered step layout - and both are inserting/reordering children inside the sameGatewayConnectionStepsstep list. Maintainers should decide a merge order and have the later PR adopt this PR'sCommandBlock/buildOneTimeSetupScriptAPI rather than reintroducing the old names.
No material conflict with the other open PRs. The dashboard PRs (#209/#214) and the deps/i18n PRs touch locales/en.json/messages.ts only additively (no competing design), #206 (hsctl login) is a separate Go CLI and does not compete with this UI's "register the gateway" copy change, and #216 (OpenShift Route console) changes how the control plane publishes the gateway endpoint but keeps the same endpoint field this UI consumes.
Findings Summary (ordered by severity, highest first)
- [Minor] Translator
descriptionforconnectionInstallPrereqsays "after gateway registration" but the alert renders before registration - Content / i18n (messages.ts L102, en.json L156) - [Minor] Installer pulled from
NVIDIA/OpenShell/main/install.sh(unpinned) while CLI version is pinned - Supply chain (gateway-connections.ts L80-81, L104)
Convention Checklist
| Convention | Result |
|---|---|
| Input validated / shell arguments quoted | Pass |
| No secrets in logs or responses | Pass |
| Reuse shared/PatternFly UI components (no duplicate component) | Pass |
| Config separate from code (i18n messages, URLs) | Pass |
| Test Diff Scrutiny (modified assertions justified, no hidden contract removal) | Pass |
| Spec updated to match behavior | Pass |
| Conventional commit messages | Pass |
Amber reviewStatus: Complete VerdictThis is a clean, well-scoped web-console change: the version-matched OpenShell install command is shell-escaped, the spec and i18n catalog are updated in lockstep, and the tests exercise ordering, copy behaviour, and the not-ready gating. My only substantive questions are about the install command's own bootstrapping assumptions and one cross-PR coordination point on the shared connection-tab components. Highlights
Findings[Minor] Install command depends on [Minor] Duplicated instruction text. Cross-PR coordinationAnother open pull request adds a new "Connect to a sandbox" step to the same Convention Checklist
Findings Summary (ordered by severity, highest first):
|
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This is a clean, well-scoped web-console change: the version-matched OpenShell install command is shell-escaped, the spec and i18n catalog are updated in lockstep, and the tests exercise ordering, copy behaviour, and the not-ready gating. My only substantive questions are about the install command's own bootstrapping assumptions and one cross-PR coordination point on the shared connection-tab components.
Highlights
buildOpenShellInstallCommandreusesshellArgument()so the endpoint is single-quoted when it contains anything outside the safe charset, and the command substitution re-tokenizes correctly. Injection surface is covered by the "quotes the gateway endpoint" test.- Readiness gating is consistent: both the install command and the setup script return
undefinedunlessphase === "Running"and an endpoint exists, and the UI renders nothing rather than a partial command. - Test-diff scrutiny: the renamed "hides the installation prerequisite until the gateway is ready" test now keeps
endpointpopulated whilephaseisProvisioning. This is a stronger assertion (matches the spec's "give a Provisioning gateway an endpoint" case), and the endpoint-absent path is still covered ingateway-connections.test.ts, so no guarantee was dropped.
Findings
[Minor] Install command depends on openshell (and jq) already being present. The command computes OPENSHELL_GATEWAY_VERSION by invoking openshell ... status | jq -r '.version' — i.e. it requires the CLI it is meant to install, plus an undocumented jq dependency. The alert is titled "Prerequisite" and the copy says "Install the OpenShell CLI version for this gateway," which reads as first-install guidance to a user who has no CLI yet. If the intent is "re-install / pin to the gateway's version for someone who already has openshell," a one-line clarification in the prerequisite text (and/or noting the jq requirement) would prevent a confusing failure. Please confirm the assumed starting state. (gateway-connections.ts) Confidence: Medium.
[Minor] Duplicated instruction text. connectionInstallPrereq and connectionSetupDescription both now open with "Install the OpenShell CLI version for this gateway…". The repetition is harmless but slightly noisy in the rendered step; consider letting the setup description focus on the register/provider/model sequence. (messages.ts) Confidence: High.
Cross-PR coordination
Another open pull request adds a new "Connect to a sandbox" step to the same Connection tab and builds directly on the current shared exports — it renders its new command through EditableCommand and imports buildSetupScript from gateway-connections.ts. This PR renames those exact symbols (EditableCommand → CommandBlock, buildSetupScript → buildOneTimeSetupScript) and reworks the same gateway-connection-steps.tsx, its test, and the .shiki-count assertions. This is a competing interface change plus a shared-composition change, not just file overlap: whichever merges first forces the other to be rewritten against the new names and to reconcile the expected number of command blocks on the tab. Maintainers should decide a merge order and have the second PR rebase onto the renamed API (and agree on the final ordering of install / setup / create-sandbox / connect-sandbox steps). PR #208.
Convention Checklist
| Convention | Result |
|---|---|
| Input validated / shell arguments escaped | Pass |
| No secrets in logs or responses | Pass |
| Config separate from code (URLs as constants) | Pass |
| i18n catalog matches message definitions | Pass |
| Spec updated to match behavior | Pass |
| Test Diff Scrutiny (no silently removed guarantees) | Pass |
Findings Summary (ordered by severity, highest first):
- [Minor] Install command assumes
openshell/jqalready installed while framed as a first-install prerequisite - UX / Logic (gateway-connections.ts) - [Minor] Duplicated "Install the OpenShell CLI version…" text across two adjacent messages - Content (messages.ts)
jsell-rh
changed the title
Amber reviewStatus: Complete VerdictREQUEST_CHANGES This is a well-structured, well-tested feature: the new |
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, well-tested feature: the new gateway_version field is written through a dedicated atomic API path that a whole-row replace cannot clobber, health/version work is ordered and bounded, and the health Service/NetworkPolicy are reconciled with update-or-create semantics. The one blocking concern is that moving the health loop from serial to concurrent workers now shares a single Keycloak client whose token read is not fully lock-protected, which is a data race in production reconciler code.
Findings
Major
Concurrent workers share a Keycloak client with an unsynchronized token read — components/control-plane/internal/reconciler/health.go
reconcileGateways (health.go:200) now fans work out across up to four concurrent workers (runGatewayWorkers), and each worker's residual-absence path can call the single shared consoleClientChecker (ConsoleClientExists). The reconciler previously ran serially, and the removed comment even said so ("The health loop is serial, so a single shared client needs no additional synchronization"). The replacement comment (health.go:72) now claims "The client protects its shared token state for concurrent use" — but that is not fully true. keycloak.Client.ensureToken writes c.token under c.mu, while doRequestRaw reads c.token to build the Authorization header outside the lock. With two workers hitting a token-refresh boundary, one worker's unlocked read of c.token races the other worker's locked write — a data race (undefined behavior) that can produce a torn/partial admin token.
The PR's -race validation did not catch this because the reconciler tests use a fake ConsoleClientChecker, so the real client's concurrency path is never exercised under the race detector.
Fix options: snapshot the token inside ensureToken and return it (so the caller never reads c.token unlocked), or guard the token read in doRequestRaw with c.mu. Either way, please correct the health.go comment so it does not assert a guarantee the client does not currently provide.
Confidence: High.
Minor
Observed version is logged without sanitizing embedded control characters — components/control-plane/internal/reconciler/gateway_version.go
observedVersion comes from the gateway's own /health response (a semi-trusted, potentially attacker-influenced source if a gateway is compromised) and is only strings.TrimSpaced — that trims the ends but leaves embedded \n/\r intact. It is then logged verbatim (gateway_version.go:62, plus the WARN lines). security.spec.md calls for log-injection sanitization on values that flow into logs. Consider rejecting versions containing control characters, or stripping \r/\n before logging. The length bound (128) and shellArgument quoting in the console already limit blast radius, hence Minor.
Confidence: Medium.
Cross-PR coordination
Two open pull requests have material overlap that maintainers must resolve.
-
#194 shifts gateway deployment away from static YAML manifests to the upstream OpenShell Helm chart and records an explicit "no NetworkPolicies" decision, replacing the same files and code paths this PR builds on (
manifests/gateway/service.yaml,manifests/gateway/networkpolicy.yaml,internal/gateway/manifests.go,internal/gateway/reconciler.go,internal/reconciler/health.go). This PR depends on those static manifests to ship the newopenshell-gateway-healthService and the controller-onlyopenshell-gateway-allow-controller-healthNetworkPolicy, and adds aCONTROL_PLANE_NAMESPACE_PLACEHOLDERsubstitution in that manifest path. Maintainers must decide which deployment mechanism owns the health Service, whether the controller-health NetworkPolicy survives the no-NetworkPolicies decision, how the controller reaches the gateway health port under the Helm model, and the merge order of the two efforts. -
#208 adds a new step to the same shared
GatewayConnectionStepscomponent and new command builders inpackages/gateway-management-ui/src/gateways/gateway-connections.ts, and imports/uses the exportedbuildSetupScript. This PR renames that export tobuildOneTimeSetupScriptand restructures the same connection-steps workflow. This is a competing interface/ownership change, not just a text merge: whoever merges second must adapt to the renamed export and reconcile the competing step layout. The owners should agree on the shared component structure and a merge order.
Findings Summary (ordered by severity, highest first)
- [Major] Concurrent health workers share a Keycloak client whose token read is unlocked, creating a data race (and a comment that overstates the safety guarantee) - Concurrency / Control Plane (health.go:72, health.go:200)
- [Minor] Observed gateway version logged without control-character sanitization - Security (log injection) (gateway_version.go:62)
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 | Pass |
| Log injection prevented | Fail |
| SecurityContext on new pod specs | N/A (no new pod specs) |
| Reconcile (update-or-create) pattern used | Pass |
| Status/owned-field updates on error paths | Pass |
| Proper context propagation (bounded timeouts, stream ctx) | Pass |
| Concurrency safety of shared state | Fail |
| Image references consistent across manifests | Pass |
| OpenAPI/proto client generated, not hand-edited | Pass |
| DB migration registered + nullable/back-compatible | Pass |
| Test Diff Scrutiny (no silently flipped assertions) | Pass |
|
|
||
| // consoleClientChecker is one long-lived Keycloak client for residual-absence | ||
| // checks. It keeps its token cache between passes. It is nil when Keycloak is | ||
| // not configured. The client protects its shared token state for concurrent |
There was a problem hiding this comment.
[Major] Concurrency: shared Keycloak client token read is not lock-protected.
This comment now asserts the console client "protects its shared token state for concurrent use," but with the switch to bounded concurrent workers (reconcileGateways/runGatewayWorkers) that guarantee does not hold. keycloak.Client.ensureToken writes c.token under c.mu, while doRequestRaw reads c.token to build the Authorization header outside the lock. Two workers hitting a token-refresh boundary race read-vs-write on c.token (undefined behavior).
The reconciler was previously serial (the prior comment said as much). Please either snapshot the token inside ensureToken and return it, or guard the token read in doRequestRaw with c.mu, and correct this comment. Note the -race run did not exercise this because the tests use a fake ConsoleClientChecker.
| log.Printf("WARN gateway version: store runtime version for %s: %v", gatewayID, err) | ||
| return | ||
| } | ||
| log.Printf("INFO gateway version: %s runtime version set to %s", gatewayID, observedVersion) |
There was a problem hiding this comment.
[Minor] Log injection: sanitize the observed version before logging.
observedVersion comes from the gateway /health response and is only strings.TrimSpaced, so embedded \r/\n survive and are logged verbatim here (and in the WARN lines above). security.spec.md calls for log-injection sanitization on externally-sourced values. Consider rejecting versions containing control characters, or stripping \r/\n before logging.
|
Follow-up stacked on this PR: #219 makes the e2e test install the openshell CLI via the console-recommended command, deriving the version from the |
Amber reviewStatus: Stopped The pull request head changed before Amber posted the review. A later job can review the new head. |
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 careful, well-structured feature: the runtime gateway_version is written through a dedicated field-owned atomic gRPC path, the newly concurrent health loop is correctly guarded (map access under mu, Keycloak token reads now return a locked snapshot), and version observation is bounded, redirect-rejecting, and size/control-char validated. I found no blocking convention or security issues; the notes below are maintainability-level, and the Cross-PR section flags coordination that needs a maintainer decision.
What I verified
- Field ownership is airtight.
Replacenow omits bothActiveSandboxCountandGatewayVersion, andSetGatewayVersionusesUPDATE ... WHERE gateway_version IS DISTINCT FROM ? RETURNING, emitting the outbox event only on an actual change. The gRPC integration test proves a whole-row update cannot clobber the reconciled version. Good, consistent with the sandbox-count precedent. - Concurrency is safe. The new bounded worker pool (
gatewayHealthWorkerCount = 4) keeps per-gateway work serial; allrouteNotReadySince/routeTornDown/routeVerifiedAtaccess goes throughh.mu; and the KeycloakensureTokenchange returns the token under lock instead of readingc.tokenafter unlock — that closes the data race the new concurrency would otherwise open. - Version observation is defensive. Bounded timeout,
CheckRedirect→ErrUseLastResponse(3xx then rejected by the status check),LimitReader+ explicit oversize check, empty/control-char/length validation. The observed value is validated before it reaches a log line, so no log-injection vector. - Migration + generated artifacts are consistent.
migrationAddGatewayVersionis registered inplugin.go; proto (field 23 +SetGatewayVersionRPC), OpenAPI (read_only), model, presenters, SDKs, and the data-model/health specs all line up with the source-of-truth definitions. - Test diff scrutiny. All modified test hunks are additive (new
GatewayVersionfields/assertions, new worker tests). No pre-existing assertion was flipped from accept→reject or optional→required, and the new field is nullable with no backfill requirement, so no removed-guarantee concern.
Findings
[Minor] Dual ownership of the health Service/NetworkPolicy — The health Service and NetworkPolicy are created both by the provisioning manifest path (service.yaml / networkpolicy.yaml via ApplyManifestToNamespace) and by ReconcileGatewayHealthAccess. The specs match today, so they converge, but two independent writers of the same object will silently fight on every interval if either definition drifts. Consider a single source of truth (e.g. have the reconciler own it and drop the manifest copy, or derive both from one spec) to prevent future divergence. Confidence: High.
[Minor] Gateway namespace is interpolated into the health URL without an explicit validity check — httpGatewayVersionObserver.Observe builds http://openshell-gateway-health.<namespace>.svc.cluster.local:... from the gateway's stored namespace. The value is control-plane-derived (low risk), but a defensive DNS-label check before URL construction would harden this against a malformed namespace ever reaching the request. Confidence: Medium.
Cross-PR coordination
A stacked test PR must merge after this one and re-implements this PR's install-command version derivation (trim → drop the first '-' and everything after → ensure a leading 'v') as a shell helper that mirrors buildOpenShellInstallCommand. The two derivations must be kept in sync, and the merge order must be honored; maintainers should confirm both before merging.
Another open PR adds a new "Connect to a sandbox" section to the same gateway Connection tab and edits the same shared files this PR restructures (gateway-connection-steps.tsx, the command builders in gateway-connections.ts, and the messages.ts / en.json catalogs, including this PR's buildSetupScript → buildOneTimeSetupScript rename). These are overlapping designs for the same UI surface, not a plain merge conflict; the owners need to agree on the tab layout, the message keys, and a merge order so one does not silently drop the other's section.
Another open PR proposes replacing the static gateway manifests with the upstream OpenShell Helm chart and explicitly decides against provisioning NetworkPolicies at all. This PR's health-access design depends on a static Service + NetworkPolicy manifest (plus the CONTROL_PLANE_NAMESPACE_PLACEHOLDER substitution) and on controller→gateway:8081 ingress being allowed. Maintainers must decide how the gateway health endpoint is exposed and access-controlled under that direction so the two approaches do not collide.
Findings Summary (ordered by severity, highest first)
- [Minor] Health Service/NetworkPolicy have two independent writers (provisioning manifests + reconciler) that can fight if definitions drift — Reconciliation / Maintainability
- [Minor] Gateway namespace interpolated into the health URL without an explicit DNS-label check — Input Validation
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound / not-found handled |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (version value, response size) | Pass |
| Reconcile (update-or-create) pattern used | Pass |
| Status/field updates on error paths preserve last value | Pass |
| Concurrency guarded (mutex, token snapshot) | Pass |
| Image/manifest references consistent across stack | Pass |
| OpenAPI/proto generated, not hand-edited | Pass |
| Migration registered | Pass |
| Conventional commit messages | Pass |
| Test diff scrutiny (no flipped assertions) | Pass |
| // ReconcileGatewayHealthAccess makes the internal gateway health endpoint | ||
| // available to the control plane. It changes only the dedicated Service and | ||
| // NetworkPolicy that this reconciler owns. | ||
| func ReconcileGatewayHealthAccess(ctx context.Context, clientset kubernetes.Interface, namespace, controlPlaneNamespace string, skipNetworkPolicies bool) error { |
There was a problem hiding this comment.
[Minor] Dual writers for the health Service/NetworkPolicy. These objects are also created by the provisioning manifest path (service.yaml / networkpolicy.yaml applied through ApplyManifestToNamespace). ReconcileGatewayHealthAccess reconciles the same names on every interval. The specs match today so they converge, but if either definition drifts the two writers will overwrite each other every pass. Consider a single source of truth (reconciler-owned, or both derived from one spec).
| }, | ||
| }, | ||
| timeout: defaultGatewayVersionTimeout, | ||
| endpoint: func(namespace string) string { |
There was a problem hiding this comment.
[Minor] Namespace goes straight into the health URL. http://openshell-gateway-health.<namespace>.svc.cluster.local:... is built from the gateway's stored namespace. It is control-plane-derived so risk is low, but a defensive K8s DNS-label validation before constructing the request would harden this against a malformed namespace ever reaching the URL.
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 feature: the runtime version is observed from the gateway /health endpoint, stored through a dedicated field-owned gRPC write that a whole-row Replace cannot clobber, and the new concurrent health loop is correctly guarded (route-timer maps behind mu, Keycloak token returned as a locked snapshot). I found no blockers; the notes below are minor, plus one deploy change that is out of scope for this feature and overlaps a dedicated PR.
Strengths
SetGatewayVersionis atomic (UPDATE ... WHERE gateway_version IS DISTINCT FROM ?) and emits the update event only on a real change;Replacenow omitsGatewayVersionalongsideActiveSandboxCount, andgrpc_integration_test.goproves a whole-row update cannot overwrite the reconciled version. Good use of the transactional-outbox pattern.- The move to 4 bounded workers is race-safe: the shared route-state maps are all accessed under
h.mu, andkeycloak.Client.ensureTokennow returns a token snapshot instead of letting callers readc.tokenunlocked. - Version observation is defensively bounded: 3s timeout, redirects rejected (
http.ErrUseLastResponse), response body size-limited, control characters and over-long values rejected, and observation failures are logged without perturbingphase/status. - Spec, data-model, OpenAPI, proto, SDKs, and web console are all updated consistently, and the read-only field is documented as control-plane-owned.
Minor findings
-
Per-tick, per-gateway health-access reconcile (
components/control-plane/internal/reconciler/gateway_version.go:43) -ReconcileGatewayHealthAccessruns for every ready gateway on every tick, issuing a Service+NetworkPolicyGet(and possibleUpdate) each pass. This is bounded and correct, but the owned resources rarely drift; consider reconciling them less frequently (e.g. only on create/first-observe or on a longer cadence) to reduce steady-state API traffic across a large fleet. Confidence: Medium. -
Unrelated deploy/auth change bundled into this feature PR (
deploy/openshift/kustomization.yaml:80-81) - addingAPI_ENV=development_oidcand restructuring the env patch to append is an OpenShift JWT-auth fix, not part of gateway-version reconciliation. It also duplicates a dedicated PR (see Cross-PR coordination). Consider dropping it here so the feature PR stays focused. Confidence: High. -
Sandbox NetworkPolicy no longer allows port 8081 (
components/control-plane/internal/gateway/reconciler.go,openshell-gateway-allow-sandbox-v2) - health access on 8081 is now restricted to the controller-only policy. The spec was updated to match (sandboxes need only gRPC 8080), so this looks intentional; please confirm no sandbox workload relies on reaching the gateway health port. Confidence: Medium.
Cross-PR coordination
- The dedicated OpenShift JWT-auth fix (
API_ENV=development_oidc) that this PR adds in its final commit todeploy/openshift/kustomization.yamlis the entire subject of another open pull request that changes the same file for the same purpose. This is a duplicate/competing solution: maintainers must decide which PR owns the deploy-env change and coordinate merge order, because the second to merge will conflict and carry a redundant fix. - Another open pull request adds a new "Connect to a sandbox" section to the same gateway Connection-tab components this PR restructures. This PR renames the shared
EditableCommandcomponent toCommandBlock(making its props optional) and renamesbuildSetupScripttobuildOneTimeSetupScriptingateway-connections.ts. That is a competing interface change to shared UI code the other PR builds on; whichever merges second must be updated to the renamed component/API, so the owners need to agree on the final component surface and a merge order. - Another open pull request is stacked on this branch and asserts, in an e2e test, the exact console-recommended install command that this PR's
buildOpenShellInstallCommandproduces (including the--suffix stripping) and consumes the new read-onlygateway_versionfield. It must merge after this PR, and any change reviewers request to the install-command format here has to be mirrored there; coordinate the command contract and merge order.
Findings Summary (ordered by severity, highest first)
- [Minor] Unrelated OpenShift JWT-auth deploy change bundled into a version-reconcile feature PR - Scope / Cross-PR (deploy/openshift/kustomization.yaml:80)
- [Minor] Per-tick, per-gateway reconcile of owned health Service/NetworkPolicy adds steady-state API traffic - Performance (gateway_version.go:43)
- [Minor] Confirm no sandbox relies on gateway port 8081 after tightening the sandbox NetworkPolicy - Spec Consistency (reconciler.go)
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 (version length/control chars, required ID) | Pass |
| Reconcile pattern used (update-or-create) | Pass |
| Status/field updated only on change; error paths logged | Pass |
| Concurrency safe (shared maps + Keycloak token guarded) | Pass |
OpenAPI client not manually edited (make generate) |
Pass |
| Image references consistent across manifests | Pass |
| Conventional commit messages | Pass |
| Test diff scrutiny (assertions additive, no flipped guarantees) | Pass |
| return | ||
| } | ||
|
|
||
| if err := gateway.ReconcileGatewayHealthAccess(ctx, h.clientset, namespace, h.controlPlaneNamespace, h.skipNetworkPolicies); err != nil { |
There was a problem hiding this comment.
[Minor] Performance: ReconcileGatewayHealthAccess runs for every ready gateway on every health tick, issuing a Service + NetworkPolicy Get (and a possible Update) each pass. It is bounded and correct, but these owned resources rarely drift. Consider reconciling them on first observation or on a longer cadence to reduce steady-state K8s API traffic across a large fleet. Confidence: Medium.
| - name: RBAC_SERVICE_ACCOUNTS | ||
| value: "service-account-hypershell-control-plane" | ||
| name: API_ENV | ||
| value: "development_oidc" |
There was a problem hiding this comment.
[Minor] Scope / Cross-PR: Adding API_ENV=development_oidc and switching the env patch to append-mode is an OpenShift JWT-auth fix unrelated to gateway-version reconciliation, and it duplicates a separate open PR that changes this same file for the same purpose. Recommend dropping it from this feature PR and coordinating with the dedicated deploy-env PR on ownership and merge order. Confidence: High.
2 participants
Jira: HYPERSHELL-259
Summary
gateway_versionAPI field through an atomic field-owned write.jq.-and all following text from the installer version. For example, usev0.0.109when the gateway reportsv0.0.109-rh9a8f8.mainbranch.Validation
make checkgo test ./...incomponents/control-planego test -race ./internal/gateway ./internal/reconcilerincomponents/control-planego test ./...incomponents/sdk-gopnpm --filter @openshift-online/hypershell-gateway-management-ui checkpnpm --filter @openshift-online/hypershell-sdk checkThe API integration harness currently stops before the tests because its shared migration setup reports
pq: got 2 parameters but the statement requires 1. The focused API compile check and an isolated PostgreSQL test for the guarded version update pass.