[RORDEV-2219] Await SessionManagerProvider readiness before PKP/PEP start accepting requests

[Dzuming]

• Synchronize tests between readonlyrest_kbn and readolnyrest-e2e-tests repo
• Fix problems with e2e tests and dynamic ROR settings when multiple Kibana instances are configured

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 73dc6ccf-6bdb-4ac4-9f4c-22191c816bee

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[10hexdev]

No action needed — that was the dependency install for the typecheck, already consumed (exit 0). The review is posted and verified: #119 (review) (1 review by 10hexdev[bot], 4 inline comments on the head SHA 750f769, worktree removed, memory updated).

[10hexdev]

Verdict — one gap to fix, no blocker.

The fixtures and the harness changes are correct as far as I could test them, and the whole 8-leg matrix is green at 750f769 (ELK 7.17.29 / 8.19.21 / 9.4.7 / 9.5.4 x docker and eck-3.5.0, "All specs passed", no retried test in the two jobs I read). The one gap is in the new sample-data retry: it accepts a response that is not an install.

Verified as sound

• The ECK rules reach every reset path. The four rules carry ${...} placeholders; the eck env defines those variables on the ES container (environments/eck-ror/kind-cluster/ror/base/es.yml:23-37) and the docker env defines them as UNUSED on purpose (environments/elk-ror/base.docker-compose.yml:23-26). Both legs re-post a fixture many times per run through /api/ror/settings?override=true, and stay up, so no fixture swap locks Kibana, the internal probe or the elastic-internal user out of ES.
• The 8 s propagation wait fires where its comment says. e2e-tests/run-tests.sh:53 passes envName=elk-ror for the docker leg, environments/elk-ror/base.docker-compose.yml:53-55 runs two replicas, and environments/eck-ror/kind-cluster/ror/base/kbn.yml:8 keeps eck at count: 1.
• Reporting.pagePath has one home. grep -rn insightsAndAlerting e2e-tests/cypress returns the two literals in Reporting.ts and one unrelated Hide_apps string; the two old copies in Sanity-check.cy.ts and Reporting.ts are gone.
• The new teardown diagnostics produce output. The eck job log of this PR ends with the pod table and the warning events sorted by timestamp (run 35494668169, job 106036011471).
• No dependency change. package.json and yarn.lock are untouched, and cypress-recurse@1.37.2 was already a dependency. I read its src/index.js to check the two new recursions.
• Not covered by this run: a base branch other than master selects dev images only, so prod-e2e-tests is skipped. Nothing here has been run against a released plugin image.

Inline findings

1. e2e-tests/cypress/support/helpers/KbnApiClient.ts:90 — the sample-data retry accepts Kibana's login page as a successful install. Fix or decide.
2. e2e-tests/cypress/support/page-objects/Settings.ts:143 — the page-unloading restore helper is used at 2 of the 8 places that post this fixture. Decide.
3. e2e-tests/cypress/support/page-objects/Settings.ts:82 — nit: limit: 3 makes the 20 s timeout unreachable.
4. e2e-tests/cypress/e2e/Impersonate.cy.ts:83 — nit: these rows have been renumbered three times on this branch.

[10hexdev]

nit: these rows are addressed by position, and this branch has already renumbered them three times.

af0c442 ("Fix user indices in impersonation e2e test"), 116a7ac ("The apm user is now in the impersonation list, so the kibana and new_user rows move down by one") and this PR's 1435e4c all edit the numbers on this line and on line 105. The rows come from the local users of the active test ACL, and this PR extends testSettings.yaml with PROBE and ELASTIC-INTERNAL — two more auth_key usernames that sort before kibana, which is what moves row 0 to row 2.

The next edit to that fixture moves them again, and the failure then reads as a missing row, not as a missing user. Impersonate.assertUser already takes the username, so the row index adds nothing: look the row up by its text (the account name is unique in that table) and drop the index argument.

[Dzuming]

Done

[sscarduzio]

Partly fixed. assertUser now finds the row by username (Impersonate.ts:53), and all six callers use the text form. The same positional lookup remains in impersonateUserFromTheList: Impersonate.cy.ts:105 and :111 still pass a literal row index. The next edit to the local-user list moves those rows again. verifyImpersonation reports the mismatch, so the failure is visible, but the index still adds nothing. Decide: drop the rowIndex argument there too, or say why it stays.

[10hexdev]

Head update, no new ask. At deeff102 the refactor reached only one of the two twins: assertUser now looks the row up by name (Impersonate.ts:53), while impersonateUserFromTheList still takes a row index (Impersonate.ts:144, find('[class=euiTableRow]').eq(rowIndex) at line 147) with callers passing literals at Impersonate.cy.ts:105 and :111. This PR had to edit the literal at line 105 from 2 to 3 for the same user. Still open: drop the rowIndex argument there too, or say why it stays.

[sscarduzio]

@/tmp/opencode/f1.md

[Dzuming]

The comment is broken here

[10hexdev]

This comment's body was lost to a tooling fault (it was published as @/tmp/opencode/f1.md). I re-derived it against deeff102 and it stands; here it is.

The per-attempt cap aborts the whole call, so the retry cannot cover the slow insert that the docstring names as the reason for the cap.

KbnApiClient.ts:92   timeout: 90000        <- the retry budget
KbnApiClient.ts:88   timeoutMs: 30000      <- the per-attempt cap
commands.ts:168      cy.task('httpCall', options, { timeout: timeoutMs })

cypress-recurse 1.37.2 runs the predicate only on a fulfilled chain (node_modules/cypress-recurse/src/index.js:197-210: return result.then(cypressRecurse), with no rejection handler). When cy.task reaches its cap, Cypress fails the command, the predicate never runs, and the recursion ends. The test fails with cy.task('httpCall') timed out after waiting 30000ms instead of the loop's own Timed out loading sample data "ecommerce" (line 95). The plugin adds no limit of its own: plugins/index.ts:86-91 passes neither timeout nor an AbortController to fetch.

Two numbers I checked, so that only the real gap stays open: cypress.config.ts:30 sets taskTimeout: 20000, so timeoutMs: 30000 is 10 s more room than the shared timeout - that half of the docstring holds. The response shape in the predicate is the shape CI prints (HTTP STATUS 200; Body: { elasticsearchIndicesCreated: { kibana_sample_data_ecommerce: 4675 }, ... }) - that half holds too. No occurrence of a timeout in either green job, so this is about the next slow run, not about this one.

One word is still wrong: line 72 says "retry with backoff", but delay: interval (line 93, interval = 5000) is constant, so every attempt is 5000 ms apart.

Decide the budget: raise timeoutMs to cover the insert you measured (the retry window is 90 s), or state in the docstring that the 30 s cap wins and a timeout ends the test. Then change "retry with backoff" to "retry at a fixed interval", which is what the code does.

[sscarduzio]

@/tmp/opencode/f3.md

[Dzuming]

The comment is broken here

[10hexdev]

This comment's body was lost to a tooling fault (it was published as @/tmp/opencode/f3.md). I re-derived it against deeff102; here is what I can confirm and the one decision I want.

The docker leg keeps no coverage of resetKibanaIndexToTemplate, and the test still needs its retry on the legs that do run it.

8.19.21 eck  job 106304703183, log line 5209: (Attempt 1 of 3) should verify kibanaIndexTemplate functionality
9.5.4  eck  job 106304703096, log line 4579: (Attempt 1 of 3) should verify kibanaIndexTemplate functionality
7.17.29 docker job 106304703250: Readonlyrest-settings.cy.ts  8 tests  7 pass  1 skip

I verified both halves of the comment in the plugin source, so the mechanism it describes is real: EnsuredIndexAndSpaceCache is an in-process Map keyed sid::kibanaIndex with a 2-minute TTL (proxy/preKibanaProxy/indexCreation/ensuredIndexAndSpaceCache.ts:20-37), and the login path calls createIndexAndSpace without consulting it (proxy/preKibanaProxy/authenticationFacade.ts:128). The fresh-session login at lines 68-70 is therefore the right shape.

Two consequences. On elk-ror the skip covers the whole body, so that workaround never runs on the env it was written for. And on the two eck legs the test still failed its first attempt, so the fresh login does not make it deterministic on a single node either.

Decide: re-enable the test on elk-ror now that the session-id workaround exists, or state in this comment that the docker leg intentionally has no coverage of this feature - and say what the residual attempt-1 failure on the eck legs is.

[sscarduzio]

@/tmp/opencode/verdict.md

[10hexdev]

That notification is for the dependency install I ran earlier to type-check the head; it completed before the review and needed no action. Nothing further to do — the review stands as posted.

[10hexdev]

Verdict — no blocker. Three items want a decision or an answer, and three nits follow. All four findings from the previous round are settled.

I re-read the whole change against both environments and the sibling suite, and I ran the numbers that each
comment depends on. CI is green on all 8 legs at deeff102 (run 35590486667). The fixtures, the
settings-propagation wait, the page-object refactors and the ECK manifest tuning all check out. What is left
is one gap in the new sample-data retry, one mitigation that did not take effect, and one coverage decision.

Run stability at this head. 7 of the 8 legs needed at least one retry (10 retried test instances; only
9.4.7 / eck-ror passed with none): should check dev tools on 3 legs, should verify kibanaIndexTemplate functionality on the 2 eck legs that run it, should correctly display all reporting data on 2, and one
each for should verify that everything works, should check settings and the activation-key test. The
retry table that plugins/index.ts:287 writes goes to the job summary page, not to the job log, so the
retried attempts leave no trace in the downloaded log.

Verified as sound

• All 8 legs green at deeff102, and the elk-ror skip still fires: the docker leg reports
  Readonlyrest-settings.cy.ts 8 tests 7 pass 1 skip.
• Every ${...} in the fixtures resolves in both environments. Scripted check over the 10 fixtures:
  KIBANA_SERVICE_ACCOUNT_TOKEN, INTERNAL_PROBE_PASS and INTERNAL_USR_PASS are defined for the docker
  stack (environments/elk-ror/base.docker-compose.yml:24-26, deliberately UNUSED) and for the ECK ES pod
  (environments/eck-ror/kind-cluster/ror/base/es.yml:23-37, from secretKeyRef). No fixture can be posted
  with an unresolved placeholder.
• The fixture change matches the config the ECK env bootstraps. Commit 116a7ac says the default fixture
  now matches the initial ROR config; ror-initial-config.yml:8, :51, :70, :75 and
  environments/elk-ror/conf/es/readonlyrest.yml:6, :33, :51 carry the same hide_apps list and the same
  apm:test rule. So the removed Observability/Management entries and the new APM rule are intended, not
  an accident of the anchor edit.
• RorApiClient's 5 s poll claim is true. readonlyrest_kbn
proxy/core/rorSettings/RorSettingsManager.ts:50: SETTINGS_REFRESH_INTERVAL = 5000. The 8000 ms wait
  covers one interval with margin, and it stays off the single-node env.
• The per-session claim at Readonlyrest-settings.cy.ts:31-35 is true, in both halves.
  proxy/preKibanaProxy/indexCreation/ensuredIndexAndSpaceCache.ts:20-37 is an in-process Map keyed
  sid::kibanaIndex with a 2 * 60 * 1000 TTL, and the login path calls createIndexAndSpace without
  consulting it (proxy/preKibanaProxy/authenticationFacade.ts:128). Clearing cookies and signing in again
  is the right shape for a test that must re-trigger the reset.
• Reporting.pagePath is the single source of truth for that route (Reporting.ts:60,
  Sanity-check.cy.ts:64), the literal appears nowhere else, and each of its three branches is exercised by a
  green leg (8.19.21, 9.5.4, and 9.4.7/7.17.29).
• KbnApiClient's "more room per attempt" half holds. cypress.config.ts:30 sets
  taskTimeout: 20000, so timeoutMs: 30000 adds 10 s. The predicate's shape is the one CI prints:
  HTTP STATUS 200; Body: { elasticsearchIndicesCreated: { kibana_sample_data_ecommerce: 4675 }, ... }.
• The Save-modal retry is correctly sized. With delay: 300 and timeout: 20000, cypress-recurse derives
  limit = 68, and the hasOverlay guard is reachable (EUI adds euiBody-hasOverlayMask to the document body
  of the settings iframe). The "single 20 s wait" it replaces is defaultCommandTimeout: 20000.
• The new teardown diagnostics run and print. runner.sh calls stop-and-clean.sh from its EXIT trap, and
  the ECK job log contains == final pod status before teardown == followed by the pod and event tables.
• The ECK manifest fields are honoured. The operator keeps a readinessProbe a template already provides,
  and publishNotReadyAddresses sits on the NodePort service the tests reach (nodePort 30011).

Inline findings

• KbnApiClient.ts:88 — decide: the per-attempt cap aborts the call, so the retry cannot cover the slow insert
  the docstring names. Needs a decision. (Posted as the reply in the thread there; its body was lost.)
• DevTools.ts:34 — the force click did not stop the flake and removed the error that named the cause.
  Needs an answer.
• Readonlyrest-settings.cy.ts:36 — decide: the docker leg keeps no coverage of
  resetKibanaIndexToTemplate. Needs a decision. (Posted as the reply in the thread there; its body was
  lost.)
• KbnApiClient.ts:78, Impersonate.ts:50, RorApiClient.ts:25 — nits, all three anchored on lines this PR
  added.

[10hexdev]

{ force: true } did not remove the flake it was added for, and it removed the error that named the cause.

run 35590486667  job 106304703132 (9.5.4, docker)   log line 1574: (Attempt 1 of 3) should check dev tools
run 35590486667  job 106304703173 (8.19.21, docker) log line 1513: (Attempt 1 of 3) should check dev tools
run 35590486667  job 106304703183 (8.19.21, eck)    log line 2025: (Attempt 1 of 3) should check dev tools

The spec passes on attempt 2, so the interaction still fails its first attempt on 3 of the 8 legs. DevTools.sendRequest runs in all five steps of Dev-tools.cy.ts:12-43, so the failure is in this path. With the actionability check disabled, the run keeps no element that covered the editor, and Cypress writes the retry table to the job summary page only, so the failing attempt leaves nothing to read in the job log.

The comment above says the cover is a false positive that "will never clear". The retries do not support that: something is still not settled when the click lands.

Decide and answer: name the condition that fails on those legs (run the two legs with retries: 1 and keep the artifacts), then wait for that condition - a bounded recurse on the toolbar, or a stronger guard than the textarea.inputarea check on line 31 - and keep force only where the cover is real.

[Dzuming]

Done

[10hexdev]

nit: timeout and interval are dead parameters. Both callers pass three arguments.

grep -rn "loadSampleData(" e2e-tests
RoAndRoStrictKibanaAccessAssertions.ts:19: kbnApiClient.loadSampleData('ecommerce', credentials, 'template_group');
Tenancy.cy.ts:88:                         kbnApiClient.loadSampleData('ecommerce', userCredentials, 'template_group');

(Home.loadSampleData is a different method.) Nothing tunes the 90 s window or the 5 s gap per call site, and a future caller that passes a small timeout silently shortens the retry budget.

Delete both parameters and put timeout: 90000 and delay: 5000 in the options object.

[10hexdev]

nit: hasImpersonateButton is dead, so the assertion at line 56 always checks exist.

Impersonate.cy.ts:58  assertUser(0, 'JohnDoe', ['group3'])
Impersonate.cy.ts:59  assertUser(0, 'RobertSmith', ['group3'])
Impersonate.cy.ts:67  assertUser(1, 'JaneDoe')
Impersonate.cy.ts:75  assertUser(2, 'JaimeRhynes', ['Customer'])
Impersonate.cy.ts:83  assertUser(3, 'kibana')
Impersonate.cy.ts:101 assertUser(2, 'kibana', ['group3'])

No caller passes a fourth argument, so false is unreachable. You removed rowIndex from this signature in f52dc097; this parameter followed the same path and stayed.

Delete it and assert .should('exist'). Add a separate method if a negative check is ever needed.

[Dzuming]

Done

[10hexdev]

nit: the env name is a string literal at three sites, and the enum that names that value is dead.

e2e-tests/cypress/support/types.ts:1  export enum EnvName { ECK_ROR = 'eck-ror', ELK_ROR = 'elk-ror' }
grep -rn "EnvName" e2e-tests        -> the declaration only
RorApiClient.ts:25 (added here)     Cypress.env().envName === 'elk-ror'
Readonlyrest-settings.cy.ts:36      Cypress.env().envName === 'elk-ror'
Activation-keys.cy.ts:18            Cypress.env().envName === 'elk-ror'

This PR adds the second of the three. A renamed value then misses one literal: the 8000 ms propagation wait does not run and CI reports an unexplained login bounce on the multi-node env only.

Decide: delete EnvName, or add one predicate (for example isMultiKibanaNodeEnv() in support/helpers) and call it from all three sites.

[Dzuming]

Done