CONSOLE-5196: Refresh admin auth mid-run to prevent session expiry cascade - #16877
CONSOLE-5196: Refresh admin auth mid-run to prevent session expiry cascade#16877shahsahil264 wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@shahsahil264: This pull request references CONSOLE-5196 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
WalkthroughThe Playwright configuration adds shared authentication setup settings and generates chained ChangesPlaywright authentication project chaining
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: shahsahil264 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/pipeline required |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
Scheduling tests matching the |
|
@rhamilto Can you PTAL! |
shahsahil264
changed the title
|
/test e2e-playwright |
9c99950 to
0b83650
Compare
|
/pipeline required |
|
Scheduling tests matching the |
|
/retest |
1 similar comment
|
/retest |
|
/pipeline required |
|
Scheduling tests matching the |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
fsgreco
left a comment
fsgreco
left a comment
There was a problem hiding this comment.
I think that the new admin-auth-refresh.setup.ts file is unnecessary; the refresh projects can point testMatch at admin-auth.setup.ts
…expiry Auth storageState is created once at test start. On CI with 300+ tests and 2 workers (~50 min runtime), the OAuth session can expire mid-run, causing every subsequent test to fail with an OAuth redirect in warmupSPA. Chain auth refresh projects between every package so each package starts with a fresh token: admin-auth → smoke → refresh-1 → console → refresh-2 → dev-console → ... Each refresh project reuses admin-auth.setup.ts (no new setup file needed). Every package gets fresh auth regardless of how many tests any individual package has. Scales automatically as packages are added. Overhead is ~35s total (7 refreshes × ~5s each). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
a940197 to
3b950fb
Compare
|
/pipeline required |
|
Scheduling tests matching the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/playwright.config.ts`:
- Around line 137-161: The package projects in the packages flatMap currently
form a sequential dependency chain through admin-auth-refresh-${i} and prevDep,
so an earlier failure skips later coverage. Remove inter-package dependencies
while preserving each package’s required auth setup, configure refresh projects
to run independently, and ensure failures are aggregated rather than blocking
subsequent packages. Add a CI test case with a failing test in a non-final
package and verify later package projects still execute.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c9e56c3d-226e-40ce-85f5-fa317b5897b3
📒 Files selected for processing (1)
frontend/playwright.config.ts
| ...packages.flatMap((pkg, i) => { | ||
| const prevDep = i === 0 ? 'admin-auth' : packages[i - 1]; | ||
| const refreshProject = | ||
| i > 0 | ||
| ? [ | ||
| { | ||
| name: `admin-auth-refresh-${i}`, | ||
| testDir: setupDir, | ||
| testMatch: 'admin-auth.setup.ts', | ||
| dependencies: [prevDep], | ||
| use: { ...chromeAuth, launchOptions: { args: chromeArgs } }, | ||
| }, | ||
| ] | ||
| : []; | ||
| return [ | ||
| ...refreshProject, | ||
| { | ||
| name: pkg, | ||
| testDir: path.resolve(__dirname, 'e2e', 'tests', pkg), | ||
| testIgnore: '**/developer/**', | ||
| dependencies: [i > 0 ? `admin-auth-refresh-${i}` : 'admin-auth'], | ||
| use: { ...chrome, storageState: adminStorageState }, | ||
| }, | ||
| ]; | ||
| }), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
printf '%s\n' '--- config outline ---'
ast-grep outline frontend/playwright.config.ts
printf '%s\n' '--- config lines ---'
sed -n '100,180p' frontend/playwright.config.ts
printf '%s\n' '--- related references ---'
rg -n --glob '!node_modules' 'admin-auth-refresh|admin-auth\.setup|playwright' frontend package.json .github 2>/dev/null | head -200Repository: openshift/console
Length of output: 16744
🏁 Script executed:
printf '%s\n' '--- complete relevant config ---'
sed -n '1,175p' frontend/playwright.config.ts
printf '%s\n' '--- setup implementations ---'
sed -n '1,220p' frontend/e2e/setup/admin-auth.setup.ts
sed -n '1,180p' frontend/e2e/setup/login-helper.ts
printf '%s\n' '--- package and CI invocation context ---'
sed -n '1,70p' frontend/package.json
sed -n '150,190p' frontend/integration-tests/test-playwright-e2e.sh
rg -n --glob '!node_modules' 'test-playwright-admin|test-playwright-e2e|--max-failures|workers|fullyParallel|forbidOnly' .github frontend/config frontend 2>/dev/null | head -200Repository: openshift/console
Length of output: 14647
🌐 Web query:
Playwright test projects dependencies dependent project runs when dependency test fails official documentation
💡 Result:
In Playwright, if a project dependency fails, the dependent projects are automatically skipped [1][2][3]. According to official Playwright documentation, dependencies are projects that must run and pass successfully before the tests in a dependent project can begin [1][2]. If any test within the dependency project fails, the dependent project will not be executed [1]. This behavior is designed to prevent tests from running in an environment that has not been correctly set up [2]. Currently, there is no native configuration option to allow dependent projects to proceed if a dependency fails [4][3]. Users have requested this functionality as a feature [4][3], but as of July 2026, the standard behavior remains that dependency failure results in the skipping of all dependent tests [1][2].
Citations:
- 1: https://playwright.dev/docs/test-projects
- 2: https://runebook.dev/en/docs/playwright/api/class-testproject/test-project-dependencies
- 3: https://stackoverflow.com/questions/77036978/allow-dependent-projects-to-run-even-if-the-first-project-fails-in-playwright-no
- 4: [Feature] Allow dependent Projects to Run even if the First Project Fails. microsoft/playwright#26854
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
import re
text = Path("frontend/playwright.config.ts").read_text()
packages = re.search(
r"const packages = \[(.*?)\];", text, re.S
).group(1)
packages = re.findall(r"'([^']+)'", packages)
deps = {}
for i, package in enumerate(packages):
refresh = f"admin-auth-refresh-{i}"
if i > 0:
deps[refresh] = [packages[i - 1]]
deps[package] = [refresh if i > 0 else "admin-auth"]
# A dependency failure skips every transitive dependent project.
failed = {"console"} # representative non-final package
skipped = set()
changed = True
while changed:
changed = False
for project, project_deps in deps.items():
if project not in skipped and any(
dep in failed or dep in skipped for dep in project_deps
):
skipped.add(project)
changed = True
print("package order:", packages)
print("dependencies:", deps)
print("failed project:", sorted(failed))
print("skipped projects:", sorted(skipped))
print("later packages skipped:", [
package for package in packages[packages.index("console") + 1:]
if package in skipped
])
PYRepository: openshift/console
Length of output: 1167
Decouple package projects from the package dependency chain. admin-auth-refresh-${i} depends on the preceding package, and each later package depends on that refresh project. If a non-final package fails, Playwright skips its refresh project and all later packages. Run each package and refresh independently, then aggregate failures. Add a CI case with one failing test in a non-final package to ensure later package coverage runs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/playwright.config.ts` around lines 137 - 161, The package projects
in the packages flatMap currently form a sequential dependency chain through
admin-auth-refresh-${i} and prevDep, so an earlier failure skips later coverage.
Remove inter-package dependencies while preserving each package’s required auth
setup, configure refresh projects to run independently, and ensure failures are
aggregated rather than blocking subsequent packages. Add a CI test case with a
failing test in a non-final package and verify later package projects still
execute.
| ...packages.flatMap((pkg, i) => { | ||
| const prevDep = i === 0 ? 'admin-auth' : packages[i - 1]; | ||
| const refreshProject = | ||
| i > 0 | ||
| ? [ | ||
| { | ||
| name: `admin-auth-refresh-${i}`, | ||
| testDir: setupDir, | ||
| testMatch: 'admin-auth.setup.ts', | ||
| dependencies: [prevDep], | ||
| use: { ...chromeAuth, launchOptions: { args: chromeArgs } }, | ||
| }, | ||
| ] | ||
| : []; | ||
| return [ | ||
| ...refreshProject, | ||
| { | ||
| name: pkg, | ||
| testDir: path.resolve(__dirname, 'e2e', 'tests', pkg), | ||
| testIgnore: '**/developer/**', | ||
| dependencies: [i > 0 ? `admin-auth-refresh-${i}` : 'admin-auth'], | ||
| use: { ...chrome, storageState: adminStorageState }, | ||
| }, | ||
| ]; | ||
| }), |
There was a problem hiding this comment.
There is something that is not convincing me here:
The logic depends on the packages array's order and position of its items, but nothing in the code makes this kind of constraint visible.
Anyone in the future can add, remove, or reorder packages without thinking about which one is "first", and this flatMap is creating a dependency chain where the first element smoke is treated specially.
I'm not completely sure that we should create this coupling (meaning making the order of packages so crucial).
Having said that, - in case we agree on this strict serial chain - the real issue for now is that we have only one special case: smoke, so my question is shoudn't we trade off this and just refresh every package uniformly as it already is? The tradeoff would be some seconds of refresh time for smoke package, but we could gain redability in case we want to change things in future (and of course we should point out that order in packages will matter from now on).
| ...packages.flatMap((pkg, i) => { | |
| const prevDep = i === 0 ? 'admin-auth' : packages[i - 1]; | |
| const refreshProject = | |
| i > 0 | |
| ? [ | |
| { | |
| name: `admin-auth-refresh-${i}`, | |
| testDir: setupDir, | |
| testMatch: 'admin-auth.setup.ts', | |
| dependencies: [prevDep], | |
| use: { ...chromeAuth, launchOptions: { args: chromeArgs } }, | |
| }, | |
| ] | |
| : []; | |
| return [ | |
| ...refreshProject, | |
| { | |
| name: pkg, | |
| testDir: path.resolve(__dirname, 'e2e', 'tests', pkg), | |
| testIgnore: '**/developer/**', | |
| dependencies: [i > 0 ? `admin-auth-refresh-${i}` : 'admin-auth'], | |
| use: { ...chrome, storageState: adminStorageState }, | |
| }, | |
| ]; | |
| }), | |
| ...packages.map((_, i) => ({ | |
| name: `admin-auth-refresh-${i}`, | |
| testDir: setupDir, | |
| testMatch: 'admin-auth.setup.ts', | |
| dependencies: [i === 0 ? 'admin-auth' : packages[i - 1]], | |
| use: { ...chromeAuth, launchOptions: { args: chromeArgs } }, | |
| })), | |
| ...packages.map((pkg, i) => ({ | |
| name: pkg, | |
| testDir: path.resolve(__dirname, 'e2e', 'tests', pkg), | |
| testIgnore: '**/developer/**', | |
| dependencies: [`admin-auth-refresh-${i}`], | |
| use: { ...chrome, storageState: adminStorageState }, | |
| })), |
|
@shahsahil264: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-ci
Bot
added
the
needs-rebase
5 participants
Analysis / Root cause:
Playwright CI runs all ~300 e2e tests using a single
storageState(saved OAuth session cookies) created once at the start of the run. The OpenShift OAuth session has a fixed TTL, and with 300+ tests running across 2 workers (~50 min total runtime), the session can expire mid-run — typically around test ~220 when dev-console tests begin.When the session expires:
warmupSPAnavigates to/but the console backend rejects the stale cookieoauth-openshift.../oauth/authorizethenauth/callbackpage-headingpage-headingis never found so the test failsstorageStatefile on disk is never updated, so every subsequent test loads the same stale cookies causing a cascade failure (10-20+ tests fail in a row)This has caused repeated CI failures across multiple migration PRs (#16741, #16751), where all dev-console, topology, and webterminal tests fail with identical
warmupSPAOAuth redirect errors — despite the test code being correct (proven by passing on runs where auth holds).Solution description:
Chain an auth refresh project between every test package so each package starts with a fresh OAuth session:
This serializes packages (each depends on the previous refresh) and inserts a lightweight
admin-auth-refresh-Nsetup project before every package after the first. Each refresh re-runs the login flow and overwriteskubeadmin.jsonwith a fresh session. Overhead is ~35s total (7 refreshes × ~5s each).Changes:
playwright.config.ts: Replace flatpackages.map()withpackages.flatMap()that generates interleaved refresh projects. Extract sharedsetupDirandchromeAuthconfig to reduce duplication.e2e/setup/admin-auth-refresh.setup.ts: New setup file that performs the same login asadmin-auth.setup.tsto refresh thestorageState. SupportsSKIP_GLOBAL_SETUP, configurable credentials viaOPENSHIFT_USERNAME/BRIDGE_KUBEADMIN_PASSWORD, and usesimport.meta.dirname.Screenshots / screen recording:
N/A (CI infrastructure change, no UI changes)
Test setup:
Tested locally against a live OCP 5.0 cluster. Verified:
admin-authcreates initialkubeadmin.jsonadmin-auth-refresh-Nre-logs in and overwriteskubeadmin.jsondev-consoletests run successfully with the refreshed session (10/10 passed, zero auth redirect failures)Test cases:
Browser conformance:
N/A (no UI changes)
Additional info:
This complements PR #16780 which reduced runtime by setting
WORKERS=2. That fix brought runtime from ~2h to ~50min, but session expiry still occurs non-deterministically depending on the ephemeral cluster OAuth token lifetime. This PR eliminates the root cause by refreshing auth before every package, ensuring no single package runs with a stale session regardless of total suite runtime.Summary by CodeRabbit