[HYPERSHELL-259] feat: reconcile gateway version for CLI installation - #210
[HYPERSHELL-259] feat: reconcile gateway version for CLI installation#210jsell-rh wants to merge 9 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: Pro Plus 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 |
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.