Repository navigation
docs(e2e): add areas 12-14 and extended qualification scenarios - #413
Conversation
…n gaps Close identified end-to-end coverage gaps in the e2e specs. e2e-testing.spec.md (renumbered core suite to 14 areas): - Area 12: ManagedCluster self-registration (idempotency, last_seen_at, 403 missing-registrar, 409 collision, 400 empty/unregistered cluster_id) and an opt-in second cluster (E2E_MULTICLUSTER) for cross-cluster placement. - Area 13: gateway release promotion (observed_release_id, Degraded-and- recover, cross-cluster promotion). - Area 14: admin-only /v1/users boundary and unknown-phase rejection. - Reconciled-status assertions (release Available/Invalid, network Valid/Invalid, deployment image == release image) and complete-teardown (CRB, Keycloak clients, per-gateway DB+role drop, no IncompleteFinalization). - Extended Platform Qualification section for HYPERSHELL-291: opt-in DNS/TLS renewal, DB credential rotation, DB backup/restore, restricted-registry, and Sippy reporting. ROKS route-mode (244) is intentionally not specced and points to the new infra epic HYPERSHELL-363; AWS Gateway API ingress (243) and service-account auth (246) are noted as already covered. - Mode table, area-count references, and env vars updated accordingly. e2e-console-browser-testing.spec.md: - CON-E2E-13 fleet-dashboard /api/promotion map (skipped when not deployed). - CON-E2E-14 cluster typeahead lists registered clusters and the install-docs link. managed-cluster-registration.spec.md and gateway-release-rollout.spec.md gain E2E Coverage requirements tying their behavior to areas 12 and 13. Assisted-by: Claude Opus 4.8 (1M context)
|
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 |
Amber reviewStatus: Complete |
![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.
…rage Add area-12 scenarios for HYPERSHELL-241: a control-plane watch disconnect -> desired-state-change -> reconnect -> convergence check (new gateway reaches Running, deleted gateway's namespace is reaped, no stale resources), and rejection of unauthorized/revoked gRPC identities (UNAUTHENTICATED, PERMISSION_DENIED for a wrong cluster_id, INVALID_ARGUMENT for a missing filter). HYPERSHELL-242 is already covered by the existing suite and needs no spec change. Assisted-by: Claude Opus 4.8 (1M context)
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Review feedback on #413: the "Manually created records" prose said "gateways assigned to it are never reconciled", implying such a gateway create is accepted but idle. That contradicted this spec's own "Gateways Reference a Registered Cluster" requirement, which rejects a create (or cluster_id PATCH) referencing an empty-oidc_subject record with 400. Clarify the prose to match the requirement: the placeholder cannot be assigned a gateway at all, so the e2e area-12 scenario expecting a 400 is correct. Assisted-by: Claude Opus 4.8 (1M context)
There was a problem hiding this comment.
Verdict
COMMENT - This spec-only PR cleanly extends the e2e coverage document to 14 areas (ManagedCluster registration + multi-cluster, control-plane reconnect/identity, release promotion, admin inventory + API validation) plus the extended qualification scenarios and two browser requirements, with no new defects in its own content. The remaining open items are cross-PR coordination rather than problems in this PR, and the single prior Major finding is now both retracted and independently addressed by the head commit.
Summary
The head commit (027cf1d) adds one line change to specs/platform/managed-cluster-registration.spec.md that rewrites the "Manually created records" prose (L72) so it no longer reads as "accepted but never reconciled" and instead points at the authoritative "Gateways Reference a Registered Cluster" rule (L240-L255: empty or unregistered cluster_id is rejected with 400). This resolves the only prior finding; the rest of the PR (area-12/13/14 requirements, reconnect/identity coverage with UNAUTHENTICATED/PERMISSION_DENIED/INVALID_ARGUMENT matching the registration spec's Watch Stream Caller Binding at L262, the 11->14 count updates across the coverage list, full-suite scenario, short/perf/long table, and performance-harness requirements, and CON-E2E-13/14) is internally consistent, ASCII-only, and free of em dashes.
Findings
No new findings in this diff. The content is spec-only and the cross-references resolve.
Previous concerns
- [Addressed] [Major] Area-12 "Gateway create rejects an unregistered cluster" allegedly contradicted
managed-cluster-registration.spec.md- prior review #413 (review) and inline #413 (comment). This was already retracted on re-review (the authoritative rule atspecs/platform/managed-cluster-registration.spec.mdL242 and the "Unregistered cluster rejected" scenario at L253-L255 do mandate the create-time 400, so the e2e scenario is correct). The head commit027cf1dadditionally removes the ambiguity that triggered the finding: L72 now reads "a gateway cannot be assigned to one: a gateway create (or aPATCHthat setscluster_id) referencing a record with an emptyoidc_subjectis rejected with 400 (see ...)" instead of the earlier "gateways assigned to it are never reconciled." Prose and requirement are now consistent; no further action needed.
Cross-PR coordination
-
#412 (provision gateways by placement intent, HYPERSHELL-361) - this is a declared breaking change that replaces the client-supplied
cluster_idon gateway create with aplacementintent: it deletes thecluster_idrequired field and property fromGatewayCreateRequest, rewrites the create 400 contract text from "acluster_idthat is empty or does not reference a ManagedCluster with a registered control plane" to "an invalid or unavailable placement intent," removes thecluster_id is requiredUI validation, removes the clustervariant="typeahead"control from the gateway create form, and demotes server-sidecluster_idto a deprecated/ignored field. That directly competes with interface and UI behavior this PR codifies as new e2e requirements: area 12's "Gateway create rejects an unregistered cluster" scenario asserts a client-suppliedcluster_idand a create-time 400 namingcluster_id, Multi-Cluster Fleet Coverage creates gateways by the second cluster'scluster_id, and CON-E2E-14 asserts a/gateways/newcluster typeahead listing every registeredManagedCluster. If #412 lands, those assertions describe a contract and a form control that no longer exist. Maintainers and both owners need to decide which PR is the source of truth for the gateway-placement interface, whether these area-12/13 and CON-E2E-14 assertions should be rewritten againstplacementintent (provider/visibility) rather thancluster_idand a cluster typeahead, and the merge order. -
#200 (define control plane reconciliation contract) - edits the deletion section of
specs/platform/e2e-testing.spec.mdto make delete completion finalization-driven: it rewrites the "Gateway record removed after delete" scenario to "removed after finalization" (404 follows the control plane removing its finalizer, the deleting resource stays durable and authoritative, and watch-delete events only schedule work). This PR's new "Reconnect converges on the current desired state" scenario instead proves convergence by the deleted gateway's "managed namespace is reaped" and frames delete cleanup as "driven by the delete snapshot." These are two different mental models of what proves delete completion, codified in the same spec file. Maintainers should decide the merge order and reconcile the reconnect scenario's convergence proof with #200's finalization-complete definition soe2e-testing.spec.mdcarries one consistent deletion/convergence contract.
Findings Summary (ordered by severity, highest first):
No open findings. The previous Major finding is addressed (see Previous concerns).
Convention Checklist
| Convention | Result |
|---|---|
| No em dashes (U+2014) introduced | Pass |
| No non-ASCII introduced | Pass |
| Internal/external spec cross-references resolve | Pass |
| Area count updated consistently (11 -> 14) | Pass |
| New requirements consistent with referenced specs | Pass |
…hift-online#413) (openshift-online#417) Close the e2e coverage gaps openshift-online#413 opened by renumbering the core suite to 14 areas. The underlying API/control-plane/UI features already existed; only the e2e scenarios were missing. e2e-openshell.sh (new long-only areas 12-14): - Area 12: ManagedCluster self-registration (idempotency, missing-role 403, name-collision 409, unregistered-cluster 400), the gRPC watch identity boundary (UNAUTHENTICATED / PERMISSION_DENIED / INVALID_ARGUMENT via grpcurl), and reconnect convergence. - Area 13: gateway release promotion (observed_release_id advances to the serving release; a failed rollout never moves off the last-good release) and reconciled release/network status assertions. - Area 14: admin-only /v1/users boundary and unknown-phase rejection. e2e-console.sh: cluster typeahead + CLI install-docs link (area 7) and fleet-dashboard promotion (area 11, skip-gated on E2E_FLEET_DASHBOARD_URL). Support: shared helpers in lib.sh (e2e_json_field moved here from browser-lib.sh, e2e_poll_resource_status, status predicates, registrar / multi-cluster / qualification env defaults) and acquire_registrar_token in drivers/kind.sh. Fixed a set -e abort where a bare VAR=$(e2e_wait_gateway_running|e2e_poll_resource_status) assignment killed the whole run on timeout instead of recording a failure. Verified on a local Kind cluster (long run 90 passed; short 20/0; console unit test 56/0; all shell unit tests pass). Two behaviours diverge from the spec and are recorded in RECONCILE.md rather than forced: - Gateway DELETE performs service-account cleanup through the control plane and returns 503 while it is down, so "delete while disconnected" is infeasible; area 12 covers create-convergence instead. - A failed rollout keeps the last-good workload serving and stays Running (not Degraded); area 13 asserts the never-moves-to-bad safety invariant. Multi-cluster (E2E_MULTICLUSTER) and extended qualification stay deferred behind their flags, pending infrastructure. Assisted-by: Claude Opus 4.8
1 participant
What
Expand e2e test coverage specification from 11 to 14 areas, closing gaps in managed-cluster registration, multi-cluster fleet placement, release promotion, and admin API boundaries.
Highlights
/v1/usersboundary and reject unknown gateway phasesE2E_MULTICLUSTER, registrar credentials, and qualification flags to control test executionScope
Specification only: documents requirements for three new test areas and extended qualification paths. Implementation of the test code itself is out of scope.