[HYPERSHELL-240] feat(ci): ephemeral PR environments for OpenShift e2e - #267
Merged
Merged
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: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Collaborator
Amber reviewStatus: Complete |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Collaborator
Amber reviewStatus: Complete |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Collaborator
Amber reviewStatus: Complete |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Collaborator
Amber reviewStatus: Complete |
HyperShell environment updated to commit
|
| Fact | Value |
|---|---|
| Namespaces | Platform: hypershell-ci-pr-267 Keycloak: hypershell-ci-pr-267-keycloak |
| OpenShift console | https://console-openshift-console.apps.rosa.hysh-aws-01.c6uk.p3.openshiftapps.com |
| API | https://hypershell-api-hypershell-ci-pr-267.apps.rosa.hysh-aws-01.c6uk.p3.openshiftapps.com |
| Web console | https://hypershell-web-console-hypershell-ci-pr-267.apps.rosa.hysh-aws-01.c6uk.p3.openshiftapps.com |
Log in through the web console with your GitHub account (you must be a member of
the configured organization or on its allowlist). The environment is time-boxed
and refreshed on every new commit.
CLI access
oc login --server=https://api.hysh-aws-01.c6uk.p3.openshiftapps.com:443 --web
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Collaborator
Amber reviewStatus: Complete |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Collaborator
Amber reviewStatus: Failed The review stopped at 2026-09-10T22:52:38Z. A later job can retry this commit. |
Collaborator
Amber reviewStatus: Complete |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Collaborator
Amber reviewStatus: Complete |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Collaborator
Amber reviewStatus: Complete |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Collaborator
Amber reviewStatus: Complete |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
squizzi
force-pushed
the
squizzi/reconcile-ephemeral-openshift-e2e-ci
branch
from
September 11, 2026 18:35
4579607 to
4bcbf9f
Compare
Collaborator
Amber reviewStatus: Complete |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Collaborator
Amber reviewStatus: Complete |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Collaborator
Amber reviewStatus: Complete |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Collaborator
Amber reviewStatus: Complete |
Keycloak 26 standard token-exchange rejects requested_subject, which broke OpenShift e2e impersonation. Enable the legacy token-exchange feature, turn off the standard-exchange client switch, and send a client-credentials subject_token before impersonating. Assisted-by: Cursor Grok 4.6 Co-authored-by: Cursor <cursoragent@cursor.com>
Area 4 failed with "Client not allowed to exchange" because Keycloak 26 legacy token-exchange needs FGAP v1 permissions on the target client. Enable admin-fine-grained-authz:v1 and have the control plane grant hypershell-e2e exchange onto gateway and frontend clients. Also register the GitHub hardcoded-role mapper under the Keycloak 26 provider id, store the broker GitHub token for the org gate, and stop printing admin/admin in the OpenShift banner. Assisted-by: Cursor Grok 4.6 Co-authored-by: Cursor <cursoragent@cursor.com>
The BFF org gate denied openshift-online members because Keycloak 26 serves the stored GitHub token as form-urlencoded with a JSON content type, and hypershell-frontend omitted the roles scope so the access token never carried broker.read-token. Parse form-encoded broker bodies regardless of content type, include roles on the frontend client, and store GitHub tokens as JSON. Assisted-by: Cursor Grok 4.6 Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Kyle Squizzato <kysquizz@redhat.com>
Public membership is visible without an org-approved OAuth App, so the
gate checks /orgs/{org}/public_members/{user} first. Private membership
uses /user/memberships/orgs/{org} and still fail-closes on 403. Helmet
hashes the denied-page inline styles so CSP style-src 'self' does not
blank the page.
Assisted-by: Cursor Grok 4.6
Co-authored-by: Cursor <cursoragent@cursor.com>
Kind already waits on plan-images and honors should_run. OpenShift ran on every origin PR, so docs-only changes still waited on Deploy PR environment and could fail Tests CI Gate. Gate the suite the same way; PR Environment deploy stays unconditional. Assisted-by: Cursor Grok 4.6 Co-authored-by: Cursor <cursoragent@cursor.com>
Gateway provisioning drove gateways to Failed on every e2e job because EnsureE2ETokenExchange could not grant token-exchange against a fresh per-PR Keycloak realm, and the create-client path rolls the client back on that failure, looping create -> 404 -> delete -> retry. Two coupled issues: - findE2EClientPolicy treated Keycloak's policy search-by-name 404 as fatal. Keycloak returns 404 (not an empty 200/204) when no policy of that name exists yet, so the first reconcile always failed. Treat 404 as "not found" and let the caller create the policy. - The policy search/create ran before any client's management permissions were enabled. Enabling FGAP is what lazily initializes realm-management's authorization resource server, without which the policy endpoints 404 outright. Enable permissions on the target (and frontend) client first, then search/create/attach. Split attachTokenExchangePolicy into enableTokenExchangePermissions plus attachPolicyToPermission so enabling happens once, up front. The test fake now returns 404 for a missing policy to reproduce real Keycloak and lock in the regression. Assisted-by: Claude Opus 4.8
Assisted-by: Cursor Grok 4.6 Signed-off-by: Kyle Squizzato <kysquizz@redhat.com>
Ubuntu mawk does not treat escaped parentheses the same way macOS awk does, so the seed_via_api range never matched and CI failed the json_named_id assertion. Switch the checks to fixed-string grep. Assisted-by: Cursor Grok 4.6 Signed-off-by: Kyle Squizzato <kysquizz@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
PR environments can lose API seed rows when openshift-up recreates the database and fails before openshift-seed. Discovery now explains empty vs Error list bodies, pins local-openshift/dev-release, and re-runs platform seed once when both lists are empty. Developer and platform-admin gateway create checks follow the API server's RBAC_DEFAULT_ROLES: Kind's unset default still expects 201; OpenShift's empty value expects 403. openshift-down and the PR-env reaper also delete control-plane-managed sibling namespaces labeled with the platform instance so leftover openshell-* projects do not outlive the environment. Assisted-by: Cursor Grok 4.6 Signed-off-by: Kyle Squizzato <kysquizz@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
oc set env without -c only patches spec.containers, so HYPERSHELL_CONSOLE_HOST never reached render-realm-config. A later start-dev --import-realm then restored localhost URIs and Keycloak rejected the BFF callback. Stamp the host with a strategic-merge patch of the init container; a full-object replace races Deployment status. Skip the oauth-secret recycle when the annotation already matches. A later reconcile also keeps the existing PR access-fact table instead of replacing it with the first-deploy placeholder. Assisted-by: Cursor Grok 4.6 Signed-off-by: Kyle Squizzato <kysquizz@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Kyle Squizzato <kysquizz@redhat.com>
Signed-off-by: Kyle Squizzato <kysquizz@redhat.com>
TestListenAndServeServesPlaintextProvisionerCalls dials the server from a goroutine racing ListenAndServe's own net.Listen call, so gRPC's default dial can fail fast on "connection refused" before the listener is bound under CI scheduling delay. Add grpc.WaitForReady(true) so the call retries until callCtx's deadline instead. Unrelated to the Keycloak realm fix in daf7b8c; this test failure was pre-existing and reproduced 10/10 flaky under load, 0/20 after the fix. Signed-off-by: Kyle Squizzato <kysquizz@redhat.com>
Keycloak runs start-dev on in-memory H2 with no persistent volume, so any Keycloak pod restart discards dev-gateway's dynamically-provisioned OIDC client while its row survives untouched in PostgreSQL. Reusing that stale dev-gateway permanently strands it in status "Keycloak client is missing": the GatewayReconciler deliberately never auto-recreates a missing client, since doing so without also restoring RoleBindings and console mappers would leave it silently half-provisioned. Until Keycloak has durable storage across restarts, seed_via_api now deletes an existing dev-gateway and creates a fresh one on every run instead of reusing it. Other named seed resources (ManagedCluster, GatewayRelease, ManagedDatabase) are unaffected and continue to be reused. Updates openshift-development.spec.md and ephemeral-pr-environments.spec.md to reflect the dev-gateway exception, and adds shell assertions in lib_test.sh. Signed-off-by: Kyle Squizzato <kysquizz@redhat.com>
Amber requested changes because hypershell-e2e shipped to every imported realm and token-exchange ran on production gateway reconcile. Omit that client, its service account, and the impersonation mapping unless enabled, and skip EnsureE2ETokenExchange unless the client is enabled. Admit Keycloak sessions with no GitHub broker identity when the org gate is on so seeded password users still work. Parse oc image info JSON instead of jsonpath for the digest fallback. Assisted-by: Cursor Grok 4.6 Co-authored-by: Cursor <cursoragent@cursor.com>
The org gate treated a 403 (never linked to GitHub, e.g. seeded password users) and a 404 (linked to GitHub but Keycloak has no stored token) from Keycloak's broker token endpoint as the same "not linked" case and admitted both. A 404 actually indicates a verifiable GitHub identity we failed to verify, so treating it like an unlinked account let a linked-but-unreadable session skip the org check entirely. 404 now throws instead of returning "not linked", routing through the existing lookup-error handling so the session is denied and the failure is logged. 403 keeps admitting, since that only occurs for accounts that never went through the GitHub broker. Addresses review feedback on PR #267. Assisted-by: Claude Sonnet 5
Web console quality gates flagged a line over the printWidth in the test file added by the previous fail-closed-on-404 commit. Assisted-by: Claude Sonnet 5
CI's OpenShift e2e run failed acquiring a per-gateway OIDC token with the openshell-admin role: the controller assigned the role at 16:15:56, two seconds after the 120s poll deadline expired at 16:15:54. The controller pod had been recycled mid-run and needed to reconnect its watch streams and replay its reconcile queue before it got to the role grant, which ordinary reconcile latency on a shared ROSA cluster only barely fits inside 120s. OpenShift reconciles measurably slower than Kind across the board, so raise every short reconcile-wait default (role-sync polling, controller rollout waits, gateway provisioning, sandbox creation, namespace GC, runtime version) from 90-180s to a uniform 300s. These are ceilings, not sleeps, so passing runs are unaffected. Assisted-by: Claude Sonnet 5
You were right that both e2e failures were races, not flakes or stale deploys. This is the second one: UserDao.Upsert did a SELECT to check for an existing username, then Create or Save depending on the result. Two concurrent requests JIT-provisioning the same brand-new identity (e.g. the developer user's first-ever call to the HyperShell API) could both miss the SELECT, both attempt Create, and the loser would hit the username unique-constraint violation. UserProvisioningMiddleware treats that error as "leave userID unset" rather than retrying, so the losing request's RBAC check saw an empty userID and 403'd -- exactly the "Developer user: gateway list returned HTTP 403" failure, and why a solo manual retry always passed (no concurrent racer, or the row already existed from a prior run). Replaced the SELECT-then-Create/Save with a single INSERT ... ON CONFLICT (username) DO UPDATE, which Postgres resolves atomically. GORM does not reliably scan the post-conflict row back into a struct with a client-generated (non-autoincrement) primary key, so the loser could still get back its own locally-generated KSUID instead of the row that actually persisted; re-read by username after the upsert so every caller resolves to the one canonical row. Added TestUserUpsert_ConcurrentFirstTimeProvisioning, which fires 8 concurrent upserts for the same new username and asserts they all succeed and resolve to the same ID. It reproduced the bug reliably against the old implementation and passes against the fix. Assisted-by: Claude Sonnet 5
squizzi
force-pushed
the
squizzi/reconcile-ephemeral-openshift-e2e-ci
branch
from
September 15, 2026 18:30
8d72c9b to
08b1a60
Compare
CI's golangci-lint flagged SA5011 on a combined "verbosity == nil || vmodule == nil" check guarding two later pointer dereferences: staticcheck's flow analysis doesn't reliably carry non-nil for both pointers past a single ||-joined t.Fatal guard, only past individual ones. Splitting into two separate nil checks (each with its own t.Fatal) resolves the false positive with no behavior change. Assisted-by: Claude Sonnet 5
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The previous fix (splitting the compound nil check into two ifs) still tripped SA5011 in CI on the exact pinned golangci-lint version (v2.12.2) even though it was clean locally against the same version -- staticcheck's flow analysis apparently still doesn't treat two sequential single-pointer nil-check-then-Fatal blocks in the same function as clearing both pointers before their later dereferences. Moved the nil-check-and-fail into its own mustLookupFlag helper, so each pointer's nil check and only use of that pointer sit in a function boundary of their own. This is the standard, more robust pattern for this SA5011 false-positive class. Verified against the CI-pinned golangci-lint v2.12.2 binary directly (not just whatever version happened to be installed locally), including a linux/amd64 cross-compiled run to rule out a platform-specific difference. Assisted-by: Claude Sonnet 5
amber-review-bot
left a comment
amber-review-bot
left a comment
Collaborator
There was a problem hiding this comment.
Verdict
COMMENT - The only change since the last review (db1b033a -> 2d45f33) is a test-only refactor in components/api-server/cmd/hypershell/main_test.go: the two inline flag.Lookup nil checks are factored into a mustLookupFlag(t, name) helper to satisfy staticcheck SA5011. The guarantee is unchanged (a missing v/vmodule flag still t.Fatalfs), so no prior blocker/major is affected; all previously addressed findings still hold at this head and two items need cross-PR maintainer coordination.
Summary
This head adds only mustLookupFlag in main_test.go; it is a staticcheck-driven extraction of an existing nil check, not a weakened assertion (a Test Diff Scrutiny pass). The substantive PR (ephemeral OpenShift PR environments, GitHub IdP + BFF org gate, e2e token-exchange, reaper CronJob, race-free user upsert) was reviewed at db1b033a and re-verified unchanged here.
Cross-PR coordination
Two material coordination items require a maintainer decision.
-
Contradictory user-inventory authorization contract (PR #279). This PR and #279 both edit
components/api-server/pkg/rbac/authorization.go,authorization_test.go, andplugins/users/integration_test.go, and they encode incompatible authorization models. This PR keepsTestUserList_AllowedForHypershellAdminasserting that the legacyhypershell-adminsrealm role lists the user inventory (200). #279 renames that exact test toTestUserList_ForbiddenForHypershellAdminWithoutPlatformAdmin(403) and re-points the other user-list tests toroles.RolePlatformAdmin, deliberately requiring effectiveplatform:adminand dropping legacyhypershell-adminsalone. Independently, #279 changes theNewRBACAuthzMiddleware(lookup, config)signature to add anactivityRecorderargument, while this PR adds a new test (TestAuthorizeApiAllowsUserWithNoBindingsToListGateways) that calls the two-argument form and adds a new gateways-collection-GET allow rule to the sameisAuthorizedfunction. This is a design decision (does legacyhypershell-adminsalone still authorize the user inventory, and what is the middleware constructor signature?), not a text merge - whichever lands second must not silently re-loosen or re-tighten the contract or leave the constructor/tests inconsistent. Maintainers must pick the model and reconcile the shared files/tests. (Both PRs also make the same trivial staticcheck split incmd/hypershell/main_test.go; noted only so the duplicate is not mistaken for a semantic clash.) -
Management API JWT audience enforcement (PR #182). #182 flips the OpenShift API-server deployment to validate
issand requireaud=hypershell-frontendon management calls (addsJWT_ISSUER/--jwt-audiencetodeploy/openshift/kustomization.yaml) and rejects tokens not minted for the management audience. This PR's OpenShift PR-env authenticates via thehypershell-e2eclient_credentialsgrant and relies on control-planeEnsureE2ETokenExchangeto mint tokens onto thehypershell-frontendaudience for management calls. They agree on the audience name, so this is an ordering/assumption dependency rather than a contradiction: whoever merges second must confirm (a) this PR's PR-env API-server carries the same--jwt-audience/--jwt-issuervalues #182 introduces, and (b) #182's reject-non-management-audience rule does not reject the exchanged token this PR's e2e flow depends on. Confirm the shared audience/issuer contract and merge order explicitly rather than by coincidence.
Previous concerns
All prior Amber findings were re-checked against the code at 2d45f33. Every file behind a previously addressed finding is unchanged from the last-reviewed db1b033a (only main_test.go moved), so the earlier fixes stand.
- [Addressed] Per-run e2e secret desync from imported realm (r3982129640) - workflow reuses the existing
e2e-client-secret, generating only on first boot. - [Addressed] Digest pinning silently degraded to a mutable tag / skopeo absent / registry auth (r3983888857) -
resolve_by_digesthard-fails for builton-pr-<sha>images; only baseline tags may fall back per the spec last-resort clause (scripts/ci/swap-openshift-images-by-digest.sh). - [Addressed] Privileged
hypershell-e2eclient + well-known default secret enabled everywhere (r3984253723) -"enabled": "${HYPERSHELL_E2E_CLIENT_ENABLED:false}"with an empty default secret (deploy/base/keycloak/keycloak.yamlL394). - [Addressed] e2e client/service-account/impersonation shipped to non-PR realms (r4006939520) -
render-realm-config.pystrips them unlessHYPERSHELL_E2E_CLIENT_ENABLED=true. - [Addressed] Token-exchange skip keyed on presence, not enablement (r4006939532) -
EnsureE2ETokenExchangereturns early onif !e2e.Enabled(components/control-plane/internal/keycloak/client.goL449). - [Addressed]
addReadTokenRoleOnCreatemissing -> broker-token 403 lockout (r3992501011) - settrueon the GitHub IdP (keycloak.yamlL765). - [Addressed] Init-container / reaper hardening (seccomp + requests/limits) (r3983888858, r3984253728) -
runAsNonRoot+seccompProfile: RuntimeDefaultand requests/limits present. - [Addressed] Secret to
GITHUB_OUTPUT, deadplan-imagesearly-exit,oc logintargeting the app Route,(admin/admin)hint, broker-token parser,read:orgcomment (r3982252488, r3983570371, r3983708258, r3993396469, r3993396477, r3992501031) - all confirmed fixed in the current tree. - [Addressed] Shell unit tests not wired to a CI gate (r3982252493) - resolved on
main:make ci-test->scripts/run-shell-unit-tests.shauto-discovers*_test.sh, invoked by.github/workflows/unit-tests.yml; answered in the existing thread. - [Still present - accepted design, on record] Hardcoded-role IdP mappers grant
platform:admin+gateway:creatorto every brokered GitHub login; org/allowlist enforcement lives only in the web-console BFF (r3993624648). A token minted directly from Keycloak bypasses the BFF gate. Accepted by the maintainer as a prod-aligned trade-off for the disposable PR realm and acknowledged in the existing thread; keep Keycloak-layer enforcement a hard prerequisite before this IdP is reused on any shared/long-lived realm. Not blocking. - [Still present - by design for ephemeral realms] Fail-open for sessions with no readable GitHub identity (r4016758519). A 403 from
/broker/github/tokenadmits the session; correct for the ephemeral realm (fresh H2 users +addReadTokenRoleOnCreate), residual risk only on a re-used realm. Recorded. - [Still present - accepted trade-off] Reaper cluster-wide
delete(r3993736446). The guard is the unit-testedpr_env_is_reapablepredicate; treat predicate/reserved-name edits as security-critical. - [Still present - minor UX trade-off] BFF org gate fail-closes on transient GitHub failures (r3992110589). Fail-closed is the correct default; denied-legit-member on a transient blip is the acknowledged residual.
- [Cannot fully verify from static review] Python builder image on the boot-critical realm-render path (r3992501015). Digest-pinned and consistent with the repo's other Dockerfiles; the Kind Keycloak-boot smoke test remains the gate.
- [Still present - Minor, low confidence]
Upsertconfirmation re-read returns withoutMarkForRollback(r4019249781).dao.gois unchanged at this head: theTakere-read (L101-102) still returns its error withoutdb.MarkForRollback(ctx, err), so a committed write can diverge from an error response. Self-healing on retry given the idempotent upsert; see the existing thread.
Findings Summary
No new finding at 2d45f33 (the sole change is the mustLookupFlag test-helper extraction, which preserves the assertion).
- [Minor]
Upsertre-read failure after a successfulINSERT ... ON CONFLICTreturns the error withoutdb.MarkForRollback(ctx, err), so a committed write and the error response can diverge - Data Integrity (plugins/users/dao.goL101-102). Low confidence; self-healing on retry. Tracked in the existing thread (r4019249781).
Prior blockers/majors are all addressed; remaining items are accepted-by-design residuals already on record with the maintainer.
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
| No secrets in logs / step outputs | Pass |
| SecurityContext + resources on pod specs | Pass |
| Reconcile / upsert is race-free (atomic ON CONFLICT) | Pass |
| Image references digest-pinned / fail-closed swap | Pass |
| Test diff scrutiny (helper extraction preserves the nil-flag guarantee) | Pass |
markturansky
pushed a commit
that referenced
this pull request
Sep 15, 2026
#267) * feat(ci): add ephemeral PR environments for OpenShift e2e Deploy a full HyperShell stack per pull request on the shared OpenShift e2e cluster so PRs can be exercised end to end before merge, instead of relying solely on Kind. Adds the pr-environment workflow, supporting CI scripts for provisioning/stamping/commenting on PR environments and swapping in PR-built image digests, and a namespace reaper CronJob plus RBAC that garbage-collects expired PR environments on a schedule. The reaper's ose-cli image is pinned to a digest (rather than :latest) to satisfy the repo's dependency-pinning policy enforced by `make check`. Assisted-by: Claude Sonnet 5 * fix(ci): render realm placeholders in an init container, not Keycloak Keycloak's declarative --import-realm does not perform ${VAR:default} substitution on imported realm JSON (upstream keycloak#20199, keycloak#12069). The ephemeral PR environment realm relied on that substitution for six placeholders, including a boolean field (identityProviders[0].enabled). Since Keycloak never resolves it, the literal string "${PR_ENV_GITHUB_IDP_ENABLED:false}" gets fed to a boolean field during JSON deserialization, which is a fatal type mismatch that crashes the JVM on every boot (CrashLoopBackOff). Add a render-realm-config init container that reads the keycloak-realm ConfigMap, resolves the placeholders itself from the same Secret-backed env vars (JSON-escaping string values, emitting a real true/false for the boolean), validates the result with json.loads, and writes it to an emptyDir that the keycloak container imports from instead of mounting the ConfigMap directly. The GitHub OAuth env vars move from the keycloak container to the init container, since that's where the substitution now happens. Assisted-by: Claude Sonnet 5 * fix(ci): shorten hypershell-e2e client description under DB column limit Keycloak's CLIENT.DESCRIPTION column is VARCHAR(255). The hypershell-e2e client's description was 531 characters, so the realm import's UPDATE CLIENT statement failed with "Value too long for column", crashing Keycloak's boot right after the previous placeholder-substitution fix started working (confirmed via the container's boot log on the live PR-267 OpenShift environment). Trimmed the description to 86 characters and moved the full rationale into a YAML comment above the ConfigMap, since comments there are not persisted to Keycloak's database and have no length limit. Validated end to end with `make openshift-up` against the hypershell-ci-pr-267 namespace: Keycloak now boots clean and the GitHub identity provider is enabled with real client credentials. Assisted-by: Claude Sonnet 5 * fix(ci): extend Keycloak's OpenShift rollout timeout to 10m The shared e2e cluster is capacity-constrained: bringing up a new Keycloak pod (e.g. after a ConfigMap or image change) can trigger the cluster autoscaler to provision a whole new node, which alone can take several minutes before the pod is even Scheduled. The previous 180s (3m) rollout timeout, shared with every other OpenShift deployment, was too tight for that and made `make openshift-up` fail even though the pod was healthy and just waiting on capacity. Give wait_for_keycloak its own 600s (10m) timeout instead of raising the shared default, since api-server/controller/web-console/postgres don't hit this autoscaler path. Kind's Keycloak wait stays at its existing 180s/120s timeouts (scripts/kind/up.sh) since Kind doesn't have this cluster-capacity failure mode. Assisted-by: Claude Sonnet 5 * fix(ci): stop using admin/admin test users in GitHub-brokered environments Ephemeral PR environments broker interactive login to GitHub and have no password grant for real users (ephemeral-pr-environments.spec.md), but make openshift-up still seeded platform resources by logging in as the realm's admin/admin test user via a password grant, and its completion banner still advertised admin/admin and developer/developer as if they were the intended login path. Both contradict the PR's own design and would mislead anyone reading the banner on a PR environment. Add github_idp_enabled(), which checks the hypershell-github-oauth Secret's idp-enabled key to detect a brokered environment. When brokered, seed_via_api() now authenticates as the hypershell-e2e service account via client_credentials (it already holds platform:admin + gateway:creator for this purpose) instead of a password grant against admin/admin, and the banner prints that login is GitHub-brokered instead of listing test user credentials. Standard (non-brokered) openshift-up usage is unaffected since it has no hypershell-github-oauth Secret to detect. Validated live against the GitHub-brokered hypershell-ci-pr-267 environment: seeding succeeded via the hypershell-e2e client credentials grant with no password grant attempted, and the banner correctly printed the GitHub-brokered message. Assisted-by: Claude Sonnet 5 * fix(ci): gate OpenShift e2e behind PR Environment deploy, drop token comment The OpenShift e2e suite ran inline inside the "PR Environment" workflow, so a deploy failure and an e2e failure were indistinguishable as a single check. Move it to its own "E2E OpenShift" job in e2e.yml, triggered via workflow_run once "PR Environment" completes, so the two failure modes surface as distinct checks. The new job re-verifies the deploy job itself succeeded (a fork PR's run reports overall success even though its deploy is skipped) and that the PR is still open before testing, since the environment can be superseded or torn down between the deploy run finishing and this job starting. Also switch the PR access comment's CLI snippet from a redacted `--token=<redacted>` template to `oc login --web`, so OpenShift drives the developer's browser through the same GitHub-gated OAuth flow the web console uses instead of requiring a separate out-of-band credential delivery step. Assisted-by: Claude Sonnet 5 * fix(ci): reskin the GitHub social login button on the Keycloak theme The GitHub "sign in" button on the login page rendered as a raw, unstyled PatternFly secondary button: full width, a heavy blue outline, and an oversized icon, because its <svg> ships with no width/height attributes and browsers fall back to a 300x150 default replaced-element size. Reskin it to match PatternFly's tertiary button (transparent fill, rounded pill border) and size the icon explicitly. Getting this right required working around three upstream quirks, each verified against Keycloak 26.7.2's actual login theme source rather than guessed: - Keycloak's base theme applies a CSS filter (invert/sepia/hue-rotate) to recolor the icon's black fill into Keycloak's own blue; it repaints pixels after fill, so it fights any custom icon color and has to be cancelled explicitly. - PatternFly's secondary/tertiary buttons paint their border via a ::after pseudo-element with its own separate border-radius variable (defaulting to a small radius), not the button box's own border-radius, so rounding the box alone left the visible border square. - The provider name ships with PatternFly's pf-v5-u-m-auto utility (margin: auto !important), which absorbs the button's flex free space on its own and pins the icon to the far left regardless of justify-content. Also zero out a doubled bottom padding: the provider list reuses the .pf-v5-c-login__main-body class for its horizontal edge-to-edge padding, but that class also carries the same large PaddingBottom as the real card body wrapping it, leaving a large empty gap under the button. Assisted-by: Claude Sonnet 5 * fix(ci): fail-closed GitHub IdP, pin built images by digest, harden Keycloak Leave the GitHub identity provider disabled until the org-gate authenticator exists, refuse on-pr image swaps that cannot resolve a digest, and match the Keycloak SecurityContext to the restricted container convention. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(ci): close remaining Amber findings except the CI shell-test gate Reuse the first-boot hypershell-e2e secret, disable that admin client outside PR environments, inspect Konflux with the cluster pull secret, and correct the access-comment oc login URL. Reaper RBAC documents its cluster-wide delete blast radius and the CronJob now has resource bounds. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(ci): re-enable the GitHub IdP when OAuth secrets are present PR environments already fail closed if the OAuth App is unset. Leaving idp-enabled=false hid Login with GitHub after realm import. Recycle Keycloak when the oauth secret hash changes so an already-booted realm re-imports the enabled identity provider. Co-authored-by: Cursor <cursoragent@cursor.com> * feat(ci): gate PR-env GitHub login in the BFF Check org membership and allowlist after the OIDC callback so denied users never get a HyperShell session, and thus no API bearer. Kind stays ungated. Post the access comment as a deploying placeholder first so it stays near the top of the PR timeline. Assisted-by: Cursor Grok 4.6 Co-authored-by: Cursor <cursoragent@cursor.com> * ci: move OpenShift e2e under Tests / E2E / OpenShift The suite was nested under PR Environment via workflow_call because GitHub needs: cannot cross workflows. Polling Deploy PR environment from e2e.yml is enough, so deploy and e2e stay distinct checks without burying the suite under the deploy workflow. Assisted-by: Cursor Grok 4.6 Co-authored-by: Cursor <cursoragent@cursor.com> * fix(ci): persist Keycloak console redirects and split skipped jobs Keycloak pod recycle re-imported localhost-only frontend redirect URIs, so the BFF callback failed with Invalid parameter: redirect_uri. Bake the console host into realm render, and mint per-gateway admin tokens via token-exchange instead of client_credentials. Move push-to-main OpenShift e2e and PR-env teardown into dedicated workflows so those jobs do not appear as skipped checks on PRs. Assisted-by: Cursor Grok 4.6 Co-authored-by: Cursor <cursoragent@cursor.com> * fix(ci): use legacy token-exchange for per-gateway e2e tokens Keycloak 26 standard token-exchange rejects requested_subject, which broke OpenShift e2e impersonation. Enable the legacy token-exchange feature, turn off the standard-exchange client switch, and send a client-credentials subject_token before impersonating. Assisted-by: Cursor Grok 4.6 Co-authored-by: Cursor <cursoragent@cursor.com> * fix(ci): grant Keycloak v1 token-exchange for OpenShift e2e Area 4 failed with "Client not allowed to exchange" because Keycloak 26 legacy token-exchange needs FGAP v1 permissions on the target client. Enable admin-fine-grained-authz:v1 and have the control plane grant hypershell-e2e exchange onto gateway and frontend clients. Also register the GitHub hardcoded-role mapper under the Keycloak 26 provider id, store the broker GitHub token for the org gate, and stop printing admin/admin in the OpenShift banner. Assisted-by: Cursor Grok 4.6 Co-authored-by: Cursor <cursoragent@cursor.com> * fix(ci): admit org members through the GitHub broker token The BFF org gate denied openshift-online members because Keycloak 26 serves the stored GitHub token as form-urlencoded with a JSON content type, and hypershell-frontend omitted the roles scope so the access token never carried broker.read-token. Parse form-encoded broker bodies regardless of content type, include roles on the frontend client, and store GitHub tokens as JSON. Assisted-by: Cursor Grok 4.6 Co-authored-by: Cursor <cursoragent@cursor.com> * fix(keycloak): pad the logout button on theme Signed-off-by: Kyle Squizzato <kysquizz@redhat.com> * fix(web-console): admit public GitHub org members without a broker token Public membership is visible without an org-approved OAuth App, so the gate checks /orgs/{org}/public_members/{user} first. Private membership uses /user/memberships/orgs/{org} and still fail-closes on 403. Helmet hashes the denied-page inline styles so CSP style-src 'self' does not blank the page. Assisted-by: Cursor Grok 4.6 Co-authored-by: Cursor <cursoragent@cursor.com> * fix(ci): skip OpenShift e2e when plan-images says not to run Kind already waits on plan-images and honors should_run. OpenShift ran on every origin PR, so docs-only changes still waited on Deploy PR environment and could fail Tests CI Gate. Gate the suite the same way; PR Environment deploy stays unconditional. Assisted-by: Cursor Grok 4.6 Co-authored-by: Cursor <cursoragent@cursor.com> * fix(keycloak): create e2e token-exchange policy on a fresh realm Gateway provisioning drove gateways to Failed on every e2e job because EnsureE2ETokenExchange could not grant token-exchange against a fresh per-PR Keycloak realm, and the create-client path rolls the client back on that failure, looping create -> 404 -> delete -> retry. Two coupled issues: - findE2EClientPolicy treated Keycloak's policy search-by-name 404 as fatal. Keycloak returns 404 (not an empty 200/204) when no policy of that name exists yet, so the first reconcile always failed. Treat 404 as "not found" and let the caller create the policy. - The policy search/create ran before any client's management permissions were enabled. Enabling FGAP is what lazily initializes realm-management's authorization resource server, without which the policy endpoints 404 outright. Enable permissions on the target (and frontend) client first, then search/create/attach. Split attachTokenExchangePolicy into enableTokenExchangePermissions plus attachPolicyToPermission so enabling happens once, up front. The test fake now returns 404 for a missing policy to reproduce real Keycloak and lock in the regression. Assisted-by: Claude Opus 4.8 * fix(scripts): ensure seeded resources are not recreated Assisted-by: Cursor Grok 4.6 Signed-off-by: Kyle Squizzato <kysquizz@redhat.com> * test(scripts): assert seed lookups with portable grep Ubuntu mawk does not treat escaped parentheses the same way macOS awk does, so the seed_via_api range never matched and CI failed the json_named_id assertion. Switch the checks to fixed-string grep. Assisted-by: Cursor Grok 4.6 Signed-off-by: Kyle Squizzato <kysquizz@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> * fix(e2e): recover empty OpenShift seed inventory and honor RBAC defaults PR environments can lose API seed rows when openshift-up recreates the database and fails before openshift-seed. Discovery now explains empty vs Error list bodies, pins local-openshift/dev-release, and re-runs platform seed once when both lists are empty. Developer and platform-admin gateway create checks follow the API server's RBAC_DEFAULT_ROLES: Kind's unset default still expects 201; OpenShift's empty value expects 403. openshift-down and the PR-env reaper also delete control-plane-managed sibling namespaces labeled with the platform instance so leftover openshell-* projects do not outlive the environment. Assisted-by: Cursor Grok 4.6 Signed-off-by: Kyle Squizzato <kysquizz@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> * fix(ci): persist Keycloak console redirects across OpenShift recycle oc set env without -c only patches spec.containers, so HYPERSHELL_CONSOLE_HOST never reached render-realm-config. A later start-dev --import-realm then restored localhost URIs and Keycloak rejected the BFF callback. Stamp the host with a strategic-merge patch of the init container; a full-object replace races Deployment status. Skip the oauth-secret recycle when the annotation already matches. A later reconcile also keeps the existing PR access-fact table instead of replacing it with the first-deploy placeholder. Assisted-by: Cursor Grok 4.6 Signed-off-by: Kyle Squizzato <kysquizz@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> * fix: no more segfault during web-console build on arm64 Signed-off-by: Kyle Squizzato <kysquizz@redhat.com> * fix: E2E token auth within keycloak Signed-off-by: Kyle Squizzato <kysquizz@redhat.com> * fix(control-plane): stop flaky provisioner transport test TestListenAndServeServesPlaintextProvisionerCalls dials the server from a goroutine racing ListenAndServe's own net.Listen call, so gRPC's default dial can fail fast on "connection refused" before the listener is bound under CI scheduling delay. Add grpc.WaitForReady(true) so the call retries until callCtx's deadline instead. Unrelated to the Keycloak realm fix in daf7b8c; this test failure was pre-existing and reproduced 10/10 flaky under load, 0/20 after the fix. Signed-off-by: Kyle Squizzato <kysquizz@redhat.com> * fix(ci): always recreate dev-gateway instead of reusing it Keycloak runs start-dev on in-memory H2 with no persistent volume, so any Keycloak pod restart discards dev-gateway's dynamically-provisioned OIDC client while its row survives untouched in PostgreSQL. Reusing that stale dev-gateway permanently strands it in status "Keycloak client is missing": the GatewayReconciler deliberately never auto-recreates a missing client, since doing so without also restoring RoleBindings and console mappers would leave it silently half-provisioned. Until Keycloak has durable storage across restarts, seed_via_api now deletes an existing dev-gateway and creates a fresh one on every run instead of reusing it. Other named seed resources (ManagedCluster, GatewayRelease, ManagedDatabase) are unaffected and continue to be reused. Updates openshift-development.spec.md and ephemeral-pr-environments.spec.md to reflect the dev-gateway exception, and adds shell assertions in lib_test.sh. Signed-off-by: Kyle Squizzato <kysquizz@redhat.com> * fix(security): keep e2e impersonation out of production realms Amber requested changes because hypershell-e2e shipped to every imported realm and token-exchange ran on production gateway reconcile. Omit that client, its service account, and the impersonation mapping unless enabled, and skip EnsureE2ETokenExchange unless the client is enabled. Admit Keycloak sessions with no GitHub broker identity when the org gate is on so seeded password users still work. Parse oc image info JSON instead of jsonpath for the digest fallback. Assisted-by: Cursor Grok 4.6 Co-authored-by: Cursor <cursoragent@cursor.com> * fix(security): fail closed for unreadable GitHub broker tokens The org gate treated a 403 (never linked to GitHub, e.g. seeded password users) and a 404 (linked to GitHub but Keycloak has no stored token) from Keycloak's broker token endpoint as the same "not linked" case and admitted both. A 404 actually indicates a verifiable GitHub identity we failed to verify, so treating it like an unlinked account let a linked-but-unreadable session skip the org check entirely. 404 now throws instead of returning "not linked", routing through the existing lookup-error handling so the session is denied and the failure is logged. 403 keeps admitting, since that only occurs for accounts that never went through the GitHub broker. Addresses review feedback on PR #267. Assisted-by: Claude Sonnet 5 * style: fix prettier formatting in github-org-gate test Web console quality gates flagged a line over the printWidth in the test file added by the previous fail-closed-on-404 commit. Assisted-by: Claude Sonnet 5 * fix(e2e): raise reconcile-wait timeouts to 300s for OpenShift CI's OpenShift e2e run failed acquiring a per-gateway OIDC token with the openshell-admin role: the controller assigned the role at 16:15:56, two seconds after the 120s poll deadline expired at 16:15:54. The controller pod had been recycled mid-run and needed to reconnect its watch streams and replay its reconcile queue before it got to the role grant, which ordinary reconcile latency on a shared ROSA cluster only barely fits inside 120s. OpenShift reconciles measurably slower than Kind across the board, so raise every short reconcile-wait default (role-sync polling, controller rollout waits, gateway provisioning, sandbox creation, namespace GC, runtime version) from 90-180s to a uniform 300s. These are ceilings, not sleeps, so passing runs are unaffected. Assisted-by: Claude Sonnet 5 * fix(api-server): make user provisioning upsert atomic You were right that both e2e failures were races, not flakes or stale deploys. This is the second one: UserDao.Upsert did a SELECT to check for an existing username, then Create or Save depending on the result. Two concurrent requests JIT-provisioning the same brand-new identity (e.g. the developer user's first-ever call to the HyperShell API) could both miss the SELECT, both attempt Create, and the loser would hit the username unique-constraint violation. UserProvisioningMiddleware treats that error as "leave userID unset" rather than retrying, so the losing request's RBAC check saw an empty userID and 403'd -- exactly the "Developer user: gateway list returned HTTP 403" failure, and why a solo manual retry always passed (no concurrent racer, or the row already existed from a prior run). Replaced the SELECT-then-Create/Save with a single INSERT ... ON CONFLICT (username) DO UPDATE, which Postgres resolves atomically. GORM does not reliably scan the post-conflict row back into a struct with a client-generated (non-autoincrement) primary key, so the loser could still get back its own locally-generated KSUID instead of the row that actually persisted; re-read by username after the upsert so every caller resolves to the one canonical row. Added TestUserUpsert_ConcurrentFirstTimeProvisioning, which fires 8 concurrent upserts for the same new username and asserts they all succeed and resolve to the same ID. It reproduced the bug reliably against the old implementation and passes against the fix. Assisted-by: Claude Sonnet 5 * fix(api-server): split compound nil check to satisfy staticcheck CI's golangci-lint flagged SA5011 on a combined "verbosity == nil || vmodule == nil" check guarding two later pointer dereferences: staticcheck's flow analysis doesn't reliably carry non-nil for both pointers past a single ||-joined t.Fatal guard, only past individual ones. Splitting into two separate nil checks (each with its own t.Fatal) resolves the false positive with no behavior change. Assisted-by: Claude Sonnet 5 * fix(api-server): isolate flag nil-check in a helper for staticcheck The previous fix (splitting the compound nil check into two ifs) still tripped SA5011 in CI on the exact pinned golangci-lint version (v2.12.2) even though it was clean locally against the same version -- staticcheck's flow analysis apparently still doesn't treat two sequential single-pointer nil-check-then-Fatal blocks in the same function as clearing both pointers before their later dereferences. Moved the nil-check-and-fail into its own mustLookupFlag helper, so each pointer's nil check and only use of that pointer sit in a function boundary of their own. This is the standard, more robust pattern for this SA5011 false-positive class. Verified against the CI-pinned golangci-lint v2.12.2 binary directly (not just whatever version happened to be installed locally), including a linux/amd64 cross-compiled run to rule out a platform-specific difference. Assisted-by: Claude Sonnet 5 --------- Signed-off-by: Kyle Squizzato <kysquizz@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Implement ephemeral OpenShift environments for every origin pull request, enabling live testing against the PR's built container images with full e2e validation.
Highlights
tests/e2e/e2e-openshell.sh) withE2E_INFRA_DRIVER=openshiftandE2E_OIDC_GRANT=client_credentials; token auth viahypershell-e2econfidential client (admin path: client-credentials; developer path: Keycloak token-exchange impersonation)Scope
Implemented (W1-W4):
hypershell-e2eclient, org/allowlist attributes, hardcoded-role mappersHandoff (requires human provisioning before merge):
OPENSHIFT_PR_ENV_SERVER_URL,OPENSHIFT_PR_ENV_TOKEN, OAuth client id/secret/callback URL, org gate + allowlistkustomize build --load-restrictor=LoadRestrictionsNone deploy/e2e/reaper | oc apply -f -${VAR:default}placeholders resolve correctly on Kind Keycloak boot (blast radius = realm import)See
skills/RECONCILE.md(ephemeral-pr-environments.spec.md section) for the full handoff checklist.