Repository navigation
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
HyperShell environment updated to commit
|
| Fact | Value |
|---|---|
| Namespaces | Platform: hypershell-ci-pr-489 Keycloak: hypershell-ci-pr-489-keycloak |
| OpenShift console | https://console-openshift-console.apps.rosa.hysh-aws-01.c6uk.p3.openshiftapps.com |
| API | https://hypershell-api-hypershell-ci-pr-489.apps.rosa.hysh-aws-01.c6uk.p3.openshiftapps.com |
| Web console | https://hypershell-web-console-hypershell-ci-pr-489.apps.rosa.hysh-aws-01.c6uk.p3.openshiftapps.com |
| Keycloak admin console | https://keycloak-hypershell-ci-pr-489-keycloak.apps.rosa.hysh-aws-01.c6uk.p3.openshiftapps.com/admin/hypershell/console/ |
Log in through the web console with your GitHub account (you must be a member of the configured organization or on its allowlist) or with a seeded test-tier username and password. To test as developer or platform-admin, sign in as that user, or open the Keycloak admin console, sign in with GitHub, and impersonate that user.
CLI access
oc login --server=https://api.hysh-aws-01.c6uk.p3.openshiftapps.com:443 --web
Amber reviewStatus: Stopped The pull request head changed before Amber posted the review. A later job can review the new head. |
![hypershell-delivery[bot]](https://avatars.githubusercontent.com/u/327175833?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/pr-extend |
ed7856c to
796d1ed
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Verdict
The new head commit (888666b, "exchange OpenShift gateway tokens in CI") is a clean, well-tested fix that adds a proper token-exchange path for the client_credentials grant and backs it with a focused regression test. However, the Major accuracy issue from the prior review is unchanged: the PR description still claims "zero generated-artifact divergence" while this head carries a breaking SDK type/builder-signature change. The code is correct; the disclosure gap keeps this a COMMENT.
New commit review (888666b)
clientCredentialsToken/gatewayTokenExchange(tests/e2e/driver/openshift.go:238-300) route theclient_credentialsgrant through the e2e service-account token-exchange flow (urn:ietf:params:oauth:grant-type:token-exchange), exchanging a SA token for a gateway-audience token. Form posts go throughpostTokenForm, which never logs theclient_secretor token (openshift.go:409-433);escapeFormURL-encodes viaurl.Values(driver/kind.go:309-315). No secrets leak.- The regression test
TestOpenShiftGatewayTokenUsesExchangeForClientCredentials(tests/e2e/driver/openshift_oidc_test.go) asserts the two-step grant sequence and the exchangeaudience. Thepanic(err)in thetestJWThelper is test-only and acceptable. - Error handling is intact; the polling loop swallows per-attempt errors by design (bounded by
E2E_GATEWAY_TOKEN_TIMEOUT) and returns the last token on success.
Description vs. diff mismatch (Major, unchanged)
The body still states "Zero generated-artifact divergence: openapi, sdk-go, sdk-typescript, and the sdk-generator are byte-identical to main" and that the provisioning-conditions generator fix "was dropped." Verified at this head against main (0f62e8c): components/sdk-go/types/gateway.go re-types ProvisioningConditions string -> []map[string]any and ServerDNSNames string -> []string, components/sdk-go/types/role.go re-types Permissions string -> map[string]any, and the public builders change signature (ServerDNSNames(string) -> ([]string), Permissions(string) -> (map[string]any)). The sdk-generator (scripts/sdk-generator/parser.go, model.go) and the TypeScript SDK also diverge. This is a real, breaking public SDK surface change that the description denies. Either update the description to document the breaking SDK change, or split the generator/SDK change into its own PR as the body itself proposes, so a maintainer does not merge an undisclosed breaking SDK change. This is already tracked in the existing inline thread (#489 (comment)); not re-filed.
Cross-PR coordination
A separate open PR (#379) changes the management-API audience from hypershell-frontend to a dedicated hypershell-api (a declared breaking change to issuer/audience validation), updates the Keycloak audience mappers and deploy overlays, and edits the retained Bash/Python e2e assets (tests/e2e/drivers/kind.sh, tests/e2e/gateway_service_account_test.py) to exercise gateway service-account token-exchange and audience isolation under that new model. This PR migrates the functional e2e suite to the Go harness and, in the new head commit, introduces its own gateway token-exchange implementation (driver/openshift.go gatewayTokenExchange, plus driver/kind_oidc.go). The two PRs make competing/overlapping changes to the same e2e OIDC token-exchange and audience surface under different ownership (Python/Bash vs Go). Maintainers should decide the merge order and which harness owns gateway service-account token-exchange coverage, and confirm that after both land the Go harness's management-API tokens satisfy #379's hypershell-api audience and that #379's audience-isolation test is not stranded in an unexercised harness. This needs a maintainer decision, not an automatic merge.
Previous concerns
- Major - description claims zero generated-artifact divergence despite a breaking SDK builder-signature change: still present. The body still contains the "Zero generated-artifact divergence" claim while
components/sdk-go/types/gateway.go:47,50andcomponents/sdk-go/types/role.go:19carry array/map types and the builders atgateway.go:120,211/role.go:61carry the new signatures. Tracked in the existing inline thread; not re-filed. - Minor - stale "release ids" wording after
release_idremoval: still present.tests/e2e/suite.go:89-90still documentsSetupSuiteas establishing "the seeded cluster and release ids" whilediscoverSeedIDsreturns only the cluster id (suite.go:119-121, loggingseed ids: cluster=%s);tests/e2e/smoke.sh:12and:55still say "cluster/release ids." Tracked in the existing inline thread; not re-filed. - Minor - matrix parallel suites share a mutable driver; GC-timing override races in
longmode: still present.tests/e2e/matrix_test.go:92-116still runs each per-cluster subtest witht.Parallel()sharing onedriver, andSetupSuitestill adjusts GC timing only inlongmode (suite.go:123-129), soE2E_MODE=long make e2e-matrixwould run N parallel GC-timing overrides against the one Deployment. Tracked in the existing inline thread; not re-filed.
Findings Summary (ordered by severity, highest first):
- [Major] PR description claims zero generated-artifact divergence, but the head makes a breaking SDK builder-signature change - Description/Disclosure (gateway.go L47/L120, role.go L19/L61, parser.go L414)
- [Minor] Stale "release ids" wording after
release_idremoval - Docs (suite.go L89-90, smoke.sh L12/L55) - [Minor] Matrix parallel suites share a mutable driver/controller; GC-timing override races in
longmode - Concurrency (matrix_test.go L92-116)
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 |
| OpenAPI/SDK client regenerated, not hand-edited | Pass |
| Generated artifacts consistent with the generator | Pass |
| PR description matches the diff | Fail |
| Test diff scrutiny (modified assertions justified) | Pass |
| Conventional commit message | Pass |
Introduce the Go e2e module (testify + client-go + gateway-api typed clientset)
per specs/platform/e2e-testing.spec.md:
- E2EInfraDriver interface, registry, and KUBECONFIG driver auto-detection
- kind driver: route/console/gateway discovery, OIDC password and
client-credentials grants, Keycloak admin role helpers, namespace-GC timing
- harness: shared kube clients, CommandRunner demo logging, poll/retry,
three-outcome reporter, cluster-CA trust + host rewrite (no insecure bypass)
- apiclient: thin wrapper over the generated sdk-go client + raw request helper
- E2ESuite: ordered phase-step dispatch, mode gating, fail-fast gates, and
phases P0 (auth, env readiness) and P1 (provision, infra verify, token+CA
trust, route discovery)
Verified green on Kind. P1.4/P1.5 CLI registration/connectivity and P2/P3 are
stubbed with honest skips pending later waves. The gateway create body uses
route={enabled:true}; TLS is verified via the cert-manager server secret
(the deployment uses cert-manager Certificates, not a certgen Job).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ustness
Complete the Go e2e suite's remaining phases and the openshell CLI path:
- P1.4/P1.5: register the gateway with the openshell CLI (per-gateway
metadata.json + oidc_token.json) and verify `status` reports Connected,
via the Kind container wrapper (cli.go).
- P2.1 sandbox lifecycle: create via CLI, wait for the pod, exec, delete, and
assert active_sandbox_count tracks create/delete (own gateway for isolation).
- P2.2 RBAC: developer vs platform-admin API boundary (gateway list/create,
delete), honoring the deployment's RBAC_DEFAULT_ROLES.
- P2.3 deletion + namespace GC: delete-driven reap and the periodic reaper of a
seeded synthetic orphan namespace with a GarbageCollected Event.
- P2.4 ManagedCluster lifecycle: registration idempotency, non-registrar 403,
name-collision 409, empty cluster_id 400 (gRPC identity + reconnect noted as
follow-ups).
- P2.5 release promotion: revision-aware rollout with observed_release_id
convergence and last-good preservation on a failed (unpullable) rollout.
- P3.1 admin inventory: /users admin boundary (403, opaque 404) and unknown
phase-write 400.
Supporting:
- driver: AcquireClientCredentialsToken for the registrar identity.
- harness: RunWithEnv for the CLI process, E2E_KUBECONTEXT override so a run can
pin a context (and the CLI wrapper's kubectl follows it via a temp kubeconfig).
- gateway create now sends route={enabled:true} + the gateway OIDC config.
- P0.1 control-plane Unauthenticated check scoped to logs since the run started.
- provisioning-timeout diagnostics (phase, conditions, controller logs).
Validated on Kind: P0.1, P0.2, P1.1-P1.4 green; dev-gateway CLI connect verified.
CLI-dependent steps (P1.5, P2.1) and the P2/P3 steps after them are implemented
but pending end-to-end validation on a clean cluster: the current dev cluster's
cloud-provider-kind data plane went stale after heavy churn and stopped serving
new gateway routes (long-lived gateways still connect).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Validated the full functional suite green on a clean Kind cluster (short: 9/0/4;
long: 13 passed, 0 failed, 0 skipped). Fixes found during that validation:
- P2.1 sandbox create streams during the image pull and can return a stream error
("missing grpc-status") even though the sandbox was created; treat create as
best-effort and use the pod reaching Running as the success signal.
- P2.1 sandbox delete can hit a transient "upstream request timeout"; retry it and
rely on active_sandbox_count returning to 0 as the authoritative signal.
- P2.4 parse the registration response's cluster_id field (not id).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…make targets Wave D of the Go e2e rewrite (all validated on Kind): - matrix_test.go (TestMatrix): runs the single-cluster E2ESuite once per registered ManagedCluster, bounded by E2E_CONCURRENCY, with a per-cluster pass/fail/skip matrix and a single fail-closed verdict (stale/unregistered clusters fail unless E2E_MANAGED_SKIP_UNHEALTHY=1). Defaults E2E_MODE=short. Validated: local-kind green. - perf_test.go (TestPerformance): batched, bounded-concurrency gateway scale-up with create/time-to-Running latency percentiles, a results JSON, fleet teardown, and optional SLO gating (E2E_PERF_MIN_SUCCESS_RATE / E2E_PERF_MAX_PROVISION_P99). Validated: 3/3 provisioned, 100%. - smoke.sh (make e2e-smoke): Bash happy-path walkthrough (token -> gateway -> CLI register/connect -> sandbox create/exec/delete -> gateway delete) that echoes each command and honors E2E_PAUSE; not the gate. Validated: 5/5. - Makefile: make e2e / e2e-performance now invoke the Go suite (go test), plus new e2e-matrix and e2e-smoke targets. Also: suite gained a cluster-id override + name suffix for the matrix runner, and the sandbox name is now a short bounded token (the 19-char sandbox-name limit was exceeded under the matrix's longer run id). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ixes
driver/openshift.go implements E2EInfraDriver against OpenShift: host discovery
from Routes (route.openshift.io via the dynamic client), OIDC issuer derived from
the Keycloak Route in ${OPENSHIFT_NAMESPACE}-keycloak, system-root TLS (ROSA
Routes serve valid certs, so no CA injection or host rewrite), the password/
client-credentials grants, Keycloak admin role helpers, and controller GC-timing
patching. Registered under "openshift"; auto-detected when route.openshift.io is
served. Validated end-to-end on an OpenShift cluster: long mode 13/13.
Fixes surfaced while bringing OpenShift up (all keep Kind green):
- P0.2 environment readiness now verifies platform dependencies by served API
group (gateway.networking.k8s.io, cert-manager.io, agents.x-k8s.io) instead of
Kind-pinned deployment namespaces, so it is portable across Kind and OpenShift.
- The admin token is refreshed at the start of each phase step, so a long run
whose earlier steps exceed the token TTL no longer 401s later steps.
- The OpenShift CLI path runs the gateway-version-matched openshell CLI image via
the container engine (the Homebrew CLI can skew the sandbox protobuf against the
downstream gateway); the gateway is reached over public DNS.
- harness.Clients exposes ContextNamespace() (OpenShift default platform namespace).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…perf suites, rewire CI The Go E2ESuite + benchmark are now the functional and performance gates, so the repository no longer carries two functional e2e implementations. Removed (replaced by the Go suite): - tests/e2e/e2e-openshell.sh (Bash functional suite) - tests/e2e/e2e-performance.sh + tests/e2e/perf/ (Bash performance suite) - scripts/perf-report.sh (replaced by Go benchmark output + benchstat) - the e2e-openshell cleanup-trap assertions in openshift_driver_test.sh Retained (per the team's call): the full Bash drivers tests/e2e/drivers/*.sh and tests/e2e/lib.sh, which the retained browser (e2e-console.sh) and smoke (smoke.sh) suites keep sourcing. (Divergence from the spec's "remove drivers/*.sh + slim lib.sh": keeping the full drivers avoids reimplementing lost infra selection for the browser suite.) Performance harness reframed as a true Go benchmark: - BenchmarkGatewayProvisioning (perf_test.go) replaces TestPerformance. It reports ttr p50/p99/max, throughput (gw/min), and success% via b.ReportMetric (native benchmark output + benchstat comparison), still writes the promotion results JSON, and SLO-gates. make e2e-performance runs it with -benchtime 1x (one-shot fleet scale test). CI + tooling: - .github/workflows/e2e.yml: the Kind and OpenShift jobs now run `make e2e` (go test TestE2E) instead of the Bash suite; the browser suite is unchanged. - Makefile: lint-e2e target (gofmt + go vet + golangci-lint) wired into lint, so the e2e module is lint-covered; e2e-performance-report removed. - CLAUDE.md / DEVELOPMENT.md updated to reference the Go suite. - errcheck lint fixes (deferred Close) across the e2e Go module. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lease_id rebase blocker Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Main removed the GatewayRelease kind and release_id/observed_release_id from the Gateway across all layers (#452): the Gateway image and supervisor_image fields make the release indirection redundant and canary rollout was never adopted. Reconcile the Go e2e suite with that schema: - drop the P2.5 release-promotion phase and its helpers (createRelease, pollReleaseStatus, patchGatewayRelease, pollObservedRelease) - stop discovering a seeded gateway release; discoverSeedIDs and perfSeedIDs resolve only the managed-cluster id - remove release_id from every gateway-create body - drop the GatewayReleases client references and ObservedReleaseID assertions; diagnostics log provisioning_conditions as-is Mirrors main's own e2e treatment, where the release-promotion area was dropped rather than re-expressed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The E2E Make targets invoked tests/e2e as a package in the repository's root module, so both Kind and OpenShift CI failed before the suite began. Run the Go test command from the E2E module and document that entry point. Assisted-by: GPT-5
The Go E2E suite still sent the removed cluster_id create field, while the API requires a placement intent. Centralize the driver-specific payload so Kind uses local-kind and OpenShift requests public AWS placement. Add a payload regression test and use the helper for functional, RBAC, validation, and performance gateway creation paths. Assisted-by: GPT-5
Generate array and object fields from OpenAPI so Gateway GET responses with provisioning_conditions decode correctly. This lets e2e observe the controller's Running state. Seed Kind and OpenShift gateways with placement intents, and cover both contracts with regressions. Assisted-by: GPT-5
Stream concurrent E2E suites with stable prefixes while preserving each suite log. Upload the retained logs when CI fails so failing phases are visible without rerunning the environment. Assisted-by: GPT-5
The OpenShift driver always used a password grant for per-gateway tokens, even when the CI workflow selected client credentials. Brokered PR users do not have password credentials, so P1.3 timed out despite correct role assignment. Use the e2e service account token-exchange flow and cover it with a focused regression test. Assisted-by: GPT-5
Run independent P2 steps concurrently and overlap setup, Kubernetes reads, periodic GC, and cleanup. The suite keeps serial gates intact and writes per-phase timings to CI summaries so performance gains remain visible. Assisted-by: GPT-5
Manage the controller-wide namespace GC override once for a long-mode matrix run. Parallel suites now share that lease, so no suite restores the controller configuration while another still relies on it. Assisted-by: GPT-5
Rely on Go's native verbose subtest output instead of maintaining a second per-phase timing table and GitHub summary hook. Assisted-by: GPT-5
Restore and populate the E2E module cache before Kind or OpenShift functional tests begin, keeping dependency downloads out of the live cluster test window. Assisted-by: GPT-5
Stop forcing the stale native openshell binary in CI. The functional suite now selects the CLI image built for the deployed gateway, avoiding sandbox protobuf wire-format skew. Assisted-by: GPT-5
Assisted-by: GPT-5
Assisted-by: GPT-5
Add functional coverage for provisioning, RBAC, service accounts, managed-cluster identity, device authorization, and infrastructure readiness. Keep controller-sensitive checks dependency-aware while parallelizing sandbox operations and independent phase work to reduce wall-clock time.
48f8bae to
c0b3147
Compare
This branch has not been deployed
1 participant
Summary
Replaces the Bash functional e2e suite with the Go/testify/client-go harness in
tests/e2e/. The Go suite is now the functional gate in the Kind and OpenShiftjobs; retained Bash assets support the browser-console suite and the standalone
smoke walkthrough.
and benchmark runners, and CLI coverage.
E2E_CONCURRENCY, overlapsnon-dependent setup and verification work, and emits per-phase CI timings.
client_credentialsCI runs.GatewayRelease, GatewayNetwork,
release_id, or release-promotion coverage.Generated SDK breaking changes
This PR intentionally changes generated artifacts; they are not byte-identical
to
main. The SDK generator, Go SDK, and TypeScript SDK were regenerated torepresent Gateway provisioning conditions and DNS names as structured values and
role permissions as a map. The public Go builders consequently change from string
inputs to
[]stringandmap[string]anyinputs.This is a breaking public SDK surface change and must be reviewed as such. It is
kept here because the Go e2e harness consumes the structured gateway response;
reviewers should split it into a follow-up only if they prefer to land the harness
and SDK contract change separately.
Cross-PR coordination
PR #379 changes the
management API audience to
hypershell-api. After both PRs land, the Go harness'smanagement API tokens (from the frontend,
hypershell-e2e, andhypershell-control-planeclients) must carry that audience. Gateway tokenexchange in this PR deliberately targets the per-gateway client instead and must
not acquire the management API audience.
A maintainer must choose the merge order and the durable owner for gateway
token-exchange/audience-isolation coverage. Until that decision, retain #379's
legacy harness coverage and re-run the Go functional suite against the new realm
and API audience configuration.
Verification
tests pass locally.
(61.4%).
Follow-ups
configuration before merging both changes.
e2e-testing.spec.mdreconciliation table in a separate pass.